Merge pull request #141 from doctolib/explicitly-ensure-indexes-removed

Added new check to ensure composite indexes are dropped ahead of removal of column

Author: CharlotteFeather

lib/safe-pg-migrations/base.rb

@@ -11,6 +11,7 @@
 require 'safe-pg-migrations/plugins/blocking_activity_logger'
 require 'safe-pg-migrations/plugins/statement_insurer/add_column'
 require 'safe-pg-migrations/plugins/statement_insurer/change_column_null'
+require 'safe-pg-migrations/plugins/statement_insurer/remove_column_index'
 require 'safe-pg-migrations/plugins/statement_insurer'
 require 'safe-pg-migrations/plugins/statement_retrier'
 require 'safe-pg-migrations/plugins/idempotent_statements'

lib/safe-pg-migrations/plugins/statement_insurer.rb

@@ -5,6 +5,7 @@ module StatementInsurer
     include Helpers::SessionSettingManagement
     include AddColumn
     include ChangeColumnNull
+    include RemoveColumnIndex
 
     def validate_check_constraint(table_name, **options)
       Helpers::Logger.say_method_call :validate_check_constraint, table_name, **options
@@ -19,7 +20,7 @@ def add_check_constraint(table_name, expression, **options)
 
       Helpers::Logger.say_method_call :add_check_constraint, table_name, expression, **options,
         validate: false
-      super table_name, expression, **options, validate: false
+      super(table_name, expression, **options, validate: false)
 
       return unless options.fetch(:validate, true)
 
@@ -74,6 +75,7 @@ def remove_column(table_name, column_name, type = nil, **options)
       foreign_key = foreign_key_for(table_name, column: column_name)
 
       remove_foreign_key(table_name, name: foreign_key.name) if foreign_key
+      remove_column_with_composite_index(table_name, column_name)
       super
     end
 

lib/safe-pg-migrations/plugins/statement_insurer/add_column.rb

@@ -21,7 +21,7 @@ def add_column(table_name, column_name, type, **options)
         null = options.delete(:null)
 
         Helpers::Logger.say_method_call(:add_column, table_name, column_name, type, options)
-        super table_name, column_name, type, **options
+        super(table_name, column_name, type, **options)
 
         Helpers::Logger.say_method_call(:change_column_default, table_name, column_name, default)
         change_column_default(table_name, column_name, default)

lib/safe-pg-migrations/plugins/statement_insurer/change_column_null.rb

@@ -16,7 +16,7 @@ def change_column_null(table_name, column_name, null, default = nil)
         add_check_constraint table_name, expression, name: constraint_name
 
         Helpers::Logger.say_method_call :change_column_null, table_name, column_name, false
-        super table_name, column_name, false
+        super(table_name, column_name, false)
 
         return unless should_remove_constraint? default_name, constraint_name
 

lib/safe-pg-migrations/plugins/statement_insurer/remove_column_index.rb

@@ -0,0 +1,23 @@
+# frozen_string_literal: true
+
+module SafePgMigrations
+  module StatementInsurer
+    module RemoveColumnIndex
+      def remove_column_with_composite_index(table, column)
+        existing_indexes = indexes(table).select { |index|
+          index.columns.size > 1 && index.columns.include?(column.to_s)
+        }
+
+        return unless existing_indexes.any?
+
+        error_message = <<~ERROR
+          Cannot drop column #{column} from table #{table} because composite index(es): #{existing_indexes.map(&:name).join(', ')} is/are present.
+          If they are still required, create the index(es) without #{column} before dropping the existing index(es).
+          Then you will be able to drop the column.
+        ERROR
+
+        raise StandardError, error_message
+      end
+    end
+  end
+end

test/StatementInsurer/remove_column_test.rb

@@ -41,7 +41,7 @@ def change
       assert_equal ['ALTER TABLE "users" DROP COLUMN "name"'], calls[2]
     end
 
-    def test_can_remove_column_without_foreign_key
+    def test_can_remove_column_without_foreign_key_or_index
       @connection.create_table(:users) { |t| t.string :name }
 
       @migration =
@@ -55,5 +55,58 @@ def change
 
       assert_equal ['ALTER TABLE "users" DROP COLUMN "name"'], calls[2]
     end
+
+    def test_can_remove_column_with_index_on_other_columns
+      @connection.create_table(:users) { |t| t.string :name, :email }
+      @connection.add_index(:users, :email)
+
+      @migration =
+        Class.new(ActiveRecord::Migration::Current) do
+          def change
+            remove_column(:users, :name)
+          end
+        end.new
+
+      calls = record_calls(@connection, :execute) { run_migration }
+
+      assert_equal ['ALTER TABLE "users" DROP COLUMN "name"'], calls[2]
+    end
+
+    def test_can_remove_column_with_dependent_index
+      @connection.create_table(:users) { |t| t.string :name, :email }
+      @connection.add_index(:users, :name)
+
+      @migration =
+        Class.new(ActiveRecord::Migration::Current) do
+          def change
+            remove_column(:users, :name)
+          end
+        end.new
+
+      calls = record_calls(@connection, :execute) { run_migration }
+
+      assert_equal ['ALTER TABLE "users" DROP COLUMN "name"'], calls[2]
+    end
+
+    def test_can_not_remove_column_with_dependent_composite_index
+      @connection.create_table(:users) { |t| t.string :name, :email }
+      @connection.add_index(:users, %i[name email], name: 'index_users_on_name_and_email')
+
+      @migration =
+        Class.new(ActiveRecord::Migration::Current) do
+          def change
+            remove_column(:users, :name)
+          end
+        end.new
+
+      error_message = <<~ERROR
+        Cannot drop column name from table users because composite index(es): index_users_on_name_and_email is/are present.
+        If they are still required, create the index(es) without name before dropping the existing index(es).
+        Then you will be able to drop the column.
+      ERROR
+
+      exception = assert_raises(StandardError, error_message) { run_migration }
+      assert_match error_message, exception.message
+    end
   end
 end