From a886532cc04bfa7ec6885ea883889f6d138961bd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9my=20Coutable?= <remy@rymai.me>
Date: Tue, 29 May 2018 17:49:52 +0200
Subject: [PATCH] Revert to caching the AR object in CacheableAttributes
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Caching the attributes as JSON and manually instantiating the record
ended-up very complex since the edge-cases such as upload fields,
serialized fields, and fields with custom accessors had to be handled.

To ensure 3 points out of 4 are checked from https://gitlab.com/gitlab-org/gitlab-ce/issues/45175
we now include the Rails version in the cache key.

Signed-off-by: Rémy Coutable <remy@rymai.me>
---
 app/models/concerns/cacheable_attributes.rb   |  34 +++--
 .../concerns/cacheable_attributes_spec.rb     | 124 ++++++++++++++----
 2 files changed, 121 insertions(+), 37 deletions(-)

diff --git a/app/models/concerns/cacheable_attributes.rb b/app/models/concerns/cacheable_attributes.rb
index b32459fdabf..dd4fccc811d 100644
--- a/app/models/concerns/cacheable_attributes.rb
+++ b/app/models/concerns/cacheable_attributes.rb
@@ -6,15 +6,16 @@ module CacheableAttributes
   end
 
   class_methods do
+    def cache_key
+      "#{name}:#{Gitlab::VERSION}:#{Gitlab.migrations_hash}:#{Rails.version}".freeze
+    end
+
     # Can be overriden
     def current_without_cache
       last
     end
 
-    def cache_key
-      "#{name}:#{Gitlab::VERSION}:#{Gitlab.migrations_hash}:json".freeze
-    end
-
+    # Can be overriden
     def defaults
       {}
     end
@@ -24,10 +25,14 @@ module CacheableAttributes
     end
 
     def cached
-      json_attributes = Rails.cache.read(cache_key)
-      return nil unless json_attributes.present?
+      retrieve_from_cache
+    end
 
-      build_from_defaults(JSON.parse(json_attributes))
+    def retrieve_from_cache
+      record = Rails.cache.read(cache_key)
+      ensure_cache_setup if record.present?
+
+      record
     end
 
     def current
@@ -35,7 +40,12 @@ module CacheableAttributes
       return cached_record if cached_record.present?
 
       current_without_cache.tap { |current_record| current_record&.cache! }
-    rescue
+    rescue => e
+      if Rails.env.production?
+        Rails.logger.warn("Cached record for #{name} couldn't be loaded, falling back to uncached record: #{e}")
+      else
+        raise e
+      end
       # Fall back to an uncached value if there are any problems (e.g. Redis down)
       current_without_cache
     end
@@ -46,9 +56,15 @@ module CacheableAttributes
       # Gracefully handle when Redis is not available. For example,
       # omnibus may fail here during gitlab:assets:compile.
     end
+
+    def ensure_cache_setup
+      # This is a workaround for a Rails bug that causes attribute methods not
+      # to be loaded when read from cache: https://github.com/rails/rails/issues/27348
+      define_attribute_methods
+    end
   end
 
   def cache!
-    Rails.cache.write(self.class.cache_key, attributes.to_json)
+    Rails.cache.write(self.class.cache_key, self)
   end
 end
diff --git a/spec/models/concerns/cacheable_attributes_spec.rb b/spec/models/concerns/cacheable_attributes_spec.rb
index 49e4b23ebc7..77f46518d47 100644
--- a/spec/models/concerns/cacheable_attributes_spec.rb
+++ b/spec/models/concerns/cacheable_attributes_spec.rb
@@ -22,7 +22,7 @@ describe CacheableAttributes do
 
       attr_accessor :attributes
 
-      def initialize(attrs = {})
+      def initialize(attrs = {}, *)
         @attributes = attrs
       end
     end
@@ -52,7 +52,7 @@ describe CacheableAttributes do
 
   describe '.cache_key' do
     it 'excludes cache attributes' do
-      expect(minimal_test_class.cache_key).to eq("TestClass:#{Gitlab::VERSION}:#{Gitlab.migrations_hash}:json")
+      expect(minimal_test_class.cache_key).to eq("TestClass:#{Gitlab::VERSION}:#{Gitlab.migrations_hash}:#{Rails.version}")
     end
   end
 
@@ -75,47 +75,106 @@ describe CacheableAttributes do
 
     context 'without any attributes given' do
       it 'intializes a new object with the defaults' do
-        expect(minimal_test_class.build_from_defaults).not_to be_persisted
+        expect(minimal_test_class.build_from_defaults.attributes).to eq(minimal_test_class.defaults)
       end
     end
 
-    context 'without attributes given' do
+    context 'with attributes given' do
       it 'intializes a new object with the given attributes merged into the defaults' do
         expect(minimal_test_class.build_from_defaults(foo: 'd').attributes[:foo]).to eq('d')
       end
     end
+
+    describe 'edge cases on concrete implementations' do
+      describe '.build_from_defaults' do
+        context 'without any attributes given' do
+          it 'intializes all attributes even if they are nil' do
+            record = ApplicationSetting.build_from_defaults
+
+            expect(record).not_to be_persisted
+            expect(record.sign_in_text).to be_nil
+          end
+        end
+      end
+    end
   end
 
   describe '.current', :use_clean_rails_memory_store_caching do
     context 'redis unavailable' do
