skip private and Vary: * responses in HttpCacheInterceptor

http-cache/README.md

@@ -54,5 +54,9 @@ Caveats
   first 200; on a 304 the cached object is returned as-is. Mutations on the returned
   object propagate to subsequent callers — return immutable values from your decoder
   if that matters.
-- `Cache-Control` directives beyond `no-store` are not interpreted; freshness windows
-  (`max-age`, etc.) are not enforced. Every cached entry triggers revalidation.
+- Freshness windows (`max-age`, etc.) are not enforced. Every cached entry triggers
+  revalidation.
+- A store is shared by every caller of the client, so responses marked
+  `Cache-Control: no-store`, `Cache-Control: private` or `Vary: *` are never stored.
+  Other `Vary` field names are not matched against the request, so include them in
+  `keyFn` when a response varies on a header the default key ignores.

http-cache/src/main/java/feign/cache/HttpCacheInterceptor.java

@@ -35,7 +35,8 @@
  * returns the previously decoded value from {@link HttpCacheStore} without re-decoding.
  *
  * <p>Successful responses (2xx) carrying an {@code ETag} or {@code Last-Modified} header are
- * stored. Responses with {@code Cache-Control: no-store} are skipped.
+ * stored, unless the response says it cannot be held in a cache shared by several callers: {@code
+ * Cache-Control: no-store}, {@code Cache-Control: private} and {@code Vary: *} are skipped.
  *
  * <p>Default scope is HTTP {@code GET}, {@code HEAD}, and {@code QUERY}; override via {@link
  * #cacheable(Function)}.
@@ -49,6 +50,8 @@ public final class HttpCacheInterceptor implements MethodInterceptor {
 
   private static final Pattern NO_STORE = Pattern.compile("(?i)\\bno-store\\b");
 
+  private static final Pattern PRIVATE = Pattern.compile("(?i)\\bprivate\\b");
+
   private final HttpCacheStore store;
   private final Function<Invocation, String> keyFn;
   private final Function<RequestTemplate, Boolean> cacheable;
@@ -118,7 +121,7 @@ private void maybeStore(String key, Object result, Response response) {
       return;
     }
     Map<String, Collection<String>> headers = response.headers();
-    if (containsNoStore(headers)) {
+    if (!shareable(headers)) {
       return;
     }
     String etag = firstHeader(headers, "ETag");
@@ -144,13 +147,31 @@ private static Boolean defaultCacheable(RequestTemplate template) {
         || "QUERY".equalsIgnoreCase(method);
   }
 
-  private static boolean containsNoStore(Map<String, Collection<String>> headers) {
+  /**
+   * Whether a {@link HttpCacheStore}, which every caller of the client shares, may hold this
+   * response. {@code no-store} and {@code private} mark a response that a shared cache must not
+   * store (RFC 9111 sections 5.2.2.5 and 5.2.2.7), and {@code Vary: *} marks one that never matches
+   * a later request (RFC 9111 section 4.1); the default key covers the method, url and body only,
+   * so storing any of them hands the entry and its validator to the next caller of the same method
+   * and url.
+   */
+  private static boolean shareable(Map<String, Collection<String>> headers) {
     for (String value : Util.valuesOrEmpty(headers, "Cache-Control")) {
-      if (value != null && NO_STORE.matcher(value).find()) {
-        return true;
+      if (value != null && (NO_STORE.matcher(value).find() || PRIVATE.matcher(value).find())) {
+        return false;
+      }
+    }
+    for (String value : Util.valuesOrEmpty(headers, "Vary")) {
+      if (value == null) {
+        continue;
+      }
+      for (String field : value.split(",")) {
+        if ("*".equals(field.trim())) {
+          return false;
+        }
       }
     }
-    return false;
+    return true;
   }
 
   private static String firstHeader(Map<String, Collection<String>> headers, String name) {

http-cache/src/test/java/feign/cache/HttpCacheInterceptorTest.java

@@ -185,6 +185,50 @@ void noStoreCacheControlPreventsStorage() throws Exception {
     assertThat(store.size()).isZero();
   }
 
+  @Test
+  void privateCacheControlPreventsStorage() throws Exception {
+    server.enqueue(
+        new MockResponse()
+            .setHeader("ETag", "\"v1\"")
+            .setHeader("Cache-Control", "max-age=60, private")
+            .setBody("payload"));
+
+    api().fetch("42");
+
+    assertThat(store.size()).isZero();
+  }
+
+  @Test
+  void varyAsteriskPreventsStorage() throws Exception {
+    server.enqueue(
+        new MockResponse()
+            .setHeader("ETag", "\"v1\"")
+            .setHeader("Vary", "Accept-Encoding, *")
+            .setBody("payload"));
+
+    api().fetch("42");
+
+    assertThat(store.size()).isZero();
+  }
+
+  @Test
+  void privateResponseIsNotRevalidatedForASecondCaller() throws Exception {
+    server.enqueue(
+        new MockResponse()
+            .setHeader("ETag", "\"v1\"")
+            .setHeader("Cache-Control", "private")
+            .setBody("caller-a-data"));
+    server.enqueue(new MockResponse().setResponseCode(304));
+
+    Api api = api();
+    assertThat(api.fetch("42")).isEqualTo("caller-a-data");
+    assertThatThrownBy(() -> api.fetch("42")).isInstanceOf(FeignException.class);
+
+    server.takeRequest(); // caller a
+    RecordedRequest second = server.takeRequest();
+    assertThat(second.getHeader("If-None-Match")).isNull();
+  }
+
   @Test
   void serverErrorPropagatesAndDoesNotEvictExistingEntry() throws Exception {
     server.enqueue(new MockResponse().setHeader("ETag", "\"v1\"").setBody("payload-1"));