CommandRecorder doesn't invoke recorded commands correctly on Ruby 3

[JoshCheek]

The ActiveRecord::Migration::CommandRecorder records commands and then replays them later. However it does not invoke the original commands correctly on Ruby 3, because it does not consider keyword arguments.

Steps to reproduce

require 'active_record'
ActiveRecord::Base.establish_connection adapter: 'sqlite3', database: ':memory:'

class ActiveRecord::Migration::CommandRecorder
  def update_view(*args) = record(:update_view, args)   # https://github.com/scenic-views/scenic/blob/v1.5.4/lib/scenic/command_recorder.rb#L14
  def invert_update_view(*args) = [:update_view, *args] # https://github.com/scenic-views/scenic/blob/v1.5.4/lib/scenic/command_recorder.rb#L31
end

# https://github.com/scenic-views/scenic/blob/v1.5.4/lib/scenic/statements.rb#L25
class ActiveRecord::ConnectionAdapters::SQLite3Adapter
  def update_view(name, version:, revert_to_version:) =
    puts("UPDATE VIEW: #{name}, version: #{version}, revert_to_version: #{revert_to_version}")
end

class SomeMigration < ActiveRecord::Migration[6.1]
  def change() = update_view(:my_view, version: 2, revert_to_version: 1)
end

pp RUBY_VERSION: RUBY_VERSION, 'ActiveRecord::VERSION': ActiveRecord::VERSION::STRING
puts
SomeMigration.migrate :down

Expected behavior

$ ruby example.rb
{:RUBY_VERSION=>"3.0.1", :"ActiveRecord::VERSION"=>"6.1.3.2"}

==  SomeMigration: reverting ==================================================
-- update_view(:my_view, {:version=>2, :revert_to_version=>1})
UPDATE VIEW: my_view, version: 2, revert_to_version: 1
   -> 0.0000s
==  SomeMigration: reverted (0.0001s) =========================================

Actual behavior

$ ruby example.rb
{:RUBY_VERSION=>"3.0.1", :"ActiveRecord::VERSION"=>"6.1.3.2"}

==  SomeMigration: reverting ==================================================
-- update_view(:my_view, {:version=>2, :revert_to_version=>1})
ar.example.rb:11:in `update_view': wrong number of arguments (given 2, expected 1; required keywords: version, revert_to_version) (ArgumentError)
	from /Users/josh/.gem/ruby/3.0.1/gems/activerecord-6.1.3.2/lib/active_record/migration.rb:931:in `block in method_missing'
	from /Users/josh/.gem/ruby/3.0.1/gems/activerecord-6.1.3.2/lib/active_record/migration.rb:899:in `block in say_with_time'
	from /Users/josh/.rubies/ruby-3.0.1/lib/ruby/3.0.0/benchmark.rb:293:in `measure'
	from /Users/josh/.gem/ruby/3.0.1/gems/activerecord-6.1.3.2/lib/active_record/migration.rb:899:in `say_with_time'
	from /Users/josh/.gem/ruby/3.0.1/gems/activerecord-6.1.3.2/lib/active_record/migration.rb:920:in `method_missing'
	from /Users/josh/.gem/ruby/3.0.1/gems/activerecord-6.1.3.2/lib/active_record/migration/command_recorder.rb:126:in `block in replay'
	from /Users/josh/.gem/ruby/3.0.1/gems/activerecord-6.1.3.2/lib/active_record/migration/command_recorder.rb:125:in `each'
	from /Users/josh/.gem/ruby/3.0.1/gems/activerecord-6.1.3.2/lib/active_record/migration/command_recorder.rb:125:in `replay'
	from /Users/josh/.gem/ruby/3.0.1/gems/activerecord-6.1.3.2/lib/active_record/migration.rb:743:in `revert'
	from /Users/josh/.gem/ruby/3.0.1/gems/activerecord-6.1.3.2/lib/active_record/migration.rb:867:in `exec_migration'
	from /Users/josh/.gem/ruby/3.0.1/gems/activerecord-6.1.3.2/lib/active_record/migration.rb:853:in `block (2 levels) in migrate'
	from /Users/josh/.rubies/ruby-3.0.1/lib/ruby/3.0.0/benchmark.rb:293:in `measure'
	from /Users/josh/.gem/ruby/3.0.1/gems/activerecord-6.1.3.2/lib/active_record/migration.rb:852:in `block in migrate'
	from /Users/josh/.gem/ruby/3.0.1/gems/activerecord-6.1.3.2/lib/active_record/connection_adapters/abstract/connection_pool.rb:462:in `with_connection'
	from /Users/josh/.gem/ruby/3.0.1/gems/activerecord-6.1.3.2/lib/active_record/migration.rb:851:in `migrate'
	from /Users/josh/.gem/ruby/3.0.1/gems/activerecord-6.1.3.2/lib/active_record/migration.rb:665:in `migrate'
	from ar.example.rb:24:in `<main>'

