Skip to content
This repository

State machine and partial updates? #157

Closed
jviney opened this Issue · 10 comments

5 participants

Jonathan Viney Aaron Pfeifer Bob Larrick John Feminella Sergio Marques
Jonathan Viney

Since v1.1 state_machine no longer seems to work if ActiveRecord partial updates are enabled, because of this:

o = Order.new
o.state_event = :place
o.changed?

Previously, o.changed? would be true.

Is this intentional? How should this be done now?

Jonathan Viney

For anyone else with the same issue, here is a simple fix that worked for me:

module StateMachineFix
  def changed?
    super || (respond_to?(:state_event) && state_event.present?)
  end
end

ActiveRecord::Base.class_eval do
  include StateMachineFix
end
Aaron Pfeifer
Owner

Hey @jviney -

Thanks for asking about this. I've been spending a lot of time thinking about this issue. I don't know that I have a definitive answer, but let's see where this goes...

The first thing is that state_machine will still work with partial updates on even though changed? returns false. Since the event will end up getting fired as part of a before_validation hook, ActiveRecord won't check whether the record is dirty until after validations have run. So you'd see the following:

v = Vehicle.first           # => #<Vehicle id: 1, state: "parked">
v.state_event = 'ignite'    # => "ignite"
v.changed?                  # => false
v.save                      # => true
v.state                     # => "idling"
Vehicle.find(v.id).state    # => "idling"

If you're not seeing that behavior, then feel free to stop me here -- because the state should still get changed on save.

So, just to set the stage - the real issue we're talking about here is the fact that changed? returns false even though state_event has been set. Here are my thoughts on that (keeping in mind that we need to think about how this works across all ORMs)...

If we wanted changed? to return true, this means that we need to do one of three things:
1. Change state_event= and state_event to use write_attribute and read_attributes respectively. Using these methods causes AR/AM's dirty tracking to kick in.
2. Change state_event= to call #{state}_will_change!. This will cause the state attribute to show up in the list regardless of whether it actually gets changed.
3. Override changed? as you have in your workaround.

Option 1 is interesting, but fails on a few fronts. First, it doesn't work across all ORMs. Some ORMs use the attributes hash as the definitive list of key-value pairs to persist to the storage engine. This will cause column-based DBs to fail since the state_event column doesn't exist. Second, it exposes the state_event in a hash of attributes (as well as the list of changed columns) that are normally expected to only be columns from the DB table -- it's unexpected behavior that will eventually cause a separate bug to get filed :)

Option 2 has its quirks as well. Interestingly this is how it used to be implemented. The understanding was that if an event was going to be fired, the state will eventually change. This was ripped out because the support for a #{attribute}_will_change! method was fairly sketchy across the various ORMs and still is. Not only that, but it also makes it appear as though the state has actually changed when it hasn't.

Option 3, as you've proposed, is honestly an idea I hadn't considered before. I think it, too, has its quirks. I would expect that if changed? returns true, then there ought to be something in changes. However, nothing will be -- and that unexpected behavior is bound to cause other issues in the future. In addition, this also means overriding a core framework method which I tend to avoid.

One top of each individual option's disadvantages, all of these options mean that we could be causing the ORM to force the record to be saved when nothing ever actually changed. The most obvious (and perhaps only) case is an event that causes a loopback; the event succeeds, but the state never changes.

So those are the options that solve the issue you're seeing. They may all result in unexpected behaviors by other people who are using the library. None of them are really great choices that would be well-supported across ORMs. Option 3 is probably the closest if I had to choose one -- it would require due diligence to validate proper cross-ORM support.

So what's this all mean...

So really I haven't given you a definitive answer yet and that's because I haven't talked about the last thing that pushed me to my current decision. I prefer to follow an ORM's conventions whenever possible. I'll often look to it or other popular libraries on how to handle situations like this. My search for libraries fell a little short -- paperclip was the most obvious, but for various reasons was not the exact same situation. However, I turned to ActiveRecord's accepts_nested_attributes feature for some guidance. Consider the example below:

class Creative < ActiveRecord::Base
  belongs_to :campaign
end

class Campaign < ActiveRecord::Base
  has_one :creative
  accepts_nested_attributes_for :creative
end

c = Campaign.first  # => #<Campaign id: 1, state: nil>
c.creative          # => nil
c.attributes = {:creative_attributes => {:description => 'Test'}}
c.changed?          # => false
c.save              # => true
c.creative          # => #<Creative id: 1, description: "Test", campaign_id: 1>

In this example, you can see that ActiveRecord "suffers" from the same problem -- even though we've made changes to the record that will eventually cause a change in the database. While the Campaign, itself, didn't end up changing -- the creative_attributes still got applied to cause a new record to get created.

My conclusion from all of this is that the current behavior is the most expected behavior. When you set state_event, nothing has actually changed yet and so changed? returning false actually makes sense. It won't be until you attempt to save the record and cause all of the callbacks to run that you've actually caused the state_event to trigger.

So what do you do...

So my general suggestion is that you don't rely on changed? to determine whether you save a record. I'm sure it has its use cases elsewhere (I've certainly used it in other pieces of code). However, if you're potentially dealing with state events, then my suggestion is that you always attempt to save -- ActiveRecord will either detect that nothing has changed and skip the DB or we'll actually change the state causing the DB to be written to. Truthfully you could even run valid? first, which will cause the state_event to get triggered... then you could manually check changed? if you wanted to.

I'm open to feedback. If anything, I'll at least take a moment to clarify some of the behaviors in the docs. Let me know what you think and I hope this helps!

Best,
Aaron

Aaron Pfeifer
Owner

Closing -- will reopen if you have further thoughts on the subject.

Aaron Pfeifer obrie closed this
Jonathan Viney

Hi Aaron,

Thanks for the detailed reply.

I've clarified the problem we were having. The state_event had been assigned using nested attributes, and not directly on the model being saved.

checkout = Checkout.find(id)
checkout.orders_attributes = { "0" => { "state_event" => "place" } }
checkout.save!

In this case, only #changed? is called on the association records to see if they should be saved.

Options

I can see how all the options you laid out have their drawbacks.

Overriding #changed? as I had done does change the AR semantics because that method currently only applies to database attributes. A similar but better option may be to override #changed to include the new state event value, but even that isn't brilliant.

My preference is probably for option 2. I think it is reasonable to say that assigning a state_event is akin to saying that the state attribute will change (or at least is intended to be changed). Event the normal AR #changes method does not represent real changes - just changes that are intended to be saved later but have yet to pass validation etc.

I'll probably patch our application along these lines - would you accept a patch as well, or was a decision already taken to remove this behaviour because of the patchy ORM support and conveying that the state attribute had been changed even though it was still the same?

Thanks,
-Jonathan.

Aaron Pfeifer
Owner
obrie commented

Hey @jviney -

I haven't forgotten about this issue -- just haven't had time to address it. I'll reopen it just to remind myself that it still needs further consideration.

Thanks for following up.. I'll be sure to do the same soon.

Aaron Pfeifer obrie reopened this
Bob Larrick

Ugh, just got bit by this yesterday / today, lost about 4 hours to it.

Please either fix in some fashion, or make a note on the readme that it doesn't work when assigning nested attributes.

Edit:
Haven't fully tested it yet but here's the approach I'm taking
Added this to the model with the state machine definition.

  def state_event=(foo)
    @state_event = foo
    self.state_will_change!
  end
John Feminella
fj commented

+1 for fixing this. If it's true that it's this simple to address the problem then I hope the PR @jviney mentions is accepted with due speed.

Aaron Pfeifer obrie closed this in 79b6976
Aaron Pfeifer
Owner
obrie commented

It took a fresh look at this issue to finally build a solution that was a reasonable compromise :) This simply overrides #changed_for_autosave? to look for whether the event attribute has been specified. It also doesn't change the behavior of #changed? which means we continue to be consistent with ActiveRecord internals.

@jviney @fj sorry it took so long! This'll be part of the upcoming 1.2.0 release.

Sergio Marques

In ActiveModel if you update the state and then immediately check for the previous_changes it seems to work fine. Still if you add an after_commit callback and check for the previous_changes in it, they will be empty. I still think this is an issue.

Aaron Pfeifer
Owner
obrie commented

@lostie I think that's expected and is consistent with how these types of attributes get handled internally in ActiveRecord. You can think of it as behaving similar to nested attributes (e.g. you wouldn't expected to see something like avatar_attributes in the list of changed attributes). It's not exactly the same, but I think it's a similar concept. The fix here was to address the core problem which was that associations were not having their events processed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.