Rollback Block Not Invoked In Iterated Actions

[redjoker011]

Hi I'm a big fan of LightService and been using this incredible gem for years, though upon creating a complex Organizer with multiple actions(combination of orchestrator methods such as reduce_if, iterate and many more functions) A colleague of mine notice that rolled_back block is not called on actions inside :iterate block(not sure with other helper methods from orchestrator) which based on docs should be invoked, Im not sure if this is a known issue or this is the intended behaviour ?

I've cloned the master branch and try to add test suite with LightService::Organizer#iterate below and the test failed to revert the number to zero which indicates that the rolled_back block inside LightService::Organizer#iterate has never called.

Just to add we're using the latest gem version, I hope This issue would be fixed and Thank You for this great gem.

# https://github.com/adomokos/light-service/blob/master/spec/acceptance/rollback_spec.rb
require 'spec_helper'

class RollbackWithIterationOrganizer
  extend  LightService::Organizer

  def self.call(number, times)
    with(number: number, times: 0..times).reduce(
      AddsOneWithRollbackAction,
      iterate(:times, AddsOneActionWithRollbackToRevertToZero),
      AddsTwoActionWithRollback,
    )
  end
end

class AddsOneActionWithRollbackToRevertToZero
  extend LightService::Action
  expects :number, :times
  promises :number

  executed do |ctx|
    ctx.number = ctx.number + 1
  end

  rolled_back do |ctx|
    # Block should be invoked
    ctx.number = 0
  end
end

class AddsOneWithRollbackAction
  extend LightService::Action
  expects :number
  promises :number

  executed do |context|
    context.fail_with_rollback! if context.number.zero?

    context.number += 1
  end

  rolled_back do |context|
    context.number -= 1
  end
end

class AddsTwoActionWithRollback
  extend LightService::Action
  expects :number

  executed do |context|
    context.number = context.number + 2
    context.fail_with_rollback!("I did not like this a bit!")
  end

  rolled_back do |context|
    context.number -= 2
  end
end

describe "Rolling back actions when there is a failure" do
  it "Increment using iterate and rollback to revert value to zero" do
    result = RollbackWithIterationOrganizer.call(1, 3)
    number = result.fetch(:number)

    expect(result).to be_failure
    expect(result.message).to eq("I did not like this a bit!")
    expect(number).to eq(0) # FAIL Should revert value to zero
  end
end

[redjoker011]

Actual Test in my local machine using Ruby 2.5

[adomokos]

@redjoker011 - thank you for reporting this missed functionality. 👍

It's funny that you are using a feature that I've built, but I've never used myself. That's why it has probably fallen through the cracks.

Would you be interested in submitting a PR to fix this? Your failing test is halfway there.

[redjoker011]

Let's see where I can help , I just need a little overview so I know where should I start

…

On Wed, Apr 29, 2020 at 7:19 AM Attila Domokos ***@***.***> wrote: @redjoker011 <https://github.com/redjoker011> - thank you for reporting this missed functionality. 👍 It's funny that you are using a feature that I've built, but I've never used myself. That's why it has probably fallen through the cracks. Would you be interested in submitting a PR to fix this? Your failing test is halfway there. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#188 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFI6JVFXG2YXU2E7OZMUPBDRO5P7BANCNFSM4MR6HKQA> .

[alessandro-fazzi]

I've noticed that too. My opinion is that it happens because while reducing all the rollbackable actions, we have

• AddsOneWithRollbackAction <-- Action
• iterate(:times, AddsOneActionWithRollbackToRevertToZero) <-- Proc
• AddsTwoActionWithRollback <-- Action

and the Proc does not respond to the rollback method. Does it make any sense?

[alessandro-fazzi]

Hello @redjoker011 have you ever had the chance to deepen this matter a bit?

[adomokos]

I added rollback to LS, but I admittedly never used it. What use cases are you using it for?

(The hard part about open source code is that I don't have insight into how it's being used/leveraged. I once had a zoom convo with a group of developers in Alabama, and they provided some context, which was helpful.)

[alessandro-fazzi]

Sincerly I'm not using it atm but I'd like to know how much reliable the feature is for future use cases. So I was corious if someone had spotted something out :)

[alessandro-fazzi]

I've newly been around this one for the sake of curiosity.

One note about the proposed spec: having

class AddsOneWithRollbackAction
  extend LightService::Action
  expects :number
  promises :number

  executed do |context|
    context.fail_with_rollback! if context.number.zero?

    context.number += 1
  end

  rolled_back do |context|
    context.number -= 1
  end
end

should bring to -1 as expected result:

• start = 1
• +1 = 2
• +(1 *4) = 6
• +2 = 8
• rollback!
• -2 = 6
• = 0
• = 0
• = 0
• = 0
• -1 = -1

If I am not missing anything obviously.

And about the organizer: I propose to stick to passing array of actions at least to simplify my playground (don't know what the interface is supposed to be...but I could read the code ;) ). So:

iterate(:times, [AddsOneActionWithRollbackToRevertToZero])

I've done an experiment with lib/light-service/organizer/iterate.rb and I'll share it even if I know I've used some trickery to write the less code possible. The worst thing should be that I've introduced an instance in the process because I needed to use state (not on context though)...I'm not able to catch if it is a design flaw ATM. That said, here it is my experiment:

require 'light-service/action.rb'

module LightService
  module Organizer
    Iterate = Struct.new(:organizer, :collection_key, :steps) do
      include ScopedReducable

      # While miming a Proc we want to respond to the :rollback message
      # proxying it to actions "contained" in the iteration
      def rollback(ctx)
        collection = ctx[collection_key]
        item_key = collection_key.to_s.singularize.to_sym
        collection.each do |item|
          ctx[item_key] = item
          steps.reverse_each do |action|
            action.rollback(ctx) if action.respond_to?(:rollback)
          end
        end

        ctx
      end

      # Mimic a Proc interface since the whole code expects this object will be a Proc
      # but it is no more :)
      def call(ctx)
        return ctx if ctx.stop_processing?

        collection = ctx[collection_key]
        item_key = collection_key.to_s.singularize.to_sym
        collection.each do |item|
          ctx[item_key] = item
          ctx = scoped_reduce(organizer, ctx, steps)
        end

        ctx
      end

      def self.run(organizer, collection_key, steps)
        new(organizer, collection_key, steps)
      end
    end
  end
end

And it seems to work against this test spec/acceptance/rollback_iteration_spec.rb

require 'spec_helper'

class RollbackWithIterationOrganizer
  extend LightService::Organizer

  def self.call(number, times)
    with(:number => number, :times => 0..times).reduce(
      AddsOneWithRollbackAction,
      iterate(:times, [AddsOneActionWithRollbackToRevertToZero]),
      AddsTwoActionWithRollback
    )
  end
end

class AddsOneActionWithRollbackToRevertToZero
  extend LightService::Action
  expects :number, :times
  promises :number

  executed do |ctx|
    ctx.number = ctx.number + 1
  end

  rolled_back do |ctx|
    # Block should be invoked
    ctx.number = 0
  end
end

class AddsOneWithRollbackAction
  extend LightService::Action
  expects :number
  promises :number

  executed do |context|
    context.fail_with_rollback! if context.number.zero?

    context.number += 1
  end

  rolled_back do |context|
    context.number -= 1
  end
end

class AddsTwoActionWithRollback
  extend LightService::Action
  expects :number

  executed do |context|
    context.number = context.number + 2
    context.fail_with_rollback!("I did not like this a bit!")
  end

  rolled_back do |context|
    context.number -= 2
  end
end

describe "Rolling back actions when there is a failure" do
  it "Increment using iterate and rollback to revert value to zero" do
    result = RollbackWithIterationOrganizer.call(1, 3)
    number = result.fetch(:number)

    expect(result).to be_failure
    expect(result.message).to eq("I did not like this a bit!")
    expect(number).to eq(-1)
  end
end

I feel like my experiment is a bit of a mess/patchwork, but I'm ok with it if it could be of inspiration. Never be shy about own code! 😄

[adomokos]

Thank you for taking a look at this @pioneerskies! I would support a draft PR that we could start iterating over, in case you don't have time to fully complete it. What do you think?

[alessandro-fazzi]

@adomokos here it is something to read. It's broken as expected 😓 but I'll wait for your first reaction there. I've seen where's the main problem, but I'll have to "study" in order proceed