response cache: fix vary header handling by supporting a defined set of headers

the cache key will be also determined by the supported vary headers values, when present; this means easier lookups, and one level hash fetch, where the same url-verb request may have multiple entries depending on those headers

checking response vary header will therefore be something done at cache response lookup; writes may override when they shouldn't though, as a full match on supported vary headers will be performed, and one can't know in advance the combo of vary headers, which is why insterested parties will have to be judicious with the new  option

lib/httpx/plugins/response_cache.rb

@@ -10,6 +10,7 @@ module HTTPX
     module ResponseCache
       CACHEABLE_VERBS = %w[GET HEAD].freeze
       CACHEABLE_STATUS_CODES = [200, 203, 206, 300, 301, 410].freeze
+      SUPPORTED_VARY_HEADERS = %w[accept accept-encoding accept-language cookie origin].sort.freeze
       private_constant :CACHEABLE_VERBS
       private_constant :CACHEABLE_STATUS_CODES
 
@@ -42,7 +43,10 @@ module HTTPX
         end
 
         def extra_options(options)
-          options.merge(response_cache_store: Store.new)
+          options.merge(
+            supported_vary_headers: SUPPORTED_VARY_HEADERS,
+            response_cache_store: Store.new,
+          )
         end
       end
 
@@ -52,6 +56,10 @@ module HTTPX
 
           value
         end
+
+        def option_supported_vary_headers(value)
+          Array(value).sort
+        end
       end
 
       module InstanceMethods
@@ -108,12 +116,26 @@ module HTTPX
           @cached_response = nil
         end
 
+        def merge_headers(*)
+          super
+          @response_cache_key = nil
+        end
+
         def cacheable_verb?
           CACHEABLE_VERBS.include?(@verb)
         end
 
         def response_cache_key
-          @response_cache_key ||= Digest::SHA1.hexdigest("httpx-response-cache-#{@verb}-#{@uri}")
+          @response_cache_key ||= begin
+            keys = [@verb, @uri]
+
+            @options.supported_vary_headers.each do |field|
+              value = @headers[field]
+
+              keys << value if value
+            end
+            Digest::SHA1.hexdigest("httpx-response-cache-#{keys.join("-")}")
+          end
         end
       end
 
@@ -193,7 +215,7 @@ module HTTPX
           @vary = begin
             return unless @headers.key?("vary")
 
-            @headers["vary"].split(/ *, */)
+            @headers["vary"].split(/ *, */).map(&:downcase)
           end
         end
 

lib/httpx/plugins/response_cache/store.rb

@@ -13,13 +13,9 @@ module HTTPX::Plugins
       end
 
       def lookup(request)
-        responses = _get(request)
+        response = _get(request)
 
-        return unless responses
-
-        response = responses.find(&method(:match_by_vary?).curry(2)[request])
-
-        return unless response
+        return unless response && match_by_vary?(request, response)
 
         response.body.rewind
 
@@ -69,47 +65,36 @@ module HTTPX::Plugins
 
         original_request = response.instance_variable_get(:@request)
 
-        return request.headers.same_headers?(original_request.headers) if vary == %w[*]
+        if vary == %w[*]
+          request.options.supported_vary_headers.each do |field|
+            return false unless request.headers[field] == original_request.headers[field]
+          end
 
-        vary.all? do |cache_field|
-          cache_field.downcase!
-          !original_request.headers.key?(cache_field) || request.headers[cache_field] == original_request.headers[cache_field]
+          return true
+        end
+
+        vary.all? do |field|
+          !original_request.headers.key?(field) || request.headers[field] == original_request.headers[field]
         end
       end
 
       def _get(request)
         @store_mutex.synchronize do
-          responses = @store[request.response_cache_key]
-
-          return unless responses
-
-          responses.reject! do |res|
-            res.body.closed?
-          end
-
-          responses
+          @store[request.response_cache_key]
         end
       end
 
       def _set(request, response)
-        _memset(request, response)
-      end
-
-      def _memset(request, *responses)
         @store_mutex.synchronize do
-          responses_store = (@store[request.response_cache_key] ||= [])
+          cached_response = @store[request.response_cache_key]
 
-          cached_response = request.cached_response
+          if cached_response
+            return if cached_response == request.cached_response
 
-          responses_store.reject! do |res|
-            res == cached_response || res.body.closed? || match_by_vary?(request, res)
+            cached_response.close
           end
 
-          responses.each do |response|
-            responses_store << response
-          end
-
-          responses_store
+          @store[request.response_cache_key] = response
         end
       end
     end

sig/plugins/response_cache.rbs

@@ -3,12 +3,19 @@ module HTTPX
     module ResponseCache
       CACHEABLE_VERBS: Array[verb]
       CACHEABLE_STATUS_CODES: Array[Integer]
+      SUPPORTED_VARY_HEADERS: Array[String]
 
       def self?.cacheable_response?: (::HTTPX::ErrorResponse | cacheResponse response) -> bool
       def self?.cached_response?: (response response) -> bool
 
+      interface _ResponseCacheOptions
+        def response_cache_store: () -> Store
+
+        def supported_vary_headers: () -> Array[String]
+      end
+
       class Store
-        @store: Hash[String, Array[cacheResponse]]
+        @store: Hash[String, cacheResponse]
 
         @store_mutex: Thread::Mutex
 
@@ -24,11 +31,9 @@ module HTTPX
 
         def match_by_vary?: (cacheRequest request, cacheResponse response) -> bool
 
