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

JSONSerializer.serializeHasMany() behaves differently depending on the way the record has been added to the store #3760

Closed
sebweaver opened this issue Sep 11, 2015 · 5 comments

Comments

@sebweaver
Copy link
Contributor

Here is a JSBin highlighting the two behaviors: http://emberjs.jsbin.com/rocano/8/edit?js,console,output

On the first case (post added by createRecord) the json is:

{ comments: [] }

On the 2nd case (post added by push) the json is:

{ comments: undefined }

Shouldn't be the same (i.e. the 1st one)? Or is it by design to have undefined relationships on the 2nd one?

@wecc
Copy link
Contributor

wecc commented Sep 11, 2015

Thanks for reporting!

The bug is here: https://github.com/emberjs/data/blob/master/packages/ember-data/lib/serializers/json-serializer.js#L1127

snapshot.hasMany() returns undefined for "unknown" relationships (docs). That is, you've pushed the record into the store, but the relationship is missing. ED cannot know if there's additional data on the server or not, that's why there's a "unknown" state of a relationship. We wouldn't want to serialize unknown relationships to [] because that might overwrite server state.

When you do store.createRecord() we consider the record as new and client-side and because of that all the relationships are known (which means snapshot.hasMany() returns an empty array instead of undefined)

The JSONAPISerializer handles this correctly, see https://github.com/emberjs/data/blob/master/packages/ember-data/lib/serializers/json-api-serializer.js#L428-L429

@sebweaver
Copy link
Contributor Author

First of all thank you for your quick and detailed answer. I've noticed the difference between the two serializers, but I didn't know whether it was on purpose or not. With your explanation it's now clear.

Thanks

@wecc
Copy link
Contributor

wecc commented Sep 13, 2015

I would still consider this a bug. Unknown relationships should be omitted when serializing records using JSONSerializer (and RESTSerializer).

@wecc wecc reopened this Sep 13, 2015
sebweaver added a commit to sebweaver/data that referenced this issue Sep 14, 2015
`serializeHasMany()` omits unknown relationships rather than setting them to
`undefined`.
sebweaver added a commit to sebweaver/data that referenced this issue Sep 14, 2015
`serializeHasMany()` omits unknown relationships of pushed record rather than
setting them to `undefined`.
@sebweaver
Copy link
Contributor Author

I've created a PR: #3765

sebweaver added a commit to sebweaver/ember-localforage-adapter that referenced this issue Sep 14, 2015
Temp workaround for ED issue emberjs/data#3760 until
the PR emberjs/data#3765 will be merged.

Also change class structure to fit the current one (with a dedicated
`_shouldSerializeHasMany` method).
sebweaver added a commit to sebweaver/ember-localforage-adapter that referenced this issue Sep 14, 2015
Temp workaround for ED issue emberjs/data#3760 until
PR emberjs/data#3765 is merged.

Change class structure also to reflect the new one introduced earlier by ED 1.13
(with dedicated `_shouldSerializeHasMany`).
sebweaver added a commit to sebweaver/data that referenced this issue Sep 16, 2015
`serializeHasMany()` omits unknown relationships of pushed record rather than
setting them to `undefined`.
bmac added a commit that referenced this issue Sep 17, 2015
[BUGFIX beta] Fix JSONSerializer.serializeHasMany() issue #3760
bmac pushed a commit that referenced this issue Sep 17, 2015
`serializeHasMany()` omits unknown relationships of pushed record rather than
setting them to `undefined`.

(cherry picked from commit 2dbb617)
@e00dan
Copy link
Contributor

e00dan commented Sep 19, 2015

Shouldn't this be closed because of #3765 merged?

@wecc wecc closed this as completed Sep 20, 2015
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

No branches or pull requests

3 participants