Always set "id" field during PUT updates

[jfexyz]

When performing a PUT update action, the id field must be set. Otherwise, the object loses this field and becomes an orphan.

I did not write a test, because this seems like the sort that should go into https://github.com/feathersjs/feathers-service-tests.

Fixes #25.

[daffl]

👍 that's exactly what I was going to suggest.

@ekryski agree?

[jfexyz]

And it seems there already is a test that should catch this: https://github.com/feathersjs/feathers-service-tests/blob/master/src/common-tests.js#L400

[daffl]

Yes but we are not testing non-standard id fields at the moment.

[ekryski]

I'd maybe put an if around this rather than completely remove it.

if (this.id === '_id') {
  delete data[this.id];
}

because I'm pretty sure that mongo will bomb if you try to do an update with an _id and are using the standard _id field.

[daffl]

So

if(this.id === '_id') {
  delete data[this.id];
} else {
  data[this.id] = id;
}

?

[ekryski]

Isn't the idea with PUT that should be sending the whole resource (including the id)? If we add the custom id back into the data object then you can make a request like so:

// PUT /messages/1
{
  "text": "oh hai!"
}

instead of what is required by every other adapter (I'm pretty sure):

// PUT /messages/1
{
  "text": "oh hai!",
  "id": 1
}

I'm ok with that. I just want to make sure we are on the same page because the other database adapters don't behave like that. For PUT's, whatever you want to store needs to be passed in the request body (ie. data). For PATCH whatever you want to change or add is sent in the request body (ie. data).

[ekryski]

So my thought was we would only need:

if (this.id === '_id') {
  delete data[this.id];
}

and the custom id field that is meant to be preserved needs to be passed. It just shouldn't be removed.

[daffl]

I don't think we really defined how other adapters behave with custom ids. Here we definitely do have to add it back as the resource would be orphaned otherwise and I don't think you should be able change an id (although we could check for if(!data[this.id])). We should probably add the same logic in patch.

[ekryski]

Yup @daffl is right. We'll need to do the same thing with patch. Let's merge this, I'll update the patch method right now and do a patch release.

[jfexyz]

I agree that you shouldn't be able to change a primary key, but the released code allows that when you're not using _id as your primary key.

[jfexyz]

i.e. We need to answer the question "Can a primary key be changed during a put/patch?" That was the reason for the logic I wrote; I assumed the answer to that question is no. But if yes, then we should also allow~ _id to be changed, if not, then we should always ignore what is passed in the body.

update I was under the impression that you could change mongo _ids by wrapping in ObjectID(). not sure where I got that, but obv is wrong.

[daffl]

Not being able to change _id is a database restriction we can do nothing about. I would say we should allow changing any other id because it is much easier to add a hook with delete hook.data.id if you want to prevent it than not having the option at all.

[jfexyz]

But remember, a hook with delete hook.data.id will create orphan resources, so it really needs to be hook.data.id = id...

[ekryski]

@daffl is right. We currently don't have tests in place for custom id fields and adding tests for it will a required a rewrite of the common tests to use fixtures instead of using the system under test to setup and tear down test data.

Even though the tests look to have passed, I'm ok accepting this, if we wrap an if statement around that section just to be safe.

[ekryski]

Apologies @joshuajabbour. That must have been a nasty bug to track down.

[jfexyz]

Yeah, I can confirm Mongo dies if you try to update the _id (unless you wrap it in mongo.ObjectID(_id), which is superfluous).

Pushing the conditional

[ekryski]

Thanks @joshuajabbour!