Fix inconsistency between Python and C http request parsers. by serhiy-storchaka · Pull Request #4973 · aio-libs/aiohttp

serhiy-storchaka · 2020-09-25T06:44:29Z

They both now correctly parse URLs containing pct-encoded sequences and work with yarl 1.6.0.

~~Yarl >= 1.6 is now required.~~

They both now correctly parse URLs containing pct-encoded sequences and work with yarl 1.6.0. Yarl >= 1.6 is now required.

webknjaz · 2020-09-25T13:48:49Z

@serhiy-storchaka looks like the CI failures are caused by this change.

codecov-commenter · 2020-09-26T08:28:18Z

Codecov Report

Merging #4973 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4973   +/-   ##
=======================================
  Coverage   97.61%   97.61%           
=======================================
  Files          43       43           
  Lines        8933     8936    +3     
  Branches     1406     1405    -1     
=======================================
+ Hits         8720     8723    +3     
  Misses         95       95           
  Partials      118      118

Flag	Coverage Δ
#unit	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiohttp/web_urldispatcher.py	`99.24% <100.00%> (+0.15%)`	⬆️
aiohttp/http_parser.py	`96.42% <0.00%> (-0.23%)`	⬇️
aiohttp/web_protocol.py	`92.69% <0.00%> (-0.05%)`	⬇️
aiohttp/tracing.py	`100.00% <0.00%> (ø)`
aiohttp/resolver.py	`100.00% <0.00%> (ø)`
aiohttp/multipart.py	`96.27% <0.00%> (ø)`
aiohttp/web_routedef.py	`100.00% <0.00%> (ø)`
aiohttp/web_exceptions.py	`99.52% <0.00%> (ø)`
aiohttp/client_exceptions.py	`100.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55e6497...adc55f4. Read the comment docs.

aiohttp/web_urldispatcher.py

tests/test_http_parser.py

Co-authored-by: Sviatoslav Sydorenko <[email protected]>

serhiy-storchaka · 2020-09-26T10:07:33Z

Now there is a question: do we want to support yarl < 1.6 or require yarl >= 1.6?

tests/test_http_parser.py

webknjaz · 2020-09-26T10:50:56Z

Now there is a question: do we want to support yarl < 1.6 or require yarl >= 1.6?

I'm okay with doing >= 1.6 for master (future 4.0) but the backport may be a bit more tricky: what if the end-users are stuck with old yarl for some reason? OTOH since it fixes a bug, it'd be okay to increase the required version if it's needed for the bugfix.

webknjaz · 2020-09-26T10:53:26Z

re: CI failures -- test_close[pyloop] is the only test that fails with Python 3.8 under Win/Mac but it also fails on master so it's unrelated to this PR.

mwhudson · 2020-10-12T03:42:49Z

Any update on this?

TBBle · 2020-10-12T06:58:04Z

What about this fix required yarl 1.6? My understanding of the yarl change is that if we just ensure we specify encoded=True for all calls of URL.build (and with_xxx and operator /, but I don't think they're used here...), then that would be compatible with yarl 1.5 as well.

The core issue as I understood it was that we were passing encoded elements to URL.build, which defaults encoded=False, and it worked before yarl 1.6 because it was ignoring the % and there were no other characters that needed re-encoding (because we'd already encoded it).

aiohttp/web_urldispatcher.py

codecov-io · 2020-10-16T10:07:58Z

Codecov Report

Merging #4973 into master will increase coverage by 0.51%.
The diff coverage is 85.18%.

@@            Coverage Diff             @@
##           master    #4973      +/-   ##
==========================================
+ Coverage   97.10%   97.62%   +0.51%     
==========================================
  Files          43       43              
  Lines        8996     9007      +11     
  Branches     1413     1414       +1     
==========================================
+ Hits         8736     8793      +57     
+ Misses        144      100      -44     
+ Partials      116      114       -2

Flag	Coverage Δ
#unit	`97.62% <85.18%> (+0.51%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiohttp/web_urldispatcher.py	`97.83% <85.18%> (-0.41%)`	⬇️
aiohttp/helpers.py	`96.71% <0.00%> (+0.23%)`	⬆️
aiohttp/web_protocol.py	`92.73% <0.00%> (+0.29%)`	⬆️
aiohttp/test_utils.py	`99.68% <0.00%> (+0.62%)`	⬆️
aiohttp/http_websocket.py	`98.64% <0.00%> (+0.81%)`	⬆️
aiohttp/http_parser.py	`96.69% <0.00%> (+1.32%)`	⬆️
aiohttp/http_writer.py	`99.08% <0.00%> (+1.83%)`	⬆️
aiohttp/connector.py	`96.80% <0.00%> (+1.91%)`	⬆️
aiohttp/frozenlist.py	`97.91% <0.00%> (+2.08%)`	⬆️
aiohttp/pytest_plugin.py	`97.51% <0.00%> (+4.96%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 967b1b9...50945e2. Read the comment docs.

serhiy-storchaka · 2020-10-19T11:46:00Z

It now works also with yarl <1.6.

asvetlov

LGTM

asvetlov · 2020-10-21T06:15:13Z

Thanks, @serhiy-storchaka

They both now correctly parse URLs containing pct-encoded sequences and work with yarl 1.6.0. Co-authored-by: Sviatoslav Sydorenko <[email protected]> Co-authored-by: Andrew Svetlov <[email protected]>

aio-libs-github-bot · 2020-10-21T06:15:20Z

💚 Backport successful

The PR was backported to the following branches:

✅ 3.7

…4973) (#5097) Backports the following commits to 3.7: - Fix inconsistency between Python and C http request parsers. (#4973) Co-authored-by: Serhiy Storchaka <[email protected]>

Fix inconsistency between Python and C http request parsers.

4a45826

They both now correctly parse URLs containing pct-encoded sequences and work with yarl 1.6.0. Yarl >= 1.6 is now required.

serhiy-storchaka requested review from asvetlov and webknjaz as code owners September 25, 2020 06:44

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Sep 25, 2020

Fix web_urldispatcher.py.

19b7c54

webknjaz reviewed Sep 26, 2020

View reviewed changes

aiohttp/web_urldispatcher.py Outdated Show resolved Hide resolved

webknjaz reviewed Sep 26, 2020

View reviewed changes

tests/test_http_parser.py Outdated Show resolved Hide resolved

serhiy-storchaka and others added 3 commits September 26, 2020 12:25

Add new tests.

8beb8a2

Update tests/test_http_parser.py

5c8b948

Co-authored-by: Sviatoslav Sydorenko <[email protected]>

Optimize _requote_path().

5d4828c

webknjaz reviewed Sep 26, 2020

View reviewed changes

tests/test_http_parser.py Show resolved Hide resolved

webknjaz mentioned this pull request Oct 2, 2020

Patch update to older branches (2.0, 3.6, etc), to address yarl API-breaking minor update #4987

Closed

TBBle reviewed Oct 12, 2020

View reviewed changes

aiohttp/web_urldispatcher.py Outdated Show resolved Hide resolved

TBBle reviewed Oct 12, 2020

View reviewed changes

aiohttp/web_urldispatcher.py Show resolved Hide resolved

serhiy-storchaka and others added 4 commits October 12, 2020 12:09

Merge branch 'master' into ss/http-parser-yarl-1.6

6dace12

Address review comments.

c996e4e

Merge branch 'master' into ss/http-parser-yarl-1.6

adc55f4

Remove test_parse_uri_non_utf8.

516e91b

asvetlov added the backport-3.7 label Oct 16, 2020

serhiy-storchaka added 2 commits October 19, 2020 13:55

Merge branch 'master' into ss/http-parser-yarl-1.6

774b3cd

Support yarl <1.6.

b2183ac

serhiy-storchaka and others added 2 commits October 19, 2020 22:53

Merge branch 'master' into ss/http-parser-yarl-1.6

4b845ac

Merge branch 'master' into ss/http-parser-yarl-1.6

50945e2

asvetlov approved these changes Oct 21, 2020

View reviewed changes

asvetlov merged commit d321923 into aio-libs:master Oct 21, 2020

aio-libs-github-bot bot mentioned this pull request Oct 21, 2020

[3.7] Fix inconsistency between Python and C http request parsers. (#4973) #5097

Merged

KingDarBoja mentioned this pull request Nov 7, 2020

Update aiohttp from ==3.7.1 to >=3.7.1 graphql-python/gql#166

Merged

Uh oh!

Conversation

serhiy-storchaka commented Sep 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webknjaz commented Sep 25, 2020

Uh oh!

codecov-commenter commented Sep 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

serhiy-storchaka commented Sep 26, 2020

Uh oh!

Uh oh!

webknjaz commented Sep 26, 2020

Uh oh!

webknjaz commented Sep 26, 2020

Uh oh!

mwhudson commented Oct 12, 2020

Uh oh!

TBBle commented Oct 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-io commented Oct 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

serhiy-storchaka commented Oct 19, 2020

Uh oh!

asvetlov left a comment

Choose a reason for hiding this comment

Uh oh!

asvetlov commented Oct 21, 2020

Uh oh!

aio-libs-github-bot bot commented Oct 21, 2020

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

serhiy-storchaka commented Sep 25, 2020 •

edited

Loading

codecov-commenter commented Sep 26, 2020 •

edited

Loading

TBBle commented Oct 12, 2020 •

edited

Loading

codecov-io commented Oct 16, 2020 •

edited

Loading