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

Log an assertion if the response from createRecord does not have an i… #4408

Merged
merged 1 commit into from
Jun 24, 2016

Conversation

bmac
Copy link
Member

@bmac bmac commented May 27, 2016

…d and the id was not already provided by the client

Closes #4346

…d and the id was not already provided by the client

Closes emberjs#4346
@bmac
Copy link
Member Author

bmac commented Jun 10, 2016

ping @emberjs/ember-data-contributors this is ready for review.

@pangratz
Copy link
Member

👍 this looks good to me! Feel free to merge - I haven't because I was wondering if this should be prefixed so it lands in beta?

@bmac bmac merged commit 380299c into emberjs:master Jun 24, 2016
@bmac bmac deleted the 4346-assert branch June 24, 2016 16:26
@pangratz
Copy link
Member

🎉

@OmShiv
Copy link

OmShiv commented Sep 16, 2016

With this update and assertion logging, my success callback is not executed. The control flow goes instead in the error call back.

record.save().then(
   () => {
       // success callback
       // before ember-data 2.8.0
   }, 
   () => {
       // error callback
       // after ember-data 2.8.0, with EmberError log for missing id.
   }
);

@fivetanley
Copy link
Member

That behavior is intended @OmShiv. Ember Data doesn't support records from the server which don't have IDs.

@fivetanley
Copy link
Member

fivetanley commented Sep 16, 2016

Local records without IDs created using store.createRecord are, however allowed. If you want to save them, you'll need to return an ID from the server.

@OmShiv
Copy link

OmShiv commented Sep 16, 2016

@fivetanley
My APIs require a PUT for creating a record for the first time. And POST for update then onwards.
When I do:

record = store.createRecord(modelType, data);
record.save().then( ... )

It fails client side, since I don't jump in success callback. Though the n/w call succeeds.

Now if I set a random id, say record.set('id', 1) before the save, it works on client side without errors. But then, the updates don't work.
record.save() doesn't work thereafter, even if I manually set the id to 1 every save attempt.

@fivetanley
Copy link
Member

Does your server return an id for the PUT request to create the record?

@OmShiv
Copy link

OmShiv commented Sep 16, 2016

none of the PUT requests in our APIs return the id. Is there a way I can set id in a serializer to the payload?

@fivetanley
Copy link
Member

You could set a fake ID in the serializer, but I'm a little confused on to how you would then update it later without an ID the server knows about. From the server's perspective that could be a different record.

@runspired
Copy link
Contributor

@fivetanley @bmac this is actually breaking. I suspect that at a minimum the assertion needs moved within the conditional, but also not all payloads return IDs, previously a null ID for a record that already knew it's ID would work, now it will not. So I also suspect this should warn with deprecation, not fail. Finally, I'm seeing situations in which although an ID is provided client side, this assertion is still triggered: it appears to be when createRecord is used to generate a record that never actually receives an ID (yes, we have those, TL;DR think a message)

Working on a fix.

@runspired
Copy link
Contributor

Fix is being worked on here: #4544

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.

5 participants