Ensure we return an actual ApplicationSetting object when fallbacking in Gitlab::CurrentSettings.current_application_settings47491-improve-current-fakeapplicationsettings-doesn-t-respond-to-all-the-methods-applicationsetting-responds-to-itself

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

5 files changed, 34 insertions, 80 deletions

diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index 3d58a14882f..7baa0a142eb 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -373,7 +373,7 @@ class ApplicationSetting < ActiveRecord::Base
   end
 
   def restricted_visibility_levels=(levels)
-    super(levels.map { |level| Gitlab::VisibilityLevel.level_value(level) })
+    super(Array(levels).compact.map { |level| Gitlab::VisibilityLevel.level_value(level) })
   end
 
   def performance_bar_allowed_group
diff --git a/lib/gitlab/current_settings.rb b/lib/gitlab/current_settings.rb
index 3cf35f499cd..86536d59fd5 100644
--- a/lib/gitlab/current_settings.rb
+++ b/lib/gitlab/current_settings.rb
@@ -9,8 +9,8 @@ module Gitlab
         end
       end
 
-      def fake_application_settings(attributes = {})
-        Gitlab::FakeApplicationSettings.new(::ApplicationSetting.defaults.merge(attributes || {}))
+      def transient_application_settings(attributes = {})
+        ::ApplicationSetting.build_from_defaults(attributes || {})
       end
 
       def method_missing(name, *args, &block)
@@ -40,7 +40,7 @@ module Gitlab
       end
 
       def uncached_application_settings
-        return fake_application_settings unless connect_to_db?
+        return transient_application_settings unless connect_to_db?
 
         current_settings = ::ApplicationSetting.current
         # If there are pending migrations, it's possible there are columns that
@@ -48,28 +48,28 @@ module Gitlab
         # and other callers from failing, use any loaded settings and return
         # defaults for missing columns.
         if ActiveRecord::Migrator.needs_migration?
-          return fake_application_settings(current_settings&.attributes)
+          return transient_application_settings(current_settings&.attributes)
         end
 
         return current_settings if current_settings.present?
 
-        with_fallback_to_fake_application_settings do
+        with_fallback_to_transient_application_settings do
           ::ApplicationSetting.create_from_defaults || in_memory_application_settings
         end
       end
 
       def in_memory_application_settings
-        with_fallback_to_fake_application_settings do
-          @in_memory_application_settings ||= ::ApplicationSetting.build_from_defaults # rubocop:disable Gitlab/ModuleWithInstanceVariables
+        with_fallback_to_transient_application_settings do
+          @in_memory_application_settings ||= transient_application_settings # rubocop:disable Gitlab/ModuleWithInstanceVariables
         end
       end
 
-      def with_fallback_to_fake_application_settings(&block)
+      def with_fallback_to_transient_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
+        transient_application_settings
       end
 
       def connect_to_db?
diff --git a/lib/gitlab/fake_application_settings.rb b/lib/gitlab/fake_application_settings.rb
deleted file mode 100644
index bb14a8cd9e7..00000000000
--- a/lib/gitlab/fake_application_settings.rb
+++ /dev/null
@@ -1,27 +0,0 @@
-# This class extends an OpenStruct object by adding predicate methods to mimic
-# ActiveRecord access. We rely on the initial values being true or false to
-# determine whether to define a predicate method because for a newly-added
-# column that has not been migrated yet, there is no way to determine the
-# column type without parsing db/schema.rb.
-module Gitlab
-  class FakeApplicationSettings < OpenStruct
-    def initialize(options = {})
-      super
-
-      FakeApplicationSettings.define_predicate_methods(options)
-    end
-
-    # Mimic ActiveRecord predicate methods for boolean values
-    def self.define_predicate_methods(options)
-      options.each do |key, value|
-        next if key.to_s.end_with?('?')
-        next unless [true, false].include?(value)
-
-        define_method "#{key}?" do
-          actual_key = key.to_s.chomp('?')
-          self[actual_key]
-        end
-      end
-    end
-  end
-end
diff --git a/spec/lib/gitlab/current_settings_spec.rb b/spec/lib/gitlab/current_settings_spec.rb
index 55490f37ac7..3571ead3843 100644
--- a/spec/lib/gitlab/current_settings_spec.rb
+++ b/spec/lib/gitlab/current_settings_spec.rb
@@ -55,7 +55,10 @@ describe Gitlab::CurrentSettings do
         end
 
         it 'returns an in-memory ApplicationSetting object' do
