Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests showing creating and saving a model with async relationships does ... #63

Conversation

craigteegarden
Copy link
Collaborator

...not POST the relationship to the server.

The issue seems to be that a model with a relationship attribute that is marked
as async: true will return a promise when that attribute is accessed using
get(). This causes the serializeBelongsTo() or serializeHasMany() method in
the serializer to get() the relationship object and incorrectly assume it is
the actual model object, when a promise was instead retrieved.

For example, with an async: true relationship, this line is trying to get the
id of the related model object but it is instead accessing the id property of
a promise, which returns undefined:

https://github.com/toranb/ember-data-django-rest-adapter/blob/master/src/serializer.js#L123

…es not POST the relationship to the server.

The issue seems to be that a model with a relationship attribute that is marked
as async: true will return a promise when that attribute is accessed using
get(). This causes the serializeBelongsTo() or serializeHasMany() method in
the serializer to get() the relationship object and incorrectly assume it is
the actual model object, when a promise was instead retrieved.

For example, with an async: true relationship, this line is trying to get the
id of the related model object but it is instead accessing the id property of
a promise, which returns undefined:

https://github.com/toranb/ember-data-django-rest-adapter/blob/master/src/serializer.js#L123
@craigteegarden
Copy link
Collaborator Author

It doesn't look like jsonSerializer's versions of these methods handle promises correctly either.

serializeBelongsTo() for ember-data-django-rest-adapter: source
serializeHasMany() for ember-data-django-rest-adapter: source

serializeBelongsTo() for jsonSerializer: source
serializeHasMany() for jsonSerializer: source

@toranb
Copy link
Owner

toranb commented Nov 15, 2013

@craigteegarden nice work on finding these scenarios -one part I don't yet understand is "what does an async belongsTo look like on the python side?"

I never put an async true for belongsTo and I'm curious what the json looks like (ie- what the python models look like)

@craigteegarden
Copy link
Collaborator Author

Ember model:

App.User = DS.Model.extend({
  firstName: DS.attr('string'),
  lastName: DS.attr('string'),
  company: DS.belongsTo('company', { async: true })
});

Python model:

class User(models.Model):
    first_name = models.CharField(max_length=50)
    last_name = models.CharField(max_length=50)
    company = models.ForeignKey(Company, null=True, blank=True, default=None)

{async: true} is required to access the company attribute using user.get('company') before the related company is loaded. This will immediately return a promise while it retrieves that record. If {async: true} is not set and the company record isn't already loaded then ember data returns undefined from the get().

Am I doing something odd with the relationships?

edit:

The JSON for /user/ would be:

{
  "first_name": "first",
  "last_name": "last",
  "company": 2
}

where the company key contains the id of the related company.

@toranb
Copy link
Owner

toranb commented Nov 16, 2013

Excellent find my friend! Does this break down in the rails backed ember-data adapter?

@craigteegarden
Copy link
Collaborator Author

The activemodel serializer only overrides serializeHasMany and it doesn't look like it handles this either:

https://github.com/emberjs/data/blob/master/packages/activemodel-adapter/lib/system/active_model_serializer.js#L49

@toranb
Copy link
Owner

toranb commented Nov 19, 2013

@craigteegarden do you have an idea about how to approach this? Assuming you need this fixed for your production app, do you have the bandwidth to solve this or did you want me to give it a look?

@craigteegarden
Copy link
Collaborator Author

TL;DR: This isn't a huge blocking issue for me at the moment.

We don't do a lot of writes or creates from our app at the moment, so this can be worked around by directly performing an ajax POST without too much grief. I am wondering if I am structuring things in a non-ideal way that causes us to need the async: true option... I will be looking into that on our side, so this isn't a huge blocker for me right now.

That said, this does seem like strange behavior that doesn't appear to be handled on the ember-data side. I think filing an issue on ember-data might be good if we can come up with a good way to reproduce it in pure ember-data.

