trailblazer / trailblazer-operation

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

adding additional steps in wrap #61

Open Quantenradierer opened 2 years ago

Quantenradierer commented 2 years ago

Hey,

We have a rails engine which defines business processes. A few other projects use this engine and make minor changes to the business processes, depending on the specific application.

Now the engine defines a wrap around a few steps, while a project wants to add a step in this wrap:

# defined in the engine:
module Concerns::TestOperation
    extend ActiveSupport::Concern

    included do
      step Wrap(Transaction) {
        step :load_something
        step :save_something
      }
    end
end

# defined in the project:
class TestOperation < Trailblazer::Operation
  include Concerns::TestOperation

  step :change_something, after: :load_something
end 

This of course won't work when :load_something is wrapped.

I already looked into various documentation about after, before and even subprocess patching but I can't find anything how to add/replace/delete steps in a Wrap?

Thank you

yogeshjain999 commented 2 years ago

Hey @Quantenradierer!!

The reason before, after etc doesn't work with Wrap is because passing set of steps to Wrap creates isolated anonymous operation and injects it into your operation, similar to what Subprocess does.

So Wrap is like an shortcut to define and inject nested operation with addition to calling wrapper . In above case, after searches steps which are present in current operation and not in nested or wrapped operation.

How about you use inheritance here to achieve similar thing ?

# in engine
class BaseOperation < Trailblazer::Operation
  step :load_something
  step :save_something
end

# in your project
class TestOperation < BaseOperation
  step :change_something, after: :load_something
end

# in your operation
step Wrap(Transaction) {
  step Subprocess(TestOperation)
}
Quantenradierer commented 2 years ago

Thank you for the response. I have thought about this as well and in the few cases for transactions or exceptions this is okay.

But for metrics which shall document if a operation failed/succeeded/raised an error it would add one additional class since we need to wrap all operations.

This is our current implementation for metrics, which overwrites the call from operations:

      def call(*, **kwargs)
        result = super
        metrics(success: result.success?, error: nil, **kwargs)
        result
      rescue StandardError => e
        metrics(success: false, error: e, **kwargs)
        raise
      end

Is there a reason why after and before do not support 'chained'-ids?

like:

step Wrap(Transaction), id: 'wrap1' {
        step :load_something
        step :save_something
      }

step :change_something, after: {wrap1: :load_something}

I am not familiar enough with the code to implement this yet but maybe in a year or so there will be a PR, if you think this belongs into trailblazer :)

Can be closed once you answer.

yogeshjain999 commented 2 years ago

Good point about the "chained" ids!! But I think patching is more explicit and configurable. What if you want to add a step in nested operation which is 2 level deeper using after ? 😉

Current implementation of id has been kept simple to use plain objects and use patching instead to explicitly modify given operation without mutation.

If you want to avoid defining extra classes for Wrap, how about below example taken from patching documentation and extended to use Wrap ?

class BaseOperation < Trailblazer::Operation
  step :load_something
  step :save_something
end

class TestOperation < Trailblazer::Operation
  # Defining this as class method in order to access it in BaseOperation scope
  def self.change_something(ctx, **)
  end

  step Wrap(Transaction) {
    step Subprocess(BaseOperation, patch: -> { step TestOperation.method(:change_something), after: :load_something })
  }
end
apotonick commented 2 years ago

Doesn't patching work here? https://trailblazer.to/2.1/docs/activity.html#activity-dsl-options-patching This implements "chained IDs" the way @Quantenradierer need it.

Quantenradierer commented 2 years ago

@yogeshjain999 I don't think this is suitable for us.

One of our requirements is that the engine must define a whole usable process.


I have tried the patching @apotonick but it does not work for wraps. The class is being copied and modified (via Class.new) but the wrap (Wrapped) is an instance and can't be copied .

I also tried to modify the wrap activity without copying it and it worked.

This was just for testing purposes, don't use this code:

def customize in helper.rb

                if activity.is_a?(Trailblazer::Macro::Wrap::Wrapped)
                  patched_activity = activity.instance_variable_get(:@operation)
                else
                  patched_activity = Class.new(activity)
                end

I also looked into before/after parameters.def find_index(sequence, insert_id) in linear.rb returns an index to an id.

Allowing insert_id to be an array would also require to return an array of indices. This of course needs a lot of changes in other code.

apotonick commented 2 years ago

So, do we agree we have a patching bug here, then?

Regarding the insert_id points, we have the chance now to push something into the new dsl minor release. Let me think about it a bit.

apotonick commented 2 years ago

@Quantenradierer How would you use the insertion logic with an array? You mean you want to insert a bunch of sequential steps at a certain point at once?

apotonick commented 2 years ago

One of our requirements is that the engine must define a whole usable process.

Maybe you're misunderstanding patching? It is possible and designed to insert/alter as many steps as you need into nested operations, isn't that what you need? :joy:

Quantenradierer commented 2 years ago

@Quantenradierer How would you use the insertion logic with an array? You mean you want to insert a bunch of sequential steps at a certain point at once?

I would expect that

step Wrap(Transaction), id: 'wrap1' {
        step :load_something
        step :save_something
      }

step :change_something, after: [:wrap1, :load_something]

results in this:

step Wrap(Transaction), id: 'wrap1' {
        step :load_something
        step :change_something
        step :save_something
      }
Quantenradierer commented 2 years ago

One of our requirements is that the engine must define a whole usable process.

Maybe you're misunderstanding patching? It is possible and designed to insert/alter as many steps as you need into nested operations, isn't that what you need? 😂

Let's take the example:

class BaseOperation < Trailblazer::Operation
  step :load_something
  step :save_something
end

class TestOperation < Trailblazer::Operation
  # Defining this as class method in order to access it in BaseOperation scope
  def self.change_something(ctx, **)
  end

  step Wrap(Transaction) {
    step Subprocess(BaseOperation, patch: -> { step TestOperation.method(:change_something), after: :load_something })
  }
end

This won't do it, cause I need to define the wrap in the BaseOperation. And then I can't modify the inside of it in the TestOperation

apotonick commented 2 years ago

Thanks - that clarifies it! You'd use patch as follows to add/alter the nested OP (pseudo code!!!).

class TestOperation < Trailblazer::Operation
  step Wrap(Transaction), patch: [:base_operation] -> { step added_step, after ... }]

We need to test this, though. Anyway, the idea is to be able to change internals from TestOperation and not somewhere deeper. I think that's what you need, right?

apotonick commented 2 years ago

Yo @Quantenradierer, moin!

I am envisioning a more intuitive syntax for patching, how about this?

class TestOperation < Trailblazer::Operation
  patch: ["wrap/Transaction", :base_operation] -> { step added_step, after ... }]

Something like that, so you don't have to repeat step (Wrap(Transaction)) and instead provide its ID.