Use the new CacheableAttributes concern in the ApplicationSetting and Appearance models

Signed-off-by: Rémy Coutable <remy@rymai.me>

5 files changed, 11 insertions, 98 deletions

diff --git a/app/models/appearance.rb b/app/models/appearance.rb
index f8713138a93..67cc84a9140 100644
--- a/app/models/appearance.rb
+++ b/app/models/appearance.rb
@@ -1,6 +1,6 @@
 class Appearance < ActiveRecord::Base
+  include CacheableAttributes
   include CacheMarkdownField
-  include AfterCommitQueue
   include ObjectStorage::BackgroundMove
   include WithUploads
 
@@ -15,16 +15,9 @@ class Appearance < ActiveRecord::Base
   mount_uploader :logo,         AttachmentUploader
   mount_uploader :header_logo,  AttachmentUploader
 
-  CACHE_KEY = "current_appearance:#{Gitlab::VERSION}".freeze
-
-  after_commit :flush_redis_cache
-
-  def self.current
-    Rails.cache.fetch(CACHE_KEY) { first }
-  end
-
-  def flush_redis_cache
-    Rails.cache.delete(CACHE_KEY)
+  # Overrides CacheableAttributes.current_without_cache
+  def self.current_without_cache
+    first
   end
 
   def single_appearance_row
diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index 451e512aef7..e8ccb320fae 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -1,11 +1,11 @@
 class ApplicationSetting < ActiveRecord::Base
+  include CacheableAttributes
   include CacheMarkdownField
   include TokenAuthenticatable
 
   add_authentication_token_field :runners_registration_token
   add_authentication_token_field :health_check_access_token
 
-  CACHE_KEY = 'application_setting.last'.freeze
   DOMAIN_LIST_SEPARATOR = %r{\s*[,;]\s*     # comma or semicolon, optionally surrounded by whitespace
                             |               # or
                             \s              # any whitespace character
@@ -229,40 +229,6 @@ class ApplicationSetting < ActiveRecord::Base
 
   after_commit do
     reset_memoized_terms
-    Rails.cache.write(CACHE_KEY, self)
-  end
-
-  def self.current
-    ensure_cache_setup
-
-    Rails.cache.fetch(CACHE_KEY) do
-      ApplicationSetting.last.tap do |settings|
-        # do not cache nils
-        raise 'missing settings' unless settings
-      end
-    end
-  rescue
-    # Fall back to an uncached value if there are any problems (e.g. redis down)
-    ApplicationSetting.last
-  end
-
-  def self.expire
-    Rails.cache.delete(CACHE_KEY)
-  rescue
-    # Gracefully handle when Redis is not available. For example,
-    # omnibus may fail here during gitlab:assets:compile.
-  end
-
-  def self.cached
-    value = Rails.cache.read(CACHE_KEY)
-    ensure_cache_setup if value.present?
-    value
-  end
-
-  def self.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
-    ApplicationSetting.define_attribute_methods
   end
 
   def self.defaults
diff --git a/app/uploaders/object_storage.rb b/app/uploaders/object_storage.rb
index a3549cada95..f2a8afccdeb 100644
--- a/app/uploaders/object_storage.rb
+++ b/app/uploaders/object_storage.rb
@@ -103,6 +103,7 @@ module ObjectStorage
     end
 
     included do
+      include AfterCommitQueue
       after_save on: [:create, :update] do
         background_upload(changed_mounts)
       end
diff --git a/spec/models/appearance_spec.rb b/spec/models/appearance_spec.rb
index 5489c17bd82..77b07cf1ac9 100644
--- a/spec/models/appearance_spec.rb
+++ b/spec/models/appearance_spec.rb
@@ -3,34 +3,11 @@ require 'rails_helper'
 describe Appearance do
   subject { build(:appearance) }
 
-  it { is_expected.to be_valid }
+  it { include(CacheableAttributes) }
+  it { expect(described_class.current_without_cache).to eq(described_class.first) }
 
   it { is_expected.to have_many(:uploads) }
 
-  describe '.current', :use_clean_rails_memory_store_caching do
-    let!(:appearance) { create(:appearance) }
-
-    it 'returns the current appearance row' do
-      expect(described_class.current).to eq(appearance)
-    end
-
-    it 'caches the result' do
-      expect(described_class).to receive(:first).once
-
-      2.times { described_class.current }
-    end
-  end
-
-  describe '#flush_redis_cache' do
-    it 'flushes the cache in Redis' do
-      appearance = create(:appearance)
-
-      expect(Rails.cache).to receive(:delete).with(described_class::CACHE_KEY)
-
-      appearance.flush_redis_cache
-    end
-  end
-
   describe '#single_appearance_row' do
     it 'adds an error when more than 1 row exists' do
       create(:appearance)
diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb
index 10d6109cae7..7e47043a1cb 100644
--- a/spec/models/application_setting_spec.rb
+++ b/spec/models/application_setting_spec.rb
@@ -3,6 +3,9 @@ require 'spec_helper'
 describe ApplicationSetting do
   let(:setting) { described_class.create_from_defaults }
 
+  it { include(CacheableAttributes) }
+  it { expect(described_class.current_without_cache).to eq(described_class.last) }
+
   it { expect(setting).to be_valid }
   it { expect(setting.uuid).to be_present }
   it { expect(setting).to have_db_column(:auto_devops_enabled) }
@@ -318,33 +321,6 @@ describe ApplicationSetting do
     end
   end
 
-  describe '.current' do
-    context 'redis unavailable' do
-      it 'returns an ApplicationSetting' do
-        allow(Rails.cache).to receive(:fetch).and_call_original
-        allow(described_class).to receive(:last).and_return(:last)
-        expect(Rails.cache).to receive(:fetch).with(ApplicationSetting::CACHE_KEY).and_raise(ArgumentError)
-
-        expect(described_class.current).to eq(:last)
-      end
-    end
-
-    context 'when an ApplicationSetting is not yet present' do
-      it 'does not cache nil object' do
-        # when missing settings a nil object is returned, but not cached
-        allow(described_class).to receive(:last).and_return(nil).twice
-        expect(described_class.current).to be_nil
-
-        # when the settings are set the method returns a valid object
-        allow(described_class).to receive(:last).and_return(:last)
-        expect(described_class.current).to eq(:last)
-
-        # subsequent calls get everything from cache
-        expect(described_class.current).to eq(:last)
-      end
-    end
-  end
-
   context 'restrict creating duplicates' do
     before do
       described_class.create_from_defaults