simplify data structure use

[davidism]

There are a bunch of things I've noticed over time that contribute to the complexity of Werkzeug's classes and functions, especially from a typing perspective.

werkzeug.datastructures.mixins contains a bunch of immutable mixins. However, they're all marked private, and they're only used to then be mixed in to the real storage class, like ImmutableList, ImmutableDict, etc. It seems like the mixins could be merged into their concrete implementations.

ImmutableList doesn't seem necessary, as that's basically what tuple is. It's also marked private.

ImmutableTypeConversionDict is never used.

ImmutableDict is only used for Map.default_converters, and there seems to be no reason to do that vs annotating it as Mapping instead of MutableMapping.

Request has three attributes parameter_storage_class, dict_storage_class, and list_storage_class. On the other hand, it doesn't have a way to customize the header storage class. The docs for these suggest that you may have reason to use ImmutableDict instead of ImmutableMultiDict for some speedup, but that speed difference should have been negligible even when it was first written, and Python has only gotten faster. It would also be confusing as multi dict returns the first value first while dict would return the last value. The docs clearly say it would be a bad idea to use mutable structures for these, so that's not a reason to keep them. It's not clear why parameter and dict would have different storage classes. I'm pretty sure I also dealt with issues in the past where some use of dict_storage_class actually needed a dict and not a multi dict, and I recall some places that didn't use the attributes either. I can't think of a good reason to customize these classes nowadays. If we wanted to accurately type Request, it should really be generic over these three types (and probably a few more things), making the annotations really complex if we were to go that far.

A bunch of header parsing functions in werkzeug.http take a cls argument, and this requires a bunch of @overload signatures to get typing correct, and I'm not even sure it's entirely correct still. Many of these are also tied to more specific data structures, such as Accept, which also use the functions in http, which requires working around circular imports. I'd like to move parse/dump functions into methods on those classes instead, from_header(cls, value) -> Self and to_header(self) -> str.

[ThiefMaster]

Is there any case where someone would want to customize header storage?

For other request data I see more benefits in it, because someone may want to have e.g. case insensitivity or similar things that would not make sense as a default or officially supported config option...

PS: I think having ImmutableDict available outside is very useful for application code unrelated to request handling, not so much the mixin though.

[davidism]

Is there any case where someone would want to customize header storage?

I can't think of any, but I also wasn't suggesting we add it. It seems like a good example of how not being able to customize hasn't produced any issues over the years.

For other request data I see more benefits in it, because someone may want to have e.g. case insensitivity or similar things that would not make sense as a default or officially supported config option...

We can guess at possible reasons, but I don't think I've ever seen this used. It could also cause issues especially with caching, where two different spellings could mean the same thing. With headers that's part of the spec, but not with args and form data. It's still possible to do as well, override Request.args (or others) and do return CaseInsensitiveDict(super().args).

A sourcegraph search for Request.parameter_storage_class suggests that it was only used to set ImmutableOrderedMultiDict instead. But that's already been removed, as the base dict has maintained insertion order for many Python versions now. dict_storage_class returns nothing, and list_storage_class only returns copies of our test suite that set it to tuple.

I think having ImmutableDict available outside is very useful for application code unrelated to request handling, not so much the mixin though.

Nowadays, we'd probably use type checking for this, Mapping. I think it can even be subclassed directly to create an immutable dict-like class without us providing one. That also brings up a related point, Mapping doesn't even provide the methods that would mutate, unlike ImmutableDict which to type checkers and IDEs looks like all the methods are available.

[davidism]

Another thing I notice as I'm working on the structured header classes is that what the Request and Response header properties return and can be set to is very inconsistent.

All the "basic" headers, using header_property, default to None if they're not in headers or if they don't parse correctly.

But structured headers may return None or an instance that has invalid data and is falsey. And whether they return None on no header or also invalid header is inconsistent. For example, Request.if_range will always return an object (that's invalid and falsey), but Request.range can return None.

For responses, this is even more inconsistent. Whether the property has a setter or deleter, and whether the setter accepts None or a header string, is inconsitent. The structured headers seem to always return an inintially invalid data, with the assumption that they will be mutated, which uses a callback to set the header. But as pointed out when refactoring WWWAuthenticate, constructing a header then doing response.www_authenticate = header is way more efficient than causing the update callback to trigger as multiple attributes are mutated on the initial invalid object.

I think we should do the following:

• Don't generate invalid falsey header objects, return None instead.
• All response header properties (request is read only) should allow set and delete. Set should accept an object or None (equivalent to delete). Set should not accept a string, requiring headers[key] = value for that.
• Header classes should not have any special set/unset methods.

I'm less sure about one other potential change: Raising 400 errors instead of returning None when parsing the header value fails. This is definitely more correct and probably beneficial to everyone in the long run, but it's more disruptive and might imply a level of validation that we don't necessarily do (parsing might still result in a nonsensical value).

I'll note that while all this sounds like a pretty disruptive refactor, I already did this extensively with Authorization and WWWAuthenticate and that seems to have been quietly successful. #2619