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

clean up cache control attributes #2980

Closed davidism closed 3 weeks ago

davidism commented 3 weeks ago

The implementation for cache control attributes is pretty messy. Some of the default values are incorrect, such as setting a string "*" for a value that should be boolean, or using -1 instead of None for a missing attribute. It is hard to type correctly, and hard to reason about when making changes.

Get rid of the property factory and implement each attribute directly instead, cleaning up the types and defaults to match the actual HTTP spec. Unfortunately, there's no way to deprecate the previous incorrect values, but from looking over it it seems like they would be unlikely to be relied upon, and it's easy to fix code that does.

davidism commented 3 weeks ago

RequestCacheControl.max_stale can have an optional value. Either it's max-stale=int or it's max-stale implying "any age". Right now, Werkzeug's type for this is int | str | None: None if it's not present at all, "*" if it's present with no value, and int otherwise.

Sourcegraph https://sourcegraph.com/search?q=context:global+lang:Python+%27request.cache_control.max_stale+%3D%3D+%22*%22%27&patternType=keyword&sm=0 finds no uses of cache_control.max_stale == "*". So it appears no one is checking for the no value case. This also just appears to be a lesser-used attribute in general, I barely found any access of cache_control.max_stale at all.

I'm thinking we simplify the type of this by breaking it into two properties: max_stale: int | None will only handle the (presumably much more common) case where it either has a value or is not present at all. max_stale_any: bool would be true or false based on the presence of the field.

Searching around a bit, .Net also uses two properties: https://learn.microsoft.com/en-us/dotnet/api/system.net.http.headers.cachecontrolheadervalue.maxstalelimit?view=net-8.0 MaxStale and MaxStaleLimit.

A thorough check would be:

if max_stale_any:
    if max_stale is None:
        present no value
    else:
        present with value

A basic check would be:

if max_stale is not None:
    present with value

Currently the check requires isinstance if you don't want a type checker to complain, for both thorough and basic checks:

if max_stale is not None:
    if isinstance(max_stale, str):
        present no value
    else:
        present with value
if max_stale is not None and not isinstance(max_stale, str):
    present with value
davidism commented 3 weeks ago

See #2981 for in depth explanation for the changes made after investigating the code and spec.