feat(java): add HTTP consumer contract extraction#1872
Conversation
|
Someone is attempting to deploy a commit to the NexusCore Team on Vercel. A member of the Team first needs to authorize it. |
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 10053 tests passed 4 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
✨ PR AutofixFound fixable formatting / unused-import issues across 14 changed lines. Comment |
|
/autofix |
|
✅ Applied autofix and pushed a commit. (apply run) |
|
Claude finished @magyargergo's task in 6m 18s —— View job Senior Review Panel — PR #1872:
|
| Aspect | Assessment |
|---|---|
role |
✅ All new paths correctly emit "consumer". Feign methods are correctly classified consumer, not provider. |
framework |
✅ Values are specific and consistent: spring-rest-template, spring-web-client, openfeign, java-http-client, apache-http-client, okhttp. Framework strings match Kotlin plugin parity. |
method |
✅ Uppercase. RestTemplate methods mapped through REST_TEMPLATE_TO_HTTP. exchange uses .toUpperCase() on the HttpMethod field name. WebClient short-form uses WEB_CLIENT_SHORT_TO_HTTP. Apache uses APACHE_HTTP_CLIENT_TO_HTTP. |
path |
✅ Comes from unquoteLiteral(). No over-inference. Template placeholders survive to the orchestrator's normalizer. |
name |
✅ Feign methods set name to the method identifier. RestTemplate/WebClient/Apache/HttpClient use null — correct, since there's no reliable caller name at the call site without scope resolution. |
confidence |
✅ Spring clients use 0.7, Java/Apache HttpClient use 0.65. OkHttp uses 0.7. Slight concern: OkHttp always defaults to GET regardless of actual verb (known limitation), making its 0.7 confidence arguably high. Consider 0.5 for OkHttp or document the heuristic. |
| Duplicate risk | Low. Patterns run on distinct structural shapes; no two patterns should fire on the same AST node. Feign methods use continue after consumer emission, preventing double-emission as provider. |
| Group sync impact | Consumer contracts have the right shape for cross-linking. But see finding #7 — no integration test confirms the link is actually made. |
Test review
Good test cases:
extracts Java Spring RestTemplate, WebClient and OkHttp literal calls— covers multiple patterns in one file with full contract verification (framework + confidence).extracts OpenFeign clients as consumers, not providers— explicitly asserts no provider from the same file.extracts OpenFeign clients without an interface path prefix— correct degenerate case.extracts OpenFeign clients with @RequestMapping interface prefixes— tests the@RequestMappingprefix path.extracts Java and Apache HttpClient literal request construction— covers 4/5 Apache verbs and 2 Java HttpClient verbs with framework+confidence checks.- Kotlin parity tests are a nice addition; the anti-overreach tests for long-form WebClient and non-
restTemplatereceiver are exactly right.
Missing test cases:
@FeignClient(path=...)+@RequestMapping(...)precedence (blocking — see finding Add support for Ollama as a local inference backend #1).HttpPatchvia Apache HttpClient —new HttpPatch("/api/orders/5")never appears in the test fixture.- Negative case for dynamic URL in RestTemplate — e.g.,
restTemplate.getForObject(baseUrl + "/path", ...)— confirm no consumer is emitted. This documents the intentional limitation. webClient.method(HttpMethod.X).uri("/path")deferred — an anti-overreach test confirming no consumer is emitted from the real long-form chain. (Kotlin has this test but Java does not.)
Weak test cases:
- The OpenFeign fixture at test line 1433 uses
url = "${order.service.url}"which contains an EL expression. The test verifies that this is ignored as a path prefix correctly. But the string would be parsed by tree-sitter as astring_literalcontaining${...}— it would be interesting to confirm this edge is actually handled byunquoteLiteral.
Regression and compatibility risks
-
SPRING_METHOD_ROUTE_PATTERNSnow feeds the Feign consumer path. Methods inside a@FeignClient-annotated interface were previously emitted as providers. Thecontinueguard at line 424 now prevents that. This is correct and is tested, but any existing group sync state that has previously stored these as providers from an older analysis run would be stale. Not a code regression, but a schema-state migration concern. -
WEB_CLIENT_PATTERNSadds a new consumer extraction path. If any production Java codebase happens to have a custom two-argumentwebClient.method(HttpMethod.X, "/path")call (e.g., a custom wrapper), it would now emit consumer contracts it didn't before. Extremely unlikely but possible. -
hasAnnotationscanning the full interface text. If an interface had// @FeignClientin a method javadoc, its@GetMappingmethods would now be promoted from provider to consumer. This is a behaviour change that could silently misclassify routes.
Suggested patch plan
Minimal changes to unblock merge:
Required (blocking):
- Add one test case to
http-route-extractor.test.tsfor@FeignClient(path=...)+@RequestMapping(...)precedence (see finding Add support for Ollama as a local inference backend #1 above for the fixture). This is a small test addition to the existing OpenFeign describe block.
Strongly recommended (non-blocking but should land in this PR or as immediate follow-up):
2. Add new HttpPatch("/api/orders/5") to the Apache HttpClient fixture and assert the PATCH consumer.
3. Add a WEB_CLIENT_PATTERNS code comment (or a "not yet supported" pin test) clarifying the two-argument form matches no real Spring API.
4. Remove PATCH from the JAVA_HTTP_CLIENT_PATTERNS method regex or add a code comment explaining it can never fire (no .PATCH() method on HttpRequest.Builder).
Deferred (follow-up issue):
5. Replace hasAnnotation regex with AST-based annotation checking.
6. Document variable name constraints in code comments.
7. Add group sync integration test for consumer → crossLink → provider.
Final merge recommendation
Do not merge until finding #1 is addressed (add the @FeignClient(path=...) + @RequestMapping(...) precedence regression test). Without this test, the first-wins guard introduced in 5bc73bd could be silently broken by any future pattern reordering.
Finding #2 (WEB_CLIENT_PATTERNS targets non-existent API) should also be resolved — either add the clarifying comment or remove the dead pattern — before merge, since it actively misleads future contributors about what is and isn't supported.
All other findings are non-blocking and can land in a follow-up issue.
|
@ShiningXu could you please look into claude's findings? 🙏 |
…atwari#1884 Two minor follow-ups from the production-readiness review: F1 — Stale block comment at the top of the Kotlin consumer suite (was: "Three consumer flavors covered here ... long-form deferred to a follow-up"). Updated to "Four consumer flavors" and removed the deferred sentence — the deferral is resolved by this PR. The kotlin.ts file header was already updated; this brings the test file comment in sync. Per DoD §2.3 (no stale comments). F2 — Replaced `expect(wcConsumers.length).toBeGreaterThanOrEqual(4)` with `expect(wcConsumers).toHaveLength(4)` in the multi-verb test. The fixture is fully deterministic — exactly 4 long-form calls, no other consumer types — so an exact count assertion is the right shape per DoD §2.7 ("use toBe / toEqual for exact expectations"). Added a comment explaining what the assertion catches that the existing per-verb toBeDefined() checks would miss (accidental 5th consumer from a duplicate query firing or a regressed receiver constraint). F3 (HEAD/OPTIONS/TRACE negative test) is intentionally not added in this PR — same precedent as abhigyanpatwari#1855 where HEAD/OPTIONS/TRACE on the short form are also implicitly excluded without a pinning test. Happy to add one in a separate PR if maintainers want explicit pinning across both forms. F4 (CI on pre-merge SHA) is the maintainer's call — the merge from main is theirs to re-trigger CI on. The merge brings only Java consumer changes (PR abhigyanpatwari#1872) and Go provider changes (PR abhigyanpatwari#1886), both in entirely separate files from this PR's Kotlin work. Local validation: - test/unit/group/http-route-extractor.test.ts: 73/73 ✅ (66 from this PR pre-merge + 7 from PR abhigyanpatwari#1872 merged via main) - npx prettier --check (changed files): clean ✅
…action (#1884) * feat(group): add Kotlin Spring WebClient long-form HTTP consumer extraction Follow-up to #1855. Extends `kotlin.ts` with the long-form WebClient fluent chain that #1855 explicitly deferred: webClient.method(HttpMethod.GET).uri("/x").retrieve().awaitBody<T>() This pattern remains common in Kotlin Spring 4 → 5 migrations and in codebases that prefer the fluent verb-as-enum style. The short form (`webClient.get().uri("/x")`) was already supported in #1855. Approach: - Single deeper tree-sitter query (`WEB_CLIENT_LONG_PATTERNS`) that matches the full chain structurally — both `.method(HttpMethod.X)` and `.uri("...")` in one pattern. Verb is captured as the `simple_identifier` of the `HttpMethod.X` field access. - Verb is whitelisted to GET/POST/PUT/DELETE/PATCH (consistent with the short-form's `WEB_CLIENT_SHORT_TO_HTTP` map). - Receiver constraint `(#eq? @obj "webClient")` mirrors the short form and Java plugin heuristic. Out of scope (intentional): - Variable-bound verbs: `val verb = HttpMethod.PATCH; webClient.method(verb)...` Source-scan can't follow the binding without graph context. Pinned by an anti-overreach test. - HEAD/OPTIONS/TRACE: not in `WEB_CLIENT_SHORT_TO_HTTP` either — keeps polyglot symmetry with java.ts and the short form. Tests: 4 new cases under `consumer extraction — fetch patterns`, gated by tree-sitter-kotlin grammar availability. positive (3) - long form GET - long form POST / PUT / DELETE / PATCH (4 verbs in 1 fixture) - no double-emit pin (long-form chain produces exactly one consumer, not one from each query) anti-regression (1) - variable-bound verb does NOT match (graph-aware concern) The previous `'does NOT match Kotlin WebClient long form (deferred to follow-up)'` test from #1855 is replaced by these — the deferred state is now resolved. Reverse-validated: temporarily disabling the long-form emit makes exactly the 3 positive tests fail; the variable-bound-verb anti- regression test continues to pass (it pins behavior independent of the emit being on or off). Local validation: - test/unit/group/http-route-extractor.test.ts: 66/66 ✅ - test/unit/group: 546/546 ✅ - npx prettier --check (changed files): clean ✅ * test(group): address Claude review findings F1 and F2 on PR #1884 Two minor follow-ups from the production-readiness review: F1 — Stale block comment at the top of the Kotlin consumer suite (was: "Three consumer flavors covered here ... long-form deferred to a follow-up"). Updated to "Four consumer flavors" and removed the deferred sentence — the deferral is resolved by this PR. The kotlin.ts file header was already updated; this brings the test file comment in sync. Per DoD §2.3 (no stale comments). F2 — Replaced `expect(wcConsumers.length).toBeGreaterThanOrEqual(4)` with `expect(wcConsumers).toHaveLength(4)` in the multi-verb test. The fixture is fully deterministic — exactly 4 long-form calls, no other consumer types — so an exact count assertion is the right shape per DoD §2.7 ("use toBe / toEqual for exact expectations"). Added a comment explaining what the assertion catches that the existing per-verb toBeDefined() checks would miss (accidental 5th consumer from a duplicate query firing or a regressed receiver constraint). F3 (HEAD/OPTIONS/TRACE negative test) is intentionally not added in this PR — same precedent as #1855 where HEAD/OPTIONS/TRACE on the short form are also implicitly excluded without a pinning test. Happy to add one in a separate PR if maintainers want explicit pinning across both forms. F4 (CI on pre-merge SHA) is the maintainer's call — the merge from main is theirs to re-trigger CI on. The merge brings only Java consumer changes (PR #1872) and Go provider changes (PR #1886), both in entirely separate files from this PR's Kotlin work. Local validation: - test/unit/group/http-route-extractor.test.ts: 73/73 ✅ (66 from this PR pre-merge + 7 from PR #1872 merged via main) - npx prettier --check (changed files): clean ✅ * refactor(group): hoist Kotlin WebClient long-form verb regex to module scope Address @magyargergo's review request on PR #1884: > Can you please extract the regexp from the for loop? 🙏 (kotlin.ts:510) Compiles the verb whitelist `^(GET|POST|PUT|DELETE|PATCH)$` once at module load instead of every iteration of the long-form scan loop. Mirrors the placement and JSDoc style of the sibling `WEB_CLIENT_SHORT_TO_HTTP` constant. Behavior is unchanged — same verb whitelist, same exclusion of HEAD/OPTIONS/TRACE for symmetry with the short form. The 4 itKotlinConsumer long-form tests added in this PR continue to pass, and the variable-bound-verb anti-overreach test continues to pin the deliberate non-match. Local validation: - test/unit/group/http-route-extractor.test.ts: 77/77 ✅ - test/unit/group: 557/557 ✅ - npx prettier --check (changed file): clean ✅ --------- Co-authored-by: henry <zhangwei2017@unipus.cn> Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
Summary
Part of #1870. Adds low-complexity Java HTTP consumer extraction for common literal client patterns without taking on dynamic URL resolution.
What changed
RestTemplate.exchange("/path", HttpMethod.X, ...)consumers.WebClientshort-form chains likewebClient.post().uri("/path").@FeignClientplus Spring MVC method annotations.HttpRequest.newBuilder().uri(URI.create("/path")).GET/POST/...request construction.HttpGet/HttpPost/HttpPut/HttpDelete/HttpPatchrequest construction.@FeignClienttext in strings/comments is not misclassified.Validation
npx vitest run test/unit/group/http-route-extractor.test.ts— 70 passednpx vitest run test/unit/group— 550 passednpx tsc --noEmit./dist/cli/index.js detect-changes --scope unstaged --repo GitNexus— risk low, 0 affected processesFollow-up
Dynamic URL assembly, wrapper tracking, fully-qualified route annotation extraction, and broader interface-controller support are intentionally left for #1873.