Issue setting belongsTo with PromiseProxy

[raytiley]

I have simple code like this:

var newRecord = this.store.createRecord('book', {
  author: this.store.find('author', 1)
});

This is giving me an exception:

Error while processing route: shows.new Cannot read property 'modelName' of undefined TypeError: Cannot read property 'modelName' of undefined
    at ember$data$lib$system$relationships$state$belongs$to$$BelongsToRelationship.addRecord (http://localhost:4200/assets/vendor.js:77824:57)
    at ember$data$lib$system$relationships$state$belongs$to$$BelongsToRelationship.setRecord (http://localhost:4200/assets/vendor.js:77776:14)
    at ember$data$lib$system$relationships$state$belongs$to$$BelongsToRelationship.setRecordPromise (http://localhost:4200/assets/vendor.js:77848:12)
    at ember$data$lib$utils$computed$polyfill$$default.set (http://localhost:4200/assets/vendor.js:83299:53)
    at Descriptor.computedPropertySet [as _set] (http://localhost:4200/assets/vendor.js:26124:20)
    at Descriptor.computedPropertySetWithSuspend [as set] (http://localhost:4200/assets/vendor.js:26090:12)
    at Object.set (http://localhost:4200/assets/vendor.js:31432:12)

I'm currently on 1.0.0-beta.19+canary.34efef04a2

[sly7-7]

@raytiley Do you know if it used to work on previous versions ?

[raytiley]

I'm pretty sure it worked before. The problem is definitely inconsistent and weird. My access of the PromiseProxy is from a service. I have a computed property on a service like such

// services/location.js
activeLocation: Ember.computed('locationId', {
  get: function() {
    this.get('store').find('show', this.get('locationId'));
  }
})

Then in a route's model hook I do:

model: function() {
  return this.get('store').createRecord('show', {
    location: this.get('location.activeLocation') // comes from the locations service above
  }
}

If the code is just like above I don't get the error. But calling save() on the new record doesn't serialize the location id into the new record.

So I tried adding a beforeModel call to make sure the activeLocation property on the service is fulfilled.

beforeModel: function() {
  return this.get('location.activeLocation');
},

model: function() {
  return this.get('store').createRecord('show', {
    location: this.get('location.activeLocation') // comes from the locations service above
  }
}

The above code throws the error in my first message. What's doubly weird is that I do a .find('location') in my application route, so all the locations should be in the store. So I don't get how the beforeModel call changes things since the data should be loaded and that findById call should resolve immediatly.

To get things working I can access .content on the PromiseProxy. So my model code becomes:

beforeModel: function() {
  return this.get('location.activeLocation');
},

model: function() {
  return this.get('store').createRecord('show', {
    location: this.get('location.activeLocation.content') // comes from the locations service above
  }
}

The above code throws no errors, and the location property in the JSON sent to the server when calling save() on the new record has the correct value.

Hope this helps.

[sly7-7]

Quickly looking at the code, it seems like

return this.get('store').createRecord('show', {
    location: this.get('location.activeLocation') // comes from the locations service above
  }

should work, at least there is a mechanism to accept setting a belongsTo with a PromiseProxy if it already has the content.
I think it could have to do with the recent move to internalModel. As the stack shows, there is an access to modelName here https://github.com/emberjs/data/blob/master/packages/ember-data/lib/system/relationships/state/belongs-to.js#L65

It would be great if you could find which one here is undefined.

Also I noticed in your example:

// services/location.js
activeLocation: Ember.computed('locationId', {
  get: function() {
    this.get('store').find('show', this.get('locationId')); // the model name seems weird for a location
  }
})

Maybe just a typo here, but if not, it could be the cause of the problem

[raytiley]

Yeah.. the model name in the service is a typo. In the stack the type property in newRecord.type is undefined.

[sly7-7]

I have absolutely no idea how that could happen. I think newRecord is an InternalModel instance, and it should definitely have a type... It would be great if you could file a failing test

[raytiley]

Can you point me in the direction of a similar test I can try to tweak? I'm super short on time for the next few weeks, and I typically suck at ED tests.

I could also do a quick screenhero session with someone if they want to poke around the stack trace live.

@igorT knows where to find me :)

[sly7-7]

@raytiley I will try to make a test now, since the base scenario looks easy to reproduce (at least I think)

[raytiley]

@sly7-7 ❤️

I'm good for now, like I said I worked around it by accessing the content property on the PromiseProxy but that seems like a short term hack.

Thanks again.

[sly7-7]

So, maybe the test is a little contrived, but it reproduces your case:

test("A record can be created with an async belongsTo already in store", function() {
  expect(1);

  var Group = DS.Model.extend({
    people: DS.hasMany()
  });

  var Person = DS.Model.extend({
    group: DS.belongsTo({ async: true })
  });

  env.registry.register('model:group', Group);
  env.registry.register('model:person', Person);

  var group;
  run(function() {
    group = store.push('group', { id: 1 });
  });

  var groupPromise = store.find('group', 1);
  groupPromise.then(async(function(group){
    var person = env.store.createRecord('person', {
      group: groupPromise
    });
    equal(person.get('group.content'), groupPromise);
  }));
});

[sly7-7]

So it turns out that your code and this code is trying to put a real record (from the promiseProxy's content property) while I think the relationship now expects an internal model.
cc/ @igorT what do you think about this use case ?

[igorT]

nice catch, I thought i handled that in the internalModel refactor, let me double check

[igorT]

@sly7-7 yes the record promise should call getRecord on the promise content. Weird that the tests pass, maybe they just assert the id, as there is a test for this case. Should be an easy fix

[sly7-7]

@igorT could you give me a pointer to this test ? also is it worth it to add the above test?
Also, maybe I miss something but it should call content._internalModel, not content.getRecord() right ?

[igorT]

https://github.com/emberjs/data/blob/master/packages/ember-data/tests/integration/relationships/one-to-one-test.js#L165 is an example. Confused why it passes.
Yes, to content._internalModel

[sly7-7]

@raytiley If you could try with the PR, it would be great :)