Refactor migration to move migrations paths to connection

Rails has some support for multiple databases but it can be hard to
handle migrations with those. The easiest way to implement multiple
databases is to contain migrations into their own folder ("db/migrate"
for the primary db and "db/seconddb_migrate" for the second db). Without
this you would need to write code that allowed you to switch connections
in migrations. I can tell you from experience that is not a fun way to
implement multiple databases.

This refactoring is a pre-requisite for implementing other features
related to parallel testing and improved handling for multiple
databases.

The refactoring here moves the class methods from the `Migrator` class
into it's own new class `MigrationContext`. The goal was to move the
`migrations_paths` method off of the `Migrator` class and onto the
connection. This allows users to do the following in their
`database.yml`:

```
development:
  adapter: mysql2
  username: root
  password:

development_seconddb:
  adapter: mysql2
  username: root
  password:
  migrations_paths: "db/second_db_migrate"
```

Migrations for the `seconddb` can now be store in the
`db/second_db_migrate` directory. Migrations for the primary database
are stored in `db/migrate`".

The refactoring here drastically reduces the internal API for migrations
since we don't need to pass `migrations_paths` around to every single
method. Additionally this change does not require any Rails applications
to make changes unless they want to use the new public API. All of the
class methods from the `Migrator` class were `nodoc`'d except for the
`migrations_paths` and `migrations_path` getter/setters respectively.

1 files changed, 80 insertions, 68 deletions

diff --git a/activerecord/test/cases/migrator_test.rb b/activerecord/test/cases/migrator_test.rb
index 1047ba1367..d674e4506e 100644
--- a/activerecord/test/cases/migrator_test.rb
+++ b/activerecord/test/cases/migrator_test.rb
@@ -89,7 +89,7 @@ class MigratorTest < ActiveRecord::TestCase
   end
 
   def test_finds_migrations
-    migrations = ActiveRecord::Migrator.migrations(MIGRATIONS_ROOT + "/valid")
+    migrations = ActiveRecord::MigrationContext.new(MIGRATIONS_ROOT + "/valid").migrations
 
     [[1, "ValidPeopleHaveLastNames"], [2, "WeNeedReminders"], [3, "InnocentJointable"]].each_with_index do |pair, i|
       assert_equal migrations[i].version, pair.first
@@ -98,7 +98,8 @@ class MigratorTest < ActiveRecord::TestCase
   end
 
   def test_finds_migrations_in_subdirectories
-    migrations = ActiveRecord::Migrator.migrations(MIGRATIONS_ROOT + "/valid_with_subdirectories")
+    migrations = ActiveRecord::MigrationContext.new(MIGRATIONS_ROOT + "/valid_with_subdirectories").migrations
+
 
     [[1, "ValidPeopleHaveLastNames"], [2, "WeNeedReminders"], [3, "InnocentJointable"]].each_with_index do |pair, i|
       assert_equal migrations[i].version, pair.first
@@ -108,7 +109,7 @@ class MigratorTest < ActiveRecord::TestCase
 
   def test_finds_migrations_from_two_directories
     directories = [MIGRATIONS_ROOT + "/valid_with_timestamps", MIGRATIONS_ROOT + "/to_copy_with_timestamps"]
-    migrations = ActiveRecord::Migrator.migrations directories
+    migrations = ActiveRecord::MigrationContext.new(directories).migrations
 
     [[20090101010101, "PeopleHaveHobbies"],
      [20090101010202, "PeopleHaveDescriptions"],
@@ -121,14 +122,14 @@ class MigratorTest < ActiveRecord::TestCase
   end
 
   def test_finds_migrations_in_numbered_directory
-    migrations = ActiveRecord::Migrator.migrations [MIGRATIONS_ROOT + "/10_urban"]
+    migrations = ActiveRecord::MigrationContext.new(MIGRATIONS_ROOT + "/10_urban").migrations
     assert_equal 9, migrations[0].version
     assert_equal "AddExpressions", migrations[0].name
   end
 
   def test_relative_migrations
     list = Dir.chdir(MIGRATIONS_ROOT) do
-      ActiveRecord::Migrator.migrations("valid")
+      ActiveRecord::MigrationContext.new("valid").migrations
     end
 
     migration_proxy = list.find { |item|
@@ -157,7 +158,7 @@ class MigratorTest < ActiveRecord::TestCase
       ["up",   "002", "We need reminders"],
       ["down", "003", "Innocent jointable"],
       ["up",   "010", "********** NO FILE **********"],
-    ], ActiveRecord::Migrator.migrations_status(path)
+    ], ActiveRecord::MigrationContext.new(path).migrations_status
   end
 
   def test_migrations_status_in_subdirectories
@@ -171,24 +172,20 @@ class MigratorTest < ActiveRecord::TestCase
       ["up",   "002", "We need reminders"],
       ["down", "003", "Innocent jointable"],
       ["up",   "010", "********** NO FILE **********"],
-    ], ActiveRecord::Migrator.migrations_status(path)
+    ], ActiveRecord::MigrationContext.new(path).migrations_status
   end
 
   def test_migrations_status_with_schema_define_in_subdirectories
