Skip to content

Fix mutability issues with headers input types#7431

Merged
nateprewitt merged 1 commit into
mainfrom
header_input_vs_stored
May 13, 2026
Merged

Fix mutability issues with headers input types#7431
nateprewitt merged 1 commit into
mainfrom
header_input_vs_stored

Conversation

@nateprewitt
Copy link
Copy Markdown
Member

We already got a helpful email, lack of header mutability in the typing contract is creating friction for some code bases. I think we'd started to go down this route but it got lost in the cleanup. I'm going to stage this as a known issue for a 2.34.1 but we'll likely wait a day or two before cutting another release to see what other feedback we missed.

Comment thread src/requests/models.py
method: str | None
url: _t.UriType | None
headers: CaseInsensitiveDict[str] | Mapping[str, str | bytes] | None
headers: MutableMapping[str, str | bytes]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not headerstype here too?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

None was wrong. We always fallback to {} when you pass None into the constructor. The *Type aliases are all mapping input from the public API, this is representing what we store.

Otherwise you need to both validate headers is not None and Mutable. I made a similar change for status_code on Response.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I guess as a tangent to that, | None being baked into the types was an opinion. I don't feel super strongly about that, it was just for terseness and keeping the None requirement grouped. We could do HeadersType | None everywhere it's used now. That applies to most of the *Types.

@nateprewitt nateprewitt marked this pull request as ready for review May 12, 2026 18:01
@nateprewitt nateprewitt mentioned this pull request May 13, 2026
@nateprewitt nateprewitt requested a review from sigmavirus24 May 13, 2026 15:20
@nateprewitt nateprewitt merged commit e511bc7 into main May 13, 2026
85 of 86 checks passed
@nateprewitt nateprewitt deleted the header_input_vs_stored branch May 13, 2026 19:16
@bastimeyer
Copy link
Copy Markdown
Contributor

bastimeyer commented May 13, 2026

This has caused the same issue as #7426

# foo.py
import requests
requests.get("http://localhost", headers={"X-Foo": "bar"})
$ ty check foo.py
All checks passed!
# foo.py
import requests
headers={"X-Foo": "bar"}
requests.get("http://localhost", headers=headers)
$ ty check foo.py
error[invalid-argument-type]: Argument to function `get` is incorrect
 --> foo.py:4:34
  |
4 | requests.get("http://localhost", headers=headers)
  |                                  ^^^^^^^^^^^^^^^ Expected `MutableMapping[str, str | bytes] | None`, found `dict[str, str]`
  |
info: type `dict[str, str]` is not assignable to any element of the union `MutableMapping[str, str | bytes] | None`
info: ├── element `bytes` of union `str | bytes` is not assignable to `str`
info: └── ... omitted 1 union element without additional context
info: Function defined here
  --> /path/to/venv/lib/python3.14/site-packages/requests/api.py:74:5
   |
74 |   def get(
   |  _____^^^-
75 | |     url: _t.UriType, params: _t.ParamsType = None, **kwargs: Unpack[_t.GetKwargs]
76 | | ) -> Response:
   | |_- Parameter declared here
   |

Found 1 diagnostic

@nateprewitt
Copy link
Copy Markdown
Member Author

There's some tension here in usage that I'm a bit at a loss for how to address. I saw this in pip right before you left this comment which is the same failure mode.

Mapping works for the dict[str, str] case for the same reasons as #7426 and #7434. Requests.headers.update() causes issues for type checkers though. MutableMapping covers the modification case, but introduces the covariance issue since header values can be either str or bytes.

Once we pass prepare() it doesn't matter because it all gets turned into a CaseInsensitiveDict but the modification window between Request creation and prepare() is ambiguous.

@nateprewitt
Copy link
Copy Markdown
Member Author

Typeshed got around this issue by just labeling headers on Request as Incomplete which is just an alias for Any.

We either need to lie about what we accept with Mapping to support the input case or lie about what we store to fix the modification case. There's a runtime change of wrapping everything in a dict on intake, but that's certainly going to break someone's custom dict implementation.

The input case is probably where we need to be overly permissive because the new failure is significantly more common than the update() call. I'll look at a partial revert of this.

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