summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorStan Hu <stanhu@gmail.com>2017-06-17 07:35:30 -0700
committerStan Hu <stanhu@gmail.com>2017-06-19 09:54:48 -0700
commit575dced5d777e2e0db58ba8dbec6438456c9ff93 (patch)
treebce5df62399a6be5501e23a8771a047d2f26fdb6
parent84cfb33fbb60898f9b56137832b2c5abb622465c (diff)
downloadgitlab-ce-575dced5d777e2e0db58ba8dbec6438456c9ff93.tar.gz
If migrations are pending, make CurrentSettings use existing values and populate missing columns with defaultssh-refactor-current-settings
master was failing because `ApplicationSetting.create_from_defaults` attempted to write to a column that did not exist in the database. This occurred in a `rake db:migrate` task, which was unable to perform the migration that would have added the missing column in the first place. In 9.3 RC2, we also had a bug where password sign-ins were disabled because there were many pending migrations. The problem occurred because `fake_application_settings` was being returned with an OpenStruct that did not include the predicate method `signup_enabled?`. As a result, the value would erroneously return `nil` instead of `true`. This commit uses the values of the defaults to mimic this behavior. This commit also refactors some of the logic to be clearer.
-rw-r--r--lib/gitlab/current_settings.rb49
-rw-r--r--lib/gitlab/fake_application_settings.rb27
-rw-r--r--spec/factories/application_settings.rb4
-rw-r--r--spec/lib/gitlab/current_settings_spec.rb31
-rw-r--r--spec/lib/gitlab/fake_application_settings_spec.rb32
5 files changed, 121 insertions, 22 deletions
diff --git a/lib/gitlab/current_settings.rb b/lib/gitlab/current_settings.rb
index 48735fd197d..284e6ad55a5 100644
--- a/lib/gitlab/current_settings.rb
+++ b/lib/gitlab/current_settings.rb
@@ -10,43 +10,49 @@ module Gitlab
delegate :sidekiq_throttling_enabled?, to: :current_application_settings
- def fake_application_settings
- OpenStruct.new(::ApplicationSetting.defaults)
+ def fake_application_settings(defaults = ApplicationSetting.defaults)
+ FakeApplicationSettings.new(defaults)
end
private
def ensure_application_settings!
- unless ENV['IN_MEMORY_APPLICATION_SETTINGS'] == 'true'
- settings = retrieve_settings_from_database?
- end
+ return in_memory_application_settings if ENV['IN_MEMORY_APPLICATION_SETTINGS'] == 'true'
- settings || in_memory_application_settings
+ cached_application_settings || uncached_application_settings
end
- def retrieve_settings_from_database?
- settings = retrieve_settings_from_database_cache?
- return settings if settings.present?
-
- return fake_application_settings unless connect_to_db?
-
+ def cached_application_settings
begin
- db_settings = ::ApplicationSetting.current
- # In case Redis isn't running or the Redis UNIX socket file is not available
+ ApplicationSetting.cached
rescue ::Redis::BaseError, ::Errno::ENOENT
- db_settings = ::ApplicationSetting.last
+ # In case Redis isn't running or the Redis UNIX socket file is not available
end
- db_settings || ::ApplicationSetting.create_from_defaults
end
- def retrieve_settings_from_database_cache?
+ def uncached_application_settings
+ return fake_application_settings unless connect_to_db?
+
+ # This loads from the database into the cache, so handle Redis errors
begin
- settings = ApplicationSetting.cached
+ db_settings = ApplicationSetting.current
rescue ::Redis::BaseError, ::Errno::ENOENT
# In case Redis isn't running or the Redis UNIX socket file is not available
- settings = nil
end
- settings
+
+ # If there are pending migrations, it's possible there are columns that
+ # need to be added to the application settings. To prevent Rake tasks
+ # and other callers from failing, use any loaded settings and return
+ # defaults for missing columns.
+ if ActiveRecord::Migrator.needs_migration?
+ defaults = ApplicationSetting.defaults
+ defaults.merge!(db_settings.attributes.symbolize_keys) if db_settings.present?
+ return fake_application_settings(defaults)
+ end
+
+ return db_settings if db_settings.present?
+
+ ApplicationSetting.create_from_defaults || in_memory_application_settings
end
def in_memory_application_settings
@@ -62,8 +68,7 @@ module Gitlab
active_db_connection = ActiveRecord::Base.connection.active? rescue false
active_db_connection &&
- ActiveRecord::Base.connection.table_exists?('application_settings') &&
- !ActiveRecord::Migrator.needs_migration?
+ ActiveRecord::Base.connection.table_exists?('application_settings')
rescue ActiveRecord::NoDatabaseError
false
end
diff --git a/lib/gitlab/fake_application_settings.rb b/lib/gitlab/fake_application_settings.rb
new file mode 100644
index 00000000000..bb14a8cd9e7
--- /dev/null
+++ b/lib/gitlab/fake_application_settings.rb
@@ -0,0 +1,27 @@
+# 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/factories/application_settings.rb b/spec/factories/application_settings.rb
new file mode 100644
index 00000000000..aef65e724c2
--- /dev/null
+++ b/spec/factories/application_settings.rb
@@ -0,0 +1,4 @@
+FactoryGirl.define do
+ factory :application_setting do
+ end
+end
diff --git a/spec/lib/gitlab/current_settings_spec.rb b/spec/lib/gitlab/current_settings_spec.rb
index fda39d78610..a566f24f6a6 100644
--- a/spec/lib/gitlab/current_settings_spec.rb
+++ b/spec/lib/gitlab/current_settings_spec.rb
@@ -32,6 +32,37 @@ describe Gitlab::CurrentSettings do
expect(current_application_settings).to be_a(ApplicationSetting)
end
+
+ context 'with migrations pending' do
+ before do
+ expect(ActiveRecord::Migrator).to receive(:needs_migration?).and_return(true)
+ end
+
+ it 'returns an in-memory ApplicationSetting object' do
+ settings = current_application_settings
+
+ expect(settings).to be_a(OpenStruct)
+ expect(settings.sign_in_enabled?).to eq(settings.sign_in_enabled)
+ expect(settings.sign_up_enabled?).to eq(settings.sign_up_enabled)
+ 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 = current_application_settings
+ app_defaults = ApplicationSetting.last
+
+ expect(settings).to be_a(OpenStruct)
+ 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]) }
+ end
+ end
end
context 'with DB unavailable' do
diff --git a/spec/lib/gitlab/fake_application_settings_spec.rb b/spec/lib/gitlab/fake_application_settings_spec.rb
new file mode 100644
index 00000000000..b793176d84a
--- /dev/null
+++ b/spec/lib/gitlab/fake_application_settings_spec.rb
@@ -0,0 +1,32 @@
+require 'spec_helper'
+
+describe Gitlab::FakeApplicationSettings do
+ let(:defaults) { { signin_enabled: false, foobar: 'asdf', signup_enabled: true, 'test?' => 123 } }
+
+ subject { described_class.new(defaults) }
+
+ it 'wraps OpenStruct variables properly' do
+ expect(subject.signin_enabled).to be_falsey
+ expect(subject.signup_enabled).to be_truthy
+ expect(subject.foobar).to eq('asdf')
+ end
+
+ it 'defines predicate methods' do
+ expect(subject.signin_enabled?).to be_falsey
+ expect(subject.signup_enabled?).to be_truthy
+ end
+
+ it 'predicate method changes when value is updated' do
+ subject.signin_enabled = true
+
+ expect(subject.signin_enabled?).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