Repository navigation
🐛 Insert queryParameters before the fragment in RequestOptions.uri - #2614
Open
Yasser-Ameur wants to merge 1 commit into
Open
Yasser-Ameur wants to merge 1 commit into
Yasser-Ameur wants to merge 1 commit into
Conversation
Problem: when path carried a #fragment and queryParameters were also given, RequestOptions.uri appended the encoded query after the fragment instead of before it, so `?x=1` (or `&x=1`) became part of the fragment string and the server never received it as a query. Cause: the uri getter built url from path/baseUrl (which may already end in a fragment) and then string-concatenated the query onto whatever url already was, with no awareness of a trailing fragment. Change: split any #fragment off url before appending the query, then re-append the fragment after the query, so the component order matches scheme:authority/path?query#fragment as RFC 3986 Section 3 defines it. Every other case (no fragment, existing ?query merged with queryParameters, baseUrl joining, absolute paths, listFormat) is unaffected since the fragment is empty in those cases. Tests: options_test.dart gains a group of boundary-value assertions on the built Uri (fragment with/without queryParameters, existing ?query plus queryParameters, empty fragment, a literal ? inside the fragment, baseUrl+relative path with a fragment). A new uri_fragment_query_test.dart proves the fix end to end against an in-process HttpServer, asserting the server actually receives the query string. All new tests were confirmed to fail on unmodified main before the fix. Co-Authored-By: Claude <[email protected]>
CaiJingLong
approved these changes
Oct 1, 2026
CaiJingLong
left a comment
CaiJingLong
left a comment
Contributor
There was a problem hiding this comment.
The fragment is now separated before query parameters are appended, and reattached afterward. I found no blocking issue in the changed behavior.
Local verification in an isolated worktree at 774c81e, using Dart 3.13.5: 36 tests passed across options_test.dart, uri_fragment_query_test.dart, encoding_test.dart, and parameter_test.dart; analysis of lib/test and formatting of the changed Dart files were clean. The real-HttpServer regression fails against base 4684e29 because the server receives an empty query, then passes on this head.
GitHub's package-verification and CodeQL runs are still action_required; the full hosted matrix has not run. Review performed with OpenAI Codex assistance.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
New Pull Request Checklist
mainbranch to avoid conflicts (via merge from master or rebase)CHANGELOG.mdin the corresponding packageAdditional context and info (if any)
AI assistance, per AGENTS.md: Claude drafted the fix and the tests; I reviewed the diff, reran the suite, and wrote this description.
This defect was found while testing dio's URL construction, not reported by anyone.
RequestOptions.uriappended the encodedqueryParametersafter an existing#fragmentinpath, sodio.get('/page#section1', queryParameters: {'x': '1'})built.../page#section1?x=1. The query text landed inside the fragment and a receiving server never sawx=1as its query string. RFC 3986 Section 3 orders a URI asscheme:authority/path?query#fragment; the query must precede the fragment.The fix splits any
#fragmentoff the built URL before the query is appended, then re-appends the fragment after the query. One change site indio/lib/src/options.dart'surigetter. Every other case (no fragment, an existing?querymerged withqueryParameters,baseUrljoining, absolute-URL paths,listFormat) is unaffected because the split-off fragment is empty for them.Added a regression test (
dio/test/uri_fragment_query_test.dart) that proves the fix through a realdart:ioHttpServerreceiving the request, plus a boundary-value test group indio/test/options_test.dart(fragment with/without query, existing?queryplusqueryParameters, an empty fragment#, a fragment containing a literal?, andbaseUrl+ relative path + fragment). All new tests were confirmed to fail onmainbefore the fix and pass after.Locally:
dart format --set-exit-if-changedon the touched files,dart analyze --fatal-infos ., and the fulldio/VM test suite (dart test --preset=default,stable, 261 passed / 5 pre-existing unrelated skips) are all clean.This does not touch TLS, redirects, cookies, header handling, timeouts/cancellation, the interceptor pipeline, or request-body encoding, so no maintainer @-mention for the extra-scrutiny areas in AGENTS.md applies.