fix(passthrough): stop request params from clobbering merged target query params - #32404
Conversation
Greptile SummaryFixes a bug where
Confidence Score: 5/5Safe to merge — the change is a targeted fix to a broken feature with no side effects on endpoints that don't use merge_query_params or default_query_params. The logic is sound: requested_query_params is guaranteed to be a dict (never None) when it enters the merge block, the ordering of managed-ID rewrite before merge is now correct, and the null assignment after merge cleanly prevents httpx from overwriting the URL. No-merge endpoints are entirely unaffected. The four new wire-level tests directly demonstrate the previously-broken and now-fixed behaviors on the real request-building path. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/pass_through_endpoints/pass_through_endpoints.py | Fixes query-param merge bug: moves merge block after managed-ID rewrite, sets requested_query_params=None so httpx does not clobber the already-merged URL query string |
| tests/test_litellm/proxy/pass_through_endpoints/test_pass_through_endpoints.py | Adds four regression tests using httpx.MockTransport to assert the final wire URL; covers merge, default_query_params, no-merge, and managed-ID-rewrite-with-merge scenarios |
Reviews (2): Last reviewed commit: "fix(passthrough): rewrite managed ids in..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
…them into the URL
Relevant issues
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Screenshots / Proof of Fix
Live proxy, real Anthropic API (models list endpoint, no mocks). Config used, note the
?limit=1baked into the target plusmerge_query_params: true:Proxy started with
.venv/bin/python litellm/proxy/proxy_cli.py --config proof_config.yaml --host 127.0.0.1 --port 49413(withANTHROPIC_BASE_URLandANTHROPIC_AUTH_TOKENunset)Before the fix, at base commit db24027, the target's
limit=1never reaches Anthropic; the plain call returns the full default page and the paged call returns everything after the cursor:After the fix, at commit dd989af, the same curls show
limit=1surviving on the wire and merging with the client'safter_id:Type
🐛 Bug Fix
Changes
pass_through_requestfolds the target URL's own query params (and any configureddefault_query_params) into the outgoing URL whenmerge_query_paramsordefault_query_paramsis set, but then still passed the incoming request's query params to httpx viaparams=. httpx'sparams=replaces the URL's entire query string, so the merged query was clobbered down to just the incoming request's params and the target's own query never reached the wire. Becausedict(request.query_params)is{}rather thanNonewhen the client sends no params, even paramless requests stripped the target's query, somerge_query_paramshas effectively never worked on the wireThe fix computes the effective incoming params once (
query_params or dict(request.query_params), the same expression the wire previously used), folds them into the merged URL with highest precedence, and setsrequested_query_paramstoNoneso httpx sends the merged URL untouched. This matches the documenteddefault_query_paramssemantics (defaults sent with every request, overridable per key by the client) and also covers thedefault_query_params-only case, where incoming params previously were not folded into the merged URL at all. Endpoints with neither option keep their existing behavior of the incoming params replacing the target query. The streaming, multipart, and raw-body call sites all consume the samerequested_query_paramsvariable, so they are all fixed by this single changeThe existing test for this feature only inspected the
urlkwarg handed tohttpx.AsyncClient.request, before httpx appliesparams=, which is why it never caught the clobbering. The new regression tests assert the final wire URL instead, by injecting anhttpx.MockTransport-backed client into litellm's client cache so the real request-building path runs: merge preserves target plus incoming params, defaults reach the wire with per-key client overrides winning, and no-merge endpoints keep replace semantics. The first two tests fail on the base commit and pass with the fixThe follow-up commit fixes an ordering regression the first commit introduced on merge-enabled endpoints that use passthrough managed object IDs: the fold ran before the managed-ID input rewrite, so
rewrite_query_idsreceivedNonewhile the un-rewritten managed ID was already baked into the URL and would have leaked upstream. The fold-and-null now runs after the managed-ID rewrite block, so the rewritten query params are what gets merged into the URL; nothing in between depends on the URL's query string (endpoint type detection keys on host and route substrings). A new regression test drives a merge-enabled endpoint with a managed ID in a query param through a fakemanaged_fileshook injected via the sameproxy_logging_obj.get_proxy_hookseam production uses, and asserts the wire URL carries the rewritten raw ID together with the target's own params; it fails without the reordering. The test helper's cache-key lookup was also tightened per review tonext(..., None)plus an assert with an explanatory message instead of an opaqueStopIteration