Merge branch...

Merge branch '45175-cache-json-instead-of-activerecord-objects-in-appearance-and-applicationsetting' into 'master'

Resolve "Cache JSON instead of ActiveRecord objects in `Appearance` and `ApplicationSetting`"

Closes #45175

See merge request gitlab-org/gitlab-ce!18754

app/controllers/admin/application_settings_controller.rb

@@ -52,7 +52,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
   private
 
   def set_application_setting
-    @application_setting = ApplicationSetting.current
+    @application_setting = ApplicationSetting.current_without_cache
   end
 
   def application_setting_params

app/models/appearance.rb

 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

app/models/application_setting.rb

 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

app/models/concerns/cacheable_attributes.rb

+module CacheableAttributes
+  extend ActiveSupport::Concern
+
+  included do
+    after_commit { self.class.expire }
+  end
+
+  class_methods do
+    # Can be overriden
+    def current_without_cache
+      last
+    end
+
+    def cache_key
+      "#{name}:#{Gitlab::VERSION}:#{Gitlab.migrations_hash}:json".freeze
+    end
+
+    def defaults
+      {}
+    end
+
+    def build_from_defaults(attributes = {})
+      new(defaults.merge(attributes))
+    end
+
+    def cached
+      json_attributes = Rails.cache.read(cache_key)
+      return nil unless json_attributes.present?
+
+      build_from_defaults(JSON.parse(json_attributes))
+    end
+
+    def current
+      cached_record = cached
+      return cached_record if cached_record.present?
+
+      current_without_cache.tap { |current_record| current_record&.cache! }
+    rescue
+      # Fall back to an uncached value if there are any problems (e.g. Redis down)
+      current_without_cache
+    end
+
+    def 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
+  end
+
+  def cache!
+    Rails.cache.write(self.class.cache_key, attributes.to_json)
+  end
+end

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

config/initializers/1_settings.rb

@@ -470,6 +470,3 @@ if Rails.env.test?
   Settings.gitlab['default_can_create_group'] = true
   Settings.gitlab['default_can_create_team']  = false
 end
-
-# Force a refresh of application settings at startup
-ApplicationSetting.expire

lib/api/settings.rb

@@ -5,7 +5,7 @@ module API
     helpers do
       def current_settings
         @current_setting ||=
-          (ApplicationSetting.current || ApplicationSetting.create_from_defaults)
+          (ApplicationSetting.current_without_cache || ApplicationSetting.create_from_defaults)
       end
     end
 

lib/api/v3/settings.rb

@@ -6,7 +6,7 @@ module API
       helpers do
         def current_settings
           @current_setting ||=
-            (ApplicationSetting.current || ApplicationSetting.create_from_defaults)
+            (ApplicationSetting.current_without_cache || ApplicationSetting.create_from_defaults)
         end
       end
 

lib/banzai/filter/plantuml_filter.rb

@@ -23,7 +23,7 @@ module Banzai
       private
 
       def settings
-        ApplicationSetting.current || ApplicationSetting.create_from_defaults
+        Gitlab::CurrentSettings.current_application_settings
       end
 
       def plantuml_setup

lib/gitlab.rb

@@ -9,6 +9,10 @@ module Gitlab
     Settings
   end
 