-    path = MIGRATIONS_ROOT + "/valid_with_subdirectories"
-    prev_paths = ActiveRecord::Migrator.migrations_paths
-    ActiveRecord::Migrator.migrations_paths = path
+    _, migrator = migrator_class(3)
+    migrator = migrator.new("valid_with_subdirectories")
 
-    ActiveRecord::Schema.define(version: 3) do
-    end
+    migrator.migrate
 
     assert_equal [
-      ["up", "001", "Valid people have last names"],
-      ["up", "002", "We need reminders"],
-      ["up", "003", "Innocent jointable"],
-    ], ActiveRecord::Migrator.migrations_status(path)
-  ensure
-    ActiveRecord::Migrator.migrations_paths = prev_paths
+      ["up", "001", "********** NO FILE **********"],
+      ["up", "002", "********** NO FILE **********"],
+      ["up", "003", "********** NO FILE **********"],
+    ], migrator.migrations_status
   end
 
   def test_migrations_status_from_two_directories
@@ -204,7 +201,7 @@ class MigratorTest < ActiveRecord::TestCase
       ["down", "20100201010101", "Valid with timestamps we need reminders"],
       ["down", "20100301010101", "Valid with timestamps innocent jointable"],
       ["up",   "20160528010101", "********** NO FILE **********"],
-    ], ActiveRecord::Migrator.migrations_status(paths)
+    ], ActiveRecord::MigrationContext.new(paths).migrations_status
   end
 
   def test_migrator_interleaved_migrations
@@ -232,25 +229,28 @@ class MigratorTest < ActiveRecord::TestCase
 
   def test_up_calls_up
     migrations = [Sensor.new(nil, 0), Sensor.new(nil, 1), Sensor.new(nil, 2)]
-    ActiveRecord::Migrator.new(:up, migrations).migrate
+    migrator = ActiveRecord::Migrator.new(:up, migrations)
+    migrator.migrate
     assert migrations.all?(&:went_up)
     assert migrations.all? { |m| !m.went_down }
-    assert_equal 2, ActiveRecord::Migrator.current_version
+    assert_equal 2, migrator.current_version
   end
 
   def test_down_calls_down
     test_up_calls_up
 
     migrations = [Sensor.new(nil, 0), Sensor.new(nil, 1), Sensor.new(nil, 2)]
-    ActiveRecord::Migrator.new(:down, migrations).migrate
+    migrator = ActiveRecord::Migrator.new(:down, migrations)
+    migrator.migrate
     assert migrations.all? { |m| !m.went_up }
     assert migrations.all?(&:went_down)
-    assert_equal 0, ActiveRecord::Migrator.current_version
+    assert_equal 0, migrator.current_version
   end
 
   def test_current_version
     ActiveRecord::SchemaMigration.create!(version: "1000")
-    assert_equal 1000, ActiveRecord::Migrator.current_version
+    migrator = ActiveRecord::MigrationContext.new("db/migrate")
+    assert_equal 1000, migrator.current_version
   end
 
   def test_migrator_one_up
@@ -289,33 +289,36 @@ class MigratorTest < ActiveRecord::TestCase
 
   def test_migrator_double_up
     calls, migrations = sensors(3)
-    assert_equal(0, ActiveRecord::Migrator.current_version)
+    migrator = ActiveRecord::Migrator.new(:up, migrations, 1)
+    assert_equal(0, migrator.current_version)
 
-    ActiveRecord::Migrator.new(:up, migrations, 1).migrate
+    migrator.migrate
     assert_equal [[:up, 1]], calls
     calls.clear
 
-    ActiveRecord::Migrator.new(:up, migrations, 1).migrate
+    migrator.migrate
     assert_equal [], calls
   end
 
   def test_migrator_double_down
     calls, migrations = sensors(3)
+    migrator = ActiveRecord::Migrator.new(:up, migrations, 1)
 
-    assert_equal(0, ActiveRecord::Migrator.current_version)
+    assert_equal 0, migrator.current_version
 
-    ActiveRecord::Migrator.new(:up, migrations, 1).run
+    migrator.run
     assert_equal [[:up, 1]], calls
     calls.clear
 
-    ActiveRecord::Migrator.new(:down, migrations, 1).run
+    migrator = ActiveRecord::Migrator.new(:down, migrations, 1)
+    migrator.run
     assert_equal [[:down, 1]], calls
     calls.clear
 
-    ActiveRecord::Migrator.new(:down, migrations, 1).run
+    migrator.run
     assert_equal [], calls
 
-    assert_equal(0, ActiveRecord::Migrator.current_version)
+    assert_equal 0, migrator.current_version
   end
 
   def test_migrator_verbosity
@@ -361,78 +364,85 @@ class MigratorTest < ActiveRecord::TestCase
 
   def test_migrator_going_down_due_to_version_target
     calls, migrator = migrator_class(3)
+    migrator = migrator.new("valid")
 
-    migrator.up("valid", 1)
+    migrator.up(1)
     assert_equal [[:up, 1]], calls
     calls.clear
 
-    migrator.migrate("valid", 0)
+    migrator.migrate(0)
     assert_equal [[:down, 1]], calls
     calls.clear
 
-    migrator.migrate("valid")
+    migrator.migrate
     assert_equal [[:up, 1], [:up, 2], [:up, 3]], calls
   end
 
   def test_migrator_output_when_running_multiple_migrations
     _, migrator = migrator_class(3)
+    migrator = migrator.new("valid")
 
-    result = migrator.migrate("valid")
+    result = migrator.migrate
     assert_equal(3, result.count)
 
     # Nothing migrated from duplicate run
-    result = migrator.migrate("valid")
+    result = migrator.migrate
     assert_equal(0, result.count)
 
-    result = migrator.rollback("valid")
+    result = migrator.rollback
     assert_equal(1, result.count)
   end
 
   def test_migrator_output_when_running_single_migration
     _, migrator = migrator_class(1)
-    result = migrator.run(:up, "valid", 1)
+    migrator = migrator.new("valid")
+
+    result = migrator.run(:up, 1)
 
     assert_equal(1, result.version)
   end
 
   def test_migrator_rollback
     _, migrator = migrator_class(3)
+    migrator = migrator.new("valid")
 
-    migrator.migrate("valid")
-    assert_equal(3, ActiveRecord::Migrator.current_version)
+    migrator.migrate
+    assert_equal(3, migrator.current_version)
 
-    migrator.rollback("valid")
-    assert_equal(2, ActiveRecord::Migrator.current_version)
+    migrator.rollback
+    assert_equal(2, migrator.current_version)
 
-    migrator.rollback("valid")
-    assert_equal(1, ActiveRecord::Migrator.current_version)
+    migrator.rollback
+    assert_equal(1, migrator.current_version)
 
-    migrator.rollback("valid")
-    assert_equal(0, ActiveRecord::Migrator.current_version)
+    migrator.rollback
+    assert_equal(0, migrator.current_version)
 
-    migrator.rollback("valid")
-    assert_equal(0, ActiveRecord::Migrator.current_version)
+    migrator.rollback
+    assert_equal(0, migrator.current_version)
   end
 
   def test_migrator_db_has_no_schema_migrations_table
     _, migrator = migrator_class(3)
+    migrator = migrator.new("valid")
 
     ActiveRecord::Base.connection.drop_table "schema_migrations", if_exists: true
     assert_not ActiveRecord::Base.connection.table_exists?("schema_migrations")
-    migrator.migrate("valid", 1)
+    migrator.migrate(1)
     assert ActiveRecord::Base.connection.table_exists?("schema_migrations")
   end
 
   def test_migrator_forward
     _, migrator = migrator_class(3)
-    migrator.migrate("/valid", 1)
-    assert_equal(1, ActiveRecord::Migrator.current_version)
+    migrator = migrator.new("/valid")
+    migrator.migrate(1)
+    assert_equal(1, migrator.current_version)
 
-    migrator.forward("/valid", 2)
-    assert_equal(3, ActiveRecord::Migrator.current_version)
+    migrator.forward(2)
+    assert_equal(3, migrator.current_version)
 
-    migrator.forward("/valid")
-    assert_equal(3, ActiveRecord::Migrator.current_version)
+    migrator.forward
+    assert_equal(3, migrator.current_version)
   end
 
   def test_only_loads_pending_migrations
@@ -440,25 +450,27 @@ class MigratorTest < ActiveRecord::TestCase
     ActiveRecord::SchemaMigration.create!(version: "1")
 
     calls, migrator = migrator_class(3)
-    migrator.migrate("valid", nil)
+    migrator = migrator.new("valid")
+    migrator.migrate
 
     assert_equal [[:up, 2], [:up, 3]], calls
   end
 
   def test_get_all_versions
     _, migrator = migrator_class(3)
+    migrator = migrator.new("valid")
 
-    migrator.migrate("valid")
-    assert_equal([1, 2, 3], ActiveRecord::Migrator.get_all_versions)
+    migrator.migrate
+    assert_equal([1, 2, 3], migrator.get_all_versions)
 
-    migrator.rollback("valid")
-    assert_equal([1, 2], ActiveRecord::Migrator.get_all_versions)
+    migrator.rollback
+    assert_equal([1, 2], migrator.get_all_versions)
 
-    migrator.rollback("valid")
-    assert_equal([1], ActiveRecord::Migrator.get_all_versions)
+    migrator.rollback
+    assert_equal([1], migrator.get_all_versions)
 
-    migrator.rollback("valid")
-    assert_equal([], ActiveRecord::Migrator.get_all_versions)
+    migrator.rollback
+    assert_equal([], migrator.get_all_versions)
   end
 
   private
@@ -483,11 +495,11 @@ class MigratorTest < ActiveRecord::TestCase
     def migrator_class(count)
       calls, migrations = sensors(count)
 
-      migrator = Class.new(ActiveRecord::Migrator).extend(Module.new {
-        define_method(:migrations) { |paths|
+      migrator = Class.new(ActiveRecord::MigrationContext) {
+        define_method(:migrations) { |*|
           migrations
         }
-      })
+      }
       [calls, migrator]
     end
 end