Skip to content
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

clean up cache control attributes #2980

Closed
davidism opened this issue Oct 30, 2024 · 2 comments · Fixed by #2981
Closed

clean up cache control attributes #2980

davidism opened this issue Oct 30, 2024 · 2 comments · Fixed by #2981
Assignees
Milestone

Comments

@davidism
Copy link
Member

davidism commented Oct 30, 2024

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 davidism added this to the 3.1.0 milestone Oct 30, 2024
@davidism davidism self-assigned this Oct 30, 2024
@davidism
Copy link
Member Author

davidism commented Oct 30, 2024

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
Copy link
Member Author

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

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant