From 25b47c25bac3375c0aa1233dae48f02077a17765 Mon Sep 17 00:00:00 2001 From: Alex Ghiculescu Date: Tue, 22 Jun 2021 13:58:19 -0500 Subject: [PATCH] Optimized Cache::Entry should support old dalli_store values Same bug as https://github.com/rails/rails/pull/42559, but a very different fix due to https://github.com/rails/rails/pull/42025. To recap, the issue: - While using the `dalli_store`, you set any value in the Rails cache with no expiry. - You change to the `mem_cache_store`. - You upgrade to Rails 7. - You try to read the same cache key you set in step 1, and it crashes on read. https://github.com/rails/rails/pull/42025 was backward compatible with entries written using the `mem_cache_store`, but *not* the `dalli_store` which did not use `ActiveSupport::Cache::Entry`. This PR attempts to fix that. --- activesupport/lib/active_support/cache.rb | 10 ++++-- .../cache/behaviors/local_cache_behavior.rb | 7 ++++ .../test/cache/stores/mem_cache_store_test.rb | 36 +++++++++++++++++++ 3 files changed, 51 insertions(+), 2 deletions(-) diff --git a/activesupport/lib/active_support/cache.rb b/activesupport/lib/active_support/cache.rb index bd96e60c95..0ec98fd901 100644 --- a/activesupport/lib/active_support/cache.rb +++ b/activesupport/lib/active_support/cache.rb @@ -844,14 +844,20 @@ module ActiveSupport extend self def load(payload) - if payload.start_with?(MARK_70_UNCOMPRESSED) + if !payload.is_a?(String) + ActiveSupport::Cache::Store.logger&.warn %{Payload wasn't a string, was #{payload.class.name} - couldn't unmarshal, so returning nil."} + + return nil + elsif payload.start_with?(MARK_70_UNCOMPRESSED) members = Marshal.load(payload.byteslice(1..-1)) elsif payload.start_with?(MARK_70_COMPRESSED) members = Marshal.load(Zlib::Inflate.inflate(payload.byteslice(1..-1))) elsif payload.start_with?(MARK_61) return Marshal.load(payload) else - raise ArgumentError, %{Invalid cache prefix: #{payload.byteslice(0).inspect}, expected "\\x00" or "\\x01"} + ActiveSupport::Cache::Store.logger&.warn %{Invalid cache prefix: #{payload.byteslice(0).inspect}, expected "\\x00" or "\\x01"} + + return nil end Entry.unpack(members) end diff --git a/activesupport/test/cache/behaviors/local_cache_behavior.rb b/activesupport/test/cache/behaviors/local_cache_behavior.rb index 125741de3d..7a7f9f066b 100644 --- a/activesupport/test/cache/behaviors/local_cache_behavior.rb +++ b/activesupport/test/cache/behaviors/local_cache_behavior.rb @@ -228,4 +228,11 @@ module LocalCacheBehavior end end end + + def test_local_cache_should_read_and_write_false + @cache.with_local_cache do + assert @cache.write("foo", false) + assert_equal false, @cache.read("foo") + end + end end diff --git a/activesupport/test/cache/stores/mem_cache_store_test.rb b/activesupport/test/cache/stores/mem_cache_store_test.rb index 9891cb3fd3..47e4999ea0 100644 --- a/activesupport/test/cache/stores/mem_cache_store_test.rb +++ b/activesupport/test/cache/stores/mem_cache_store_test.rb @@ -232,6 +232,42 @@ class MemCacheStoreTest < ActiveSupport::TestCase assert_compressed(LARGE_OBJECT) end + def test_can_load_raw_values_from_dalli_store + key = "test-with-value-the-way-the-dalli-store-did" + + @cache.instance_variable_get(:@data).with { |c| c.set(@cache.send(:normalize_key, key, nil), "value", 0, compress: false) } + assert_nil @cache.read(key) + assert_equal "value", @cache.fetch(key) { "value" } + end + + def test_can_load_raw_falsey_values_from_dalli_store + key = "test-with-false-value-the-way-the-dalli-store-did" + + @cache.instance_variable_get(:@data).with { |c| c.set(@cache.send(:normalize_key, key, nil), false, 0, compress: false) } + assert_nil @cache.read(key) + assert_equal false, @cache.fetch(key) { false } + end + + def test_can_load_raw_values_from_dalli_store_with_local_cache + key = "test-with-value-the-way-the-dalli-store-did-with-local-cache" + + @cache.instance_variable_get(:@data).with { |c| c.set(@cache.send(:normalize_key, key, nil), "value", 0, compress: false) } + @cache.with_local_cache do + assert_nil @cache.read(key) + assert_equal "value", @cache.fetch(key) { "value" } + end + end + + def test_can_load_raw_falsey_values_from_dalli_store_with_local_cache + key = "test-with-false-value-the-way-the-dalli-store-did-with-local-cache" + + @cache.instance_variable_get(:@data).with { |c| c.set(@cache.send(:normalize_key, key, nil), false, 0, compress: false) } + @cache.with_local_cache do + assert_nil @cache.read(key) + assert_equal false, @cache.fetch(key) { false } + end + end + private def random_string(length) (0...length).map { (65 + rand(26)).chr }.join