mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-09 03:17:54 +00:00
fix(ingestion/routes): suppress wrong unprefixed route under class-array @RequestMapping; cover named-array + class-array parity
Addresses PR review on #2281: - P2 class-array wrong-route: class branches now match the array form only to detect it; a method-level array route under a class-level array-form @RequestMapping is suppressed rather than emitted with a dropped prefix, so ingestion stays a strict subset of the group scan. Scalar method paths under an array class prefix are unchanged (pre-existing). Full class-array cross-product support tracked in a follow-up. - P2 named-array coverage: added value={...}/path={...} parity cases, a consumes/produces array false-positive case, and a dedicated empty-provider-set assertion. - P3 stale comments: updated the routeCoverage comment in java.ts and the route-parse-skip test note; narrowed the parity test drift claim. routeCoverage stays 'partial'.
This commit is contained in:
parent
4cbd5ab4b4
commit
c68bdb6c28
4 changed files with 157 additions and 17 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -45,9 +45,12 @@ import {
|
|||
* 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). Array form on a
|
||||
* class-level `@RequestMapping` prefix is not matched here yet (rare; left to a
|
||||
* follow-up) — `@value` on the class branches stays a single string literal.
|
||||
* 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,
|
||||
|
|
@ -57,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
|
||||
|
|
@ -65,7 +70,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
|
||||
(method_declaration
|
||||
(modifiers
|
||||
(annotation
|
||||
|
|
@ -105,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<number, string>();
|
||||
const classesWithArrayPrefix = new Set<number>();
|
||||
|
||||
for (const match of matches) {
|
||||
const caps: Record<string, Parser.SyntaxNode> = {};
|
||||
|
|
@ -121,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);
|
||||
}
|
||||
|
|
@ -150,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).
|
||||
|
|
|
|||
|
|
@ -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 });
|
||||
|
|
|
|||
|
|
@ -13,8 +13,17 @@
|
|||
* 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, so the two
|
||||
* can't silently drift again.
|
||||
* 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';
|
||||
|
|
@ -94,4 +103,97 @@ public class PlainController {
|
|||
`;
|
||||
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);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue