From a703999c62aa9950920bbc621aa332da337c68ca Mon Sep 17 00:00:00 2001 From: schneems Date: Mon, 11 Jan 2016 10:49:53 -0600 Subject: Clean up duplicate migration logic --- activerecord/lib/active_record/migration.rb | 37 ++++++++++++++++------------- activerecord/test/cases/migration_test.rb | 2 +- 2 files changed, 21 insertions(+), 18 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index f699e83ab3..471c5dc05a 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -1165,33 +1165,21 @@ module ActiveRecord private + # Used for running a specific migration. def run_without_lock migration = migrations.detect { |m| m.version == @target_version } raise UnknownMigrationVersionError.new(@target_version) if migration.nil? - unless (up? && migrated.include?(migration.version.to_i)) || (down? && !migrated.include?(migration.version.to_i)) - begin - execute_migration_in_transaction(migration, @direction) - rescue => e - canceled_msg = use_transaction?(migration) ? ", this migration was canceled" : "" - raise StandardError, "An error has occurred#{canceled_msg}:\n\n#{e}", e.backtrace - end - end + execute_migration_in_transaction(migration, @direction) end + # Used for running multiple migrations up to or down to a certain value. def migrate_without_lock - if !target && @target_version && @target_version > 0 + if invalid_target? raise UnknownMigrationVersionError.new(@target_version) end runnable.each do |migration| - Base.logger.info "Migrating to #{migration.name} (#{migration.version})" if Base.logger - - begin - execute_migration_in_transaction(migration, @direction) - rescue => e - canceled_msg = use_transaction?(migration) ? "this and " : "" - raise StandardError, "An error has occurred, #{canceled_msg}all later migrations canceled:\n\n#{e}", e.backtrace - end + execute_migration_in_transaction(migration, @direction) end end @@ -1199,11 +1187,26 @@ module ActiveRecord migrated.include?(migration.version.to_i) end + # Return true if a valid version is not provided. + def invalid_target? + !target && @target_version && @target_version > 0 + end + def execute_migration_in_transaction(migration, direction) + return if down? && !migrated.include?(migration.version.to_i) + return if up? && migrated.include?(migration.version.to_i) + + Base.logger.info "Migrating to #{migration.name} (#{migration.version})" if Base.logger + ddl_transaction(migration) do migration.migrate(direction) record_version_state_after_migrating(migration.version) end + rescue => e + msg = "An error has occurred, " + msg << "this and " if use_transaction?(migration) + msg << "all later migrations canceled:\n\n#{e}" + raise StandardError, msg, e.backtrace end def target diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb index cfa223f93e..4d264caffb 100644 --- a/activerecord/test/cases/migration_test.rb +++ b/activerecord/test/cases/migration_test.rb @@ -301,7 +301,7 @@ class MigrationTest < ActiveRecord::TestCase e = assert_raise(StandardError) { migrator.run } - assert_equal "An error has occurred, this migration was canceled:\n\nSomething broke", e.message + assert_equal "An error has occurred, this and all later migrations canceled:\n\nSomething broke", e.message assert_no_column Person, :last_name, "On error, the Migrator should revert schema changes but it did not." -- cgit v1.2.3 From 4d60e93174a3d6d90b1a06fc7515cb5cd749a6f3 Mon Sep 17 00:00:00 2001 From: schneems Date: Mon, 11 Jan 2016 11:58:12 -0600 Subject: Set environment even when no migration runs This PR addresses the issue described in https://github.com/rails/rails/pull/22967#issuecomment-170251635. If the database is non empty and has no new migrations than `db:migrate` will not set the environment. This PR works by always setting the environment value on successful `up` migration regardless of whether or not a migration was actually executed. --- activerecord/lib/active_record/migration.rb | 11 ++++++++++- activerecord/test/cases/migration_test.rb | 27 +++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index 471c5dc05a..8dedd4c8a2 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -1170,6 +1170,8 @@ module ActiveRecord migration = migrations.detect { |m| m.version == @target_version } raise UnknownMigrationVersionError.new(@target_version) if migration.nil? execute_migration_in_transaction(migration, @direction) + + record_environment end # Used for running multiple migrations up to or down to a certain value. @@ -1181,6 +1183,14 @@ module ActiveRecord runnable.each do |migration| execute_migration_in_transaction(migration, @direction) end + + record_environment + end + + # Stores the current environment in the database. + def record_environment + return if down? + ActiveRecord::InternalMetadata[:environment] = ActiveRecord::Migrator.current_environment end def ran?(migration) @@ -1236,7 +1246,6 @@ module ActiveRecord else migrated << version ActiveRecord::SchemaMigration.create!(version: version.to_s) - ActiveRecord::InternalMetadata[:environment] = ActiveRecord::Migrator.current_environment end end diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb index 4d264caffb..fe74ca127f 100644 --- a/activerecord/test/cases/migration_test.rb +++ b/activerecord/test/cases/migration_test.rb @@ -398,6 +398,33 @@ class MigrationTest < ActiveRecord::TestCase ENV["RACK_ENV"] = original_rack_env end + + def test_migration_sets_internal_metadata_even_when_fully_migrated + current_env = ActiveRecord::ConnectionHandling::DEFAULT_ENV.call + migrations_path = MIGRATIONS_ROOT + "/valid" + old_path = ActiveRecord::Migrator.migrations_paths + ActiveRecord::Migrator.migrations_paths = migrations_path + + ActiveRecord::Migrator.up(migrations_path) + assert_equal current_env, ActiveRecord::InternalMetadata[:environment] + + original_rails_env = ENV["RAILS_ENV"] + original_rack_env = ENV["RACK_ENV"] + ENV["RAILS_ENV"] = ENV["RACK_ENV"] = "foofoo" + new_env = ActiveRecord::ConnectionHandling::DEFAULT_ENV.call + + refute_equal current_env, new_env + + sleep 1 # mysql by default does not store fractional seconds in the database + + ActiveRecord::Migrator.up(migrations_path) + assert_equal new_env, ActiveRecord::InternalMetadata[:environment] + ensure + ActiveRecord::Migrator.migrations_paths = old_path + ENV["RAILS_ENV"] = original_rails_env + ENV["RACK_ENV"] = original_rack_env + end + def test_proper_table_name_on_migration reminder_class = new_isolated_reminder_class migration = ActiveRecord::Migration.new -- cgit v1.2.3