From cf718cb675161871779af35dd94953392fef2dd6 Mon Sep 17 00:00:00 2001 From: fatkodima Date: Fri, 20 Dec 2019 21:46:45 +0200 Subject: [PATCH 1/3] Add add_column_with_default_safely and update_column_in_batches migration helpers --- CHANGELOG.md | 2 + lib/strong_migrations.rb | 20 +---- lib/strong_migrations/checker.rb | 38 +--------- lib/strong_migrations/migration_helpers.rb | 85 ++++++++++++++++++++-- lib/strong_migrations/util.rb | 28 +++++++ test/migration_helpers_test.rb | 54 ++++++++++++++ test/strong_migrations_test.rb | 3 +- test/test_helper.rb | 3 + 8 files changed, 174 insertions(+), 59 deletions(-) create mode 100644 lib/strong_migrations/util.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index b6304411..1248a0b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ - Added `add_foreign_key_safely` method - Added `add_null_constraint_safely` and `remove_null_constraint_safely` methods +- Added `add_column_safely` method +- Added `backfill_column_safely` method - Added `statement_timeout` and `lock_timeout` functionality ## 0.5.1 (2019-12-17) diff --git a/lib/strong_migrations.rb b/lib/strong_migrations.rb index 334e2983..b54cf18a 100644 --- a/lib/strong_migrations.rb +++ b/lib/strong_migrations.rb @@ -2,6 +2,7 @@ require "active_support" # modules +require "strong_migrations/util" require "strong_migrations/checker" require "strong_migrations/database_tasks" require "strong_migrations/migration" @@ -25,28 +26,15 @@ class << self self.error_messages = { add_column_default: "Adding a column with a non-null default causes the entire table to be rewritten. -Instead, add the column without a default value, then change the default. +Instead, add the column without a default value, backfill and then change the default. class %{migration_name} < ActiveRecord::Migration%{migration_suffix} - def up - %{add_command} - %{change_command} - end - - def down - %{remove_command} - end -end - -Then backfill the existing rows in the Rails console or a separate migration with disable_ddl_transaction!. - -class Backfill%{migration_name} < ActiveRecord::Migration%{migration_suffix} disable_ddl_transaction! def change - %{code} + %{command} end -end%{append}", +end", add_column_json: "There's no equality operator for the json column type, which can diff --git a/lib/strong_migrations/checker.rb b/lib/strong_migrations/checker.rb index 8d1afd73..9d331a07 100644 --- a/lib/strong_migrations/checker.rb +++ b/lib/strong_migrations/checker.rb @@ -1,5 +1,7 @@ module StrongMigrations class Checker + include Util + attr_accessor :direction def initialize(migration) @@ -81,20 +83,8 @@ def perform(method, *args) default = options[:default] if !default.nil? && !(postgresql? && postgresql_version >= 110000) - - if options[:null] == false - options = options.except(:null) - append = " - -Then add the NOT NULL constraint." - end - raise_error :add_column_default, - add_command: command_str("add_column", [table, column, type, options.except(:default)]), - change_command: command_str("change_column_default", [table, column, default]), - remove_command: command_str("remove_column", [table, column]), - code: backfill_code(table, column, default), - append: append + command: command_str("add_column_safely", [table, column, type, options]) end if type.to_s == "json" && postgresql? @@ -186,10 +176,6 @@ def set_timeouts private - def connection - @migration.connection - end - def version @migration.version end @@ -202,22 +188,6 @@ def version_safe? version && version <= StrongMigrations.start_after end - def postgresql? - %w(PostgreSQL PostGIS).include?(connection.adapter_name) - end - - def postgresql_version - @postgresql_version ||= begin - target_version = StrongMigrations.target_postgresql_version - if target_version && defined?(Rails) && (Rails.env.development? || Rails.env.test?) - # we only need major version right now - target_version.to_i * 10000 - else - connection.execute("SHOW server_version_num").first["server_version_num"].to_i - end - end - end - def raise_error(message_key, header: nil, **vars) return unless StrongMigrations.check_enabled?(message_key, version: version) @@ -238,7 +208,7 @@ def raise_error(message_key, header: nil, **vars) def constraint_str(statement, identifiers) # not all identifiers are tables, but this method of quoting should be fine - code = statement % identifiers.map { |v| connection.quote_table_name(v) } + code = quote_identifiers(statement, identifiers) "safety_assured do\n execute '#{code}' \n end" end diff --git a/lib/strong_migrations/migration_helpers.rb b/lib/strong_migrations/migration_helpers.rb index 9305a573..93f8882e 100644 --- a/lib/strong_migrations/migration_helpers.rb +++ b/lib/strong_migrations/migration_helpers.rb @@ -1,5 +1,7 @@ module StrongMigrations module MigrationHelpers + include Util + def add_foreign_key_safely(from_table, to_table, **options) ensure_postgresql(__method__) ensure_not_in_transaction(__method__) @@ -73,16 +75,87 @@ def remove_null_constraint_safely(table_name, column_name, name: nil) end end + def add_column_safely(table_name, column_name, type, **options) + ensure_postgresql(__method__) + ensure_not_in_transaction(__method__) + + default = options.delete(:default) + + if postgresql_version >= 110000 || default.nil? + add_column(table_name, column_name, options) + else + reversible do |dir| + dir.up do + transaction do + add_column(table_name, column_name, type, default: nil, **options) + change_column_default(table_name, column_name, default) + end + + default_after_type_cast = connection.type_cast(default) + backfill_column_safely(table_name, column_name, default_after_type_cast) + + allow_null = options[:null] + add_null_constraint_safely(table_name, column_name) unless allow_null + end + + dir.down do + remove_column(table_name, column_name) + end + end + end + end + + def backfill_column_safely(table_name, column_name, value, batch_size: 1000) + ensure_not_in_transaction(__method__) + + table = Arel::Table.new(table_name) + count_arel = table.project(Arel.star.count) + total = connection.exec_query(count_arel.to_sql).first["count"] + + return if total == 0 + + primary_key = connection.primary_key(table_name) + + start_arel = table + .project(table[primary_key]) + .order(table[primary_key].asc) + .take(1) + + start_pk = connection.exec_query(start_arel.to_sql).first[primary_key] + + loop do + finish_arel = table + .project(table[primary_key]) + .where(table[primary_key].gteq(start_pk)) + .order(table[primary_key].asc) + .skip(batch_size) + .take(1) + + finish_result = connection.exec_query(finish_arel.to_sql).first + + update_arel = Arel::UpdateManager.new + .table(table) + .set([[table[column_name], value]]) + .where(table[primary_key].gteq(start_pk)) + + if finish_result + finish_pk = finish_result[primary_key] + update_arel = update_arel.where(table[primary_key].lt(finish_pk)) + start_pk = finish_pk + end + + safety_assured { execute(update_arel.to_sql) } + + break unless finish_pk + end + end + private def ensure_postgresql(method_name) raise StrongMigrations::Error, "`#{method_name}` is intended for Postgres only" unless postgresql? end - def postgresql? - %w(PostgreSQL PostGIS).include?(connection.adapter_name) - end - def ensure_not_in_transaction(method_name) if connection.transaction_open? raise StrongMigrations::Error, "Cannot run `#{method_name}` inside a transaction. Use `disable_ddl_transaction` to disable the transaction." @@ -109,9 +182,5 @@ def on_delete_update_statement(delete_or_update, action) raise "'#{action}' is not supported for :on_update or :on_delete.\nSupported values are: :nullify, :cascade, :restrict" end end - - def quote_identifiers(statement, identifiers) - statement % identifiers.map { |v| connection.quote_table_name(v) } - end end end diff --git a/lib/strong_migrations/util.rb b/lib/strong_migrations/util.rb new file mode 100644 index 00000000..4432c690 --- /dev/null +++ b/lib/strong_migrations/util.rb @@ -0,0 +1,28 @@ +module StrongMigrations + module Util + def connection + ActiveRecord::Base.connection + end + + def postgresql? + %w(PostgreSQL PostGIS).include?(connection.adapter_name) + end + + def postgresql_version + @postgresql_version ||= begin + target_version = StrongMigrations.target_postgresql_version + if target_version && defined?(Rails) && (Rails.env.development? || Rails.env.test?) + # we only need major version right now + target_version.to_i * 10000 + else + connection.postgresql_version + end + end + end + + def quote_identifiers(statement, identifiers) + # not all identifiers are tables, but this method of quoting should be fine + statement % identifiers.map { |v| connection.quote_table_name(v) } + end + end +end diff --git a/test/migration_helpers_test.rb b/test/migration_helpers_test.rb index 102c2f18..0107d78a 100644 --- a/test/migration_helpers_test.rb +++ b/test/migration_helpers_test.rb @@ -12,6 +12,18 @@ def change end end +class AddColumnSafely < TestMigration + def change + add_column_safely :users, :nice, :boolean, default: true, null: false + end +end + +class BackfillColumnSafely < TestMigration + def up + backfill_column_safely :users, :city, "Kyiv", batch_size: 1 + end +end + class MigrationHelpersTest < Minitest::Test def test_add_foreign_key_safely skip unless postgresql? @@ -50,6 +62,48 @@ def test_add_null_constraint_safely migrate(AddNullConstraintSafely, direction: :down) end + def test_add_column_safely_raises_inside_transaction + skip unless postgresql? + error = assert_raises(StrongMigrations::Error) { migrate_inside_transaction(AddColumnSafely) } + assert_match "Cannot run `add_column_safely` inside a transaction", error.message + end + + def test_add_foreign_key_safely_raises_for_non_postgres + skip if postgresql? + error = assert_raises(StrongMigrations::Error) { migrate(AddColumnSafely) } + assert_match "is intended for Postgres", error.message + end + + def test_add_column_safely + skip unless postgresql? + + User.reset_column_information + migrate(AddColumnSafely) + + column = User.columns.find { |c| c.name == "nice" } + assert_equal :boolean, column.type + assert_equal "true", column.default + assert_equal false, column.null + + migrate(AddColumnSafely, direction: :down) + ensure + User.reset_column_information + end + + def test_backfill_column_safely_raises_inside_transaction + error = assert_raises(StrongMigrations::Error) { migrate_inside_transaction(BackfillColumnSafely) } + assert_match "Cannot run `backfill_column_safely` inside a transaction", error.message + end + + def test_backfill_column_safely + User.create([{ name: "John", city: "San Francisco" }, { name: "Jane", city: "London" }]) + migrate(BackfillColumnSafely) + users = User.all + assert users.all? { |u| u.city == "Kyiv" } + ensure + User.delete_all + end + private def connection diff --git a/test/strong_migrations_test.rb b/test/strong_migrations_test.rb index 8482a6ac..7377a868 100644 --- a/test/strong_migrations_test.rb +++ b/test/strong_migrations_test.rb @@ -61,7 +61,7 @@ def change class AddColumnDefaultSafe < TestMigration def change add_column :users, :nice, :boolean - change_column_default :users, :nice, false + change_column_default :users, :nice, from: nil, to: false end end @@ -328,6 +328,7 @@ def test_add_column_default def test_add_column_default_safe assert_safe AddColumnDefaultSafe + assert_safe AddColumnDefaultSafe, direction: :down end def test_add_column_json diff --git a/test/test_helper.rb b/test/test_helper.rb index 253a3f4e..4cd09806 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -48,6 +48,9 @@ def change end migrate CreateUsers +class User < ActiveRecord::Base +end + class Minitest::Test def postgresql? ENV["ADAPTER"].nil? From 41c3a5eb848414953b2cf72618fd9498475bad04 Mon Sep 17 00:00:00 2001 From: fatkodima Date: Sun, 22 Dec 2019 15:31:42 +0200 Subject: [PATCH 2/3] Add rename_column_safely and rename_column_safely_cleanup migration helpers --- lib/strong_migrations/migration_helpers.rb | 246 ++++++++++++++++++++- lib/strong_migrations/util.rb | 4 + test/migration_helpers_test.rb | 92 ++++++++ 3 files changed, 341 insertions(+), 1 deletion(-) diff --git a/lib/strong_migrations/migration_helpers.rb b/lib/strong_migrations/migration_helpers.rb index 93f8882e..a4a1858e 100644 --- a/lib/strong_migrations/migration_helpers.rb +++ b/lib/strong_migrations/migration_helpers.rb @@ -109,7 +109,7 @@ def backfill_column_safely(table_name, column_name, value, batch_size: 1000) ensure_not_in_transaction(__method__) table = Arel::Table.new(table_name) - count_arel = table.project(Arel.star.count) + count_arel = table.project(Arel.star.count.as("count")) total = connection.exec_query(count_arel.to_sql).first["count"] return if total == 0 @@ -150,6 +150,58 @@ def backfill_column_safely(table_name, column_name, value, batch_size: 1000) end end + def rename_column_safely(table_name, old, new) + if !postgresql? && !mysql? + raise StrongMigrations::Error, "`#{__method__}` is intended for Postgres and Mysql only" + end + + ensure_not_in_transaction(__method__) + ensure_trigger_privileges(table_name) + + reversible do |dir| + dir.up do + copy_column(table_name, old, new) + safety_assured { create_column_rename_triggers(table_name, old, new) } + end + + dir.down do + trigger_name = rename_column_trigger_name(table_name, old, new) + + safety_assured do + remove_column_rename_triggers(table_name, trigger_name) + + if mysql? + # Foreign key should be removed before removing column + foreign_key = foreign_key_for(table_name, new) + remove_foreign_key(table_name, column: new) if foreign_key + end + + remove_column(table_name, new) + end + end + end + end + + def rename_column_safely_cleanup(table_name, old, new) + ensure_not_in_transaction(__method__) + ensure_trigger_privileges(table_name) + + reversible do |dir| + dir.up do + trigger_name = rename_column_trigger_name(table_name, old, new) + safety_assured do + remove_column_rename_triggers(table_name, trigger_name) + remove_column(table_name, old) + end + end + + dir.down do + copy_column(table_name, new, old) + safety_assured { create_column_rename_triggers(table_name, old, new) } + end + end + end + private def ensure_postgresql(method_name) @@ -182,5 +234,197 @@ def on_delete_update_statement(delete_or_update, action) raise "'#{action}' is not supported for :on_update or :on_delete.\nSupported values are: :nullify, :cascade, :restrict" end end + + def ensure_trigger_privileges(table_name) + privileges = + if postgresql? + trigger_privileges_postgresql?(table_name) + else + # It is very hard to check in Mysql if user has trigger privileges. + # Let's assume that 'yes' and fail later. + true + end + + unless privileges + raise StrongMigrations::Error, "Current database user cannot create, execute, or drop triggers on the #{table_name} table." + end + end + + def trigger_privileges_postgresql?(table_name) + quoted_table = connection.quote(table_name) + row = connection.exec_query("SELECT has_table_privilege(#{quoted_table}, 'TRIGGER')") + row.first["has_table_privilege"] + rescue ActiveRecord::StatementInvalid + # Non-existing table + false + end + + def copy_column(table_name, old, new) + old_column = columns(table_name).find { |c| c.name == old.to_s } + + add_column(table_name, new, old_column.type, + limit: old_column.limit, + precision: old_column.precision, + scale: old_column.scale, + comment: old_column.comment + ) + + change_column_default(table_name, new, old_column.default) if old_column.default.present? + + value_arel = Arel::Table.new(table_name)[old] + backfill_column_safely(table_name, new, value_arel) + + add_null_constraint_safely(table_name, new) unless old_column.null + + copy_foreign_key(table_name, old, new) + copy_indexes(table_name, old, new) + end + + def copy_foreign_key(table_name, old, new) + fk = foreign_key_for(table_name, old) + return unless fk + + options = { + column: new, + primary_key: fk.primary_key, + on_delete: fk.on_delete, + on_update: fk.on_update + } + + if postgresql? + add_foreign_key_safely(table_name, fk.to_table, options) + else + add_foreign_key(table_name, fk.to_table, options) + end + end + + def foreign_key_for(table_name, column_name) + column_name = column_name.to_s + foreign_keys(table_name).find { |fk| fk.column == column_name } + end + + def copy_indexes(table_name, old, new) + old = old.to_s + new = new.to_s + + indexes = indexes(table_name).select { |index| index.columns.include?(old) } + + indexes.each do |index| + new_columns = index.columns.map do |column| + column == old ? new : column + end + + options = copy_index_options(index, old, new) + options[:algorithm] = :concurrently if postgresql? + add_index(table_name, new_columns, options) + end + end + + def copy_index_options(index, old, new) + unless index.name.include?(old) + raise StrongMigrations::Error, <<~ERROR + Cannot copy the index #{index.name} as it does not contain old column in its name. + Rename it manually before proceeding. + ERROR + end + + name = index.name.gsub(old, new) + + options = { + unique: index.unique, + name: name, + length: index.lengths, + order: index.orders, + where: index.where, + using: index.using, + } + + if ActiveRecord::VERSION::STRING >= "5.2" + options[:opclass] = index.opclasses + end + + options + end + + def create_column_rename_triggers(table_name, old, new) + trigger_name = rename_column_trigger_name(table_name, old, new) + quoted_table = connection.quote_table_name(table_name) + quoted_old = connection.quote_column_name(old) + quoted_new = connection.quote_column_name(new) + + if postgresql? + create_column_rename_triggers_postgresql(trigger_name, quoted_table, quoted_old, quoted_new) + else + create_column_rename_triggers_mysql(trigger_name, quoted_table, quoted_old, quoted_new) + end + end + + def rename_column_trigger_name(table_name, old, new) + "trigger_rails_" + Digest::SHA256.hexdigest("#{table_name}_#{old}_#{new}").first(10) + end + + def create_column_rename_triggers_postgresql(trigger, table, old, new) + execute <<~SQL + CREATE OR REPLACE FUNCTION #{trigger}() RETURNS trigger AS $$ + BEGIN + NEW.#{new} := NEW.#{old}; + RETURN NEW; + END; + $$ LANGUAGE 'plpgsql'; + SQL + + execute <<~SQL + DROP TRIGGER IF EXISTS #{trigger} ON #{table} + SQL + + execute <<~SQL + CREATE TRIGGER #{trigger} BEFORE INSERT OR UPDATE ON #{table} + FOR EACH ROW EXECUTE PROCEDURE #{trigger}(); + SQL + end + + def create_column_rename_triggers_mysql(trigger, table, old, new) + insert_trigger = "#{trigger}_before_insert" + update_trigger = "#{trigger}_before_update" + + execute <<~SQL + DROP TRIGGER IF EXISTS #{insert_trigger} + SQL + + execute <<~SQL + CREATE TRIGGER #{insert_trigger} BEFORE INSERT ON #{table} + FOR EACH ROW SET NEW.#{new} = NEW.#{old}; + SQL + + execute <<~SQL + DROP TRIGGER IF EXISTS #{update_trigger} + SQL + + execute <<~SQL + CREATE TRIGGER #{update_trigger} BEFORE UPDATE ON #{table} + FOR EACH ROW SET NEW.#{new} = NEW.#{old}; + SQL + end + + def remove_column_rename_triggers(table, trigger) + if postgresql? + remove_column_rename_triggers_postgresql(table, trigger) + else + remove_column_rename_triggers_mysql(trigger) + end + end + + def remove_column_rename_triggers_postgresql(table, trigger) + execute("DROP TRIGGER IF EXISTS #{trigger} ON #{table}") + execute("DROP FUNCTION IF EXISTS #{trigger}()") + end + + def remove_column_rename_triggers_mysql(trigger) + insert_trigger = "#{trigger}_before_insert" + update_trigger = "#{trigger}_before_update" + + execute("DROP TRIGGER IF EXISTS #{insert_trigger}") + execute("DROP TRIGGER IF EXISTS #{update_trigger}") + end end end diff --git a/lib/strong_migrations/util.rb b/lib/strong_migrations/util.rb index 4432c690..bfad6df2 100644 --- a/lib/strong_migrations/util.rb +++ b/lib/strong_migrations/util.rb @@ -8,6 +8,10 @@ def postgresql? %w(PostgreSQL PostGIS).include?(connection.adapter_name) end + def mysql? + connection.adapter_name == "Mysql2" + end + def postgresql_version @postgresql_version ||= begin target_version = StrongMigrations.target_postgresql_version diff --git a/test/migration_helpers_test.rb b/test/migration_helpers_test.rb index 0107d78a..3dc1fc39 100644 --- a/test/migration_helpers_test.rb +++ b/test/migration_helpers_test.rb @@ -24,6 +24,34 @@ def up end end +class AddMoneyToUsers < TestMigration + def change + safety_assured do + add_column :users, :money, :decimal, limit: 4, default: 100.0, precision: 10, scale: 3, comment: "Balance in $" + add_index :users, :money + end + end +end + +class RenameColumnSafely < TestMigration + def change + rename_column_safely :users, :money, :balance + end +end + +class RenameColumnSafelyForeignKey < TestMigration + def change + safety_assured { add_foreign_key :users, :orders } + rename_column_safely :users, :order_id, :new_order_id + end +end + +class RenameColumnSafelyCleanup < TestMigration + def change + rename_column_safely_cleanup :users, :money, :balance + end +end + class MigrationHelpersTest < Minitest::Test def test_add_foreign_key_safely skip unless postgresql? @@ -104,6 +132,70 @@ def test_backfill_column_safely User.delete_all end + def test_rename_column_safely_raises_inside_transaction + error = assert_raises(StrongMigrations::Error) { migrate_inside_transaction(RenameColumnSafely) } + assert_match "Cannot run `rename_column_safely` inside a transaction", error.message + end + + def test_rename_column_safely_copies_column + migrate(AddMoneyToUsers) + migrate(RenameColumnSafely) + columns = connection.columns("users") + old_column = columns.find { |c| c.name == "money" } + new_column = columns.find { |c| c.name == "balance" } + + assert_equal old_column.type, new_column.type + assert_equal old_column.limit, new_column.limit + assert_equal old_column.default, new_column.default + assert_equal old_column.precision, new_column.precision + assert_equal old_column.scale, new_column.scale + assert_equal old_column.comment, new_column.comment + ensure + migrate(RenameColumnSafely, direction: :down) + migrate(AddMoneyToUsers, direction: :down) + end + + def test_rename_column_safely_copies_foreign_keys + migrate(RenameColumnSafelyForeignKey) + assert connection.foreign_key_exists?(:users, column: :new_order_id) + ensure + migrate(RenameColumnSafelyForeignKey, direction: :down) + refute connection.foreign_key_exists?(:users, column: :new_order_id) + end + + def test_rename_column_safely_copies_column_indexes + migrate(AddMoneyToUsers) + migrate(RenameColumnSafely) + assert connection.index_exists?(:users, :balance) + ensure + migrate(RenameColumnSafely, direction: :down) + migrate(AddMoneyToUsers, direction: :down) + end + + def test_rename_column_safely_copies_data_to_new_column + User.reset_column_information + migrate(AddMoneyToUsers) + migrate(RenameColumnSafely) + user = User.create(name: "Dima", city: "Kyiv", money: 10.0) + assert_equal 10.0, user.reload.balance + ensure + User.delete_all + migrate(RenameColumnSafely, direction: :down) + migrate(AddMoneyToUsers, direction: :down) + end + + def test_rename_column_safely_cleanup_removes_old_column + migrate(AddMoneyToUsers) + migrate(RenameColumnSafely) + migrate(RenameColumnSafelyCleanup) + + assert_equal false, connection.column_exists?(:users, :money) + ensure + migrate(RenameColumnSafelyCleanup, direction: :down) + migrate(RenameColumnSafely, direction: :down) + migrate(AddMoneyToUsers, direction: :down) + end + private def connection From 41f2eb2a4726136b4e031c6b00581a61a734039a Mon Sep 17 00:00:00 2001 From: fatkodima Date: Sun, 29 Dec 2019 03:06:26 +0200 Subject: [PATCH 3/3] Add change_column_safely migration helper --- CHANGELOG.md | 1 + lib/strong_migrations.rb | 16 ++++- lib/strong_migrations/checker.rb | 24 ++++++-- lib/strong_migrations/migration_helpers.rb | 72 +++++++++++++++++----- test/migration_helpers_test.rb | 47 ++++++++++++++ test/strong_migrations_test.rb | 2 +- test/test_helper.rb | 1 + 7 files changed, 141 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1248a0b7..5a1b9e9b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ - Added `add_null_constraint_safely` and `remove_null_constraint_safely` methods - Added `add_column_safely` method - Added `backfill_column_safely` method +- Added `change_column_safely` method - Added `statement_timeout` and `lock_timeout` functionality ## 0.5.1 (2019-12-17) diff --git a/lib/strong_migrations.rb b/lib/strong_migrations.rb index b54cf18a..43e323f9 100644 --- a/lib/strong_migrations.rb +++ b/lib/strong_migrations.rb @@ -49,7 +49,21 @@ def change 3. Backfill data from the old column to the new column 4. Move reads from the old column to the new column 5. Stop writing to the old column -6. Drop the old column", +6. Drop the old column + +To achieve this, you can use: + +class %{migration_name} < ActiveRecord::Migration%{migration_suffix} + disable_ddl_transaction! + + def up + %{up_command} + end + + def down + %{down_command} + end +end", remove_column: "ActiveRecord caches attributes which causes problems when removing columns. Be sure to ignore the column%{column_suffix}: diff --git a/lib/strong_migrations/checker.rb b/lib/strong_migrations/checker.rb index 9d331a07..c5189997 100644 --- a/lib/strong_migrations/checker.rb +++ b/lib/strong_migrations/checker.rb @@ -91,15 +91,31 @@ def perform(method, *args) raise_error :add_column_json end when :change_column - table, column, type = args + table, column, type, options = args + options ||= {} safe = false + found_column = connection.columns(table).find { |c| c.name.to_s == column.to_s } + raise StrongMigrations::Error, "Column '#{column}' of relation '#{table}' does not exist" unless found_column + # assume Postgres 9.1+ since previous versions are EOL if postgresql? && type.to_s == "text" - found_column = connection.columns(table).find { |c| c.name.to_s == column.to_s } - safe = found_column && found_column.type == :string + safe = found_column.type == :string + end + + unless safe + down_options = {} + options.each do |option, value| + if value != found_column.send(option) + down_options[option] = found_column.send(option) + end + end + previous_type = found_column.type + + raise_error :change_column, + up_command: command_str("change_column_safely", [table, column, type, options]), + down_command: command_str("change_column_safely", [table, column, previous_type, down_options]) end - raise_error :change_column unless safe when :create_table table, options = args options ||= {} diff --git a/lib/strong_migrations/migration_helpers.rb b/lib/strong_migrations/migration_helpers.rb index a4a1858e..0292f06f 100644 --- a/lib/strong_migrations/migration_helpers.rb +++ b/lib/strong_migrations/migration_helpers.rb @@ -150,7 +150,7 @@ def backfill_column_safely(table_name, column_name, value, batch_size: 1000) end end - def rename_column_safely(table_name, old, new) + def rename_column_safely(table_name, old, new, options = {}) if !postgresql? && !mysql? raise StrongMigrations::Error, "`#{__method__}` is intended for Postgres and Mysql only" end @@ -160,7 +160,7 @@ def rename_column_safely(table_name, old, new) reversible do |dir| dir.up do - copy_column(table_name, old, new) + copy_column(table_name, old, new, options) safety_assured { create_column_rename_triggers(table_name, old, new) } end @@ -183,15 +183,20 @@ def rename_column_safely(table_name, old, new) end def rename_column_safely_cleanup(table_name, old, new) - ensure_not_in_transaction(__method__) + if !postgresql? && !mysql? + raise StrongMigrations::Error, "`#{__method__}` is intended for Postgres and Mysql only" + end + ensure_trigger_privileges(table_name) reversible do |dir| dir.up do trigger_name = rename_column_trigger_name(table_name, old, new) - safety_assured do - remove_column_rename_triggers(table_name, trigger_name) - remove_column(table_name, old) + transaction do + safety_assured do + remove_column_rename_triggers(table_name, trigger_name) + remove_column(table_name, old) + end end end @@ -202,6 +207,34 @@ def rename_column_safely_cleanup(table_name, old, new) end end + def change_column_safely(table_name, column_name, type, options = {}) + ensure_postgresql(__method__) + ensure_not_in_transaction(__method__) + ensure_trigger_privileges(table_name) + + reversible do |dir| + dir.up do + temp_column = "#{column_name}_for_type_change" + rename_column_safely(table_name, column_name, temp_column, type: type, **options) + + transaction do + rename_column_safely_cleanup(table_name, column_name, temp_column) + safety_assured { rename_column(table_name, temp_column, column_name) } + end + end + + dir.down do + # same error message as Active Record + raise ActiveRecord::IrreversibleMigration, <<~ERROR + This migration uses #{__method__}, which is not automatically reversible. + To make the migration reversible you can either: + 1. Define #up and #down methods in place of the #change method. + 2. Use the #reversible method to define reversible behavior. + ERROR + end + end + end + private def ensure_postgresql(method_name) @@ -259,27 +292,34 @@ def trigger_privileges_postgresql?(table_name) false end - def copy_column(table_name, old, new) + def copy_column(table_name, old, new, options = {}) old_column = columns(table_name).find { |c| c.name == old.to_s } + type, limit, default, null, precision, scale, collation, comment = copy_column_options(old_column, options) - add_column(table_name, new, old_column.type, - limit: old_column.limit, - precision: old_column.precision, - scale: old_column.scale, - comment: old_column.comment + add_column(table_name, new, type, + limit: limit, + precision: precision, + scale: scale, + comment: comment ) - change_column_default(table_name, new, old_column.default) if old_column.default.present? + change_column_default(table_name, new, default) if default.present? value_arel = Arel::Table.new(table_name)[old] backfill_column_safely(table_name, new, value_arel) - add_null_constraint_safely(table_name, new) unless old_column.null + add_null_constraint_safely(table_name, new) unless null copy_foreign_key(table_name, old, new) copy_indexes(table_name, old, new) end + def copy_column_options(column, new_options) + [:type, :limit, :default, :null, :precision, :scale, :collation, :comment].map do |option| + new_options.fetch(option, column.send(option)) + end + end + def copy_foreign_key(table_name, old, new) fk = foreign_key_for(table_name, old) return unless fk @@ -300,14 +340,14 @@ def copy_foreign_key(table_name, old, new) def foreign_key_for(table_name, column_name) column_name = column_name.to_s - foreign_keys(table_name).find { |fk| fk.column == column_name } + connection.foreign_keys(table_name).find { |fk| fk.column == column_name } end def copy_indexes(table_name, old, new) old = old.to_s new = new.to_s - indexes = indexes(table_name).select { |index| index.columns.include?(old) } + indexes = connection.indexes(table_name).select { |index| index.columns.include?(old) } indexes.each do |index| new_columns = index.columns.map do |column| diff --git a/test/migration_helpers_test.rb b/test/migration_helpers_test.rb index 3dc1fc39..a77fe22e 100644 --- a/test/migration_helpers_test.rb +++ b/test/migration_helpers_test.rb @@ -52,6 +52,22 @@ def change end end +class ChangeColumnSafely < TestMigration + def up + change_column_safely :users, :bio, :string, default: "No bio" + end + + def down + change_column_safely :users, :bio, :string, default: nil + end +end + +class ChangeColumnSafelyNonReversible < TestMigration + def change + change_column_safely :users, :bio, :string, default: "No bio" + end +end + class MigrationHelpersTest < Minitest::Test def test_add_foreign_key_safely skip unless postgresql? @@ -196,6 +212,32 @@ def test_rename_column_safely_cleanup_removes_old_column migrate(AddMoneyToUsers, direction: :down) end + def test_change_column_safely_raises_inside_transaction + skip unless postgresql? + error = assert_raises(StrongMigrations::Error) { migrate_inside_transaction(ChangeColumnSafely) } + assert_match "Cannot run `change_column_safely` inside a transaction", error.message + end + + def test_change_column_safely_is_not_reversible + skip unless postgresql? + migrate(ChangeColumnSafelyNonReversible) + + assert_raises(ActiveRecord::IrreversibleMigration) do + migrate(ChangeColumnSafelyNonReversible, direction: :down) + end + end + + def test_change_column_safely + skip unless postgresql? + migrate(ChangeColumnSafely) + column = column_for("users", "bio") + assert_equal "No bio", column.default + + migrate(ChangeColumnSafely, direction: :down) + column = column_for("users", "bio") + assert_nil column.default + end + private def connection @@ -208,4 +250,9 @@ def migrate_inside_transaction(migration) migrate(migration) end end + + def column_for(table, name) + name = name.to_s + connection.columns(table).find { |c| c.name == name } + end end diff --git a/test/strong_migrations_test.rb b/test/strong_migrations_test.rb index 7377a868..71313b54 100644 --- a/test/strong_migrations_test.rb +++ b/test/strong_migrations_test.rb @@ -73,7 +73,7 @@ def change class ChangeColumn < TestMigration def change - change_column :users, :properties, :bad_name + change_column :users, :name, :string, null: false end end diff --git a/test/test_helper.rb b/test/test_helper.rb index 4cd09806..661d6eb8 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -38,6 +38,7 @@ class CreateUsers < TestMigration def change create_table "users" do |t| t.string :name + t.string :bio t.string :city t.references :order end