Document that querystring values are not unquoted twice - #4209
Conversation
|
the from_url docstrings still promise the old behaviour: "The username, password, hostname, path and all querystring values are passed through urllib.parse.unquote" (client.py:194, connection.py:2938, cluster.py:669, plus the three asyncio twins). querystring values are the one item in that list that no longer goes through unquote after this, so it should probably be dropped from the sentence. |
parse_qs already percent-decodes query values, so from_url does not pass them through unquote a second time. The docstrings still listed them alongside username, password, hostname and path.
6a5dbf3 to
abc7b06
Compare
|
You're right, and thanks for the precise pointers — I'd missed that the sentence wraps across two lines. Since #4222 landed the actual fix on master, I've narrowed this PR down to just the docs half, which is the part still outstanding. All six |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abc7b062aa
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
petyaslavova
left a comment
There was a problem hiding this comment.
Hey @winklemad, thank you for your contribution! The change LGTM.
Description of change
urllib.parse.parse_qsalready percent-decodes query values. Both the sync andasync URL parsers subsequently called
unquoteon those values, causingdoubly encoded data such as
%2520to become a space instead of the literal%20.Remove the redundant second decoding step in both implementations and add
mirrored sync/async regression tests proving query values are decoded exactly
once.
Fixes #4208
Pull Request check-list
Please make sure to review and check all of these items:
changed files, and full Vulture pass.
files on
master.NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.
Note
Low Risk
Doc-only changes to public API descriptions; no runtime behavior in the shown diff.
Overview
Updates
from_url/ConnectionPool.from_urldocstrings in sync and async client, cluster, and connection modules so they match actual URL parsing behavior.The docs no longer say querystring values are passed through
urllib.parse.unquote. They now state that username, password, hostname, and path are unquoted, while query parameters are decoded once byparse_qsand are not unquoted again—avoiding the misleading impression of double-decoding (e.g.%2520becoming a space instead of%20, per #4208).Reviewed by Cursor Bugbot for commit ed28558. Bugbot is set up for automated code reviews on this repo. Configure here.