FilteredRecordArray has null values after unloadRecord w/ 2.13 and newer.

[workmanw]

This is very similar to #4986, however it has a slightly different cause so I wanted to file it as a separate issue.

What's happening

I created specific twiddle to demonstrated:

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

I would direct your attention to the removeTodo action in the todo-list controller. What's happening is that when I call unloadRecord the filtered array replaces the value of the model that was just unloaded with null.

This null value exists only until the end of the runloop because there is a _flush function scheduled to run in the action queue to clean these up.

Why is this happening?

With 2.13 there was a change made (#4593) that set's the internal model's record to null when it's reset or dematerialized (this is new functionality and important to understanding the issue). See: internal-model.js#L360.

Also, because the filtered array's content is a list of internal models, it has to look up the record via objectAtContent which produces a null values for unloaded records. See: record-array.js#L107-L110. So that means when someone tries to use objectAt or toArray it'll get a list with a null value in place of the item that was just unloaded.

What did it do before?

With 2.12 the record was not nullified from the internal model. So while the array would produce a stale value (i.e. an unloaded record) until the end of the runloop, it was more desirable than a null value.

Why is this a problem?

In my case, after the unload action happens and it schedules a _flush task in the actions queue, however it moves on to other tasks in the render queue. That's when views and computed properties access the array, hit the null value that they weren't expecting and throw an exception.

I could make my application more durable to assume that values in these filtered arrays could be null, but it feels like the right answer is they should never been null.

[stefanpenner]

@workmanw if you have cycles, do you mind submitting a failing test?

[workmanw]

@stefanpenner Sure thing. It shouldn't be hard at all. I'll see if I can get it done today or tomorrow.

[workmanw]

Done: #5095 :).

[stefanpenner]

thanks. unloadRecord having immediately side-affects is somewhat at odds with batching array changes.

What do you think about unloadRecord not having any affects until that run flushes?

[workmanw]

@stefanpenner I'm +1 on that. I'll modify my test tonight and ping you again .

[workmanw]

@stefanpenner Done. Thanks a lot for giving this attention.

[stefanpenner]

Thanks a lot for giving this attention.

no problem, its important to flesh out all these pieces.

@stefanpenner I'm +1 on that.

I still need to think about this both RecordArrays and ManyArrray's. I would love to unify them, so that it can be a consistent experience (and it may prove quite tricky to address this issue without unifying them).

[stefanpenner]

@workmanw if you can also noodle on it some that would be handy. I have some extra ED time coming up, I will try to use some of that for ^.

[workmanw]

@stefanpenner Hmm. I had hoped the fix for #4986 would also fix this issue. But I can see how it is a slightly different issue.

[stefanpenner]

Ya, that RecordArray infrastructure is unfortunately different, and it does batching... So these are slightly at odds.... or another way to put it, a fun puzzle ;)

[workmanw]

@stefanpenner I had wondered to myself if it could be as simple as using a computed property to filter out the null entries on the filtered array.

[stefanpenner]

@stefanpenner I had wondered to myself if it could be as simple as using a computed property to filter out the null entries on the filtered array.

It's likely for some cases, but will likely cause some perf issues with large arrays that get many small mutations.

[Kilowhisky]

Not sure if this helps but i was able to push off the immediate explosion by pushing the resetRecord until right before the _checkForOrphanedInternalModels had been executed.

unloadRecord() {
    this.send('unloadRecord');
    this.dematerializeRecord();
    if (this._scheduledDestroy === null) {
      this._scheduledDestroy = run.schedule('destroy', this, () => {
          this.resetRecord();
          this._checkForOrphanedInternalModels();
      });
    }
  }

Here's the interesting part. This block above was applied to ED-2.13 and it works up until the point where i attempt to load models of the type that was unloaded from the store.

"Assertion Failed: You looked up the 'positions' relationship on a 'asset' with id 222222222222222 but some of the associated records were not loaded. Either make sure they are all loaded together with the parent record, or specify that the relationship is async ('DS.hasMany({ async: true })'

But...

If i apply the same block change to the 2.17 branch the app doesn't error at all. But..... i'm unable to re-materialize any records that were unloaded.

this._scheduledDestroy = run.backburner.schedule('destroy', this, () => {
          this.resetRecord();
          this._checkForOrphanedInternalModels();
      });

Is that progress?

[runspired]

I'll be looking into this as part of #5378. Thanks for #5095

[runspired]

I've confirmed that #5378 will resolve this, incidentally it is due to this fix for #5350

55fd924