Data: Loading belongsTo removes relationship for other instances

[makepanic]

Hi,
after upgrading to the most recent data version (3.1.1 -> 3.5.0), I'm seeing different behavior when retrieving a inverse relationship.
That is as soon as I call instance.get('relationship') then the relationship model can't be used from any other instance that has the same relationship.

Setup

Given 2 models:

    class Person extends Model {
      @hasMany('dog', { async: false, })
      dogs;
    }

    class Dog extends Model {
      @belongsTo('person', { async: true, })
      person;

      @attr('string')
      name;
    }

Where:

• api returns a relationship without resource linkage for Person -> dogs
• api returns relationship with resource linkage for Dog -> person

Reproduction

assert.equal(dog1.belongsTo('person').id(), '1');
assert.equal(dog2.belongsTo('person').id(), '1');

await dog1.get('person');

assert.equal(dog1.belongsTo('person').id(), '1');
assert.equal(dog2.belongsTo('person').id(), '1'); // 💥

• https://github.com/makepanic/data/commit/3598ee45e70c263269923393d38875f86d6d3c62#diff-6d44095bc68c4170bdd69687536c84f4R4896

The issue sounds related to what https://github.com/emberjs/data/pull/5608 already fixed.

Versions

Run the following command and paste the output below: npm ls ember-source && npm ls ember-cli && npm ls ember-data.

ember-source: 3.5.0
ember-cli: 3.3.0

[runspired]

@makepanic could you PR that test commit?

[runspired]

@makepanic I realized that your report above is missing the ember-data version

[makepanic]

i've seen it in ember-data 3.5.0 and 3.4.2.
It doesn't happen in 3.1.1.

The PR with failing ember-data tests is on current master which was 3.5.0 afaik

[brandontksmith]

I can confirm this in 3.4.2 as well. I switched back to 3.1.1 without issues.

[makepanic]

I'm currently "bisecting" when this issue was introduced.
So far I can say:

• 3.2.0 ok
• 3.3.0 ok
• 3.3.1 not ok
• 3.4.0 not ok

As the issue was introduced from 3.3.0 to 3.3.1 it's most likely caused by https://github.com/emberjs/data/pull/5541

If one removes the added code from https://github.com/emberjs/data/pull/5541/files#diff-71266fb7fe629103f166f694e660e11aR685 it's ok again. This means that block of code is causing the issue.

If one removes the this.updateData(data, initial) it's not breaking the relationships.

[makepanic]

If anyone stumbles over this, here's the last status of this (via discord):

That fix fixed the opposite behavior for a ton of folks
Roughly speaking both the folks that it fixed something for and the folks that it broke are doing something bad
It’s a “fix” we intend to deprecate
Here’s the issue I’d opened for that:
https://github.com/emberjs/rfcs/issues/360

Ftr that fix restored an older implicit behavior we’d lost somewhere around 2.16/2.18, although it seems that in reality there were two implicit behaviors

[XaserAcheron]

Well, this was nasty -- this issue manifested as a random race condition in my app that's been driving me nuts all week. 9_6

Reading the above post and the linked RFC, I'm confused on what the state of this is. It's talking about a deprecation, but what's being _deprecated_, exactly? The current state of things is _broken_ -- we need a _fix_, not a deprecation. :P

For context, I was trying to adopt the async: false school of thought (i.e. preload data and avoid data-loads via model.get), but I just had to hit the abort switch. In several places I'm fetching a hasMany relationship via include: ... or similar, and if the included records was already in the store and had their own relationships, they got wiped. That's certainly not correct behavior.

Things technically work again now that I've gone back to async: true, but I miss not having to worry about proxy objects. :(

[EDIT] Spoke too soon. Things are broken in very weird, other ways with async: true now, 'cause a piece of the app was implemented by another dev using async: false from the get-go. tl;dr I gotta either fork ember-data or downgrade because of this one.

[makepanic]

I've rebased the failing test onto the latest master and noted that the "easy" workaround doesn't work anymore.

It looks like the payload got normalized before reaching the _setupRelationships method.
The methods now sees the payload relationship data as {data: [dog1]} which causes computeChanges to apply this new truth and remove the dog2 relationship.

This is caused by data calling ensureRelationshipIsSetToParent which takes the belongsTo source (dog1) and normalizes it onto the hasMany inverse record (person) to the resource linkage mentioned above.

I guess there are now more complex workarounds for this issue:

• don't normalize the resource linkage which should restore the old workaround
• ensureRelationshipIsSetToParent should normalize using all inverse records

[runspired]

I believe this is identical to #5866 which also presented a simple path forward available in #5880. Closing in favor of the issue with PR

[makepanic]

I've already checked if the PR solves the failing test when writing the last comment.
Sadly it didn't work due to the same reason described above.

I haven't have time to bisect it yet but it looks like changes to ensureRelationshipIsSetToParent ,possibly by https://github.com/emberjs/data/commit/8184519134615f709e18a29ba18b0cd638a7fcde , introduced the payload normalization which causes this case to never reach the branch that the PR fixes.

If I remove ensureRelationshipIsSetToParent from source, then the PR fixes the issue.

The tl;dr is basically (note incorrectly normalizing the data array):

Should i create a new issue for the incorrect resource linkage normalization?

[makepanic]

I don't fully understand the relationship data fixing yet, but only doing it if relationshipData != undefined, it completes all tests and also fixes this issue when rebasing the change onto https://github.com/emberjs/data/pull/5880 .

I'm not really sure if it's a good idea though as it potentially introduces a regression that 5608 fixed.
@runspired would you be ok with only fixing the inverse relationship data if relationshipData is not undefined?

[runspired]

@makepanic the caveat is that an incorrect relationshipData payload would still be allowable then (the intent was to get to the point we would assert).

That said, I trust the tests we added and if they pass with that change and you can add a test that covers the edge case we missed then I am all for it.

Looking into things I think there are two bugs here: the first is that we don't merge payloads sometimes when we should during the fixing.

The second is that sometimes the payload is going to be partial, and in that case we need a mechanism by which to apply the state to the store in a partial manner. This mechanism is coming in the form of operations over the next 6 months.