fix(connectors): don't panic on empty/slash-less relative URI templates#9790
fix(connectors): don't panic on empty/slash-less relative URI templates#9790aaronArinder wants to merge 2 commits into
Conversation
http 1.4.2 tightened PathAndQuery validation: it now rejects an empty
string outright and requires a leading `/` on any non-empty,
non-query-only relative path — whereas 1.4.0 (currently locked)
silently normalized both cases (empty -> "/", "users" stayed "users"
and was later joined correctly by make_uri's own slash-handling
logic).
Two call sites relied on the old lenient behavior:
- http_json_transport.rs: PathAndQuery::from_static("") panicked for
the empty-path/empty-query case. Now uses "/" directly, matching
what the old version already normalized to internally.
- string_template.rs: interpolate_uri's relative-URI branch passed
the raw interpolated string straight to PathAndQuery::from_str,
which is what actually broke on both an empty connect/source
template and a connect_template without a leading slash (e.g.
"hello" instead of "/hello"). Normalize before parsing instead.
Verified against http 1.4.2 directly (bumped Cargo.lock locally,
confirmed the panics reproduce without this fix and are resolved with
it, ran the full apollo-federation connectors suite and
apollo-router's plugins::connectors:: suite) before reverting
Cargo.lock back to the committed 1.4.0.
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: 0a567a7afbf53c6092b7b4f3 ✅ AI Style Review — No Changes DetectedNo MDX files were changed in this pull request. Review Log: View detailed log
|
This comment has been minimized.
This comment has been minimized.
goto-bus-stop
left a comment
There was a problem hiding this comment.
LGTM, I guess the tests should already cover this case as tests were failing on the http update PR!
|
Tick the box to add this pull request to the merge queue (same as
|
benjamn
left a comment
There was a problem hiding this comment.
Consider adding a test that fully pins the new normalization? The fix looks correct, but nothing currently guards it: the closest existing test (relative_uris, string_template.rs:648) only checks that a path-and-query exists, not what it normalizes to, so a slashless input like "path" could regress to the wrong value and the test would still pass. There's also no case at all for the empty-string input (the "" → "/" branch this PR adds). Suggestion: assert the concrete result for the two new cases, e.g. "" normalizes to / and "path" normalizes to /path.
Summary
http1.4.2 tightenedPathAndQueryvalidation: it now rejects an empty string outright and requires a leading/on any non-empty, non-query-only relative path.http1.4.0 (currently locked/shipped) silently normalized both — an empty string became"/", and a slash-less relative path like"hello"was accepted as-is and correctly joined later bymake_uri's own slash-handling logic.test_updated_cargo_depsnightly job panicking on every run once dependency resolution picked uphttp1.4.2 (see companion PR retiring that job) — but it's a real latent bug independent of that job: any router instance that happens to resolvehttp1.4.2+ (e.g. viacargo update, or onceCargo.toml'shttp = "1.4.0"range naturally advances) will panic building connector request URIs for the empty-path/empty-query case or any connect template lacking a leading/.http_json_transport.rs:PathAndQuery::from_static("")panicked for the empty-path/empty-query case. Now usesPathAndQuery::from_static("/")directly — exactly what 1.4.0 already normalized""to internally (verified:PathAndQuery::from_static("").as_str() == "/"under 1.4.0).string_template.rs:interpolate_uri's relative-URI branch passed the raw interpolated string straight toPathAndQuery::from_str, which is what actually broke on an empty connect/source template and on a connect template without a leading slash (e.g."hello"instead of"/hello"). Now normalizes before parsing.Verification
Cargo.lock'shttpto 1.4.2 locally and confirmed the original panics reproduce without this fix.http1.4.2):cargo test -p apollo-federation --lib connectors::→ 1512 passed, 0 failed (including the previously-panickingtest_make_uriandtest_interpolate_uricases).cargo test -p apollo-router --lib plugins::connectors::→ 107 passed, 0 failed.Cargo.lockback to the committedhttp = "1.4.0"and re-ran the same test modules — all still pass, confirming no behavior change under the currently-shipped dependency version.cargo fmt -p apollo-federation --checkclean.Test plan
test_make_uriandtest_interpolate_urisuites pass under bothhttp1.4.0 (current) and 1.4.2 (future)apollo-router'splugins::connectors::suite passes under both