Thoughts?

@craigteegarden
Copy link
Collaborator Author

This PR discusses async relationships and possibly even shows a way to access the promise-wrapped data in the serialize method: emberjs/data#1491

@toranb
Copy link
Owner

toranb commented Dec 4, 2013

I believe (as you mentioned above) that this needs some love from the core library so I opened an issue (showing the async belongsTo as broken for now as I've hit this in production)

emberjs/data#1542

@nickiaconis
Copy link

I'm looking to use this adapter for an application I'm building, but this issue might be a deal breaker. Does this only come into play during a create+save sequence? Would pairing async hasMany with synchronous belongsTo, given the hasMany is always empty upon creation, circumvent this issue?

I suppose I will find these answers eventually through trial and error, but any information I can gather beforehand is greatly appreciated.

@toranb
Copy link
Owner

toranb commented Dec 6, 2013

@codefox421 you are correct. It's unfortunate but this is a complete show stopper for anyone with a non-embedded api. I'm waiting for this to complete my first commercial ember.js + django app because my entire api is async

If someone was doing 100% readonly app development this wouldn't effect them as it's only when you do an update or create.

Keep an eye on the core ember-data issue as it's the real fix that's required to move forward

@toranb
Copy link
Owner

toranb commented Dec 31, 2013

Just a quick update -I've got a branch with the async belongsTo relationship working for both POST and PUT

f78b260

This still requires the PR mentioned above be pulled into ember-data core, so until then I included the custom ember-data build that @jherdman has a branch for

https://github.com/jherdman/data/tree/bug-polymorphic-async-belongs-to

The branch above was specifically for async polymorphic belongsTo but it laid the ground work for this adapter PR

The ember-data core issue is still open and I'll be working against it until it's officially pulled in emberjs/data#1535

@toranb
Copy link
Owner

toranb commented Dec 31, 2013

@craigteegarden just a note -I used your awesome tests as the scaffold but found that in my real apps I would structure the hasMany / belongsTo relationship like this

App.Customer = DS.Model.extend({
    name: DS.attr('string'),
    appointments: DS.hasMany('appointment', { async: true})
});

App.Appointment = DS.Model.extend({
    start: DS.attr('string'),
    details: DS.attr('string'),
    customer: DS.belongsTo('customer', { async: true})
});

@craigteegarden
Copy link
Collaborator Author

@toranb awesome! Great job tracking that, I hope they pull it in soon.

Need me to do anything more on this PR? I can rebase, etc.

@toranb
Copy link
Owner

toranb commented Jan 2, 2014

I think this PR will actually die in a few weeks/ or months. I'm actually planning to submit a PR to ember-data core that will pull this in for anyone using the library (as this feature is required regardless of your backend).

But I'm waiting for the mentioned PR above to be completely pulled in first. Also, I needed this in my project and thought to extend it for other who might be trying to ship like us :)

@craigteegarden
Copy link
Collaborator Author

Awesome :-)
On Jan 1, 2014 8:30 PM, "Toran Billups" notifications@github.com wrote:

I think this PR will actually die in a few weeks/ or months. I'm actually
planning to submit a PR to ember-data core that will pull this in for
anyone using the library (as this feature is required regardless of your
backend).

But I'm waiting for the mentioned PR above to be completely pulled in
first. Also, I needed this in my project and thought to extend it for other
who might be trying to ship like us :)


Reply to this email directly or view it on GitHubhttps://github.com//pull/63#issuecomment-31434617
.

@jherdman
Copy link

jherdman commented Jan 2, 2014

@toranb FYI my branch's history had to be rewritten to make it more amenable for the Ember Data team. You may want to double check your work.

@andykrier
Copy link

@toranb @codefox421 Any luck with a work around for this? I tried the brach referenced, but still no luck. Any info would be greatly appreciated.

@toranb
Copy link
Owner

toranb commented Feb 17, 2014

@andykrier the branch referenced has changed, instead use the ember-data build included in the WIP branch for this project. If you use that ember-data.js along w/ the adapter (also included in that branch) you should be able to POST / PUT to an async child model.

What error are you getting? What do your JS models look like? your django models?

@andykrier
Copy link

Hey Toran,

Thanks so much for getting back to me. I pulled the ember-data.js and adapter.js from the WIP branch, but still no luck. When I try and save a record, none of the "belongsTo" fields are included in the PUT. Here are my models:

App.Location = DS.Model.extend({
name: attr(),
number: attr(),
type: attr('number'),
phone_numbers: DS.hasMany('phonenumber', {async: true}),
addresses: DS.hasMany('address', {async: true}),
emails: DS.hasMany('email', {async: true})
});

App.Phonenumber = DS.Model.extend({
location: DS.belongsTo('location'),
number: attr(),
type: attr('number'),
user: DS.belongsTo('user')
});

App.Address = DS.Model.extend({
address1: attr(),
address2: attr(),
address3: attr(),
city: attr(),
country: attr(),
location: DS.belongsTo('location'),
poastalcode: attr(),
state: attr(),
type: attr(), // belongs to address type
user: DS.belongsTo('user')
});

App.Email = DS.Model.extend({
email: attr(),
type: attr(), //belongs to email type
location: DS.belongsTo('location'),
user: DS.belongsTo('user')
});

and here is my controller with save action:

App.LocationController = Ember.ObjectController.extend({
actions: {
save: function() {
var loc = this.get('model');
loc.save();
this.transitionToRoute('location', loc);

    }
}

});

When I save a location, only the following data is included:

{
"name":"District83 Update",
"number":"83",
"type":3
}

Thanks again,
Andy

On Feb 16, 2014, at 6:23 PM, Toran Billups notifications@github.com wrote:

@andykrier the branch referenced has changed, instead use the ember-data build included in the WIP branch for this project. If you use that ember-data.js along w/ the adapter (also included in that branch) you should be able to POST / PUT to an async child model.

What error are you getting? What do your JS models look like? your django models?


Reply to this email directly or view it on GitHub.

@toranb
Copy link
Owner

toranb commented Feb 19, 2014

Can you update your belongsTo definitions to look something like this and give it a try? I've only had luck using async on both ends of the relationship

location: belongsTo('location', { async: true})

Also, right before you save it (this line - loc.save();) -what does the model look like? does it have a resolved belongsTo? Does it have all the primitives you expect (before the save). As I'm guessing this get('model') line pulls from a bound template ... I wanted to be sure it was rock solid first.

For example, instead of doing the get('model') line, if you constructed it manually using the async on both sides, does it work?

@g-cassie
Copy link
Contributor

Just lurking here. @andykrier it looks like all of the relationships on Location are hasMany. So I think you're issue is that you want to POST hasMany keys (e.g. phone_numbers: [1, 2, 3], not belongsTo keys (e.g. location: 1 on a serialized PhoneNumber object). In my experience, this can only be done by editing serializeHasMany in DjangoRESTAdapter to change 'manyToNone' to 'manyToOne'. Take a look at emberjs/data#1494 on the Ember Data repo.

Many apologies if I am totally off base.

@andykrier
Copy link

@toranb Thanks for explaining that the async attribute needed to be in both ends of the relationship. Once I made this change, I looked at the model before the save and was not able to make much sense of it. I Think I have some more reading before I understand this completely.

@g-cassie I made the change that you suggested just to see if what would happen and it worked. All related fields were included in the PUT payload. Again, I have some learning to do before I understand why.... thanks for the tip.

@dustinfarris
Copy link
Collaborator

@craigteegarden can we close this?

@craigteegarden
Copy link
Collaborator Author

Yes, let's close for now. This still seems to be a lingering issue in ember data and not something we can address in this project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants