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

fixes serializeHasMany relationshipType #1678

Closed
wants to merge 4 commits into from

Conversation

fsmanuel
Copy link
Contributor

there is a typo in the JSONSerializer serializeHasMany that prevents hasMany relations from getting serialized.

@abuiles
Copy link
Member

abuiles commented Jan 21, 2014

@fsmanuel this is failing in travis, also could you please add tests for this?

@rwjblue
Copy link
Member

rwjblue commented Jan 21, 2014

@abuiles
Copy link
Member

abuiles commented Jan 21, 2014

@rjackson wonder though about the TODO in the code.

@fsmanuel
Copy link
Contributor Author

@abuiles @rjackson sorry for that. ok i'll add it back and add the ManyToOne.

@fsmanuel
Copy link
Contributor Author

it seems to be intentional that there is no manyToOne. i added a test for my concern but serializeIntoHash and serializeIntoHash with decamelized typeKey of the integration/serializer/rest - RESTSerializer suite are now failing.

@fsmanuel
Copy link
Contributor Author

@abuiles added a test and fixed the failing

@@ -391,9 +391,9 @@ DS.JSONSerializer = Ember.Object.extend({

var relationshipType = DS.RelationshipChange.determineRelationshipType(record.constructor, relationship);

if (relationshipType === 'manyToNone' || relationshipType === 'manyToMany') {
if (relationshipType === 'manyToNone' || relationshipType === 'manyToOne' || relationshipType === 'manyToMany') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All three are needed, how about:

    var relationshipTypes = ['manyToNone', 'manyToMany', 'manyToOne'];

    if (relationshipTypes.indexOf(relationshipType) > -1) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't know which one is faster. but yours seems cleaner.

@pixelhandler
Copy link
Contributor

@fsmanuel I rebased PR #1751 which also addresses this issue, I asked @igorT to take a look at that one, (said please) so hoping for some feedback soon.

@quaertym
Copy link

👍

1 similar comment
@GetContented
Copy link

👍

@pixelhandler
Copy link
Contributor

@fsmanuel PR #1751 was merged, can you confirm that fixes the same issue on this PR?

@igorT
Copy link
Member

igorT commented May 18, 2014

OneToMany are not serialized by default, because the JSON serializer assumes by default that you would save your id's as foreign keys on the child. If you need to serialize them, you can now instead of editing the serializeHasMany code, apply the EmbeddedRecordsMixin and say

attrs: { children: {serialize: 'ids'}}

I do think this is a bit non obvious, would be nice to document it better.

@igorT igorT closed this May 18, 2014
@fsmanuel
Copy link
Contributor Author

@igorT thanks for the clarification and your work on ember-data. will try the attrs approach tomorrow. wasn't aware of the possibility.
my problem with the serializers was that the JSON serializer seemed to wrong in some areas and all the other serializers (REST, ActiveModel) fixed the mistakes but the JSON serializer stayed the same.

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