Use calling migration version for safe_version? when calling revert

[joshmenden]

The Problem

Let's suppose you initialize your StrongMigrations like so

# config/initializers/strong_migrations.rb
StrongMigrations.start_after = 20230101000000

But, let's say you have a pair of migrations that StrongMigrations would consider "dangerous" from a long time ago that look like this

# db/migrate/20210101000000_dangerous_migration.rb
class DangerousMigration < ActiveRecord::Migration[7.1]
  def change
    # ... some dangerous operation
  end
end

# db/migrate/20210101000001_revert_dangerous_migration.rb
class RevertDangerousMigration < ActiveRecord::Migration[7.1]
  def change
    revert DangerousMigration
  end
end

Under these circumstances, StrongMigrations will fail on the RevertDangerousMigration, even though I would expect it to pass without error since both of these migrations occur before the configured start_after.

My debugging leads me to believe that this is because, when we call revert DangerousMigration, the underlying dangerous operation call on the migration being reverted triggers the method_missing logic that ultimately checks the safety of the migration being reverted, but StrongMigration has no context of this migration's version since the migration is called as a class with no context for the original filename. In other words, StrongMigrations doesn't have knowledge of the version of the migration being reverted, only that of the migration doing the revert. When StrongMigrations doesn't see a version, it assumes the migration is unsafe, causing it to fail.

The Solution

I had the thought to override the revert method and check the migration's version there before moving onto the child migration. I'm open to other ideas about how best to solve this.

Ultimately, these changes make it so that, in a migration where you call the revert method, StrongMigrations compares the calling migration's version against StrongMigration.start_after to decide whether or not to run checks on the reverted migration.

Because of the details of the way the tests are set up, the reason why these tests would fail before the changes aren't obvious at first, so let me explain.

In the failing test, the first migration AddTableDangerously is given the default migration version 123 that is defined here.

When we call assert_safe RevertAddTableDangerously in the test, we only end up calling the helper migrate function on the RevertAddTableDangerously migration. When this migration then calls the change method on the migration it is reverting, AddTableDangerously, we don't give it the default 123 version, leaving it as nil, which then fails the version_safe? method here.

This is not identical to my tests with the gem in a dummy live app, but it's identical in that the child migration that is being reverted also has a nil version when running with the dummy app, causing it to fail before the changes are implemented.

Now, with the changes, we implement a def revert method that checks the version, then passes everything on to super with a safety_assured block if the calling migration's version is safe.

I looked to active_record/migration.rb#revert for the method signature that I needed to override and then pass onto super.

[ankane]

Hi @joshmenden, thanks for the PR! The approach looks good. For testing, reverting a create_table migration should always be safe (since drop_table is), so think there may be another issue around revert.

[ankane]

I think this might address it, but need to do more testing.

Edit: Addressed in f8aa998

[ankane]

Merged in the commit above (updated to use the existing revert migration to address the issue above). Thanks for reporting and the fix!

[joshmenden]

Thanks @ankane, and thanks for providing such excellent open source software!