From 71672dfa6afbb374b24ae5457a58204708d948ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Wed, 12 Dec 2018 12:59:06 +0100 Subject: Return an ApplicationSetting in CurrentSettings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This replaces the use of fake_application_settings with `::ApplicationSetting.build`_from_defaults. The reason is that `fake_application_settings` doesn't have the custom accessors that `ApplicationSetting` has, e.g. `#commit_email_hostname`, thus this can lead to unexpected `nil` values which comes from the database column instead of `.default_commit_email_hostname` returned by `ApplicationSetting#commit_email_hostname`. Using `::ApplicationSetting.build_from_defaults` should be safe as it doesn't try to `INSERT` a DB record, in contrary to `::ApplicationSetting.create_from_defaults` which we used to use, and which created issues that the introduction of `fake_application_settings` tried to resolve (575dced5). Signed-off-by: Rémy Coutable --- app/models/application_setting.rb | 2 +- ...ge-request-due-to-nil-commit_email_hostname.yml | 5 ++ lib/gitlab/current_settings.rb | 10 +++- spec/lib/gitlab/current_settings_spec.rb | 61 ++++++++++++++-------- 4 files changed, 55 insertions(+), 23 deletions(-) create mode 100644 changelogs/unreleased/54953-error-500-viewing-merge-request-due-to-nil-commit_email_hostname.yml diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 4319db42019..54139f61a16 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -382,7 +382,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/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..e6c80bed5a5 100644 --- a/lib/gitlab/current_settings.rb +++ b/lib/gitlab/current_settings.rb @@ -50,7 +50,15 @@ 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) + db_attributes = current_settings&.attributes || {} + column_names = ::ApplicationSetting.column_names + final_attributes = ::ApplicationSetting + .defaults + .merge(db_attributes) + .stringify_keys + .slice(*column_names) + + return ::ApplicationSetting.new(final_attributes) end return current_settings if current_settings.present? diff --git a/spec/lib/gitlab/current_settings_spec.rb b/spec/lib/gitlab/current_settings_spec.rb index 55490f37ac7..06772fffce7 100644 --- a/spec/lib/gitlab/current_settings_spec.rb +++ b/spec/lib/gitlab/current_settings_spec.rb @@ -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 + 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 - 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) + 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 + + context 'with no ApplicationSetting DB record' do + it_behaves_like 'a non-persisted ApplicationSetting object' do + let(:current_settings) { described_class.current_application_settings } + end 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 -- cgit v1.2.1 From 1ae28e0e7b5996536e4c8005c362d3f82088c77c Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 14 Dec 2018 17:14:21 +0800 Subject: Return fake if and only if there's no database Because `connect_to_db?` already detects if the table exists or not --- lib/gitlab/current_settings.rb | 31 ++++++++++--------------------- 1 file changed, 10 insertions(+), 21 deletions(-) diff --git a/lib/gitlab/current_settings.rb b/lib/gitlab/current_settings.rb index e6c80bed5a5..168542f77e6 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 @@ -58,28 +54,21 @@ module Gitlab .stringify_keys .slice(*column_names) - return ::ApplicationSetting.new(final_attributes) - end - - return current_settings if current_settings.present? - - with_fallback_to_fake_application_settings do - ::ApplicationSetting.create_from_defaults || in_memory_application_settings + ::ApplicationSetting.new(final_attributes) + elsif current_settings.present? + current_settings + else + ::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 - 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? -- cgit v1.2.1 From cc5099c5ce79a7c062e2197b47f5f8a81bb48292 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 14 Dec 2018 22:42:07 +0800 Subject: Move schema aware defaults to build_from_defaults This way we can reuse the safe setting --- app/models/concerns/cacheable_attributes.rb | 7 ++++++- lib/gitlab/current_settings.rb | 9 +-------- 2 files changed, 7 insertions(+), 9 deletions(-) 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/lib/gitlab/current_settings.rb b/lib/gitlab/current_settings.rb index 168542f77e6..570bfb80fba 100644 --- a/lib/gitlab/current_settings.rb +++ b/lib/gitlab/current_settings.rb @@ -47,14 +47,7 @@ module Gitlab # defaults for missing columns. if ActiveRecord::Migrator.needs_migration? db_attributes = current_settings&.attributes || {} - column_names = ::ApplicationSetting.column_names - final_attributes = ::ApplicationSetting - .defaults - .merge(db_attributes) - .stringify_keys - .slice(*column_names) - - ::ApplicationSetting.new(final_attributes) + ::ApplicationSetting.build_from_defaults(db_attributes) elsif current_settings.present? current_settings else -- cgit v1.2.1 From 0ff27ce05960598e5a1c0a1115180db0feeb689a Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 14 Dec 2018 22:45:14 +0800 Subject: Method `create_from_defaults` will never give nil --- app/models/application_setting.rb | 2 +- lib/gitlab/current_settings.rb | 3 +-- spec/lib/gitlab/current_settings_spec.rb | 19 +++++++------------ 3 files changed, 9 insertions(+), 15 deletions(-) diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 54139f61a16..86ffd198ab1 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -311,7 +311,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 = {}) diff --git a/lib/gitlab/current_settings.rb b/lib/gitlab/current_settings.rb index 570bfb80fba..552aad83dd4 100644 --- a/lib/gitlab/current_settings.rb +++ b/lib/gitlab/current_settings.rb @@ -51,8 +51,7 @@ module Gitlab elsif current_settings.present? current_settings else - ::ApplicationSetting.create_from_defaults || - in_memory_application_settings + ::ApplicationSetting.create_from_defaults end end diff --git a/spec/lib/gitlab/current_settings_spec.rb b/spec/lib/gitlab/current_settings_spec.rb index 06772fffce7..add8867d243 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 @@ -157,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 -- cgit v1.2.1 From 25ef6f51c9899cc26661a2daac7ca0d47b95d443 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Sat, 15 Dec 2018 01:01:04 +0800 Subject: Use fake application settings for migration tests --- spec/spec_helper.rb | 2 ++ spec/support/helpers/migrations_helpers.rb | 16 ++++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index fb3421b61d3..ae5284c071b 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -216,6 +216,8 @@ 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 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 -- cgit v1.2.1 From c3641185f47eb7c775906a31f099460b34053aea Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Sat, 15 Dec 2018 02:01:24 +0800 Subject: Fix cacheable_attributes_spec.rb by adding column_names --- spec/models/concerns/cacheable_attributes_spec.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) 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 -- cgit v1.2.1 From 32101a608325617fc5afa896cd7388b643d1d512 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Sat, 15 Dec 2018 04:17:48 +0800 Subject: Make sure we clear the application settings after migration tests. --- spec/spec_helper.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index ae5284c071b..e8554377d77 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -223,6 +223,8 @@ RSpec.configure do |config| config.after(:context, :migration) do schema_migrate_up! + + Gitlab::CurrentSettings.clear_in_memory_application_settings! end config.around(:each, :nested_groups) do |example| -- cgit v1.2.1 From cc06bb2c6ec1facf2b1eb50803dbedc076abe017 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Mon, 17 Dec 2018 15:10:45 +0100 Subject: Simplify spec/lib/gitlab/current_settings_spec.rb a bit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- spec/lib/gitlab/current_settings_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/lib/gitlab/current_settings_spec.rb b/spec/lib/gitlab/current_settings_spec.rb index add8867d243..caf9fc5442c 100644 --- a/spec/lib/gitlab/current_settings_spec.rb +++ b/spec/lib/gitlab/current_settings_spec.rb @@ -113,6 +113,8 @@ describe Gitlab::CurrentSettings do end 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 @@ -132,9 +134,7 @@ describe Gitlab::CurrentSettings do end context 'with no ApplicationSetting DB record' do - it_behaves_like 'a non-persisted ApplicationSetting object' do - let(:current_settings) { described_class.current_application_settings } - end + it_behaves_like 'a non-persisted ApplicationSetting object' end context 'with an existing ApplicationSetting DB record' do -- cgit v1.2.1