search

Meteor-Community-Packages / meteor-collection-hooks

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

currentUserId may be lost in a publish function with yielding operations #37

Closed mizzao closed 10 years ago

mizzao commented 10 years ago

I was just reading some of our past discussions about collection hooks. In particular, in considering the use of global variables, it occurred to me that the value of currentUserId might be overwritten when yielding operations happen inside a publish function and a different publish function runs a find operation. I don't think find itself yields, but forEach probably would, and then a find used after that might have the wrong userId.

I didn't know how to solve this at the time, but the proper solution to this would be to bind the publish userId in a Meteor.EnvironmentVariable and call the original publish function using the withValue function of this variable. This would ensure that after any yielding operations, the function will still always see the same userId.

In fact, this is how Meteor.userId() itself is implemented; it's just too bad that it depends on DDP.currentInvocation and is therefore not available in publish functions: https://github.com/meteor/meteor/blob/devel/packages/accounts-base/accounts_server.js#L7

I can implement this and add a test for it when I get a moment - just writing it down for now.

mizzao commented 10 years ago

In meteor/meteor#2221 I suggested harmonizing the userId APIs in method calls and publish functions, but it seems we'll need to convince @glasser of its value first.

mizzao commented 10 years ago

Hey @matb33, I really need a function to determine whether the current dynamic scope was part of a publish function or not, so I added CollectionHooks.isWithinPublish() while implementing this. Hope you don't mind - if anything looks strange, I'd be happy to adjust.

I added tests for this in addition to some of the existing find/findOne tests. However, I could not think of a reliable way to test whether the userId is stored properly across yielding operations. I believe the current tests should suffice to ensure it's working, though - as long as Meteor.EnvironmentVariable is behaving as intended.

mizzao commented 10 years ago

This also fixed a potential error where if a publish function yielded, hooks that run in other fibers before the publish function returned would get a value for userId when there shouldn't have been one. This led to a couple of hard-to-reproduce errors in my app, but I think I finally narrowed it down to this.