-
Notifications
You must be signed in to change notification settings - Fork 3.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
Python3: Accommodate both highcase and lowcase for response header key #25039
Conversation
if PY3: | ||
# For Firefox, the response header names are converted to lowercase strings for consistent lookups. | ||
# https://www.selenium.dev/selenium/docs/api/javascript/module/selenium-webdriver/lib/http_exports_Response.html | ||
try: | ||
assert 'Content-Type' in headers |
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.
So I understand properly; this has always been an 'issue' (spec-compliant issue, perhaps), with geckodriver. Back in py2, we didn't notice because HTTPResponse was normalizing the cases. In py3, HTTPResponse no longer normalizes the case so this fails for geckodriver?
I wonder if we should be normalizing headers in HTTPResponse then... cc @jgraham @Hexcles
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.
Sorry I missed this. Python 2 seems to normalize (lower) the keys when parsing and storing the HTTP message (I haven't checked the source code, but just an observation based on #22858 (comment)), while Python 3 in fact does case-insensitive lookup while preserving the casing, which arguably is a better approach:
https://github.com/python/cpython/blob/7bcc6456ad4704da9b287c8045768fa53961adc5/Lib/email/message.py#L462-L468
https://github.com/python/cpython/blob/7bcc6456ad4704da9b287c8045768fa53961adc5/Lib/email/message.py#L423-L424
It was really puzzling to me why #22858 was needed in the first place.
Turns out, we actually coerced headers
into a dict...
headers = dict(http_response.getheaders()) |
We might want to stop doing that, or at least use this class from wptserve
instead
wpt/tools/wptserve/wptserve/request.py
Line 373 in 42a4b17
class RequestHeaders(dict): |
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.
Original discussion appears to be here: #14074
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.
Robert and I chatted about this, and we think making Response.from_http
use a class like RequestHeaders
would be best, as it both solves the case sensitivity problem as well as supports multiple headers (which the current code doesn't do).
The downside is that it would introduce a dependency from webdriver to wptserve, or otherwise require code duplication. At this point I am leaning towards code duplication, though it would be nice to perhaps introduce a WPT 'utils' library that everything can depend on? (Generally speaking wpt is quite messy atm, with multiple 'subprojects' that are rarely standalone subprojects anymore.)
cc @jgraham and @gsnedders
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.
Actually, looks like RequestHeaders
assumes a fair amount about its input headers that isn't true for the usage in transport.py, where the headers are just a list of tuples. So the init code is slightly different at least.
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.
Playing with this in #25680
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.
Closes #25039 Co-authored-by: Robert Ma <robertma@chromium.org>
Closes #25039 Co-authored-by: Robert Ma <robertma@chromium.org> Co-authored-by: Robert Ma <robertma@chromium.org>
…atform-tests#25680) Closes web-platform-tests#25039 Co-authored-by: Robert Ma <robertma@chromium.org> Co-authored-by: Robert Ma <robertma@chromium.org>
…e insensitively, a=testonly Automatic update from web-platform-tests [WIP] Make webdriver client handle headers case insensitively (#25680) Closes web-platform-tests/wpt#25039 Co-authored-by: Robert Ma <robertma@chromium.org> Co-authored-by: Robert Ma <robertma@chromium.org> -- wpt-commits: 9da95c4ddd67ffc5010c1048250a7c67c0bee897 wpt-pr: 25680
…e insensitively, a=testonly Automatic update from web-platform-tests [WIP] Make webdriver client handle headers case insensitively (#25680) Closes web-platform-tests/wpt#25039 Co-authored-by: Robert Ma <robertma@chromium.org> Co-authored-by: Robert Ma <robertma@chromium.org> -- wpt-commits: 9da95c4ddd67ffc5010c1048250a7c67c0bee897 wpt-pr: 25680
No description provided.