[ActiveRecord] migrations: update statements are silently being executed in both directions

[makmic]

I recently stumbled across a weird behavior in ActiveRecord migrations that appears to be a bug: update statements within the change method are being silently executed during the rollback phase when they are neither wrapped in an up_only nor a reversible block. For example:

class ResetUsersUpdatedAt < ActiveRecord::Migration[7.1]
  
  def change
    update(<<~SQL.squish)
      UPDATE users
      SET updated_at = NOW()
    SQL
  end
  
end

When I run this migration upwards, it behaves as expected. db:migrate yields the following output:

== 20240411074748 ResetUsersUpdatedAt: migrating ==========================
-- execute("UPDATE users SET updated_at = NOW()")
   -> 0.0007s
== 20240411074748 ResetUsersUpdatedAt: migrated (0.0188s) =================

However, the output of db:rollback is empty:

== 20240411074748 ResetUsersUpdatedAt: reverting ==========================
== 20240411074748 ResetUsersUpdatedAt: reverted (0.0680s) =================

A quick glance at the updated_at column reveals that the update statement was being silently executed in both direction.
My expectation would have been that it would raise an ActiveRecord::IrreversibleMigration just like execute – or otherwise print the executed statement to STDOUT.

Steps to reproduce

Run the code below:

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"
  gem "rails"
  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"

ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :payments, force: true do |t|
    t.text :migration_type
  end
end

class Payment < ActiveRecord::Base
end

class SetMigrationTypeWithUpdate < ActiveRecord::Migration::Current
  def change
    migration_type = reverting? ? 'down' : 'up'
    update(<<~SQL.squish)
      UPDATE payments
      SET migration_type = "#{migration_type}"
    SQL
  end
end

class SetMigrationTypeWithExecute < ActiveRecord::Migration::Current
  def change
    migration_type = reverting? ? 'down' : 'up'
    execute(<<~SQL.squish)
      UPDATE payments
      SET migration_type = "#{migration_type}"
    SQL
  end
end

class BugTest < Minitest::Test
  def test_update_migration_up
    Payment.create!(migration_type: 'none')
    SetMigrationTypeWithUpdate.migrate(:up)

    assert_equal "up", Payment.last!.migration_type
  end

  # This test case is red and yields two unexpected assertions:
  # * no exception is being raised
  # * the update statement was executed (the value was changed to "down")
  #
  # And on top of that, no output to STDOUT was being made so it happened silently..
  def test_update_migration_down
    assert_raises ActiveRecord::IrreversibleMigration do
      SetMigrationTypeWithUpdate.migrate(:down)
      assert_equal "up", Payment.last!.migration_type
    end
  end

  def test_execute_migration_up
    Payment.create!(migration_type: 'none')
    SetMigrationTypeWithExecute.migrate(:up)

    assert_equal "up", Payment.last!.migration_type
  end

  def test_execute_migration_down
    assert_raises ActiveRecord::IrreversibleMigration do
      SetMigrationTypeWithExecute.migrate(:down)
      assert_equal "up", Payment.last!.migration_type
    end
  end
end

Expected behavior

• The test case test_update_migration_down should be green.
• If I use update within def change and try to roll back that migration, it should raise an ActiveRecord::IrreversibleMigration exception
• Any SQL statement that is being executed during the rollback phase is being printed to the STDOUT

Actual behavior

• The test case test_update_migration_down is red
• No exception is being made
• The rollback phase does not print the "silent" update statement to the STDOUT

System configuration

Rails version: 7.1.3.2
Ruby version: 3.3.0

[makmic]

Thank you for looking into this issue. I did check if they are affected from the same bug, but it might be worth testing similar statements like insert and delete as part of the fix as well.

[rafaelfranca]

Yeah, that is what I was trying to avoid with my patch. Because new methods could be added an we forget to handle them. But my patch broke way more than I expected, so I just reverted it for now. Failing to find a way to make sure all commands are recorded, we will have to explicitly list all commands that can't be reverted.