Add around steps #85

flash-gordon · 2017-11-17T19:22:35Z

Around steps are supposed to be a default means for support for database transactions and similar things. If you know how rack middleware or rpsec's around blocks work, it's the same, otherwise, examples will follow. Around steps are useful when you need to keep track of some externally-provided resource which cannot be handled automatically. For instance, you may create a temporary file and want to clean it in any case, no matter if something failed or not. But the most common use case is DB transactions, no doubt. Handling them with around steps is still cumbersome but, nevertheless, more reliable and straight-forward than other options:

rollback = Class.new(StandardError.new)
AppContainer.register(:transaction) do |input, &next_step|
  result = nil
  begin
    AppContainer[:persistence].transaction do
      result = next_step.(Success(input))
      raise rollback if result.failure?
      result
    end
  rescue rollback
    result
  end
end

class ShinyTransaction
  include Dry::Transaction(container: App::Container)

  step :process
  step :verify
  around :transaction # starts a transaction
  step :persist_one
  step :persist_two
end

Here exceptions are used for control flow to rollback a transaction but this way both ActiveRecord and Sequel successfully handle this case. I'll have another look if we can come up with a better example for this. In any case, we could provide a default implementation for this case with a simple wrapper:

AppContainer.register(:transaction) do
  Dry::Transaction::ROMTransaction.new(AppContainer[:persistence])
end

Basic support
Add tests for different cases
Refactor
Add docs

These can be used for implementing database transactions and similar things

flash-gordon · 2017-11-18T19:14:52Z

Looks like all APIs is private so there is not much to document, we should add docs on the site instead.
@timriley I tested this against two apps using dry-transaction, everything seems to work as expected. I verified that transactions are successfully managed with the code similar to what I used in the specs, i.e. commits/rollbacks/exception handling, so pls have a look.

flash-gordon · 2017-11-18T20:22:37Z

lib/dry/transaction/step.rb

+        end
+      end
+
+      def once(next_step)


I know this is looking smarter then needed but distinguishing around steps from others would require changing the API of step adapters but I'm not sure if we want to do it this time, although we could.

How do you think things would look if you did change the step adapter API? If we're going to change it, now is kind of the time to do it, while we're still pre-1.0 and discovering (with the introduction of this step) the various different things we might want to be doing.

So AFAICT the reason for this is that the step adapter may call the &continue proc more than once? Would you mind pointing out where/how this might happen?

re API, I need to play with it a bit, I'll come up with an example later. Maybe adding something like yields? to the adapter would be enough
re once continue will be called once on every regular step and called twice on an around step: the first time in yield of the step definition and the second on the explicit continue.(result) call. TBH, having a second thought I realized this has some drawbacks, for instance, it doesn't allow to halt the execution in an around step and I imagine this could be useful somehow.

timriley

Thanks @flash-gordon, this is super neat and I'm really happy to see this kind of thing built into dry-transaction at last 🙏

Everything overall feels good to me. I left a couple of comments about minor things inline.

As for some more general thoughts:

When I first saw this PR I wondered about naming. e.g. if something is called around, shouldn't it be clear exactly around what it's wrapping? i.e. I thought to myself, wouldn't it better if we could do this...

around :transaction do
  step :persist_user
  step :persist_account
end

Instead of the linear approach of:

around :transaction
step :persist_user
step :persist_account

But I realise this'd probably add a significant degree of complexity to the DSL and could open up the possibility of a lot of errors if people tried to nest steps inside other steps that don't support this around-style wrapping.

So I guess we just need to make it clear in the docs that "around" really means "around all the subsequent steps".

What do you think?

Thanks for tidying up the specs, too. That's looking much neater.

timriley · 2017-11-22T11:19:51Z

lib/dry/transaction/step.rb

@@ -7,6 +7,7 @@ module Transaction
    # @api private
    class Step
      UNDEFINED = Object.new.freeze
+      NOOP = :itself.to_proc.freeze


I notice this same NOOP proc is called LOOPBACK inside Stack. Is the difference in naming intentional?

Yeah, sort of, I'd say they serve different purposes. In fact, NOOP is only needed for unit tests and we could pass it there instead

timriley · 2017-11-22T11:21:38Z

lib/dry/transaction/step.rb

+        end
+      end
+
+      def once(next_step)


So AFAICT the reason for this is that the step adapter may call the &continue proc more than once? Would you mind pointing out where/how this might happen?

timriley · 2017-11-22T11:24:50Z

lib/dry/transaction/step.rb

          broadcast :step_succeeded, step_name, *args
          value
        }.or { |value|
          broadcast :step_failed, step_name, *args, value
          Failure(StepFailure.new(self, value))
        }
+
+        continue.(result)


Do you feel this line might make more sense underneath step_adapter.call(self, *args, &continue) inside the with_broadcast do ... block above? Feels like this particular line isn't about broadcasting at all...

You're right, though we cannot have it inside the block because this would continue the execution before broadcasting and I believe we want to emit step_{succeeded,failed} before calling the next step.

timriley · 2017-11-22T11:39:19Z

lib/dry/transaction/stack.rb

+        @stack.(m)
+      end
+
+      def compile(steps)


Oh, I reckon it'd be good to drop this under a private, if you don't mind :)

flash-gordon · 2017-11-26T12:42:43Z

Speaking of blocks in the DSL, I thought about it too and abandoned the idea quite soon as this would complicate the understanding of how transactions work. I think it's better to have nicer docs where we should refer to Rack as a similar concept. Interestingly, if you apply this similarity further you may find out that having blocks in the DSL is equal to implementing a router engine :)

This commit changes the existign adapter API, however, the changes are essentially cosmetic. Step adapters don't get steps as input parameters anymore, this was clearly a mixing of concerns, as in, a step calls its adapter and then the adapter calls the step back. I added two simple wrapper classes called StepAdater and Callable. StepAdater simply wraps an adater object. Since we don't use inheritance (and this is awesome) we don't have common behavior between adapters besides the `.call` method. A StepAdapter instance stores step options and the actual operation to be called. It also dynamically determines whether the adaper yields a block thus make it possible to distinguish around-ish steps from before-ish ones. To do so it relies on the list of arguments of the adapter, if it has a block object then the adapter can control the execution. I know this might look unnecessary smart but I think there will be not that many around-like adapters, in fact, I think there will be exactly one so it's better to have the adapter interface thiner. Callable is a simple wrapper around the actual operation, it exists only for checking operation's arity. These classes simplified the Step class quite significantly and allowed me to separate flows for around-like adapters without any hacks.

flash-gordon · 2017-11-26T14:37:34Z

@timriley here we go 😅 It took me a few hours to come up with this variant of API. In fact, it didn't change too much. I extracted some code from the Step class and moved it to other objects, this finally solved the puzzle I had with two different execution flows. Instead of having that nasty once hack we now have a simple if statement. The new adapter API accepts a callable operation, its options and an array of arguments. Whether the .call method has a block parameter determines if the adapter manages the execution flow. It might look hacky too but I believe it's fine here because we're not going to have a lot of around-like adapters.

Also, I simplified the unit tests for adapters, they obviously had some redundant checks.

Hope you'll like it 😄 As always, I'll be happy to tweak things if needed

timriley

@flash-gordon I love it! I really like how you fixed the conflation of concerns between Step and StepAdapter too. I also agree that avoiding blocks in the DSL is for the best :)

Anyway, really happy with this. Are you OK for this to be merged in now?

timriley · 2017-11-26T22:06:17Z

lib/dry/transaction/stack.rb

@@ -2,7 +2,7 @@ module Dry
  module Transaction
    # @api private
    class Stack
-      LOOPBACK = :itself.to_proc.freeze
+      RETURN = -> x { x }


This is a nice naming improvement :)

flash-gordon · 2017-11-27T00:13:29Z

Yes!

ilnurnasyrov2 · 2018-01-30T17:06:03Z

But what if you want to make some dirty things like sending http request?

    Class.new do
      include Dry::Transaction(container: Test::Container)

      around :transaction
      step :persist_user

      step :send_http_request
    end

You wouldn't be able to use around because you don't wan't to make http request in db transaction.

flash-gordon · 2018-01-30T17:16:28Z

@ilnurnasyrov what's dry-transaction supposed to do in this case in your opinion? My personal advice is don't call external services from transactions, use event subscriptions.

ilnurnasyrov2 · 2018-01-30T20:14:11Z

Oh, i understand. I just thought that variant with block is awesome, because it is flexible.

    Class.new do
      include Dry::Transaction(container: Test::Container)

      around :transaction do
         step :persist_user
      end

      step :send_http_request
    end

flash-gordon · 2018-01-30T20:28:49Z

I'm not against this syntax but it's not that easy to implement without quite a bit of rewriting the gem. And I think, it's unlikely you're gonna have cases like this in real life, and if you are you always can add one more method for placing the transaction block there. I really do want people to report something meaningful/reasonable then I'll be happy to discuss and revisit the syntax.

flash-gordon added 2 commits November 18, 2017 16:46

Add basic implementation for around steps

0087f7b

These can be used for implementing database transactions and similar things

Add more specs for around steps

fc3651a

flash-gordon force-pushed the around-steps branch from 78d6d3e to fc3651a Compare November 18, 2017 13:46

flash-gordon added 2 commits November 18, 2017 17:42

Refactor stack class

220c3c2

Add unit tests

65ca2f4

flash-gordon requested a review from timriley November 18, 2017 19:14

flash-gordon changed the title ~~[WIP] Add around steps~~ Add around steps Nov 18, 2017

Refactor #once

a26a356

flash-gordon commented Nov 18, 2017

View reviewed changes

timriley reviewed Nov 22, 2017

View reviewed changes

flash-gordon added 2 commits November 26, 2017 17:01

Make Stack#compile private

67a204e

timriley approved these changes Nov 26, 2017

View reviewed changes

flash-gordon merged commit 67a204e into master Nov 27, 2017

flash-gordon deleted the around-steps branch November 27, 2017 08:29

timriley mentioned this pull request Nov 27, 2017

Share example of how a business transaction could be wrapped in a database transaction #29

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add around steps #85

Add around steps #85

flash-gordon commented Nov 17, 2017 •

edited

Loading

flash-gordon commented Nov 18, 2017

flash-gordon Nov 18, 2017

timriley Nov 22, 2017

timriley Nov 22, 2017

flash-gordon Nov 25, 2017

timriley left a comment •

edited

Loading

timriley Nov 22, 2017

flash-gordon Nov 25, 2017

timriley Nov 22, 2017

timriley Nov 22, 2017

flash-gordon Nov 25, 2017

timriley Nov 22, 2017

flash-gordon Nov 25, 2017

flash-gordon commented Nov 26, 2017

flash-gordon commented Nov 26, 2017

timriley left a comment

timriley Nov 26, 2017

flash-gordon commented Nov 27, 2017

ilnurnasyrov2 commented Jan 30, 2018 •

edited

Loading

flash-gordon commented Jan 30, 2018

ilnurnasyrov2 commented Jan 30, 2018 •

edited

Loading

flash-gordon commented Jan 30, 2018

Add around steps #85

Add around steps #85

Conversation

flash-gordon commented Nov 17, 2017 • edited Loading

flash-gordon commented Nov 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timriley left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash-gordon commented Nov 26, 2017

flash-gordon commented Nov 26, 2017

timriley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash-gordon commented Nov 27, 2017

ilnurnasyrov2 commented Jan 30, 2018 • edited Loading

flash-gordon commented Jan 30, 2018

ilnurnasyrov2 commented Jan 30, 2018 • edited Loading

flash-gordon commented Jan 30, 2018

flash-gordon commented Nov 17, 2017 •

edited

Loading

timriley left a comment •

edited

Loading

ilnurnasyrov2 commented Jan 30, 2018 •

edited

Loading

ilnurnasyrov2 commented Jan 30, 2018 •

edited

Loading