-
Notifications
You must be signed in to change notification settings - Fork 38.6k
Make CacheControl immutable #33366
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
Make CacheControl immutable #33366
Conversation
I think I would rather try to make CacheControl immutable than introducing a builder for this. Additionally this builder is not guiding developers to typical setups and might also allow invalid combinations. Would you like to give the immutability update a try? At first glance this should not require API changes. |
7296ca1
to
825ce78
Compare
@bclozel I've pushed an initial set of changes that make |
This looks good @kashike, thanks. |
@kashike sorry but looking at how it's used downstream we've decided to revert this. It really comes as a surprise that the instance can no longer be reused as it was before. I don't know if having an immutable version of |
Hey @snicoll! I am still interested in this, but before I make another attempt it would be useful to see how |
See above, spring-projects/spring-boot@7343254 in particular. Then we had more issues testing things as well. In short, we can't really change the existing class to return a new instance if the caller has just been calling those methods and expect the instance they had created using one of the static methods to be updated. With all we know we may not have crafted the class the way we did but given its name, I am not sure there's much we can do about it, i.e. something that wouldn't be worse in terms of readability or consistency. |
The current
CacheControl
class has (in my opinion) a few drawbacks:static final
field for re-usabilityCacheControl