Make userId available when a hooked function is called inside a Meteor.publish callback about meteor-collection-hooks HOT 28 CLOSED

So what I think needs to happen is to monkey-patch probably a function in livedata_server.js. Not sure which exactly yet, but it'd be the one that is responsible for invoking the callback specified in Meteor.publish. When our monkey-patched version gets called, we store this.userId in a temporary global, then invoke the normal version of the function. Then when you run our monkey-patched find or findOne within the publish callback, it will grab the global userId and pass it along to the before/after hooks. Once we reach back to our original monkey-patched function from livedata_server, we clear out the global.

from meteor-collection-hooks.

It seems to me like using the global may be dangerous. Because the publish callback is allowed to unblock (the client subscription), this implicitly says that multiple callbacks may be running (even at the same time) in different fibers. Hence there's no clean way to store the right userId in a global. I don't completely understand how the fibers work in Meteor but maybe you can chip in.

The getUserId function also generally needs to be replaced, because that is not usable on the client - there is a different implementation.

from meteor-collection-hooks.

I might be wrong but my interpretation of the execution of JS is that there is always only a single event loop, and Fibers are just doing some high-level stuff to simulate "threads"... but at the end of the day, it's still a single thread. For example, if you put an infinite loop in one of your publish functions, none of your other publish functions will ever run, Fibers or not -- your app will completely stall. That's because the function never has a chance to finish, so the next tick never occurs.

So as long as the execution path of our code never goes off into an async path (one where execution continues in the next tick) , we are guaranteed that our variables will be as expected.

I'm certainly no expert though, this is just piecing disparate knowledge together. I quickly googled "js event loop" and came across this: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/EventLoop
Specifically, I think we're taking advantage of "Run-to-completion"

from meteor-collection-hooks.

Okay...thanks for explaining that!

I added some client-server tests that check for a userId being loaded properly in the hooks. Client environment and server methods work; the big issue is server publish which we need to implement. See here:
https://github.com/matb33/meteor-collection-hooks/blob/server-userId/client_server_userId_tests.coffee

Sorry it's in coffeescript. However I work faster with coffeescript and I thought it would be okay for testing only. I did keep the main files in JS and even converted my spaces to tabs, though I disagree with using tabs :) I left your other test file alone as well, although you can either use this one or edit that one to add more client-server tests. We can add in a test for publish when we figure out how to do that properly.

Meteor really needs a better testing framework for this kind of stuff :)

from meteor-collection-hooks.

I've been trying to write this functionality and I hit a roadblock. The function I need to monkey-patch is maybeAuditArgumentChecks (last function in livedata_server.js). Unfortunately it's wrapped in a closure by the bundling process. Specifically, the call is that last line return f.apply(context, args); where f is the callback the user defined in Meteor.publish.

I'll hold for now while I think about another approach. Only thing I can think of at the moment is to wrap f beforehand...

from meteor-collection-hooks.

Another approach may be to just change something in the Meteor code so that userId is available as a global somewhere during publish functions. That would probably be a much easier and straightforward approach to take. If we see the collection hooks as something that will get integrated into Meteor eventually, this makes sense.

from meteor-collection-hooks.

Yeah, it might be time to formalize a pull request over to Meteor. It's a bigger undertaking than what we're doing though -- it'd be a complete rewrite. I say this because collection-hooks, as great as it is, isn't Meteor core quality. Specifically, the hook callbacks should be on a per-document basis, just like allow/deny.

If we could get to that point, i.e. a more "natural" extension of what Meteor is already doing with allow/deny, I think we'd stand a chance at integrating this into Meteor core.

from meteor-collection-hooks.

OK...let's think a bit about how to do this and I'll work with you to get this completed properly. Trying to monkey-patch this functionality is going to be very difficult and unmaintainable especially if the undocumented APIs change.

For now, I've just given the monkey-patched find and findOne an extra userId argument, which I'll use in my application and tell others to use until we figure this out.

from meteor-collection-hooks.

Sounds good. We'll have to start a dialogue with Meteor to find out if they are even interested. They may have other ideas to accomplish similar functionality. I'll ask around

from meteor-collection-hooks.

Digging through the meteor-core group, someone though up an interesting approach: https://groups.google.com/forum/#!topic/meteor-core/DzfX-ZirrtA

It would allow hooks to get fired even when the database is changed directly.

from meteor-collection-hooks.

I managed to get it working. The before and after hooks for find and findOne have access to this.userId when used within Meteor.publish: 8ccd52f

from meteor-collection-hooks.

What's the impetus for giving it in this instead of just as an argument for everything else?

