Python3: Accommodate both highcase and lowcase for response header key #25039
Conversation
wpt-pr-bot
requested review from
andreastt,
AutomatedTester,
mjzffr,
shs96c and
whimboo
| 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.
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.
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...
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
There was a problem hiding this comment.
Original discussion appears to be here: #14074
There was a problem hiding this comment.
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.
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.
Playing with this in #25680
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.