diff --git a/CHANGELOG.md b/CHANGELOG.md index b6304411..5a1b9e9b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ - 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 `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 334e2983..43e323f9 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 @@ -61,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 8d1afd73..c5189997 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,35 +83,39 @@ 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? 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 ||= {} @@ -186,10 +192,6 @@ def set_timeouts private - def connection - @migration.connection - end - def version @migration.version end @@ -202,22 +204,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 +224,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..0292f06f 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,172 @@ 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, options = {}) + 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, options) + 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) + 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) + transaction do + safety_assured do + remove_column_rename_triggers(table_name, trigger_name) + remove_column(table_name, old) + end + end + end + + dir.down do + copy_column(table_name, new, old) + safety_assured { create_column_rename_triggers(table_name, old, new) } + end + 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) 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 +268,203 @@ 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, 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, type, + limit: limit, + precision: precision, + scale: scale, + comment: comment + ) + + 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 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 + + 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 + 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 = connection.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..a77fe22e 100644 --- a/test/migration_helpers_test.rb +++ b/test/migration_helpers_test.rb @@ -12,6 +12,62 @@ 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 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? @@ -50,6 +106,138 @@ 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 + + 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 @@ -62,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 8482a6ac..71313b54 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 @@ -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 @@ -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..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 @@ -48,6 +49,9 @@ def change end migrate CreateUsers +class User < ActiveRecord::Base +end + class Minitest::Test def postgresql? ENV["ADAPTER"].nil?