System configuration

Rails version: 6.1.3.2

Ruby version: ruby 3.0.1p64 (2021-04-05 revision 0fb782ee38) [arm64-darwin20]

Fixing it

For the "expected behaviour" step, I got it to work by changing the replay method to this (I'm not asserting this is correct, I find this keyword change very confusing, TBH):

class ActiveRecord::Migration::CommandRecorder
  def replay(migration)
    commands.each do |cmd, args, block|
      if args.last.respond_to?(:to_hash) && args.last.to_hash.all? { |k, _| k.is_a? Symbol }
        *ordinals, keywords = args
      else
        ordinals, keywords = args, {}
      end
      migration.send(cmd, *ordinals, **keywords, &block)
    end
  end
end

[JoshCheek]

cc @maribies

[JoshCheek]

Okay, I think I have it figured out:

The command recorder needs to keep ordinal and keyword args separate, the way it keeps the block arg separate. So for example, the record method, here, and several of the others around it, would look like this:

def record(command, *ordinals, **keywords, &block)
  if @reverting
    @commands << inverse_of(command, *ordinals, **keywords, &block)
  else
    @commands << [command, ordinals, keywords, block]
  end
end

def inverse_of(command, *ordinals, **keywords, &block)
  method = :"invert_#{command}"
  # ...
  send(method, *ordinals, **keywords, &block)
end

def replay(migration)
  commands.each do |cmd, args, keywords, block|
    migration.send(cmd, *args, **keywords, &block)
  end
end

I'm almost certain this is the only way it can become correct.

[eregon]

See https://eregon.me/blog/2021/02/13/correct-delegation-in-ruby-2-27-3.html
(*, **) does not work reliably on Ruby 2.7 (only on 3+).

So I think you either want ruby2_keywords def m(*args, &block) or (...).
If serialization of the kwargs is needed (e.g., store it outside the Ruby process), you might need Hash.ruby2_keywords_hash? / Hash.ruby2_keywords_hash.

[JoshCheek]

Thankyou, @eregon. Okay, after reading your blog and trying a whole bunch of things out, I think the only way to fix it is to fix the caller of record:

class ActiveRecord::Migration::CommandRecorder
-  def update_view(*args) = record(:update_view, args)
+  ruby2_keywords def update_view(*args) = record(:update_view, args)
  def invert_update_view(*args) = [:update_view, *args]
end

So the error occurred during the replay, but it was introduced during the recording step, like 6 method calls away, from a third party gem, and there doesn't seem to be much ActiveRecord can do about it.

I guess, I'll close this and propose to Scenic to declare those methods with ruby2_keywords, here.

The state of Ruby keywords is really rough right now, but seeing your examples of how it's broken in different ways on different versions was really helpful. Also seeing the Hash.ruby2_keywords_hash? was very clarifying. Anyway, on at least 5 separate occasions now, I've thought "this man is doing the Lord's work!" lol. So, 🤷 maybe I'll get to buy you a beer one of these days 🍻

[eregon]

That sounds good, and thank you for the kind words :)

