feat(group): add Kotlin Spring WebClient long-form HTTP consumer extraction#1884
Conversation
…action Follow-up to abhigyanpatwari#1855. Extends `kotlin.ts` with the long-form WebClient fluent chain that abhigyanpatwari#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 abhigyanpatwari#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 abhigyanpatwari#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 ✅
|
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 10060 tests passed 4 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
|
Claude finished @magyargergo's task in 5m 31s —— View job Production-Readiness Review: PR #1884feat(group): add Kotlin Spring WebClient long-form HTTP consumer extraction Tasks
Review bar for this PRReviewed against DoD.md v2.0.0 (§2.1–§2.9, §5 five-axis gates, §6 not-Done signals). This is a narrow source-scan consumer extraction change. Relevant DoD items: correctness on the real runtime path, test assertions that catch broken wiring, no stale comments, exact assertions over bounds-only assertions, no unrelated churn. Problem being solvedKotlin Spring services using the enum-style WebClient fluent API — Current PR state
Merge status and mergeabilityCI green on Repository history considered
Branch hygiene assessmentClean. Diff is exactly 2 files: Understanding of the change
FindingsF1 — Stale block comment at test:1670–1675Risk: Comment left unupdated after the deferred gap is resolved. Evidence to check: Lines 1670–1675 of Confirmed evidence: The comment still says "Three consumer flavors" and "intentionally deferred to a follow-up" after this PR adds the fourth flavor and resolves the deferral. The kotlin.ts file header was correctly updated to list four patterns (lines 23–26), but the test file block comment was not. Recommended fix: Update the comment to say "Four consumer flavors covered here (RestTemplate, WebClient short, OkHttp, WebClient long form)." Remove the deferred sentence. Blocks merge: No. DoD §2.3 flags stale comments as a not-Done signal, but this is a misleading comment in a test file, not executable logic. Minor follow-up acceptable. Fix this → F2 —
|
| Concern | Verified outcome |
|---|---|
| Hidden Unicode | Clean — grep returned no hits |
| Kotlin tests ran in CI | Confirmed — 66/66 in http-route-extractor.test.ts, 0 skips |
| Double-emit | Pinned by exact toHaveLength(1) assertion |
| Variable-bound verb | Pinned by exact toHaveLength(0) assertion |
| Short-form query contamination | Structurally impossible — "method" fails short-form verb regex |
| Path normalization | Verified correct: numeric IDs → {param}, tested for PUT/DELETE/PATCH |
dedupeContracts behavior |
Key is `contractId |
| Contract framework label | spring-web-client asserted in 2 of 4 new tests |
| OkHttp / RestTemplate / provider behavior | Untouched files; existing tests still present in CI |
| #1834 named-arg Kotlin safety | Provider query blocks unchanged |
Open questions
None. All production-risk questions are resolved by code inspection and CI evidence.
Final verdict
production-ready with minor follow-ups
The core logic is correct: the tree-sitter query structurally matches the long-form chain, verb whitelisting is sound, the double-emit guarantee is proven by an exact assertion, variable-bound verbs are safely rejected, contract IDs normalize correctly, and all 66 tests passed across 3 CI platforms. Two minor items do not block production correctness: (1) a stale block comment at test:1670–1675 still says "three consumer flavors / deferred to follow-up" — it should say four and drop the deferral sentence; (2) toBeGreaterThanOrEqual(4) in the multi-verb test should be toBe(4) per DoD §2.7. The pre-merge SHA gap in CI is low-risk given the merge only brings in Java consumer changes with no shared infrastructure overlap. Recommend updating the stale comment and weak assertion in this PR or immediately after; neither affects runtime behavior.
|
@henry201605 could you please look into these 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 ✅
|
@magyargergo Thanks for the review! 🙏 Went through the four findings — quick notes below. F1 (stale comment, "Three consumer flavors / deferred") — fixed in F2 ( F3 (no negative test for HEAD / OPTIONS / TRACE) — intentionally not added here. As Claude noted, the same verbs are also implicitly excluded on the short-form path from #1855 without an explicit pin, so adding one only on the long form would create asymmetry. Happy to open a separate small PR adding HEAD/OPTIONS/TRACE pins for both short and long forms together if you'd like consistent coverage — just say the word. F4 (CI ran on pre-merge SHA Local validation after the new commit:
Happy to make any further adjustments! |
…e scope Address @magyargergo's review request on PR abhigyanpatwari#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 ✅
|
@magyargergo Thanks for the catch! 🙏 Done in commit |
Summary
Follow-up to #1855. Adds support for the WebClient long-form fluent chain that #1855 explicitly deferred:
The short form (
webClient.get().uri("/x")) was already covered in #1855. The long form remains common in Kotlin Spring 4 → 5 migrations and in codebases that prefer the fluent verb-as-enum style.Approach
A single deeper tree-sitter query (
WEB_CLIENT_LONG_PATTERNS) matches the full chain structurally — both.method(HttpMethod.X)and.uri("...")are captured in one pattern, no walk-up helper needed.(call_expression (navigation_expression (call_expression (navigation_expression (simple_identifier) @obj (#eq? @obj "webClient") (navigation_suffix (simple_identifier) @method_call (#eq? @method_call "method"))) (call_suffix (value_arguments . (value_argument (navigation_expression (simple_identifier) @httpMethodCls (#eq? @httpMethodCls "HttpMethod") (navigation_suffix (simple_identifier) @verb)))))) (navigation_suffix (simple_identifier) @uri (#eq? @uri "uri"))) (call_suffix (value_arguments . (value_argument . (string_literal) @path))))Verb is captured as the
simple_identifierunderHttpMethod'snavigation_suffix(the literal field nameGET/POST/...) and whitelisted to^(GET|POST|PUT|DELETE|PATCH)$for symmetry with the short form'sWEB_CLIENT_SHORT_TO_HTTPmap.Receiver constraint
(#eq? @obj "webClient")mirrors the short form and the Java plugin's heuristic.Out of scope (intentional)
val verb = HttpMethod.PATCH; webClient.method(verb).... Source-scan can't follow the binding without graph context. Pinned by an anti-overreach test in the consumer suite.WEB_CLIENT_SHORT_TO_HTTPeither — keeps polyglot symmetry withjava.tsand the short form.Tests
4 new cases under
consumer extraction — fetch patterns, gated by tree-sitter-kotlin grammar availability.Positive (3)
Anti-regression (1)
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 validation
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 — exactly the right shape).
Local validation
http-route-extractor.test.ts— 66 / 66 ✅test/unit/group— 546 / 546 ✅npx prettier --check(changed files) — clean ✅