Make sure nil is accepted as cached value
#505
Conversation
zerc
force-pushed
the
504-last_transition-cache-doesnt-work-for-newly-created-objects
branch
3 times, most recently
from
a51bc2d
to
8678111
Compare
|
Can you please add a test case that fails without this PR and passes with? I can't see how this makes any meaningful difference in behaviour |
It checks the number of db queries done when there are no state transitions yet.
If there are no previous transitions `nil` will be returned and should be cached. Otherwise, the caching doesn't work for the initial transition.
zerc
force-pushed
the
504-last_transition-cache-doesnt-work-for-newly-created-objects
branch
from
8678111
to
8b8a9bb
Compare
| before { adapter.create(:x, :y) } | ||
|
|
There was a problem hiding this comment.
In tests of the #last method we were always creating a transition. This is not how it happens in reality.
| expect(adapter.last).to eq(nil) | ||
| expect(adapter.last).to eq(nil) | ||
| end | ||
| end |
There was a problem hiding this comment.
This test verifies that the caching of the #last method works even when there are no transitions been made yet.
| else | ||
| @last_transition ||= history.last |
There was a problem hiding this comment.
tl;dr; this caching approach doesn't work when history.last returns nil
When the object is just created and there are no sate transitions, history.last returns nil because:
transitions_for_parent.order(:sort_key) => []
transitions_for_parent.order(:sort_key).last => nil
and then it's stored in @last_transition variable.
If we call #last again (which happens when there are multiple guard_transition against the initial state transition) this statement:
@last_transition ||= history.last
will call history.last again which would result in the database query (which will return zero rows).
|
@Tabby I've added the test and some comments to clarify the problem. |
zerc
deleted the
504-last_transition-cache-doesnt-work-for-newly-created-objects
branch
This was introduced in #505 and caused an error being thrown if one attempts to reset the cache of `last_transition`.
This was introduced in #505 and caused an error being thrown if one attempts to reset the cache of `last_transition`.
This was introduced in gocardless#505 and caused an error being thrown if one attempts to reset the cache of `last_transition`.
Context
If there are no previous transitions then
nilwill be returned and it should be cached. Otherwise, during the initial transition multiple database queries will be made to get the last transition (which doesn't exist).See #504 for more details