For example, if find is used on the client, it should use Meteor.userId() as it did before. (Can't think of a use case off the top of my head, but seems to make sense to support.) It seems like now the userId will only be available for publishes.

from meteor-collection-hooks.

Ah yes good point, I was focused on how userId was delivered to Meteor.publish, i.e. as this.userId. I was mimicking this behavior. Since the before and after hooks are our own to mess with, you're right we should just pass userId as the first parameter to match the rest of our before/after API.

from meteor-collection-hooks.

I think this.userId is a bit of a hack on their part anyway. I'm betting by 1.0 there will be a better way to access context in publishes.

from meteor-collection-hooks.

Updated to pass userId as first param: 0efcbb5

from meteor-collection-hooks.

Yes, this approach matches the other methods better as well as allow/deny callbacks.

I'm going to make a post to meteor-core just to verify that this approach isn't going to break anything or be messed up in future releases. It will also be a good opportunity to make a pitch for supporting better hooking operations on collections and hopefully getting this stuff implemented in upstream eventually :)

Follow https://groups.google.com/forum/#!topic/meteor-core/JGA_LQDEqw0

from meteor-collection-hooks.

Question about https://github.com/meteor/meteor/blob/master/packages/accounts-base/accounts_server.js:

Do you understand what it says with the part

//The way to make this work in a publish is to do
// Meteor.find(this.userId()).observe and recompute when the user
// record changes.

from meteor-collection-hooks.

I think the comment meant to say Meteor.users.find(Meteor.userId()).observe ? I'm not sure it matters anyway, since this.userId is accurate within publish

from meteor-collection-hooks.

No, I think it's talking about doing Meteor.users.find(this.userId).observe inside the publish, because you're interested when userRecord.foo changes, you might want to publish something different depending on the value of foo. However, even given this, I'm not sure how you'd be able to re-publish something given that the publish callback has already run. That's what confuses me.

from meteor-collection-hooks.

Hi @matb33,

I was just thinking about how to ensure that the userId works correctly in publish functions and wanted to just follow up with what you said before about Fibers and yielding. I independently realized what I see that you already said before - as long as the Fiber in a publish function doesn't yield due to an async operation (to another publish function), the state of the global userId will be consistent.

However, what types of Meteor operations would cause the Fiber to yield and possibly mess up the stored value of userId in the next tick?

Is there any way to store the value of the current userId in the Fiber that is computing the publish function instead of a global, as is done with Meteor._currentInvocation in accounts-base?

https://github.com/meteor/meteor/blob/master/packages/accounts-base/accounts_server.js

from meteor-collection-hooks.

@mizzao I just realized I never answered your question. This sounds like the job of Meteor.bindEnvironment (maybe)

For the "refactor" branch, I'm not trying anything fancy to get the userId for the find/findOne hooks unfortunately. I'd rather think of a more solid approach.

from meteor-collection-hooks.

@matb33 I've been out of the country for a couple of weeks but now I'm back and ready to help you get all this working.

The ability to have userId in find and publish is really important to me. Have you been tackling that in the refactor branch? (I remember you said you were going to hold off for a bit.) What's the summary and how can I help?

from meteor-collection-hooks.

@mizzao I wonder if passing this.userId as a fake option in your find could work for now?

Meteor.publish("test", function () {
  return coll.find({abc: 1}, {userId: this.userId});
});

In the collection hook, since you get access to the options, you could take the userId, do whatever, and clean it up:

coll.before.find(function (userId, selector, options) {
  userId = userId || options.userId;
  options.userId && delete options.userId;

  // Do whatever with userId, like modify selector etc
});

from meteor-collection-hooks.

@matb33 Remember our discussion about making all of the collection operations transparent to the user, so that they wouldn't need to deal with a changed API? I can use this for now, but it's a stop-gap measure. I'll see if I can propose a better way to do things.

Another question: is that delete really necessary? Would the garbage collector not take care of it?

from meteor-collection-hooks.

@mizzao yeah I recall the convo, and yeah it would be nice for sure. I'm just hesitant to do anything further with that userId trick until we close off your issue with collection-hooks messing up your inserts etc

The delete isn't necessary, it's just a personal preference -- i.e. I didn't want the underlying find to even see the fake userId field. Probably won't care, but without looking at the find source, I can't say for sure if maybe find may throw an error if an unrecognized option is passed along. If not, great, no need for delete. But just playing it safe.

from meteor-collection-hooks.

After writing a MySQL driver for Meteor, I've learned some new tricks that can apply here. I'm going to take another stab at making the userId available in the find hooks.

from meteor-collection-hooks.

@matb33 I thought your current version already did that? In any case, looking forward to it.

from meteor-collection-hooks.

My tricks didn't make any difference -- same general approach as last time. Open an issue if this change causes any problems!

from meteor-collection-hooks.