Merge branch...

Merge branch '5794-we-should-failover-gracefully-when-we-can-t-connect-to-geo-tracking-database' into 'master'

Keep ShaAttribute from halting startup when we can’t connect to Geo tracking database

Closes #5794

See merge request gitlab-org/gitlab-ee!5502

app/models/concerns/sha_attribute.rb

@@ -2,20 +2,35 @@ module ShaAttribute
   extend ActiveSupport::Concern
 
   module ClassMethods
-    def sha_attribute(name, database_available = true)
+    def sha_attribute(name)
       return if ENV['STATIC_VERIFICATION']
-      return unless database_available && 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

ee/app/models/geo/project_registry.rb

@@ -10,8 +10,8 @@ class Geo::ProjectRegistry < Geo::BaseRegistry
   ignore_column :repository_verification_checksum
   ignore_column :wiki_verification_checksum
 
-  sha_attribute :repository_verification_checksum_sha, ::Gitlab::Geo.geo_database_configured?
-  sha_attribute :wiki_verification_checksum_sha, ::Gitlab::Geo.geo_database_configured?
+  sha_attribute :repository_verification_checksum_sha
+  sha_attribute :wiki_verification_checksum_sha
 
   belongs_to :project
 

ee/changelogs/unreleased/5794-we-should-failover-gracefully-when-we-can-t-connect-to-geo-tracking-database.yml

+---
+title: ShaAttribute no longer stops startup if database is missing
+merge_request: 5502
+author:
+type: fixed

spec/models/concerns/sha_attribute_spec.rb

@@ -13,6 +13,11 @@ describe ShaAttribute do
   end
 
   describe '#sha_attribute' do
+    context 'when in non-production' do
+      before do
+        allow(Rails.env).to receive(:production?).and_return(false)
+      end
+
       context 'when the table exists' do
         before do
           allow(model).to receive(:table_exists?).and_return(true)
@@ -31,15 +36,51 @@ describe ShaAttribute do
       end
 
       context 'when the table does not exist' do
-      before 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 'does nothing' do
+      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)
 
-        model.sha_attribute(:name)
+          expect { model.sha_attribute(:name) }.to raise_error(ActiveRecord::NoDatabaseError)
+        end
+      end
+    end
+
+    context 'when in production' do
+      before do
+        allow(Rails.env).to receive(:production?).and_return(true)
+      end
+
+      it 'defines a SHA attribute' do
+        expect(model).not_to receive(:table_exists?)
+        expect(model).not_to receive(:columns)
+        expect(model).to receive(:attribute).with(:sha1, an_instance_of(Gitlab::Database::ShaAttribute))
+
+        model.sha_attribute(:sha1)
       end
     end
   end