CrowCpp / Crow

A Fast and Easy to use microframework for the web.
https://crowcpp.org
Other
3.24k stars 356 forks source link

Middleware doesn't work when added through the constructor #861

Open Guiorgy opened 2 months ago

Guiorgy commented 2 months ago

I noticed a strange behaviour when playing around with middlewares, when I add my middleware to the app as a template argument it works as expected, however, when I add the same middleware as a constructor argument, neither before_handle and after_handle are executed.

I added a log to the after_handle function:

void after_handle(crow::request& req, crow::response& res, context& ctx) {
    CROW_LOG_ERROR << "wtf?!";
    // ...
}

Compiling and running the following:

crow::App<MyMiddleware> app;

MyMiddleware& middleware = app.get_middleware<MyMiddleware>();
// configure middleware

// define routes

app.bindaddr("127.0.0.1").port(8080).run();

Sending a single request prints "wtf?!" as expected:

template middleware

However, if I compile and run the following:

MyMiddleware middleware;
// configure middleware

crow::App<> app(middleware);

// define routes

app.bindaddr("127.0.0.1").port(8080).run();

And send a single request, I don't get the "wtf?!" message:

constructor middleware

The reference docs Say about the constructor "Construct Crow with a subset of middleware.", what does that mean exactly? Can this be mentioned in the Middleware guide?

Guiorgy commented 2 months ago

Also an FYI, in middleware.h, in the is_after_handle_arity_3_impl struct value is defined as a constexpr, while in is_before_handle_arity_3_impl as a const.

Guiorgy commented 2 months ago

From looking at the source, I think the middlewares passed to the constructor replace those defined in the template arguments as an alternative initialization to the app.get_middleware() approach:

MyMiddleware middleware;
// configure middleware

crow::App<MyMiddleware> app(middleware);

// define routes

app.bindaddr("127.0.0.1").port(8080).run();

If this is correct, it would sure be useful to have a static_assert that prevents passing middleware types missing in the App template arguments.

I'll try to submit a PR when I get time.