fix(urls): preserve repeated query parameters#40877
Conversation
Code Review Agent Run #418e36Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40877 +/- ##
==========================================
- Coverage 64.14% 64.13% -0.02%
==========================================
Files 2652 2652
Lines 143488 143561 +73
Branches 33110 33123 +13
==========================================
+ Hits 92042 92071 +29
- Misses 49837 49876 +39
- Partials 1609 1614 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Superset uses Git pre-commit hooks courtesy of pre-commit. To install run the following: A series of checks will now run when you make a git commit. Alternatively it is possible to run pre-commit by running pre-commit manually: |
There was a problem hiding this comment.
Pull request overview
This PR fixes superset.utils.urls.modify_url_query to correctly preserve repeated query parameters when updating/adding URL query args, aligning URL serialization behavior with how Superset URLs can legitimately encode multi-valued parameters.
Changes:
- Replace manual query-string rebuilding (which collapsed repeated params) with
urllib.parse.urlencode(..., doseq=True). - Preserve existing encoding behavior by using
quote_via=urllib.parse.quote(spaces as%20) andsafe="/". - Add unit tests covering (1) preserving existing repeated parameters and (2) adding list values as repeated parameters.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
superset/utils/urls.py |
Switch query serialization to urlencode(..., doseq=True) to keep repeated parameters. |
tests/unit_tests/utils/urls_tests.py |
Add coverage for repeated existing params and list-valued params becoming repeated query args. |
eschutho
left a comment
eschutho
left a comment
There was a problem hiding this comment.
Summary
modify_url_query previously rebuilt the query string by taking only the first value (v[0]) from each parameter list, silently dropping all but the first value of repeated parameters like filter=a&filter=b. This PR replaces that manual serialization with urllib.parse.urlencode(..., doseq=True), which correctly expands multi-value lists into repeated key=value pairs. The quote_via=urllib.parse.quote, safe="/" arguments preserve the existing encoding behavior (spaces as %20, slashes unencoded). Two new unit tests cover the fixed behavior, and all callers in the codebase pass scalar kwargs so there's no backward-compatibility risk.
Findings
LOW
Docstring doesn't mention list-kwarg behavior (superset/utils/urls.py:53-55) — the fix makes passing a list a meaningful, supported input, but the docstring still just says "Replace or add parameters to a URL." WDYT about a one-liner addition like "Passing a list value produces repeated parameters (e.g., tag=['a','b'] → tag=a&tag=b)."? Would make the contract discoverable without reading the implementation.
Missing test for overwriting a multi-value param with a scalar — the existing tests cover overwriting a scalar param with another scalar; the new tests cover preserving and adding multi-value params. A case like modify_url_query("?filter=a&filter=b", filter="c") → ?filter=c would round out coverage and document the intent that kwargs replace rather than append. Totally optional since the behavior is obvious from params[k] = v, but could be handy for future readers.
LOW / NIT
Implicit string concatenation in test assertion (tests/unit_tests/utils/urls_tests.py:60-63) — the last test joins the expected string across two lines via implicit concatenation:
assert (
test_url
== "http://localhost:9000/explore/?existing=ok"
"&tag=alpha%20value&tag=beta/value"
)Not wrong at all, but could we put it on a single line (or use an explicit +) to stay consistent with the other test assertions in the file?
Overall
Approve — clean, well-scoped fix using the right stdlib tool; encoding behavior is preserved, all existing tests pass, and the new cases directly cover the reported bug.
johan-rocha
force-pushed
the
fix/modify-url-query-repeated-params
branch
from
d94d0f3 to
f93b46d
Compare
Code Review Agent Run #be50b9Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Preserve Repeated Query Parameters
SUMMARY
This PR fixes
modify_url_queryso repeated query parameters are preserved whenupdating or adding URL parameters.
Previously,
modify_url_queryparsed query parameters into lists usingparse_qs, but rebuilt the query string manually by taking only the first valueof each parameter. As a result, URLs containing repeated parameters such as:
could be incorrectly serialized, after adding another parameter, as:
This change replaces the manual query string serialization with
urllib.parse.urlencode(..., doseq=True), allowing multiple values for the samequery parameter to be correctly expanded and preserved.
The implementation keeps the existing URL encoding behavior by using
quote_via=urllib.parse.quoteandsafe="/", ensuring spaces are encoded as%20and path-like values containing/remain readable.Additional unit tests were added to cover:
Existing tests continue to cover scalar query parameter replacement.
TESTING INSTRUCTIONS
Run:
Or run only the tests related to this change:
ADDITIONAL INFORMATION