+  def self.migrations_hash
+    @_migrations_hash ||= Digest::MD5.hexdigest(ActiveRecord::Migrator.get_all_versions.to_s)
+  end
+
   COM_URL = 'https://gitlab.com'.freeze
   APP_DIRS_PATTERN = %r{^/?(app|config|ee|lib|spec|\(\w*\))}
   SUBDOMAIN_REGEX = %r{\Ahttps://[a-z0-9]+\.gitlab\.com\z}

lib/gitlab/current_settings.rb

@@ -9,8 +9,8 @@ module Gitlab
         end
       end
 
-      def fake_application_settings(defaults = ::ApplicationSetting.defaults)
-        Gitlab::FakeApplicationSettings.new(defaults)
+      def fake_application_settings(attributes = {})
+        Gitlab::FakeApplicationSettings.new(::ApplicationSetting.defaults.merge(attributes || {}))
       end
 
       def method_missing(name, *args, &block)
@@ -25,43 +25,35 @@ module Gitlab
 
       def ensure_application_settings!
         return in_memory_application_settings if ENV['IN_MEMORY_APPLICATION_SETTINGS'] == 'true'
-
-        cached_application_settings || uncached_application_settings
-      end
-
-      def cached_application_settings
-        begin
-          ::ApplicationSetting.cached
-        rescue ::Redis::BaseError, ::Errno::ENOENT, ::Errno::EADDRNOTAVAIL
-          # In case Redis isn't running or the Redis UNIX socket file is not available
-        end
-      end
-
-      def uncached_application_settings
         return fake_application_settings unless connect_to_db?
 
-        db_settings = ::ApplicationSetting.current
-
+        current_settings = ::ApplicationSetting.current
         # If there are pending migrations, it's possible there are columns that
         # need to be added to the application settings. To prevent Rake tasks
         # and other callers from failing, use any loaded settings and return
         # defaults for missing columns.
         if ActiveRecord::Migrator.needs_migration?
-          defaults = ::ApplicationSetting.defaults
-          defaults.merge!(db_settings.attributes.symbolize_keys) if db_settings.present?
-          return fake_application_settings(defaults)
+          return fake_application_settings(current_settings&.attributes)
         end
 
-        return db_settings if db_settings.present?
+        return current_settings if current_settings.present?
 
+        with_fallback_to_fake_application_settings do
           ::ApplicationSetting.create_from_defaults || in_memory_application_settings
         end
+      end
 
       def in_memory_application_settings
-        @in_memory_application_settings ||= ::ApplicationSetting.new(::ApplicationSetting.defaults) # rubocop:disable Gitlab/ModuleWithInstanceVariables
-      rescue ActiveRecord::StatementInvalid, ActiveRecord::UnknownAttributeError
-        # In case migrations the application_settings table is not created yet,
-        # we fallback to a simple OpenStruct
+        with_fallback_to_fake_application_settings do
+          @in_memory_application_settings ||= ::ApplicationSetting.build_from_defaults # rubocop:disable Gitlab/ModuleWithInstanceVariables
+        end
+      end
+
+      def with_fallback_to_fake_application_settings(&block)
+        yield
+      rescue
+        # In case the application_settings table is not created yet, or if a new
+        # ApplicationSetting column is not yet migrated we fallback to a simple OpenStruct
         fake_application_settings
       end
 

spec/lib/gitlab/current_settings_spec.rb

 require 'spec_helper'
 
 describe Gitlab::CurrentSettings do
-  include StubENV
-
   before do
     stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false')
   end
 
-  describe '#current_application_settings' do
+  describe '#current_application_settings', :use_clean_rails_memory_store_caching do
     it 'allows keys to be called directly' do
       db_settings = create(:application_setting,
         home_page_url: 'http://mydomain.com',
@@ -19,46 +17,54 @@ describe Gitlab::CurrentSettings do
       expect(described_class.metrics_sample_interval).to be(15)
     end
 
-    context 'with DB available' do
+    context 'when ENV["IN_MEMORY_APPLICATION_SETTINGS"] is true' do
       before do
-        # For some reason, `allow(described_class).to receive(:connect_to_db?).and_return(true)` causes issues
-        # during the initialization phase of the test suite, so instead let's mock the internals of it
-        allow(ActiveRecord::Base.connection).to receive(:active?).and_return(true)
-        allow(ActiveRecord::Base.connection).to receive(:table_exists?).and_call_original
-        allow(ActiveRecord::Base.connection).to receive(:table_exists?).with('application_settings').and_return(true)
+        stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'true')
       end
 
-      it 'attempts to use cached values first' do
-        expect(ApplicationSetting).to receive(:cached)
+      it 'returns an in-memory ApplicationSetting object' do
+        expect(ApplicationSetting).not_to receive(:current)
 
         expect(described_class.current_application_settings).to be_a(ApplicationSetting)
+        expect(described_class.current_application_settings).not_to be_persisted
+      end
     end
 
-      it 'falls back to DB if Redis returns an empty value' do
-        expect(ApplicationSetting).to receive(:cached).and_return(nil)
-        expect(ApplicationSetting).to receive(:last).and_call_original.twice
-
-        expect(described_class.current_application_settings).to be_a(ApplicationSetting)
+    context 'with DB unavailable' do
+      before do
+        # For some reason, `allow(described_class).to receive(:connect_to_db?).and_return(false)` causes issues
+        # during the initialization phase of the test suite, so instead let's mock the internals of it
+        allow(ActiveRecord::Base.connection).to receive(:active?).and_return(false)
       end
 
-      it 'falls back to DB if Redis fails' do
-        db_settings = ApplicationSetting.create!(ApplicationSetting.defaults)
+      it 'returns an in-memory ApplicationSetting object' do
+        expect(ApplicationSetting).not_to receive(:current)
 
-        expect(ApplicationSetting).to receive(:cached).and_raise(::Redis::BaseError)
-        expect(Rails.cache).to receive(:fetch).with(ApplicationSetting::CACHE_KEY).and_raise(Redis::BaseError)
+        expect(described_class.current_application_settings).to be_a(Gitlab::FakeApplicationSettings)
+      end
+    end
 
-        expect(described_class.current_application_settings).to eq(db_settings)
+    context 'with DB available' do
+      # This method returns the ::ApplicationSetting.defaults hash
+      # but with respect of custom attribute accessors of ApplicationSetting model
+      def settings_from_defaults
+        ar_wrapped_defaults = ::ApplicationSetting.build_from_defaults.attributes
+        ar_wrapped_defaults.slice(*::ApplicationSetting.defaults.keys)
       end
 
-      it 'creates default ApplicationSettings if none are present' do
-        expect(ApplicationSetting).to receive(:cached).and_raise(::Redis::BaseError)
-        expect(Rails.cache).to receive(:fetch).with(ApplicationSetting::CACHE_KEY).and_raise(Redis::BaseError)
+      before do
+        # For some reason, `allow(described_class).to receive(:connect_to_db?).and_return(true)` causes issues
+        # during the initialization phase of the test suite, so instead let's mock the internals of it
+        allow(ActiveRecord::Base.connection).to receive(:active?).and_return(true)
+        allow(ActiveRecord::Base.connection).to receive(:cached_table_exists?).with('application_settings').and_return(true)
+      end
 
+      it 'creates default ApplicationSettings if none are present' do
         settings = described_class.current_application_settings
 
         expect(settings).to be_a(ApplicationSetting)
         expect(settings).to be_persisted
-        expect(settings).to have_attributes(ApplicationSetting.defaults)
+        expect(settings).to have_attributes(settings_from_defaults)
       end
 
       context 'with migrations pending' do
@@ -69,7 +75,7 @@ describe Gitlab::CurrentSettings do
         it 'returns an in-memory ApplicationSetting object' do
           settings = described_class.current_application_settings
 
-          expect(settings).to be_a(OpenStruct)
+          expect(settings).to be_a(Gitlab::FakeApplicationSettings)
           expect(settings.sign_in_enabled?).to eq(settings.sign_in_enabled)
           expect(settings.sign_up_enabled?).to eq(settings.sign_up_enabled)
         end
@@ -81,7 +87,7 @@ describe Gitlab::CurrentSettings do
           settings = described_class.current_application_settings
           app_defaults = ApplicationSetting.last
 
-          expect(settings).to be_a(OpenStruct)
+          expect(settings).to be_a(Gitlab::FakeApplicationSettings)
           expect(settings.home_page_url).to eq(db_settings.home_page_url)
           expect(settings.signup_enabled?).to be_falsey
           expect(settings.signup_enabled).to be_falsey
@@ -91,34 +97,29 @@ describe Gitlab::CurrentSettings do
           settings.each { |key, _| expect(settings[key]).to eq(app_defaults[key]) }
         end
       end
-    end
 
-    context 'with DB unavailable' do
-      before do
-        # For some reason, `allow(described_class).to receive(:connect_to_db?).and_return(false)` causes issues
-        # during the initialization phase of the test suite, so instead let's mock the internals of it
-        allow(ActiveRecord::Base.connection).to receive(:active?).and_return(false)
+      context 'when ApplicationSettings.current is present' do
+        it 'returns the existing application settings' do
+          expect(ApplicationSetting).to receive(:current).and_return(:current_settings)
+
+          expect(described_class.current_application_settings).to eq(:current_settings)
+        end
       end
 
+      context 'when the application_settings table does not exists' do
         it 'returns an in-memory ApplicationSetting object' do
-        expect(ApplicationSetting).not_to receive(:current)
-        expect(ApplicationSetting).not_to receive(:last)
+          expect(ApplicationSetting).to receive(:create_from_defaults).and_raise(ActiveRecord::StatementInvalid)
 
-        expect(described_class.current_application_settings).to be_a(OpenStruct)
+          expect(described_class.current_application_settings).to be_a(Gitlab::FakeApplicationSettings)
         end
       end
 
-    context 'when ENV["IN_MEMORY_APPLICATION_SETTINGS"] is true' do
-      before do
-        stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'true')
-      end
-
+      context 'when the application_settings table is not fully migrated' do
         it 'returns an in-memory ApplicationSetting object' do
