diff --git a/gitnexus/src/core/group/extractors/http-patterns/java.ts b/gitnexus/src/core/group/extractors/http-patterns/java.ts index 0e98159dd..37c16dea0 100644 --- a/gitnexus/src/core/group/extractors/http-patterns/java.ts +++ b/gitnexus/src/core/group/extractors/http-patterns/java.ts @@ -678,13 +678,16 @@ export const JAVA_HTTP_PLUGIN: HttpLanguagePlugin = { language: Java, // routeCoverage intentionally LEFT at the default 'partial' (#2138 Part 2). // The graph provider set is a strict *subset* of this scan()'s provider set — - // ingestion does NOT emit a Route node for (1) array-form `@GetMapping({...})`, - // (2) interface-inherited Spring routes, or (3) the 2nd verb of a same-URL - // GET+POST pair (Route nodes are URL-keyed). Declaring 'complete' here would - // let the parse-skip drop those group-only providers. Java flips to 'complete' - // only once ingestion provider extraction matches this scan (a follow-up: - // array-form query branch + interface-inheritance emission + per-verb Route - // identity). `hasConsumerSignals` below is kept ready for that flip. + // ingestion does NOT emit a Route node for (1) a method-level array route + // nested under a class-level array-form `@RequestMapping` (ingestion suppresses + // it rather than drop the prefix; bare/scalar-prefixed array methods ARE now + // emitted — see #2280), (2) interface-inherited Spring routes, or (3) the 2nd + // verb of a same-URL GET+POST pair (Route nodes are URL-keyed). Declaring + // 'complete' here would let the parse-skip drop those group-only providers. + // Java flips to 'complete' only once ingestion provider extraction matches this + // scan (a follow-up: class-level array-form prefix support + interface- + // inheritance emission + per-verb Route identity — tracked in #2280). + // `hasConsumerSignals` below is kept ready for that flip. // Consumer signals this plugin's scan() can detect: RestTemplate / WebClient / // OkHttp / Java-HttpClient / Apache-HttpClient call sites, OpenFeign // (`@FeignClient` + `@RequestLine`) interfaces, and Spring 6 HTTP Interface diff --git a/gitnexus/src/core/ingestion/route-extractors/spring.ts b/gitnexus/src/core/ingestion/route-extractors/spring.ts index a92d09afe..c0e0ea369 100644 --- a/gitnexus/src/core/ingestion/route-extractors/spring.ts +++ b/gitnexus/src/core/ingestion/route-extractors/spring.ts @@ -39,6 +39,18 @@ import { * @node → enclosing declaration (class_declaration | method_declaration) * @value → the string-literal argument * @key → the named-argument member key (absent for positional form) + * + * Method-level routes accept both the bare string form `@GetMapping("/x")` and + * the array form `@GetMapping({"/a","/b"})` (positional or `path =`/`value =`): + * a multi-element array yields one match per element, so the Phase 2 loop emits + * one route per path with no special-casing. This mirrors the group-layer + * `java.ts` query so the two Spring extractors stay in parity (#2138 follow-up; + * the divergence here was the root of the #2265 array-form gap). The class-level + * `@RequestMapping` branches also match the array form, but only to *detect* it: + * an array-form class prefix can't be resolved to a single string, so Phase 2 + * suppresses that class's method-level array routes rather than emit them with a + * dropped prefix (a wrong route). Full class-array cross-product support is left + * to a follow-up (#2280). */ const ROUTE_ANNOTATION_QUERY = new Parser.Query( Java, @@ -48,7 +60,9 @@ const ROUTE_ANNOTATION_QUERY = new Parser.Query( (modifiers (annotation name: (identifier) @ann - arguments: (annotation_argument_list (string_literal) @value)))) @node + arguments: (annotation_argument_list + [(string_literal) @value + (element_value_array_initializer (string_literal) @value)])))) @node (class_declaration (modifiers (annotation @@ -56,12 +70,15 @@ const ROUTE_ANNOTATION_QUERY = new Parser.Query( arguments: (annotation_argument_list (element_value_pair key: (identifier) @key - value: (string_literal) @value))))) @node + value: [(string_literal) @value + (element_value_array_initializer (string_literal) @value)]))))) @node (method_declaration (modifiers (annotation name: (identifier) @ann - arguments: (annotation_argument_list (string_literal) @value)))) @node + arguments: (annotation_argument_list + [(string_literal) @value + (element_value_array_initializer (string_literal) @value)])))) @node (method_declaration (modifiers (annotation @@ -69,7 +86,8 @@ const ROUTE_ANNOTATION_QUERY = new Parser.Query( arguments: (annotation_argument_list (element_value_pair key: (identifier) @key - value: (string_literal) @value))))) @node + value: [(string_literal) @value + (element_value_array_initializer (string_literal) @value)]))))) @node ] `, ); @@ -93,8 +111,15 @@ export function extractSpringRoutes( ): ExtractedDecoratorRoute[] { const matches = ROUTE_ANNOTATION_QUERY.matches(tree.rootNode); - // Phase 1: collect class-level @RequestMapping prefixes keyed by node id + // Phase 1: collect class-level @RequestMapping prefixes keyed by node id. + // A scalar prefix (`@RequestMapping("/base")`) is stored in prefixByClassId. + // A class whose @RequestMapping uses the array form (`@RequestMapping({...})`) + // is instead recorded in classesWithArrayPrefix: there is no single prefix to + // store, and Phase 2 uses this to suppress that class's method-level array + // routes rather than emit them unprefixed (a wrong route — see #2280). Full + // class-array cross-product support is out of scope here. const prefixByClassId = new Map(); + const classesWithArrayPrefix = new Set(); for (const match of matches) { const caps: Record = {}; @@ -109,6 +134,10 @@ export function extractSpringRoutes( if (node.type === 'class_declaration' && annNode.text === 'RequestMapping') { if (!isRouteMemberKey(keyNode)) continue; + if (valueNode.parent?.type === 'element_value_array_initializer') { + classesWithArrayPrefix.add(node.id); + continue; + } const prefix = unquoteSpringLiteral(valueNode.text); if (prefix !== null) prefixByClassId.set(node.id, prefix); } @@ -138,6 +167,20 @@ export function extractSpringRoutes( const routePath = unquoteSpringLiteral(valueNode.text); if (routePath === null) continue; const enclosingClass = findEnclosingClass(node); + + // Suppress a method-level *array-form* route nested under a class-level + // array-form @RequestMapping. The class prefix is one of several values that + // cannot be resolved to a single string here, so emitting the route would + // drop the prefix and yield a wrong unprefixed Route (a false signal, worse + // than a missing one). Skipping keeps ingestion a strict subset of the group + // scan — safe under routeCoverage:'partial'. Full class-array cross-product + // support is tracked in #2280. (Scalar method paths under an array class + // prefix are left unchanged: that pre-existing divergence is out of scope.) + const isArrayElement = valueNode.parent?.type === 'element_value_array_initializer'; + if (isArrayElement && enclosingClass && classesWithArrayPrefix.has(enclosingClass.id)) { + continue; + } + const classPrefix = enclosingClass ? (prefixByClassId.get(enclosingClass.id) ?? '') : ''; // `node` is the annotated `method_declaration`; its name field is the // handler method name (resolved to a symbol UID later by the routes phase). diff --git a/gitnexus/test/integration/route-parse-skip.test.ts b/gitnexus/test/integration/route-parse-skip.test.ts index 18845585b..6dccdd224 100644 --- a/gitnexus/test/integration/route-parse-skip.test.ts +++ b/gitnexus/test/integration/route-parse-skip.test.ts @@ -173,7 +173,11 @@ public class AController { `; const dir = mkRepo({ 'AController.java': AC }); try { - // Graph resolves only /covered (ingestion has no array-form Route node). + // The mock DB resolves only /covered; it deliberately omits the array-form + // routes to exercise the source-scan fallback. (Ingestion now DOES emit + // array-form Route nodes under a scalar/absent class prefix — see #2280 — + // but this test asserts the group extractor still recovers them via scan + // when the graph happens to lack them, which is what 'partial' guarantees.) const out = await new HttpRouteExtractor().extract( makeDb([ { file: 'AController.java', routePath: '/covered', method: 'GET', resolved: true }, @@ -182,7 +186,7 @@ public class AController { repo, ); const paths = providerPaths(out); - // The array-form routes are graph-only-absent but survive via source scan. + // The array-form routes are absent from this mock graph but survive via source scan. expect(paths).toEqual(expect.arrayContaining(['GET::/a', 'GET::/b'])); } finally { fs.rmSync(dir, { recursive: true, force: true }); diff --git a/gitnexus/test/unit/group/spring-route-parity.test.ts b/gitnexus/test/unit/group/spring-route-parity.test.ts new file mode 100644 index 000000000..6ecab61ae --- /dev/null +++ b/gitnexus/test/unit/group/spring-route-parity.test.ts @@ -0,0 +1,199 @@ +/** + * Parity test for the two Spring route extractors (#2078 maintainer request, + * #2138 follow-up). + * + * GitNexus parses Spring `@(Get|Post|...)Mapping` annotations in TWO places: + * - ingestion `route-extractors/spring.ts` → `extractSpringRoutes` (produces + * graph `Route` nodes) + * - group `http-patterns/java.ts` → `JAVA_HTTP_PLUGIN.scan` (produces + * cross-repo HTTP contracts) + * + * They serve different layers and stay separate, but they MUST agree on the + * set of provider (method, path) routes they recognise for the same source — + * otherwise the graph under-covers what the group scan sees, which is exactly + * the divergence behind the #2265 array-form gap (the group query matched + * `@GetMapping({"/a","/b"})`, ingestion's didn't). This test runs one shared + * fixture through both and asserts the provider sets are identical for the + * shapes ingestion claims to cover: bare, named-arg, and array-form method + * routes under a *scalar* (or absent) class prefix. + * + * Known, deliberate divergence (NOT covered by the equality assertions): a + * method-level array route nested under a class-level *array-form* + * @RequestMapping. Ingestion suppresses it (it can't resolve which of several + * class prefixes to apply, and a dropped-prefix route is a wrong signal), while + * the group layer emits the full cross-product. The last test pins this so the + * suppression can't silently regress into emitting wrong routes; full + * class-array support is tracked in #2280. + */ +import { describe, it, expect } from 'vitest'; +import Parser from 'tree-sitter'; +import Java from 'tree-sitter-java'; +import { extractSpringRoutes } from '../../../src/core/ingestion/route-extractors/spring.js'; +import { JAVA_HTTP_PLUGIN } from '../../../src/core/group/extractors/http-patterns/java.js'; + +function parse(src: string): Parser.Tree { + const p = new Parser(); + p.setLanguage(Java); + return p.parse(src); +} + +/** Canonical `METHOD /a/b` form so prefix-join / slash / case differences wash out. */ +function canon(method: string, ...segments: string[]): string { + const path = `/${segments.join('/').split('/').filter(Boolean).join('/')}`; + return `${method.toUpperCase()} ${path.toLowerCase()}`; +} + +/** ingestion side: join the class prefix + method path the way the routes phase does. */ +function ingestionProviders(src: string): Set { + return new Set( + extractSpringRoutes(parse(src), 'X.java').map((r) => + canon(r.httpMethod, r.prefix ?? '', r.routePath), + ), + ); +} + +/** group side: provider detections (path already prefix-joined by the plugin). */ +function groupProviders(src: string): Set { + return new Set( + JAVA_HTTP_PLUGIN.scan(parse(src)) + .filter((d) => d.role === 'provider') + .map((d) => canon(d.method, d.path)), + ); +} + +describe('Spring route extractor parity — ingestion spring.ts vs group java.ts', () => { + it('agree on bare, named-arg, and array-form method routes under a class prefix', () => { + const src = `package com.example; +import org.springframework.web.bind.annotation.*; + +@RestController +@RequestMapping("/api/orders") +public class OrderController { + @GetMapping("/list") public Object list() { return null; } + @PostMapping(path = "/make") public Object make() { return null; } + @PutMapping(value = "/update") public Object update() { return null; } + @GetMapping({"/a", "/b"}) public Object multi() { return null; } +} +`; + const ingestion = ingestionProviders(src); + const group = groupProviders(src); + + // The array form is the regression that motivated this: both must see all four. + expect(group).toEqual( + new Set([ + 'GET /api/orders/list', + 'POST /api/orders/make', + 'PUT /api/orders/update', + 'GET /api/orders/a', + 'GET /api/orders/b', + ]), + ); + expect(ingestion).toEqual(group); + }); + + it('agree on a no-prefix controller with a positional array', () => { + const src = `package com.example; +import org.springframework.web.bind.annotation.*; + +@RestController +public class PlainController { + @GetMapping("/solo") public Object solo() { return null; } + @DeleteMapping({"/x", "/y", "/z"}) public Object many() { return null; } +} +`; + expect(ingestionProviders(src)).toEqual(groupProviders(src)); + }); + + it('agree on named-arg ARRAY forms (path = {...} / value = {...})', () => { + // Guards the spring.ts named-array query branch specifically: positional + // arrays alone would not exercise it. + const src = `package com.example; +import org.springframework.web.bind.annotation.*; + +@RestController +@RequestMapping("/api") +public class NamedArrayController { + @PutMapping(value = {"/update", "/modify"}) public Object upd() { return null; } + @DeleteMapping(path = {"/x", "/y"}) public Object del() { return null; } +} +`; + const group = groupProviders(src); + expect(group).toEqual( + new Set(['PUT /api/update', 'PUT /api/modify', 'DELETE /api/x', 'DELETE /api/y']), + ); + expect(ingestionProviders(src)).toEqual(group); + }); + + it('do not leak non-route arrays (consumes/produces) as routes — array analogue', () => { + // The scalar `produces` anti-regression already exists in the route tests; + // this is its array form. `consumes`/`produces` arrays must never surface as + // provider routes; only the path value does. + const src = `package com.example; +import org.springframework.web.bind.annotation.*; + +@RestController +public class ContentTypeController { + @GetMapping(value = "/v", consumes = {"application/json", "application/xml"}, produces = {"application/json"}) + public Object v() { return null; } + @PostMapping(consumes = {"application/json"}) + public Object noPath() { return null; } +} +`; + const ingestion = ingestionProviders(src); + const group = groupProviders(src); + // Only the explicit path leaks through; the consumes/produces arrays do not, + // and the path-less @PostMapping contributes nothing. + expect(group).toEqual(new Set(['GET /v'])); + expect(ingestion).toEqual(group); + + // Pure-consumes controller (no path anywhere) → EMPTY provider set on both + // sides: a consumes/produces array must never be misread as a route path. + const consumesOnly = `package com.example; +import org.springframework.web.bind.annotation.*; + +@RestController +public class ConsumesOnlyController { + @PostMapping(consumes = {"application/json", "application/xml"}) + public Object a() { return null; } + @PutMapping(produces = {"application/json"}) + public Object b() { return null; } +} +`; + expect(groupProviders(consumesOnly)).toEqual(new Set()); + expect(ingestionProviders(consumesOnly)).toEqual(new Set()); + }); + + it('pins the deliberate class-array divergence: ingestion suppresses, group emits cross-product (#2280)', () => { + // A method-level array under a class-level ARRAY-form @RequestMapping. There + // is no single class prefix to apply, so ingestion suppresses the route + // rather than emit it unprefixed (a wrong signal). The group layer emits the + // full cross-product. This is a KNOWN gap (#2280), pinned here so the + // suppression can't silently regress into emitting wrong unprefixed routes. + const src = `package com.example; +import org.springframework.web.bind.annotation.*; + +@RestController +@RequestMapping({"/base/one", "/base/two"}) +public class MultiPrefixController { + @GetMapping({"/primary", "/alias"}) public Object x() { return null; } +} +`; + const ingestion = ingestionProviders(src); + const group = groupProviders(src); + + // group: correct cross-product of the two class prefixes × two method paths. + expect(group).toEqual( + new Set([ + 'GET /base/one/primary', + 'GET /base/two/primary', + 'GET /base/one/alias', + 'GET /base/two/alias', + ]), + ); + // ingestion: suppressed — emits NO route for the array method (never a wrong + // unprefixed `GET /primary` / `GET /alias`). + expect(ingestion).toEqual(new Set()); + // And the divergence is asymmetric-by-design: ingestion ⊊ group here. + expect(ingestion).not.toEqual(group); + }); +});