Meteor-Community-Packages / meteor-collection-hooks

Meteor Collection Hooks
https://atmospherejs.com/matb33/collection-hooks
MIT License
657 stars 92 forks source link

Collection updates ($inc) in hooks running twice and double incrementing server values. #140

Closed evolross closed 9 years ago

evolross commented 9 years ago

In a Collection JS file in my collections folder (both client and server) I have an after.update hook like this:

Widgets.after.update(function(userId, doc, fieldNames, modifier, options) {

    if(modifier.$set && modifier.$set.approved === true) {
        //  Increment gadget.totalPurchases - THIS IS GETTING CALLED TWICE
        Gadgets.update(doc.gadgetId, {$inc: {totalPurchases: 1}}, function(error) {
          if(error) {
            // error code
          }
        });
     }
});

I assume this is because it's running on both client and server. However, your documentation says that it "works on the client, server, and mix". Is this a bug? If you use $set this doesn't happen because the same value gets set on both client and server. But an $inc increments twice and saves all the way to the server a value that's doubly incremented and wrong.

If you're doing client/server collection hooks, is the idea to convert any $inc updates to $set updates before you use them in a collection hook to prevent this from happening? If so, you should update the documentation.

Same problem here:

http://stackoverflow.com/questions/29092731/collection-before-hook-called-twice

The work-around is to wrap all my collection hook code in Meteor.isServer brackets. Is there a drawback to not having collection hooks on the client? It seems to work just fine from the client this way.

evolross commented 9 years ago

I found another issue sort of related to this. Let's say I indeed use a $inc to update a collection (Gadgets above) and then I have a second before.update hook on the Gadgets collection to set a flag in the modifier object based on totalPurchases exceeding a threshhold before it writes. In the Gadget's before.update hook the modifier object looks like {'$inc': {totalPurchases: 1}}.

If in that hook I want to add a set to the modifier object with a $set statement before the update to set some flag like to true: modifier.$set.flag: true, it returns an error.

Can you not mix or "add to" modifiers? If not, it just makes sense to convert any $inc updates to $set updates anyway since it limits you to what you can do in a before hook.

evolross commented 9 years ago

Looks like there's an issue converting an $inc to a $set... they resolve asynchronously and won't actually increment the database in time if a lot of objects are added at once. So this isn't a solution.

More details here: http://stackoverflow.com/questions/31617512/mixing-collection-hooks-modifier-operations-in-hook-causes-error

matb33 commented 9 years ago

I'll close this as it appears to be all related to your same issue report

evolross commented 9 years ago

My other post was just me trying to solve this issue. If you put a $inc collection hook in a client/server code-space, it still gets called twice. I might mention this in your documentation - to explain wrapping a hook in Meteor.isServer or placing it in a server-only file. That or elaborate further on what "Works across client, server or a mix" means exactly.

As a double-increment all the way to the server isn't expected behavior.

matb33 commented 9 years ago

I agree the documentation could be clearer on this.