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

Explicitly-defined id field on new objects not being sent with POST on save #115

Closed
e3b0c442 opened this issue Jul 15, 2015 · 11 comments
Closed
Assignees
Labels

Comments

@e3b0c442
Copy link

Hello all,

I have several models in my app where the user explicitly sets the id field at object creation.

Beginning with Ember Data 1.0.0-beta.19 up to and including the current ED 1.13.5, this explicitly-set id field is not being sent to the server in the POST data when calling .save().

This has been tested both with EDA 0.5.6 and the current master version after the #108 merge. Reverting to ED 1.0.0-beta.18 and EDA 0.5.6 restores the functionality.

Please let me know if I can provide any additional information that may help.

@dustinfarris
Copy link
Owner

Good catch. Either id is being stripped from the snapshot object (unlikely), or we are somehow blocking the includeId option for createRecord. Will investigate.

@dustinfarris dustinfarris self-assigned this Jul 15, 2015
@benkonrath
Copy link
Collaborator

Ember data has an includeId option that adapters or applications can enable. But maybe we should wait until #114 is in before we investigate too much with the current code.

One question: Should we enable this option by default or should we expect applications to enable it?

@dustinfarris
Copy link
Owner

Yes, will wait for #114 before trying anything.

As far as I can tell, includeId should always happen for createRecord:

https://github.com/emberjs/data/blob/master/packages/ember-data/lib/adapters/rest-adapter.js#L556

@benkonrath
Copy link
Collaborator

@bierdybard If you have a chance, a failing test case would be helpful.

@e3b0c442
Copy link
Author

Hi guys,

Thanks for looking at this.

I'm a rookie on the JS/Ember side so I'm not quite sure how to capture and test against the POST payload. I've just discovered Sinon so I'll see if I can use that to pull something together.

@dustinfarris
Copy link
Owner

@bierdybard I wouldn't use Sinon for this—at least not yet. Take a look at our existing acceptance tests for inspiration. I think you can probably get away with modifying an existing test to assert that the proper POST request is made.

I would start here: https://github.com/dustinfarris/ember-django-adapter/blob/master/tests/acceptance/crud-success-test.js#L127-L145

@e3b0c442
Copy link
Author

Thanks for the pointer @dustinfarris.

I've modified the 'Create record' test to set an ID field and test against the request payload.

Unfortunately, the test still passes with ED 1.13.5 / EDA master. When setting the ID field it does indeed appear in the request payload, contrary to what I'm seeing in my app.

Continuing to dig.

@dustinfarris
Copy link
Owner

Are you testing against the pretender server's handled requests? or are you just checking the returned object?

https://github.com/trek/pretender#hooks

@e3b0c442
Copy link
Author

In response to your latest question -- against the server.handledRequests property.

I found the issue, but I think it may be an Ember Data bug, not EDA.

When the ID is set in store.createRecord:

this.store.createRecord('post', {
    id: 4,
    postTitle: 'New post',
    postBody: 'New post content'
});

Everything is hunky dory.

However, If I don't set an ID on the initial create, or if I try to change the ID after create, the changes do not stick. The POST request either contains no ID or the originally-created ID, even though model.get('id') returns whatever change was made post-creation.

Appreciate your help with this. I'll submit a pull request for the changes to the test because I think it's still valuable to test that behavior.

@e3b0c442
Copy link
Author

Hey all, there is an existing issue (emberjs/data#3360) that covers this issue. I think we can probably close this one.

Thanks for the extra eyes!

@dustinfarris
Copy link
Owner

Good to know, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants