trailblazer / trailblazer-operation

Trailblazer's Operation implementation.
https://trailblazer.to/2.1/docs/operation.html
87 stars 24 forks source link

Binary state methods for custom outputs. #63

Open elShiaLabeouf opened 1 year ago

elShiaLabeouf commented 1 year ago

Hello @apotonick and the Trailblazer team!

I think Adding outputs is a really powerful feature since it enables handling multiple outcomes during the operation run.

I also enjoy using operation.success? and operation.failure? methods that come in handy in controller actions routing.

But when I'm using a custom output in an operation, to check which pipeline the operation went, I have to do the following:

operation = Payment::Operation::Create.(provider: "bla-unknown")
if operation.event.to_h[:semantic] == :provider_invalid
  render "error"
else
  render "success"
end  

I think it would be more intuitive and kinda elegant if instead of operation.event.to_h[:semantic] == :provider_invalid I could use a binary state method operation.provider_invalid?

I'd like to come up with a proposal to add such methods to a number of classes/objects like #<Trailblazer::Activity::Railway::End::Success semantic=:success>, #<Trailblazer::Activity::End semantic=:paypal> etc..

I have tested my changes locally and it works like this:

require 'trailblazer/operation'

class Execute < Trailblazer::Operation
  UsePaypal = Class.new(Trailblazer::Activity::Signal)

  step :find_provider, Output(UsePaypal, :paypal) => End(:paypal)
  step :charge_creditcard

  def find_provider(ctx, params:, **)
    return true unless params[:provider] == :paypal
    UsePaypal
  end

  def charge_creditcard(ctx, **)
    ctx[:charged] = true
  end
end

op = Execute.call({ params: { provider: :paypal } })
op.paypal?  # => true
op.success? # => false
op.failure? # => false

op = Execute.call({ params: { provider: :not_paypal } })
op.paypal?  # => false
op.success? # => true
op.failure? # => false

The changes

To achieve this I had to add bits of code to three gems:

  1. First, I added success? failure? fail_fast? pass_fast? and <custom_output>? to Trailblazer::Activity::End:

    
    # trailblazer-activity-0.16.0/lib/trailblazer/activity/structures.rb
    module Trailblazer
    class Activity
    class End
      def initialize(semantic:, **options)
        @options = options.merge(semantic: semantic)
    
        define_singleton_method("#{semantic}?") { true } # <-- this
        %i[success failure fail_fast pass_fast].each do |name| # <-- this
          define_singleton_method("#{name}?") { name == semantic } # <-- this
        end  # <-- this
      end
2. Then I added the new custom output binary state method to `End::Success`, `End::Failure` objects in `Trailblazer::Activity::DSL::Linear::Normalizer::OutputTuples#register_additional_outputs` (which is called every time a new custom output is defined):
```ruby
# trailblazer-activity-dsl-linear-1.2.3/lib/trailblazer/activity/dsl/linear/normalizer/output_tuples.rb
            def register_additional_outputs(ctx, output_tuples:, outputs:, **)
              # this below
              default_ouputs = %i[success failure fail_fast pass_fast]
              railway_ends = ctx[:sequence].find_all { |(semantic, railway_end, *)| default_ouputs.include?(semantic) }

              output_tuples.each do |(output, connector)|
                railway_ends.each do |(semantic, railway_end, *)|
                  railway_end.define_singleton_method("#{output.semantic}?") { output.semantic == semantic }
                end
              end
              # ---
              output_tuples =
                output_tuples.collect do |(output, connector)|
              ...
  1. and finally changed the constructor, success?, failure? and added method_missing for delegation in Trailblazer::Operation::Result:
    
    # trailblazer-operation/lib/trailblazer/operation/result.rb

class Trailblazer::Operation class Result def initialize(event, data) @event, @data = event, data end

def success?
  @event.to_h[:semantic] == :success
end

def failure?
  @event.to_h[:semantic] == :failure
end

private def method_missing(symbol, *args)
  if @event.respond_to?(symbol)
    @event.send(symbol, *args)
  else
    super
  end
end

after that I tested the code with the example above and got those result.

---

I understand that the code I propose to add might seem complicated, but on the other hand in return we get:
- a **totally new** feature of binary state methods in `trailblazer-activity`.
- **new custom outputs' binary state methods** as an addition to the good old `success?` and `failure?` in `trailblazer-operation`.

The code above can be added to the Trailblazer family step by step, PR by PR and it won't break a thing. I'm willing to create this series of PRs.

Looking forward to hearing your thoughts on my proposal! 
apotonick commented 1 year ago

Hi @elShiaLabeouf, wow, you have lots of ideas, I like that! :green_heart:

Regarding your specific requirement, my suggestion would be to enhance Operation::Result, which is the actual object exposing #success and #failure. There is no need to change deeper gems (in my understanding), or am I wrong here?

One important thing to keep in mind is that calling operations is a concept that will happen in an endpoint in future versions of TRB - meaning that methods like success? will not be used on the user side. You configure your endpoints with a higher level API. This is the reason that only Operation has the binary result methods, these concepts don't exist in activity and dsl.

apotonick commented 1 year ago

Oh BTW, one more thing! An Output is not a terminus (or "end" as we used to call it). An output defines which outgoing connection goes where, and that might lead to a specific terminus, however, sometimes this might just define what will be the next step, so be careful not to confuse those two concepts.

I will soon explain more about the Wiring API in a Trailblazer Tales episode. :beers:

apotonick commented 1 year ago

Hey @elShiaLabeouf feel free to hit me up directly on https://trailblazer.zulipchat.com to discuss :point_up:

elShiaLabeouf commented 1 year ago

Oh, I see.

I've come up with this minimalistic PR. Please take a look.

I didn't realize the outputs were accessible from the operation class itself.

elShiaLabeouf commented 1 year ago

That was quite a bit of overengineering before 😂

eliten00b commented 4 months ago

Any update on this? I think it would be nice to check easier for different end semantics. Maybe just a one new method?

operation = Payment::Operation::Create.(provider: "bla-unknown")
if operation.success?
  render "success"
elsif operation.semantic == :provider_invalid # or operation.semantic?(:provider_invalid)
  render "provider_error"
else
  render "error"
end  
apotonick commented 4 months ago

We're putting this into the endpoint gem - I am working on it as I type, actually! Will update you in the next days! :green_heart:

apotonick commented 4 months ago

@eliten00b @elShiaLabeouf Here's a rough outlook of how I envision the interface: https://github.com/trailblazer/trailblazer-endpoint/blob/master/test/matcher_test.rb#L17 - it will look slightly less clumsy in a controller. Any comments here? :beers:

eliten00b commented 4 months ago

Does this mean any custom terminus is not consider a failure anymore?

apotonick commented 4 months ago

@eliten00b Interesting! A custom output has never been a "failure" per definition, that's why we gave it an attribute "semantic". It's up to the user to decide what "not_found" means. What makes you think that this is now different with the above example?

eliten00b commented 4 months ago

True, I never read that a custom terminus is consider a failure.

Most of our operation use the run Operation do; end in rails controllers. When we now started with custom terminus it skips the block. Which is ok, as it is not the ideal success or pass_fast semantic.

When I check Operation.().failure? it is true if the terminus is a custom one.

From your code it reads that at least the DSL#failure does not check the same as the Operation#failure? method. Which does sound ok to me, if the check become more fine.

From the initial code:

op = Execute.call({ params: { provider: :paypal } })
op.paypal?  # => true
op.success? # => false
op.failure? # => false

currently it would be:

op = Execute.call({ params: { provider: :paypal } })
# => <Trailblazer::Operation::Railway::Result:false>
op.event.to_h[:semantic] == :paypal # => true
op.success? # => false
op.failure? # => true
apotonick commented 4 months ago

@eliten00b Good point! Let me finalize my code over here and then we can discuss the API for endpoint. :coffee: