-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Implement ETag support #5298
Implement ETag support #5298
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5298 +/- ##
==========================================
+ Coverage 97.17% 97.18% +0.01%
==========================================
Files 41 41
Lines 8768 8849 +81
Branches 1404 1421 +17
==========================================
+ Hits 8520 8600 +80
- Misses 130 131 +1
Partials 118 118
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
value=ETAG_ANY, | ||
) | ||
else: | ||
for match in LIST_QUOTED_ETAG_RE.finditer(etag_header): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It accepts also garbage like "1",spam"2"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you're right.
To solve this problem I rejected the LIST_QUOTED_ETAG_RE
approach, and switched to split
+ re.fullmatch
.
Now only "1"
would be matched from proposed garbage string. I've checked nginx
behaviour and it's consistent with suggested solution. nginx
matches etag values from header until first invalid is found. New approach is also 10x faster than old.
Main disadvantage is that memory consumption will probably increase though, but I don't think that really long If-Match, If-None-Match
headers are so widespreaded.
if etag_header == ETAG_ANY: | ||
yield ETag( | ||
is_weak=False, | ||
value=ETAG_ANY, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is some inconsistency here. If ETag is "*"
, the value attribute will contain quotes. In all other cases they will be stripped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite. *
is a kind of special value, which is used without quotes. So if someone somehow will use "*"
instead of *
, it will be considered as regular etag.
Unfortunatly ETag(value='*')
produced from "*"
is indistinguishable from one produced from *
.
So, for example:
"*", "tag", "other-tag" -> (ETag(value='*'), ETag(value='other-tag'))
* -> (ETag(value='*'), )
Maybe it's better to create separate type ETAG_ANY
and change Optional[Tuple[ETag, ...]]
to Optional[Union[ETAG_ANY, Tuple[ETag, ...]]]
"*", "tag", "other-tag" -> (ETag(value='*'), ETag(value='other-tag'))
* -> ETAG_ANY
@asvetlov what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right.
@serhiy-storchaka thank you very much for the review! @greshilov please address comments, they are very valuable. |
0a01987
to
b66aad1
Compare
@serhiy-storchaka, @asvetlov thanks for the review! I appreciate your help. I marked conversations as resolved where I used proposed solution without any changes, sorry if I shouldn't have done it by myself. |
aiohttp/__init__.py
Outdated
from .helpers import ( | ||
BasicAuth as BasicAuth, | ||
ChainMapProxy as ChainMapProxy, | ||
ETag as ETag, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not simply from .helpers import BasicAuth, ChainMapProxy, ETag
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's a kind of outer interface protection measures. Andrew can tell for sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we have no policy for such cases.
Usually, we use as
only for importing under a different name if there is a conflict.
Please feel free to drop as
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if etag_header == ETAG_ANY: | ||
yield ETag( | ||
is_weak=False, | ||
value=ETAG_ANY, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right.
aiohttp/web_request.py
Outdated
value=ETAG_ANY, | ||
) | ||
else: | ||
for raw_etag in etag_header.split(","): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if ETag contains comma? "1,2"
Actually the approach with LIST_QUOTED_ETAG_RE was not bad. You need just add |(.)
at the end of the regexp. That group will be matched only if there is a garbage, so you can check it and raise an exception.
LIST_QUOTED_ETAG_RE = re.compile(fr"{_QUOTED_ETAG}(?:\s*,\s*|$)|(.)")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, completely forgot about this case while thinking about optimizations. Tests were updated to reflect this situation.
Thanks for the smart solution!
aiohttp/__init__.py
Outdated
from .helpers import ( | ||
BasicAuth, | ||
ChainMapProxy, | ||
ETag, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from .helpers import ( | |
BasicAuth, | |
ChainMapProxy, | |
ETag, | |
) | |
from .helpers import BasicAuth, ChainMapProxy, ETag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
c4b70ef
to
5f0ca5e
Compare
I also have to mention, that according to rfc7232 |
Agree |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Sorry for delay in the review
@greshilov please merge. Please edit the commit message first: press "Squash and merge" -> edit the text to cleanup meaningless records about intermediate commits -> press "Confirm squash and merge". |
Unfortunately, github doesn't show me this button. It says only users with write access can do this. But I can squash commits manually before merge, if needed! |
Co-authored-by: Serhiy Storchaka <[email protected]>
Co-authored-by: Serhiy Storchaka <[email protected]>
@webknjaz, @Dreamsorcerer can you help me with squash/merging this? Andrew approved this PR but he has no spare time now. I'm also planning to do a backport to 3.8 this week. |
@@ -38,7 +38,7 @@ | |||
) | |||
from .cookiejar import CookieJar as CookieJar, DummyCookieJar as DummyCookieJar | |||
from .formdata import FormData as FormData | |||
from .helpers import BasicAuth as BasicAuth, ChainMapProxy as ChainMapProxy | |||
from .helpers import BasicAuth, ChainMapProxy, ETag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are the imports changed here?
I'm assuming the style in this file follows the no implicit imports approach from Mypy:
https://mypy.readthedocs.io/en/stable/command_line.html#cmdoption-mypy-no-implicit-reexport
Although a little odd that it's not enforced in the Mypy tests if that is the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those classes are already presented in the __all__
variable.
I agree about inconsistency with mypy
style flags.
This change was previously discussed, I'll tag you there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great example of why style/formatting changes should be separated from the functional ones: a tiny disagreement can block a whole lot of legitimate changes, plus this makes it harder to notice important behavior changes.
I'll just leave this here: https://mtlynch.io/code-review-love/.
else: | ||
return any(etag.value == etag_value for etag in etags if not etag.is_weak) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick, this could be less nested:
else: | |
return any(etag.value == etag_value for etag in etags if not etag.is_weak) | |
return any(etag.value == etag_value for etag in etags if not etag.is_weak) |
self.etag = etag_value # type: ignore | ||
self.last_modified = last_modified # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was agreed in other PRs to add specific error codes in brackets.
@@ -29,6 +29,7 @@ Dict | |||
Discord | |||
Django | |||
Dup | |||
ETag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm almost sure that the spellchecker is case-insensitive.
|
||
|
||
@pytest.mark.parametrize( | ||
"header,header_attr", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FTR it's usually better readable (though lesser known) to use iterables in parameterize:
"header,header_attr", | |
("header", "header_attr"), |
) | ||
def test_etag_invalid_value_set(invalid_value) -> None: | ||
resp = StreamResponse() | ||
with pytest.raises(ValueError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The best practice is to always include , match=r'...'
argument demonstrating the intended exception reason more precisely.
@greshilov I didn't do a thorough review but commented with a few generic advices in several places. None of them are critical and since Andrew has already approved the patch, I'm going to merge it now. Feel free to send improvements in follow-up PRs. |
@greshilov waiting for the backport to 3.8 now. |
@webknjaz sure, thank you! |
This change adds an `etag` property to the response object and `if_match`, `if_none_match` properties to the request object. Also, it implements ETag support in static routes and fixes a few bugs found along the way. Refs: * https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.26 * https://tools.ietf.org/html/rfc7232#section-2.3 * https://tools.ietf.org/html/rfc7232#section-6 PR aio-libs#5298 by @greshilov Resolves aio-libs#4594 Co-Authored-By: Serhiy Storchaka <[email protected]> Co-Authored-By: Andrew Svetlov <[email protected]>
This change adds an `etag` property to the response object and `if_match`, `if_none_match` properties to the request object. Also, it implements ETag support in static routes and fixes a few bugs found along the way. Refs: * https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.26 * https://tools.ietf.org/html/rfc7232#section-2.3 * https://tools.ietf.org/html/rfc7232#section-6 PR #5298 by @greshilov Resolves #4594 Co-Authored-By: Serhiy Storchaka <[email protected]> Co-Authored-By: Andrew Svetlov <[email protected]> Co-authored-by: Serhiy Storchaka <[email protected]> Co-authored-by: Andrew Svetlov <[email protected]>
This change adds an `etag` property to the response object and `if_match`, `if_none_match` properties to the request object. Also, it implements ETag support in static routes and fixes a few bugs found along the way. Refs: * https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.26 * https://tools.ietf.org/html/rfc7232#section-2.3 * https://tools.ietf.org/html/rfc7232#section-6 PR aio-libs#5298 by @greshilov Resolves aio-libs#4594 Co-authored-by: Serhiy Storchaka <[email protected]> Co-authored-by: Andrew Svetlov <[email protected]>
This change adds an `etag` property to the response object and `if_match`, `if_none_match` properties to the request object. Also, it implements ETag support in static routes and fixes a few bugs found along the way. Refs: * https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.26 * https://tools.ietf.org/html/rfc7232#section-2.3 * https://tools.ietf.org/html/rfc7232#section-6 PR aio-libs#5298 by @greshilov Resolves aio-libs#4594 Co-authored-by: Serhiy Storchaka <[email protected]> Co-authored-by: Andrew Svetlov <[email protected]>
What do these changes do?
This PR adds
etag
property to the response object andif_match
,if_none_match
properties to the request object. Also it implementsETag
support in static routes and fixes few bugs found along the way.Despite the low priority of static files handling for
aiohttp
, I believeETag
and related headers has a wide application beyond this purpose.Are there changes in behavior for the user?
Nope, no general changes in server behaviour.
Related issue number
Fixes #4594
Checklist
CONTRIBUTORS.txt
CHANGES
folder<issue_id>.<type>
for example (588.bugfix)issue_id
change it to the pr id after creating the pr.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.This change is