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..a4a1858e 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,139 @@ 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.as("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 + + 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) 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." @@ -110,8 +235,196 @@ def on_delete_update_statement(delete_or_update, action) end end - def quote_identifiers(statement, identifiers) - statement % identifiers.map { |v| connection.quote_table_name(v) } + 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 new file mode 100644 index 00000000..bfad6df2 --- /dev/null +++ b/lib/strong_migrations/util.rb @@ -0,0 +1,32 @@ +module StrongMigrations + module Util + def connection + ActiveRecord::Base.connection + end + + 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 + 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..3dc1fc39 100644 --- a/test/migration_helpers_test.rb +++ b/test/migration_helpers_test.rb @@ -12,6 +12,46 @@ 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 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? @@ -50,6 +90,112 @@ 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 + + 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 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?