From 717361615493ca86c997192049b30763bda04fb6 Mon Sep 17 00:00:00 2001 From: HoneyryderChuck Date: Wed, 9 Apr 2025 15:03:57 +0100 Subject: [PATCH] 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 | 28 +++++++++-- lib/httpx/plugins/response_cache/store.rb | 49 +++++++------------ sig/plugins/response_cache.rbs | 17 +++++-- test/response_cache_store_test.rb | 23 +++++++++ .../requests/plugins/response_cache.rb | 8 +-- test/support/response_cache_store_tests.rb | 26 +++------- 6 files changed, 87 insertions(+), 64 deletions(-) diff --git a/lib/httpx/plugins/response_cache.rb b/lib/httpx/plugins/response_cache.rb index ab4e9a4a..2a48183c 100644 --- a/lib/httpx/plugins/response_cache.rb +++ b/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 diff --git a/lib/httpx/plugins/response_cache/store.rb b/lib/httpx/plugins/response_cache/store.rb index 9c148d99..ada7ea71 100644 --- a/lib/httpx/plugins/response_cache/store.rb +++ b/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 diff --git a/sig/plugins/response_cache.rbs b/sig/plugins/response_cache.rbs index 1c7db3c8..80194b74 100644 --- a/sig/plugins/response_cache.rbs +++ b/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 diff --git a/test/response_cache_store_test.rb b/test/response_cache_store_test.rb index 8bafe4ad..6be5c95f 100644 --- a/test/response_cache_store_test.rb +++ b/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 diff --git a/test/support/requests/plugins/response_cache.rb b/test/support/requests/plugins/response_cache.rb index abfaf5bd..6850cc41 100644 --- a/test/support/requests/plugins/response_cache.rb +++ b/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 diff --git a/test/support/response_cache_store_tests.rb b/test/support/response_cache_store_tests.rb index c4736511..79861345 100644 --- a/test/support/response_cache_store_tests.rb +++ b/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