Move SchemaMigration to migration_context #36439

eileencodes · 2019-06-07T15:47:59Z

This PR moves the schema_migration to migration_context so that we
can access the schema_migration per connection.

This does not change behavior of the SchemaMigration if you are using
one database. This also does not change behavior of any public APIs.
Migrator is private as is MigrationContext so we can change these as
needed.

We now need to pass a schema_migration to Migrator so that we can
run migrations on the right connection outside the context of a rake
task.

The bugs this fixes were discovered while debugging the issues around
the SchemaCache on initialization with multiple database. It was clear
that get_all_versions wouldn't work without these changes outside the
context of a rake task (because in the rake task we establish a
connection and change AR::Base.connection to the db we're running on).

Because the SchemaCache relies on the SchemaMigration information we
need to make sure we store it per-connection rather than on
ActiveRecord::Base.

cc/ @tenderlove, @rafaelfranca, @jhawthorn, @matthewd

eileencodes · 2019-06-07T15:48:18Z

activerecord/lib/active_record/connection_adapters/abstract_adapter.rb

+
+                                  define_singleton_method(:connection) { conn }
+                                end
+                              end


This was @tenderlove's idea 😉

kamipo · 2019-06-10T12:03:11Z

activerecord/lib/active_record/connection_adapters/abstract_adapter.rb

+        MigrationContext.new(migrations_paths, schema_migration)
+      end
+
+      def schema_migration


Is this intended to be exposed in the api doc?

If the class was not, I think we shouldn't make the method public.

kamipo · 2019-06-10T12:05:56Z

activerecord/lib/active_record/connection_adapters/abstract_adapter.rb

+        @schema_migration ||= begin
+                                conn = self
+                                Class.new(ActiveRecord::SchemaMigration) do
+                                  self.table_name = "schema_migrations"


Is this necessary? Actually the SchemaMigration super class is not abstract class.

kamipo · 2019-06-10T12:07:14Z

activerecord/lib/active_record/migration.rb

@@ -885,13 +885,14 @@ def method_missing(method, *arguments, &block)

    def copy(destination, sources, options = {})
      copied = []
+      schema_migration = options[:schema_migration] || ActiveRecord::Base.connection.schema_migration


Is this options[:schema_migration] will be used in the future? It is not used yet in this PR.

activerecord/lib/active_record/migration.rb

kamipo · 2019-06-10T12:24:53Z

activerecord/test/cases/migration_test.rb

@@ -84,7 +85,7 @@ def test_migration_version_matches_component_version

  def test_migrator_versions
    migrations_path = MIGRATIONS_ROOT + "/valid"
-    migrator = ActiveRecord::MigrationContext.new(migrations_path)
+    migrator = ActiveRecord::MigrationContext.new(migrations_path, ActiveRecord::Base.connection.schema_migration)


Is this intentionallly not used @schema_migration here?

rafaelfranca · 2019-06-13T16:32:20Z

activerecord/lib/active_record/connection_adapters/abstract_adapter.rb

+        MigrationContext.new(migrations_paths, schema_migration)
+      end
+
+      def schema_migration


If the class was not, I think we shouldn't make the method public.

activerecord/lib/active_record/migration.rb

kamipo · 2019-06-13T19:40:52Z

activerecord/lib/active_record/migration.rb

+      schema_migration.normalize_migration_number(version)
+    end
+
+    def create_schema_migration_table


Are these internal methods should be public?
create_schema_migration_table is not used yet, and others are not used from public.

9ef5b7e...remove-internal-methods-in-migration-context

From 27cf3da4c0ede28302bbb4e27fa53d1aadb6bc7d Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono <kamipo@gmail.com> Date: Mon, 10 Jun 2019 20:34:03 +0900 Subject: [PATCH] Remove internal methods which are not used from public --- .../connection_adapters/abstract_adapter.rb | 2 +- activerecord/lib/active_record/migration.rb | 30 ++++--------------- 2 files changed, 6 insertions(+), 26 deletions(-) diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb index 3a08900f3d07..3d0993dfd7e1 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb @@ -166,7 +166,7 @@ def migration_context # :nodoc: MigrationContext.new(migrations_paths, schema_migration) end - def schema_migration + def schema_migration # :nodoc: @schema_migration ||= begin conn = self spec_name = conn.pool.spec.name diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index 0256d2b710f9..aa2e7f95aec0 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -885,7 +885,7 @@ def method_missing(method, *arguments, &block) def copy(destination, sources, options = {}) copied = [] - schema_migration = options[:schema_migration] + schema_migration = ActiveRecord::Base.connection.schema_migration FileUtils.mkdir_p(destination) unless File.exist?(destination) @@ -1022,26 +1022,6 @@ def initialize(migrations_paths, schema_migration) @schema_migration = schema_migration end - def table_exists? - schema_migration.table_exists? - end - - def all_versions - schema_migration.all_versions.map(&:to_i) - end - - def normalized_versions - schema_migration.normalized_versions - end - - def normalize_migration_number(version) - schema_migration.normalize_migration_number(version) - end - - def create_schema_migration_table - schema_migration.create_table - end - def migrate(target_version = nil, &block) case when target_version.nil? @@ -1092,8 +1072,8 @@ def open end def get_all_versions - if table_exists? - all_versions + if schema_migration.table_exists? + schema_migration.all_versions.map(&:to_i) else [] end @@ -1130,12 +1110,12 @@ def migrations end def migrations_status - db_list = normalized_versions + db_list = schema_migration.normalized_versions file_list = migration_files.map do |file| version, name, scope = parse_migration_filename(file) raise IllegalMigrationNameError.new(file) unless version - version = normalize_migration_number(version) + version = schema_migration.normalize_migration_number(version) status = db_list.delete(version) ? "up" : "down" [status, version, (name + scope).humanize] end.compact

The entire class is nodoc'd.

Took the majority of suggested changes and amended my original commit.

eileencodes · 2019-06-14T14:53:12Z

Ok - I think this is ready now.

This PR moves the `schema_migration` to `migration_context` so that we can access the `schema_migration` per connection. This does not change behavior of the SchemaMigration if you are using one database. This also does not change behavior of any public APIs. `Migrator` is private as is `MigrationContext` so we can change these as needed. We now need to pass a `schema_migration` to `Migrator` so that we can run migrations on the right connection outside the context of a rake task. The bugs this fixes were discovered while debugging the issues around the SchemaCache on initialization with multiple database. It was clear that `get_all_versions` wouldn't work without these changes outside the context of a rake task (because in the rake task we establish a connection and change AR::Base.connection to the db we're running on). Because the `SchemaCache` relies on the `SchemaMigration` information we need to make sure we store it per-connection rather than on ActiveRecord::Base. [Eileen M. Uchitelle & Aaron Patterson]

