failed test: createRecord creates two records on ember 3.0

[dwickern]

After upgrading ember from 2.18.2 to 3.0, with ember-data unchanged at version 2.18.1, a call to createRecord creates 2 records.

It happens under these conditions:

1. The call to createRecord sets up a relationship which has an inverse
2. The inverse array is being observed (e.g. used in a template)

Here's a shortened example of the problem:

const Post = Model.extend({
  comments: hasMany('comment')
});
const Comment = Model.extend({
  post: belongsTo('post')
});

// non-working example
let post = this.get('model');
let comment = this.store.createRecord('comment', { post });
comment === post.get('comments.firstObject'); // false!! actually 2 comments were created

It works if you set up the relationship after construction:

// workaround
let post = this.get('model');
let comment = this.store.createRecord('comment');
comment.set('post', post); // don't pass directly to createRecord
comment === post.get('comments.firstObject'); // true

It also works if nobody is observing the comments array.

The stack trace explains why: createRecord updates the inverse record array synchronously before actually creating the record

create (container.js:420)
create (container.js:178)       <<<============
getRecord (-private.js:6597)
objectAt (-private.js:4180)
objectAt (array.js:50)
objectAtContent (array_proxy.js:110)
objectAt (array_proxy.js:149)
objectAt (array.js:50)
addObserverForContentKey (array.js:600)
arrayDidChange (array.js:554)
arrayContentDidChange (array.js:105)
_emberMetal.Mixin.create._Mixin$create.arrayContentDidChange (array.js:281)
exports.default._object.default.extend._EmberObject$extend._arrangedContentArrayDidChange (array_proxy.js:221)
applyStr (ember-utils.js:528)
sendEvent (ember-metal.js:257)
arrayContentDidChange (array.js:108)
_emberMetal.Mixin.create._Mixin$create.arrayContentDidChange (array.js:281)
internalReplace (-private.js:4224)
_addInternalModels (-private.js:4240)
addInternalModel (-private.js:4412)
addInternalModel (-private.js:3745)
addInternalModel (-private.js:4733)
setInternalModel (-private.js:4646)
set (-private.js:12386)
computedPropertySet (ember-metal.js:3456)
computedPropertySetWithSuspend (ember-metal.js:3438)
computedPropertySetEntry (ember-metal.js:3416)
Class (core_object.js:100)
_ClassMixinProps.create (core_object.js:277)
create (container.js:420)
create (container.js:178)       <<<============
getRecord (-private.js:6597)
createRecord (-private.js:9922)

[dwickern]

I also see a deprecation error on this line which doesn't seem right

user.get('messages.firstObject')
// Getting the '@each' property on object <DS.Errors:ember9921> is deprecated

[mmun]

@dwickern That deprecation error arrises because when assert.equal is false it tries to serialize the objects. Not sure the best way to address that. We could try making @each non-enumerable. cc @rwjblue

[mmun]

So after some initial sleuthing, that same call stack appears when using Ember 2.11 as well. It's not clear to me why two records are created.

[dwickern]

@mmun Thanks for looking into this. Sure enough, I see the same reentrant call into getRecord on 2.11. My test passes on 2.11-2.18 so something changed in 3.0 to make the bug visible.

[mmun]

@dwickern I'll keep looking later today.

[mmun]

So I've got an update. I haven't been able to do a full root cause as it is simply too complicated for me and I don't have the time to step through it at all. However, what I can say is that it looks like a messy interaction between array events being triggered during initialization (i.e. before finishChains) caused by Ember Data when it is splicing into inverse relationships during createRecord.

It's not obvious to me whether this is a bug in the current object model or simply a known limitation. I'll ask Kris later. Fortunately, we can avoid this issue relatively easily by changing

data/addon/-private/system/store.js

Line 361 in 1732cd5

let record = internalModel.getRecord(properties);

let record = internalModel.getRecord();
setProperties(record, properties);

This will allow the record to be fully instantiated and assigned into the internal model's _record field before we do any complex logic like splicing into inverse relationships. I've confirmed that this works in the reproduction app given in emberjs/ember.js#16258.

A couple implementation notes: 1) we probably want to remove the properties parameter entirely for getRecord. 2) with my proposed implementation we end up assigning id to the record twice. Not sure how big of a deal that is.

[mmun]

Here are some issues where this behaviour was initially introduced:

• init event fires before leaving empty state #4509
• [BUGFIX beta] [fixes #4509] createRecord initializes correctly #4931

I believe it is acceptable to revert that PR until it can be implemented correctly (i.e. not creating two records recursively). I think it would also be fine if someone wants to attempt to fix the current implementation.

[mmun]

I bisected the ember.js commit down to what I expected it to be: emberjs/ember.js#16166.

Still, I'm not sure whether this is more of Ember or Ember data's fault, since the trivial refactor of moving the property assignments described in #5359 (comment) to avoid the double create fixes the issue in emberjs/ember.js#16258 as well.

[dwickern]

The fix from #5359 (comment) LGTM, I'll open a PR unless you want to look into it further

[mmun]

@dwickern I am worried that, while the fix I described will correctly fix this bug, it is undoing something that was done intentionally to solve #4509.

I believe it ought to be possible to fix this bug without compromising but I haven't been able to figure it out yet. I've pinged some other core team members to take a look. If we can't get to the bottom of it in the next couple weeks than I think we'll have to do the fix I described.