I think any method taking *args, and then later calling a method which may use keyword arguments needs to be ruby2_keywords or (...) on 2.7+, otherwise the fact they were keyword arguments will be lost. (if Ruby 3+ is required, then (*args, **kwargs) also works; (*args) only works on 2.6 and before for passing kwargs).
So I would think all methods like def foo(*args); do_something_with(args); end in scenic need / should use ruby2_keywords.

Enabling the keyword warnings on Ruby 2.7 might be helpful here to help find the issue (https://eregon.me/blog/2021/02/13/correct-delegation-in-ruby-2-27-3.html#post-scriptum)

[JoshCheek]

Enabling the keyword warnings on Ruby 2.7 might be helpful here to help find the issue (https://eregon.me/blog/2021/02/13/correct-delegation-in-ruby-2-27-3.html#post-scriptum)

Yeah, that helps a lot, it does report this issue:

---

Unfortunately, I think these keywords are going to be a bit of a PITA for a a while to come still 😞, because the problem is introduced far away from where it crashes.

Here is a reproduction of 70% of what actually happened. You can see how far away the problematic code is from the final explosion (and the omitted parts could affect it, as well), making it pretty difficult to debug (esp given that my mental model wasn't correct):

RUBY_VERSION  # => "3.0.1"

# ActiveRecord::Migration::CommandRecorder (declaring this on `self` for simplicity)
  $commands = [] # made it global so we can look at what got recorded at the end
  command_recorder = Object.new
  def command_recorder.record(*command, &block) # https://github.com/rails/rails/blob/v6.1.4/activerecord/lib/active_record/migration/command_recorder.rb#L78
    $commands << (command << block) # note that command is `[method_name, args_ary]`, restargs here don't matter
  end

  def command_recorder.replay(migration) # https://github.com/rails/rails/blob/v6.1.4/activerecord/lib/active_record/migration/command_recorder.rb#L124
    $commands.each do |cmd, args, block|
      migration.send(cmd, *args, &block)
    end
  end

# ActiveRecord::Migration
  Migration = Struct.new(:connection) do
    ruby2_keywords def method_missing(method, *arguments, &block) # https://github.com/rails/rails/blob/v6.1.4/activerecord/lib/active_record/migration.rb#L915
      connection.send(method, *arguments, &block) # THIS IS WHERE THE ERROR GETS REPORTED
    end
  end
  connection = Object.new
  migration = Migration.new(connection)

# Scenic
  # THIS IS WHERE THE ERROR ACTUALLY IS
  def command_recorder.update_view(*args) # https://github.com/scenic-views/scenic/blob/v1.5.4/lib/scenic/command_recorder.rb#L14
    record(:update_view, args)
  end

  $successful_invocations = []
  def connection.update_view(name, version: nil, revert_to_version: nil) # https://github.com/scenic-views/scenic/blob/v1.5.4/lib/scenic/statements.rb#L91
    $successful_invocations = [name, version, revert_to_version]
  end

# Proposed change (fork so we can try with and without the change)
  (has_my_change = !!fork) && class << command_recorder
    ruby2_keywords :update_view
  end

# Testing
  # This happens in my migration, it's what gets recorded/replayed and leads to the failure
  command_recorder.update_view :my_view, version: 2, revert_to_version: 1

  # This code is run by https://github.com/rails/rails/blob/v6.1.4/activerecord/lib/active_record/migration.rb#L741
  # You'll notice I simplified it by ignoring the translation to the inversion command, and by directly calling it on the
  # command recorder instead of swapping the command-recorder for the connection on the first invocation,
  # but those steps are in the middle of all this, too, and could play a part
  is_kw_hash = Hash.ruby2_keywords_hash? $commands.last[1].last
  begin
    command_recorder.replay migration
    result = $successful_invocations
  rescue
    result = $!
  end

# Results
  has_my_change # => true,             false
  is_kw_hash    # => true,             false
  result        # => [:my_view, 2, 1], #<ArgumentError: wrong number of arguments (given 2, expected 1)>