dry-rb / dry-transaction

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

[WIP] Allow steps to take inputs from any output #119

Closed waiting-for-dev closed 5 years ago

waiting-for-dev commented 5 years ago

This follows from a conversation in the forums.

Basically, it transforms dry-transaction into a DSL for the result monad, allowing any step to take as inputs the outputs from any other previous step. It is controlled with the input: option in the step adapter. For example:

step :log, with: :log, input: [:process, :persist]

would make the log operation to take as positional arguments the outputs of :process and :persist steps, along with any other additional step argument.

Inputs order is not coupled to steps order, so you can do:

step :log, with: :log, input: [:persist, :process]

You can always reference the initial transaction input with the :_initial reserved word:

step :log, with: :log, input: [:_initial]

An empty list corresponds to the "continue" or "then" (>>) monadic operation.

If the input: option is not given, the behaviour defaults to be like always: to take the last step output as input.

An alternative API could be more explicit about this being a DSL for monads:

step :log, with: :log, type: :bind, input: [:persist, :process]
step :log_two, with: :log_two, type: :then

In this case, :type would default to :bind, while :then would ignore an :input option when it is given. This would be more explicit, but it would add more complexity to the code and mainly to an end user not familiar with monads.

Important: This commit breaks the around: step behaviour. If dry-transaction becomes a DSL for monads, then there is no place for around: as it is. I think we should redesign it making it something between the transaction and the steps, but not a step adapter itself. This way we can still have the steps being pure monadic code, while the transaction's around: behaviour would deal with anything else.

waiting-for-dev commented 5 years ago

Further developing on the :around subject, if I'm not missing something, we could still keep a backward compatible API. The only difference in the behaviour would be that it would wrap all the transaction steps instead of only subsequent steps. But, anyway, I think that wrapping subsequent steps is not the right abstraction:

# Current behaviour
step :validate
around :rescue_if_crash
step :persist_1
step :persist_2
step :it_will_never_crash # why does it need to be within  the around block?

Whether we are super-specific and forget about monadic stuff:

step :validate
around :rescue_if_crash do
  step :persist_1
  step :persist_2
end
step :it_will_never_crash

or we can let around: as something at the transaction level with the steps stack as purely monadic code:

around :rescue_if_crash # It is not a step, it just wraps the call to the stack.

step :validate
step :persist_1
step :persist_2
step :i_will_never_crash

I like more the second option. If you like the whole idea I could adapt around: in this same PR so that master is not broken.

waiting-for-dev commented 5 years ago

Sorry, still one more thought. In fact, I think that we could go well without any kind of :around behaviour, although it would not be backward compatible. If we want to side with good practices, usually what will follow a monadic operation is a case analysis on its result. It can just be left for the user after he calls Transaction#call, or if we want to support scenarios like the database transaction out of the box (which makes sense) we can provide a way to execute a piece of code on failure:

step :validate
step :persist

on_failure :rollback

private

def rollback
  raise MyDb::Rollback
end
timriley commented 5 years ago

Thanks for all of this, @waiting-for-dev. Just wanted to let you know my plate's a bit full for the next week and half, but this is at the top of my list for after that!

waiting-for-dev commented 5 years ago

Thanks for the feedback @timriley . No worries, I understand open source time is gold :)

waiting-for-dev commented 5 years ago

By the way, in a third thought (sorry, I think my brain also works through steps) I see that the on_failure proposal would not work for the typical rollback mechanism: open a block and then raise to rollback. So I think we are left with the around:, which I would use encompassing all the steps.

Just wanted to let this comment here but no worries about the timing.

waiting-for-dev commented 5 years ago

Hey @timriley , sorry for the push, but did you find the time to look into it?

I think dry-transaction is a very nice library with interesting potentialities. If we share the view about what it could become and it is ok with you, I'd like to help improving it and making progress towards its 1.0 version.

Surely this is not the best place to outline all the ideas I have, but it is just a summary and we should create separate issues in case you agree with them:

