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

Fix #1492 and fall back to serializing records into IDs when not embedded. #1494

Closed
wants to merge 2 commits into from
Closed

Conversation

kaukas
Copy link
Contributor

@kaukas kaukas commented Nov 7, 2013

In serializeHasMany the JSONSerializer was trying to serialize manyToNone relations (don't even know what that means) instead of manyToOne. I found @hg-schneider had already fixed that with a pull request pending (#1488). I used it as a base for my changes.

I had to modify other AMS integration tests to add empty arrays since hasMany is now serialized even when empty. Please review and confirm that this is correct and expected logic.

hg-schneider and others added 2 commits November 3, 2013 11:55
… in function serializeHasMany

The serializer did serialize manyToNone and manyToMany relationships.
There is nothing to do in a manyToNone relationship.
ManyToOne relationships weren't handled so I changed manyToNone into manyToOne
When `embedded: 'always'` AMS embeds records properly. Otherwise it
skips the dependencies altogether. This commit makes AMS fall back to
the default RESTSerializer and serialize records to an array of their
IDs.
@tstirrat
Copy link
Contributor

Could you also allow this functionality to run when embedded === 'load'

As I understand it, it seems to be the same logic.

@kaukas
Copy link
Contributor Author

kaukas commented Nov 28, 2013

@tstirrat, the fix is for serialization. embedded: 'load' applies to extraction only, it does not affect serialization.

@tstirrat
Copy link
Contributor

It is my understanding that embedded 'load' means to extract embedded, but
when saving (serializing), it will use _id and _ids.
On 28 Nov 2013 02:00, "kaukas" [email protected] wrote:

@tstirrat https://github.com/tstirrat, the fix is for serialization. embedded:
'load' applies to extraction only, it does not affect serialization.


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

@kurko
Copy link
Contributor

kurko commented Dec 28, 2013

What this PR does is fixing a problem that I've seen too many people talk about lately. @kaukas could you take a look again at this? It'd be very good to have this PR merged imo.

@consideRatio
Copy link
Contributor

Uncertain as i am, but i think...

relevant_to_my_interests

@MartinElvar
Copy link

I would love to see a merge of this, what's the hold up? 🐹

@g-cassie
Copy link

+1

@stefanpenner
Copy link
Member

@MartinElvar lack of time to review :(

@danielkywang
Copy link

+1

@Hammz
Copy link

Hammz commented Apr 22, 2014

Would be absolutely amazing to see this in release 1.0, this is ugently needed!

@leostera
Copy link

+1

@MartinElvar
Copy link

Here is a a quick fix for those of you who's in great need for a fix 😄

// Apply hack that fixes ManyToOne serialization.
// Use until this is merged, or fixed otherwise.
// https://github.com/emberjs/data/pull/1494
DS.ActiveModelSerializer.reopen({
  serializeHasMany : function(record, json, relationship) {
    var key = relationship.key;
    var relationshipType = DS.RelationshipChange.determineRelationshipType(record.constructor, relationship);

    if (relationshipType === 'manyToNone' || relationshipType === 'manyToMany' || relationshipType === 'manyToOne') {
      var payloadKey = this.keyForRelationship ? this.keyForRelationship(key, "hasMany") : key;
      json[payloadKey] = Ember.get(record, key).mapBy('id');
    }
  }
});

@quaertym
Copy link

quaertym commented May 1, 2014

👍

@edimoldovan
Copy link

Hi everyone!
Am I correct in thinking that this is not merged yet? If so, I would definitely buy at least beer to the team member who merges it :)
And, @MartinElvar thank you for the fix, it works like a charm!

@kalmanh
Copy link
Contributor

kalmanh commented Jul 10, 2014

+1 for @MartinElvar fix above

@igorT
Copy link
Member

igorT commented Jul 16, 2014

This was fixed with the merge of the better embedded records support thanks to @pixelhandler. You can now declare embedded: 'ids' in your attrs hash if you want to serialize the hasMany side.

@igorT igorT closed this Jul 16, 2014
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.