Add support for route middleware

[mjdickinson]

Allow middleware to be added per route in order to support this style of route declaration:

api.post(
	'/user',
	authenticate(),
	authorize({ role: 'admin' },
	validate({
		body: { /*...schema...*/ }
	})
	async (req, res) => {
		/*
		   business logic
		*/
		return {}
	}
)

...where authenticate, authorize and validate are middleware (factory) functions.

[jeremydaly]

This is completely possible. Just to confirm the format:

api.post(
  '/user',
  authenticate(),
  authorize({ role: 'admin' }),
  validate({ body: { /*...schema...*/ } }),
  async (req, res) => {
    /* business logic */
    return {}
  }
)

What would the signature be of these factory functions? Middleware in Lambda API requires the acceptance of three parameters, REQUEST, RESPONSE and next(). Would you expect authorize({ role: 'admin' }) to return a function with that signature so that it could be processed like normal middleware? Or are you envisioning something else?

[nickmeldrum]

I was just about to add a similar request because I wanted some of my middleware to only operate on a particular method as well as endpoint.

I was thinking that the post() and get() methods etc. could allow a next() method passed in, which is I think possible in express?

However it looks like this request would also satisfy my request?

[nickmeldrum]

and my 2cents would be exactly as you describe - I would expect my middleware function to have to have the same signature whether I include it like the OP suggested or if I include it using the api.use() method.

[jeremydaly]

Not sure we would want to allow next() to be passed into route methods, but if route specific middleware uses the same signature as use(), then it's entirely possible to read the last parameter as the handler and then any proceeding functions as middleware. The middleware would be passed the REQUEST, RESPONSE and next() parameters and would control the execution flow just like normal middleware.

If that is how it is envisioned, then it should be fairly simple to implement.

[mjdickinson]

Yes, that's exactly what I had in mind. Middleware is middleware, whether it's used in use() or in METHOD(). I see in the current implementation middleware processing and route handling are implemented independently. The simplest solution would be to add route middleware processing into route handling. But then you end up with middleware being processed in two places: one for use() middleware and one for route middleware. Do you think it is worth refactoring towards a more unified approach, where use() middleware is also processed as part of route handling?

…

On Thu, Dec 20, 2018 at 8:48 AM Jeremy Daly ***@***.***> wrote: Not sure we would want to allow next() to be passed into route methods, but if route specific middleware uses the same signature as use(), then it's entirely possible to read the last parameter as the handler and then any proceeding functions as middleware. The middleware would be passed the REQUEST, RESPONSE and next() parameters and would control the execution flow just like normal middleware. If that is how it is envisioned, then it should be fairly simple to implement. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#87 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABwi8_d5F1wezl0HzY60ShMEDelKOoGdks5u679EgaJpZM4Za4zn> .

[jeremydaly]

We could move middleware processing into the route, but mappings are all preprocessed anyway. So, it might be better to add method filtering to middleware, and then just map it correctly when processing route declarations. That way middleware processing doesn't need to change AND you get the benefit of adding use to restrict methods if you wanted to do it that way.

I'm thinking this is just a convenience method, and really doesn't change the underlying processing too much.

[mjdickinson]

You're right, that's probably the simplest. It just seems a little inelegant to me having routing logic implemented twice: once to lookup the applicable middleware for an incoming request, and once to lookup the handler. Also, with the style of declaring middleware in route declarations, the list of registered middleware functions could get quite large - and it gets iterated over on every request. This seems a shame given that you already have the *_routes* tree data structure for efficient handler lookup. The routing logic could be unified by including middleware in the *_routes* data structure (instead of having a separate *_middleware* list). Then you'd get efficient lookup of both middleware and handler. If you do want to leave the existing middleware implementation as it is, it would be possible to use *_routes* for route middleware only. Or, as you suggest, add method filtering to the existing implementation. Let me know what you think.

…

On Thu, Dec 20, 2018 at 10:45 AM Jeremy Daly ***@***.***> wrote: We *could* move middleware processing into the route, but mappings are all preprocessed anyway. So, it might be better to add method filtering to middleware, and then just map it correctly when processing route declarations. That way middleware processing doesn't need to change AND you get the benefit of adding use to restrict methods if you wanted to do it that way. I'm thinking this is just a convenience method, and really doesn't change the underlying processing too much. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#87 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABwi83A-MoNO9-9ZIVwiIsjUhu_ZMuxWks5u69q9gaJpZM4Za4zn> .

[jeremydaly]

Hmm, I see what you mean. Could be useful to use the nested routes tree to store middleware mappings. I'd need to think about how that would work for global middleware though as we'd have to handle wildcard routes too. I wouldn't want global middleware to be tied to each route though, seems as though we could do that higher up the tree.

[mjdickinson]

Agreed. Global middleware should be attached to the root node (i.e. path /*). Other wildcard middleware is attached to the appropriate node as wildcard middleware (not route specific). When the tree is walked for an incoming request, applicable wildcard middleware (including global) is executed as you go. If a matching route is found, its middleware (global then route specific) is executed before the handler.

…

On Thu, Dec 20, 2018 at 12:22 PM Jeremy Daly ***@***.***> wrote: Hmm, I see what you mean. Could be useful to use the nested routes tree to store middleware mappings. I'd need to think about how that would work for global middleware though as we'd have to handle wildcard routes too. I wouldn't want global middleware to be tied to each route though, seems as though we could do that higher up the tree. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#87 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABwi88BeoC9kKXagtRbjFAXn7ijRr3Loks5u6_GJgaJpZM4Za4zn> .

[jeremydaly]

I'm thinking about changing the entire execution stack so that even the 'handler route' is just like another piece of middleware, similar to how Express creates their middleware stacks. They essentially have the same signatures (minus the next()), so it might be more efficient to build the stack when it parses the request, and then just iterate through all the middlewares.

Need to think about maintaining processing order though, especially for wildcard and other global routes.

[jeremydaly]

• Double check route matching based on method. Should be returning a "Method not allowed" if the route exists.

[mjdickinson]

Thanks!