mongodb-js / jsonpatch-to-mongodb

Convert JSON patches into a MongoDB update
MIT License
35 stars 27 forks source link

"Can't use add op without position" adding properties to object #12

Open withoutlogin opened 4 years ago

withoutlogin commented 4 years ago

Hi,

I think there's some kind of regression about following closed issue: https://github.com/mongodb-js/jsonpatch-to-mongodb/issues/6

With the latest npm version (1.0.0), I have following issue:

import * as jsonpatch from "fast-json-patch";
import toMongodb from "jsonpatch-to-mongodb";

const obj1 = { key: "value" };
const obj2 = { key: "value", newKey: "newValue" };
const patch = jsonpatch.compare(obj1, obj2); // [ { op: 'add', path: '/newKey', value: 'newValue' } ] 
toMongodb(patch); // throws: Unsupported Operation! can't use add op without position

As you can see patch is fully correct and should result with {$set: {newKey: "newValue"}}.

Thanks.

mrcranky commented 4 years ago

It looks like the new code assumes that adds are only ever adding values to arrays maybe?

Given how the spec is written, I'm not sure it's possible to write a complete Mongo update operation from just the patch document alone? Don't you need a copy of the target document so that you can test for whether or not the path targets properties which already exist, and whether or not they're objects / arrays?

mrcranky commented 4 years ago

Actually, for this case at least, it seems like the spec can be handled without knowledge of the target object. We're dealing with the second and third cases from https://tools.ietf.org/html/rfc6902#section-4.1: o If the target location specifies an array index, a new value is inserted into the array at the specified index. o If the target location specifies an object member that does not already exist, a new member is added to the object. o If the target location specifies an object member that does exist, that member's value is replaced.

Case 1 has been handled in the code, cases 2 and 3 both happen to map to $set, and that will have the desired effect regardless of whether or not the target location already exists. I'll see about making a pull request to cover this case.

ddimitrioglo commented 4 years ago

Any updates on this? Facing the same issue.

mrcranky commented 4 years ago

When the PR wasn't accepted, I ended up going ahead and spending a day or so rolling our own module to do this, along the lines of my previous reply above; i.e. taking the existing document as an additional parameter so that operations can be applied with knowledge of the previous document. Sadly it is not open sourced, though I will ask if our company is okay with publishing it.

ddimitrioglo commented 4 years ago

@mrcranky that would be great! Looking forward! 🤞 😉

ratio91 commented 3 years ago

I have the same issue. would appreciate if you could share your solution @mrcranky. thanks!

gadicc commented 2 years ago

annual ping for @mrcranky to see if you were able to publish this :pray: :grin:

mrcranky commented 1 year ago

annual ping for @mrcranky to see if you were able to publish this 🙏 😁

Okay, so, pre-empting the next annual ping, I have some positive news. I left my previous job, which never got around to giving the go-ahead, and so have had enough time to make https://www.npmjs.com/package/rfc6902-mongodb which is another attempt at the same functionality. It's my first npm module / github open source project, so if folks want to test it out, issues can be reported here and I'll see about fixing them.

gadicc commented 1 year ago

Ah amazing! Thanks so much, @mrcranky, and congrats on your first package! I'll check this out when I'm next working on the relevant package of mine... might be a while, but I've starred your repo in the meantime and thanks so much for reporting back and publishing! :tada:

ddimitrioglo commented 1 year ago

Thank you @mrcranky! I'll definitely keep your new package in mind when I have some related work to do.

Regarding the first package - it's awesome! Small note: do not include tests into the package, and think of using typescript, which would make the maintenance work much easier. Anyway - good luck!