Commit 9c723bff authored by Stan Hu's avatar Stan Hu

Merge branch '54953-error-500-viewing-merge-request-due-to-nil-commit_email_hostname' into 'master'

Return an ApplicationSetting in CurrentSettings

See merge request gitlab-org/gitlab-ce!23766
parents 68e312b2 82bf55c8
...@@ -312,7 +312,7 @@ class ApplicationSetting < ActiveRecord::Base ...@@ -312,7 +312,7 @@ class ApplicationSetting < ActiveRecord::Base
end end
def self.create_from_defaults def self.create_from_defaults
create(defaults) build_from_defaults.tap(&:save)
end end
def self.human_attribute_name(attr, _options = {}) def self.human_attribute_name(attr, _options = {})
...@@ -383,7 +383,7 @@ class ApplicationSetting < ActiveRecord::Base ...@@ -383,7 +383,7 @@ class ApplicationSetting < ActiveRecord::Base
end end
def restricted_visibility_levels=(levels) def restricted_visibility_levels=(levels)
super(levels.map { |level| Gitlab::VisibilityLevel.level_value(level) }) super(levels&.map { |level| Gitlab::VisibilityLevel.level_value(level) })
end end
def strip_sentry_values def strip_sentry_values
......
...@@ -23,7 +23,12 @@ module CacheableAttributes ...@@ -23,7 +23,12 @@ module CacheableAttributes
end end
def build_from_defaults(attributes = {}) def build_from_defaults(attributes = {})
new(defaults.merge(attributes)) final_attributes = defaults
.merge(attributes)
.stringify_keys
.slice(*column_names)
new(final_attributes)
end end
def cached def cached
......
---
title: Return an ApplicationSetting in CurrentSettings
merge_request: 23766
author:
type: fixed
...@@ -7,10 +7,6 @@ module Gitlab ...@@ -7,10 +7,6 @@ module Gitlab
Gitlab::SafeRequestStore.fetch(:current_application_settings) { ensure_application_settings! } Gitlab::SafeRequestStore.fetch(:current_application_settings) { ensure_application_settings! }
end end
def fake_application_settings(attributes = {})
Gitlab::FakeApplicationSettings.new(::ApplicationSetting.defaults.merge(attributes || {}))
end
def clear_in_memory_application_settings! def clear_in_memory_application_settings!
@in_memory_application_settings = nil @in_memory_application_settings = nil
end end
...@@ -50,29 +46,22 @@ module Gitlab ...@@ -50,29 +46,22 @@ module Gitlab
# and other callers from failing, use any loaded settings and return # and other callers from failing, use any loaded settings and return
# defaults for missing columns. # defaults for missing columns.
if ActiveRecord::Migrator.needs_migration? if ActiveRecord::Migrator.needs_migration?
return fake_application_settings(current_settings&.attributes) db_attributes = current_settings&.attributes || {}
::ApplicationSetting.build_from_defaults(db_attributes)
elsif current_settings.present?
current_settings
else
::ApplicationSetting.create_from_defaults
end end
return current_settings if current_settings.present?
with_fallback_to_fake_application_settings do
::ApplicationSetting.create_from_defaults || in_memory_application_settings
end end
def fake_application_settings(attributes = {})
Gitlab::FakeApplicationSettings.new(::ApplicationSetting.defaults.merge(attributes || {}))
end end
def in_memory_application_settings def in_memory_application_settings
with_fallback_to_fake_application_settings do
@in_memory_application_settings ||= ::ApplicationSetting.build_from_defaults @in_memory_application_settings ||= ::ApplicationSetting.build_from_defaults
end end
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
end
def connect_to_db? def connect_to_db?
# When the DBMS is not available, an exception (e.g. PG::ConnectionBad) is raised # When the DBMS is not available, an exception (e.g. PG::ConnectionBad) is raised
......
...@@ -54,7 +54,7 @@ describe Gitlab::CurrentSettings do ...@@ -54,7 +54,7 @@ describe Gitlab::CurrentSettings do
expect(ApplicationSetting).not_to receive(:current) expect(ApplicationSetting).not_to receive(:current)
end 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) expect(described_class.current_application_settings).to be_a(Gitlab::FakeApplicationSettings)
end end
...@@ -91,6 +91,14 @@ describe Gitlab::CurrentSettings do ...@@ -91,6 +91,14 @@ describe Gitlab::CurrentSettings do
allow(ActiveRecord::Base.connection).to receive(:cached_table_exists?).with('application_settings').and_return(true) allow(ActiveRecord::Base.connection).to receive(:cached_table_exists?).with('application_settings').and_return(true)
end 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 it 'creates default ApplicationSettings if none are present' do
settings = described_class.current_application_settings settings = described_class.current_application_settings
...@@ -99,34 +107,45 @@ describe Gitlab::CurrentSettings do ...@@ -99,34 +107,45 @@ describe Gitlab::CurrentSettings do
expect(settings).to have_attributes(settings_from_defaults) expect(settings).to have_attributes(settings_from_defaults)
end end
context 'with migrations pending' do context 'with pending migrations' do
before do before do
expect(ActiveRecord::Migrator).to receive(:needs_migration?).and_return(true) expect(ActiveRecord::Migrator).to receive(:needs_migration?).and_return(true)
end end
it 'returns an in-memory ApplicationSetting object' do shared_examples 'a non-persisted ApplicationSetting object' do
settings = described_class.current_application_settings let(:current_settings) { described_class.current_application_settings }
expect(settings).to be_a(Gitlab::FakeApplicationSettings) it 'returns a non-persisted ApplicationSetting object' do
expect(settings.sign_in_enabled?).to eq(settings.sign_in_enabled) expect(current_settings).to be_a(ApplicationSetting)
expect(settings.sign_up_enabled?).to eq(settings.sign_up_enabled) expect(current_settings).not_to be_persisted
end end
it 'uses the existing database settings and falls back to defaults' do it 'uses the default value from ApplicationSetting.defaults' do
db_settings = create(:application_setting, expect(current_settings.signup_enabled).to eq(ApplicationSetting.defaults[:signup_enabled])
home_page_url: 'http://mydomain.com', end
signup_enabled: false)
settings = described_class.current_application_settings it 'uses the default value from custom ApplicationSetting accessors' do
app_defaults = ApplicationSetting.last 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
expect(settings).to be_a(Gitlab::FakeApplicationSettings) context 'with no ApplicationSetting DB record' do
expect(settings.home_page_url).to eq(db_settings.home_page_url) it_behaves_like 'a non-persisted ApplicationSetting object'
expect(settings.signup_enabled?).to be_falsey end
expect(settings.signup_enabled).to be_falsey
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 }
# Check that unspecified values use the defaults it_behaves_like 'a non-persisted ApplicationSetting object'
settings.reject! { |key, _| [:home_page_url, :signup_enabled].include? key }
settings.each { |key, _| expect(settings[key]).to eq(app_defaults[key]) } 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
end end
...@@ -138,17 +157,12 @@ describe Gitlab::CurrentSettings do ...@@ -138,17 +157,12 @@ describe Gitlab::CurrentSettings do
end end
end end
context 'when the application_settings table does not exists' do context 'when the application_settings table does not exist' do
it 'returns an in-memory ApplicationSetting object' do it 'returns a FakeApplicationSettings object' do
expect(ApplicationSetting).to receive(:create_from_defaults).and_raise(ActiveRecord::StatementInvalid) expect(Gitlab::Database)
.to receive(:cached_table_exists?)
expect(described_class.current_application_settings).to be_a(Gitlab::FakeApplicationSettings) .with('application_settings')
end .and_return(false)
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)
expect(described_class.current_application_settings).to be_a(Gitlab::FakeApplicationSettings) expect(described_class.current_application_settings).to be_a(Gitlab::FakeApplicationSettings)
end end
......
...@@ -20,6 +20,10 @@ describe CacheableAttributes do ...@@ -20,6 +20,10 @@ describe CacheableAttributes do
@_last ||= new('foo' => 'a', 'bar' => 'b') @_last ||= new('foo' => 'a', 'bar' => 'b')
end end
def self.column_names
%w[foo bar baz]
end
attr_accessor :attributes attr_accessor :attributes
def initialize(attrs = {}, *) def initialize(attrs = {}, *)
...@@ -75,13 +79,13 @@ describe CacheableAttributes do ...@@ -75,13 +79,13 @@ describe CacheableAttributes do
context 'without any attributes given' do context 'without any attributes given' do
it 'intializes a new object with the defaults' 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
end end
context 'with attributes given' do context 'with attributes given' do
it 'intializes a new object with the given attributes merged into the defaults' 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
end end
......
...@@ -216,11 +216,15 @@ RSpec.configure do |config| ...@@ -216,11 +216,15 @@ RSpec.configure do |config|
# Each example may call `migrate!`, so we must ensure we are migrated down every time # Each example may call `migrate!`, so we must ensure we are migrated down every time
config.before(:each, :migration) do config.before(:each, :migration) do
use_fake_application_settings
schema_migrate_down! schema_migrate_down!
end end
config.after(:context, :migration) do config.after(:context, :migration) do
schema_migrate_up! schema_migrate_up!
Gitlab::CurrentSettings.clear_in_memory_application_settings!
end end
config.around(:each, :nested_groups) do |example| config.around(:each, :nested_groups) do |example|
......
...@@ -62,6 +62,22 @@ module MigrationsHelpers ...@@ -62,6 +62,22 @@ module MigrationsHelpers
klass.reset_column_information klass.reset_column_information
end 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 def previous_migration
migrations.each_cons(2) do |previous, migration| migrations.each_cons(2) do |previous, migration|
break previous if migration.name == described_class.name break previous if migration.name == described_class.name
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment