adomokos / light-service Goto Github PK

At one point, we needed key aliasing for our actions to avoid 1 line Actions. You can find more about it here.

Now that we have Orchestrator functionality in Organizers, I think this feature is obsolete. We could accomplish the same thing with an execute actions.

So instead of doing this:

class SomeAction
   aliases :my_key => :key_alias
end

We could do this:

class SomeOrganizer
  extend LightService::Organizer

  def self.call(id)
    with(:id => id).reduce(actions)
  end

  def self.actions
    [SomeAction1,
     SomeAction2,
     execute(->(ctx) { ctx[:key_alias] = ctx[:my_key] }),
     AnotherAction]
  end
end

See how the execute block now is responsible for aliasing.

Let me know if you're NOT OK with deprecating (and eventually removing in 1.0) key aliasing.

Users of LS are confused about what happens when context.fail! or context.skip_all! is called from an action. We need to add a couple of tests around this + a section in documentation has to be added.

Is this expected behavior?

class Action
  include LightService::Action
  promises :something

  executed do |ctx|
    ctx.something = nil
  end
end

ctx = LightService::Context.new
Action.execute(ctx) # => {:something=>nil}

Basically, you can circumvent giving something to a promised key by setting it to nil. Based on a variety of ruby literature, and blogs, avoiding nil can save me from a lot of trouble. I'm not sure if this should be part of light-service though.

In the README there is an example action:

class CalculatesOrderTaxAction
  extend ::LightService::Action
  expects :order, :tax_percentage

  executed do |context|
    order.tax = (order.total * (tax_percentage/100)).round(2)
  end
end

This doesn't actually work, order has to be accessed through context, it's not available directly in the executed block.

I've actually hit this issue myself and it would be nice to be able to reference the items in the context that have been defined with expects without the prefix..

My workaround at the moment is to assign the variables myself:

class CalculatesOrderTaxAction
  extend ::LightService::Action
  expects :order, :tax_percentage

  executed do |context|
    order = context.order
    tax_percentage = context.tax_percentage

    order.tax = (order.total * (tax_percentage/100)).round(2)
  end
end

Do you think this kind of sugar is worth adding?

The idea of Orchestrators was born when we had way too much logic in a Rails controller action. It made a lot of sense then and there, but as we used it more, we realized it just has unnecessary complexity.

I'd like to kill Orchestrators and add all its functionality to Organizers, that way an Orchestrator could be a series of Actions:

[
  FindsClientAction,
  SubmitsOrderAction,
  SendsNotificationsAction
]

or have more complex workflows:

[
  FindsClientsAction,
  iterates(:clients, [
    MarksClientInactive,
    SendsNotificationAction
   ]
]

All this work is happening in the 1.0 branch,
and it won't be backward compatible.

Right now, LS will break with AS5. I specified "< 5.0" for now, but we should make it work with and without 5.0.

I'm not sure how you want this to be used. Most open source projects in github use MIT though. http://opensource.org/licenses/MIT

I can't just create a PR though. This is your call. :)

We've been using LightService in the context of a resque job, and I want to know your take on handling errors.

For quite some time, we've been doing it in this form (consider this a use case):

class SomeOrganizer
  @queue = :high

  include LightService::Organizer

  def self.perform
    begin
      with(context).reduce [SomeAction]
    rescue SomeError => e
      # do something unique
      raise e, "modified message different from e.message"
    end
  end
end

class SomeAction
  include LightService::Action
  executed do |context|
    begin
      # do some action
    rescue AnotherError => e
      # do something unique
      raise e, "unique message"
    end
  end
end

It's quite tedious to do this for every action/organizer, so maybe we can do this in the topmost organizer:

class SomeOrganizer
  include LightService::Organizer

  def self.execute
    begin
      with(context).reduce [SomeAction]
    rescue SomeError => e
      # do something unique
      raise e, "modified message different from e.message"
    rescue AnotherError => e
      # do something unique
      raise e, "unique message"     
    end
  end
end

... which begins to clutter when we rescue a lot of errors.

Here's one option we can implement to make it cleaner and less nested:

In Rails, there's rescue_from

http://apidock.com/rails/ActionController/Rescue/ClassMethods/rescue_from

If we copy that, then maybe something like this could work:

class SomeOrganizer
  include LightService::Organizer
  rescue_from SomeError, :with => :do_something
  rescue_from AnotherError, :with => :do_something_else

  def self.execute
    with(context).reduce [SomeAction]
  end

  def self.do_something(e)
    # do something unique
    raise e, "modified message different from e.message"
  end

  def self.do_something_else(e)
    # do something unique
    raise e, "unique message"     
  end

end

This is different from set_failure! since we may not have reign over the services we use, e.g. SomeAction is a Service Object using a 3rd party client-gem raising some custom error class. We can force ourselves to use set_failure!, but we'd end up using rescue to catch the errors anyway.

I embarked on a refactoring of my app yesterday and began incorporating expects/promises into my actions. I like not needing to fetch everything, but as I refactored I kept causing spec failures.
The spec failures return the following:

     Failure/Error: FetchesInAppPurchases.for_game(game)
     LightService::ExpectedKeysNotInContextError:
       expected :games to be in the context
     # ./app/services/fetches_in_app_purchases.rb:13:in `for_game'
     # ./spec/services/fetches_in_app_purchases_spec.rb:20:in `block (3 levels) in <top (required)>'

See the backtrace names the service (specifically my call to with) but it is difficult to determine which action of the six is having the problem. I have no recourse but to keep close track of the context while refactoring, double-checking the available keys or inserting tracer statements to delete later.

I didn't know about the Kernel#fail method, though apparently it is an alias for Kernel#raise. What I don't understand is why the exception raised by ContextKeyVerifier, cleanly called in the body of the singleton execute method, traces to the call to ::with. I think it's because the following call to #with is inside the Organizer module and I guess rspec cuts out anything in the backtrace not in the project directory.

I think the thing to do is to return the name of the Action in the error message. Sorry I don't have any more time to investigate this now.

Right now a reducer only has the ability to handle a single around_each handler (https://github.com/adomokos/light-service/blob/master/lib/light-service/organizer/with_reducer.rb#L11-L14) It would be awesome if it was possible to have multiple around_each handlers specified, similar to a middleware chain in rack.

Let's say we have a generic action that expects a user to be supplied in the context.

Later, we create a new Organizer and for the context of that Organizer and its associated Actions, instead of user we care about the idea of an owner.

However, I want to use the generic action that expects the context to have an user. Currently, this causes me to have to create a small, specific action to convert the idea of an owner to an user:

class ExampleAction
  expects :owner
  promises :user

  executed do |context|
    context.user = context.owner
  end
end

This can quickly get out of hand though. Alternatively, at the time I create the owner object, I could associate it with an user at the same time:

class ExampleAction2
  expects :params
  promises :owner, :user

  executed do |context|
    context.user = context.owner = User.new(context.params)
  end
end

The trouble I have here is that our use of an action in this Organizer requiring an user leaks into an action that is really focused on the idea of an owner, which means that a more generic action meant to be reused is now leaking into other actions in order to be reused.

I'd like to propose we add the idea of expects aliases to LightService:

class ExampleAction3
  expects :user
  alias_context :user, :owner

  executed do |context|
    context.user.behavior!

    # which is equivalent to saying:
    # context.owner.behavior!
  end
end

What I hope ExampleAction3 demonstrates is how LightService can alias an object in the context
with another object in the context, and correctly send messages to the aliased object in the context when the aliased object exists. If there is no aliased object in the context, then expects and the context validation works as normal.

@adomokos, what do you think about a call interface as well?

MyOrganizer.call(1, 2)
MyOrganizer.(1,2) # apparently, you can do this!

Right now you can mix-in an organizer or action specific logic by "including" the module, like this:

class SomeOrganizer
  include LightService::Organizer

  def self.for(user)
  end
end

This is silly, the logic mixed into the class will be at class level, and the default for that in Ruby is "extend". We should phase out "include" with a warning and just use "extend" going forward.

Thoughts?

module LightService
  module Organizer
    #...
    def with(data = {})
      @context = data.kind_of?(::LightService::Context) ?
                 data :
                 LightService::Context.make(data)
      self
    end
    #...
  end
end

Why can't it be something like:

module LightService
  module Organizer
    #...
    def with(data = {})
      @context = LightService::Context.make(data)
      self
    end
    #...
  end
end

or ...

module LightService
  module Organizer
    #...
    def with(data = {})
      @context = LightService::Context.return_or_make(data)
      self
    end
    #...
  end
end

We are thinking about using LS for ETL processing. However, instrumentation is key, and measuring how long each action takes is important to us.

I am thinking about exposing 2 events from the LS pipeline.

1. Before Action
2. After Action

That way we could hook into the events and measure the execution. I would also think that this kind of events would be beneficial for others to extend LS.

This is how it would look like:

class SomeOrganizer
  extend LightService::Organizer

  def self.call(user)
    with(:user).reduce(actions)
  end

  def self.actions
    [
      OneAction,
      TwoAction,
      ThreeAction,
      FourAction
    ]
  end

  def register_event_handlers
    for(OneAction).before_action do |ctx|
      puts "OneAction before action event"
    end

    for(OneAction).after_action do |ctx|
      puts "OneAction after action event"
    end
  end
end

This way extending LS wiht custom event handling mechanism is feasible. I would love to use this functionaity as an "around advice"-like benchmarking in my actions.

Has anybody tried doing something like this before? If your answer is yes, what did you do? Maybe we could use that in LS.

If not, what do you think about this approach?

Thanks!

As our business requirements and code complexity grows, we see the need of weaving together Organizers just like we do with Actions. We have conventions: an Action can not call other Action(s), an Organizer can not invoke other Organizer(s). This is how we try to keep our code simple and easy to follow.

We have been calling multiple Organizers from controller actions which is a nice way of pushing out logic from Rails, but it seems it's not enough. We could build new organizers that reuse all the actions from the other organizers, but I am not sure that's the best solution, as the Organizer is responsible for one specific logic. For example, one Organizer handles Payment, the other communicates to an external API if the Payment was successful.

We've been considering "Orchestrators" or "Super Organizers", that would treat Organizers as higher level Actions. With this change the Organizers have a uniform interface, they might express expects and promises as well.

Do you have any thoughts on this?

We've recently had a few cases where this happens

class CoffeeMaker
  include LightService::Organizer

  def self.execute
    reduce(GrindsCoffee,SkimsMilk,AddsMilk,AddsSugar)
  end
end

Of course, if you don't SkimsMilk, you'll never AddsMilk. Right now, we're doing it like this:

class AddsMilk
  include LightService::Action
  expects :cup, :milk

  executed do |ctx|
    if ctx.milk # set to nil in `SkimsMilk` so that `AddsMilk` doesn't raise errors
      ctx.cup.add ctx.milk
    end
  end
end

It would be nice if we could do this:

class AddsMilk
  include LightService::Action
  expects :cup
  skip_if_no :milk # now we don't need to set `ctx.milk = nil` in `SkimsMilk`

  executed do |ctx|
      ctx.cup.add ctx.milk
  end
end

skip_all! doesn't fit our use case because we should still have some ☕ even without milk.

I added the rollback feature to LS a while ago. I have not used it since. Is this something people are using?

I am considering removing it in the next major version.

I just saw your most recent commit: 972c83f

Just want to ask why calling to_s within a string interpolation is safer than letting the interpolation call to_s for you.

http://stackoverflow.com/questions/10076579/string-concatenation-vs-interpolation-in-ruby

Right now, LS does not enforce a method name for organizers. You need to use the executed block in action, which generates the execute method, but there is no guideline for organizers. In fact, I've done this in the past:

FindsUser.by_id(id)

I am proposing a rule, that would warn the user to name the organizer's entry method like this:

FindsUser.by_id(id)
# Warning: The organizer should have a class method `call`.

And this warning wouldn't be triggered when the organizer has a call method.

FindsUser.call(id)

We began our work on "Orchestrators" ( see #65 for more details ), and we need to have a class method with a predetermined name an orchestrator can call in the call chain.

In the past week, I've observed two developers being confused with the use of ctx.fail!.

Consider this code:

class FindsUserAction
  extend LightService::Action

  expects :params
  promises :user

  executed do |ctx|
    ctx.fail!("user_id wasn't in the params") unless params.keys.include?('user_id')
    
    ctx.user = User.find(params[:user_id])
  end
end

They were surprised to see that the code after the ctx.fail! in this action got executed.
Yes, as you need to explicitly return from the executed block by calling next:

class FindsUserAction
  extend LightService::Action

  expects :params
  promises :user

  executed do |ctx|
    if params.keys.include?('user_id') == false
      ctx.fail!("user_id wasn't in the params")
      next ctx
    end 

    ctx.user = User.find(params[:user_id])
  end
end

After seeing the engineers got tripped on this one, I am of the opinion, that the first example should behave as the developers expect it: when fail! is invoked, it should return from the execution context immediately without explicitly calling next ctx.

example:

class CalculatesTax
  include LightService::Organizer

  def self.for_order(order)
    with(order: order).reduce \
      [
        LooksUpTaxPercentageAction,
        CalculatesOrderTaxAction,
        ProvidesFreeShippingAction
      ]
  end
end

Do you test for the return value of LightService::Organizer, or do you test if the LS::Actions are called in the order actions receive .execute?

Referencing #9

The use case: In a LS::Action, I want to delete some unused values inside context before handing it off to the next action in sequence. context can get very messy

e.g.
If there are 10 actions and each adds a key-value pair in context, by the 10th action, there's at least pairs, not all of which are used at all.

When we test an action we first create a context like this

ctx = LightService:Context.make({:arg1 => 'arg1', :arg2 => 'arg2'})
Action.execute(ctx)

Maybe it will be better if we can just pass in the hash of args when we call execute like this

Action.execute({:arg => 'arg1', :arg2 => 'arg2'})

then the hash will be converted into a LightService::Context in the background

Thoughts?

We are not exposing :outcome in the Context, we should remove that.

Sometimes, I'd like to be able to add custom key on the context especially when you have condition in your service.

Example:

class Contributions::Create
  extend LightService::Action

  expects :contribution, :user
  promises :redirect_to

  executed do |context|

    if context.contribution.wire_transfer?
      service = CreateWithWireTransfer.for_project

      if service.success?
        context.redirect_to = nil # <= I'd like to be able to not set this
      else
        context.fail!(service.message, service.error_code)
      end
    elsif context.contribution.credit_card?
      service = CreateWithCreditCard.for_project

      if service.success?
        context.redirect_to = service.redirect_to
      else
        context.fail!(service.message, service.error_code)
      end
    end
  end
end

I came across this situation today: a series of action completed, there was no failure but an action completed in a state that I needed to communicate back to the caller.

Right now, LS supports successes and failures. When you fail an action, it immediately stops the processing for the remaining actions. I think we should be able to push the context into a warning state that would not stop the processing for the rest of the actions but could bubble up its message as a warning to the caller.

Summary:

Trying to use a ContextFactory for testing an Organizer with a with_callback (and possibly other methods) will lead to full execution of the Organizer to build the returned context.

Failure recreation:

# create a new organizer Callback and Action
module TestDoubles
  class CallbackOrganizer
    extend LightService::Organizer

    def self.call(number)
      with(:number => number).reduce(actions)
    end

    def self.actions
      [
        AddsOneAction,
        with_callback(CallbackAction, [
          AddsTwoAction,
          AddsThreeAction
        ])
      ]
    end
  end

  class CallbackAction
    extend LightService::Action
    expects :number, :callback

    executed do |context|
      context.number += 10
      context.number =
        context.callback.call(context).fetch(:number)
    end
  end
end

RSpec.describe TestDoubles::AddsTwoAction do
  it "does not execute a callback entirely" do
    context = LightService::Testing::ContextFactory
                      .make_from(TestDoubles::CallbackOrganizer)
                      .for(described_class)
                      .with(:number => 0)

    # add 1, add 10, then stop before executing first add 2
    expect(context.number).to eq(11)
  end
end

# => Failures:

  1) TestDoubles::AddsTwoAction does not execute a callback entirely from a ContextFactory
     Failure/Error: expect(context.number).to eq(11)

       expected: 11
            got: 16

Technicals:

What is going on is the ContextFactory enumerates through the duplicated organizer.actions, however when used with a with_callback(FooAction, [ SomeStepAction ]) the callback action and subsequent steps get flattened into a lazily executed Proc here.

Therefore a ContextFactoryOrganizer.organizer .actions will never have the action constant in the array to match on.

Proposed Solution:
The ContextFactoryOrganizer is going to have to get smarter and build out an Action execution path tree for these steps and wrap them in a new organizer that halts at the expected Action

Based on issue #1, context should work like a hash with additional features. We encountered a problem in using some of LS::Context's methods such as #delete. The solution can be any of the ff.:

1. we enlist all methods that you want to expose, or
2. delegate everything to the instance variable @context if method is missing.

Let me know if this was intended by design.

Is it possible if I could test an LightService::Action using rspec without having to depend on LightService::Context?

Thanks for writing this gem!

Hi,

i need to use organizzer in instance class, i try this

class Test 
extend LightService::Organizer

def call
    with({}).reduce(
        Hello
    )
end

but i receive undefined method `with' for ...

I try also use

include LightService::Organizer

but i receive :
DEPRECATION WARNING: including LightService::Organizer is deprecated. Please use extend LightService::Organizer

My question is how can instance one class ad use organizzer inside this class.

Thanks

Hi i just want to ask why orchestrator does not raise failing context unlike organizers? is this intended or is there another way to identify whenever an organizer/action fails when executing steps in orhcestrator?

Steps required:

1. Move special cases into wiki
2. Make README refer to wiki

This is the recommended naming pattern. I chose light-service based on a blog post I read at the time when I started LightService, but I think now that the name is misleading.

I am not alone with this unique naming convention. The project RestClient follows the same naming convention.

I would be curious to see what other folks are thinking about the possibility of renaming the gem from light-service to light_service.

Wrote a tentative page using http://github.com/rstacruz/flatdoc.

http://padi.github.io/light-service

Can't seem to make a pull request on a non-existing gh-pages branch

      def with(data = {})
        @context = data.kind_of?(::LightService::Context) ?
                     data :
                     LightService::Context.make(data)
        self
      end

All specs still pass even when it's changed to this:

      def with(data = {})
        @context = LightService::Context.make(data)
        self
      end

a quick check on spec/organizer_spec.rb says #with should not create a context given that the parameters is a context.

PR coming soon...

@adomokos, I saw an internal tool used at Github that is a lot like LightService. One nice they had was this notion of Promises and Expectations. Used the following way:

class FrothMilk
  include LightService::Action
  expects :milk, :cup
  promises :frothed_milk_in_cup

  executed {...}
end

You can immediately tell what this action is expects from the context, and what this action promises to set in the context. An organizer would find this useful because it can see the chain of actions and check if they're compatible by looking at the expectations and promises.

What do you think? Would you like this included in LightService?

Usually, the order of actions are dependent on previous actions, but I caught myself rearranging orders where it makes sense. However, sometimes the action has to be invoked after another one.

Would it make sense to introduce an after macro that would express this dependency?

So the action would look like this:

class SomeAction
  extend LightService::Action

  after SomeOtherAction
  expects :user
  promises :client

  executed do |ctx|
    ...
  end
end

And when somebody places this action into a workflow where the SomeOtherAction was not invoked prior to SomeAction#execute call, it will throw an error.

I am hesitant to add this logic as it could be abused, but maybe, somebody else bumped into this before.

Let me know what you think!

https://gist.github.com/ngpestelos/5779508

Please let us know if this example is OK.

I just want to start a high level conversation about how LightService could support concurrency, agnostic of the underlying Ruby interpreter -- eg. how can we run multiple actions, organizers, or orchestrators "at the same time", allowing the interpreter to schedule the work appropriately.

I feel like this would be an important next step in advancing this project, either in practice or in conceptualization.

I had to do this recently:

class SomeAction
  includes LightService::Action
  expects :one_thing
  promises :the_other_thing

  executed |context| do
    unless context.one_thing.ok?
      context.fail!("I don't like what happened here!")
     end

    context.the_other_thing = "Everything's good"
  end
end

However, I received a PromisedKeysNotInContextError when I had to fail the context as it was early in the process and the_other_thing was not added to it yet.

I should be able to push the context into a failure state regardless of the promised keys being in the context or not.

Referencing: #1

Testing forces the gem user to "have to know" the internals of LightService i.e. discover that failure? should be stubbed everytime a hash is to be passed under execute. Ideally, LightService::Actions should be testable individually without having to stub the context hash everytime.

While reading through the source isn't necessarily bad, an extra indirection can waste developer time.

Is coercing a hash to be a context a good idea? It does make it more developer-friendly but I'm not entirely sure if that violates some other principle.

I am thinking about (deprecating first) and then removing error codes. I don't think it's widely used, it was added for a specific use case in the past.

Would anybody be against that?

As I added documentation for the context#fail! and context#skip_all! methods, I realized that the name of "skip_all" is misleading. It really should be "skip_rest" or simply "skip" as the organizer is not going to skip all the actions, only the remaining ones.

when i use LightService::Organizer i activate the logger:
LightService::Configuration.logger = Logger.new(STDOUT)
and i see the log work fine

module AppMeter
  class Meter
     extend LightService::Organizer

    def self.run()
      with({}).reduce(
        [
          InitAutoit,
          InitBrowser,
          MinimizeDos,
          OpenSite,
          ClickConsenti
        ]
      )
    end
end
end

but in when i use Orchestrator i don't see nothing

module AppMeter
  class Meter
     extend LightService::Orchestrator

    def self.run()
      with({}).reduce(
        [
          InitAutoit,
          InitBrowser,
          MinimizeDos,
          OpenSite,
          ClickConsenti,
          iterate(:links, [DownloadFiles])
        ]
      )
    end
end
end

my program work fine but i don't see the log.

Summary

Including a ctx.skip_remaining! in a reduce_until step causes an infinite loop of execution.

Recreation

class OrchestratorTestSkipState
  extend LightService::Orchestrator

  def self.run_inside_skip
    with(:number => 1).reduce([
                                reduce_until(->(ctx) { ctx[:number] == 3 }, [
                                             TestDoubles::SkipAllAction,
                                             TestDoubles::AddOneAction ])
                              ])
  end
end

it 'skips all the rest of the actions' do
  result = OrchestratorTestSkipState.run_inside_skip

  expect(result).to be_success
  expect(result[:number]).to eq(1)
end

Another useful feature I saw in Github's internal tool (see #30), was parallelizing of actions. What do you think about:

class MakeCapuccino
  include LightService::Organizer

  def self.run
    with({}).reduce(
      parallel(
        FrothMilk,
        MakeEspresso,
      ),
      CombineMilkAndEspresso,
    )
  end
end

Hey,

Really happy about orchestrators so far !

I just noticed that the context passed to an organizer from an iterator didn't include the singularized key (:link in my case), but did for an action...

I read the acceptance spec but it only test it with an action too. I will take a look to write another spec.

Have a good day

I'm not sure how to confirm this, but there are suspicions about this line:

To make my test failures more meaningful when asserting result.success?, I'd like to throw an error when ctx.fail! is called in my test environment. I believe this is best done by adding a callback/hook to an orchestrator that would run when the orchestrator fails. I'm opening this issue to present an idea of how that could work, and welcome people to discuss it (and then I'd be glad to implement it).

Would we rather have something naive like this:

class CalculatesTax
  extend LightService::Organizer

  def self.call(order)
    with(:order => order).reduce(
        LooksUpTaxPercentageAction,
        CalculatesOrderTaxAction,
        ProvidesFreeShippingAction
      )
  end

  def self.after_fail
    # Do something
  end
end

or something a bit more like Active Record does callbacks, like this:

class CalculatesTax
  extend LightService::Organizer

  after_fail :do_something

  def self.call(order)
    with(:order => order).reduce(
        LooksUpTaxPercentageAction,
        CalculatesOrderTaxAction,
        ProvidesFreeShippingAction
      )
  end

  def self.do_something
    # Do something
  end
end

I'm not sure a given orchestrator is likely to have as many reasons to stack several same-type callbacks on top of each other, so I'm not sure anything beyond the naive solution is needed; but I'd welcome people's take about it.