Simplify Gitlab::CurrentSettings now that the logic is in CacheableAttributes

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

lib/gitlab/current_settings.rb

@@ -9,8 +9,8 @@ module Gitlab
         end
       end
 
-      def fake_application_settings(defaults = ::ApplicationSetting.defaults)
+      def fake_application_settings(attributes = {})
-        Gitlab::FakeApplicationSettings.new(defaults)
+        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
+          return fake_application_settings(current_settings&.attributes)
-          defaults.merge!(db_settings.attributes.symbolize_keys) if db_settings.present?
-          return fake_application_settings(defaults)
         end
 
-        return db_settings if db_settings.present?
+        return current_settings if current_settings.present?
 
-        ::ApplicationSetting.create_from_defaults || in_memory_application_settings
+        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
+        with_fallback_to_fake_application_settings do
-      rescue ActiveRecord::StatementInvalid, ActiveRecord::UnknownAttributeError
+          @in_memory_application_settings ||= ::ApplicationSetting.build_from_defaults # rubocop:disable Gitlab/ModuleWithInstanceVariables
-        # In case migrations the application_settings table is not created yet,
+        end
-        # we fallback to a simple OpenStruct
+      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

@@ -1,17 +1,15 @@
 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',
+        home_page_url: 'http://mydomain.com',
-                           signup_enabled: false)
+        signup_enabled: false)
 
       expect(described_class.home_page_url).to eq(db_settings.home_page_url)
       expect(described_class.signup_enabled?).to be_falsey
@@ -19,77 +17,16 @@ 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
+        stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'true')
-        # 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)
       end
 
-      it 'attempts to use cached values first' do
+      it 'returns an in-memory ApplicationSetting object' do
-        expect(ApplicationSetting).to receive(:cached)
+        expect(ApplicationSetting).not_to receive(:current)
 
         expect(described_class.current_application_settings).to be_a(ApplicationSetting)
-      end
+        expect(described_class.current_application_settings).not_to be_persisted
-
-      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)
-      end
-
-      it 'falls back to DB if Redis fails' do
-        db_settings = ApplicationSetting.create!(ApplicationSetting.defaults)
-
-        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 eq(db_settings)
-      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)
-
-        settings = described_class.current_application_settings
-
-        expect(settings).to be_a(ApplicationSetting)
-        expect(settings).to be_persisted
-        expect(settings).to have_attributes(ApplicationSetting.defaults)
-      end
-
-      context 'with migrations pending' do
-        before do
-          expect(ActiveRecord::Migrator).to receive(:needs_migration?).and_return(true)
-        end
-
-        it 'returns an in-memory ApplicationSetting object' do
-          settings = described_class.current_application_settings
-
-          expect(settings).to be_a(OpenStruct)
-          expect(settings.sign_in_enabled?).to eq(settings.sign_in_enabled)
-          expect(settings.sign_up_enabled?).to eq(settings.sign_up_enabled)
-        end
-
-        it 'uses the existing database settings and falls back to defaults' do
-          db_settings = create(:application_setting,
-                               home_page_url: 'http://mydomain.com',
-                               signup_enabled: false)
-          settings = described_class.current_application_settings
-          app_defaults = ApplicationSetting.last
-
-          expect(settings).to be_a(OpenStruct)
-          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
-
-          # Check that unspecified values use the defaults
-          settings.reject! { |key, _| [:home_page_url, :signup_enabled].include? key }
-          settings.each { |key, _| expect(settings[key]).to eq(app_defaults[key]) }
-        end
       end
     end
 
@@ -102,23 +39,87 @@ describe Gitlab::CurrentSettings do
 
       it 'returns an in-memory ApplicationSetting object' do
         expect(ApplicationSetting).not_to receive(:current)
-        expect(ApplicationSetting).not_to receive(:last)
 
-        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
+    context 'with DB available' do
-      before do
+      # This method returns the ::ApplicationSetting.defaults hash
-        stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'true')
+      # 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 'returns an in-memory ApplicationSetting object' do
+      before do
-        expect(ApplicationSetting).not_to receive(:current)
+        # For some reason, `allow(described_class).to receive(:connect_to_db?).and_return(true)` causes issues
-        expect(ApplicationSetting).not_to receive(:last)
+        # 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
 
-        expect(described_class.current_application_settings).to be_a(ApplicationSetting)
+      it 'creates default ApplicationSettings if none are present' do
-        expect(described_class.current_application_settings).not_to be_persisted
+        settings = described_class.current_application_settings
+
+        expect(settings).to be_a(ApplicationSetting)
+        expect(settings).to be_persisted
+        expect(settings).to have_attributes(settings_from_defaults)
+      end
+
+      context 'with migrations pending' do
+        before do
+          expect(ActiveRecord::Migrator).to receive(:needs_migration?).and_return(true)
+        end
+
+        it 'returns an in-memory ApplicationSetting object' do
+          settings = described_class.current_application_settings
+
+          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
+
+        it 'uses the existing database settings and falls back to defaults' do
+          db_settings = create(:application_setting,
+                               home_page_url: 'http://mydomain.com',
+                               signup_enabled: false)
+          settings = described_class.current_application_settings
+          app_defaults = ApplicationSetting.last
+
+          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
+
+          # Check that unspecified values use the defaults
+          settings.reject! { |key, _| [:home_page_url, :signup_enabled].include? key }
+          settings.each { |key, _| expect(settings[key]).to eq(app_defaults[key]) }
+        end
+      end
+
+      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).to receive(:create_from_defaults).and_raise(ActiveRecord::StatementInvalid)
+
+          expect(described_class.current_application_settings).to be_a(Gitlab::FakeApplicationSettings)
+        end
+      end
+
+      context 'when the application_settings table is not fully migrated' do
+        it 'returns an in-memory ApplicationSetting object' do
+          expect(ApplicationSetting).to receive(:create_from_defaults).and_raise(ActiveRecord::UnknownAttributeError)
+
+          expect(described_class.current_application_settings).to be_a(Gitlab::FakeApplicationSettings)
+        end
       end
     end
   end