unloadRecord of child record causes hasMany relationship to be invalid using [email protected]

[sudowork]

Example repo and integration test to reproduce the issue

• ember-data 2.13.1 (broken)
• ember-data 2.12.2 (working)

Update: I wrote a test case to test this behavior sudowork@89a34ba. I bisected the issue to have started with 6d96eda (from #4593).

Update 2: Converted the repo I had to an ember-twiddle now that I can load 2.13: https://ember-twiddle.com/df7fa30d2eb3d32efe16185f4f376128?openFiles=controllers.todo-list.js%2C

Steps to reproduce:

// models/todo-list.js
import DS from 'ember-data';
import { hasMany } from 'ember-data/relationships';

export default DS.Model.extend({
  todos: hasMany('todo', { async: false }),
});

1. Assume you have a model todo-list which hasMany todo items as todos already loaded in the store and is currently in scope.
2. Call todos.removeObject(todo) to remove the item from the relationship
3. Call this.store.unloadRecord(todo) to clean up the record from the store
4. Explicitly try accessing get(this, 'model.todos') again to load the todo records. (Expected result: [], Actual result: Assertion error thrown).

The assertion error thrown is due to this assertion that checks that manyArray.isEvery('isEmpty', false) on [null].

Gifs

2.13.1 (broken)

2.12.2 (working)

Note: On 2.12, it does not even go through getRecords() when retrieving the records again.

[sudowork]

Just briefly skimming the change log, this commit #4593 looks suspicious. However, I don't currently know enough about the ember-data internals/haven't dug into it enough to have an educated guess.

[sudowork]

I've written a test case for this here. Please let me know if my understanding of how this should behave is incorrect. sudowork@89a34ba

And I've bisected to verify that ba5874b does pass the test; however, 6d96eda (from #4593) fails the test.

[sudowork]

Calling clearRelationships() instead of destroyRelationships() here in InternalModel#dematerializeRecord appears to do have the correct behavior for the test I wrote. However, it does trigger extraneous arrayWillChange and arrayDidChange events on the manyArray for the relationship. I'm also not sure if it breaks any expectations with rematerializing.

[sudowork]

cc @hjdivad

[sly7-7]

I just experienced this as well. I think that unloading records of a hasMany leaves the underlying many array with internalModels with null records. I'm not sure this is intended, and don't know what we want for the relationship (does the many array should be cleared, or still contain 'empty' internalModels ?)

[Kilowhisky]

I had to rollback because i experienced this.

When i unload records it doesn't seem to fully clear reflexive relationships. Any time i tried to access a hasMany relationship in another model where one of the hasMany has been unloaded it explodes saying that the record is undefined. The 'unloaded' record still appears in the hasMany canonicalState but it has the flag isEmpty: true and isDestroyed: true. Reloading the record back into the store doesn't appear to fix this either.

I tried to make twiddle for this but twiddle hasn't been updated to 2.13 yet.

[sudowork]

I'm happy to attempt a fix for this as long as there's some agreement over what the correct behavior should be @sly7-7 @stefanpenner @hjdivad

[IBue]

Here is a little twiddle demonstrating the error in 2.13 when unloading an inverse hasMany-relationship:
https://ember-twiddle.com/243ca2dab9b4615dd67115fbb1d83881

ember-data version can be switched in twiddle.json

[sudowork]

Now that ember-twiddle supports ember-data 2.13, here's the repo I had created as a Twiddle: https://ember-twiddle.com/df7fa30d2eb3d32efe16185f4f376128?openFiles=controllers.todo-list.js%2C

Some interesting things:

• If I use createRecord to create the child, then there is no issue with unloading the record. It seems to only be when the record was created when pushing the relationship into the store.

[workmanw]

This is also a show stopper for us. I can appreciate the fact that it's nice to synchronize this unloadRecord action with the runloop to prevent redundancy. Unfortunately this broke in several things in our application. Most cases were as @sudowork stated, using computed.map or some similar helper to iterate over a list it never expected to be null.

Also, FWIW some of these were also using store.filter arrays. Which I know is deprecated, but still usable.

[workmanw]

Wondering if #5011 fixes this? I'll try in my app tomorrow.

[sly7-7]

@workmanw I thought the same thing, but I tried in my app, and for relationships, the internalModel has still their records set to null.

[sudowork]

@workmanw @sly7-7 I updated my test branch (#4987) to the latest master and the issue persists. I didn't attempt it yet in a real app.

[workmanw]

Yeap, I can confirm that unfortunately this issue still exists in our application w/ 2.14.2. We're stuck on ember-data-2.12.x for the time being.

[workmanw]

So I've spent almost all day trying to refamiliarize myself with ember data internals which seem to be a moving target. Looking back at when this last worked, ember-data-2.12.x, did the following:

clearRelationships() {
  this.eachRelationship((name, relationship) => {
    if (this._relationships.has(name)) {
      let rel = this._relationships.get(name);
      rel.clear();
      rel.destroy();
    }
  });
  Object.keys(this._implicitRelationships).forEach((key) => {
    this._implicitRelationships[key].clear();
    this._implicitRelationships[key].destroy();
  });
}

See: v2.12.2/internal-model.js

The key here is that it called clear on the relationship itself. Even when the inverseKey was undefined. Here's a stack from my app:

---

However, with the update from #4593, it no longer takes this action when the inverseKey is undefined. The reason is that there is now a destroyRelationships which does not clear relationships. If dematerializeRecord called clearRelationships instead of destroyRelationships, this would resolve the issue.

I can confirm that making this change fixes the issue with the test repo provided by @sudowork . However it still leaves an issue with my application which is using filtering. :(

I did not yet submit a PR because I'm not confident that this isn't really just me playing whack-a-mole. I think I'll need several more days to a week to fully grok everything that's going on. If someone from the core team wants to help me out I'm happy to explore some options.

[jamesarosen]

I think just the following will also cause this:

// model:foo
save() {
  return this._super()
  .then(() => {
    if (!this.get('canHaveBars')) {
      this.get('bars').forEach((bar) => {
        bar.unloadRecord();
      });
    }
  });
}

[marcelitocs]

I'm also waiting the resolution for this problem

[XaserAcheron]

Could a maintainer at least acknowledge this? This is stopping me from upgrading too, and now that 2.14 is a thing, I'm a bit concerned about being two versions behind.

[stefanpenner]

Could a maintainer at least acknowledge this?

acknowledged + I am actively working/thinking/exploring this.

Sorry, I should have left a message. Thanks for the reminder.

[XaserAcheron]

Thanks @stefanpenner -- that sets the mind at ease. :)

[rinoldsimon]

Facing similar problem. Is the bug fixed in ember version 2.14.0 ?

[richard-viney]

@rinoldsimon We encountered this issue trying to upgrade to 2.14.4 and so are staying on 2.12.2 for now.

[stefanpenner]

Just a quick update. We have found the issue, ED is working around behavior in ember. We have just landed a change in ember emberjs/ember.js#15510 which will make fixing this issue in ember-data simpler.

I aim to land the fix sometime in the next week as time permits. Sorry about letting this one linger so long.

[sudowork]

Awesome, thanks so much for the update @stefanpenner !