Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

breaking change in 5.5.0 with literal_invoker #809

Open
womblep opened this issue Feb 6, 2023 · 4 comments
Open

breaking change in 5.5.0 with literal_invoker #809

womblep opened this issue Feb 6, 2023 · 4 comments

Comments

@womblep
Copy link

womblep commented Feb 6, 2023

Hi,
It looks like there is a breaking change in the latest version 5.5.0 with callbacks.
I call transitions by calling them with arguments like this_event!(arg1, arg2). This has worked perfectly up to the latest change.
Now I get:

wrong number of arguments (given 1, expected 2) (ArgumentError)
2023-02-06T08:49:54.900982054Z /usr/src/app/my_aasm.rb:190:in `this_event'
2023-02-06T08:49:54.900988684Z /usr/local/bundle/gems/aasm-5.5.0/lib/aasm/core/invokers/literal_invoker.rb:34:in `exec_subject'
2023-02-06T08:49:54.900992314Z /usr/local/bundle/gems/aasm-5.5.0/lib/aasm/core/invokers/literal_invoker.rb:19:in `invoke_subject'
2023-02-06T08:49:54.900995604Z /usr/local/bundle/gems/aasm-5.5.0/lib/aasm/core/invokers/base_invoker.rb:48:in `invoke'
2023-02-06T08:49:54.900998574Z /usr/local/bundle/gems/aasm-5.5.0/lib/aasm/core/invoker.rb:85:in `invoke'
2023-02-06T08:49:54.901001844Z /usr/local/bundle/gems/aasm-5.5.0/lib/aasm/core/transition.rb:75:in `invoke_callbacks_compatible_with_guard'
2023-02-06T08:49:54.901005324Z /usr/local/bundle/gems/aasm-5.5.0/lib/aasm/core/transition.rb:49:in `execute'
2023-02-06T08:49:54.901007694Z /usr/local/bundle/gems/aasm-5.5.0/lib/aasm/core/event.rb:158:in `block in _fire'
2023-02-06T08:49:54.901010154Z /usr/local/bundle/gems/aasm-5.5.0/lib/aasm/core/event.rb:148:in `each'
2023-02-06T08:49:54.901012504Z /usr/local/bundle/gems/aasm-5.5.0/lib/aasm/core/event.rb:148:in `_fire'
2023-02-06T08:49:54.901014034Z /usr/local/bundle/gems/aasm-5.5.0/lib/aasm/core/event.rb:51:in `fire'
2023-02-06T08:49:54.901015474Z /usr/local/bundle/gems/aasm-5.5.0/lib/aasm/aasm.rb:106:in `aasm_fire_event'
2023-02-06T08:49:54.901016814Z /usr/local/bundle/gems/aasm-5.5.0/lib/aasm/persistence/orm.rb:132:in `block in aasm_fire_event'
2023-02-06T08:49:54.901018414Z /usr/local/bundle/gems/aasm-5.5.0/lib/aasm/persistence/active_record_persistence.rb:105:in `block in aasm_transaction'
2023-02-06T08:49:54.901019934Z /usr/local/bundle/gems/activerecord-7.0.4.2/lib/active_record/connection_adapters/abstract/transaction.rb:319:in `block in within_new_transaction'
2023-02-06T08:49:54.901021564Z /usr/local/bundle/gems/activesupport-7.0.4.2/lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `handle_interrupt'
2023-02-06T08:49:54.901029124Z /usr/local/bundle/gems/activesupport-7.0.4.2/lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `block in synchronize'
2023-02-06T08:49:54.901030844Z /usr/local/bundle/gems/activesupport-7.0.4.2/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `handle_interrupt'
2023-02-06T08:49:54.901032584Z /usr/local/bundle/gems/activesupport-7.0.4.2/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `synchronize'
2023-02-06T08:49:54.901034614Z /usr/local/bundle/gems/activerecord-7.0.4.2/lib/active_record/connection_adapters/abstract/transaction.rb:317:in `within_new_transaction'
2023-02-06T08:49:54.901036104Z /usr/local/bundle/gems/activerecord-7.0.4.2/lib/active_record/connection_adapters/abstract/database_statements.rb:316:in `transaction'
2023-02-06T08:49:54.901037544Z /usr/local/bundle/gems/activerecord-7.0.4.2/lib/active_record/transactions.rb:209:in `transaction'
2023-02-06T08:49:54.901038964Z /usr/local/bundle/gems/aasm-5.5.0/lib/aasm/persistence/active_record_persistence.rb:103:in `aasm_transaction'
2023-02-06T08:49:54.901040384Z /usr/local/bundle/gems/aasm-5.5.0/lib/aasm/persistence/orm.rb:131:in `aasm_fire_event'
2023-02-06T08:49:54.901041854Z /usr/local/bundle/gems/aasm-5.5.0/lib/aasm/base.rb:126:in `block in event'
@StragaSevera
Copy link

StragaSevera commented Feb 10, 2023

I have similar problems. It happens only in Ruby 3.2.

    event :close do
      before do |closed_at:|
        self.closed_at = closed_at
      end

      transitions from: :activated, to: :closed
    end
    # ...
    loan.close!(closed_at: Time.current)

results in:
ArgumentError: missing keyword: :closed_at

@tagliala
Copy link

tagliala commented Mar 13, 2023

I'm not able to replicate the first issue.

However, here it is a reduced test case for the second issue, affecting Ruby 3.2 only.

I think they are separate issues because the second one also affects aasm 5.4.0

# frozen_string_literal: true

require 'bundler/inline'

begin
  gemfile(true) do
    source 'https://rubygems.org'

    gem 'aasm', '5.4.0' # it also affects 5.5.0
    gem 'activerecord'
    gem 'pg'
  end

  require 'aasm'
  require 'active_record'
  require 'logger'
rescue Gem::LoadError => e
  puts "\nMissing Dependency:\n#{e.backtrace.first} #{e.message}"
rescue LoadError => e
  puts "\nError:\n#{e.backtrace.first} #{e.message}"
  exit 1
end

ActiveRecord::Base.establish_connection(adapter: 'postgresql', database: 'bench')
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :loans, force: true do |t|
    t.string :status
    t.timestamp :closed_at
  end
end

class Loan < ActiveRecord::Base
  include AASM

  aasm(column: :status) do
    state :activated, initial: true
    state :closed

    event :close do
      before do |closed_at:|
        self.closed_at = closed_at
      end

      transitions from: :activated, to: :closed
    end
  end
end

loan = Loan.create!

loan.close! closed_at: Time.current

@eric-hemasystems
Copy link

I've also gotten the second issue. The root of it is here. The argument spat is being used to forward argument but starting in Ruby 3 hash arguments are not automatically converted to keyword arguments. This means both the position splat and the keyword splat needs to be used.

A quick hack I implemented is:

module AASM
  module Ruby3ProcCompat
    def exec_proc(parameters_size)
      kwargs = {}
      has_keyword_args = subject.parameters.any? { |(type, _)| type.to_s.starts_with? "key" }
      if has_keyword_args
        kwargs = args.extract_options!
        parameters_size -= 1 if !kwargs.empty? && parameters_size > 0
      end

      return record.instance_exec(&subject) if parameters_size.zero?
      return record.instance_exec(*args, **kwargs, &subject) if parameters_size < 0
      record.instance_exec(*args[0..(parameters_size - 1)], **kwargs, &subject)
    end
  end
end

AASM::Core::Invokers::ProcInvoker.prepend AASM::Ruby3ProcCompat

It hasn't really gotten much testing so use with caution. A more complete solution probably needs more refactoring. A few things that to point out:

  • There is conditional logic depending on if proc responds to parameters. I feel like this could be removed. parameters has been supported since Ruby 1.9.2 which is far beyond the supported version of Ruby. At the same time what is wrong with using arity. It seems simpler and what the literal invoker uses for example.
  • The literal invoker recently worked around this via PR Run specs on ruby 3.2 version #808. We could follow that impl although I somewhat question that impl. What if a callback accepts both position arguments and keyword argument? That impl seems to just handle either position or keyword args but not both. Also that impl would support position arguments where the first argument is a hash.
  • Does AASM assume ActiveSupport? I.E. could a real impl us extract_options! like I am using since I think that is a Rails thing.
  • It would be nice to unify how arguments are passed between the different invokers so they don't drift in the future.

@eric-hemasystems
Copy link

I actually wonder if the original posters issue relates to the fix in #808? Could one of their args be a hash even though the method only accepts position params and therefore it's being invoked incorrectly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants