Python: fix: preserve URL query parameters in DefaultHttpRequestHandler

[Manjunath Janardhan (manjunathshiva)]

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=alpha plus query_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

DefaultHttpRequestHandler.send() passed query_parameters to httpx's params=, which replaces any
query already on the URL, so URL-embedded parameters were dropped. Review then found that every
variant which routes the query through httpx.QueryParams -- params=, or simply letting the
client's own AsyncClient.params merge run -- also rewrites the query it keeps.

• What are the major changes?

  Implemented as the revised (b1) shape chosen in review: build through client.build_request(),
  overwrite only the query portion of the request's raw_path, then client.send(). The URL's own
  query is never parsed and rebuilt, so it reaches the transport byte-for-byte.

  Measured on httpx 0.28.1, routing it through QueryParams rewrites %20 in a value as +,
  expands a bare download into download=, and reorders interleaved duplicates -- x=1&y=2&x=3
  goes out as x=1&x=3&y=2. Base64 and %3A round-trip unchanged, so a signature is not rewritten
  merely by being base64; a scheme that signs a value holding an encoded space is. With client params
  configured it is worse than a rewrite: build_request() with the query still on the URL returns
  just the client param, dropping the URL's query outright.

  Three query sources now compose with defined roles:

  • The URL's query is preserved verbatim, including duplicate keys, bare flags and existing
    percent-escapes.
  • query_parameters append rather than replace, so ?filter=region&filter=status plus
    {"filter": "tenant"} sends all three, matching ResolveRequestUri in .NET.
  • Client-level AsyncClient.params are defaults: a key applies only when neither request-level
    source mentions it.

  Composing bytes by hand means owning the escaping, so a query holding literal non-ASCII is
  percent-encoded rather than raising UnicodeEncodeError, which the previous params= path masked.
  The escape set leaves RFC 3986 sub-delims, :@/?, bracket key names and existing % escapes
  alone, so an already-valid query -- including a presigned signature -- is untouched.

• What is the impact of these changes?

  A declarative HTTP action combining a pre-built URL with query_parameters now sends every
  parameter, with the URL's portion unmodified. That matters for servers that distinguish download
  from download=, that read repeated keys positionally, or that verify a signature over a value
  containing an encoded space.

  Checked against a real Azure Blob SAS URL rather than asserted: it returns 200 both before and
  after this change, because its keys are unique so nothing reorders and its signature characters
  round-trip. An earlier draft of this description said the old behaviour could invalidate a
  presigned URL, which is too strong for that shape and is corrected here.

  Two observable behaviour changes, both requested in review:

  • On a colliding key, query_parameters no longer replaces the URL's value; both are sent. Anyone
    depending on the old override behaviour will see a different request.
  • A preserved query keeps %20 where it previously went out as +.

  The client contract is otherwise unchanged, and I verified rather than assumed it: client headers,
  cookies including path-scoped ones, and auth all still apply; timeout reaches
  request.extensions["timeout"] with no USE_CLIENT_DEFAULT sentinel leaking through; a client
  base_url still contributes its path; redirects behave the same because send() defaults
  follow_redirects and auth to USE_CLIENT_DEFAULT exactly as request() does; and a signing
  httpx.Auth flow sees the final composed target, because auth runs inside send() after the
  rewrite.

  Only the caller's own URL query is verbatim. Everything appended is escaped, so a
  query_parameters value or key containing & or = -- or a client default containing them --
  cannot introduce an extra query pair. That has its own test.

• What do you want reviewers to focus on?

  The breaking-change question. The checklist box below says this is not a breaking change, which was
  true of the first iteration but is arguable now that a colliding key appends instead of overriding.
  It is a bug fix toward .NET parity and it is what review asked for, so I have not changed the label
  or the title myself -- please say which you want.

  Second, the escape set in _encode_query. It is deliberately permissive so an already-valid query
  is untouched, and I checked it is a no-op against percent-escapes, +, bare flags, bracket keys,
  every sub-delim and a presigned signature. If you would rather it were stricter, that is a
  one-line change.

  Test coverage for the above: 1004 passing in poe test -P declarative, the changed file at 100%
  line coverage, poe syntax and poe typing clean across all five checkers. One caveat worth
  stating: of the new tests, test_client_param_applies_only_when_key_absent_from_both_sources also
  passes against the previous implementation, which happened to produce the same bytes for that
  input. It pins the defaults-only rule rather than acting as a regression guard.

Related Issue

Fixes #7749 (Python side; .NET was already correct, per the issue)

Contribution Checklist

• The code builds clean without any errors or warnings
• All unit tests pass, and I have added new tests where possible
• The PR follows the Contribution Guidelines
• This PR is linked to an issue and there is no other open PR for this issue (see Related Issue above).
• This is not a breaking change.

[Copilot]

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_parameters are appended.
• Updated the handler to build the final request URL manually (instead of using httpx’s params=) 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.

[github-actions]

Python Test Coverage Report •

File	Stmts	Miss	Cover	Missing
packages/declarative/agent_framework_declarative/_workflows
_http_handler.py	97	0	100%
TOTAL	47257	4364	90%

Python Unit Test Overview

Tests	Skipped	Failures	Errors	Time
9617	36 💤	0 ❌	0 🔥	2m 7s ⏱️

[Manjunath Janardhan (manjunathshiva)]

Evan Mattson (@moonbox3) Gentle ping — no rush, just following up on the options above. My lean is (c) (hybrid: HTTPX handles the common no-client-params path; fall back to httpx.Request + client.send when client.params is non-empty), but if you'd rather keep it simpler, (b1) works too. Happy to implement whichever you prefer whenever you get a chance to weigh in.

[Evan Mattson (moonbox3)]

Manjunath Janardhan (@manjunathshiva),

Thanks for investigating. Let’s use a revised (b1): build through client.build_request(), replace only the request URL's query with our composed raw query, then call client.send(). HTTPX supports modifying a built request, so we can preserve client configuration without the hybrid branch or shared-client mutation.

Preserve the original query bytes and append explicit parameters, including duplicate keys. Client defaults should apply only when that key is absent from both request-level sources. Pass timeout to build_request().

Please add raw-query assertions covering encoding, bare flags, and interleaved duplicates, plus coverage that client headers/cookies/auth and timeout survive when client params are configured. Keep the fragment and trailing-? tests, and update the current precedence test to retain both URL and explicit values while excluding the conflicting client default. Thanks for your help with this.

[Manjunath Janardhan (manjunathshiva)]

Thanks — revised (b1) is in, pushed as bd6c16d44. Point by point against your list:

• Build through client.build_request(), replace only the URL's query, then client.send(). Done.
  One wrinkle worth knowing: the build has to happen with the query already stripped. Left on the URL,
  build_request() runs the client's params merge, and on httpx 0.28.1 that returns just the client
  param -- ?term=a%20b&download&x=1 with params={"cd": "1"} becomes ?cd=1, dropping the URL's
  query outright rather than rewriting it. So the composed query is written over the built request's
  raw_path afterwards.
• Preserve the original query bytes; append explicit parameters, including duplicate keys. Done.
  The URL's query is never parsed, so duplicates, bare flags, escapes and ordering all survive, and
  query_parameters append after it.
• Client defaults only when the key is absent from both request-level sources. Done, with its own
  test covering all three cases in one request.
• Pass timeout to build_request(). Done; it lands in request.extensions["timeout"], and the
  client default does not leak a USE_CLIENT_DEFAULT sentinel through.
• Raw-query assertions for encoding, bare flags, interleaved duplicates. Done, asserted on
  raw_path rather than the decoded params view, with and without client params configured. I also
  read the request line off a real socket, since MockTransport never serialises the request.
• Client headers/cookies/auth and timeout survive when client params are configured. Done in one
  test. Also checked beyond your list, because moving off client.request() touches more than the
  query: path-scoped cookies match as before, a client base_url still contributes its path,
  redirects are unchanged, and a signing httpx.Auth flow sees the final composed target because auth
  runs inside send() after the rewrite.
• Keep the fragment and trailing-? tests. Kept, unmodified.
• Update the precedence test. Done; it now asserts the exact raw query
  /items?key=urlval&key=qpval, so both request-level values survive and the conflicting client
  default is excluded.

Two things the shape turned up that you should see rather than discover:

Composing bytes by hand means owning the escaping, and a URL query holding literal non-ASCII --
?q=café -- has no byte form for a request target. That raised UnicodeEncodeError where the old
params= path had quietly percent-encoded it. There is now a helper that escapes only what cannot
appear in a query and leaves sub-delims, :@/?, bracket key names and existing % escapes alone, so
an already-valid query stays byte-identical.

And a correction to my own PR description: I had written that the old behaviour "can invalidate a
presigned URL". I provisioned a storage account and tried a real Azure Blob SAS URL, and it returns
200 both before and after this change -- its keys are unique so nothing reorders, and base64 and
%3A round-trip through QueryParams unchanged. The claim was too strong and the description now
says so. What does break is a scheme signing a value that holds an encoded space: an S3-style
response-content-disposition=attachment%3B%20filename%3D%22a%20b.txt%22 came back as
...%3B+filename%3D%22a+b.txt%22 on the old path. That has its own test, and the SAS-shaped test is
kept as a guard with a docstring saying it passed beforehand too.

Two things I would rather you decide than assume:

The checklist on this PR still says it is not a breaking change. That was true of the first
iteration, but a colliding key now appends instead of overriding and a preserved query keeps %20
where it went out as +, so requests change for anyone relying on the old behaviour. Both are what
you asked for and both move toward .NET parity, so I have not touched the label or the title -- say
which you want and I will apply it.

And the escape set in _encode_query is deliberately permissive so an already-valid query is
untouched; I checked it is a no-op against percent-escapes, +, bare flags, bracket keys, every
sub-delim and a presigned signature. Happy to make it stricter if you would rather.

poe test -P declarative is 1004 passing with the changed file at 100% line coverage, poe syntax
and poe typing clean across all five checkers. Detail on the two inline threads.