Skip to content

fix(routes): accept relative JAX-RS @Path templates and compose Scala class-level @Path (#2147) - #2158

Open
htarnacki wants to merge 5 commits into
DeusData:mainfrom
htarnacki:fix/jaxrs-relative-path
Open

htarnacki wants to merge 5 commits into
DeusData:mainfrom
htarnacki:fix/jaxrs-relative-path

Conversation

@htarnacki

Copy link
Copy Markdown

What does this PR do?

Fixes #2147.

JAX-RS allows @Path templates without a leading slash (@Path("widgets"), @Path("{id}")); the route extractor rejected them, so such resources produced no Route nodes and no HANDLES edges. Two commits:

  1. fix(routes): accept relative JAX-RS @Path templates — accept relative templates on class and method level and normalize the composed path so it is always rooted (/widgets/{id}). Existing tests updated to use the relative spelling for the JAX-RS cases, a rooted case is kept.
  2. fix(routes): compose class-level @Path for Scala JAX-RS resources — the class-level prefix was joined with the method path for Java and Kotlin but not for Scala; Scala now composes the same way. New pipeline test.

No behaviour change for Spring / other frameworks: rooted templates compose exactly as before (follow-up to #1005).

Checklist

  • Every commit is signed off (git commit -s) — required, CI rejects
    unsigned commits (DCO, see CONTRIBUTING.md)
  • Tests pass locally (make -f Makefile.cbm test) — all suites green except 10 test_cli.c install/uninstall cases that fail identically on a clean main in my environment (activation transaction I/O failed: target-entry-snapshot, environment-specific).
  • Lint passes (make -f Makefile.cbm lint-ci) — clang-format-20 and check-dco.sh clean; clang-tidy / cppcheck are not installed locally, relying on CI for those two.
  • New behavior is covered by a test (reproduce-first for bug fixes)

@htarnacki
htarnacki requested a review from DeusData as a code owner September 10, 2026 08:41
@github-actions

Copy link
Copy Markdown

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

@htarnacki htarnacki closed this Sep 10, 2026
@htarnacki htarnacki reopened this Sep 10, 2026
@htarnacki
htarnacki force-pushed the fix/jaxrs-relative-path branch from 5bb94c5 to b5f119c Compare September 10, 2026 09:21
@htarnacki

Copy link
Copy Markdown
Author

CI status: everything is green except test / test-windows-guards, which went red in tests/windows/test_daemon_stability.py::section_cold_storm ("cold-storm client 0 failed (racing daemon spawn): secure CLI coordination could not be created (endpoint)").

I believe that one is pre-existing / environmental rather than caused by this PR: the change is confined to JAX-RS @Path extraction in internal/cbm/extract_defs.c plus tests, and the same guard failed in the same way on recent unrelated PR runs (fix/decision-b-win-sid-trust, fix/1954-uninstall-symlinked-config, ci/windows-helper-spawn-timeout). Happy to rebase/re-push to re-run if you prefer.

@htarnacki
htarnacki force-pushed the fix/jaxrs-relative-path branch from b5f119c to 334e0b0 Compare September 14, 2026 08:37
@DeusData DeusData added bug Something isn't working parsing/quality Graph extraction bugs, false positives, missing edges priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Sep 19, 2026
@DeusData

Copy link
Copy Markdown
Owner

Thank you for the JAX-RS relative-path cases and Scala coverage. Current extraction rejects the relative forms at the identified path, so these regressions address a concrete gap. Please retain the rooted, non-JAX-RS negative control so the change cannot accidentally broaden unrelated route extraction. No empty commit or CI-only push is being requested.

JAX-RS allows @path values without a leading slash, both on the class
and on the method. The route extractor rejected such literals, so
resources such as `@Path("widgets")` + `@Path("{id}")` produced no Route
nodes and no HANDLES edges at all. Accept relative templates and
normalize the class/method composition so the resulting path is always
rooted.

Existing JAX-RS Java probe tests are updated to use relative templates
so they guard the regression.

Signed-off-by: Hubert Tarnacki <hubert.tarnacki@gmail.com>
Scala JAX-RS resources declare @path on the class exactly like Java and
Kotlin, but the class-level prefix was only joined with the method path
for those two languages. Include Scala so a method route is emitted as
`<class path>/<method path>` instead of the bare method template.

Adds a Scala probe test covering both an empty and a non-empty method
template.

Signed-off-by: Hubert Tarnacki <hubert.tarnacki@gmail.com>
@htarnacki
htarnacki force-pushed the fix/jaxrs-relative-path branch from 334e0b0 to d493124 Compare September 21, 2026 07:17
@htarnacki

Copy link
Copy Markdown
Author

Updated per your note — the branch base is unchanged, only the test coverage moved:

  • The original rooted fixtures (@Path("/api/v1/widgets") / @Path("/count")) in extract_java_jaxrs_path_composition_issue1005 and handles_jaxrs_java are restored unmodified.
  • Relative templates are now covered by separate tests: extract_java_jaxrs_relative_path_templates (class + method + {id}/tags template) and handles_jaxrs_java_relative_path.
  • Added a non-JAX-RS negative control in both suites (extract_java_spring_relative_string_not_route_path, handles_spring_java_relative_string_not_route): @RequestMapping("api") + @GetMapping("orders") still yields the / fallback and no /api/orders Route node.

Verified locally by swapping in main's extract_defs.c: rooted fixtures and both negative controls pass there, the relative variants fail there and pass with this change.

@DeusData

Copy link
Copy Markdown
Owner

Thank you — this is exactly what was asked for, and the way you verified it is the part I appreciate most: swapping in main's extract_defs.c and showing that the rooted fixtures and both negative controls pass there while the relative variants fail is a proper fails-without-the-fix proof, not just a green run.

Two things on our side:

The test / test-msan red is ours. It failed on daemon_bootstrap_fails_fast_when_daemon_dies_at_publication (tests/test_daemon_bootstrap.c:1116). We attributed that test yesterday: before it listens, the daemon hashes its own ~450 MB test image, which takes about 26 s under MSan against a 30 s client budget, so the verdict rides on a few seconds of runner speed. Nothing in a route-extraction change can influence it. Rebuilding that test so it no longer depends on timing is on our list.

I have updated your branch with main (it was 93 commits behind), which starts a fresh full CI run against today's tree. That is the verdict that counts; if the MSan leg trips on the same test again I will rerun it from here. Nothing further is needed from you for now — the next step is mine: reading the new tests against the restored originals and building the merge result locally.

@DeusData

Copy link
Copy Markdown
Owner

The local half is done, on the merge result of your head 61cfb09b with today's main:

check result
build (-Werror, ASan + UBSan) clean
extraction 371 passed
edge_types_probe 62 passed
pipeline / lang_contract 281 / 41 passed
memory-core linter no growth
production reverted, your tests kept extract_java_jaxrs_relative_path_templates fails with "/" != "/api/v1/widgets", and the route probe reports /api/v1/widgets and /api/v1/widgets/count missing

So the tests bind to the fix, which is the property that matters, and the change is scoped the way it should be: allow_relative is true at exactly one call site (the JAX-RS @Path branch) and false at every generic mapping, so nothing else can start inventing routes from bare strings. Adding Scala to the class-level composition is right too.

One small edge, and it is a behaviour change, so I would like it closed before this merges. route_path_from_string_node now ends with

return path[0] ? cbm_arena_sprintf(a, "/%s", path) : path;

For @Path("") that returns the empty string rather than NULL. On main an empty value is rejected (path[0] != '/'), the method's path stays unset, and extract_route_from_annotations falls back to "/"; with this change jax_path is "", which is non-NULL, so *out_path becomes "". @Path("") on a method is legal JAX-RS and means "the class path itself". Returning NULL for the empty case keeps today's behaviour:

return path[0] ? cbm_arena_sprintf(a, "/%s", path) : NULL;

plus one assertion for @Path("") next to your relative-template test. If you push it now it costs nothing: your CI run is still queued behind a busy runner pool, and a new push simply replaces it.

After that and a green matrix this merges. Thank you for the careful follow-through on the negative controls.

…path

Accepting relative JAX-RS templates made route_path_from_string_node return
"" for @path(""), which is non-NULL and so replaced the "/" fallback with an
empty method path. @path("") is legal JAX-RS and means "the class path
itself"; on main the empty value was rejected and the method fell back to
"/" like a method without @path. Return NULL for the empty case so that
behaviour is unchanged.

The regression covers both shapes: with a class-level @path the method
composes to the class path, and without one the route stays "/" (with the
empty string it was dropped entirely, because join_route_paths returns the
NULL prefix for an empty path).

Signed-off-by: Hubert Tarnacki <hubert.tarnacki@gmail.com>
@htarnacki

Copy link
Copy Markdown
Author

Closed the @Path("") edge in 246e4190, on top of your merge commit (no rebase):

return path[0] ? cbm_arena_sprintf(a, "/%s", path) : NULL;

One note on the test shape. A single assertion next to the relative-template test would not have bound to the fix: with a class-level @Path present, join_route_paths already treats an empty method path as "use the prefix", so "" and the "/" fallback compose to the same /api/v1/widgets. The case where the behaviour change is observable is a resource without a class-level @Path — there join_route_paths(NULL, "") returns the NULL prefix and the route is dropped altogether, whereas main yields "/".

So the regression is a dedicated extract_java_jaxrs_empty_path_means_class_path covering both shapes:

fixture expected with : path (previous revision)
@Path("api/v1/widgets") class, method @GET @Path("") /api/v1/widgets passes (masked by composition)
class without @Path, method @GET @Path("") / fails: index->route_path is NULL

Verified locally: with the fix extraction 372 passed / edge_types_probe 62 passed; with the previous : path line the new test fails at the second row.

The 4 clang-format findings in tests/test_extraction.c (around the parseJsonBody<T> TypeScript fixture) come in with the merge from main, not from this branch; I left them alone.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working parsing/quality Graph extraction bugs, false positives, missing edges priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Java/Scala JAX-RS: relative @Path templates (no leading slash) produce no Route nodes; Scala class-level @Path not composed

2 participants