Dry-transaction: allow injecting operations for steps defined as methods?


#1

Currently in dry-transaction if a step is defined with a method, it seems you cannot inject an alternate implementation. The logic to initialize the transaction will always prefer the method over a passed in operation.
It doesn’t feel consistent to me that the way a step is defined should affect the ability to override it, and there don’t appear to be tests describing the situation, so I’m guessing this is just an implementation detail, not desired behavior.
Is this a change people would like to see? I’d be happy to make a PR for it!


#2

Well, my hopes for this being an obvious and nonintrusive change were dashed as soon as I tried to implement it and ran the tests :slight_smile:

I had read the docs about wrapping steps, but didn’t connect the dots. If you inject a step definition into a transaction, and a method of the same name exists, that method will wrap the injected operation, not replace it. If you aren’t calling super in your method, injecting a different implementation does nothing.
This does feel inconsistent, as I mentioned in my original post, but I’m new to this dry-rb ecosystem.
Clearly this is not a simple change now, so any higher level thoughts on the subject? Maybe wrapping should be explicitly added to the API as a step option or something, so it doesn’t step on method-defined steps?


#3

Hi @rhec, thanks for taking the time to explore this and share it here in the forum. You’re actually not the first person to consider this kind of behaviour; it came up via a GitHub issue in the last few weeks, too.

One of my responses to the issue explained the reason why we didn’t allow instance method-based steps to be overridden from the outside:

[When developing the class-based transactions], I considered those instance methods to be “intrinsic” to the class, i.e. not separate enough to be some sort of external concern/external object. i.e. not the kind of thing that should be changeable from the outside. Providing alternative objects for these when testing almost feels as much as a “smell” as e.g. stubbing private methods when testing.

So we don’t have any plans currently to make it so steps provided by instance methods can be replaced from the outside.

That said, I’d still love to hear what you were hoping to do. Maybe with some more understanding of various use cases we might be able to provide something that helps you.


#4

Thanks for the reply and your interest @timriley, reading through that github issue is helpful.
To give you more context on what I’m doing:
I’m working with an existing and actively developed Rails app and I am looking for a way to improve the separation of our business logic from lower-level concerns like persistence and validation - i.e. get away from ‘fat’ models. So everything I read about dry-rb sounds great! Philosophically we are aligned.
The challenging part is getting started, and I was hoping to be able to introduce pieces of dry-rb one at a time in areas where I think they can provide value in isolation. The less I have to re-organize existing code the better, and I’d love to avoid introducing a lot of new concepts at once.
My thinking is transactions are nice, high-level concept to start with. So defining a transaction as a class, with no container, and a set of methods as operations is very appealing to me. Simple to write, obvious to the reader what it’s doing, no decisions to make about how to organize your code, and still very useful! It’s the perfect way to get started, and I’d guess that’s why it’s the first example in the docs.
So given all that context, what I was actually doing is just trying to prove I could inject different operations in my tests, for all the reasons it’s good to be able to inject different operations when they come from a container.
I know I’m repeating this point, so hopefully I’m not belaboring it, but even after your explanation I don’t quite get why an operation should be different based on the details of how it’s implemented. If it’s callable and returns an either monad, it seems like it should behave consistently. I understand it could just be convention, and if conventions are useful, they can certainly be worth some inconsistency and increase in learning curve. But it does feel like you are giving up a lot of simplicity and consistency for this convention. Maybe I just haven’t run into the right use case for wrapping operations, but it doesn’t seem worth the tradeoff to me at this point.

Thanks again if you made it this far - long post!


#5

@rhec You usually wrap operation to correct input and output to match injected operation API.
If you need the more explicit way to work with operations you can inject them to current operation by using dry-auto_inject. For example, we use it to inject some helpers:
include Import[h: 'rails.url_helpers']

If you implement business logic in transaction methods you probably don’t want to test it in isolation. And if you want - just extract it to another transaction.
In tests, you usually worry about some side effects (like persistence). For this, you can extract them into separated generic operation and use it everywhere.

class MakeSomeWork
  include Dry::Transaction(container: Container)
  
  step :work
  step :persist

  def work(model:, **input)
    # make some work
  end

  def persist(model:, **input)
    super(model)
  end
end

class MakeSomeWork
  include Dry::Transaction::Operation
  include Import['persist']
  
  def call(model:, **input)
    # make some work
    persist.call(model)
  end
end

#6

Thanks for the response, @rhec!

Yep, right now dry-transaction won’t let you inject operations if they don’t come from a container. This is what we’ll be addressing in response to issue #75 (the one I linked above). Once this is done, any step that you define will be injectable.

However, if you have instance methods matching step names, then those will always be called first, and it’ll be up to the method body to call super if it wants to ensure the step’s injected operation object runs.


#7

Thanks @semenovDL @timriley
I’m making progress!