-        def _get: (cacheRequest request) -> Array[cacheResponse]?
+        def _get: (cacheRequest request) -> cacheResponse?
 
-        def _set: (cacheRequest request, cacheResponse response) -> Array[cacheResponse]
-
-        def _memset: (cacheRequest request, *cacheResponse response) -> Array[cacheResponse]
+        def _set: (cacheRequest request, cacheResponse response) -> void
       end
 
       module InstanceMethods
@@ -72,6 +77,8 @@ module HTTPX
       end
 
 
+      type cacheOptions = Options & _ResponseCacheOptions
+
       type cacheRequest = Request & RequestMethods
 
       type cacheResponse = Response & ResponseMethods

test/response_cache_store_test.rb

@@ -6,6 +6,29 @@ require "httpx/plugins/response_cache/store"
 class ResponseCacheStoreTest < Minitest::Test
   include ResponseCacheStoreTests
 
+  def test_internal_store_set
+    internal_store = store.instance_variable_get(:@store)
+
+    request = make_request("GET", "http://example.com/")
+
+    response = cached_response(request)
+    assert internal_store.size == 1
+    assert internal_store[request.response_cache_key] == response
+    response1 = cached_response(request, extra_headers: { "content-language" => "en" })
+    assert internal_store.size == 1
+    assert internal_store[request.response_cache_key] == response1
+    response2 = cached_response(request, extra_headers: { "content-language" => "en", "vary" => "accept-language" })
+    assert internal_store.size == 1
+    assert internal_store[request.response_cache_key] == response2
+
+    request.merge_headers("accept-language" => "pt")
+    response3 = cached_response(request, extra_headers: { "content-language" => "pt", "vary" => "accept-language" }, body: "teste")
+    assert internal_store.size == 2
+    assert internal_store[request.response_cache_key] == response3
+  end
+
+  private
+
   def store
     @store ||= Plugins::ResponseCache::Store.new
   end

test/support/requests/plugins/response_cache.rb

@@ -45,21 +45,21 @@ module Requests
         uncached = cache_client.get(cache_control_uri)
         verify_status(uncached, 200)
         assert cache_client.connection_count == 1, "a request should have been made"
-        assert(store.values.any? { |r| r.include?(uncached) })
+        assert store.value?(uncached)
 
         cached = cache_client.get(cache_control_uri)
         verify_status(cached, 200)
         assert cache_client.connection_count == 1, "no request should have been performed"
         assert uncached.body == cached.body, "bodies should have the same value"
         assert !uncached.body.eql?(cached.body), "bodies should have different references"
-        assert(store.values.any? { |r| r.include?(uncached) })
+        assert store.value?(uncached)
 
         sleep(2)
         after_expired = cache_client.get(cache_control_uri)
         verify_status(after_expired, 200)
         assert cache_client.connection_count == 2, "a conditional request should have been made"
-        assert(store.values.none? { |r| r.include?(uncached) })
-        assert(store.values.any? { |r| r.include?(after_expired) })
+        assert !store.value?(uncached)
+        assert store.value?(after_expired)
       end
     end
   end

test/support/response_cache_store_tests.rb

@@ -11,12 +11,13 @@ module ResponseCacheStoreTests
     assert cached_response.body == response.body
     assert store.cached?(request)
 
-    request2 = make_request("GET", "http://example.com/", headers: { "accept" => "text/plain" })
+    request2 = make_request("GET", "http://example.com/")
     cached_response2 = store.lookup(request2)
+    assert cached_response2
     assert cached_response2.headers == response.headers
     assert cached_response2.body == response.body
 
-    request3 = make_request("POST", "http://example.com/", headers: { "accept" => "text/plain" })
+    request3 = make_request("POST", "http://example.com/")
     assert store.lookup(request3).nil?
   end
 
@@ -124,27 +125,12 @@ module ResponseCacheStoreTests
     store.prepare(request3)
     assert request3.cached_response == response
     assert request3.headers.key?("if-none-match")
-    request4 = make_request("GET", "http://example.com/", headers: { "accept" => "text/plain", "user-agent" => "Linux Bowser" })
+    request4 = make_request("GET", "http://example.com/", headers: { "accept" => "text/plain", "accept-language" => "en" })
     store.prepare(request4)
     assert request4.cached_response.nil?
     assert !request4.headers.key?("if-none-match")
   end
 
-  def test_internal_store_set
-    internal_store = store.instance_variable_get(:@store)
-
-    request = make_request("GET", "http://example.com/")
-    response = cached_response(request)
-    assert internal_store[request.response_cache_key].size == 1
-    assert internal_store[request.response_cache_key].include?(response)
-    response1 = cached_response(request)
-    assert internal_store[request.response_cache_key].size == 1
-    assert internal_store[request.response_cache_key].include?(response1)
-    response2 = cached_response(request, extra_headers: { "content-encoding" => "gzip" })
-    assert internal_store[request.response_cache_key].size == 1
-    assert internal_store[request.response_cache_key].include?(response2)
-  end
-
   private
 
   def teardown
@@ -179,9 +165,9 @@ module ResponseCacheStoreTests
     @store ||= Plugins::ResponseCache::FileStore.new
   end
 
-  def cached_response(request, status: 200, extra_headers: {})
+  def cached_response(request, status: 200, extra_headers: {}, body: "test")
     response = response_class.new(request, status, "2.0", { "date" => Time.now.httpdate, "etag" => "ETAG" }.merge(extra_headers))
-    # response.body.write("test")
+    response.body.write(body)
     store.cache(request, response)
     response
   end