Drop lone surrogates that split a percent escape in the Cython quoter#1752
Open
rodrigobnogueira wants to merge 1 commit into
Open
Drop lone surrogates that split a percent escape in the Cython quoter#1752rodrigobnogueira wants to merge 1 commit into
rodrigobnogueira wants to merge 1 commit into
Conversation
psf-chronographer
Bot
added
the
bot:chronographer:provided
Merging this PR will improve performance by 20.37%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | test_long_query_with_pct |
3.4 ms | 2.7 ms | +28.3% |
| ⚡ | test_quoter_quote_utf8 |
129.7 µs | 114.9 µs | +12.94% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing rodrigobnogueira:fix/quoter-surrogate-percent-recombine (8f244e1) with master (a57cbee)
The Cython quoter scans a string code point by code point, so a lone surrogate landing inside a "%XX" escape broke it: the "%" was treated as a literal and re-encoded to "%25" while the hex digits were left in place (for example "%2", a lone surrogate, then "0" quoted to "%2520"). The pure-Python quoter drops lone surrogates with errors="ignore" before scanning, so it read the same input as "%20". Skip lone surrogates while reading the two hex digits so the C quoter recombines the escape the same way, restoring parity between the two backends.
rodrigobnogueira
force-pushed
the
fix/quoter-surrogate-percent-recombine
branch
from
dd4fcff to
8f244e1
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1752 +/- ##
==========================================
- Coverage 99.86% 99.81% -0.05%
==========================================
Files 21 21
Lines 4310 4319 +9
Branches 252 252
==========================================
+ Hits 4304 4311 +7
- Misses 4 5 +1
- Partials 2 3 +1
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:
|
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
1 participant
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.
What do these changes do?
The Cython quoter scans a string code point by code point, so a lone
surrogate that lands inside a
%XXescape broke it: the%wastreated as a literal and re-encoded to
%25while the hex digitswere left in place (for example
%2, a lone surrogate, then0quoted to
%2520). The pure-Python quoter drops lone surrogates witherrors="ignore"before it scans, so it read the same input as%20. The C quoter now skips lone surrogates while reading the twohex digits, so both backends recombine the escape identically.
This is the second of two lone-surrogate parity gaps between the
backends; #1751 handles the case where a lone surrogate is the only
character that changes.
Are there changes in behavior for the user?
Yes, on the compiled extension only: requoting a string where a lone
surrogate falls within a
%XXescape now drops the surrogate andkeeps the escape (matching the pure-Python build) instead of
percent-encoding the
%.Is it a substantial burden for the maintainers to support this?
No. It adds a small helper that skips lone surrogates in the requote
lookahead of
yarl/_quoting_c.pyx; the pure-Python quoter alreadybehaves this way, so there is no public API change.
Related issue number
No public issue; related to #1751.
Checklist
CONTRIBUTORS.txt(N/A; file not present in repo)CHANGES/folderTest details (optional, for reviewers)
Tests (both backends):
python -m pytest tests/test_quoting.py tests/test_url_parsing.py, and the same withYARL_NO_EXTENSIONS=1; all pass. The newtest_quoting.pycase runs under both thepy_quoterandc_quoterfixtures and fails on the unpatched C extension (%2520vs%20), confirming it catches the bug. A differential fuzz of the C and pure-Python quoters over surrogate-laden inputs across the built-in quoter configurations shows no remaining divergence in this class. Pre-commit (ruff, codespell, cython-lint) and a Cython line-tracing coverage build of the changed lines pass locally.