-          expect(described_class.current_application_settings).to be_a(Gitlab::FakeApplicationSettings)
+          settings = described_class.current_application_settings
+
+          expect(settings).to be_a(ApplicationSetting)
+          expect(settings).not_to be_persisted
         end
 
         it 'does not issue any query' do
@@ -107,9 +110,15 @@ describe Gitlab::CurrentSettings do
           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)
+            expect(settings).to be_a(ApplicationSetting)
+            expect(settings).not_to be_persisted
+            expect(settings.signup_enabled?).to eq(settings.signup_enabled)
+          end
+
+          it 'returns an ApplicationSetting object that responds to ApplicationSetting methods' do
+            settings = described_class.current_application_settings
+
+            expect(settings.key_restriction_for('dsa')).to eq(0)
           end
 
           it 'uses the existing database settings and falls back to defaults' do
@@ -119,14 +128,12 @@ describe Gitlab::CurrentSettings do
             settings = described_class.current_application_settings
             app_defaults = ApplicationSetting.last
 
-            expect(settings).to be_a(Gitlab::FakeApplicationSettings)
+            expect(settings).to be_a(ApplicationSetting)
+            expect(settings).not_to be_persisted
             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]) }
+            expect(settings.two_factor_grace_period).to eq(48)
           end
         end
 
@@ -142,7 +149,10 @@ describe Gitlab::CurrentSettings 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)
+            settings = described_class.current_application_settings
+
+            expect(settings).to be_a(ApplicationSetting)
+            expect(settings).not_to be_persisted
           end
         end
 
@@ -150,7 +160,10 @@ describe Gitlab::CurrentSettings 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)
+            settings = described_class.current_application_settings
+
+            expect(settings).to be_a(ApplicationSetting)
+            expect(settings).not_to be_persisted
           end
         end
       end
diff --git a/spec/lib/gitlab/fake_application_settings_spec.rb b/spec/lib/gitlab/fake_application_settings_spec.rb
deleted file mode 100644
index af12e13d36d..00000000000
--- a/spec/lib/gitlab/fake_application_settings_spec.rb
+++ /dev/null
@@ -1,32 +0,0 @@
-require 'spec_helper'
-
-describe Gitlab::FakeApplicationSettings do
-  let(:defaults) { { password_authentication_enabled_for_web: false, foobar: 'asdf', signup_enabled: true, 'test?' => 123 } }
-
-  subject { described_class.new(defaults) }
-
-  it 'wraps OpenStruct variables properly' do
-    expect(subject.password_authentication_enabled_for_web).to be_falsey
-    expect(subject.signup_enabled).to be_truthy
-    expect(subject.foobar).to eq('asdf')
-  end
-
-  it 'defines predicate methods' do
-    expect(subject.password_authentication_enabled_for_web?).to be_falsey
-    expect(subject.signup_enabled?).to be_truthy
-  end
-
-  it 'predicate method changes when value is updated' do
-    subject.password_authentication_enabled_for_web = true
-
-    expect(subject.password_authentication_enabled_for_web?).to be_truthy
-  end
-
-  it 'does not define a predicate method' do
-    expect(subject.foobar?).to be_nil
-  end
-
-  it 'does not override an existing predicate method' do
-    expect(subject.test?).to eq(123)
-  end
-end