-      it 'returns an uncached record' do
+      before do
         allow(minimal_test_class).to receive(:last).and_return(:last)
-        expect(Rails.cache).to receive(:read).and_raise(Redis::BaseError)
+        expect(Rails.cache).to receive(:read).with(minimal_test_class.cache_key).and_raise(Redis::BaseError)
+      end
 
-        expect(minimal_test_class.current).to eq(:last)
+      context 'in production environment' do
+        before do
+          expect(Rails.env).to receive(:production?).and_return(true)
+        end
+
+        it 'returns an uncached record and logs a warning' do
+          expect(Rails.logger).to receive(:warn).with("Cached record for TestClass couldn't be loaded, falling back to uncached record: Redis::BaseError")
+
+          expect(minimal_test_class.current).to eq(:last)
+        end
+      end
+
+      context 'in other environments' do
+        before do
+          expect(Rails.env).to receive(:production?).and_return(false)
+        end
+
+        it 'returns an uncached record and logs a warning' do
+          expect(Rails.logger).not_to receive(:warn)
+
+          expect { minimal_test_class.current }.to raise_error(Redis::BaseError)
+        end
       end
     end
 
     context 'when a record is not yet present' do
       it 'does not cache nil object' do
         # when missing settings a nil object is returned, but not cached
-        allow(minimal_test_class).to receive(:last).twice.and_return(nil)
+        allow(ApplicationSetting).to receive(:current_without_cache).twice.and_return(nil)
 
-        expect(minimal_test_class.current).to be_nil
-        expect(Rails.cache.exist?(minimal_test_class.cache_key)).to be(false)
+        expect(ApplicationSetting.current).to be_nil
+        expect(Rails.cache.exist?(ApplicationSetting.cache_key)).to be(false)
       end
 
-      it 'cache non-nil object' do
-        # when the settings are set the method returns a valid object
-        allow(minimal_test_class).to receive(:last).and_call_original
+      it 'caches non-nil object' do
+        create(:application_setting)
 
-        expect(minimal_test_class.current).to eq(minimal_test_class.last)
-        expect(Rails.cache.exist?(minimal_test_class.cache_key)).to be(true)
+        expect(ApplicationSetting.current).to eq(ApplicationSetting.last)
+        expect(Rails.cache.exist?(ApplicationSetting.cache_key)).to be(true)
 
         # subsequent calls retrieve the record from the cache
-        last_record = minimal_test_class.last
-        expect(minimal_test_class).not_to receive(:last)
-        expect(minimal_test_class.current.attributes).to eq(last_record.attributes)
+        last_record = ApplicationSetting.last
+        expect(ApplicationSetting).not_to receive(:current_without_cache)
+        expect(ApplicationSetting.current.attributes).to eq(last_record.attributes)
+      end
+    end
+
+    describe 'edge cases' do
+      describe 'caching behavior', :use_clean_rails_memory_store_caching do
+        it 'retrieves upload fields properly' do
+          ar_record = create(:appearance, :with_logo)
+          ar_record.cache!
+
+          cache_record = Appearance.current
+
+          expect(cache_record).to be_persisted
+          expect(cache_record.logo).to be_an(AttachmentUploader)
+          expect(cache_record.logo.url).to end_with('/dk.png')
+        end
+
+        it 'retrieves markdown fields properly' do
+          ar_record = create(:appearance, description: '**Hello**')
+          ar_record.cache!
+
+          cache_record = Appearance.current
+
+          expect(cache_record.description).to eq('**Hello**')
+          expect(cache_record.description_html).to eq('<p dir="auto"><strong>Hello</strong></p>')
+        end
       end
     end
   end
@@ -127,27 +186,36 @@ describe CacheableAttributes do
       end
     end
 
-    context 'when cached settings do not include the latest defaults' do
+    context 'when cached is warm' do
       before do
-        Rails.cache.write(minimal_test_class.cache_key, { bar: 'b', baz: 'c' }.to_json)
-        minimal_test_class.define_singleton_method(:defaults) do
-          { foo: 'a', bar: 'b', baz: 'c' }
-        end
+        # Warm up the cache
+        create(:appearance).cache!
       end
 
-      it 'includes attributes from defaults' do
-        expect(minimal_test_class.cached.attributes[:foo]).to eq(minimal_test_class.defaults[:foo])
+      it 'retrieves the record from cache' do
+        expect(ActiveRecord::QueryRecorder.new { Appearance.cached }.count).to eq(0)
+        expect(Appearance.cached).to eq(Appearance.current_without_cache)
       end
     end
   end
 
   describe '#cache!', :use_clean_rails_memory_store_caching do
-    let(:appearance_record) { create(:appearance) }
+    let(:record) { create(:appearance) }
 
     it 'caches the attributes' do
-      appearance_record.cache!
+      record.cache!
 
-      expect(Rails.cache.read(Appearance.cache_key)).to eq(appearance_record.attributes.to_json)
+      expect(Rails.cache.read(Appearance.cache_key)).to eq(record)
+    end
+
+    describe 'edge cases' do
+      let(:record) { create(:appearance) }
+
+      it 'caches the attributes' do
+        record.cache!
+
+        expect(Rails.cache.read(Appearance.cache_key)).to eq(record)
+      end
     end
   end
 end