Allow policies to run in series or parallel

[devinivy]

Allow policies to run in series or parallel

[mark-bradshaw]

@devinivy So, one thing that MrHorse does is ask each policy to specifically indicate whether handling further policies should happen or not. So you can bail out at any place with a specific error. One thing I'm unclear on is what happens if you are running a few policies in parallel and a couple of them want to respond back with an error. Who wins?

So I'm a bit concerned that this might unnecessarily complicate things. Do you have a use case for yourself that needs parallel policies?

[devinivy]

That's a good point! My use-case is just trying to save some time by allowing IO to happen in tandem.

I can see why you would want to avoid the added complexity here, but I do have a proposition for how that could work if you decide you're interested. You basically resolve all successes/errors in a set of parallel processes then, to make sure the server behaves deterministically, you take into account the order that they are listed in configuration when choosing the error response. The Items module used by hapi has something that caters to this sort of behavior: https://github.com/hapijs/items#itemsparallelexecutetasks-callback

[mark-bradshaw]

Seems like a good change. I'll add it to the queue.

[devinivy]

Cool! Another way to look at this that could achieve the same effect:
If you allow policy modules to be passed directly into route config, it would be easy enough to create a helper that composes multiple policies into a new policy that runs them in parallel and handles the error response. By default the helper could use the specs outlined above.

Ex.

var MrHorse = require('mrhorse');

// In route config
{

/* ... */

  config: {
    policies: [
      'loggedInToday',
       MrHorse.parallel('isAdmin', 'hasRights', 'smellsGood'),
      'moreStuff'
    ]
  }

/* ... */

}


/* or */ 


// In route config again
{

/* ... */

  config: {
    policies: [
      'loggedInToday',
       MrHorse.parallel('isAdmin', 'hasRights', 'smellsGood', function(errs) {
         // handle parallel errors in some custom way
       }),
      'moreStuff'
    ]
  }

/* ... */

}

At that point, passing ['isAdmin', 'hasRights', 'smellsGood'] could just be sugar for MrHorse.parallel('isAdmin', 'hasRights', 'smellsGood'). If you're interested in this approach, I may be able to pull together a PR.

[mark-bradshaw]

I'd be very interested in that approach. And PRs are always welcome! :)

[devinivy]

Not finished by any means, but I've prodded at this in my free time: master...devinivy:master.

Any idea what a nice way would be to use default apply point settings when dynamically running a policy? Right now I'm just asserting that a dynamic policy in route options needs to specify its apply point explicitly.

[mark-bradshaw]

Looks like a good start. Thanks.

On Sun, Jul 5, 2015 at 10:54 PM devin ivy [email protected] wrote:

Not finished by any means, but I've prodded at this in my free time:
master...devinivy:master
master...devinivy:master

—
Reply to this email directly or view it on GitHub
#11 (comment)
.

[devinivy]

Still plugging away at this, and looking for some input if anyone has thoughts.

The main issue is that at the time MrHorse.parallel('policy1', ..., 'policyN') is called in a route's configuration, we shouldn't assume that those policies have been loaded– policies may be loaded before or after the routes are configured. That means we can't figure out which apply point to use until the route is being used, and furthermore we can't tell if there is even a well-defined apply point given the list of policy names without their individual settings.

I've opted to sidestep this by allowing policy functions to provide a property that contains a list of policies by which the handler for an apply point can determine if it should run the raw policy that MrHorse.parallel returns. Does that make any sense? I was wondering if anyone has a better option!

I'm also keeping performance in mind. Do we really want each apply point handler to be spending time trying figure out if it's allowed to run a policy? I suppose it's relatively fast, but I do miss the simplicity and quickness of the policy-apply-point lookup table.

Is it possible that we can/should cache the list of policies to run per route?

[mark-bradshaw]

Just to make sure I'm clear on what you are thinking, are you wanting to have a lookup table for each route+applyPoint? I'm ok with that. It'd certainly make the handler stupid simple, while the addPolicy function would get a bit more complicated. I definitely am in favor of shifting more processing up front, and less on each request.

And are you concerned that you can have a misconfigured route, and we can't notify until the route is used for the first time? So, you'd like to have something more deterministic?

I didn't track when you said: "I've opted to sidestep this by allowing policy functions to provide a property that contains a list of policies by which the handler for an apply point can determine if it should run the raw policy that MrHorse.parallel returns." Can you expand/clarify that for me?

[devinivy]

It's a lot easier to see this in the PR I just posted than it is to explain. Basically, now there are two different types of policies: those loaded then referenced in a route config using a string, and those added dynamically into the route config as policy functions.

The policies that are loaded are fine! There's already a simple lookup table in data that allows us to quickly compute which policies should run on the route. But the dynamic policies need a little more work, particularly in the case of MrHorse.parallel. Here are the two main issues:

1. We need to apply the plugin's default apply point in the absence of an explicit one at the time of running each extension handler.
2. We need to make sure that the list of policies passed to MrHorse.parallel all have the same apply point, again, at the time of running each extension handler.

I'm toying with ideas to pre-compute or cache the answers to those two questions. In PR #17 you can see my current solution, which is a little more naive than that.

[devinivy]

I ended-up memoizing the function that determines the apply point based upon a list of policy names. Feels pretty good for now!

[mark-bradshaw]

Awesome. I need to focus on a side project tonight so I can't look over
what you're doing with enough brain juice to be useful right now.
Hopefully tomorrow. If you feel like you're on the right track then I
think you should just keep going.

On Tue, Jul 7, 2015 at 1:39 PM devin ivy [email protected] wrote:

I ended-up memoizing the function that determines the apply point based
upon a list of policy names. Feels pretty good for now!

—
Reply to this email directly or view it on GitHub
#11 (comment)
.

[devinivy]

Cool! No huge rush on this on my end. I feel like the PR has settled down. It's fully tested, and I'm feeling mostly satisfied, but it definitely needs a second look.

[mark-bradshaw]

I've merged in #17. Would you mind doing one more little thing? Would you mind adding a small example to the example folder showing parallels in action?

[devinivy]

👍 will do. This issue can save as a placeholder for that to be done.

[devinivy]

PR #21 will hopefully round this out!