From 5d6a56fe4a0cdde433c11f846f337fc00aa9029b Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Fri, 28 Aug 2026 15:49:57 +0200 Subject: [PATCH] Polishing CacheControl behavior This commit builds on the previous commit and ensures that "must-understand" is only used with "no-store". This check is performed at runtime as a staged interface/builder would be a major breaking change for a behavior that is highlighted as "SHOULD" in the specification. This commit also performs similar runtime checks for: * cache-public + cache-private * cache-public + no-store See gh-36918 --- .../org/springframework/http/CacheControl.java | 16 ++++++++++++++-- .../springframework/http/CacheControlTests.java | 17 +++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/http/CacheControl.java b/spring-web/src/main/java/org/springframework/http/CacheControl.java index 7ebf62eacdd..bdd46b8e0c2 100644 --- a/spring-web/src/main/java/org/springframework/http/CacheControl.java +++ b/spring-web/src/main/java/org/springframework/http/CacheControl.java @@ -21,6 +21,7 @@ import java.util.concurrent.TimeUnit; import org.jspecify.annotations.Nullable; +import org.springframework.util.Assert; import org.springframework.util.StringUtils; /** @@ -171,13 +172,16 @@ public class CacheControl { * Add a "must-understand" directive. *

This directive limits caching of the response to caches that * understand and conform to the requirements for the response status code. - * The {@link #noStore()} directive should also be set as a fallback for - * caches that do not implement the "must-understand" directive. + *

The {@link #noStore()} directive must have been set beforehand, as a + * fallback for caches that do not implement the "must-understand" directive. * @return {@code this}, to facilitate method chaining + * @throws IllegalStateException if the "no-store" directive has not been set * @since 7.1 * @see rfc9111 section 5.2.2.3 */ public CacheControl mustUnderstand() { + Assert.state(this.noStore, "The \"no-store\" directive should be set as a fallback, " + + "use CacheControl.noStore().mustUnderstand() instead"); this.mustUnderstand = true; return this; } @@ -214,9 +218,14 @@ public class CacheControl { * even if the response would normally be non-cacheable or cacheable * only within a private cache. * @return {@code this}, to facilitate method chaining + * @throws IllegalStateException if the "private" or "no-store" directive has already been set * @see rfc7234 section 5.2.2.5 */ public CacheControl cachePublic() { + Assert.state(!this.cachePrivate, "The \"private\" directive has already been set, " + + "which is mutually exclusive with \"public\""); + Assert.state(!this.noStore, "The \"no-store\" directive has already been set, " + + "which contradicts \"public\""); this.cachePublic = true; return this; } @@ -226,9 +235,12 @@ public class CacheControl { *

This directive indicates that the response message is intended * for a single user and MUST NOT be stored by a shared cache. * @return {@code this}, to facilitate method chaining + * @throws IllegalStateException if the "public" directive has already been set * @see rfc7234 section 5.2.2.6 */ public CacheControl cachePrivate() { + Assert.state(!this.cachePublic, "The \"public\" directive has already been set, " + + "which is mutually exclusive with \"private\""); this.cachePrivate = true; return this; } diff --git a/spring-web/src/test/java/org/springframework/http/CacheControlTests.java b/spring-web/src/test/java/org/springframework/http/CacheControlTests.java index dbc65c87ce6..f980fa055e6 100644 --- a/spring-web/src/test/java/org/springframework/http/CacheControlTests.java +++ b/spring-web/src/test/java/org/springframework/http/CacheControlTests.java @@ -22,6 +22,7 @@ import java.util.concurrent.TimeUnit; import org.junit.jupiter.api.Test; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatIllegalStateException; /** @@ -84,6 +85,22 @@ class CacheControlTests { assertThat(cc.getHeaderValue()).isEqualTo("no-store, must-understand"); } + @Test + void mustUnderstandWithoutNoStoreRejected() { + assertThatIllegalStateException().isThrownBy(() -> CacheControl.maxAge(1, TimeUnit.HOURS).mustUnderstand()); + } + + @Test + void cachePublicAndCachePrivateRejected() { + assertThatIllegalStateException().isThrownBy(() -> CacheControl.empty().cachePrivate().cachePublic()); + assertThatIllegalStateException().isThrownBy(() -> CacheControl.empty().cachePublic().cachePrivate()); + } + + @Test + void cachePublicWithNoStoreRejected() { + assertThatIllegalStateException().isThrownBy(() -> CacheControl.noStore().cachePublic()); + } + @Test void staleIfError() { CacheControl cc = CacheControl.maxAge(1, TimeUnit.HOURS).staleIfError(2, TimeUnit.HOURS);