Move SchemaMigration to an independent object #45908
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eileencodes
force-pushed
the
redo-schema-migration
branch
2 times, most recently
from
664a46e
to
b58d56e
Compare
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.
eileencodes
force-pushed
the
redo-schema-migration
branch
from
b58d56e
to
436277d
Compare
9 tasks
eileencodes
added a commit
to eileencodes/rails
that referenced
this pull request
Sep 12, 2022
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..
This was referenced Sep 13, 2022
Merged
|
Doesn't this require a deprecation notice in 7.0 ? cause this bug when upgrading to 7.1 |
SchemaMigration has always been a private, internal class so we don't have to do a deprecation. The |
davinlagerroos
pushed a commit
to umn-asr/oracle-enhanced
that referenced
this pull request
Feb 21, 2024
After rails/rails#45908 we can no longer manipulate the ar_schema_migrations table via the ActiveRecord::SchemaMigration class, go through the default instance instead.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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::SchemaMigrationbecause theconnection is always on
Basein the rake tasks. This will free us upto do less hacky stuff in the migrations and tasks.
Note: once this is merged I'm going to do the same thing with InternalMetadata.