[BUGFIX] Fix issues with links #5257
Conversation
when one end of a relationship is marked {inverse:null} keep the two relationships separate in the cache
minor optimisation too
Don't overwrite links in cached payload when populating the other side of a relationship
|
This fixes #5235 (initially reported in #5219). This bug is blocking our ED upgrade. @BryanCrotaz Thanks a lot; feel free to include the #5235 failing test. |
?
| } | ||
| } | ||
| }) | ||
| }, 10); |
There was a problem hiding this comment.
I think we can skip the new Promise/run.later(..., 10) and simplify this method with just return Promise.resolve(...)
return Promise.resolve({
data: {
id,
type: 'user',
relationships: {
organisation: {
data: { id: 1, type: 'organisation' }
}
}
}
});
| } | ||
| } | ||
| }) | ||
| }, 10); |
| } else { | ||
| resolve({ data: [] }); | ||
| } | ||
| }, 10); |
There was a problem hiding this comment.
Same story here. Returning Promise.resolve(...) in each branch of this conditional should allow us to remove the run.later.
|
Overall this change looks good. I just had a few comments for some cleanup in the tests. |
|
@bmac changes made |
| } | ||
| return resolve({ data }); | ||
| } | ||
| })); |
There was a problem hiding this comment.
I am not sure about this approach for fixing the failing test, as it seems this introduces a change in the semantics: before no link was fetched, now it is when we get the kids relationship on line 138/153:
assert.ok(middleAger.get('kids').includes(kid));This essentially boils down to if a relationship should be considered loaded when link & data is provided 😔 .
For this PR it might be best to change the assertion so we check the content of the relationship in a side-effect free way:
assert.ok(middleAger.hasMany('kids').value().includes(kid));| this._cache[lhsKey] = newRelationship; | ||
| if (rhsKey && !this._cache[rhsKey]) { | ||
| this._cache[rhsKey] = newRelationship; | ||
| } |
There was a problem hiding this comment.
This makes the comment above the code outdated, as we are not always populating the cache with the same payload. It might makes sense to update the comment to reflect that...
|
@hjdivad @runspired would you have time to take a look at these changes? |
|
@pangratz I don't think this is necessary if we fix the inverse bug but I'll reconfirm. |
|
Any idea why this hasn't been merged/accepted yet? |
|
Mostly it's the wrong fix. |
|
thanks @runspired . Is there a valid fix elsewhere? Or is it still being considered / worked on. I had cherry picked this commit into the production version of ember-data we are using, and its been working fine, however I'd prefer to stay in line with what is going to get released. |
|
@mattraydub yes, #5230 |
|
@BryanCrotaz has #5230 resolved this issue? |
|
Haven’t hadn’t a chance to test it yet |
|
@pangratz there may be a separate issue but I dug in months ago and it most definitely did resolve this. |
|
@pangratz that test is wrong. Neither side specifies inverse: null. Does it still fail if you fix the test? Overwriting in the non null case is the expected behavior. |
|
Also cc @hjdivad, I suspect we could do a nicer thing here and merge links so as to keep both unless links is explicitly overwritten by the API |
|
@runspired uh la la. I can confirm that |
|
It's not clear to me why we must add |
|
@sly7-7 if you do not specify inverse null, then we auto-resolve the other side of the relationship to create a link between the two classes. This results in either side being able to set the state of the relationship, which the test is asking us to not do. |
Fix issue #5209 and #5211
when one end of a relationship is marked {inverse:null} keep the two relationships separate in the cache and don't create the ":" cache entry
minor optimisation in relationship-payloads.js - don't scan an array multiple times
In RelationshipPayloads, don't overwrite an inverse relationship if it's got links instead of data
Fixup nested relationships test which now needs to download its links
This should be backported to 2.14,15,16