New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX] enable lazy-relationship payloads to work with polymorphic relationships #5230

Merged
merged 16 commits into from Feb 28, 2018

Conversation

Projects
None yet
8 participants
@runspired
Copy link
Contributor

runspired commented Oct 15, 2017

Once reviewed/approved, this should be backported to 2.14, 2.15, and 2.16 beta.

Resolves #5037 #5055 #5208 #5209 #5211 #5267 #5268

Problem

Ember Data 2.14 introduced lazy-relationship parsing; however, this parsing used left-side/right-side keying that was incompatible with polymorphic relationships. This resulted in payloads for newly encountered polymorphic types causing all previous payloads to be dereferenced.

In addition, a number of bugs were discovered where we were incorrectly setting up RelationshipPayloads as if there was an inverse when there was none, or where the inverse resolved to the key :. This PR fixes that from the perspective of RelationshipPayloads but part of the problem was a bug in our calculation of inverseFor which is resolved with #5261

Solution

This PR resolves these issues with three primary changes:

  1. relationship-info is now calculated and stored separately from the RelationshipPayloads objects. This not only should be a performance win for us by enabling us to avoid calculating out the info in several scenarios where we would have needed to repeatedly redo the work before, but helps to resolve lookup for scenarios where there is no inverse or the inverse is polymorphic. Additionally it cleans up many areas of the lazy-relationship code substantially to have this separation of concerns.

  2. RelationshipPayloads are now always cached on the manager by the resolved base-model-name instead of model-name. This enables us to confidently always utilize the correct RelationshipPayloads even when encountering a polymorphic type.

  3. The payloads held for a relationship in RelationshipPayloads are now stored in a two-level cache by model-name and id instead of a single-level cache of just id. This enables situations in which polymorphic siblings do not share a common unique-id constraint to work properly.

Tests

  • one-side polymorphic belongsTo / hasMany
  • both-sides polymorphic belongsTo / hasMany
  • multi-table polymorphism (e.g. when IDs are not unique across all polymorphic types)
  • reflexive polymorphic relationships
  • self-referential non-reflexive polymorphic relationships
@bmac

bmac approved these changes Oct 16, 2017

Copy link
Member

bmac left a comment

lgtm

@hjdivad
Copy link
Member

hjdivad left a comment

Looks great @runspired

Just noting the todos we talked about OOB

inverseRelationshipMeta.options !== undefined &&
inverseRelationshipMeta.options.polymorphic === true;

assert(`Both side of a relationship cannot be polymorphic`, !this._lhsIsPolymorphic || !this._rhsIsPolymorphic);

This comment has been minimized.

@hjdivad

hjdivad Oct 19, 2017

Member

as we discussed out of band, it's fine for both sides to be polymorphic

