fix: flatten JS/TS template-literal URLs to {} placeholders - #1008
Conversation
|
Thanks for putting this together. I have triaged it into 0.9.1-rc as the fix candidate for #1006. Review focus will be template-literal flattening semantics, placeholder handling, and confirming that existing static-literal fetch/query extraction stays unchanged. |
|
Reviewed and verified locally — the approach is right, the flattening semantics match the server-side placeholder exactly, and static-literal behavior is unchanged. Three things before merge, two mechanical and one small hardening: Verified working (your branch merged onto current main):
1. DCO (blocking): commit 2. clang-format (blocking): two lines need wrapping — // extract_unified.c:527
char *value =
flat_value ? (char *)flat_value : cbm_node_text(ctx->arena, value_node, ctx->source);
// extract_unified.c:578 (initializer wraps)
.enclosing_func_qn =
state->enclosing_func_qn ? state->enclosing_func_qn : ctx->module_qn,3. Hardening (please include): in if (strcmp(ak, "template_string") == 0) {
const char *flat = cbm_template_string_text(ctx->arena, arg, ctx->source);
if (flat) {
return strip_and_validate_string_arg(ctx->arena, (char *)flat);
}
}(Also noticed With those three in and CI green this is good to merge. Nice work on keying the placeholder to the server-side canonical form rather than inventing a new one. |
d2868cd to
007ee18
Compare
|
Applied all three points: sign-off added to the commit, the two lines in extract_unified.c are now clang-format clean, and the flattened template text goes through strip_and_validate_string_arg in extract_positional_url as suggested. Full suite passes locally with the regression test for #1006 intact. |
|
Reviewed and verified — this is ready on the merits, just needs a quick rebase. Verified on your latest commit (007ee18) merged onto current main: extraction suite green ( The only thing blocking merge: your sibling PR #1007 (JAX-RS, same author) just merged, and since both touch |
…#1006) Client-side URL extraction only recognized static string literals; any template literal was silently skipped, so parameterized endpoints never produced HTTP_CALLS edges or Route nodes and cross-repo route matching missed them (the server side already normalizes path params to {}). New cbm_template_string_text() flattens a template_string node: string fragments verbatim, each ${...} substitution becomes {}. Wired into: - extract_positional_url / extract_string_value (call-arg URLs) - handle_string_refs (URL-shaped refs from const/return positions) - handle_string_constants (module-level const lookups) `/api/v1/things/${id}` now yields __route__ANY__/api/v1/things/{} and the enclosing function gets the HTTP_CALLS edge, joining the canonical placeholder shape of server-side routes. Signed-off-by: Charles Queiroz <fcqueiroz@liquibase.com>
007ee18 to
a029b3a
Compare
|
Rebased onto current main. The conflict was the adjacent RUN_TEST registration from #1007 in tests/test_extraction.c, resolved keeping both. Full suite on the rebased branch: 6358 passed, 1 skipped, 0 failures. lint-format clean. |
|
A note so this PR's status is not a mystery: #1010 physically contains this PR's head commit That means the review conversation has effectively moved to #1010, and I did not want you to think this one was being ignored. Your #1010 needs one change before either can land, and it is not in your diff: its URL-builder recording has no language gate, so on C and Go a helper returning One small asymmetry worth knowing, since it lives in your half: Nothing needed from you right now unless you and @CharlesQueiroz would rather unstack them — your call, and either way is fine by us. |
|
Merged — thank you, and good news on the CI too. Your checks were never actually broken. I re-ran them and everything came back green with no change to your code. The earlier red was a CodeQL gate that polled out 57 seconds before CodeQL itself succeeded on the same commit — pure infrastructure timing. You were never being asked to fix anything; the PR just looked red. Apologies for the time it spent looking like your problem. On the change itself. The implementation was reviewed carefully as part of the #1010 stack, since that PR builds on this commit. It is bounds-checked before every write, arena-allocated with the copy-out done correctly, and carries no lifetime or cross-thread hazard — the map is per-file and walk-local. One asymmetry worth knowing about, not a blocker and arguably belonging downstream: On #1010: it needs one change before it can follow — its URL-builder recording has no language gate, so on C and Go a helper returning Thanks for the careful work on both. |
Fixes #1006.
Root cause
Client-side URL extraction keyed on static string node kinds only (
is_string_like/is_string_node);template_stringwas never included, so any URL built with a template literal was invisible:Meanwhile the server side normalizes path params to
{}(__route__GET__/api/v1/things/{}), so the join key existed on one side only and cross-repo route matching missed every parameterized endpoint.Fix
New
cbm_template_string_text()(helpers.c) flattens atemplate_stringnode:string_fragmentchildren verbatim, eachtemplate_substitutionbecomes the canonical{}placeholder. Wired into the four extraction points that previously accepted only static strings:extract_positional_urlandextract_string_value(extract_calls.c): call-argument URLs, keyword and positional.handle_string_refs(extract_unified.c): URL-shaped string refs from const/return positions.handle_string_constants(extract_unified.c): module-level const lookup table.Template literals now behave exactly like the equivalent static literal with
{}in place of each interpolation; behavior for static strings is unchanged.Validation
extract_ts_template_string_url_issue1006:fetch(/api/v1/things/${id})yieldsfirst_string_arg == "/api/v1/things/{}", and the return-position template lands instring_refsas"/api/v1/things/{}/detail".__route__ANY__/api/v1/things/{}and thequeryFn -HTTP_CALLS-> /api/v1/things/{}edge.