Set CacheControl for static resources from properties file #10514
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
| Assert.state(this.cachePeriod == null || this.cacheControl == null, | ||
| "Only one of cache-period or cache-control may be set."); | ||
| if (this.cacheControl != null) { | ||
| if (this.cacheControl.getMaxAge() != null) { |
There was a problem hiding this comment.
if cacheControl has been asserted above, what is the need to check that is not null again?
There was a problem hiding this comment.
It's asserted that not both cachePeriod and cacheControl are set. So cacheControl can be null at this point.
| } | ||
|
|
||
| static org.springframework.http.CacheControl createCacheControl(CacheControl cacheControlProperties) { | ||
| org.springframework.http.CacheControl cacheControl; |
There was a problem hiding this comment.
-
it looks weird for me, it creates the instance of org.springframework.http.CacheControl by updating a property (as maxAge), without any kind of builder or instantiating the constructor.
-
Also "createCacheControl" inside a class named "CacheControl" to return an object from another different class...what about "transformToHttpSpringCacheControl"
-
the method might be an instance method of CacheControl, so you don't need to pass the argument and cacheControlProperties call the method getting the new type.
There was a problem hiding this comment.
it looks weird for me [...]
Yes, it looks weird to me too... But the org.springframework.http.CacheControldoesn't have a public constructor or any setters. It it itself more like a builder (all methods return the instance) that builds a string (the cache header).
Also "createCacheControl" inside a class named "CacheControl" [...]
Done and I also renamed the CacheControl class to CacheControlProperties
the method might be an instance method
Changed
| cacheControlProperties.getsMaxAge(), | ||
| TimeUnit.SECONDS); | ||
| } | ||
| return cacheControl; |
There was a problem hiding this comment.
In mi opinion, this is a very long method, with some if-else, what make easy to loose the track. Might it be separated in meaningful methods what make the method more readable? What do you think?
There was a problem hiding this comment.
I extracted some parts but not sure if it really is better know.
| @Test | ||
| public void cachePeriod() throws Exception { | ||
| this.contextRunner.withPropertyValues("spring.resources.cache-period:5") | ||
| .run((context) -> { |
There was a problem hiding this comment.
you might extract the functionality in a method, such as assertCachePeriod(Context context). There are many nested blocks, uneasy to read. what do you reckon?
| this.contextRunner | ||
| .withPropertyValues("spring.resources.cache-control.max-age:5", | ||
| "spring.resources.cache-control.proxy-revalidate:true") | ||
| .run((context) -> { |
There was a problem hiding this comment.
I think it has the same problem than further up
tinexw
force-pushed
the
9432
branch
5 times, most recently
from
aad2732 to
3507bbf
Compare
bclozel
added
priority: normal
type: enhancement
|
@bclozel I addressed your comments, could you have another look, please? |
|
Hi @tinexw - I've merged your changes in e2bc90b and polished things a bit on top. Nothing major, just stuff that happened recently:
Thanks again for this new contribution! |
|
@bclozel Thanks! |
4 participants
First attempt at providing a solution for #9432
Note that the properties for
publicandprivateare currently calledcache-publicandcache-private. I'm not sure if it's possible to change that? I can look into it if the overall approach is ok.