Thought on ActionHero's middleware system

[Danial Khosravi]

Hey,

I've been using ActionHero for a while and I absolutely love it. Thanks Evan !

Coming from ExpressJs background i’m used to using middleware and I really like the way ExpressJs handles middleware (which you can set middleware for all the requests using “app.use()” or you can set specific middleware on routes and you can easily change their order of execution).

Long story short what is happening in ActionHero now is :

• You set middleware using addPreProcessor or addPostProcessor which get executed before and after your actions. You can set a priority value for your middleware in order to execute them in a specific order, however this order is the same for all actions and there isn't anyway to change the order of execution of middlewares differently for different actions
  
  
• Every middleware gets executed on every action and your actions don’t know which middlewares are going to get executed before/after them and have no idea about their order. Although I agree  operation is really fast , but I don’t think it’s good idea to execute every middleware before/after every action call, and then check if that middleware should be run or not especially in applications with a lot of middlewares, for example : if(actionTemplate.authenticated === true) in https://github.com/evantahler/actionhero-tutorial/blob/master/initializers/middleware.js which  have to have some sort of checking in your middleware and still you can’t adjust the order of execution(which matters in a lot of cases)

and I think this has room for improvement.

The short term solution that I came up with in our application is :

1. In my actions I have the middleware names specified as strings in order in an array:
   https://gist.github.com/DanialK/4c0cdbbbdfbe079c1777
2. and I have a middlewares.js initializer which is like this gist:
   https://gist.github.com/DanialK/e8dcb57a5dcea78038ac

This way I have option of running middlewars in different orders on specific actions, and if I were to execute a middleware on all actions, I can directly add it suing addPreProcessor/addPostProcessor.

This is working great for our application, but I guess it would be great if it was part of ActionHero !!

I would like to think of a better solution and work on it and possibly send a PR, I would like to know what you guys think about it and how you're dealing with it in your applications :)

[Clay Himmelberger]

You can set a priority for all pre and post processors. An action will not know what comes next, but you will! For pre processors, an error will prevent further ones from even executing.

This is a common pattern for all types of middle ware in actionhero, even recently added for chat events in v10.

[Evan Tahler]

I *think* this is mostly a matter of personal preference.   I personally like the globally defined middleware + priority pattern (which I'll call "rails-style"), but I understand that others may have a preference for a more declarative pattern (which I'll call "express-style"), as Daniel is suggesting.    Yes, in applications which have an exceedingly huge amount of middlewares, the lookup in a hash will be faster than iterating though a large array, but then you wasting memory-space with the hash.... so who knows. 

I have 3 main reasons for keeping the system as it is now:

1) It's a breaking change

2) As shown in the gist, you can implement "express-style" middleware from "rails-style" middleware fairly easily, but I don't think the reverse is possible without overloading the actionProcessor (haven't given it that much thought yet)

3) The current action middleware has a homogenous API to the other middlewares (connection and chat).  Having a consistent API wherever possible is something I value.  It would be very hard to implement "express-style" connection middleware without modifying stuff deep in the sreverPrototype.  It's not impossible, but I would require all middlewares to have similar APIs, so a changing one of these would be changing all 3.

That said, if the community wants to make the change, we can!  Send up a PR, and we'll leave it open for comments. 

[crob...@medialantern.com]

I'm a little confused about the proposed solution. It seems like the initial concern was performance - calling each middleware function and having it then check actionTemplate.SHOULDIRUNTHIS_CHECKER is an extra step if you know you don't need to do it... But in JS, calling functions is very fast... If the alternative is a string hash and series of if/then checks across it, I think you'd need a special set of circumstances (LOTS of middleware components, and many permutations of required/not required) before you'd see much of a performance difference. It would be interesting to benchmark, anyway.

We haven't been that concerned with performance in this layer. Until recently, we were developing our APIs either in Java (Jetty container) or PHP (Apache/Nginx). We were averaging 30ms for a typical Java request/response and 50ms for PHP. With ActionHero we're down to like 22ms on average - a big improvement. We'll be doing a lot of benchmarking soon, but we doubt we can get this much lower in an app that depends on a database and Redis to do its work. Those are the bottlenecks.

