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

store.push throws with JSONAPI document { data: null } #3790

Closed
mitchlloyd opened this Issue Sep 21, 2015 · 10 comments

Comments

Projects
None yet
7 participants
@mitchlloyd
Contributor

mitchlloyd commented Sep 21, 2015

The JSONAPI spec indicates that the following payload is a valid response when asking for a has-one relationship from a server:

{"data": null}

However, when that normalized response makes it to store.push, ED throws the following error:

Assertion Failed: You must include an 'id' for undefined in an object passed to 'push'"

It should be simple to add a failing test case for this in store-test.js but I'm getting timeout errors when trying to install the ED dependencies at the moment.

@wecc

This comment has been minimized.

Show comment
Hide comment
@wecc

wecc Sep 23, 2015

Contributor

Agree, we should fix this.

Contributor

wecc commented Sep 23, 2015

Agree, we should fix this.

@phcoliveira

This comment has been minimized.

Show comment
Hide comment
@phcoliveira

phcoliveira Oct 7, 2015

I was just about to post the same thing. I am glad I have found this first.

Just to be sure, JSON API also states that { "data": [] } is a valid response for querying collections.

I felt there was something wrong when I saw the error mentioned by @mitchlloyd, although I am not certain about how this case should be handled.

@wecc, could you please explain how the fix should behave? I am guessing that the appropriate callback will be .then() instead of .catch().

phcoliveira commented Oct 7, 2015

I was just about to post the same thing. I am glad I have found this first.

Just to be sure, JSON API also states that { "data": [] } is a valid response for querying collections.

I felt there was something wrong when I saw the error mentioned by @mitchlloyd, although I am not certain about how this case should be handled.

@wecc, could you please explain how the fix should behave? I am guessing that the appropriate callback will be .then() instead of .catch().

@Ramblurr

This comment has been minimized.

Show comment
Hide comment
@Ramblurr

Ramblurr Nov 11, 2015

Any idea when this fix will make it into a release?

Ramblurr commented Nov 11, 2015

Any idea when this fix will make it into a release?

bmac added a commit that referenced this issue Nov 25, 2015

Allow store.push to accept { data: null }
Closes #3790

(cherry picked from commit 2f17c2d)
@pangratz

This comment has been minimized.

Show comment
Hide comment
@pangratz

pangratz Jan 26, 2016

Member

@Ramblurr this should be fixed in 2.2.1.

Member

pangratz commented Jan 26, 2016

@Ramblurr this should be fixed in 2.2.1.

@jherdman

This comment has been minimized.

Show comment
Hide comment
@jherdman

jherdman Mar 1, 2016

Contributor

I think there may be a regression in 2.4.0 with respect to behaviour described in this pull request. I have a model hook like this in an acceptance test:

return this.store.queryRecord('some-model', { limit: 1 });

Whereas this model hook works with Ember Data 2.3.3, the same test now fails with 2.4.0. I'll see if I can put together a reproduction mini app today.

Contributor

jherdman commented Mar 1, 2016

I think there may be a regression in 2.4.0 with respect to behaviour described in this pull request. I have a model hook like this in an acceptance test:

return this.store.queryRecord('some-model', { limit: 1 });

Whereas this model hook works with Ember Data 2.3.3, the same test now fails with 2.4.0. I'll see if I can put together a reproduction mini app today.

@bmac

This comment has been minimized.

Show comment
Hide comment
@bmac

bmac Mar 1, 2016

Member

What does the server response look like for this query?

Member

bmac commented Mar 1, 2016

What does the server response look like for this query?

@jherdman

This comment has been minimized.

Show comment
Hide comment
@jherdman

jherdman Mar 1, 2016

Contributor

Like this:

{ "plural_model_name": [] }
Contributor

jherdman commented Mar 1, 2016

Like this:

{ "plural_model_name": [] }
@jherdman

This comment has been minimized.

Show comment
Hide comment
@jherdman

jherdman Mar 3, 2016

Contributor

Apologies for the delay, but I have a reproduction available now.

The repo lives here: https://github.com/jherdman/issue-3790-reproduction. Run the app, hit the index page in your browser. Note the Ember Data 2.4 complains about the empty response, whereas 2.3.3 does not.

Contributor

jherdman commented Mar 3, 2016

Apologies for the delay, but I have a reproduction available now.

The repo lives here: https://github.com/jherdman/issue-3790-reproduction. Run the app, hit the index page in your browser. Note the Ember Data 2.4 complains about the empty response, whereas 2.3.3 does not.

@jherdman

This comment has been minimized.

Show comment
Hide comment
@jherdman

jherdman Mar 7, 2016

Contributor

Hey friends. Should I open a brand new ticket for this?

Contributor

jherdman commented Mar 7, 2016

Hey friends. Should I open a brand new ticket for this?

@bmac

This comment has been minimized.

Show comment
Hide comment
@bmac

bmac Mar 7, 2016

Member

Yes please.

Member

bmac commented Mar 7, 2016

Yes please.

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