diff options
author | Rémy Coutable <remy@rymai.me> | 2018-06-07 16:54:30 +0300 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2018-06-07 18:57:27 +0300 |
commit | f0e110493fbcc6eb496ac7c16e7ee5b0c88a956e (patch) | |
tree | cec77241d6a18bfc95756bf12012b73af58cfc80 | |
parent | 6afe6fa6bcb3bdb09bd49ba638a37af2a8c7a6ec (diff) |
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>
-rw-r--r-- | app/models/application_setting.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/current_settings.rb | 18 | ||||
-rw-r--r-- | lib/gitlab/fake_application_settings.rb | 27 | ||||
-rw-r--r-- | spec/lib/gitlab/current_settings_spec.rb | 35 | ||||
-rw-r--r-- | spec/lib/gitlab/fake_application_settings_spec.rb | 32 |
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 |