Merge branch '207126-more-descriptive-error-messages-in-migration-helpers' into 'master'

Resolve "More descriptive error messages in Migration Helpers"

See merge request gitlab-org/gitlab!25457

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
@@ -931,7 +931,10 @@ module Gitlab
       def column_for(table, name)
         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?
+
+        column
       end
 
       # This will replace the first occurrence of a string in a column with
@@ -1166,6 +1169,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

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
@@ -587,6 +588,8 @@ describe Gitlab::Database::MigrationHelpers do
   end
 
   describe '#add_column_with_default' do
+    let(:column) { Project.columns.find { |c| c.name == "id" } }
+
     context 'outside of a transaction' do
       context 'when a column limit is not set' do
         before do
@@ -601,6 +604,9 @@ describe Gitlab::Database::MigrationHelpers do
 
           expect(model).to receive(:change_column_default)
             .with(:projects, :foo, 10)
+
+          expect(model).to receive(:column_for)
+            .with(:projects, :foo).and_return(column)
         end
 
         it 'adds the column while allowing NULL values' do
@@ -655,6 +661,7 @@ describe Gitlab::Database::MigrationHelpers do
         it 'adds the column with a limit' do
           allow(model).to receive(:transaction_open?).and_return(false)
           allow(model).to receive(:transaction).and_yield
+          allow(model).to receive(:column_for).with(:projects, :foo).and_return(column)
           allow(model).to receive(:update_column_in_batches).with(:projects, :foo, 10)
           allow(model).to receive(:change_column_null).with(:projects, :foo, false)
           allow(model).to receive(:change_column_default).with(:projects, :foo, 10)
@@ -721,50 +728,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(:change_column_null).with(:users, :new, false)
+          expect(model).to receive(:update_column_in_batches)
 
-        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(: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)
+
+          model.rename_column_concurrently(:users, :old, :new)
+        end
 
-        model.rename_column_concurrently(:users, :old, :new)
+        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
@@ -1133,8 +1158,9 @@ describe Gitlab::Database::MigrationHelpers do
       expect(column.name).to eq('id')
     end
 
-    it 'returns nil when a column does not exist' do
-      expect(model.column_for(:users, :kittens)).to be_nil
+    it 'raises an error when a column does not exist' do
+      error_message = /Could not find column "kittens" on table "users"/
+      expect { model.column_for(:users, :kittens) }.to raise_error(error_message)
     end
   end