restrictions are not applied when more than one to-state is available #197

Closed
the8472 opened this Issue Jul 3, 2012 · 13 comments

Comments

Projects
None yet
2 participants

the8472 commented Jul 3, 2012

class MyClass
  state_machine :initial => :state_one do
    event :my_event do
      transition [:state_one, :state_two] => [:state_three, :state_four]
    end
  end
end

obj = MyClass.new
puts obj.class.state_machines[:states].event[:my_event].transition_for(obj,{:to => :state_four})
# <StateMachine::Transition attribute=:state event=:my_event from="state_one" to="state_three">

The matcher should probably intersect the whitelists if any is provided.

the8472 commented Jul 3, 2012

monkeypatch:

module StateMachine
  class Event
    def transition_for(object, requirements = {})
      assert_valid_keys(requirements, :from, :to, :guard)
      requirements[:from] = machine.states.match!(object).name unless custom_from_state = requirements.include?(:from)

      branches.each do |branch|
        if match = branch.match(object, requirements)
          # Branch allows for the transition to occur
          from = requirements[:from]

          to = if match[:to].is_a? StateMachine::LoopbackMatcher
            from
          elsif requirements.key?(:to)
            match[:to].filter(Array.wrap(requirements[:to])).first 
          else
            match[:to].filter(machine.states.map(&:name)).first
          end

          return Transition.new(object, machine, name, from, to, !custom_from_state)
        end
      end

      # No transition matched
      nil
    end
  end
end

Edit: Updated monkeypatch to account for the additional issues mentioned below

the8472 commented Jul 17, 2012

This is also an issue for

 transition :some_state => any

when combined with requirements restricting the to-state, as the values of an AllMatcher are empty it'll just transition back to the from state even though the requirements say otherwise.

This is basically some adverse interaction between the statelessness of the matchers, transition requirements and the branch matching.

Owner

obrie commented Feb 18, 2013

Thanks to the heads up -- using a matcher for a to state is not a valid use of the transition API -- it's only applicable to callbacks. I would instead be inclined to raise an exception for this case.

the8472 commented Feb 18, 2013

  1. We do use => any transitions for some administrative purposes, it's pretty useful to reuse the state machine logic (after/before callbacks) and still have the option to override the state.
    Also => same is a matcher too...
  2. This isn't just about the AllMatcher. It's also about passing arrays, which i assume get converted to WhitelistMatchers
  3. I think having multiple exit paths makes sense in combination with :to requirements. When no requirements are specified the first available path is taken. But with requirements, e.g. specified by the user, it's possible to take multiple routes. To have more flexible branching.
Owner

obrie commented Feb 18, 2013

@the8472 I'm a little confused. You can already specify multiple exit paths for an event like so:

event :activate do
  transition :inactive => :active, :if => :condition_one
  transition :inactive => same, :if => :condition_two
  transition :inactive => :frozen, :active => same
end

To say that a state can transition to from some state to multiple states doesn't make sense within state_machine -- it has no idea what state it should go to.

Maybe I'm missing something but I would have no idea how to parse this transition: transition [:state_one, :state_two] => [:state_three, :state_four] -- unless what you're really trying to do here is: transition :state_one => :state_three, :state_two => :state_four. That is, when the event fires how would it know what state to transition to?

the8472 commented Feb 18, 2013

You can already specify multiple exit paths for an event like so

You can also do this:

event :activate do
  transition :inactive => :active_one
  transition :inactive => :active_two
end

which I shortened to

event :activate do
  transition :inactive => [:active_one, :active_two]
end

Of course i do not expect the state_machine to have any smartness here, it can simply use the first available transition by default. But in our user-interface we provide the option to pick a different path with otherwise identical effects.

E.g. we have a review event that has the target states review_positive and review_negative (and maybe a bunch of others as the customer requires). The user starts the review transition, is offered multiple paths, picks one of them and enters a comment. The target state is then passed down as to requirement in the Event#transition_for method and the transition is executed.

The key point is that the side-effects of the event are the same (comment is persisted, other process participants get notified, ...), the pre-conditions are the same (must be eligible for review) and the only difference is the target state on which the process will fork into different pathways.

Owner

obrie commented Feb 18, 2013

I follow now what you're trying to do. It's a pretty unusual use case and, in most cases, indicates user error -- in most scenarios, your first example would result in logic that would never be reached unless you started interacting with some of the more advanced state_machine APIs.

Maybe what we need is to support this use case, but output a warning that the transition will never be reached using the basic event APIs. I think I need to dwell on this one for a little bit. Any strong objections to a warning in this case given how rarely this behavior is intended?

Owner

obrie commented Feb 18, 2013

Let me ask one other follow-up question: in most cases, folks would treat those two transitions as occurring under two separate events -- one that would result in active_one and one that would result in active_two. Have you considered this solution?

the8472 commented Feb 18, 2013

unless you started interacting with some of the more advanced state_machine APIs.

We use those almost exclusively. The instance methods are not versatile enough.

Any strong objections to a warning in this case given how rarely this behavior is intended?

If i can stub it out or something so nobody will be annoyed when they see it during application startup that should be fine. I'm aware that this is pretty advanced usage. We've built a lot of stuff around the state machine.

I would have submitted this as a pull request but there was a certain lack of activity so I wasn't sure if it would get accepted. If you're willing to cover these cases then I'll see if I can extract our monkey patches into a PR

Have you considered this solution?

Yes, but it was important that the paths have identical behavior. The diverging behavior would be in follow-up transitions. We also have some logic in our UI that allows for several events to be chained from the user-perspective. Some of our process steps have side-effects that don't interact gracefully with each other when executed at once, so some transitions are split into multiple steps.

We also considered storing the result in separate fields (think review_success), but that complicated the state machines with a lot of :if => :condition ... declarations that ultimately have the only purpose of introducing a fork in the available paths.
From a graph-theory perspective it felt more natural to simply have multiple exit edges from one vertex

Maybe it's a bit more obvious when i explain how we use state_machine

  1. we have a single controller that takes care of all state-machines for all classes and subclasses.
  2. we use cells for view components, which get bound to state machine events via configuration.
  3. the controller does a lot of reflection on the machine, events, branches, transitions, ... to figure out which components to present to the user, which paths are available, which objects need to be built and passed back to the event handlers, etc.
  4. we're trying to split processes into reusable logic elements and tie them all together in the state machine to describe processes. Those processes get quite complicated, involving several parallel state machines and forking paths.

This is how one of our simpler transition looks like:

multi path transition

obrie closed this in c80508b Mar 30, 2013

Owner

obrie commented Mar 30, 2013

Done :) Decided that I also shouldn't generate warnings here... hope this helps!

obrie was assigned Mar 30, 2013

the8472 commented Apr 3, 2013

1.2 looks good so far. One less monkey patch \o/

Owner

obrie commented Apr 4, 2013

Great to hear! :)

the8472 commented Apr 9, 2013

Ah, there seem to be some issues with the graph drawing. It doesn't handle the alternative target states.

I don't use the path finding algorithms (they seem to cause stack overflows or near-infinite loops in some cases), but those might be affected too.

Basically anything that does path-finding separate from the .transition_for() method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment