Adding fields on update isn't currently possible
Closed this issue · 16 comments
Due to the loop here: https://github.com/feathersjs/feathers-mongoose/blob/master/src/service.js#L129
It's not possible to update a document and add a field at the same time.
@ekryski why are we setting fields to null? What does that do? Is it to leave the old fields alone? That seems more like how patch should behave.
Wouldn't you use patch anyway?
@marshallswain it's to fill any previous fields that aren't getting updated with null. I have a comment above that code but it must not be clear. We might be able to remove that but I vaguely remember there being issues with inserting a new object with the same ID as a previous one, and removing keys from an existing mongoose model.
I'm not sure your issue is a problem with the loop, or the feathers-mongoose driver for that matter. From what I remember Mongoose doesn't allow you to add fields to your model if they aren't defined on the schema, unless you set up a generic sub document.
My scenario is that I've added a field to my schema. Now I'm trying to fill the field with data, but since we're only looping through the oldObject's keys (which doesn't include the new field) we're losing the new field's data.
I would use patch, but I haven't added patch support to can-connect, yet. :)
hmmm. There might be a bug there then. Maybe we should first loop through the old keys and "zero" them out with null (or try and remove them) if they don't exist in the new object and then just do Object.assign({}, oldObjectNulledOut, newObject);
So looks like in the mongoose docs there is a better solution. We might actually be able to remove the extra _get call as well, which would be a nice performance boost.
this.Model.update({ [this.id]: id }, newObject, {overwrite: true}).then((result) => { ... });That looks good.
👍 that's what it should do IMHO
maybe this should be configurable? pass in something like $overwrite=true in the params/data?
@jamesjnadeau personally I think it should always overwrite it if you are using UPDATE, otherwise you should be using PATCH. I can already see @daffl saying "That's the RESTful way" ha ha.
So I think for now, let's keep that the default behaviour. We can keep the idea of making it configurable in the back of our mind, but I'm wary to add special params that are adapter specific, so if we are to add this it should be across all database adapters.
You're right on both accounts! I switched patch and update in my head when I was thinking about this, sorry :)
Would be nice if this was configurable like we talked about in #51, using options upon initialization....
I'm thinking I'll fix this, this morning but it will be a major release, correcto @daffl?
That's a bug isn't it? Why major?
Wait what was it doing before? The tests definitely expect the object to be replaced on update which it at least seemed to have been doing.
It was technically replacing the entire document, but didn’t support modifying the schema and adding attributes.
On Feb 9, 2016, at 10:55 AM, David Luecke notifications@github.com wrote:
Wait what was it doing before? The tests definitely expect the object to be replaced on update which it at least seemed to have been doing.
—
Reply to this email directly or view it on GitHub #48 (comment).
The only "breaking" difference will be that if you try and access fields that were removed they will be undefined and not null. Still falsy values and I doubt anyone is depending on them explicitly being null at this point.