Local middleware

[dranikpg]

Hi! I've restructured middleware invocation and added per-handler middleware as discussed in the issue.

What it looks like

It's fully backward compatible and all (including local) middleware as previously has to be listed in the App<>.
Local middleware extends crow::ILocalMiddleware and is not called by default. It has to be enabled with a middleware() call on the rule.

// extend ILocalMiddleware
struct MyLocalMiddleware : crow::ILocalMiddleware
{ ... }

// Now only MyGlobalMiddleware is called for every request
crow::App<MyGlobalMiddleware, MyLocalMiddleware> app;

CROW_ROUTE(app, "/")
// Enable MyLocalMiddleware for this handler
.middleware<decltype(app), MyLocalMiddleware>()
([]() { ... });

What I've changed

I'll try to briefly explain my changes and decisions here, but I'll also leave some PR comments.

This is the current call chain for all requests:

* request *
  | 
http_connection.handle()
  |
  | - middleware is called here
  |
app.handle()
  |
--- middleware type information is preserved until here ---
  | 
router.handle()
  |
rule.handle()
  | 
  |  - this seems to be the best place to squeeze in local middleware invocation
  |
* handler *

Getting type info up the chain

To allow wrapping the handler directly with middleware we'd have to somehow get the type information to it. This could be done in two ways:

• templatize the router/rules to wire it all the way through the call chain
• inject it during handler registration

The first approach is somewhat more complex. It'd would seem more reasonable to move global middleware invocation from something as low-level as an http connection further up the chain to the router and restructure it.

The second one is straightforward, but has some tradeoffs:

• middleware<decltype(app), ...> might look somewhat strange
• middleware<> always has to be the last one in the builder (this can be fixed but requires some refactoring)

CROWD_ROUTE(app, "/")
.middleware<>()  // doesn't work this way
.methods(...)

• the structure as a whole remains somewhat messy

I've chosen the second approach as it seems more likely to be fully implemented 😁

Filtering middleware at compile time

http_connection was responsible for handling all middleware, including template magic. I've moved most of it to middleware.h, some parts to utility.h.

To reuse the same code for calling global and local middleware, I've generalized the middleware_call_helper to include a CallCriteria, which can be used to determine whether a specific middleware should be called.

template<template<typename QueryMW> class CallCriteria, ...>
bool middleware_call_helper(...) 
{
    using CurrentMW = ...
    if (!CallCriteria<CurrentMW>::value) 
    {
        return;
    }
}

Now a handlers criteria simply checks whether the queried middleware is present in its enabled middleware.

template<typename F, 
  typename App,              // App::mw_container_t holds all middleware
  typename... Middlewares    // enabled middleware
>
struct handler_middleware_wrapper
{
    template<typename MW>
    struct middleware_call_criteria
    {
        static constexpr bool value = black_magic::has_type<MW, std::tuple<Middlewares...>>::value;
    };

    void operator() 
    {
        // call only enabled middleware
        middleware_call_helper<middleware_call_criteria, typename App::mw_container_t, ...>(...)
    }

The http_connection still handles global middleware and calls all middleware with middleware_call_criteria_only_global.

I've also refactored middleware_call_helper to use tuple indexing (like after_handlers_call_helper does) instead of recursively unpacking the parameter pack, as it is not available to the handler in its raw form.

Transparently wrapping handlers

Router rules already do it and hide all handlers behind a std::function<void(const request, response, args...)>.

Handlers with middleware are wrapped in handler_middleware_wrapper, which extracts the middleware context from the request, and calls all the local middlewares, which require both a mutable request and response. This means that we have to get a mutable request all the way through the chain, so I've removed the interfering const qualifiers.

I've moved the handler wrapping logic from TaggedRule::operator(Func&&) to wrapped_handler_call in middleware.h. I've also refactored it to use a more elegant is_callable sfinae check and allow mutable requests as parameters.

To sum up

The call chain now looks the following:

* request *
  | 
http_connection.handle()
  |
  | - calls only global middleware
  | - stores middlewares in request
  |
app.handle()
  |
--- middleware type information is lost here ---
  | 
router.handle()
  |
rule.handle()
  | 
  |                  | --- middleware type information has been injected during handler wrapping ---
  |                  |
handler_middleware_wrapper ()
  |
  | - retrieves middlewares from request and cast to correct type
  | - calls only enabled middleware
  |
* handler *

This is how a handler is wrapped:

* Rule as RuleParameterTraits *
  |
middlewares<decltype(app), Mw1, Mw2, ...>()
  |
detail::handler_call_bridge
  |
  | - overloads operator() to and registers itself 
  |    to act transparent and remove redundant nesting/parentheses
  | - static_asserts that middleware is local and present in app
  |
detail::handler_middleware_wrapper
  |
  | - declares its middleware_call_criteria
  | - transparently wraps a handler
  |
* handler *

Issues

Global middleware is called before local, there is no way to intertwine them. To support this all middleware invocation has to be moved directly to the handler, oh, and all handlers had to be wrapped. I can't guess whether this is a big tradeoff, because the solution to this is quite complex.

The local invocation order is defined not in the middleware() call, but in the App params. This can be fixed, but requires restructuring all middleware call helpers.

At the time of writing I haven't touched dynamic rules. I've never used them and it seems like they are only part of the CROW_MSVC_WORKAROUND.
I also don't know whether CatchAll routes should support middleware, so I've restricted it to only TaggedRules.

Testing

I've included a small example and unit test. I've tested it with both gcc/clang on my small crow projects.
Would be great if you'd test it out.

The changes are somewhat verbose in some parts, but therefore c++11 compliant.

[dranikpg]

Remove const qualifiers up to rule->handle() to allow passing mutable requests into local middleware

[dranikpg]

move to middleware.h

[dranikpg]

store middleware in request

[dranikpg]

from http_connection

[dranikpg]

check if middleware MW should be not called globally

[dranikpg]

makes registering middleware less verbose

[dranikpg]

const issue

[dranikpg]

all this now uses detail::wrapped_handler_call

[dranikpg]

some things for black magic I didn't find

[The-EDev]

I've noticed a couple things that I can't place on lines of code:

The middleware example isn't added to examples/CMakeLists.txt, I did it locally as such:

add_executable(example_middleware example_middleware.cpp)
add_warnings_optimizations(example_middleware)
target_link_libraries(example_middleware PUBLIC Crow::Crow)

---

The after_handle does not affect the response (which it should), this only happens with local middleware. (The handle is called, but the response doesn't change)

---

At the time of writing I haven't touched dynamic rules. I've never used them and it seems like they are only part of the CROW_MSVC_WORKAROUND.

I've seen multiple places where dynamic rules are used beyond old MSVC support (see #164 and #308)

---

I also don't know whether CatchAll routes should support middleware, so I've restricted it to only TaggedRules.

I would think it's a good idea if they did, although it's not crucial, we can probably create an issue for it to address later.

---

As you may have noticed, I didn't go into Most of the code relating to templates, this is because While I did understand what's happening in a general sense (Thanks to the very helpful descriptions you wrote). I have no clue how it's actually working, it might be best if someone with a better understanding took a look at them. Besides that I'll still try to understand what's going on to properly review the template code. Any resources, material, or descriptions that are helpful would be greatly appreciated!

Thank you again for the work you've done and I'm sorry for taking so long to get this review out!

[crow-clang-format]

--- include/crow/http_response.h	(before formatting)
+++ include/crow/http_response.h	(after formatting)
@@ -19,7 +19,8 @@
     template<typename Adaptor, typename Handler, typename... Middlewares>
     class Connection;
 
-    namespace detail {
+    namespace detail
+    {
         template<typename F, typename App, typename... Middlewares>
         struct handler_middleware_wrapper;
     } // namespace detail

[The-EDev]

One thing I noticed with the latest commit is that if a before_handle ends a response, no other before_handle or after_handle runs. It might be worth adding that in the documentation (you don't need to, I can deal with that).

Edit: This is interesting, if a middleware registered first in the app, it's after_handle seems to run (Even though the before_handle of a middleware registered later ends the response) but not modify the response... This might be a problem in case the after_handle does anything not related to the response. @dranikpg could you please look into this?

[dranikpg]

• Oh, I didn't push to lastest before marking it ready for review. After handlers should be able to change the request now
• Yes, an ending response in before_handle cancels the chain. This behaviour is from http connection
• I've added example_middleware to cmake
• Yes, I think It'd be easier to cover other route types later
• Hard to recomment something specific on templates. The CallCriteria is something akin to template based policies. The other changes are very close to present crow code. I'll maybe ask someone I know to take at look at it

[dranikpg]

I reproduced your issue and it indeed looks like a bug. This is because the complete_request_handler_ has to be empty before calling any middleware so that such cases won't arise. It should now be identical to http_connections logic.

[The-EDev]

The local_middleware test is failing, the check at line 1425 is getting a 500 instead of the expected 403 (usually Crow returns a 500 in case an exception is thrown during execution)

[dranikpg]

This is because it depends now on the full request chain. So the local middleware test has to use real requests like all the other middleware tests do. Sorry for pushing separately 😓

[The-EDev]

No worries, as long as the formatting bot doesn't write a 2000 page report it's all fine 😆

[dasfex]

From С++ point of view (and my) looks nice.

[The-EDev]

I'm sorry it took me this long to look at the latest changes, from what I can see this is the new logic:

middlewares execute their before and after handles based on the order of registration in the app, if a before_handle of a middleware ends the response, any after_handles of middlewares registered before it will still run. This is consistent with the way global middlwares work.

This is more about the design of the framework rather than the actual code, but I do think this is a problem that needs to be addressed.
Because currently, the only factor in whether or not a middleware cares that a response has ended outside the handler is it's registration position.
I think it would be best change middlewares such that running the before_handle and after_handle after a response was ended in a before_handle is a parameter for the middleware, with later before_handles preventing earlier after_handles of middlewares that don't set this parameter from running.

But that's way out of the scope of this PR, I'll open a separate issue to discuss it.

[dranikpg]

The current crow middleware logic seems quite reasonable to me. Middleware that hasn't been called generally has no cleanup to perform. For basic response rewriting it should be registered first.
This is kind of the same behaviour as it is in popular frameworks like Laravel with recursive middleware handlers.

if I recall correctly, we decided to cover the remaining route types in another PR. Are there any issues left to be addressed?

[The-EDev]

Sorry for the late response. There shouldn't be any issues, I'll just do one last review (and hopefully understand the template logic) and merge the PR

[The-EDev]

Looks all good, though there's some stuff to clarify in the docs (primarily the load order, lack of support for dynamic requests, and what happens when a request is cancelled). I'll handle those in a later PR.

I do expect some bugs to creep up after this is merged (as they always do). And while I understand almost all of the code, I would be very thankful if you stuck around in case there was something that myself or the other developers couldn't fix.

Thanks again for the work you've done, and thanks for bearing with the review process.

[dranikpg]

No problem, I'll stick around for support and further enhancements 😄