From 84cfb33fbb60898f9b56137832b2c5abb622465c Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sat, 17 Jun 2017 07:30:15 -0700 Subject: Revert "Merge branch 'rs-revert-11842' into 'master'" This reverts commit ad521bde1bb556709edd39d8a9aa67ee47605b91, reversing changes made to 3a38e5f1ab914bc4eaeecda6e18caaa7ca9ea5a7. --- lib/gitlab/current_settings.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'lib') diff --git a/lib/gitlab/current_settings.rb b/lib/gitlab/current_settings.rb index 2fd5452dd78..48735fd197d 100644 --- a/lib/gitlab/current_settings.rb +++ b/lib/gitlab/current_settings.rb @@ -62,7 +62,8 @@ module Gitlab active_db_connection = ActiveRecord::Base.connection.active? rescue false active_db_connection && - ActiveRecord::Base.connection.table_exists?('application_settings') + ActiveRecord::Base.connection.table_exists?('application_settings') && + !ActiveRecord::Migrator.needs_migration? rescue ActiveRecord::NoDatabaseError false end -- cgit v1.2.1 From 575dced5d777e2e0db58ba8dbec6438456c9ff93 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sat, 17 Jun 2017 07:35:30 -0700 Subject: If migrations are pending, make CurrentSettings use existing values and populate missing columns with defaults 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. --- lib/gitlab/current_settings.rb | 49 ++++++++++++++++++--------------- lib/gitlab/fake_application_settings.rb | 27 ++++++++++++++++++ 2 files changed, 54 insertions(+), 22 deletions(-) create mode 100644 lib/gitlab/fake_application_settings.rb (limited to 'lib') 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 -- cgit v1.2.1