Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix too lax application/json prefix-only matching #6180

Merged
merged 9 commits into from
Nov 3, 2021

Conversation

scop
Copy link
Contributor

@scop scop commented Oct 31, 2021

What do these changes do?

Fix too lax application/json matching.

For example, application/jsonfoo or application/json-seq should not be treated as valid for JSON.

Are there changes in behavior for the user?

Related issue number

Fixes #5896

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

@scop scop requested a review from asvetlov as a code owner October 31, 2021 06:08
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 31, 2021
@codecov
Copy link

codecov bot commented Oct 31, 2021

Codecov Report

Merging #6180 (7bc72c7) into master (c408ca1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 7bc72c7 differs from pull request most recent head 434c6c5. Consider uploading reports for the commit 434c6c5 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6180   +/-   ##
=======================================
  Coverage   93.29%   93.30%           
=======================================
  Files         102      103    +1     
  Lines       30351    30362   +11     
  Branches     2729     2729           
=======================================
+ Hits        28315    28328   +13     
+ Misses       1858     1857    -1     
+ Partials      178      177    -1     
Flag Coverage Δ
unit 93.22% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
aiohttp/client_reqrep.py 98.02% <100.00%> (-0.01%) ⬇️
aiohttp/helpers.py 96.92% <100.00%> (ø)
aiohttp/web_request.py 95.90% <100.00%> (-0.01%) ⬇️
tests/test_helpers.py 98.89% <100.00%> (+<0.01%) ⬆️
tests/conftest.py 66.66% <0.00%> (-1.71%) ⬇️
aiohttp/__init__.py 100.00% <0.00%> (ø)
tests/test___all__.py 100.00% <0.00%> (ø)
tests/test_worker.py 22.44% <0.00%> (+2.04%) ⬆️

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 c408ca1...434c6c5. Read the comment docs.

@scop
Copy link
Contributor Author

scop commented Oct 31, 2021

Note that this will conflict with #6181, will fix the other after one of these is merged.

@asvetlov
Copy link
Member

#6181 is merged, please fix conflicts

@scop scop force-pushed the not-just-prefix-json branch from 382c326 to 1fbddb6 Compare November 1, 2021 15:33
@scop
Copy link
Contributor Author

scop commented Nov 1, 2021

Done/rebased.

E.g. application/jsonfoobar is not expected for it, but ones with
parameters -- for example charset -- are. The IANA registered
application/json-seq is a good example.
@scop scop force-pushed the not-just-prefix-json branch from 1fbddb6 to 4a4dc9f Compare November 1, 2021 15:35
tests/test_helpers.py Outdated Show resolved Hide resolved
tests/test_helpers.py Outdated Show resolved Hide resolved
CHANGES/6180.bugfix Outdated Show resolved Hide resolved
Copy link
Member

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you please replace resp.headers['Content-Type'] check with resp.content_type?
It can simplify things by dropping parameters support (e.g. charset) from regexp.

@scop
Copy link
Contributor Author

scop commented Nov 2, 2021

Done.

@webknjaz
Copy link
Member

webknjaz commented Nov 3, 2021

I don't have any additional comments beyond those few nitpicks so I'll let @asvetlov decide when to merge this.

scop and others added 2 commits November 3, 2021 07:27
Co-authored-by: Sviatoslav Sydorenko <[email protected]>
Co-authored-by: Sviatoslav Sydorenko <[email protected]>
@asvetlov
Copy link
Member

asvetlov commented Nov 3, 2021

LGTM

@asvetlov asvetlov enabled auto-merge (squash) November 3, 2021 18:52
@asvetlov asvetlov merged commit f5a1734 into aio-libs:master Nov 3, 2021
@scop scop deleted the not-just-prefix-json branch November 4, 2021 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Any media type starting with application/json treated as JSON
3 participants