Fix inconsistency between Python and C http request parsers. #4973

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 <wk.cvs.github@sydorenko.org.ua>

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 <wk.cvs.github@sydorenko.org.ua> Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>

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 <storchaka@gmail.com>

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 <wk.cvs.github@sydorenko.org.ua>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix inconsistency between Python and C http request parsers. #4973

Fix inconsistency between Python and C http request parsers. #4973

serhiy-storchaka commented Sep 25, 2020 •

edited

Loading

webknjaz commented Sep 25, 2020

codecov-commenter commented Sep 26, 2020 •

edited

Loading

serhiy-storchaka commented Sep 26, 2020

webknjaz commented Sep 26, 2020

webknjaz commented Sep 26, 2020

mwhudson commented Oct 12, 2020

TBBle commented Oct 12, 2020 •

edited

Loading

codecov-io commented Oct 16, 2020 •

edited

Loading

serhiy-storchaka commented Oct 19, 2020

asvetlov left a comment

asvetlov commented Oct 21, 2020

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

Fix inconsistency between Python and C http request parsers. #4973

Fix inconsistency between Python and C http request parsers. #4973

Conversation

serhiy-storchaka commented Sep 25, 2020 • edited Loading

webknjaz commented Sep 25, 2020

codecov-commenter commented Sep 26, 2020 • edited Loading

Codecov Report

serhiy-storchaka commented Sep 26, 2020

webknjaz commented Sep 26, 2020

webknjaz commented Sep 26, 2020

mwhudson commented Oct 12, 2020

TBBle commented Oct 12, 2020 • edited Loading

codecov-io commented Oct 16, 2020 • edited Loading

Codecov Report

serhiy-storchaka commented Oct 19, 2020

asvetlov left a comment

Choose a reason for hiding this comment

asvetlov commented Oct 21, 2020

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

💚 Backport successful

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