dry-rb / dry-transaction

Business transaction DSL
http://dry-rb.org/gems/dry-transaction
MIT License
468 stars 55 forks source link

Using locally defined steps with Dry::Container does not work #61

Closed jonolsson closed 7 years ago

jonolsson commented 7 years ago

Using locally defined steps and at the same time as using Dry::Container for other steps fails due to Dry::Container throwing an exception when the step can't be resolved.

classContainer
  extend Dry::Container::Mixin
  register :verify,   -> input { Right(input) }
end
klass = Class.new do 
  include Dry::Transaction(container: Container)

  step :thing_to_do

  def thing_to_do(input)
    Right(input)
  end
end.new
klass.({})

Results in:

Dry::Container::Error: Nothing registered with the key :thing_to_do
   gems/dry-container-0.6.0/lib/dry/container/resolver.rb:22:in `block in call'
   gems/dry-container-0.6.0/lib/dry/container/resolver.rb:21:in `fetch'
   gems/dry-container-0.6.0/lib/dry/container/resolver.rb:21:in `call'
   gems/dry-container-0.6.0/lib/dry/container/mixin.rb:112:in `resolve'
   gems/dry-container-0.6.0/lib/dry/container/mixin.rb:125:in `[]'
...

Can be fixed by changing lib/dry/transaction/operation_resolver.rb line 8 to:

operation = kwargs.fetch(step.step_name) {
  ops_container and ops_container.key?(step.operation_name) and ops_container[step.operation_name]
}

I have a PR ready to be submitted for this change.

jcmfernandes commented 7 years ago

Thanks @jonolsson, hitted the exact same problem today and was about to open an issue here 😉 that's exactly the root cause.

IMHO, specs should be using a Dry::Container instance as the container instead of a hash because, AFAIK, no example in the documentation uses a hash as the container.

GustavoCaso commented 7 years ago

Thanks @jonolsson for creating the issue. I would love to see that PR. I just check the specs and we test that all steps are local but we did not test for that case when we actually pass a Container https://github.com/dry-rb/dry-transaction/blob/master/spec/integration/transaction_spec.rb#L159-L180

timriley commented 7 years ago

Thanks for the report @jonolsson! I have a moment now so I'm taking a look at this.

timriley commented 7 years ago

@jonolsson your fix as described here looks good, will be happy to accept your PR once it's ready, thanks!

jonolsson commented 7 years ago

Nice, I have submitted the PR now.

timriley commented 7 years ago

Fixed in #64