Skip to content

'spring.resources.cache.period' is always overridden by 'spring.resources.cache.cachecontrol' #16730

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

'spring.resources.cache.period' is always overridden by 'spring.resources.cache.cachecontrol' #16730

wants to merge 1 commit into from

Conversation

eiselems
Copy link
Contributor

@eiselems eiselems commented May 6, 2019 •
edited
Loading

Before this change it got overwritten by forwarding an empty
CacheControl to Spring. Spring itself sets CacheSeconds already
correctly in absence (=null) of a CacheControl.

Also:

  • Fixes bug in WebMvcAutoConfigurationTests.cachePeriod which prevented it to assert anything

See: #16488

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 6, 2019
@eiselems
Copy link
Contributor Author

eiselems commented May 6, 2019 •
edited
Loading

Concourse seems to have issues pulling the docker image :(
https://ci.spring.io/teams/spring-boot/pipelines/spring-boot-2.1.x/jobs/build-pull-requests/builds/29

About the code, once it gets merged onto master the tests needs to be guarded by
Assertions.setExtractBareNamePropertyMethods(false);

Similar to this PR where it was done to the WebFluxAutoconfigurationTests.class a2c5a79

bclozel
bclozel reviewed May 6, 2019
View reviewed changes
Copy link
Member

@bclozel bclozel left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the light of this PR, I think we should close the related issue and fix this at the Framework level.
We should keep this PR to fix the test once the issue is resolved in Framework.

...rc/main/java/org/springframework/boot/autoconfigure/web/servlet/WebMvcAutoConfiguration.java Outdated
CacheControl cacheControl = this.resourceProperties.getCache()
ResourceProperties.Cache.Cachecontrol cacheControl = this.resourceProperties
.getCache().getCachecontrol();
if (cachePeriod != null && cacheControl.getMaxAge() == null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cachePeriod is not only about max-age; if cachePeriod=-1 nothing is done, 0 means a no-store configuration, a positive number results in a max-age configuration. But this should delegate to Spring Framework and not hardcode those assumptions here.
A short term solution would be to check whether CacheControl.getHeaderValue() is not null and only apply it in this case. A better solution IMO would be to create a Spring Framework issue and getting this solved at the Framework level (see first comment in #16488).

Copy link
Contributor Author

@eiselems eiselems May 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your review, really sounds reasonable 👍
I think we just need to keep in mind that Webflux has basically the same implementation and when resolved it should also getting fixed there. See: https://github.com/spring-projects/spring-boot/blob/master/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/reactive/WebFluxAutoConfiguration.java#L169-L177

@eiselems
Fix 'spring.resources.cache.period' for WebMvc
b4cb03d 
Before this change it got overwritten by forwarding an empty
CacheControl to Spring. Spring itself sets CacheSeconds already
correctly in absence (=null) of a CacheControl.

Also:
* Fixes bug in WebMvcAutoConfigurationTests.cachePeriod which prevented it to assert anything

See: #16488
@eiselems
Copy link
Contributor Author

eiselems commented May 8, 2019 •
edited
Loading

@bclozel maybe you want to have another look. I documented my findings in #16488
Basically it comes down to Spring already doing the right thing. It was just that boot was just never giving null as CacheControl

Not sure what is up with concourse 😓
https://ci.spring.io/teams/spring-boot/pipelines/spring-boot-2.1.x/jobs/build-pull-requests/builds/30 is now hangig for an hour and counting,

the follow up build triggered by my update on the branch is waiting for 'checking max-in-flight is not reached' because of the earlier build. (PS: it worked on my machine before pushing it)

Edit: After the first build timed out, the second one built flawlessly 🤷‍♂

@eiselems eiselems mentioned this pull request May 8, 2019
Closed
@bclozel bclozel added this to the 2.1.x milestone May 10, 2019
@bclozel bclozel added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels May 10, 2019
@bclozel bclozel self-assigned this May 10, 2019
@bclozel bclozel modified the milestones: 2.1.x, 2.1.5 May 10, 2019
@bclozel bclozel changed the title Fix 'spring.resources.cache.period' for WebMvc 'spring.resources.cache.period' is always overridden by 'spring.resources.cache.cachecontrol' May 10, 2019
bclozel added a commit that referenced this pull request May 10, 2019
@bclozel
Merge pull request #16730 from eiselems
00b2954 
* pr/16730:
  Fix 'spring.resources.cache.period' for WebMvc
@bclozel bclozel closed this in 94a9748 May 10, 2019
@eiselems eiselems deleted the fixCache2.1.x branch May 11, 2019 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bclozel bclozel bclozel left review comments

Assignees

@bclozel bclozel

Labels
type: bug A general bug
Projects
None yet
Milestone
2.1.5
Development

Successfully merging this pull request may close these issues.
3 participants
@eiselems @bclozel @spring-projects-issues