pluginaweek / state_machine

Adds support for creating state machines for attributes on any Ruby class
http://www.pluginaweek.org
MIT License
3.74k stars 510 forks source link

Transition in after :save hook fails in DataMapper (works in a second save) #164

Closed lsimoneau closed 11 years ago

lsimoneau commented 12 years ago

In our application, there are situations where we want to transition an object automatically from pending to accepted when it's created. I've tried to use a DataMapper after :create hook for this purpose, but for some reason the transition is not happening.

I moved the event into an after :save hook, which didn't help, except if you save the model a second time, in which case it does. I've created a Gist to reduce and demonstrate the problem: https://gist.github.com/1929985 (using the Vehicle example).

On first save, the state is still parked, but when saving again it transitions to idling. I've tried to dig through the code to see why this might be but I've been unable to come up with anything.

obrie commented 12 years ago

Hey @rssaddict -

Whelp, I'll admit you've thrown me a curveball here :) Thanks for bringing this issue up. As it turns out, DataMapper prevents you from calling save within a save callback. The idea is that it's preventing you from potentially going into an infinite loops that results in a stack too deep exception.

Of course, state_machine is currently relying on the ability to do exactly that. Under the hood, calling an event method actually uses attribute-based events when possible. This means that state_machine will set state_event to "ignite" and then attempt to save the object -- this will trigger the save callbacks that cause state_machine to run the event.

Currently, the only way I can think of to solve this is to not rely on attribute-based events in this manner for DataMapper. It's a fairly straightforward change, but I need to think about this one a little bit more because it'll make transitions behave slightly differently in DataMapper than other ORM integrations.

In the meantime, I might instead suggest triggering initial transitions like so:

class Vehicle
  include DataMapper::Resource

  state_machine :initial => :parked do
    event :ignite do
      transition :parked => :idling
    end
  end

  def initialize(*)
    super
    self.state_event = 'ignite'
  end
end

That may not be the exact implementation, but the idea is that you're setting a default event to trigger when the vehicle gets created. This is the way I typically implement it.

lsimoneau commented 12 years ago

Thanks. Setting the {attribute}_event in initialize works. Interestingly, it doesn't work if you set it in a before :save hook, which I don't quite understand.

Thinking about the larger issue, it makes sense not to save the record twice, so triggering transitions in after :save hooks might just be a generally bad idea. Maybe some sort of more general functionality that allows you to set conditions in which a record will be automatically transitioned prior to saving?

obrie commented 12 years ago

state_event actually gets processed prior to validation, not save -- that's why it doesn't work if you set it in a before :save hook. This is because we want to ensure that any state-based validations get run properly.

I think you're right about triggering transitions in after :save hooks just generally being a bad idea. The truth of the matter is that you can never save any changes in an after :save hook even when you're taking state_machine out of the equation. I added some documentation on this in the DataMapper integration to hopefully help people in the future.

It sounds like perhaps what you're proposing is the ability to set the initial state_event for the machine directly within the state_machine definition. It's definitely an interesting idea, but I'm going to have to think about that a little bit more and under what conditions it'll actually work.

asdavey commented 12 years ago

I'm wondering if the use of after :save hooks internally by state_machine is the cause of the problem I'm seeing: events raised in an after_transition callback silently fail to trigger a further transition.

If you have a look at the gist https://gist.github.com/2815992 I've included two models with state_machine, one with Datamapper, the other without. In my non-datamapper version, events fired in an after_transition callback work as expected, but in the datamapper version, the second event fails to trigger a transition.

obrie commented 12 years ago

Hey @asdavey -

You're definitely right. By hooking event transitions into around :save / around :validation hooks in DataMapper, this prevents state_machine from invoking multiple events during a single transaction for that record. However, there isn't much of a choice when you're using the state_event attribute for REST APIs. The only time at which we can evaluate that event is by hooking into one of DataMapper's callbacks. It's worth noting that this behavior is only a problem with DataMapper.

The only possible alternative here is to have a way to detect when the callback hooks won't work (i.e. we're already inside of a DataMapper save call) and changing state_machine behavior based on that. If anyone's interested in solving that problem, I'd be happy to take a look at a solution. It's honestly not on my short list right now.

asdavey commented 12 years ago

If I understand you correctly, at present a caller can set state_event but the transition won't occur until save is invoked.

I don't think I need that since I prefer to call fire_state_event (most of my events a triggered by the result of http calls). So as a workaround could I disable the default :action=>:save and instead trigger a save in a normal before_transition {self.save} callback?

obrie commented 12 years ago

You can certainly give that shot :) You'll want to make sure that you halt the transition if the record fails to save.

obrie commented 11 years ago

After revamping the integration for DataMapper, I've come to the conclusion that it's impossible to persist additional changes made to your record within an after :save callback. Because of the way DataMapper's run_once feature works for saving, it's just not possible to make changes in that callback. I'd encourage anyone who's interested in having this work (as it does in other ORMs) to follow up with the DataMapper maintainers.

Thanks for bringing it up! I've added a caveat in the README in case this comes up again.