Improve messages for certain migration errors

Display a more detailed error message when a migration references an
existing database object (like column or foreign key) that doesn't
exist.

changelogs/unreleased/207126-more-descriptive-error-messages-in-migration-helpers.yml

+---
+title: Improve error messages of failed migrations
+merge_request: 25457
+author:
+type: changed

lib/gitlab/database/migration_helpers.rb

@@ -215,7 +215,7 @@ module Gitlab
         fk_name = name || concurrent_foreign_key_name(source, column)
 
         unless foreign_key_exists?(source, name: fk_name)
-          raise "cannot find #{fk_name} on #{source} table"
+          raise missing_schema_object_message(source, "foreign key", fk_name)
         end
 
         disable_statement_timeout do
@@ -750,7 +750,7 @@ module Gitlab
 
         check_trigger_permissions!(table)
 
-        old_col = column_for(table, old_column)
+        old_col = column_for(table, old_column, raise_if_missing: true)
         new_type = type || old_col.type
         max_index = 0
 
@@ -922,10 +922,13 @@ module Gitlab
       end
 
       # Returns the column for the given table and column name.
-      def column_for(table, name)
+      def column_for(table, name, raise_if_missing: false)
         name = name.to_s
 
-        columns(table).find { |column| column.name == name }
+        column = columns(table).find { |column| column.name == name }
+        raise(missing_schema_object_message(table, "column", name)) if column.nil? && raise_if_missing
+
+        column
       end
 
       # This will replace the first occurrence of a string in a column with
@@ -1135,6 +1138,18 @@ into similar problems in the future (e.g. when new tables are created).
 
       private
 
+      def missing_schema_object_message(table, type, name)
+        <<~MESSAGE
+          Could not find #{type} "#{name}" on table "#{table}" which was referenced during the migration.
+          This issue could be caused by the database schema straying from the expected state.
+
+          To resolve this issue, please verify:
+            1. all previous migrations have completed
+            2. the database objects used in this migration match the Rails definition in schema.rb or structure.sql
+
+        MESSAGE
+      end
+
       def tables_match?(target_table, foreign_key_table)
         target_table.blank? || foreign_key_table == target_table
       end
@@ -1151,7 +1166,7 @@ into similar problems in the future (e.g. when new tables are created).
       end
 
       def create_column_from(table, old, new, type: nil)
-        old_col = column_for(table, old)
+        old_col = column_for(table, old, raise_if_missing: true)
         new_type = type || old_col.type
 
         add_column(table, new, new_type,

spec/lib/gitlab/database/migration_helpers_spec.rb

@@ -383,7 +383,8 @@ describe Gitlab::Database::MigrationHelpers do
         it 'raises an error' do
           expect(model).to receive(:foreign_key_exists?).and_return(false)
 
-          expect { model.validate_foreign_key(:projects, :user_id) }.to raise_error(/cannot find/)
+          error_message = /Could not find foreign key "fk_name" on table "projects"/
+          expect { model.validate_foreign_key(:projects, :user_id, name: :fk_name) }.to raise_error(error_message)
         end
       end
     end
@@ -721,50 +722,68 @@ describe Gitlab::Database::MigrationHelpers do
 
       before do
         allow(model).to receive(:transaction_open?).and_return(false)
-        allow(model).to receive(:column_for).and_return(old_column)
       end
 
-      it 'renames a column concurrently' do
-        expect(model).to receive(:check_trigger_permissions!).with(:users)
+      context 'when the column to rename exists' do
+        before do
+          allow(model).to receive(:column_for).and_return(old_column)
+        end
 
-        expect(model).to receive(:install_rename_triggers_for_postgresql)
-          .with(trigger_name, '"users"', '"old"', '"new"')
+        it 'renames a column concurrently' do
+          expect(model).to receive(:check_trigger_permissions!).with(:users)
 
-        expect(model).to receive(:add_column)
-          .with(:users, :new, :integer,
-               limit: old_column.limit,
-               precision: old_column.precision,
-               scale: old_column.scale)
+          expect(model).to receive(:install_rename_triggers_for_postgresql)
+            .with(trigger_name, '"users"', '"old"', '"new"')
 
-        expect(model).to receive(:change_column_default)
-          .with(:users, :new, old_column.default)
+          expect(model).to receive(:add_column)
+            .with(:users, :new, :integer,
+                 limit: old_column.limit,
+                 precision: old_column.precision,
+                 scale: old_column.scale)
 
-        expect(model).to receive(:update_column_in_batches)
+          expect(model).to receive(:change_column_default)
+            .with(:users, :new, old_column.default)
+
+          expect(model).to receive(:update_column_in_batches)
 
-        expect(model).to receive(:change_column_null).with(:users, :new, false)
+          expect(model).to receive(:change_column_null).with(:users, :new, false)
 
-        expect(model).to receive(:copy_indexes).with(:users, :old, :new)
-        expect(model).to receive(:copy_foreign_keys).with(:users, :old, :new)
+          expect(model).to receive(:copy_indexes).with(:users, :old, :new)
+          expect(model).to receive(:copy_foreign_keys).with(:users, :old, :new)
 
-        model.rename_column_concurrently(:users, :old, :new)
+          model.rename_column_concurrently(:users, :old, :new)
+        end
+
+        context 'when default is false' do
+          let(:old_column) do
+            double(:column,
+                 type: :boolean,
+                 limit: nil,
+                 default: false,
+                 null: false,
+                 precision: nil,
+                 scale: nil)
+          end
+
+          it 'copies the default to the new column' do
+            expect(model).to receive(:change_column_default)
+              .with(:users, :new, old_column.default)
+
+            model.rename_column_concurrently(:users, :old, :new)
+          end
+        end
       end
 
-      context 'when default is false' do
-        let(:old_column) do
-          double(:column,
-               type: :boolean,
-               limit: nil,
-               default: false,
-               null: false,
-               precision: nil,
-               scale: nil)
+      context 'when the column to be renamed does not exist' do
+        before do
+          allow(model).to receive(:columns).and_return([])
         end
 
-        it 'copies the default to the new column' do
-          expect(model).to receive(:change_column_default)
-            .with(:users, :new, old_column.default)
+        it 'raises an error with appropriate message' do
+          expect(model).to receive(:check_trigger_permissions!).with(:users)
 
-          model.rename_column_concurrently(:users, :old, :new)
+          error_message = /Could not find column "missing_column" on table "users"/
+          expect { model.rename_column_concurrently(:users, :missing_column, :new) }.to raise_error(error_message)
         end
       end
     end