Commit 71672dfa authored by Rémy Coutable's avatar Rémy Coutable

Return an ApplicationSetting in CurrentSettings

This replaces the use of fake_application_settings with
`::ApplicationSetting.build`_from_defaults. The reason is that
`fake_application_settings` doesn't have the custom accessors that
`ApplicationSetting` has, e.g. `#commit_email_hostname`, thus this
can lead to unexpected `nil` values which comes from the database
column instead of `.default_commit_email_hostname` returned by
`ApplicationSetting#commit_email_hostname`.

Using `::ApplicationSetting.build_from_defaults` should be safe as it
doesn't try to `INSERT` a DB record, in contrary to
`::ApplicationSetting.create_from_defaults` which we used to use, and
which created issues that the introduction of
`fake_application_settings` tried to resolve (575dced5).
Signed-off-by: default avatarRémy Coutable <remy@rymai.me>
parent ffef28cc
......@@ -382,7 +382,7 @@ class ApplicationSetting < ActiveRecord::Base
end
def restricted_visibility_levels=(levels)
super(levels.map { |level| Gitlab::VisibilityLevel.level_value(level) })
super(levels&.map { |level| Gitlab::VisibilityLevel.level_value(level) })
end
def strip_sentry_values
......
---
title: Return an ApplicationSetting in CurrentSettings
merge_request: 23766
author:
type: fixed
......@@ -50,7 +50,15 @@ 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)
db_attributes = current_settings&.attributes || {}
column_names = ::ApplicationSetting.column_names
final_attributes = ::ApplicationSetting
.defaults
.merge(db_attributes)
.stringify_keys
.slice(*column_names)
return ::ApplicationSetting.new(final_attributes)
end
return current_settings if current_settings.present?
......
......@@ -91,6 +91,14 @@ describe Gitlab::CurrentSettings do
allow(ActiveRecord::Base.connection).to receive(:cached_table_exists?).with('application_settings').and_return(true)
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
settings = described_class.current_application_settings
......@@ -99,34 +107,45 @@ describe Gitlab::CurrentSettings do
expect(settings).to have_attributes(settings_from_defaults)
end
context 'with migrations pending' do
context 'with pending migrations' do
before do
expect(ActiveRecord::Migrator).to receive(:needs_migration?).and_return(true)
end
it 'returns an in-memory ApplicationSetting object' do
settings = described_class.current_application_settings
shared_examples 'a non-persisted ApplicationSetting object' do
it 'returns a non-persisted ApplicationSetting object' do
expect(current_settings).to be_a(ApplicationSetting)
expect(current_settings).not_to be_persisted
end
it 'uses the default value from ApplicationSetting.defaults' do
expect(current_settings.signup_enabled).to eq(ApplicationSetting.defaults[:signup_enabled])
end
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)
it 'uses the default value from custom ApplicationSetting accessors' do
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
context 'with no ApplicationSetting DB record' do
it_behaves_like 'a non-persisted ApplicationSetting object' do
let(:current_settings) { described_class.current_application_settings }
end
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 = described_class.current_application_settings
app_defaults = ApplicationSetting.last
expect(settings).to be_a(Gitlab::FakeApplicationSettings)
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]) }
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 }
it_behaves_like 'a non-persisted ApplicationSetting object'
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
......
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