Skip to content

Correct annotation of headers parameter of HTTP event handlers #5854

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

Merged
merged 3 commits into from
Aug 6, 2021

Conversation

mthuurne
Copy link
Contributor

@mthuurne mthuurne commented Aug 6, 2021

The standard library implementation (I checked Python 3.8.10) passes http.client.HTTPMessage for the headers parameter, which does not implement Mapping[str, str]: the value type is Any instead of str and __iter__() is missing. Also HTTPMessage has a lot of methods that are not present in Mapping.

Please read the individual commit comments for more details.

While the documentation of `BaseHandler.http_error_default()` describes
the `hdrs` (`headers` in most other handlers) as "a mapping object with
the headers of the error", the implementation that is located in
`URLopener._open_generic_http()` will pass `response.msg` instead,
which is of type `http.client.HTTPMessage`.
When the standard library constructs `HTTPError`, it will
pass an `http.client.HTTPMessage`, which is a subclass of
`email.message.Message`. Picking the superclass for the
annotations gives users the flexibility to for example
the result of the `email.message_from_X()` functions.

The only thing unique to `HTTPMessage` is the undocumented
`getallmatchingheaders()` method, which is only called by
`http.server.CGIHTTPRequestHandler.run_cgi()`. That class
gets its headers from `http.client.parse_headers()` and not
from `HTTPError`, so I think it's safe to use `Message`
as the annotation.
@mthuurne mthuurne force-pushed the http-handler-headers branch from 3919b6a to 92bff52 Compare August 6, 2021 00:59
@github-actions

This comment has been minimized.

2 similar comments
@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 6, 2021

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@srittau srittau merged commit 9bba8a4 into python:master Aug 6, 2021
@mthuurne mthuurne deleted the http-handler-headers branch August 6, 2021 10:24
@asottile
Copy link
Contributor

fwiw, this makes it a little annoying to mock an HTTPError since you have to feed through the right mapping type instead of using a plain dict

@srittau
Copy link
Collaborator

srittau commented Dec 20, 2021

fwiw, this makes it a little annoying to mock an HTTPError since you have to feed through the right mapping type instead of using a plain dict

I actually had the same issue. The documentation just states:

headers
The HTTP response headers for the HTTP request that caused the HTTPError.

The stdlib always initializes HTTPError with a Message, so having additional guarantees is a nice bonus. On the other hand I can see the use case for initializing your own HTTPErrors with a plain dict. I wouldn't mind (partly) reverting this, but have no strong feelings.

@mthuurne
Copy link
Contributor Author

Maybe custom Protocol would be a solution? Reverting everything would just move problems to a different place, namely when you want to create HTTPErrors in request handlers.

@asottile
Copy link
Contributor

it wasn't too difficult to update for me, though email.message.Message doesn't have a terribly convenient way to construct from a dictionary that I could find so I ended up with:

def _msg(dct: Mapping[str, str]) -> email.message.Message:
    msg = email.message.Message()
    for k, v in dct.items():
        msg[k] = v
    return msg

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants