gocardless / statesman

A statesmanlike state machine library.
https://gocardless.com/blog/statesman/
MIT License
1.78k stars 163 forks source link

last_transition cache doesn't work for newly created objects #504

Closed zerc closed 1 year ago

zerc commented 1 year ago

Hi there,

I was poking around and stumbled upon an interesting bug (as I think).

Steps to reproduce

  1. Pick up an active record model with Statesman::Adapters::ActiveRecordQueries enabled
  2. Create a fresh instance
  3. Call obj.last_transition multiple times

Expected behaviour: the database is only queried on the first call Actual behaviour: the database will be queried on every call

It only affects the newly created objects i.e. if you try the same with the object that has transitions already - it would work as expected.

Root cause

Looks like this line does not account for empty results and the history.last will always be evaluated and hitting the database (unless the transitions are preloaded).

Impact

If the object has multiple guards on the initial transition they will cause unneeded queries to the database on the initial transition.

zerc commented 1 year ago

The way I'm thinking it could be fixed is by having some UNSET singleton constant assigned to @last_transition property which would allow to adjust the condition as follow:

def last(force_reload: false)
  if force_reload
    @last_transition = history(force_reload: true).last
  elsif @last_transition == UNSET
    @last_transition = history.last
  else
    nil
  end
end

Would this make sense?