dry-rb / dry-transaction

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

Add around steps #85

Closed flash-gordon closed 6 years ago

flash-gordon commented 6 years ago

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
flash-gordon commented 6 years ago

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 commented 6 years ago

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 :)

flash-gordon commented 6 years ago

@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

flash-gordon commented 6 years ago

Yes!

ilnurnasyrov2 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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.