[BUGFIX] ensure destroy-sync cleanup is correct #5438
Conversation
|
also cc @efx we had the mechanism "almost" correct in our previous attempt at testing for this, we were missing a key detail though: the push of new data must occur in the same run-loop as the unload. |
|
Ironically this is roughly what I think we will need (was reverted) #5058 EDIT they were previously un-reverted and then repurposed. However they turn out to be not what we need here. |
|
Ah, nice find! Do you recall the reason those changes were reverted? e.g. caused other issues? |
runspired
changed the title
There was a problem hiding this comment.
Looks good.
I have a minor cleanup comment but i'd be happy to merge without it.
I would like to know why the relationship payload changes from data: null to data: [] and want to confirm that this is just aesthetic and either one would work. If the relationship payloads are not equivalent i think it would be a regression.
| // The createRecord path will take _existingInternalModelForId() | ||
| // which will call `destroySync` instead for this unload + then | ||
| // sync createRecord scenario. Once we have true client-side | ||
| // delete signaling, we should never call destroySync |
There was a problem hiding this comment.
These comments are good but this logic relies on _internalModelForId and _existingInternalModelForId being called from separate code paths which is nonobvious and brittle.
I'm 👍 on merging this to get the fix in, but we'll want to clean this up.
There was a problem hiding this comment.
My hope is that once we have remove operations we will simply stop supporting unloadRecord + createRecord which is the only reason why this divergence exists. Instead you would be required to remove before the add to ensure the store fully forgets about the previous record by that ID.
| let knownBoats = store._internalModelsFor('boat'); | ||
|
|
||
| // ensure we loaded the people and boats | ||
| assert.equal(knownPeople.models.length, 1, 'one person record is loaded'); |
There was a problem hiding this comment.
These are fine, but better assertions would match against eg an array of ids rather than just the length as that would catch more regressions.
eg
assert.deepEqual(knownPeople.models.map(m => m.id), ['1'], 'one person record is loaded');i don't think it matters enough to not merge an important bug fix, but is an improvement we can make to the tests.
There was a problem hiding this comment.
(the above comment applies to the rest of the tests that assert against size)
| cars: { data: null } | ||
| cars: { | ||
| data: [] | ||
| } |
There was a problem hiding this comment.
Why does this payload change? these are both equivalent relationship payloads no?
There was a problem hiding this comment.
"cosmetic".
[] for empty collections, null for empty resource identifiers.
The spec is specific on this although we have been lax historically: http://jsonapi.org/format/#document-resource-object-linkage
There was a problem hiding this comment.
(the tests passed either way, just thought it was good to continue working us little by little closer to full spec)
|
Test failure is against ember-canary for the changes there that break our use of the |
resolves #5424
Bug
Unloading the record from a hasMany relationship immediately prior to pushing that same record with that same relationship state back into the store results in a disconnected internal-model being left behind in the has-many relationship.
Root Cause
When the unload and push are in the same run loop, we were calling
destroySyncto complete the destroy and before giving the user a newInternalModelinstead ofcancelDestroy. Because a deletion was not signaled,destroySyncdid not remove theInternalModelfrom relationships. This is semantically what we expect from destroy, minus the return of a brand new internal-model.Background
The
destroySynclogic branch was added to supportunloadRecordbeing immediately followed bycreateRecordfor a record with the sameID. This case is not one we wish to support long-term, once there is a clear ability for users to signal to the store that a record has been remotely deleted.Fix
The
createRecordcodepath utilizes_existingInternalModelForId()to check for existing internal models. This method is where we've added thedestroySynclogic previously.The
_internalModelForId()also implemented a check anddestroySynccall, but did so unnecessarily (poorly constructed test scenarios led us to believe this was correct when it wasn't). This meant that onpushwe also calleddestroySyncwhen what we wanted wascancelDestroy. We now callcancelDestroy.