…igration-context Move SchemaMigration to migration_context

rafaelfranca · 2019-06-14T18:51:58Z

👍

kamipo · 2019-06-14T20:29:34Z

activerecord/lib/active_record/migration.rb

@@ -884,13 +884,14 @@ def method_missing(method, *arguments, &block)

    def copy(destination, sources, options = {})
      copied = []
+      schema_migration = options[:schema_migration] || ActiveRecord::SchemaMigration


I'm still don't get about this :schema_migration option.
This option is not used yet in this PR, so any test is not failed even if this option is removed.
Is this option used in GitHub?
If so, should we have a test case for this option?

diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index 9db017cded..a6dd135b5f 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -884,7 +884,7 @@ def method_missing(method, *arguments, &block) def copy(destination, sources, options = {}) copied = [] - schema_migration = options[:schema_migration] || ActiveRecord::SchemaMigration + schema_migration = ActiveRecord::SchemaMigration FileUtils.mkdir_p(destination) unless File.exist?(destination)

I will add a test for it.

Refer rails/rails#36439 https://github.com/rails/rails/blob/aae270de9e0862f31b14642908472d235a17936f/activerecord/lib/active_record/schema_migration.rb#L47-L49 Here `all_versions` returns version numbers as an array of Integer, while Oracle enhanced adapter expects them as an array of String. ```ruby def all_versions order(:version).pluck(:version).map(&:to_i) end ``` Fix rsim#1893

I think we should change this, but not in 6-0-stable since that's already in RC and I was trying to only make changes that won't require any app changes. This reverts a portion of rails#36439 that made all schema migration version numbers get dumped as an integer. While it doesn't _really_ matter it did change behavior. We should bring this back in 6.1 with a deprecation.

This commit in Rails: rails/rails@7cc27d7 Updated the ActiveRecord::MigrationContext.new arguments to also include a second argument that passed the base class(?) for all migrations. Originally, this commit was coded to basically use the default value from the `ActiveRecord::Migration#copy` method: https://github.com/rails/rails/blob/7cc27d749c3563e6b278ad01d233cb92ea3b7935/activerecord/lib/active_record/migration.rb#L887 schema_migration = options[:schema_migration] || ActiveRecord::SchemaMigration Which seemed to cause trouble when running the specs. Using the author of PaperTrail's comment as inspiration: rails/rails#36439 (comment) Making use of ::ActiveRecord::Base.connection.schema_migration seems to work as a good second arg for our use cases.

The schema_dumper_extension_test.rb starts failing after this Rails PR: rails/rails#36439 I think the fix is to execute the migration code inside a block that doesn't allow switching between databases. When we run a migration, we should only run it against *one* database.

This PR is similar to rails#36439 which moved schema migrations to the connection and migration context. This PR does the same thing for the internal metadata class. Previously the metadata class was only accessible on Base which makes it a little weird when switching connections. This didn't really cause any issues but I noticed it when working on a PR to change how connections work in the database rake tasks. In a multiple database environment it makes sense to access the `ar_internal_metadata` on the existing connection rather than on `Base.connection`. I previously didn't implement this because I felt we wanted only one place to ask for the metadata information but recently realized that we were still recording metadata on all the databases, so we might as well record it correctly. Applications should notice no change in behavior because this is only accessed through the rake tasks. Additionally behavior between schema migrations and internal metadata are now more similar. Eventually I'd like neither of these to inherit from or rely on Base, but we have a lot more work to do before that's possible.

Previously, SchemaMigration inherited from ActiveRecord::Base. This is problematic for multiple databases and resulted in building the code in AbstractAdapter that was previously there. Rather than hacking around the fact that SchemaMigration inherits from Base, this PR makes SchemaMigration an independent object. Then each connection can get it's own SchemaMigration object. This change required defining the methods that SchemaMigration was depending on ActiveRecord::Base for (ex create!). I reimplemented only the methods called by the framework as this class is no-doc's so it doesn't need to implement anything beyond that. Now each connection gets it's own SchemaMigration object which stores the connection. I also decided to update the method names (create -> create_version, delete_by -> delete_version, delete_all -> delete_all_versions) to be more explicit. This change also required adding a NullSchemaMigraiton class for cases when we don't have a connection yet but still need to copy migrations from the MigrationContext. Ultimately I think this is a little weird - we need to do so much work to pick up a set of files? Maybe something to explore in the future. Aside from removing the hack we added back in #36439 this change will enable my work to stop clobbering and depending directly on Base.connection in the rake tasks. While working on this I discovered that we always have a `ActiveRecord::SchemaMigration` because the connection is always on `Base` in the rake tasks. This will free us up to do less hacky stuff in the migrations and tasks.

