From 18094a17458aab8c9d629b86946b61e0e7bab251 Mon Sep 17 00:00:00 2001 From: Brett Walker Date: Tue, 8 May 2018 00:14:00 +0000 Subject: =?UTF-8?q?Backport:=20Keep=20ShaAttribute=20from=20halting=20star?= =?UTF-8?q?tup=20when=20we=20can=E2=80=99t=20connect=20to=20a=20database?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/models/concerns/sha_attribute.rb | 29 +++++++--- ...e-can-t-connect-to-geo-tracking-database-ce.yml | 5 ++ spec/models/concerns/sha_attribute_spec.rb | 67 +++++++++++++++++----- 3 files changed, 81 insertions(+), 20 deletions(-) create mode 100644 changelogs/unreleased/5794-we-should-failover-gracefully-when-we-can-t-connect-to-geo-tracking-database-ce.yml diff --git a/app/models/concerns/sha_attribute.rb b/app/models/concerns/sha_attribute.rb index 703a72c355c..3340dc96e9f 100644 --- a/app/models/concerns/sha_attribute.rb +++ b/app/models/concerns/sha_attribute.rb @@ -4,18 +4,33 @@ module ShaAttribute module ClassMethods def sha_attribute(name) return if ENV['STATIC_VERIFICATION'] - return unless table_exists? + + validate_binary_column_exists!(name) unless Rails.env.production? + + attribute(name, Gitlab::Database::ShaAttribute.new) + end + + # This only gets executed in non-production environments as an additional check to ensure + # the column is the correct type. In production it should behave like any other attribute. + # See https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/5502 for more discussion + def validate_binary_column_exists!(name) + unless table_exists? + warn "WARNING: sha_attribute #{name.inspect} is invalid since the table doesn't exist - you may need to run database migrations" + return + end column = columns.find { |c| c.name == name.to_s } - # In case the table doesn't exist we won't be able to find the column, - # thus we will only check the type if the column is present. - if column && column.type != :binary - raise ArgumentError, - "sha_attribute #{name.inspect} is invalid since the column type is not :binary" + unless column + raise ArgumentError.new("sha_attribute #{name.inspect} is invalid since the column doesn't exist") end - attribute(name, Gitlab::Database::ShaAttribute.new) + unless column.type == :binary + raise ArgumentError.new("sha_attribute #{name.inspect} is invalid since the column type is not :binary") + end + rescue => error + Gitlab::AppLogger.error "ShaAttribute initialization: #{error.message}" + raise end end end diff --git a/changelogs/unreleased/5794-we-should-failover-gracefully-when-we-can-t-connect-to-geo-tracking-database-ce.yml b/changelogs/unreleased/5794-we-should-failover-gracefully-when-we-can-t-connect-to-geo-tracking-database-ce.yml new file mode 100644 index 00000000000..4db0ff4f3a0 --- /dev/null +++ b/changelogs/unreleased/5794-we-should-failover-gracefully-when-we-can-t-connect-to-geo-tracking-database-ce.yml @@ -0,0 +1,5 @@ +--- +title: ShaAttribute no longer stops startup if database is missing +merge_request: 18726 +author: +type: fixed diff --git a/spec/models/concerns/sha_attribute_spec.rb b/spec/models/concerns/sha_attribute_spec.rb index 21893e0cbaa..592feddf1dc 100644 --- a/spec/models/concerns/sha_attribute_spec.rb +++ b/spec/models/concerns/sha_attribute_spec.rb @@ -13,33 +13,74 @@ describe ShaAttribute do end describe '#sha_attribute' do - context 'when the table exists' do + context 'when in non-production' do before do - allow(model).to receive(:table_exists?).and_return(true) + allow(Rails.env).to receive(:production?).and_return(false) end - it 'defines a SHA attribute for a binary column' do - expect(model).to receive(:attribute) - .with(:sha1, an_instance_of(Gitlab::Database::ShaAttribute)) + context 'when the table exists' do + before do + allow(model).to receive(:table_exists?).and_return(true) + end - model.sha_attribute(:sha1) + it 'defines a SHA attribute for a binary column' do + expect(model).to receive(:attribute) + .with(:sha1, an_instance_of(Gitlab::Database::ShaAttribute)) + + model.sha_attribute(:sha1) + end + + it 'raises ArgumentError when the column type is not :binary' do + expect { model.sha_attribute(:name) }.to raise_error(ArgumentError) + end + end + + context 'when the table does not exist' do + it 'allows the attribute to be added' do + allow(model).to receive(:table_exists?).and_return(false) + + expect(model).not_to receive(:columns) + expect(model).to receive(:attribute) + + model.sha_attribute(:name) + end end - it 'raises ArgumentError when the column type is not :binary' do - expect { model.sha_attribute(:name) }.to raise_error(ArgumentError) + context 'when the column does not exist' do + it 'raises ArgumentError' do + allow(model).to receive(:table_exists?).and_return(true) + + expect(model).to receive(:columns) + expect(model).not_to receive(:attribute) + + expect { model.sha_attribute(:no_name) }.to raise_error(ArgumentError) + end + end + + context 'when other execeptions are raised' do + it 'logs and re-rasises the error' do + allow(model).to receive(:table_exists?).and_raise(ActiveRecord::NoDatabaseError.new('does not exist')) + + expect(model).not_to receive(:columns) + expect(model).not_to receive(:attribute) + expect(Gitlab::AppLogger).to receive(:error) + + expect { model.sha_attribute(:name) }.to raise_error(ActiveRecord::NoDatabaseError) + end end end - context 'when the table does not exist' do + context 'when in production' do before do - allow(model).to receive(:table_exists?).and_return(false) + allow(Rails.env).to receive(:production?).and_return(true) end - it 'does nothing' do + it 'defines a SHA attribute' do + expect(model).not_to receive(:table_exists?) expect(model).not_to receive(:columns) - expect(model).not_to receive(:attribute) + expect(model).to receive(:attribute).with(:sha1, an_instance_of(Gitlab::Database::ShaAttribute)) - model.sha_attribute(:name) + model.sha_attribute(:sha1) end end end -- cgit v1.2.1