Correct Accept-Encoding parsing for tokens and q-values#12390
Correct Accept-Encoding parsing for tokens and q-values#12390metsw24-max wants to merge 4 commits intoaio-libs:masterfrom
Conversation
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
So, this is a developer sending a response. I'm not sure the additional complexity and overhead here is worth the validation. It seems unlikely that a developer is going to make such a mistake (and not notice it when testing).
|
I simplified this. I removed the extra validation in the no-text constructor path and updated tests accordingly, so we no longer raise for header plus content_type or charset in that developer-facing case. The stricter conflict check when text is provided remains unchanged. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12390 +/- ##
==========================================
- Coverage 98.92% 98.90% -0.02%
==========================================
Files 134 134
Lines 46616 46660 +44
Branches 2429 2434 +5
==========================================
+ Hits 46114 46151 +37
- Misses 373 377 +4
- Partials 129 132 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Merging this PR will not alter performance
Comparing Footnotes
|
You appear to have removed an existing, cheap check, while leaving all your complex validation in. My comment still stands and I think there needs to be a strong argument for the inclusion of this code. |
What do these changes do?
Fix Accept-Encoding parsing to use exact token matching, properly handle comma-separated values, and respect q values. Adds a regression test.
Are there changes in behavior for the user?
Yes:
Invalid tokens (e.g., xgzip) no longer match valid encodings
Encodings with q=0 are ignored
Valid encodings work as expected
Is it a substantial burden for the maintainers to support this?
No. The logic is clearer and covered by tests, improving maintainability.