That said, another argument here is that calling the middleware and having every one of them then immediately check "am I needed?" is sort of an anti-pattern. It's not just messy - it also makes it hard for plugin authors to create generic middlewares that are only used in some cases. There's no standard for how action templates should define things like "I don't need XYZ middleware" as keys in their meta-data blocks, so ah-ratelimit-plugin (for example) has a clumsy-but-necessary system where you have to make calls to define limits for specific actions, separately from loading the plugin itself.

Session middleware is the same thing. We disregard session data for anonymous requests, for performance reasons. For some basic requests, we only need to decode the user's JWT, which comes to us via a 'Authorization: Bearer' header. For protected areas, we need to load the whole user object to do permissions-checking. We've defined custom keys in our action templates that control this - but it's pretty hard-coded to our app. It's not something we'd feel comfortable Open Sourcing because they're just random key names that we chose - what if AH Core wanted to use them for something some day?

If there was going to be a change, my vote would be to continue what AH already embraces - be declarative about action definitions to make them easy to maintain. Imagine a 'middleware' key similar to what inputs does now. You could have a very dev-friendly pattern like this:

...
middleware: [{
'ah-rate-limit': {
perSecond: 1,
perHour: 600
},
'session-loader': {
loadFullUser: true
}
}],
...

This kind of thing doesn't need to be a breaking change, because middleware can continue to work exactly the way it does now. It's just an extra array to iterate through when processing middleware before calling an action. But now a plugin or middleware author could call a new function, like registerPreprocessor. This could be a sister to addPreprocessor but it doesn't actually attach itself to the collection of middleware to call on every request. Instead, it just makes it available when the above array was being iterated later. And as suggested above, those middleware callbacks could receive config overrides as a parameter that let you define per-action configs.

Evan, this is a pretty straightforward change IMO and wouldn't be breaking. Would you be interested in a PR that implements it? We're working on a product Beta right now so we wouldn't get to it until next week, but if you thought it might be suitable for 10.x we'd definitely be happy to write it because we're going to be doing some refactoring soon, and this would clean up some of our code. (We might also be able to release our session middleware as a plugin - it supports Auth0 for those of you who do/want to use that service.)

[Evan Tahler]

I'm not quite sure how you would implement the middleware hash without adding some new features.  Right now, middleware can be anonymous functions, so how would you reference them by name?  Or are you just suggesting to name-space configuration options that actions might have?

Either way, of course send in a PR! Looking at code is superior to talking in the abstract :) 

[Chad Robinson]

I think I will - the more I think about it, it would really clean up a
lot of our code.

What I was going to suggest is that, where an initializer or plugin
would normally call api.actions.addPreProcessor() to proactively inspect
every request, this option would add a new API method like
api.actions.registerPreProcessor(). It would have almost the same signature:

api.actions.registerPreProcessor(name, callback, priority) { ... }

You could then do this in an action template:

exports.action = {
name: 'myAction',
middleware: {
'my-session-checker': { priority: 100, loadSession: true,
loadUser: false }
]
};

This method would make the change backwards-compatible. Existing
middleware would simply ignore these values and use global configs. New
middleware could do both - if configured globally, they would
addPreProcessor and work that way, so they can add support for this over
time. And apps can use a mix of both types without changing anything.

I think all that's required to support this is a simple change here:

https://github.com/evantahler/actionhero/blob/decea34c1e508b60faf850751171cf3c42b27c5a/initializers/actionProcessor.js#L126

This block of code is constructing the list of middleware to run, and
has access to self.actionTemplate. All it needs is a few lines of code
to add the additional preprocessors listed in the actionTemplate. If you
think that isn't a hare-brained idea, I'll send the PR tonight.

Thanks,
Chad

[Evan Tahler]

Bump.

I'm curious to hear more folk's thoughts on this! 

[Chad Robinson]

Do you want to see a pull request for it, to make it easier to discuss?

[Clay Himmelberger]

I like the idea, since more flexibility is a good thing. A project I'm working on is not at the point to have custom middleware for individual actions - a generic flag in the action and common middleware works great. But I like the concept here, and should any actions require something individualized, this flexibility is a good thing.

So long as it's a non-breaking change to v10, I say +1!

[Gautam Mehra]

i like the idea too .. selective middleware for actions

 

.. this would avoid going through all the functions and checking is actionTemplate attr.. even though this isn't much of a performance hit (accessing functions and then checking actionTemplate attrs).. its i think something that could/should be avoided 

.. our ah app is growing as we have converting more code over to  node and it would be a great feature for us personally..

+1