From b317df3cb2842109b1b6a2245b1cb96e4b40d9ee Mon Sep 17 00:00:00 2001 From: Colin Bendell Date: Mon, 27 Mar 2023 15:45:02 -0400 Subject: [PATCH 1/4] refactor to use only the unversioned cache entry * uses single cache key * stores the headers in the cache to make the etag available * validates against the etag for strict cache hit * soft etag matches based on TTL for a stale-while-revalidate --- .rubocop.yml | 2 +- Gemfile | 4 +- Gemfile.lock | 13 +- lib/response_bank/middleware.rb | 15 +- lib/response_bank/response_cache_handler.rb | 158 +++++++++---------- lib/response_bank/version.rb | 2 +- response_bank.gemspec | 9 +- test/controller_test.rb | 5 +- test/middleware_test.rb | 125 ++++++++++----- test/response_cache_handler_test.rb | 162 +++++++++++--------- 10 files changed, 285 insertions(+), 210 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index e5fad12..71559cb 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -2,6 +2,6 @@ inherit_from: - https://shopify.github.io/ruby-style-guide/rubocop.yml AllCops: - TargetRubyVersion: 2.4 + TargetRubyVersion: 2.7 Exclude: - vendor/bundle/**/* diff --git a/Gemfile b/Gemfile index 67a0bb6..2c58d7c 100644 --- a/Gemfile +++ b/Gemfile @@ -5,6 +5,6 @@ source "https://rubygems.org" gemspec gem 'rails', '~> 7.0.4' -gem 'rubocop', '1.48.0', require: false +gem 'rubocop', require: false, group: :test +gem 'mocha', require: false, group: :test gem 'simplecov', require: false, group: :test - diff --git a/Gemfile.lock b/Gemfile.lock index 0f57209..a0ed822 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,7 +1,7 @@ PATH remote: . specs: - response_bank (1.1.0) + response_bank (1.2.0) msgpack useragent @@ -151,7 +151,7 @@ GEM rake (13.0.6) regexp_parser (2.7.0) rexml (3.2.5) - rubocop (1.48.0) + rubocop (1.48.1) json (~> 2.3) parallel (~> 1.10) parser (>= 3.2.0.0) @@ -175,8 +175,6 @@ GEM timeout (0.3.2) tzinfo (2.0.6) concurrent-ruby (~> 1.0) - tzinfo-data (1.2023.2) - tzinfo (>= 1.0.0) unicode-display_width (2.4.2) useragent (0.16.10) websocket-driver (0.7.5) @@ -188,14 +186,13 @@ PLATFORMS ruby DEPENDENCIES - minitest (>= 5.13.0) - mocha (>= 1.10.0) + minitest (>= 5.18.0) + mocha rails (~> 7.0.4) rake response_bank! - rubocop (= 1.48.0) + rubocop simplecov - tzinfo-data (>= 1.2019.3) BUNDLED WITH 2.3.11 diff --git a/lib/response_bank/middleware.rb b/lib/response_bank/middleware.rb index fd7c1b5..d755b08 100644 --- a/lib/response_bank/middleware.rb +++ b/lib/response_bank/middleware.rb @@ -3,6 +3,10 @@ module ResponseBank class Middleware + # Limit the cached headers + # TODO: Make this lowercase/case-insentitive as per rfc2616 §4.2 + CACHEABLE_HEADERS = ["Location", "Content-Type", "ETag", "Content-Encoding", "Last-Modified", "Cache-Control", "Expires", "Surrogate-Keys", "Cache-Tags"].freeze + REQUESTED_WITH = "HTTP_X_REQUESTED_WITH" ACCEPT = "HTTP_ACCEPT" USER_AGENT = "HTTP_USER_AGENT" @@ -20,7 +24,6 @@ def call(env) if env['cacheable.cache'] if [200, 404, 301, 304].include?(status) headers['ETag'] = env['cacheable.key'] - headers['X-Alternate-Cache-Key'] = env['cacheable.unversioned-key'] if ie_ajax_request?(env) headers["Expires"] = "-1" @@ -38,22 +41,18 @@ def call(env) body_gz = ResponseBank.compress(body_string) + cached_headers = headers.slice(*CACHEABLE_HEADERS) # Store result - cache_data = [status, headers['Content-Type'], body_gz, timestamp] - cache_data << headers['Location'] if status == 301 + cache_data = [status, cached_headers, body_gz, timestamp] ResponseBank.write_to_cache(env['cacheable.key']) do payload = MessagePack.dump(cache_data) ResponseBank.write_to_backing_cache_store( env, - env['cacheable.key'], + env['cacheable.unversioned-key'], payload, expires_in: env['cacheable.versioned-cache-expiry'], ) - - if env['cacheable.unversioned-key'] - ResponseBank.write_to_backing_cache_store(env, env['cacheable.unversioned-key'], payload) - end end # since we had to generate the gz version above already we may diff --git a/lib/response_bank/response_cache_handler.rb b/lib/response_bank/response_cache_handler.rb index 2e438cb..7ee28a1 100644 --- a/lib/response_bank/response_cache_handler.rb +++ b/lib/response_bank/response_cache_handler.rb @@ -29,8 +29,8 @@ def initialize( def run! @env['cacheable.cache'] = true - @env['cacheable.key'] = versioned_key_hash - @env['cacheable.unversioned-key'] = unversioned_key_hash + @env['cacheable.key'] = entity_tag_hash + @env['cacheable.unversioned-key'] = cache_key_hash ResponseBank.log(cacheable_info_dump) @@ -41,32 +41,32 @@ def run! end end - def versioned_key_hash - @versioned_key_hash ||= key_hash(versioned_key) + def entity_tag_hash + @entity_tag_hash ||= hash(entity_tag) end - def unversioned_key_hash - @unversioned_key_hash ||= key_hash(unversioned_key) + def cache_key_hash + @cache_key_hash ||= hash(cache_key) end private - def key_hash(key) + def hash(key) "cacheable:#{Digest::MD5.hexdigest(key)}" end - def versioned_key - @versioned_key ||= ResponseBank.cache_key_for(key: @key_data, version: @version_data) + def entity_tag + @entity_tag ||= ResponseBank.cache_key_for(key: @key_data, version: @version_data) end - def unversioned_key - @unversioned_key ||= ResponseBank.cache_key_for(key: @key_data) + def cache_key + @cache_key ||= ResponseBank.cache_key_for(key: @key_data) end def cacheable_info_dump log_info = [ - "Raw cacheable.key: #{versioned_key}", - "cacheable.key: #{versioned_key_hash}", + "Raw cacheable.key: #{entity_tag}", + "cacheable.key: #{entity_tag_hash}", ] if @env['HTTP_IF_NONE_MATCH'] @@ -78,68 +78,32 @@ def cacheable_info_dump def try_to_serve_from_cache # Etag - response = serve_from_browser_cache(versioned_key_hash) - + response = serve_from_browser_cache(entity_tag_hash, @env['HTTP_IF_NONE_MATCH']) return response if response - # Memcached - response = if @serve_unversioned - serve_from_cache(unversioned_key_hash, "Cache hit: server (unversioned)") - else - serve_from_cache(versioned_key_hash, "Cache hit: server") - end - + response = serve_from_cache(cache_key_hash, entity_tag_hash, @cache_age_tolerance) return response if response - @env['cacheable.locked'] ||= false - - if @env['cacheable.locked'] || ResponseBank.acquire_lock(versioned_key_hash) - # execute if we can get the lock - @env['cacheable.locked'] = true - elsif serving_from_noncurrent_but_recent_version_acceptable? - # serve a stale version - response = serve_from_cache(unversioned_key_hash, "Cache hit: server (recent)", @cache_age_tolerance) - - return response if response - end - # No cache hit; this request cannot be handled from cache. # Yield to the controller and mark for writing into cache. refill_cache end - def serving_from_noncurrent_but_recent_version_acceptable? - @cache_age_tolerance > 0 - end - - def serve_from_browser_cache(cache_key_hash) - # Support for Etag variations including: - # If-None-Match: abc - # If-None-Match: "abc" - # If-None-Match: W/"abc" - # If-None-Match: "abc", "def" - if (if_none_match = @env["HTTP_IF_NONE_MATCH"]) - etags = if_none_match.split(",") - etags.each do |tag| - tag.sub!(/\"?\s*\z/, "") - tag.sub!(/\A\s*(W\/)?\"?/, "") - end - - if etags.include?(cache_key_hash) - @env['cacheable.miss'] = false - @env['cacheable.store'] = 'client' + def serve_from_browser_cache(entity_tag, if_none_match) + if etag_matches?(entity_tag, if_none_match) + @env['cacheable.miss'] = false + @env['cacheable.store'] = 'client' - @headers.delete('Content-Type') - @headers.delete('Content-Length') + @headers.delete('Content-Type') + @headers.delete('Content-Length') - ResponseBank.log("Cache hit: client") + ResponseBank.log("Cache hit: client") - [304, @headers, []] - end + [304, @headers, []] end end - def serve_from_cache(cache_key_hash, message, cache_age_tolerance = nil) + def serve_from_cache(cache_key_hash, match_entity_tag = "*", cache_age_tolerance = nil) raw = ResponseBank.read_from_backing_cache_store(@env, cache_key_hash, backing_cache_store: @cache_store) if raw @@ -148,37 +112,77 @@ def serve_from_cache(cache_key_hash, message, cache_age_tolerance = nil) @env['cacheable.miss'] = false @env['cacheable.store'] = 'server' - status, content_type, body, timestamp, location = hit + status, headers, body, timestamp, location = hit - if cache_age_tolerance && page_too_old?(timestamp, cache_age_tolerance) - ResponseBank.log("Found an unversioned cache entry, but it was too old (#{timestamp})") - - nil - else - @headers['Content-Type'] = content_type + # polyfill headers for legacy versions + headers = { 'Content-Type' => headers.to_s } if headers.is_a? String + headers['Location'] = location if location - @headers['Location'] = location if location + @env['cacheable.locked'] ||= false - if @env["gzip"] - @headers['Content-Encoding'] = "gzip" + # to preserve the unversioned/versioned logging messages from past releases we split the match_entity_tag test + if match_entity_tag == "*" + ResponseBank.log("Cache hit: server (unversioned)") + # page tolerance only applies for versioned + etag mismatch + elsif etag_matches?(headers['ETag'], match_entity_tag) + ResponseBank.log("Cache hit: server") + else + # cache miss; check to see if any parallel requests already are regenerating the cache + if ResponseBank.acquire_lock(match_entity_tag) + # execute if we can get the lock + @env['cacheable.locked'] = true + return nil + elsif stale_while_revalidate?(timestamp, cache_age_tolerance) + # cache is being regenerated, can we avoid piling on and use a stale version in the interim? + ResponseBank.log("Cache hit: server (recent)") else - # we have to uncompress because the client doesn't support gzip - ResponseBank.log("uncompressing for client without gzip") - body = ResponseBank.decompress(body) + ResponseBank.log("Found an unversioned cache entry, but it was too old (#{timestamp})") + return nil end + end - ResponseBank.log(message) + # version check + # unversioned but tolerance threshold + # regen + @headers = @headers.merge(headers) - [status, @headers, [body]] + if @env["gzip"] + @headers['Content-Encoding'] = "gzip" + else + # we have to uncompress because the client doesn't support gzip + ResponseBank.log("uncompressing for client without gzip") + body = ResponseBank.decompress(body) end + [status, @headers, [body]] end end - def page_too_old?(timestamp, cache_age_tolerance) - !timestamp || timestamp < (Time.now.to_i - cache_age_tolerance) + def etag_matches?(entity_tag, if_none_match) + # Support for Etag variations including: + # If-None-Match: abc + # If-None-Match: "abc" + # If-None-Match: W/"abc" + # If-None-Match: "abc", "def" + # If-None-Match: "*" + return false unless entity_tag + return false unless if_none_match + + # strictly speaking an unquoted etag is not valid, yet common + # to avoid unintended greedy matches in we check for naked entity then includes with quoted entity values + if_none_match == "*" || if_none_match == entity_tag || if_none_match.include?(%{"#{entity_tag}"}) + end + + def stale_while_revalidate?(timestamp, cache_age_tolerance) + return false if !cache_age_tolerance + return false if !timestamp + + timestamp >= (Time.now.to_i - cache_age_tolerance) end def refill_cache + # non cache hits do not yet have the lock + ResponseBank.acquire_lock(entity_tag_hash) unless @env['cacheable.locked'] + @env['cacheable.locked'] = true @env['cacheable.miss'] = true ResponseBank.log("Refilling cache") diff --git a/lib/response_bank/version.rb b/lib/response_bank/version.rb index 0bc7c22..c47b5fe 100644 --- a/lib/response_bank/version.rb +++ b/lib/response_bank/version.rb @@ -1,4 +1,4 @@ # frozen_string_literal: true module ResponseBank - VERSION = "1.1.0" + VERSION = "1.2.0" end diff --git a/response_bank.gemspec b/response_bank.gemspec index 1af2ad9..7fe5c45 100644 --- a/response_bank.gemspec +++ b/response_bank.gemspec @@ -15,16 +15,15 @@ Gem::Specification.new do |s| s.files = Dir["lib/**/*.rb", "README.md", "LICENSE.txt"] s.require_paths = ["lib"] - s.required_ruby_version = ">= 2.4.0" + s.required_ruby_version = ">= 2.7.0" s.metadata["allowed_push_host"] = "https://rubygems.org" s.add_runtime_dependency("useragent") s.add_runtime_dependency("msgpack") - s.add_development_dependency("minitest", ">= 5.13.0") - s.add_development_dependency("mocha", ">= 1.10.0") + s.add_development_dependency("minitest", ">= 5.18.0") + s.add_development_dependency("mocha", ">= 2.0.0") s.add_development_dependency("rake") - s.add_development_dependency("rails", ">= 5.0") - s.add_development_dependency("tzinfo-data", ">= 1.2019.3") + s.add_development_dependency("rails", ">= 6.1") end diff --git a/test/controller_test.rb b/test/controller_test.rb index b2f9465..08b02da 100644 --- a/test/controller_test.rb +++ b/test/controller_test.rb @@ -57,6 +57,7 @@ def test_cache_control_no_store_set_for_uncacheable_requests def test_server_cache_hit controller.request.env['gzip'] = false @cache_store.expects(:read).returns(page_serialized) + ResponseBank::ResponseCacheHandler.any_instance.expects(:entity_tag_hash).returns('*').at_least_once controller.expects(:render).with(plain: 'hi.', status: 200) controller.send(:response_cache) {} @@ -64,7 +65,7 @@ def test_server_cache_hit def test_client_cache_hit controller.request.env['HTTP_IF_NONE_MATCH'] = 'deadbeef' - ResponseBank::ResponseCacheHandler.any_instance.expects(:versioned_key_hash).returns('deadbeef').at_least_once + ResponseBank::ResponseCacheHandler.any_instance.expects(:entity_tag_hash).returns('deadbeef').at_least_once controller.expects(:head).with(:not_modified) controller.send(:response_cache) {} @@ -77,6 +78,6 @@ def controller end def page_serialized - MessagePack.dump([200, "text/html", ResponseBank.compress("hi."), 1331765506]) + MessagePack.dump([200, {"Content-Type" => "text/html"}, ResponseBank.compress("hi."), 1331765506]) end end diff --git a/test/middleware_test.rb b/test/middleware_test.rb index ae001b5..eccac2a 100644 --- a/test/middleware_test.rb +++ b/test/middleware_test.rb @@ -16,7 +16,8 @@ def app(_env) def not_found(env) env['cacheable.cache'] = true env['cacheable.miss'] = true - env['cacheable.key'] = '"abcd"' + env['cacheable.key'] = '"etag_value"' + env['cacheable.unversioned-key'] = '"not_found_cache_key"' body = block_given? ? [yield] : ['Hi'] [404, { 'Content-Type' => 'text/plain' }, body] @@ -25,7 +26,8 @@ def not_found(env) def cached_moved(env) env['cacheable.cache'] = true env['cacheable.miss'] = false - env['cacheable.key'] = '"abcd"' + env['cacheable.key'] = '"etag_value"' + env['cacheable.unversioned-key'] = '"cached_moved_cache_key"' env['cacheable.store'] = 'server' [301, { 'Location' => 'http://shopify.com' }, []] @@ -34,7 +36,8 @@ def cached_moved(env) def moved(env) env['cacheable.cache'] = true env['cacheable.miss'] = true - env['cacheable.key'] = '"abcd"' + env['cacheable.key'] = '"etag_value"' + env['cacheable.unversioned-key'] = '"moved_cache_key"' [301, { 'Location' => 'http://shopify.com', 'Content-Type' => 'text/plain' }, []] end @@ -42,17 +45,28 @@ def moved(env) def cacheable_app(env) env['cacheable.cache'] = true env['cacheable.miss'] = true - env['cacheable.key'] = '"abcd"' + env['cacheable.key'] = '"etag_value"' + env['cacheable.unversioned-key'] = '"cacheable_app_cache_key"' body = block_given? ? [yield] : ['Hi'] [200, { 'Content-Type' => 'text/plain' }, body] end +def cacheable_app_limit_headers(env) + env['cacheable.cache'] = true + env['cacheable.miss'] = true + env['cacheable.key'] = '"etag_value"' + env['cacheable.unversioned-key'] = '"cacheable_app_limit_headers_cache_key"' + + body = block_given? ? [yield] : ['Hi'] + [200, { 'Content-Type' => 'text/plain', 'Extra-Headers' => 'not-cached', 'Cache-Tags' => 'tag1, tag2'}, body] +end + def cacheable_app_with_unversioned(env) env['cacheable.cache'] = true env['cacheable.miss'] = true - env['cacheable.key'] = '"abcd"' - env['cacheable.unversioned-key'] = '"abcd-unversioned"' + env['cacheable.key'] = '"etag_value"' + env['cacheable.unversioned-key'] = '"cacheable_app_with_unversioned_cache_key"' body = block_given? ? [yield] : ['Hi'] [200, { 'Content-Type' => 'text/plain' }, body] @@ -61,7 +75,8 @@ def cacheable_app_with_unversioned(env) def already_cached_app(env) env['cacheable.cache'] = true env['cacheable.miss'] = false - env['cacheable.key'] = '"abcd"' + env['cacheable.key'] = '"etag_value"' + env['cacheable.unversioned-key'] = '"already_cached_app_cache_key"' env['cacheable.store'] = 'server' body = block_given? ? [yield] : ['Hi'] @@ -71,7 +86,8 @@ def already_cached_app(env) def client_hit_app(env) env['cacheable.cache'] = true env['cacheable.miss'] = false - env['cacheable.key'] = '"abcd"' + env['cacheable.key'] = '"etag_value"' + env['cacheable.unversioned-key'] = '"client_hit_app_cache_key"' env['cacheable.store'] = 'client' body = block_given? ? [yield] : [''] @@ -93,8 +109,9 @@ def test_cache_miss_and_ignore ware = ResponseBank::Middleware.new(method(:app)) result = ware.call(env) + headers = result[1] - assert_nil(result[1]['ETag']) + assert_nil(headers['ETag']) end def test_cache_miss_and_not_found @@ -105,7 +122,8 @@ def test_cache_miss_and_not_found ware = ResponseBank::Middleware.new(method(:not_found)) result = ware.call(env) - assert_equal('"abcd"', result[1]['ETag']) + headers = result[1] + assert_equal('"etag_value"', headers['ETag']) end def test_cache_hit_and_moved @@ -115,9 +133,10 @@ def test_cache_hit_and_moved ware = ResponseBank::Middleware.new(method(:cached_moved)) result = ware.call(env) + headers = result[1] - assert_equal('"abcd"', result[1]['ETag']) - assert_equal('http://shopify.com', result[1]['Location']) + assert_equal('"etag_value"',headers['ETag']) + assert_equal('http://shopify.com', headers['Location']) end def test_cache_miss_and_moved @@ -126,16 +145,43 @@ def test_cache_miss_and_moved env = Rack::MockRequest.env_for("http://example.com/index.html") ware = ResponseBank::Middleware.new(method(:moved)) result = ware.call(env) + headers = result[1] - assert_equal('"abcd"', result[1]['ETag']) - assert_equal('http://shopify.com', result[1]['Location']) + assert_equal('"etag_value"', headers['ETag']) + assert_equal('http://shopify.com', headers['Location']) + end + + def test_cache_miss_and_store_limited_headers + ResponseBank::Middleware.any_instance.stubs(timestamp: 424242) + ResponseBank.cache_store.expects(:write).with( + '"cacheable_app_limit_headers_cache_key"', + MessagePack.dump([200, {'Content-Type' => 'text/plain', 'ETag' => '"etag_value"', 'Cache-Tags' => 'tag1, tag2'}, ResponseBank.compress('Hi'), 424242]), + raw: true, + expires_in: nil, + ).once + + env = Rack::MockRequest.env_for("http://example.com/index.html") + + ware = ResponseBank::Middleware.new(method(:cacheable_app_limit_headers)) + result = ware.call(env) + headers = result[1] + + assert(env['cacheable.cache']) + assert(env['cacheable.miss']) + + assert_equal('"etag_value"', headers['ETag']) + assert_equal('miss', headers['X-Cache']) + assert_nil(env['cacheable.store']) + + # no gzip support here + assert(!headers['Content-Encoding']) end def test_cache_miss_and_store ResponseBank::Middleware.any_instance.stubs(timestamp: 424242) ResponseBank.cache_store.expects(:write).with( - '"abcd"', - MessagePack.dump([200, 'text/plain', ResponseBank.compress('Hi'), 424242]), + '"cacheable_app_cache_key"', + MessagePack.dump([200, {'Content-Type' => 'text/plain', 'ETag' => '"etag_value"' }, ResponseBank.compress('Hi'), 424242]), raw: true, expires_in: nil, ).once @@ -144,34 +190,38 @@ def test_cache_miss_and_store ware = ResponseBank::Middleware.new(method(:cacheable_app)) result = ware.call(env) + headers = result[1] assert(env['cacheable.cache']) assert(env['cacheable.miss']) - assert_equal('"abcd"', result[1]['ETag']) - assert_equal('miss', result[1]['X-Cache']) + assert_equal('"etag_value"', headers['ETag']) + assert_equal('miss', headers['X-Cache']) assert_nil(env['cacheable.store']) # no gzip support here - assert(!result[1]['Content-Encoding']) + assert(!headers['Content-Encoding']) end def test_cache_miss_and_store_with_shortened_cache_expiry env = Rack::MockRequest.env_for("http://example.com/index.html") env['cacheable.versioned-cache-expiry'] = 30.seconds - ResponseBank.cache_store.expects(:write).with('"abcd"', anything, has_entries(expires_in: 30.seconds)) - ResponseBank.cache_store.expects(:write).with('"abcd-unversioned"', anything, has_entries(expires_in: nil)) + ResponseBank.cache_store.expects(:write).with('"cacheable_app_with_unversioned_cache_key"', anything, has_entries(expires_in: 30.seconds)) ware = ResponseBank::Middleware.new(method(:cacheable_app_with_unversioned)) result = ware.call(env) + + headers = result[1] + assert_equal('"etag_value"', headers['ETag']) + assert_equal('miss', headers['X-Cache']) end def test_cache_miss_and_store_on_moved ResponseBank::Middleware.any_instance.stubs(timestamp: 424242) ResponseBank.cache_store.expects(:write).with( - '"abcd"', - MessagePack.dump([301, 'text/plain', ResponseBank.compress(''), 424242, 'http://shopify.com']), + '"moved_cache_key"', + MessagePack.dump([301, {'Location' => 'http://shopify.com', 'Content-Type' => 'text/plain', 'ETag' => '"etag_value"'}, ResponseBank.compress(''), 424242]), raw: true, expires_in: nil, ).once @@ -180,23 +230,24 @@ def test_cache_miss_and_store_on_moved ware = ResponseBank::Middleware.new(method(:moved)) result = ware.call(env) + headers = result[1] assert(env['cacheable.cache']) assert(env['cacheable.miss']) - assert_equal('"abcd"', result[1]['ETag']) - assert_equal('miss', result[1]['X-Cache']) + assert_equal('"etag_value"', headers['ETag']) + assert_equal('miss', headers['X-Cache']) assert_nil(env['cacheable.store']) # no gzip support here - assert(!result[1]['Content-Encoding']) + assert(!headers['Content-Encoding']) end def test_cache_miss_and_store_with_gzip_support ResponseBank::Middleware.any_instance.stubs(timestamp: 424242) ResponseBank.cache_store.expects(:write).with( - '"abcd"', - MessagePack.dump([200, 'text/plain', ResponseBank.compress('Hi'), 424242]), + '"cacheable_app_cache_key"', + MessagePack.dump([200, {'Content-Type' => 'text/plain', 'ETag' => '"etag_value"' }, ResponseBank.compress('Hi'), 424242]), raw: true, expires_in: nil, ).once @@ -206,16 +257,17 @@ def test_cache_miss_and_store_with_gzip_support ware = ResponseBank::Middleware.new(method(:cacheable_app)) result = ware.call(env) + headers = result[1] assert(env['cacheable.cache']) assert(env['cacheable.miss']) - assert_equal('"abcd"', result[1]['ETag']) - assert_equal('miss', result[1]['X-Cache']) + assert_equal('"etag_value"', headers['ETag']) + assert_equal('miss', headers['X-Cache']) assert_nil(env['cacheable.store']) # gzip support! - assert_equal('gzip', result[1]['Content-Encoding']) + assert_equal('gzip', headers['Content-Encoding']) assert_equal([ResponseBank.compress("Hi")], result[2]) end @@ -226,11 +278,12 @@ def test_cache_hit_server ware = ResponseBank::Middleware.new(method(:already_cached_app)) result = ware.call(env) + headers = result[1] assert(env['cacheable.cache']) assert(!env['cacheable.miss']) assert_equal('server', env['cacheable.store']) - assert_equal('"abcd"', result[1]['ETag']) + assert_equal('"etag_value"', headers['ETag']) end def test_cache_hit_client @@ -240,11 +293,12 @@ def test_cache_hit_client ware = ResponseBank::Middleware.new(method(:client_hit_app)) result = ware.call(env) + headers = result[1] assert(env['cacheable.cache']) assert(!env['cacheable.miss']) assert_equal('client', env['cacheable.store']) - assert_equal('"abcd"', result[1]['ETag']) + assert_equal('"etag_value"', headers['ETag']) end def test_ie_ajax @@ -280,11 +334,12 @@ def test_cache_hit_server_with_ie_ajax ware = ResponseBank::Middleware.new(method(:already_cached_app)) result = ware.call(env) + headers = result[1] assert(env['cacheable.cache']) assert(!env['cacheable.miss']) assert_equal('server', env['cacheable.store']) - assert_equal('"abcd"', result[1]['ETag']) - assert_equal("-1", result[1]['Expires']) + assert_equal('"etag_value"', headers['ETag']) + assert_equal("-1", headers['Expires']) end end diff --git a/test/response_cache_handler_test.rb b/test/response_cache_handler_test.rb index 9dd3e06..47d6efc 100644 --- a/test/response_cache_handler_test.rb +++ b/test/response_cache_handler_test.rb @@ -6,7 +6,7 @@ class ResponseCacheHandlerTest < Minitest::Test def setup @cache_store = stub.tap { |s| s.stubs(read: nil) } - controller.request.env['HTTP_IF_NONE_MATCH'] = 'deadbeefdeadbeef' + controller.request.env['HTTP_IF_NONE_MATCH'] = '"should-not-match"' ResponseBank.stubs(:acquire_lock).returns(true) end @@ -15,7 +15,7 @@ def controller end def handler - @handler = ResponseBank::ResponseCacheHandler.new( + @handler ||= ResponseBank::ResponseCacheHandler.new( key_data: controller.send(:cache_key_data), version_data: controller.send(:cache_version_data), cache_store: @cache_store, @@ -24,20 +24,22 @@ def handler serve_unversioned: controller.send(:serve_unversioned_cacheable_entry?), cache_age_tolerance: controller.send(:cache_age_tolerance_in_seconds), headers: controller.response.headers, - &proc { [200, {}, 'some text'] } + &proc { [200, {}, 'dynamic output'] } ) end - def page - [200, "text/html", ResponseBank.compress("hi."), 1331765506] + def page(cache_hit = true) + etag = cache_hit ? handler.entity_tag_hash : "not-cached" + [200, {"Content-Type" => "text/html", "ETag" => etag}, ResponseBank.compress("cached output"), 1331765506] end - def page_serialized - MessagePack.dump(page) + def page_cache_entry(cache_hit = true) + MessagePack.dump(page(cache_hit)) end - def page_uncompressed - [200, "text/html", "hi.", 1331765506] + def page_uncompressed(cache_hit = true) + etag = cache_hit ? handler.entity_tag_hash : "not-cached" + [200, {"Content-Type" => "text/html", "ETag" => etag}, "cached output", 1331765506] end def test_cache_miss_block_is_only_called_once_if_it_return_nil @@ -59,112 +61,125 @@ def test_cache_miss_block_is_only_called_once_if_it_return_nil my_handler.run! assert_equal(1, called) - assert_env(true, nil) + assert_cache_miss(true, nil) end def test_cache_miss _, _, body = handler.run! - assert_equal('some text', body) - assert_env(true, nil) + assert_equal('dynamic output', body) + assert_cache_miss(true, nil) end def test_client_cache_hit - controller.request.env['HTTP_IF_NONE_MATCH'] = handler.versioned_key_hash + controller.request.env['HTTP_IF_NONE_MATCH'] = handler.entity_tag_hash handler.run! - assert_env(false, 'client') + assert_cache_miss(false, 'client') end def test_client_cache_hit_quoted - controller.request.env['HTTP_IF_NONE_MATCH'] = "\"#{handler.versioned_key_hash}\"" + controller.request.env['HTTP_IF_NONE_MATCH'] = "\"#{handler.entity_tag_hash}\"" handler.run! - assert_env(false, 'client') + assert_cache_miss(false, 'client') end def test_client_cache_hit_multi - controller.request.env['HTTP_IF_NONE_MATCH'] = "foo, \"#{handler.versioned_key_hash}\", bar" + controller.request.env['HTTP_IF_NONE_MATCH'] = "foo, \"#{handler.entity_tag_hash}\", bar" handler.run! - assert_env(false, 'client') + assert_cache_miss(false, 'client') end def test_client_cache_hit_weak - controller.request.env['HTTP_IF_NONE_MATCH'] = "W/\"#{handler.versioned_key_hash}\"" + controller.request.env['HTTP_IF_NONE_MATCH'] = "W/\"#{handler.entity_tag_hash}\"" handler.run! - assert_env(false, 'client') + assert_cache_miss(false, 'client') + end + + def test_client_cache_hit_wildcard + controller.request.env['HTTP_IF_NONE_MATCH'] = "*" + handler.run! + assert_cache_miss(false, 'client') end def test_client_cache_miss_partial - controller.request.env['HTTP_IF_NONE_MATCH'] = "aaa#{handler.versioned_key_hash}zzz" + controller.request.env['HTTP_IF_NONE_MATCH'] = "aaa#{handler.entity_tag_hash}zzz" handler.run! - assert_env(true, nil) + assert_cache_miss(true, nil) end def test_server_cache_hit - controller.request.env['gzip'] = false - @cache_store.expects(:read).with(handler.versioned_key_hash, raw: true).returns(page_serialized) - expect_page_rendered(page_uncompressed) - handler.run! - assert_env(false, 'server') + @cache_store.expects(:read).with(handler.cache_key_hash, raw: true).returns(page_cache_entry) + expect_page_rendered(page_uncompressed, false) + assert_cache_miss(false, 'server') end def test_server_cache_hit_support_gzip - controller.request.env['gzip'] = true - @cache_store.expects(:read).with(handler.versioned_key_hash, raw: true).returns(page_serialized) - expect_compressed_page_rendered(page) - handler.run! - assert_env(false, 'server') + @cache_store.expects(:read).with(handler.cache_key_hash, raw: true).returns(page_cache_entry) + expect_page_rendered(page(true)) + assert_cache_miss(false, 'server') end def test_server_recent_cache_hit @controller.stubs(:cache_age_tolerance_in_seconds).returns(999999999999) - @cache_store.expects(:read).with(handler.unversioned_key_hash, raw: true).returns(page_serialized) - expect_page_rendered(page_uncompressed) - ResponseBank.expects(:acquire_lock).with(handler.versioned_key_hash) - handler.run! - assert_env(false, 'server') + @cache_store.expects(:read).with(handler.cache_key_hash, raw: true).returns(page_cache_entry(false)) + ResponseBank.expects(:acquire_lock).with(handler.entity_tag_hash) + expect_page_rendered(page(false)) + + assert_cache_miss(false, 'server') end def test_server_recent_cache_acceptable_but_none_found @controller.stubs(:cache_age_tolerance_in_seconds).returns(999999999999) _, _, body = handler.run! - assert_equal('some text', body) - assert_env(true, :anything) + assert_equal('dynamic output', body) + assert_cache_miss(true, :anything) end def test_nil_timestamp_in_second_lookup_causes_a_cache_miss ResponseBank.stubs(:acquire_lock).returns(false) @controller.stubs(:cache_age_tolerance_in_seconds).returns(999999999999) - @cache_store.expects(:read).with(handler.unversioned_key_hash, raw: true).returns(MessagePack.dump(page[0..2])) + cache_page = page(false) + @cache_store.expects(:read).with(handler.cache_key_hash, raw: true).returns(MessagePack.dump(cache_page[0..2])) + handler.run! + + assert_cache_miss(true, :anything) + end + + def test_server_recent_cache_miss + @controller.stubs(:cache_age_tolerance_in_seconds).returns(999999999999) + @cache_store.expects(:read).with(handler.cache_key_hash, raw: true).returns(page_cache_entry(false)) + + ResponseBank.expects(:acquire_lock).with(handler.entity_tag_hash).returns(true) handler.run! - assert_env(true, :anything) + + assert_cache_miss(true, 'server') end + def test_recent_cache_available_but_not_acceptable ResponseBank.stubs(:acquire_lock).returns(false) @controller.stubs(:cache_age_tolerance_in_seconds).returns(15) - @cache_store.expects(:read).with(handler.unversioned_key_hash, raw: true).returns(page_serialized) + @cache_store.expects(:read).with(handler.cache_key_hash, raw: true).returns(page_cache_entry(false)) _, _, body = handler.run! - assert_equal('some text', body) - assert_env(true, :anything) + assert_equal('dynamic output', body) + assert_cache_miss(true, :anything) end def test_force_refill_cache @controller.stubs(force_refill_cache?: true) - controller.request.env['HTTP_IF_NONE_MATCH'] = handler.versioned_key_hash - @cache_store.stubs(:read).with(handler.versioned_key_hash, raw: true).returns(page_serialized) + controller.request.env['HTTP_IF_NONE_MATCH'] = handler.entity_tag_hash + @cache_store.expects(:read).with(handler.cache_key_hash, raw: true).never _, _, body = handler.run! - assert_env(true, nil) - assert_equal('some text', body) + assert_cache_miss(true, nil) + assert_equal('dynamic output', body) end def test_serve_unversioned_cacheable_entry - controller.request.env['gzip'] = false assert(@controller.respond_to?(:serve_unversioned_cacheable_entry?, true)) - @controller.expects(:serve_unversioned_cacheable_entry?).returns(true).times(4) - @cache_store.expects(:read).with(handler.unversioned_key_hash, raw: true).returns(page_serialized) - expect_page_rendered(page_uncompressed) - handler.run! - assert_env(false, 'server') + @controller.expects(:serve_unversioned_cacheable_entry?).returns(true).times(1) + @cache_store.expects(:read).with(handler.cache_key_hash, raw: true).returns(page_cache_entry) + expect_page_rendered(page) + assert_cache_miss(false, 'server') end def test_double_render_still_renders @@ -177,11 +192,15 @@ def test_double_render_still_renders handler.run! end - def assert_env(miss, store) - vkh = handler.versioned_key_hash - uvkh = handler.unversioned_key_hash - assert_equal(true, controller.request.env['cacheable.cache']) - assert_equal(miss, controller.request.env['cacheable.miss']) + def assert_cache_miss(miss, store) + etag = handler.entity_tag_hash + unversioned_cache_key = handler.cache_key_hash + assert_equal(true, controller.request.env['cacheable.cache']) + assert_equal(miss, controller.request.env['cacheable.miss']) + + if (miss) + assert_equal(true, controller.request.env['cacheable.locked']) + end if store.nil? assert_nil(controller.request.env['cacheable.store']) @@ -189,22 +208,23 @@ def assert_env(miss, store) assert_equal(store, controller.request.env['cacheable.store']) end - assert_equal(vkh, controller.request.env['cacheable.key']) - assert_equal(uvkh, controller.request.env['cacheable.unversioned-key']) + assert_equal(etag, controller.request.env['cacheable.key']) + assert_equal(unversioned_cache_key, controller.request.env['cacheable.unversioned-key']) end - def expect_page_rendered(page) - _status, content_type, body, _timestamp = page - ResponseBank.expects(:decompress).returns(body).once + def expect_page_rendered(cache_entry, compressed_body = true) + controller.request.env['gzip'] = compressed_body - @controller.response.headers.expects(:[]=).with('Content-Type', content_type) - end + _status, _headers, _body, _timestamp = cache_entry + ResponseBank.expects(:decompress).never if compressed_body + ResponseBank.expects(:decompress).returns(_body).once unless compressed_body + + status, headers, body = handler.run! - def expect_compressed_page_rendered(page) - _status, content_type, _body, _timestamp = page - ResponseBank.expects(:decompress).never + assert_equal(status, _status) + assert_equal(headers["Content-Type"], _headers['Content-Type']) + assert_equal(headers["Content-Encoding"], "gzip") if compressed_body - @controller.response.headers.expects(:[]=).with('Content-Type', content_type) - @controller.response.headers.expects(:[]=).with('Content-Encoding', "gzip") + body end end From 9edf8c3f8ca4f904cf826a87409f6a2a8d88178a Mon Sep 17 00:00:00 2001 From: Colin Bendell Date: Mon, 27 Mar 2023 17:33:26 -0400 Subject: [PATCH 2/4] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Rafael Mendonça França --- lib/response_bank/response_cache_handler.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/response_bank/response_cache_handler.rb b/lib/response_bank/response_cache_handler.rb index 7ee28a1..f6115e8 100644 --- a/lib/response_bank/response_cache_handler.rb +++ b/lib/response_bank/response_cache_handler.rb @@ -131,13 +131,13 @@ def serve_from_cache(cache_key_hash, match_entity_tag = "*", cache_age_tolerance if ResponseBank.acquire_lock(match_entity_tag) # execute if we can get the lock @env['cacheable.locked'] = true - return nil + return elsif stale_while_revalidate?(timestamp, cache_age_tolerance) # cache is being regenerated, can we avoid piling on and use a stale version in the interim? ResponseBank.log("Cache hit: server (recent)") else ResponseBank.log("Found an unversioned cache entry, but it was too old (#{timestamp})") - return nil + return end end From cf8faa3422ecd5a5b5dc675ef14d476575099322 Mon Sep 17 00:00:00 2001 From: Colin Bendell Date: Tue, 28 Mar 2023 11:26:22 -0400 Subject: [PATCH 3/4] version the cache key schema to avoid polyfill requirements --- lib/response_bank.rb | 6 +++--- lib/response_bank/response_cache_handler.rb | 13 ++++++------- test/response_bank_test.rb | 11 +++++++++++ 3 files changed, 20 insertions(+), 10 deletions(-) diff --git a/lib/response_bank.rb b/lib/response_bank.rb index 671b9b7..92a1325 100644 --- a/lib/response_bank.rb +++ b/lib/response_bank.rb @@ -49,11 +49,11 @@ def cache_key_for(data) key = hash_value_str(data[:key]) - return key unless data.key?(:version) + key = %{#{data[:key_schema_version]}:#{key}} unless data[:key_schema_version].blank? - version = hash_value_str(data[:version]) + key = %{#{key}:#{hash_value_str(data[:version])}} unless data[:version].blank? - [key, version].join(":") + key when Array data.inspect when Time, DateTime diff --git a/lib/response_bank/response_cache_handler.rb b/lib/response_bank/response_cache_handler.rb index f6115e8..d92d197 100644 --- a/lib/response_bank/response_cache_handler.rb +++ b/lib/response_bank/response_cache_handler.rb @@ -3,6 +3,8 @@ module ResponseBank class ResponseCacheHandler + CACHE_KEY_SCHEMA_VERSION = 1 + def initialize( key_data:, version_data:, @@ -25,6 +27,7 @@ def initialize( @force_refill_cache = force_refill_cache @cache_store = cache_store @headers = headers || {} + @key_schema_version = @env.key?('cacheable.key_version') ? @env.key['cacheable.key_version'] : CACHE_KEY_SCHEMA_VERSION end def run! @@ -56,11 +59,11 @@ def hash(key) end def entity_tag - @entity_tag ||= ResponseBank.cache_key_for(key: @key_data, version: @version_data) + @entity_tag ||= ResponseBank.cache_key_for(key: @key_data, version: @version_data, key_schema_version: @key_schema_version) end def cache_key - @cache_key ||= ResponseBank.cache_key_for(key: @key_data) + @cache_key ||= ResponseBank.cache_key_for(key: @key_data, key_schema_version: @key_schema_version) end def cacheable_info_dump @@ -112,11 +115,7 @@ def serve_from_cache(cache_key_hash, match_entity_tag = "*", cache_age_tolerance @env['cacheable.miss'] = false @env['cacheable.store'] = 'server' - status, headers, body, timestamp, location = hit - - # polyfill headers for legacy versions - headers = { 'Content-Type' => headers.to_s } if headers.is_a? String - headers['Location'] = location if location + status, headers, body, timestamp = hit @env['cacheable.locked'] ||= false diff --git a/test/response_bank_test.rb b/test/response_bank_test.rb index 1240994..052be86 100644 --- a/test/response_bank_test.rb +++ b/test/response_bank_test.rb @@ -33,6 +33,17 @@ def test_cache_key_with_key_and_version assert_equal(expected, ResponseBank.cache_key_for(key: @data, version: version)) end + def test_cache_key_with_version + key = "/index.html" + version = 42 + assert_equal('/index.html', ResponseBank.cache_key_for({key: key})) + assert_equal('/index.html:42', ResponseBank.cache_key_for({key: key, version: version})) + assert_equal('/index.html:42', ResponseBank.cache_key_for({key: key, version: version, key_schema_version: nil})) + assert_equal('/index.html:42', ResponseBank.cache_key_for({key: key, version: version, key_schema_version: ""})) + assert_equal('1:/index.html:42', ResponseBank.cache_key_for({key: key, version: version, key_schema_version: 1})) + end + + def test_cache_key_for_array assert_equal('["foo", "bar", "baz"]', ResponseBank.cache_key_for(%w[foo bar baz])) end From 2902eb736de1cc5973a686a6b9c6d663bafc54cc Mon Sep 17 00:00:00 2001 From: Colin Bendell Date: Tue, 28 Mar 2023 14:43:02 -0400 Subject: [PATCH 4/4] cleaned up conditional formatting of the cache key --- lib/response_bank.rb | 4 ++-- test/response_bank_test.rb | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/response_bank.rb b/lib/response_bank.rb index 92a1325..ac5ed30 100644 --- a/lib/response_bank.rb +++ b/lib/response_bank.rb @@ -49,9 +49,9 @@ def cache_key_for(data) key = hash_value_str(data[:key]) - key = %{#{data[:key_schema_version]}:#{key}} unless data[:key_schema_version].blank? + key = %{#{data[:key_schema_version]}:#{key}} if data[:key_schema_version] - key = %{#{key}:#{hash_value_str(data[:version])}} unless data[:version].blank? + key = %{#{key}:#{hash_value_str(data[:version])}} if data[:version] key when Array diff --git a/test/response_bank_test.rb b/test/response_bank_test.rb index 052be86..e312a4c 100644 --- a/test/response_bank_test.rb +++ b/test/response_bank_test.rb @@ -39,7 +39,6 @@ def test_cache_key_with_version assert_equal('/index.html', ResponseBank.cache_key_for({key: key})) assert_equal('/index.html:42', ResponseBank.cache_key_for({key: key, version: version})) assert_equal('/index.html:42', ResponseBank.cache_key_for({key: key, version: version, key_schema_version: nil})) - assert_equal('/index.html:42', ResponseBank.cache_key_for({key: key, version: version, key_schema_version: ""})) assert_equal('1:/index.html:42', ResponseBank.cache_key_for({key: key, version: version, key_schema_version: 1})) end