Merge branch '54953-error-500-viewing-merge-request-due-to-nil-commit_email_hostname' into 'master'

Return an ApplicationSetting in CurrentSettings

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

8 files changed, 96 insertions, 59 deletions

diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index 73be94eade6..88746375c67 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -312,7 +312,7 @@ class ApplicationSetting < ActiveRecord::Base
   end
 
   def self.create_from_defaults
-    create(defaults)
+    build_from_defaults.tap(&:save)
   end
 
   def self.human_attribute_name(attr, _options = {})
@@ -383,7 +383,7 @@ class ApplicationSetting < ActiveRecord::Base
   end
 
   def restricted_visibility_levels=(levels)
-    super(levels.map { |level| Gitlab::VisibilityLevel.level_value(level) })
+    super(levels&.map { |level| Gitlab::VisibilityLevel.level_value(level) })
   end
 
   def strip_sentry_values
diff --git a/app/models/concerns/cacheable_attributes.rb b/app/models/concerns/cacheable_attributes.rb
index 75592bb63e2..3d60f6924c1 100644
--- a/app/models/concerns/cacheable_attributes.rb
+++ b/app/models/concerns/cacheable_attributes.rb
@@ -23,7 +23,12 @@ module CacheableAttributes
     end
 
     def build_from_defaults(attributes = {})
-      new(defaults.merge(attributes))
+      final_attributes = defaults
+        .merge(attributes)
+        .stringify_keys
+        .slice(*column_names)
+
+      new(final_attributes)
     end
 
     def cached
diff --git a/changelogs/unreleased/54953-error-500-viewing-merge-request-due-to-nil-commit_email_hostname.yml b/changelogs/unreleased/54953-error-500-viewing-merge-request-due-to-nil-commit_email_hostname.yml
new file mode 100644
index 00000000000..8fd127acf2b
--- /dev/null
+++ b/changelogs/unreleased/54953-error-500-viewing-merge-request-due-to-nil-commit_email_hostname.yml
@@ -0,0 +1,5 @@
+---
+title: Return an ApplicationSetting in CurrentSettings
+merge_request: 23766
+author:
+type: fixed
diff --git a/lib/gitlab/current_settings.rb b/lib/gitlab/current_settings.rb
index 477f9101e98..552aad83dd4 100644
--- a/lib/gitlab/current_settings.rb
+++ b/lib/gitlab/current_settings.rb
@@ -7,10 +7,6 @@ module Gitlab
         Gitlab::SafeRequestStore.fetch(:current_application_settings) { ensure_application_settings! }
       end
 
-      def fake_application_settings(attributes = {})
-        Gitlab::FakeApplicationSettings.new(::ApplicationSetting.defaults.merge(attributes || {}))
-      end
-
       def clear_in_memory_application_settings!
         @in_memory_application_settings = nil
       end
@@ -50,28 +46,21 @@ 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)
-        end
-
-        return current_settings if current_settings.present?
-
-        with_fallback_to_fake_application_settings do
-          ::ApplicationSetting.create_from_defaults || in_memory_application_settings
+          db_attributes = current_settings&.attributes || {}
+          ::ApplicationSetting.build_from_defaults(db_attributes)
+        elsif current_settings.present?
+          current_settings
+        else
+          ::ApplicationSetting.create_from_defaults
         end
       end
 
-      def in_memory_application_settings
-        with_fallback_to_fake_application_settings do
-          @in_memory_application_settings ||= ::ApplicationSetting.build_from_defaults
-        end
+      def fake_application_settings(attributes = {})
+        Gitlab::FakeApplicationSettings.new(::ApplicationSetting.defaults.merge(attributes || {}))
       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
+      def in_memory_application_settings
+        @in_memory_application_settings ||= ::ApplicationSetting.build_from_defaults
       end
 
       def connect_to_db?
diff --git a/spec/lib/gitlab/current_settings_spec.rb b/spec/lib/gitlab/current_settings_spec.rb
index 55490f37ac7..caf9fc5442c 100644
--- a/spec/lib/gitlab/current_settings_spec.rb
+++ b/spec/lib/gitlab/current_settings_spec.rb
@@ -54,7 +54,7 @@ describe Gitlab::CurrentSettings do
           expect(ApplicationSetting).not_to receive(:current)
         end
 
-        it 'returns an in-memory ApplicationSetting object' do
+        it 'returns a FakeApplicationSettings object' do
           expect(described_class.current_application_settings).to be_a(Gitlab::FakeApplicationSettings)
         end
 
@@ -91,6 +91,14 @@ describe Gitlab::CurrentSettings do
           allow(ActiveRecord::Base.connection).to receive(:cached_table_exists?).with('application_settings').and_return(true)
         end
 
+        context 'with RequestStore enabled', :request_store do
+          it 'fetches the settings from DB only once' do
+            described_class.current_application_settings # warm the cache
+
+            expect(ActiveRecord::QueryRecorder.new { described_class.current_application_settings }.count).to eq(0)
+          end
+        end
+
         it 'creates default ApplicationSettings if none are present' do
           settings = described_class.current_application_settings
 
