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.

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