Does `multiverse` raise a `PendingMigration` error if not migrated?
Closed this issue · 6 comments
For instance, in a vanilla Rails app if you attempt to start the app with migrations pending, Rails throws PendingMigrationError
, reminding you to run rake db:migrate
.
From what I've tried so far, it does not seem like the current behaviour supports that -- the app does not care if there are pending migrations in other DBs.
Was wondering if such a thing exists for multiverse
or if there's a plan towards that?
@edisonywh and I have looked into this, and came up with a solution for our app. (Rails 4.2.11)
We looked into how Rails does it (inserting the migration checker into the middleware), and figured we could do the same:
Rails.application.config.middleware.insert_after '::ActionDispatch::Callbacks', 'ActiveRecord::Migration::CheckPendingCustom'
@ankane What do you think? Perhaps we could open a PR for this?
Hey @edisonywh and @jooeycheng, you're right that Multiverse doesn't support this right now. Feel free to post the solution you come up with. Since Multiverse will be phased on with Rails 6, I don't think it's worth the effort for a PR.
Although Multiverse is no longer useful in Rails 6 onwards, I think there are still quite a number of people who will still be on Rails 5 and below. I don't mind creating a PR for this (also allows us to upstream our fix). But I'm not sure how to write an elegant solution and a test for this. 🙈
Anyway, here's what we did - in our app, we have have Rails Engine which connects to a different database with multiverse
.
With inspiration from active_record/migration.rb, this is what we came up with:
- Create a
CheckPendingMigration
class. For our case, we're using Rails Engine, hence we created the file on the Engine bcos it's sort of the Engine's domain.
# (my_engine) ./app/utilities/my_engine/check_pending_migration.rb
#
# we use `rake db:task` instead of `rails db:task` bcos we're on Rails 4 (yes, we're old)
module MyEngine
class PendingMigrationError < ActiveRecord::MigrationError
def initialize
super("Migrations are pending. To resolve this issue, run:\n\n\tbundle exec rake db:migrate:all")
end
end
class CheckPendingMigration
def initialize(app)
@app = app
end
def call(env)
if connection.supports_migrations?
raise MyEngine::PendingMigrationError if ActiveRecord::Migrator.needs_migration?(connection)
end
@app.call(env)
end
private
def connection
MyEngine::CustomDatabase.connection
end
end
end
- Then, append the
CheckPendingMigration
class into the "Rails middleware" on app initialize (this is how Rails does it)
# ./app/config/initializers/check_pending_migration.rb
if Rails.env.development? || Rails.env.test?
Rails.application.config.middleware.insert_after 'ActionDispatch::Callbacks', 'MyEngine::CheckPendingMigration'
end
- Notice the
db:migrate:all
task in step 1. That's custom:
# ./lib/tasks/db.rake
namespace :db do
namespace :migrate do
task all: :environment do
puts 'Migrating main database'
Rake::Task["db:migrate"].invoke
%w[custom_db1 custom_db2].each do |db|
puts "Migrating #{db} database with multiverse"
`DB=#{db} bundle exec rake db:migrate`
end
end
end
end
- If you're using Capistrano for deployments. Refer to #28.
Notice we prompt rake db:migrate:all
instead of DB=custom rake db:migrate
. This is due a bug in ActiveRecord::Migrator.needs_migration?(connection)
. It doesn't respect the given connection
, and takes the default ActiveRecord::Base.connection
. So, we're not able to check if migrations are pending on our custom
database. We added a db:migrate:all
command to always attempt to migrate all databases instead.
Explanation for the bug mentioned above:
See active_record/migration.rb and active_record/schema_migration.rb. Notice SchemaMigration
inherits from ActiveRecord::Base
, so SchemaMigration
checks will always run on the default ActiveRecord::Base.connection
, and only check for pending migrations on that connection. This has since been refactored to work differently in Rails 6.
Hey @jooeycheng, thanks for sharing. Seems like it should be enough for other developers to add to their app if desired. As I mentioned before, I don't think it's worth the effort for a PR.
Got it @ankane. Thanks!
I think we can consider this issue closed.
Sounds good, also added a note in the readme about this limitation.