Previously, SchemaMigration inherited from ActiveRecord::Base. This is problematic for multiple databases and resulted in building the code in AbstractAdapter that was previously there. Rather than hacking around the fact that SchemaMigration inherits from Base, this PR makes SchemaMigration an independent object. Then each connection can get it's own SchemaMigration object. This change required defining the methods that SchemaMigration was depending on ActiveRecord::Base for (ex create!). I reimplemented only the methods called by the framework as this class is no-doc's so it doesn't need to implement anything beyond that. Now each connection gets it's own SchemaMigration object which stores the connection. I also decided to update the method names (create -> create_version, delete_by -> delete_version, delete_all -> delete_all_versions) to be more explicit. This change also required adding a NullSchemaMigraiton class for cases when we don't have a connection yet but still need to copy migrations from the MigrationContext. Ultimately I think this is a little weird - we need to do so much work to pick up a set of files? Maybe something to explore in the future. Aside from removing the hack we added back in rails#36439 this change will enable my work to stop clobbering and depending directly on Base.connection in the rake tasks. While working on this I discovered that we always have a `ActiveRecord::SchemaMigration` because the connection is always on `Base` in the rake tasks. This will free us up to do less hacky stuff in the migrations and tasks.

Previously, SchemaMigration inherited from ActiveRecord::Base. This is problematic for multiple databases and resulted in building the code in AbstractAdapter that was previously there. Rather than hacking around the fact that SchemaMigration inherits from Base, this PR makes SchemaMigration an independent object. Then each connection can get it's own SchemaMigration object. This change required defining the methods that SchemaMigration was depending on ActiveRecord::Base for (ex create!). I reimplemented only the methods called by the framework as this class is no-doc's so it doesn't need to implement anything beyond that. Now each connection gets it's own SchemaMigration object which stores the connection. I also decided to update the method names (create -> create_version, delete_by -> delete_version, delete_all -> delete_all_versions) to be more explicit. This change also required adding a NullSchemaMigraiton class for cases when we don't have a connection yet but still need to copy migrations from the MigrationContext. Ultimately I think this is a little weird - we need to do so much work to pick up a set of files? Maybe something to explore in the future. Aside from removing the hack we added back in #36439 this change will enable my work to stop clobbering and depending directly on Base.connection in the rake tasks. While working on this I discovered that we always have a `ActiveRecord::SchemaMigration` because the connection is always on `Base` in the rake tasks. This will free us up to do less hacky stuff in the migrations and tasks.

Followup to rails#45908 to match the same behavior as SchemaMigration Previously, InternalMetadata inherited from ActiveRecord::Base. This is problematic for multiple databases and resulted in building the code in AbstractAdapter that was previously there. Rather than hacking around the fact that InternalMetadata inherits from Base, this PR makes InternalMetadata an independent object. Then each connection can get it's own InternalMetadata object. This change required defining the methods that InternalMetadata was depending on ActiveRecord::Base for (ex create!). I reimplemented only the methods called by the framework as this class is no-doc's so it doesn't need to implement anything beyond that. Now each connection gets it's own InternalMetadata object which stores the connection. This change also required adding a NullInternalMetadata class for cases when we don't have a connection yet but still need to copy migrations from the MigrationContext. Ultimately I think this is a little weird - we need to do so much work to pick up a set of files? Maybe something to explore in the future. Aside from removing the hack we added back in rails#36439 this change will enable my work to stop clobbering and depending directly on Base.connection in the rake tasks. While working on this I discovered that we always have a ActiveRecord::InternalMetadata because the connection is always on Base in the rake tasks. This will free us up to do less hacky stuff in the migrations and tasks. Both schema migration and internal metadata are blockers to removing `Base.connection` and `Base.establish_connection` from rake tasks, work that is required to drop the reliance on `Base.connection` which will enable more robust (and correct) sharding behavior in Rails..

eileencodes added this to the 6.0.0 milestone Jun 7, 2019

rails-bot bot added the activerecord label Jun 7, 2019

