diff --git a/lib/safe-pg-migrations/base.rb b/lib/safe-pg-migrations/base.rb index e7d354d..4a2763b 100644 --- a/lib/safe-pg-migrations/base.rb +++ b/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' diff --git a/lib/safe-pg-migrations/plugins/statement_insurer.rb b/lib/safe-pg-migrations/plugins/statement_insurer.rb index b9ccde3..d533b28 100644 --- a/lib/safe-pg-migrations/plugins/statement_insurer.rb +++ b/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 diff --git a/lib/safe-pg-migrations/plugins/statement_insurer/add_column.rb b/lib/safe-pg-migrations/plugins/statement_insurer/add_column.rb index cd57316..372823b 100644 --- a/lib/safe-pg-migrations/plugins/statement_insurer/add_column.rb +++ b/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) diff --git a/lib/safe-pg-migrations/plugins/statement_insurer/change_column_null.rb b/lib/safe-pg-migrations/plugins/statement_insurer/change_column_null.rb index 85624bb..895bf13 100644 --- a/lib/safe-pg-migrations/plugins/statement_insurer/change_column_null.rb +++ b/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 diff --git a/lib/safe-pg-migrations/plugins/statement_insurer/remove_column_index.rb b/lib/safe-pg-migrations/plugins/statement_insurer/remove_column_index.rb new file mode 100644 index 0000000..5748d0a --- /dev/null +++ b/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 diff --git a/test/StatementInsurer/remove_column_test.rb b/test/StatementInsurer/remove_column_test.rb index 1c55abb..08d36bb 100644 --- a/test/StatementInsurer/remove_column_test.rb +++ b/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