@@ -99,34 +107,45 @@ describe Gitlab::CurrentSettings do
           expect(settings).to have_attributes(settings_from_defaults)
         end
 
-        context 'with migrations pending' do
+        context 'with pending migrations' 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
+          shared_examples 'a non-persisted ApplicationSetting object' do
+            let(:current_settings) { described_class.current_application_settings }
+
+            it 'returns a non-persisted ApplicationSetting object' do
+              expect(current_settings).to be_a(ApplicationSetting)
+              expect(current_settings).not_to be_persisted
+            end
+
+            it 'uses the default value from ApplicationSetting.defaults' do
+              expect(current_settings.signup_enabled).to eq(ApplicationSetting.defaults[:signup_enabled])
+            end
+
+            it 'uses the default value from custom ApplicationSetting accessors' do
+              expect(current_settings.commit_email_hostname).to eq(ApplicationSetting.default_commit_email_hostname)
+            end
+
+            it 'responds to predicate methods' do
+              expect(current_settings.signup_enabled?).to eq(current_settings.signup_enabled)
+            end
+          end
 
-            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)
+          context 'with no ApplicationSetting DB record' do
+            it_behaves_like 'a non-persisted ApplicationSetting object'
           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]) }
+          context 'with an existing ApplicationSetting DB record' do
+            let!(:db_settings) { ApplicationSetting.build_from_defaults(home_page_url: 'http://mydomain.com').save! && ApplicationSetting.last }
+            let(:current_settings) { described_class.current_application_settings }
+
+            it_behaves_like 'a non-persisted ApplicationSetting object'
+
+            it 'uses the value from the DB attribute if present and not overriden by an accessor' do
+              expect(current_settings.home_page_url).to eq(db_settings.home_page_url)
+            end
           end
         end
 
@@ -138,17 +157,12 @@ describe Gitlab::CurrentSettings do
           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)
+        context 'when the application_settings table does not exist' do
+          it 'returns a FakeApplicationSettings object' do
+            expect(Gitlab::Database)
+              .to receive(:cached_table_exists?)
+              .with('application_settings')
+              .and_return(false)
 
             expect(described_class.current_application_settings).to be_a(Gitlab::FakeApplicationSettings)
           end
diff --git a/spec/models/concerns/cacheable_attributes_spec.rb b/spec/models/concerns/cacheable_attributes_spec.rb
index 827fbc9d7d5..689e7d3058f 100644
--- a/spec/models/concerns/cacheable_attributes_spec.rb
+++ b/spec/models/concerns/cacheable_attributes_spec.rb
@@ -20,6 +20,10 @@ describe CacheableAttributes do
         @_last ||= new('foo' => 'a', 'bar' => 'b')
       end
 
+      def self.column_names
+        %w[foo bar baz]
+      end
+
       attr_accessor :attributes
 
       def initialize(attrs = {}, *)
@@ -75,13 +79,13 @@ describe CacheableAttributes do
 
     context 'without any attributes given' do
       it 'intializes a new object with the defaults' do
-        expect(minimal_test_class.build_from_defaults.attributes).to eq(minimal_test_class.defaults)
+        expect(minimal_test_class.build_from_defaults.attributes).to eq(minimal_test_class.defaults.stringify_keys)
       end
     end
 
     context 'with 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')
+        expect(minimal_test_class.build_from_defaults(foo: 'd').attributes['foo']).to eq('d')
       end
     end
 
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 4042120e2c2..89357056c93 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -216,11 +216,15 @@ RSpec.configure do |config|
 
   # Each example may call `migrate!`, so we must ensure we are migrated down every time
   config.before(:each, :migration) do
+    use_fake_application_settings
+
     schema_migrate_down!
   end
 
   config.after(:context, :migration) do
     schema_migrate_up!
+
+    Gitlab::CurrentSettings.clear_in_memory_application_settings!
   end
 
   config.around(:each, :nested_groups) do |example|
diff --git a/spec/support/helpers/migrations_helpers.rb b/spec/support/helpers/migrations_helpers.rb
index 5887c3eab74..cc1a28cb264 100644
--- a/spec/support/helpers/migrations_helpers.rb
+++ b/spec/support/helpers/migrations_helpers.rb
@@ -62,6 +62,22 @@ module MigrationsHelpers
     klass.reset_column_information
   end
 
+  # In some migration tests, we're using factories to create records,
+  # however those models might be depending on a schema version which
+  # doesn't have the columns we want in application_settings.
+  # In these cases, we'll need to use the fake application settings
+  # as if we have migrations pending
+  def use_fake_application_settings
+    # We stub this way because we can't stub on
+    # `current_application_settings` due to `method_missing` is
+    # depending on current_application_settings...
+    allow(ActiveRecord::Base.connection)
+      .to receive(:active?)
+      .and_return(false)
+
+    stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false')
+  end
+
   def previous_migration
     migrations.each_cons(2) do |previous, migration|
       break previous if migration.name == described_class.name