@@ -205,6 +209,22 @@ export default class RelationshipPayloadsManager {
let lhsKey = `${modelName}:${relationshipName}`;
let rhsKey = `${inverseModelName}:${inverseRelationshipName}`;

This comment has been minimized.

@hjdivad

hjdivad Oct 19, 2017

Member

as discussed out of band, in both cases we want the key to be the model name defined by the inverse relationship, when an inverse relationship exists

This comment has been minimized.

@hjdivad

hjdivad Oct 19, 2017

Member

so we should always create RelationshipPayloads with the base name and then add the actual model name

@@ -44,10 +44,32 @@ export default class RelationshipPayloads {
constructor(store, modelName, relationshipName, relationshipMeta, inverseModelName, inverseRelationshipName, inverseRelationshipMeta) {

This comment has been minimized.

@hjdivad

hjdivad Oct 19, 2017

Member

maybe s/modelName/baseModelName or something here as we want this to always be the base model name

@@ -126,14 +159,28 @@ export default class RelationshipPayloads {
}
}

_isSide(potentialMatches, modelName, relationshipName) {

This comment has been minimized.

@hjdivad
@@ -674,3 +678,72 @@ test('get can retrieve payloads with self-links in reflexive relationships', fun
let entry = this.relationshipPayloadsManager.get('user', 1, 'friends');
assert.deepEqual(entry, friendsPayload, 'self-link in reflexive relationship');
});

test('handles relationships where one side is polymorphic', function(assert) {

This comment has been minimized.

@hjdivad

hjdivad Oct 19, 2017

Member

as discussed out of band, need more tests

  • push subtype then push basetype
  • push basetype then push subtype
  • both sides poly

This comment has been minimized.

@danielspaniel

danielspaniel Oct 26, 2017

Any way I can keep this moving? Is it possible for me to merge tests into this feature branch? Never tried that before

This comment has been minimized.

@hjdivad

hjdivad Oct 26, 2017

Member

@danielspaniel you can open pull requests against runspired/data to branch fix-polymporphic-payloads and when they're merged it'll update this pr. I've done it before and it works surprisingly well.

@danielspaniel if you have time to contribute tests for this that would be super helpful. Thanks ^_^

This comment has been minimized.

@danielspaniel

danielspaniel Oct 26, 2017

I have all tests ready but @runspired added this to RelationshipPayloads constructor:

assert(`Both side of a relationship cannot be polymorphic`, !this._lhsIsPolymorphic || !this._rhsIsPolymorphic);

That is showstopper, since previously in ember data you could do that .. so I think that assertion is perhaps guarding agains something I don't know what.

But I removed that assertion and all ember-data tests passed EXCEPT the new test I wrote for "both sides polymorphic", so maybe that had something to do with it .. not sure

This comment has been minimized.

@danielspaniel

danielspaniel Oct 26, 2017

This feature ( lazy relationship loading ) is super complex and was implemented with very limited tests in the first place.
I am thinking it might be better to dump the feature, rather than keep patching it.

Was this your feature @hjdivad ? Is there a great value to it. If there is I am not seeing it.
Would you be up for dumping it/ removing it?

This comment has been minimized.

@danielspaniel

danielspaniel Oct 26, 2017

So, if a feature is not done properly and introduces very serious bugs, are you telling me you are reluctant to back out the feature because you would lose the nice performance gains?
That makes no sense.
First off, whoever wrote the feature should take responsibility and either fix it or remove it.
I am willing to help out as much as I can, but the way it was done really looks like spaghetti to me, so I can't do much to save it.
Secondly, if I wrote this feature, I would be embarrassed to keep it, if it broke ember-data. Does not really matter what kind of fun it offers if it is broken.

How long should this broken feature sit in ember-data and just kill off polymorphic relationship users?
Is it right to just wait around for some other work someone else is doing? Who knows how long that other thing Igor is working on will be done. Seems irrelevant to me, especially since he is not involved in this discussion much. It has only been you, me , Chris and Stephan ( saying he liked the original bug posting )

This comment has been minimized.

@runspired

runspired Oct 26, 2017

Contributor

@danielspaniel Sorry for being a bit quiet. Last week when @hjdivad and I discussed this PR OOB we discussed that assert. Polymorphic to Polymorphic in ED has always been very buggy, and I wasn't sure it really worked at all and I thought it'd be too difficult to achieve here; however, in that conversation David and I also arrived at a solution for supporting it. So long as we always resolve to the baseModelName on both sides, the approach this PR outlines will work for both sides being polymorphic as well. I will update the PR with that approach.

This comment has been minimized.

@runspired

runspired Oct 26, 2017

Contributor

@danielspaniel can you PR the tests you've added to my branch?

This comment has been minimized.

@danielspaniel

danielspaniel Oct 26, 2017

sure thing .. gimme a few moments

This comment has been minimized.

@danielspaniel

danielspaniel Oct 26, 2017

I submitted PR to your branch of ED just now

@hjdivad

This comment has been minimized.

Copy link
Member

hjdivad commented Oct 24, 2017

@runspired possibly also #5208

@runspired

This comment has been minimized.

Copy link
Contributor

runspired commented Oct 27, 2017

Updated to handle polymorphic-to-polymorphic relationships and with the help of @danielspaniel we now have more tests. I would like to clean up the testing a bit more but this is ready for another review pass by @hjdivad.

The only piece that concerns me is that we now MUST lookup the inverse greedily in a situation we did not need to before here: https://github.com/emberjs/data/pull/5230/files#diff-1c53e096d92ba3a647478f3f51185a01R178

I'm also sure now that we've got an implementation that covers the appropriate cases that this is due for some further refactoring and simplification.

@BryanCrotaz

This comment has been minimized.

Copy link
Contributor

BryanCrotaz commented Oct 27, 2017

Haven't looked at source in detail but have you tested with multiple levels of model extension?

E.g. A extends B extends C, with relationships introduced at each level?

@runspired

This comment has been minimized.

Copy link
Contributor

runspired commented Oct 27, 2017

@BryanCrotaz those cases introduce no additional complexity or scenarios. A relationship must explicitly declare that it is polymorphic, and it must use the base type's type when doing so.

@BryanCrotaz

This comment has been minimized.

Copy link
Contributor

BryanCrotaz commented Oct 27, 2017

@runspired :) will try it out

@danielspaniel

This comment has been minimized.

Copy link

danielspaniel commented Oct 29, 2017

This fix is still not complete enough for polymorphic relationships. I found that polymorphic self referential relationships don't work .. 2 options

  • I can add the failing tests to this branch, @runspired can send more brainwaves at the problem
    or
  • we can just give up now, rip out this lazy relationships manager and save everyone who loves polymorphism from dying an early death from alcoholism
@runspired

This comment has been minimized.

Copy link
Contributor

runspired commented Oct 29, 2017

Added more failing tests for the polymorphic self-reflexive case both with and without full-linkage in the push. Full linkage already worked, incomplete linkage was fixed by fixing the isReflexive test to use the baseModelName instead of the given modelName when setting up the RelationshipPayloads instance.

@runspired runspired force-pushed the runspired:fix-polymporphic-payloads branch from 4c24628 to b8589d6 Oct 29, 2017

@danielspaniel

This comment has been minimized.

Copy link

danielspaniel commented Oct 29, 2017

I guess you beat me to it .. I was working on it but you added a whole ton of self-reflexive tests that were the ones that were breaking for me

@danielspaniel

This comment has been minimized.

Copy link

danielspaniel commented Oct 29, 2017

actually check out my test ( in the PR ) .. it still fails ( and I just rebased )

@runspired

This comment has been minimized.

Copy link
Contributor

runspired commented Oct 29, 2017

I'm afk but reading those asserts I believe they assert the incorrect thing.

@runspired

This comment has been minimized.

Copy link
Contributor

runspired commented Oct 30, 2017

@danielspaniel ported and fixed up your test asserts, found the issue was with self-referential but non-reflexive relationships (as you setup). Added fix.

@danielspaniel

This comment has been minimized.

Copy link

danielspaniel commented Oct 30, 2017

Your getting ever so very close to fixing all this @runspired .. you are still hanging in there which is pretty awesome.

ok .. how's about this for some philosophical ramblings.

lets say we have polymorphic hats and the data is like this:

  const bigHatData = { data: { id: 1, type: "big-hat", attributes: { type: "big-hat" }, relationships: { "user": { "data": { "type": "user", "id": "1" } } } } };
  const smallHatData = { data: { id: 1, type: "small-hat", attributes: { type: "small-hat" }, relationships: { "user": { "data": { "type": "user", "id": "1" } } } } };

upon close inspection one sees that the two models have the same id ( 1 ) but they are different sub-types.

If you push the user and then these two hats .. the test to get the user hats fails ( only finds 1 .. not 2 )

yours/our tests are doing this

  const bigHatData = { data: { id: 1, type: "big-hat", attributes: { type: "big-hat" }, relationships: { "user": { "data": { "type": "user", "id": "1" } } } } };
  const smallHatData = { data: { id: 2, type: "small-hat", attributes: { type: "small-hat" }, relationships: { "user": { "data": { "type": "user", "id": "1" } } } } };

using a different id for every subtype and therefore they pass .. but I think that the first way should pass as well .. because it used to in the past and I think polymorphic models can have same id for different type

you want me to PR this failing test ?

or you want to give up now and get a free carton of fig newtons and whatever is behind curtain number 2

@runspired

This comment has been minimized.

Copy link
Contributor

runspired commented Oct 30, 2017

This is a hard one because this case has actually never worked properly. It seems to work on the surface but has long plagued us as a source of bug reports.

@danielspaniel

This comment has been minimized.

Copy link

danielspaniel commented Oct 30, 2017

I hear you, but I not sure what to tell the databases that are storing the polymorphic data ( different tables ) this way .. since they going to laugh at me if I ask for different id's tomorrow

@runspired

This comment has been minimized.

Copy link
Contributor

runspired commented Oct 30, 2017

@danielspaniel I would push some tests for this case, I will see what I can do but can't promise much atm.

@runspired

This comment has been minimized.

Copy link
Contributor

runspired commented Oct 30, 2017

@danielspaniel just a thought, but worst case it would be pretty easy for you to rectify the ID conflicts client-side by normalizing them to ${modelName}-${id} on the way in and then stripping the modelName while serializing on the way out.

@danielspaniel

This comment has been minimized.

Copy link

danielspaniel commented Oct 30, 2017

I guess I could, but so would everyone else .. and currently without the lazy relationship thing, if you just create stuff like that , it works .. since create bypasses the loading lazy thing-a-majigger.

runspired added some commits Nov 21, 2017

/**
Manages the payloads for both sides of a single relationship, across all model
instances.
Manages the payloads for both sides of a single relationship, across all model

This comment has been minimized.

@igorT

igorT Nov 22, 2017

Member

Are the whitespace changes on purpose?

@BryanCrotaz

This comment has been minimized.

Copy link
Contributor

BryanCrotaz commented Nov 30, 2017

Does this overlap with my PR #5268? It seems as though we're fixing the same problem in very different ways.

@runspired

This comment has been minimized.

Copy link
Contributor

runspired commented Nov 30, 2017

@BryanCrotaz 100% overlap

@BryanCrotaz

This comment has been minimized.

Copy link
Contributor

BryanCrotaz commented Nov 30, 2017

My solution seems simpler but then I think I must therefore be missing something...

@runspired

This comment has been minimized.

Copy link
Contributor

runspired commented Nov 30, 2017

Check the various test cases added :P

unload(modelName, id, relationshipName) {
this._flushPending();

if (this._isLHS(modelName, relationshipName)) {
delete this._lhsPayloads[id];
delete this.lhs_payloads.delete(modelName, id);

This comment has been minimized.

@runspired

runspired Dec 5, 2017

Contributor

this is a mistake?

This comment has been minimized.

@runspired

runspired Dec 5, 2017

Contributor

seems sketch this passes tests

@method
*/
@method
*/
_isLHS(modelName, relationshipName) {

This comment has been minimized.

@runspired

runspired Dec 5, 2017

Contributor

Can you please rewrite/add to the comments here

if (isRelationship === true) {
return isSelfReferential === true || // itself
modelName === relInfo.lhs_baseModelName || // base or non-polymorphic
relInfo.lhs_modelNames.indexOf(modelName) !== -1; // polymorphic

This comment has been minimized.

@runspired

runspired Dec 5, 2017

Contributor

please add a comment here

@danielspaniel

This comment has been minimized.

Copy link

danielspaniel commented Dec 31, 2017

I guess this never made it to 2.18 .. any idea when this might get merged ?

@cibernox

This comment has been minimized.

Copy link
Contributor

cibernox commented Jan 3, 2018

I believe I'm hitting this. Is there any more work to be done?

@BryanCrotaz

This comment has been minimized.

Copy link
Contributor

BryanCrotaz commented Feb 7, 2018

Has this stalled? What’s left to do? Can we all help?

@igorT igorT merged commit b93b3e7 into emberjs:master Feb 28, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
inverseCache.set(modelName, relationshipName, info);

// Greedily populate the inverse
inverseCache.set(inverseBaseModelName, inverseRelationshipName, info);

This comment has been minimized.

@BryanCrotaz

BryanCrotaz Feb 28, 2018

Contributor

in #5268 I did this (line 284-288) by only using baseModelName as a cache key - I think that would mean that you don't have to greedily populate the inverse - all polymorphic types would use the same key.

This comment has been minimized.

@runspired

runspired Feb 28, 2018

Contributor

@BryanCrotaz that turns out to be error prone

This comment has been minimized.

@runspired

runspired Feb 28, 2018

Contributor

(I originally did that too)

This comment has been minimized.

@BryanCrotaz

BryanCrotaz Feb 28, 2018

Contributor

Why can't the world just be perfect? Gah.

@BryanCrotaz

This comment has been minimized.

Copy link
Contributor

BryanCrotaz commented Feb 28, 2018

Congrats. This is a biggie.

bmac added a commit to bmac/data that referenced this pull request Mar 1, 2018

[BUGFIX] enable lazy-relationship payloads to work with polymorphic r…
…elationships (emberjs#5230)

* [BUGFIX] enable lazy-relationship payloads to work with polymorphic relationships

(cherry picked from commit b93b3e7)

bmac added a commit to bmac/data that referenced this pull request Mar 1, 2018

[BUGFIX] enable lazy-relationship payloads to work with polymorphic r…
…elationships (emberjs#5230)

* [BUGFIX] enable lazy-relationship payloads to work with polymorphic relationships

(cherry picked from commit b93b3e7)

bmac added a commit that referenced this pull request Mar 1, 2018

[BUGFIX] enable lazy-relationship payloads to work with polymorphic r…
…elationships (#5230)

* [BUGFIX] enable lazy-relationship payloads to work with polymorphic relationships

(cherry picked from commit b93b3e7)
@bmac

This comment has been minimized.

Copy link
Member

bmac commented Mar 5, 2018

I've back-ported this fix to 2.14.11, 2.15.4, 2.16.4, 2.17.1, 2.18.2, and 3.0.2.

@danielspaniel

This comment has been minimized.

Copy link

danielspaniel commented Mar 5, 2018

This was a good weekend. Thanks for the merge and backports.

@kennethlarsen kennethlarsen referenced this pull request Mar 26, 2018

Merged

Ember 3.1 Release Blog Post #3230

1 of 13 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment