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

Can't pass arguments on events with bangs in Ruby 3.0 #834

Open
endangurura opened this issue Jun 28, 2023 · 14 comments
Open

Can't pass arguments on events with bangs in Ruby 3.0 #834

endangurura opened this issue Jun 28, 2023 · 14 comments

Comments

@endangurura
Copy link

I'm working on upgrading an application from Rails 5.2 to latest.
Currently running:

rails 6.1
ruby 3.0
aasm 5.5 (I tried 5.4 too, same results)

When I try to call events with bangs, I get an ArgumentError wrong number of arguments (given 1, expected 0)

event :accept, after: :send_accept_notification do
  transitions from: :pending, to: :accepted, after: after: proc { |user| check_accept_permission(user) }
end

call

job = Job.new
job.accept!(current_user)  # => raises ArgumentError
job.accept(current_user)  # => works but as expected no persistence

With Ruby 2.7 the ArgumentError is not raised

@that-jill
Copy link

Can validate that this is happening on my application:

Rails 6.0.6.1
Ruby 3.0.6 (formerly 2.7.7)
AASM 5.5.0

request.accept!    # => ArgumentError: wrong number of arguments (given 1, expected 0)
request.accept     # => succeeds

@kwerle
Copy link

kwerle commented Sep 22, 2023

I'm guessing the problem is somewhere between

   124:       safely_define_method klass, "#{name}!", ->(*args, &block) do
   125:         aasm(aasm_name).current_event = :"#{name}!"
   126:         aasm_fire_event(aasm_name, event, {:persist => true}, *args, &block)
   127:       end

and

   121:       # Returns true if event was fired successfully and transaction completed.
   122:       def aasm_fire_event(state_machine_name, name, options, *args, &block)
   123:         return super unless aasm_supports_transactions? && options[:persist]

This is made complicated by the changes in args around ruby 3. In the lower code (ruby 3.2 in my case), args is an array with one hash in it. That's probably not what's wanted. Getting that to work for ruby 2.x and 3.x may involve something like
args_hash = Array(args).first

@kwerle
Copy link

kwerle commented Sep 22, 2023

Ah - it looks like maybe this is fixed in https://github.com/aasm/aasm/tree/add_ruby_3.2_in_github_workflows

Looks like that was only a partial fix.

@xjunior
Copy link
Contributor

xjunior commented Sep 25, 2023

@kwerle is there a PR for that? I feel the proposed solution is a bit hacky, and also resolves the problem where it blows, not where it originates. The better solution would be to separate the keyword arguments in the Invoker, and add Ruby 3.2 support there.

@kwerle
Copy link

kwerle commented Sep 25, 2023

I found that location by stepping into my code where it was failing. I certainly didn't dig into the AASM code to figure out the right fix. It seems likely that my research could be useful for writing a failing test if nothing else. This issue will block my company from doing a ruby upgrade - which is not a priority for us this quarter. It will become a priority in a couple of months. I'm kind of hoping that it'll be fixed and pushed by then :-)

@xjunior
Copy link
Contributor

xjunior commented Oct 12, 2023

@kwerle we're in the same situation, and we might be taking a stab at it soon.

@xjunior
Copy link
Contributor

xjunior commented Oct 19, 2023

@kwerle the fix you mentioned is present on 5.5.0. Also, aasm is currently running with ruby 3.0 and rails 6.1 (not 6.0 yet). This example seems to implement the situations that both you and @that-jill discussed, and it seems to pass under all conditions. Can you create a failing spec?

@kwerle
Copy link

kwerle commented Oct 19, 2023

Can you create a failing spec?

I'm on an M1 mac and do all my dev work in docker containers. I have not been able to get the aasm test environment up and running so I don't believe I will be able to write a good test. If nobody else tackles this before we really need it, I'll provide a monkey patch that addresses the issue - probably later this quarter.

@xjunior
Copy link
Contributor

xjunior commented Oct 20, 2023

I forgot to link the example I believe represents your problem now, and is passing on main.

describe 'parametrised events' do

Do you agree that it's the same case? If you don't, we could work together to get a failing spec and I can take care of fixing it.

@kwerle
Copy link

kwerle commented Nov 3, 2023

I'm starting to dig into this, and the first broken spot I see is we have:

    event :submit do
      before { |some_variable:| self.foo = some_variable }
      transitions from: ...
      success :...
    end

my_object.submit(some_variable: 1234)

It looks like in ruby 3.2 and aasm 5.5.0 before is no longer being called with the some_variable: hash parameter.

@kwerle
Copy link

kwerle commented Nov 3, 2023

Ah - instead it is being passes as a hash: {:some_variable =>1234}

@kwerle
Copy link

kwerle commented Nov 7, 2023

For those following with interest - I think I've given enough detail to formulate a reasonable test in #834 (comment) . I will not be pursuing this further - our workaround is to simply accept the hash and use that.

@fbraure
Copy link

fbraure commented Dec 4, 2023

We have the same problem here. Has anyone found a way to fix it?

@xjunior
Copy link
Contributor

xjunior commented Jan 4, 2024

@fbraure the current workaround is to simply accept the argument as a hash and fetch options from it, as pointed by @kwerle

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

5 participants