Added handling of push/pull and arrayFilter operators

[vinerich]

Summary

MongoDB supports update operations like $set, $pull, $push and arrayFilters. In the initial support for updateQueries (48fef773b5cfd91cd422a05c9e2a660dc9b9f5fd) only $set operations were supported.

See https://github.com/codepunkt/mongoose-patch-history/pull/30#issuecomment-452579189 for reference.

@codepunkt The culprit was this line which builds up the query used to find the updated documents after the update has been applied:

https://github.com/codepunkt/mongoose-patch-history/blob/450c5ebc6c2e1270cab5de636882d4b08ddae393/src/index.js#L266-L270

For an update like Post.updateMany({ tags: 'cats' }, { $push: { tags: 'dogs' } }) the resulting query conditions would be { tags: 'cats', $push: { tags: 'dogs' } } which is not valid and was being rejected by Mongo.

This PR instead stores the _id values of the documents that are going to be updated along with their data so that they can be looked up directly instead of having to build up a condition which will match all of the same documents. I can't think of any cases for which this approach would not work, but if there is anything that you can think of I'd happily adjust my solution to cater for those.

This PR includes the changes from #30 to support $push and $pull operators, credits go to @bausmeier. This changes were only implemented for updateMany() calls so I used the same pattern to support updateOne() calls. For updateOne() calls a variation had been made to support upserting of documents.

The same logic works for arrayFilters since the problem didn't change. (I can elaborate this further if wished.)

If anything is unclear about the included changes or the variation made to support upserting let me know.

Edit: If this PR gets merged this closes #24 too. The pattern for all other MongoDB update operations (like $inc https://github.com/codepunkt/mongoose-patch-history/pull/24#issue-205808641) is the same and gets solved through #30 and the small changes made by me.

[vinerich]

The tests are failing because arrayFilters are not known to the mongoDB version used by CI at the moment. If you merge #60 and rebase the failing tests will disappear.

[codepunkt]

@vinerich Thanks, sounds good! Will this be backwards-compatible or break code used with previous versions?

[vinerich]

I didn't modify old tests so I'd say this is not breaking.

For myself, I used this bug to push changes to a collection without creating patches. I don't know if anybody used it this way but if yes, that would be kind of breaking. But as they relied on a bug idk how to handle this.

To support the described behaviour, I'll take on #33 later this day to provide the exclude option with documentation for readme.

[vinerich]

Note that upserting with updateMany() will not work. But I'm not sure that this worked before the changes. I believe it didn't work before.

[codepunkt]

Note that upserting with updateMany() will not work. But I'm not sure that this worked before the changes. I believe it didn't work before.

To be honest, i don't remember. It's possible that it didn't work. Aside from that, now that i merged your other two PRs, this PR has conflicts in tests that i can't resolve via GitHub UI - it would be nice if you could do that by merging master into your branch and then updating this PR.

If you're interested, you can also add yourself as a contributor to the package.json 👍

[codepunkt]

I didn't modify old tests so I'd say this is not breaking.

For myself, I used this bug to push changes to a collection without creating patches. I don't know if anybody used it this way but if yes, that would be kind of breaking. But as they relied on a bug idk how to handle this.

To support the described behaviour, I'll take on #33 later this day to provide the exclude option with documentation for readme.

In that sense i'd consider it a breaking change, because what you did before won't be possible anymore (even though the previous behaviour wasn't exactly planned). Looks like we'll release a 2.0.0 soon 😉

[vinerich]

In that sense i'd consider it a breaking change, because what you did before won't be possible anymore (even though the previous behaviour wasn't exactly planned). Looks like we'll release a 2.0.0 soon 😉

It is breaking in some way yeah. So I'm good with 2.0.0. I'll try to tackle #33 in the evening and maybe take on #35 too. I'd like to directly include both changes so you don't need to do the release process twice.

[vinerich]

To be honest, i don't remember. It's possible that it didn't work.

From my opinion one shouldn't upsert with updateMany() anyway so that's ok 😄 I looked at the old code and I'd say it could've worked in the past, but it was not covered by tests. I could try to get this behaviour (adding a patch when upserting a document with udpateMany()) included but I'm not sure if its worth the effort.

Aside from that, now that i merged your other two PRs, this PR has conflicts in tests that i can't resolve via GitHub UI - it would be nice if you could do that by merging master into your branch and then updating this PR.

Done

If you're interested, you can also add yourself as a contributor to the package.json 👍

I think I can do that. I'd say I also add @bausmeier to this if he and you are ok with that? Atleast it was his code and approach that I used.

Further I'd also add @RedHatter for #33 and #35 since I'm planning on using his code and tests and just provide a small additional feature and documentation for the README.

Edit: Ok I thought the other guys which provided some code were in the contributing too. I'd give the decision too you then. I'll certainly add me (thanks for that).

[codepunkt]

From my opinion one shouldn't upsert with updateMany() anyway so that's ok 😄 I looked at the old code and I'd say it could've worked in the past, but it was not covered by tests. I could try to get this behaviour (adding a patch when upserting a document with udpateMany()) included but I'm not sure if its worth the effort.

That'd be nice, but that's a question of effort. If you'd be able to provide that, we could merge it in! 😏

If you're interested, you can also add yourself as a contributor to the package.json 👍

I think I can do that. I'd say I also add @bausmeier to this if he and you are ok with that? Atleast it was his code and approach that I used.

Further I'd also add @RedHatter for #33 and #35 since I'm planning on using his code and tests and just provide a small additional feature and documentation for the README.

Sure, go ahead! 👍

[vinerich]

I'd consider this finished.

[codepunkt]

Merged. Thanks for your efforts! Is there anything else that you think is missing or are you good for now with the master?

[vinerich]

Regarding your questing of anything missing, I'd like to have #63 and #64 included in the release since this are both features I'd like to use.