Multi-document updates uses "dirty" modifier from previous calls to the collection hook

[tjramage]

I have noticed that when calling Collection.update with { multi: true } set as an option, the collection.before.update hook is called once per document, however, the modifier parameter is the same object that was used in the previous iteration.

This is dangerous if we are setting modifier values based solely on the individual properties of the document we are currently iterating over.

Is this intended behaviour?

If this is working correctly, then I think the doc parameter should really be the whole set of documents that are going to be updated (an array of docs), as opposed to looping through each document one by one and running the hook each time. The way this is currently working makes it seem like you should be able to work with each individual document, along with a 'clean' modifier in each iteration, allowing you to add/edit/remove properties on each specific update.

It would be good to clear this up for others, as it seems like a pretty big oversight that has likely tripped up many people...

For reference, we are using the latest version of Meteor (v1.8.1).

[evolross]

I'm trying to understand what you mean by this - if you're updating a bunch of documents using the same query and modifier with {multi: true} - how/why would you have the ability to change the modifier mid-update?

You're certainly free to update/overwrite/cancel the modifier for each document's update (i.e. each iteration of the hook). And though likely not optimal for multiple documents, you can even query the database if you need current data from pervious iteration (e.g. counts, flags, etc.)

What's the use-case here?

BTW, there's no one really updating this package - you may want to try checking if there's any currently updated forks.

[tjramage]

@evolross — yep, totally understand that and I did end up manually iterating through each object and updating one by one. The problem is that the hook runs once per document that is updated — misleading the developer into thinking that it's a hook which is running for just a single document update.

I was more suggesting that it's made clear in the docs / readme, or the logic is changed to return a set of documents that will be affected by the modifier (rather than running the hook potentially thousands of times, once for each document...).

Appreciate the library isn't really maintained these days, though. Which is a shame, as Meteor is such a brilliant framework to work with! :)

[vparpoil]

I just bumped into this in one of our apps. Here is the use case :

you use the before.update function to compute the field computed of the collection with something like:

field computed = field a + field b

Let's say you have two documents with b = 0 in one and b = 1 in the second and want to update a value in both items using one update with {multi:true}. In this case, you want the computed field to be updated in both documents with the correct value => and this fails due to the behavior highlighted here.

Propositions of possible ways to fix the behavior of the package :

• when {multi: true}, call the before.update only once and not once per document. Add a mention of this in the documentation
• when {multi: true}, call the before.update once per document and fix the update behavior (seems difficult and could lead to performance issues when updating a lot of docs, which is obviously why one would use {multi:true})

After having a look at the code, both options seems difficult to implement. The only simple thing is to add a note in the documentation

@StorytellerCZ what do you think ?

Thanks !

[StorytellerCZ]

Well, let's start with the note in the documentation to get the simple thing done. @vparpoil since you have broken it down, can you do the PR?
Honestly I prefer the first method better, even if it has its drawbacks and could potentially be breaking.

[vparpoil]

Here is a try for an update to the documentation

If we want to avoid the multiple calls of before.update function when using multi:true, I think we will have to change the api for this case because the before.update hook receive the document about to be updated as a parameter (doc), and this won't be doable anymore. We could send either an array with the documents about to be updated, or null/undefined.