Make userId available when a hooked function is called inside a Meteor.publish callback

[mizzao]

Hooks are great when used on the client. However, they are a bit hobbled on the server. It would be great to use them naturally inside publish functions.

This entails two issues. The first is just trying to set the userId at all:

Meteor.Collection.prototype.find = function (doc, callback) {
        var result, userId = getUserId.call(this);

Even if we do something like foo.find.call(this) inside a publish, the getUserId call does not work properly. It'll throw an error like TypeError: Object #<Object> has no method '_getFindSelector'. So there's no way to hook the userId into a server-side find right now.

Second, it would be great to somehow make this.userId directly available to the hook, without having to change the API usage. i.e, foo.find() should be callable as normal inside a publish, and the find can be filtered by userId or whatever.

A workaround for now would be to allow the userId to be passed in as an optional argument to the functions. This at least makes it possible, although doesn't fix the change to the API.

[matb33]

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.

[mizzao]

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.

[matb33]

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"

[mizzao]

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 :)

[matb33]

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...

[mizzao]

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.

[matb33]

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.

[mizzao]

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.

[matb33]

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

[matb33]

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.

[matb33]

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

[mizzao]

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.

[matb33]

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.

[mizzao]

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.

[matb33]

Updated to pass userId as first param: 0efcbb5

[mizzao]

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

[mizzao]

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.

[matb33]

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

[mizzao]

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.

[mizzao]

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

[matb33]

@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.

[mizzao]

@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?

[matb33]

@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
});

[mizzao]

@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?

[matb33]

@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.

[matb33]

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.

[mizzao]

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

[matb33]

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