Closed postmodern closed 1 year ago
I didn't remove ActiveRecord::SchemaMigration.table_name_prefix
, I removed the inheritance of ActiveRecord::Base
from SchemaMigration
so that each connection can have its own schema migration object without hacks. Since it's no longer inheriting from Base
you can't call methods that were defined on Base
. You can simply set the Base.table_name_prefix
instead now. It has the same effect.
I explained why it was necessary and why there wasn't a deprecation here https://github.com/rails/rails/pull/45908/.
I'm not familiar with ronin but it looks like the schema migration class is only used to set the table prefix, so I'd drop that class in favor of initializing a SchemaMigration
object with a connection in the MigrationContext
and let Active Record handle the rest. Other than prefix, I don't see a reason ronin needs it's own special Schema Migration class.
Edit: I don't think you need any of the MigrationContext
either, you can set the migrations_paths
in the database.yml
and Active Record will pick it up. Alt option is to set it directly with the ActiveRecord::Migrator.migrations_paths=
setter (which is also private but still better than overwriting MigrationContext
.
@eileencodes expanding on what's already in the README, ronin-db-activerecord
is a ruby library containing ActiveRecord models. It is meant to be used by both other rubygems (such as ronin-db), other web applications (ex: ronin-app), or even other 3rd-party Rails applications. The purpose of setting a table-name prefix in SchemaMigration
is to separate the ronin-db
migrations from any other app's own migrations.
Setting ActiveRecord::Base.table_name_prefix
would set the table name prefix globally which is not ideal; maybe someone wants to use ronin-db-activerecord
in some other app or some other Ruby library with it's own additional ActiveRecord models. The Ronin::DB::
models should have their own table name prefix separate from the app's own namespace or database table name prefix. So it seemed logical to also give the schema_migrations
table it's own prefix name. If Ronin::DB::Migrations
cannot access ronin_schema_migrations
, but instead uses schema_migrations
, then it will erroneously re-run previously ran migrations against any existing databases.
I'm not familiar with ronin but it looks like the schema migration class is only used to set the table prefix, so I'd drop that class in favor of initializing a
SchemaMigration
object with a connection in theMigrationContext
and let Active Record handle the rest. Other than prefix, I don't see a reason ronin needs it's own special Schema Migration class.Edit: I don't think you need any of the
MigrationContext
either, you can set themigrations_paths
in thedatabase.yml
and Active Record will pick it up. Alt option is to set it directly with theActiveRecord::Migrator.migrations_paths=
setter (which is also private but still better than overwritingMigrationContext
.
@eileencodes Do you feel comfortable trying to submit a PR for this? The other reason why I sub-classed ActiveRecord::MigrationContext
is to pass in my own internal db/migrate/
path, and the SchemaMigration
class which sets a custom ronin_
table name prefix which separates ronin-db-activerecord
migrations from any other app migrations. ActiveRecord::MigrationContext#initialize
expects 1..3 arguments. I need a ActiveRecord::MigrationContext
object for the context
method so I can run the migrations in db/migrate/
. If I initialize ActiveRecord::MigrationContext
directly, what arguments should I give it?
The code which actually calls Ronin::DB::Migrations
lives in ronin-db
, and allows running migrations after calling ActiveRecord::Base.establish_connection
or if the database is brand new. Note that ronin-db
and ronin-app
do not have a classical database.yml
file, but manages a ~/.config/ronin-db/databases.ym
file which may contain multiple database configurations and allows connecting to them individually by name (ex: ronin-db add foo sqlite3://path/to/foo.sqlite3
and ronin-db ips --db foo
).
I wish I could simply add the db/migrate/
path to ActiveRecord::Migrator.migrations_paths
and call ActiveRecord::Migrator.up!
or something similar. I understand that ActiveRecord
is tailored for Rails apps, but it seems like it could be improved to support using it in libraries or Ruby CLI apps, where the models and migrations might live in one or more other gems and the user specifies/controls the database configuration. It doesn't seem like ActiveRecord
has a public API for running the migrations, so I had to get creative and tried using the private API.
Setting ActiveRecord::Base.table_name_prefix would set the table name prefix globally which is not ideal
I would accept a PR to Rails to add a specific table prefix or custom name to SchemaMigration. Not sure if I'll have time to prioritize this soon.
Do you feel comfortable trying to submit a PR for this?
I honestly don't have the bandwidth to work on this, I was trying to help guide you a bit. The migrator code is a bit legacy and needs work but I haven't been able to prioritize that over other more pressing issues in Active Record.
It doesn't seem like ActiveRecord has a public API for running the migrations, so I had to get creative and tried using the private API
The public API is MigrationContext although I only made it public because Heroku needed it. I don't think it's a great API but again, I have other priorities and limited time. Hoping to get to improving migrations sometime this year. There's also a new ExecutionStrategy that might be useful here, it lets you hook into the methods migrations run to change behavior and how the migrations are executed.
I would accept a PR to Rails to add a specific table prefix or custom name to SchemaMigration. Not sure if I'll have time to prioritize this soon.
How long would you estimate it would take to get merged and released? Right now ronin-db
and every other library which pulls it in (ex: ronin-nmap, ronin-masscan, ronin-recon, ronin-app) as a dependency are dead in the water, due to ActiveRecord 7.1. Would this go into ActiveRecord 7.2 or 7.1.2? What are the chances such a PR would even be accepted since I'm re-adding a feature to a private API?
As an immediate stop-gap measure, I may have to lock the activerecord
dependency to ~> 7.0, < 7.1.0
.
I honestly don't have the bandwidth to work on this, I was trying to help guide you a bit. The migrator code is a bit legacy and needs work but I haven't been able to prioritize that over other more pressing issues in Active Record.
Could you provide example code of how to run migrations using ActiveRecord 7.1, or how to pass in a connection, or how to call ActiveRecord::Migrator
without explicitly passing it a db/migrate/
directory? I'm not familiar with how the private API changed in ActiveRecord 7.1. Example code that I could test locally and adapt would be more useful.
There's also a new ExecutionStrategy that might be useful here, it lets you hook into the methods migrations run to change behavior and how the migrations are executed.
How would I use ExecutionStrategy
? It sounds like it's used for overriding methods used within migrations such as create_table
? It also appears to be part of the private API, so I'm back the dilemma of relying on the private API which may change unexpectedly.
The public API is MigrationContext although I only made it public because Heroku needed it. I don't think it's a great API but again, I have other priorities and limited time. Hoping to get to improving migrations sometime this year.
Understood. This might be something I would be interested in helping with, since I need a stable API to run migrations from a non-local db/migrate/
directory which lives in a gem.
OK it took me a while to realize I could use connection
within the ActiveRecord::MigrationContext
sub-class, even before it is initialized, to obtain the connection necessary to initialize my ActiveRecord::SchemaMigration
class. I also realized I could just override #table_name
. Also decided to just target activerecord
~> 7.1, and avoid adding 7.0
vs. > 7.1
version checking logic to try to support both 7.0.x and 7.1.x. Not ideal, but works around the show stopper breakage.
https://github.com/ronin-rb/ronin-db-activerecord/commit/0530773f4423a520695322969245cc606173c013
It also appears that I can safely run all migrations using the default SchemaMigration
and schema_migrations
table, since all of the ronin-db-activerecord
0.1.x migrations create tables and use the if_not_exists: true
keyword argument. This may tempt me to switch from using the special ronin_schema_migrations
table to the schema_migrations
table. However, I worry about using ronin-db-activerecord
in other Rails apps that have their own migrations, where some weird db version confusion occurs between the ronin-db-activerecord
migration version numbers and the app's own migration version numbers (ex: if ronin-db-activerecord
's migrations are ran schema_migrations
will contain version 1 - 35, which will make ActiveRecord::MigrationContext
think the database is already migrated if the app's local db/migrate/
contains migration versions < 35). While it is possible to add a "scope" to the migration filenames (ex: 1234_foo_bar_baz.scope.rb
), neither the scope or not end up in the schema_migrations
table. Maybe I should switch to dates for the migration version numbers?
@eileencodes after confirming that switching to using the default schema_migrations
table would not break existing databases, I went with your suggestion of using ActiveRecord::MigrationContext
directly with just the ronin-db-activerecord/db/migrate/
directory path, which works with both activerecord 7.0 and 7.1. Hopefully this limits my exposure to the ActiveRecord private API and any future changes. Thanks for the hints!
activerecord 7.1 apparently removed
ActiveRecord::SchemaMigration.table_name_prefix
andActiveRecord::SchemaMigration.create_table
. Appears thatActiveRecord::SchemaMigration.create_table
was changed to be an instance methodActiveRecord::SchemaMigration#create_table
.If we try to remove
self.table_name_prefix = 'ronin_'
, we get this error: