Skip to content

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

Closed
wants to merge 1 commit into from

Conversation

kashike
Copy link
Contributor

@kashike kashike commented Aug 12, 2024

The current CacheControl class has (in my opinion) a few drawbacks:

  1. it is mutable - there's been times this has caused me (and others, I would assume) headaches when stored in a static final field for re-usability
  2. there's no way to remove a directive from an already-created CacheControl

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Aug 12, 2024
@bclozel bclozel added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Aug 12, 2024
@bclozel
Copy link
Member

bclozel commented Aug 12, 2024

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.

@bclozel bclozel added the status: waiting-for-feedback We need additional information before we can continue label Aug 12, 2024
@kashike kashike force-pushed the feature/cache-control branch from 7296ca1 to 825ce78 Compare August 12, 2024 07:05
@kashike kashike changed the title Builder-friendly CacheControl CacheControl is now immutable Aug 12, 2024
@kashike
Copy link
Contributor Author

kashike commented Aug 12, 2024

@bclozel I've pushed an initial set of changes that make CacheControl immutable - is this along the lines of what you were looking for? Is there any sort of validation you want to introduce here (ex: disallow public & private) to disallow invalid combinations?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Aug 12, 2024
@snicoll
Copy link
Member

snicoll commented Aug 12, 2024

This looks good @kashike, thanks.

@snicoll snicoll added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels Aug 12, 2024
@snicoll snicoll added this to the 6.2.0-M7 milestone Aug 12, 2024
@snicoll snicoll changed the title CacheControl is now immutable Make CacheControl immutable Aug 12, 2024
@snicoll snicoll self-assigned this Aug 12, 2024
snicoll pushed a commit that referenced this pull request Aug 12, 2024
snicoll added a commit that referenced this pull request Aug 12, 2024
@snicoll snicoll closed this in a1ec766 Aug 12, 2024
snicoll added a commit to spring-projects/spring-boot that referenced this pull request Aug 12, 2024
@snicoll snicoll removed this from the 6.2.0-M7 milestone Aug 12, 2024
@snicoll snicoll added the status: declined A suggestion or change that we don't feel we should currently apply label Aug 12, 2024
@snicoll
Copy link
Member

snicoll commented Aug 12, 2024

@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 CacheControl is worth the change but if you wan to purse this, please open a new issue so that we can discuss if there's another way.

snicoll added a commit that referenced this pull request Aug 12, 2024
This reverts commit a1ec766, reversing
changes made to e27192e.

See gh-33366
@kashike
Copy link
Contributor Author

kashike commented Aug 12, 2024

Hey @snicoll! I am still interested in this, but before I make another attempt it would be useful to see how CacheControl is being used downstream - are you able to share what kind of issues were encountered?

@snicoll
Copy link
Member

snicoll commented Aug 12, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants