Python: fix: preserve URL query parameters in DefaultHttpRequestHandler - #7765
Conversation
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds regression coverage and updates the default HTTP request handler to preserve any query string already present in HttpRequestInfo.url when appending query_parameters.
Changes:
- Added an async regression test ensuring URL query strings are preserved and
query_parametersare appended. - Updated the handler to build the final request URL manually (instead of using
httpx’sparams=) to avoid replacing an existing query string.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| python/packages/declarative/tests/test_default_http_request_handler.py | Adds regression test validating preservation of URL-embedded query params when appending query_parameters. |
| python/packages/declarative/agent_framework_declarative/_workflows/_http_handler.py | Changes URL construction to append encoded query_parameters without dropping an existing query string. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
agent-framework-automation
Bot
added
the
python
GitHub Actions
Inactive
The declarative handler passed query_parameters to httpx's params= kwarg, which replaces any query string already present in the URL. URL-embedded parameters (api-version, tenant, ...) were silently dropped when combined with query_parameters. Append encoded query_parameters to the URL's existing query using the same rules as .NET DefaultHttpRequestHandler.ResolveRequestUri: keep existing parameters, use '&' if the URL already contains '?', skip empty keys, and percent-encode via quote_via=quote so spaces encode as %20 (matching Uri.EscapeDataString) rather than form-encoded '+'. The URL is left unchanged when query_parameters is empty or contains only empty keys. Fixes microsoft#7749 (Python side; .NET was already correct)
7bcb9cc to
0edb412
Compare
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
GitHub Actions
Inactive
…equestHandler Address moonbox3 review on microsoft#7765: passing params= or omitting it in client.request lets the httpx AsyncClient's params= replace the URL's raw query, so embedding query_parameters in the URL could silently drop the URL-embedded query whenever a caller's AsyncClient(params=...) was supplied. Collect client params, URL-embedded params, and query_parameters into a single explicit params= list (client < URL < query_parameters precedence, with later sources overriding duplicates) and pass that to client.request. URLunsplit strips the raw query from the URL so it is not double-applied, and urlsplit keeps the final query before any #fragment. Regression test: three-source merge preserved; duplicate key overridden; empty keys dropped. Fixes microsoft#7749 review feedback
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
|
|
||
| _merge_layer(list(client.params.multi_items())) | ||
| overridden.update(merged) | ||
| _merge_layer(list(httpx.URL(url).params.multi_items())) |
There was a problem hiding this comment.
Following up on the earlier report: this commit preserves client defaults, but could we avoid round-tripping the URL's existing query through httpx.URL.params? Even when client.params and query_parameters are empty, ?term=a%20b&download is sent as ?term=a+b&download=, and interleaved pairs are reordered. That can invalidate presigned URLs or change server interpretation, despite the requirement that an otherwise untouched URL remain unchanged. Could we retain raw.query verbatim and append only the separately encoded additions?
There was a problem hiding this comment.
Both concerns confirmed against httpx 0.28.1 — and they're coupled, so wanted to put the options in front of you before writing code:
What I verified
URL(u)parse alone is byte-preserving; anyURL(u, params=…)re-encodes throughQueryParams, which decodes%20→+, expandsdownload→download=, and normalises pair order.copy_with(raw_path=…)bypasses this channel and is verbatim.Client.request/build_request/sendall route throughClient._merge_queryparams. Empirically: any params value (emptyQueryParams(), empty dict) is falsy, so whenclient.paramsis non-empty, the merge fires no matter what request-level params are passed — the final URL always ends up re-encoded throughQueryParamsunless the call goes throughparams=None.Client._merge_queryparamstherefore has three controllable call shapes only:client.request(..., params=<list-as-strings>)— merged, but re-encoded (what Adding Microsoft SECURITY.MD #2 complains about)client.request(..., params=None)— client params still merged in (still re-encoded)- bypass
client.requestentirely:client.send(httpx.Request(method, url_str, headers=..., content=...))— URL is verbatim (no params merge fires)
Fix shapes I can see
- (a)
params=<merged list>accepting re-encode of the whole query (current state after d81257a). Handles all three sources; comment 1's?term=a%20b&downloadround-trip remains. Also has the duplicate-key semantic moonbox3 flagged in Adding Microsoft SECURITY.MD #2. - (b) Verbatim URL: build the URL string first by appending urlencoded(client.params MINUS url-keyed collisions) + urlencoded(query_parameters) after the raw query via urlsplit/urlunsplit, then call without
params=. Matches .NETResolveRequestUribyte-for-byte. To avoid_merge_queryparamsre-introducing client.params on the dispatch side, either:- (b1) use
httpx.Request+client.send(request, timeout=...)— verbatim and correct but loses client-level headers/cookies/auth merges (we already rely on none of these on the agent-side call path, though a caller-builtAsyncClient(headers=…, cookies=…, auth=…)would see a silent behaviour change), or - (b2) temporarily clear
client.paramsunder a lock at the send site (thread-safe but still not async-safe across two concurrent sends on the same client — two overlapping sends can leave the client's params stripped).
- (b1) use
- (c) Hybrid: HTTPX stays in control for clients where
client.paramsis empty (99% of devui users — preserves headers/cookies/auth merges, accepts the URL round-trip), switch tohttpx.Request+client.sendonly whenclient.paramsis non-empty (accepting the header/cookies/auth loss for that corner).
Tradeoffs
- Comment 2 ("URL duplicate keys must survive along with appended
query_parameters") is trivially fixable in (a) by treatingquery_parametersas append-only over the URL layer; but (a) can't address comment 1. - Comment 1 requires dropping
params=at the call site; the only lossless options then are (b1) / (c). (b2) is unsafe.
Which do you want? My lean is (c), falling back to (b1) if you'd rather keep it simple and accept the small behaviour drift for client-configured headers/cookies/auth.
(Also flagging: when client.params is non-empty, client.request(params=None) re-encodes the URL even today on main, before this PR — so strictly, comment 1's ?term=a%20b → ?term=a+b regression is only about the URL+query_parameters path when no client params are set; if you'd like me to narrow the scope and not fix the no-client-params case to verbatim, happy to take that route too.)
| def _merge_layer(pairs: list[tuple[str, str]]) -> None: | ||
| seen: set[str] = set() | ||
| for key, value in pairs: | ||
| if key in overridden and key not in seen: |
There was a problem hiding this comment.
What happens when the same key is already present in the URL? At _http_handler.py:195, the first explicit occurrence resets the list, so ...?filter=region&filter=status plus query_parameters={"filter": "tenant"} sends only filter=tenant. The issue and .NET implementation preserve the existing values and append the new pair. Could the client-default override happen separately, then treat the URL and query_parameters as one request-level list?
There was a problem hiding this comment.
Depends on which fix shape you pick on the other thread (verbatim-URL reply directly above — the one about
?term=a%20b round-tripping through QueryParams). In (a) I can switch query_parameters to append-only to match .NET exactly (client params still override URL keys when they collide, per the old _merge_queryparams behaviour). In (b) / (c) the URL stays byte-identical and query_parameters lands as a separate appended delta anyway, so duplicate keys just work. Happy to proceed once the main choice lands.
Motivation & Context
When a declarative HTTP action supplies both a URL already containing query parameters and additional
query_parameters, the URL-embedded parameters were silently dropped.https://api.example.test/items?api-version=2025-01-01&tenant=alphaplusquery_parameters={page: 2}would send only?page=2. This breaks any request that combines a pre-built URL with additional parameters — the issue case (api-version / tenant) is the canonical example of a request silently hitting the wrong endpoint.Description & Review Guide
In
DefaultHttpRequestHandler.send()(packages/declarative/_workflows/_http_handler.py), the handler passedquery_parametersstraight through to httpx'sparams=keyword argument, which replaces any query string already present in the URL. Build the final URL explicitly instead.The implementation mirrors the .NET
DefaultHttpRequestHandler.ResolveRequestUribehavior exactly:query_parametersafter existing URL query with&, or start a new query with?if none.quote_via=quoteso spaces become%20(matchingUri.EscapeDataString), not form-encoded+. Insertion order is preserved (no sorting), matching .NET's dictionary enumeration.No change to .NET — its handler already preserves the URL query string.
_http_handler.py+ one regression testtest_query_parameters_preserve_url_query_stringcovering api-version + tenant + addedpage.query_parametersnow retain both, matching .NET parity. No behavior change when either side is empty.%20vs+) match the project's cross-language parity expectations.Related Issue
Fixes #7749 (Python side; .NET was already correct, per the issue)
Contribution Checklist