Dry::Transaction::T.new(step: op1, tee: op2).call
include Dry::Transaction(type: :maybe)
# ...
include Dry::Transaction(type: :try, catch: NoMethodError)
Dry::Monads::Result.check(true, "a") # => Success("a")
Dry::Monads::Result.check(false, "a") # => Failure("a")
Dry::Monads::Maybe.check(true, "a") # => Some("a")
Dry::Monads::Maybe.check(false, "a") # => None

@flash-gordon @solnic maybe you also have something to say about all of this.

timriley commented 5 years ago

Very sorry for the delay, @waiting-for-dev. I'll try get some thoughts to you about this next week.

flash-gordon commented 5 years ago

For inputs I'd rather add some simplest integration with dry-effects once it's out. Throwing in more options to step definitions looks like going in the wrong direction for this task. What I have in mind is this:

class Operation
  include Dry::Transaction(container: Test::Container)

  state :user # state is per-transaction-run

  step :find
  step :process

  def one(values)
    self.user = user_repo.find(values[:user_id])
    ....
  end

  def two(values) 
    user_repo.update(user.new(values))
  end
end

This can also be done with direct usage of Thread.current, I wouldn't be bothered by this fact since the semantics is identical to algebraic effects.

waiting-for-dev commented 5 years ago

Very sorry for the delay, @waiting-for-dev. I'll try get some thoughts to you about this next week.

Don't worry @timriley . Thanks for any feedback you can give me.

For inputs I'd rather add some simplest integration with dry-effects once it's out.

Thanks for your comments @flash-gordon . Hmm... I see the point of using effects at the transaction level, however I'm afraid it could turn step adapters into something too wild, as in fact you would be dealing with steps of a chain of bind operations on a monad transformer. But it certainly could be a great addition. However, I see it as something orthogonal to just accepting previous outputs as inputs in steps downstream. A lot of times you just need to use the output of a previous operation, but this output is not a state, it is just an intermediate value you want to get ride of at the end of the chain execution.

flash-gordon commented 5 years ago

Well, here I would what's the the need of this? Are your transactions that complex they require some visibility rules applied? If so then it's probably something wrong with the code itself. We definitely don't want the DSL to be a smaller buggy version of ruby as a result.

waiting-for-dev commented 5 years ago

The transaction doesn't need to be complex at all and the code doesn't need to have something wrong. Surely I can explain it better with an example:

input.bind do |i|
  process_input(i).bind do |pi|
    log_process(i, pi).bind do
      Success(pi)
    end
  end
end

Or an alternative formulation of the problem: the state would not help for operations which does not require any input:

input.bind do |i|
  persist(i).bind do |id|
    increase_db_counter.bind do
      Success(id)
    end
  end
end
timriley commented 5 years ago

Hi @waiting-for-dev, I appreciate all the thinking and energy you've contributed to this project! However, after reflecting on your ideas, and on the position of dry-transaction within the dry-rb family in general, I think everyone would be best served by adopting dry-monads' Do notation for building sequences of dependant operations. Do notation already supports the flexibility you've been discussing here, when it comes to passing multiple different output objects to subsequent steps.

Given this, we've decided to bring active development of dry-transaction to a close. See #127 for more info. If this is the kind of feature you'd like to bring into the world, you've be most welcome to fork and rename the project and continue work on it :) Thanks again! ❤️

waiting-for-dev commented 5 years ago

Thanks for your answer @timriley !

Do notation already supports the flexibility you've been discussing here, when it comes to passing multiple different output objects to subsequent steps.

Yeah, do notation provides with an imperative way of dealing with bind chains. However, the idea I found worth of consideration was being able to adapt outputs to the needed monad shape along with easily define those steps as standalone classes to easy its testing an reuse. I thought that if Ruby had something to offer to the well known monads world was this help coming from a DSL.

you've be most welcome to fork and rename the project and continue work on it

That's a possibility. I'll report here any decision I make.