pallets / werkzeug

The comprehensive WSGI web application library.
https://werkzeug.palletsprojects.com
BSD 3-Clause "New" or "Revised" License
6.66k stars 1.74k forks source link

MutableMapping cannot be instantiated from Headers #2989

Closed progval closed 2 weeks ago

progval commented 2 weeks ago

This bit of code from requests passes an instance of werkzeug's Headers to collections.abc.MutableMapping.update(): https://github.com/psf/requests/blob/main/src/requests/adapters.py#L375

Essentially, it does something like this:

>>> from werkzeug.datastructures.headers import Headers
>>> from requests.structures import CaseInsensitiveDict
/usr/lib/python3/dist-packages/requests/__init__.py:109: RequestsDependencyWarning: urllib3 (2.2.3) or chardet (5.2.0)/charset_normalizer (3.0.1) doesn't match a supported version!
  warnings.warn(
>>> headers = Headers([('Content-Type', 'text/plain'), ('Content-Length', '1')])
>>> CaseInsensitiveDict(headers)
{'Content-Type': 'text/plain', 'Content-Length': '1'}

but it does not work since Werkzeug 3.1.0:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3/dist-packages/requests/structures.py", line 44, in __init__
    self.update(data, **kwargs)
  File "<frozen _collections_abc>", line 949, in update
  File "/home/dev/.local/lib/python3.11/site-packages/werkzeug/datastructures/headers.py", line 88, in __getitem__
    return self.__class__(self._list[key])
                          ~~~~~~~~~~^^^^^
TypeError: list indices must be integers or slices, not tuple

This is because CaseInsensitiveDict calls the update method of collections.abc.MutableMapping, which calls iter() on the header object to get what it assumes is an iterator of keys (but for Headers it's an iterator of (key, value) pairs), then calls getitem for each "key".

Environment:

davidism commented 2 weeks ago

I don't know the right answer to this. Headers is not technically a MutableMapping, but it is way more annoying to use with type checkers if it doesn't inherit from MutableMapping. It mostly behaves like a mapping, and that's what most users expect, but sometimes it behaves like its underlying list, such as __iter__. When we were using type stubs, the stubs just lied about its inheritance. When we moved off of type stubs, I made it actually inherit. It looks like the default implementation of MutableMapping.update does if isinstance(other, Mapping), so this is now doing the wrong thing.

davidism commented 2 weeks ago

Headers doesn't actually satisfy the MutableMapping interface, so I guess it makes sense to remove it. Technically, passing Headers to a parameter annotated as MutableMapping is wrong, since like the issue here the function might try to use the value as a mapping in ways Headers doesn't satisfy. But at the same time, most places that expect a mapping will still behave correctly, so now your type checker complains about something that looks like a mapping to you, and you either need to ignore it or add an unnecessary dict(headers) conversion. 🤷‍♂️ I guess I'll remove that and see if anything else comes up.

progval commented 2 weeks ago

or add an unnecessary dict(headers) conversion

By the way, I was quite puzzled about that; but it turns out that dict() actually calls keys() instead of __iter__(). Makes me wonder why MutableMapping doesn't do that too.