JsonSerializer's serialization for embedded has-many relationships includes too many records #576

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
@yipdw
Contributor

yipdw commented Jan 4, 2013

When serializing a has-many relationship on a model object, DS.JSONSerializer serializes all records in all has-many relationships on that model object. Therefore, if a model has multiple embedded has-many relationships, the serialized form of that model object will contain the same set of records in each relationship.

This can be seen by adding another has-many relationship to the Post model in tests/integration/embedded/embedded_without_ids_test.js, not adding any code to add records to that relationship, adjusting the deepEqual assertions to expect empty arrays as the serialization result, and running the changed tests.

This pull request contains an implementation of the above test suite change as well as a proposed fix. All comments are, of course, welcome -- I'm still finding my way around the Ember Data codebase and tests.

Demonstrate unwanted record sharing.
By adding a pingbacks relationship and not modifying it in any test, we
should end up with an empty array in all serialized Posts, i.e.

    {
        "post": {
            "comments": [
                {
                    "title": "Wouldn't a more lightweight...",
                    "user": null
                },
                {
                    "title": "This does not seem to reflect...",
                    "user": null
                }
            ],
            "pingbacks": [],
            "title": "A New MVC Framework in Under 100 Lines of Code"
        }
    }

However, what we get is

    {
        "post": {
            "comments": [
                {
                    "title": "Wouldn't a more lightweight...",
                    "user": null
                },
                {
                    "title": "This does not seem to reflect...",
                    "user": null
                }
            ],
            "pingbacks": [
                {
                    "title": "Wouldn't a more lightweight...",
                    "user": null
                },
                {
                    "title": "This does not seem to reflect...",
                    "user": null
                }
            ],
            "title": "A New MVC Framework in Under 100 Lines of Code"
        }
    }
@wagenet

View changes

packages/ember-data/lib/serializers/json_serializer.js
- }, this);
+ if (this.embeddedType(type, name)) {
+ if (target = get(record, name)) {
+ for(var i=0, l=get(target, 'length'); i<l; i++) {

This comment has been minimized.

@wagenet

wagenet Jan 9, 2013

Member

@yipdw Seems like it would be cleaner to use target.forEach here.

@wagenet

wagenet Jan 9, 2013

Member

@yipdw Seems like it would be cleaner to use target.forEach here.

This comment has been minimized.

@yipdw

yipdw Jan 9, 2013

Contributor

@wagenet: Sure thing; change applied.

@yipdw

yipdw Jan 9, 2013

Contributor

@wagenet: Sure thing; change applied.

Consider relationship for embedded hasMany records.
When serializing a relationship, the previous algorithm serialized all
records in _all_ has-many relationships, not just all records for the
specified relationship.
@tomdale

This comment has been minimized.

Show comment
Hide comment
@tomdale

tomdale Jan 11, 2013

Member

Thanks! I merged this, plus improved it a bit in 6c2346d.

Member

tomdale commented Jan 11, 2013

Thanks! I merged this, plus improved it a bit in 6c2346d.

@tomdale tomdale closed this Jan 11, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment