Merge branch 'fix-application-setting-nil-cache' into 'master'

Prevent ApplicationSetting to cache nil value

Closes #39275

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

4 files changed, 25 insertions, 2 deletions

diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index 3568e72e463..2216ac0402d 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -194,7 +194,10 @@ class ApplicationSetting < ActiveRecord::Base
     ensure_cache_setup
 
     Rails.cache.fetch(CACHE_KEY) do
-      ApplicationSetting.last
+      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)
diff --git a/changelogs/unreleased/fix-application-setting-nil-cache.yml b/changelogs/unreleased/fix-application-setting-nil-cache.yml
new file mode 100644
index 00000000000..a5f028e3d69
--- /dev/null
+++ b/changelogs/unreleased/fix-application-setting-nil-cache.yml
@@ -0,0 +1,5 @@
+---
+title: Fix application setting to cache nil object
+merge_request:
+author:
+type: fixed
diff --git a/spec/lib/gitlab/current_settings_spec.rb b/spec/lib/gitlab/current_settings_spec.rb
index d57ffcae8e1..492659a82b0 100644
--- a/spec/lib/gitlab/current_settings_spec.rb
+++ b/spec/lib/gitlab/current_settings_spec.rb
@@ -21,7 +21,7 @@ describe Gitlab::CurrentSettings do
 
       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
+        expect(ApplicationSetting).to receive(:last).and_call_original.twice
 
         expect(current_application_settings).to be_a(ApplicationSetting)
       end
diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb
index c7a9eabdf06..ee51f2bf13b 100644
--- a/spec/models/application_setting_spec.rb
+++ b/spec/models/application_setting_spec.rb
@@ -193,6 +193,21 @@ describe ApplicationSetting do
         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 'restricted signup domains' do