-        expect(ApplicationSetting).not_to receive(:current)
-        expect(ApplicationSetting).not_to receive(:last)
+          expect(ApplicationSetting).to receive(:create_from_defaults).and_raise(ActiveRecord::UnknownAttributeError)
 
-        expect(described_class.current_application_settings).to be_a(ApplicationSetting)
-        expect(described_class.current_application_settings).not_to be_persisted
+          expect(described_class.current_application_settings).to be_a(Gitlab::FakeApplicationSettings)
+        end
       end
     end
   end

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)

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

spec/models/concerns/cacheable_attributes_spec.rb

+require 'spec_helper'
+
+describe CacheableAttributes do
+  let(:minimal_test_class) do
+    Class.new do
+      include ActiveModel::Model
+      extend ActiveModel::Callbacks
+      define_model_callbacks :commit
+      include CacheableAttributes
+
+      def self.name
+        'TestClass'
+      end
+
+      def self.first
+        @_first ||= new('foo' => 'a')
+      end
+
+      def self.last
+        @_last ||= new('foo' => 'a', 'bar' => 'b')
+      end
+
+      attr_accessor :attributes
+
+      def initialize(attrs = {})
+        @attributes = attrs
+      end
+    end
+  end
+
+  shared_context 'with defaults' do
+    before do
+      minimal_test_class.define_singleton_method(:defaults) do
+        { foo: 'a', bar: 'b', baz: 'c' }
+      end
+    end
+  end
+
+  describe '.current_without_cache' do
+    it 'defaults to last' do
+      expect(minimal_test_class.current_without_cache).to eq(minimal_test_class.last)
+    end
+
+    it 'can be overriden' do
+      minimal_test_class.define_singleton_method(:current_without_cache) do
+        first
+      end
+
+      expect(minimal_test_class.current_without_cache).to eq(minimal_test_class.first)
+    end
+  end
+
+  describe '.cache_key' do
+    it 'excludes cache attributes' do
+      expect(minimal_test_class.cache_key).to eq("TestClass:#{Gitlab::VERSION}:#{Gitlab.migrations_hash}:json")
+    end
+  end
+
+  describe '.defaults' do
+    it 'defaults to {}' do
+      expect(minimal_test_class.defaults).to eq({})
+    end
+
+    context 'with defaults defined' do
+      include_context 'with defaults'
+
+      it 'can be overriden' do
+        expect(minimal_test_class.defaults).to eq({ foo: 'a', bar: 'b', baz: 'c' })
+      end
+    end
+  end
+
+  describe '.build_from_defaults' do
+    include_context 'with defaults'
+
+    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
+      end
+    end
+
+    context 'without 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
+  end
+
+  describe '.current', :use_clean_rails_memory_store_caching do
+    context 'redis unavailable' do
+      it 'returns an uncached record' do
+        allow(minimal_test_class).to receive(:last).and_return(:last)
+        expect(Rails.cache).to receive(:read).and_raise(Redis::BaseError)
+
+        expect(minimal_test_class.current).to eq(:last)
+      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)
+
+        expect(minimal_test_class.current).to be_nil
+        expect(Rails.cache.exist?(minimal_test_class.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
+
+        expect(minimal_test_class.current).to eq(minimal_test_class.last)
+        expect(Rails.cache.exist?(minimal_test_class.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)
+      end
+    end
+  end
+
+  describe '.cached', :use_clean_rails_memory_store_caching do
+    context 'when cache is cold' do
+      it 'returns nil' do
+        expect(minimal_test_class.cached).to be_nil
+      end
+    end
+
+    context 'when cached settings do not include the latest defaults' 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
+      end
+
+      it 'includes attributes from defaults' do
+        expect(minimal_test_class.cached.attributes[:foo]).to eq(minimal_test_class.defaults[:foo])
+      end
+    end
+  end
+
+  describe '#cache!', :use_clean_rails_memory_store_caching do
+    let(:appearance_record) { create(:appearance) }
+
+    it 'caches the attributes' do
+      appearance_record.cache!
+
+      expect(Rails.cache.read(Appearance.cache_key)).to eq(appearance_record.attributes.to_json)
+    end
+  end
+end