fix(routes): find a route path wherever it sits in a decorator's arguments - #2008
Conversation
…ments
Java and Kotlin put no order on annotation attributes, so `path` can
appear after `method`, `produces` and `consumes`:
@RequestMapping(method = RequestMethod.GET,
produces = MediaType.APPLICATION_JSON_VALUE,
consumes = MediaType.APPLICATION_JSON_VALUE,
path = "/orders")
extract_route_path_from_args stopped after the third argument, so the
path was never read and no Route node formed. The endpoint was in the
source and not in the graph.
Four call sites read this function, so the miss covered plain decorator
arguments, Spring mappings, and both JAX-RS paths.
The loop now checks every argument. That matches find_drf_kwarg_in_args
a few lines below, which walks the same kind of node with no cap. Cost
stays small: the loop returns on the first path-shaped string, and each
argument's own subtree walk is still bounded by find_route_path_literal.
The recursive breadth guard inside find_route_path_literal keeps
DECORATOR_SCAN_LIMIT. That loop descends to CBM_DESCENDANT_MAX_DEPTH, so
breadth 3 by depth 6 is 729 visits at worst and unbounded breadth there
is not bounded at all.
The test fails without the change, reporting the class-level "/api"
route present and "/api/orders" missing.
Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
|
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. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
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. |
|
Approved. Verified against The diagnosis is right for the right reason: Java and Kotlin put no order on annotation attributes, so Two things I want to credit specifically: The RED output proves the test is not vacuous. And you drew the line in the right place. Removing the cap here while keeping Citing Merging once CI reports green — our Actions pool is badly backlogged right now, so it may sit a while. That is on us, not on this PR. |
|
Merged as The reason this was straightforward to accept is that you framed it correctly: Java and Kotlin impose no order on annotation attributes, so a positional cap on an unordered construct is a defect rather than a tuning value. Two things I want to credit specifically, because they are what separated this from a one-line constant bump: The RED output proved the fixture was not vacuous. And you drew the line in the right place. Removing the cap on the flat loop while keeping Citing #2009 is at 33 of 34 checks and I will merge it as soon as its last one reports — our Actions pool has been badly backlogged today, so the wait is ours, not yours. |
Java and Kotlin put no order on annotation attributes, so a
pathwritten aftermethod,producesandconsumesis ordinary source:extract_route_path_from_argsstopped after the third argument, so the path was never read and no Route node formed. The endpoint sat in the source and not in the graph.Reproduced before it was fixed
The test in this PR fails on current
main:available: /apiis the class-level@RequestMapping("/api"). Only the method route is lost.The change
One loop bound. The loop now checks every argument.
That matches
find_drf_kwarg_in_argsa few lines below, which walks the same kind of node with no cap. Cost stays small: the loop returns on the first path-shaped string, and each argument keeps its own bounded subtree walk infind_route_path_literal.Four call sites read this function, so the miss covered plain decorator arguments, Spring mappings, and both JAX-RS paths.
What is deliberately unchanged
find_route_path_literalkeepsDECORATOR_SCAN_LIMIT. That loop is recursive and descends toCBM_DESCENDANT_MAX_DEPTH, so breadth 3 by depth 6 is 729 visits at worst and unbounded breadth there is not bounded at all. The constant now guards only that loop, which is what its name describes.Tests
handles_spring_java_path_attribute_fourthintests/test_edge_types_probe.c, beside the existing Spring tests. It asserts the exact Route set rather than a HANDLES count, because the class-level mapping still produces one route on its own and a count alone would pass.Full C suite on this branch: 7780 passed, 7 skipped. Two
test_cli.cinstall/uninstall tests fail on my machine with or without this change, because they read the coding agents actually installed there.