eileencodes commented Jun 7, 2019

View reviewed changes

eileencodes added the needs backport label Jun 7, 2019

eileencodes force-pushed the move-schema-migration-to-migration-context branch 2 times, most recently from efa1723 to 90107be Compare June 7, 2019 16:12

kamipo reviewed Jun 10, 2019

View reviewed changes

eileencodes force-pushed the move-schema-migration-to-migration-context branch from 90107be to 766beef Compare June 11, 2019 18:32

rails-bot bot added the railties label Jun 11, 2019

eileencodes force-pushed the move-schema-migration-to-migration-context branch from 766beef to 9ef5b7e Compare June 11, 2019 20:26

rafaelfranca reviewed Jun 13, 2019

View reviewed changes

kamipo reviewed Jun 13, 2019

View reviewed changes

eileencodes force-pushed the move-schema-migration-to-migration-context branch 5 times, most recently from fb532cf to f9deb37 Compare June 14, 2019 14:47

eileencodes force-pushed the move-schema-migration-to-migration-context branch from f9deb37 to 7cc27d7 Compare June 14, 2019 15:15

eileencodes merged commit f813119 into rails:master Jun 14, 2019

eileencodes added a commit that referenced this pull request Jun 14, 2019

Merge pull request #36439 from eileencodes/move-schema-migration-to-m…

e94dc52

…igration-context Move SchemaMigration to migration_context

eileencodes mentioned this pull request Jun 14, 2019

Update multi-db docs #36488

Merged

kamipo reviewed Jun 14, 2019

View reviewed changes

eileencodes deleted the move-schema-migration-to-migration-context branch June 14, 2019 20:32

yahonda mentioned this pull request Jun 18, 2019

OracleEnhancedAdapter structure dump schema migrations multi insert is supported should dump schema migrations using multi inserts failure rsim/oracle-enhanced#1893

Closed

yahonda mentioned this pull request Jun 19, 2019

Convert version numbers from Integer to String rsim/oracle-enhanced#1896

Closed

j15e mentioned this pull request Apr 29, 2021

Update test_helper for Rails 6+ compatibility derrickreimer/sequenced#49

Closed

bquorning mentioned this pull request Jun 30, 2021

Fix schema loading zendesk/active_record_shards#254

Merged

uki1014 mentioned this pull request Oct 8, 2021

Bump up version for rails 6.0 and Fix ::ActiveRecord::MigrationContext argument waka/seed-snapshot#15

Merged

eileencodes mentioned this pull request Aug 19, 2022

Move InternalMetadata to migration context #45862

Merged

eileencodes mentioned this pull request Aug 30, 2022

Move SchemaMigration to an independent object #45908

Merged

bootleq mentioned this pull request Sep 8, 2022

Fix can't initialize with Rails 6 bootleq/migrant#2

Merged

eileencodes mentioned this pull request Sep 9, 2022

Move InternalMetadata to an independent object #45982

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move SchemaMigration to migration_context #36439

Move SchemaMigration to migration_context #36439

eileencodes commented Jun 7, 2019

eileencodes Jun 7, 2019

kamipo Jun 10, 2019

rafaelfranca Jun 13, 2019

eileencodes Jun 14, 2019

kamipo Jun 10, 2019

eileencodes Jun 14, 2019

kamipo Jun 10, 2019

kamipo Jun 10, 2019

rafaelfranca Jun 13, 2019

kamipo Jun 13, 2019

eileencodes Jun 14, 2019

eileencodes Jun 14, 2019

eileencodes commented Jun 14, 2019

rafaelfranca commented Jun 14, 2019

kamipo Jun 14, 2019

eileencodes Jun 14, 2019

Move SchemaMigration to migration_context #36439

Move SchemaMigration to migration_context #36439

Conversation

eileencodes commented Jun 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileencodes commented Jun 14, 2019

rafaelfranca commented Jun 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment