Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Route specific middleware #45

Closed
The-EDev opened this issue Nov 9, 2020 · 9 comments · Fixed by #327
Closed

Route specific middleware #45

The-EDev opened this issue Nov 9, 2020 · 9 comments · Fixed by #327
Labels
feature Code based project improvement help wanted Contributions are requested

Comments

@The-EDev
Copy link
Member

The-EDev commented Nov 9, 2020

This is me daydreaming but it would be nice if middlewares could be defined per route as well as per server. Those defined per server will run on every request as usual, but the ones defined per route will run only on that request (I'm mainly talking about before_handle and after_handle)

@The-EDev The-EDev added the feature Code based project improvement label Nov 9, 2020
@The-EDev The-EDev changed the title Request specific middleware Route specific middleware Nov 9, 2020
@The-EDev
Copy link
Member Author

The-EDev commented Nov 10, 2020

ok some news, middlewares are defined with the App, and are then tied to the connection itself through do_accept() (http_server.h - 188), the defined middlewares then get their helper called in the handle() function (http_connection.h - 318) and complete_request() (http_connection.h - 348), it could be possible to check if a middleware is defined for a route or even switch them to be per route, but that's one big task.

@The-EDev The-EDev added the help wanted Contributions are requested label Jul 2, 2021
@The-EDev
Copy link
Member Author

The-EDev commented Aug 22, 2021

Alright, so using the current middleware configuration with routes is next to impossible. But there might be some merit to a simpler idea: Have a pre-handler and a post-handler for each route. construct your middleware, and then assign its pre and post handler's to the route's own, then call them inside the router. This solution does have the problem of being a bit more messy and not letting you use get() for context, but unfortunately I'm not experienced enough to tackle anything related to the black_magic namespace..

@dranikpg
Copy link
Member

It should really be no that complex. Yes, black_magic instead of type_traits is a bit annoying :(

A handler is simply called in routing.h:

[f](const request&, response& res, Args... args) {
    res = response(f(args...));
    res.end();
});

Which means that we can wrap its functor with accoriding middleware:

template<typename T, typename... MDS>
auto middlewares(T f, MDS... mds) {
    return [f = std::move(f), mds = std::make_tuple(std::forward<MDS>(mds)...)] 
            (const crow::request& req) -> decltype(f()) {
        std::apply([&req](auto ...x){(x.before_handle(req), ...);}, mds);
        auto res = f();
        std::apply([&req](auto ...x){(x.after_handle(req), ...);}, mds);
        return res;
    };
}

makes the following work

CROW_ROUTE(app, "/") (middlewares(([](){
        return std::string("Hello world");
    }), RequestMiddlware{}));

Now we just have to change handler.h a bit to make the response available.

The last part are contexts. Those should shared accross different handlers. Crow has to store them somehow.

What solution comes to mind? Let's list them in the Crow<> varargs anyway, but add an optional (by inheriting some LocalMiddleware):

using call_type = std::false_type // or true_type for global;

which will tell Crow to skip them for global requests. We'll fish them out for handlers, again, using type magic.

You seem to be pretty active in other issues. What do you think of it? I'll give it a try in the nearest future.
What do you think of dropping C++11 support and going 14-17? It would help with a lot of issues in the future.

@dranikpg
Copy link
Member

So I've looked at the code more in detail.
We'd basically have to apply the <Middlewares...> pack to the Router and to all (Tagged)Rules.
Those Rules create their own handlers by hiding them behind a std::function. The best place to add middleware invocation is there because all type information is preserved up to this point. We don't even need a wrapper - we can just extend the operator()(Func&& f) to accept a middleware pack.

All the SFINAE stuff could be moved to a helper function. Besides that, it currently doesn't allow to accept both a request and a response which could be fixed.

The codebase will become more complex, but we'll get route based middleware without runtime overhead 😃

@The-EDev
Copy link
Member Author

@dranikpg Thanks for taking interest in this issue.

Admittedly a lot of the stuff you talked about went over my head, it like knowing a hammer exists but having no idea how to swing it. I'll still do my best to help.


What do you think of dropping C++11 support and going 14-17? It would help with a lot of issues in the future.

As helpful as this would be, I want to stay away from dropping compatibility until either other libraries (like sqlpp17) are ready, or statistics show almost nobody still using C++11. It's also worth mentioning that Crow still compiles for and uses some features of C++14 and C++17 if they are available. But only when a C++11 alternative is available.


The codebase will become more complex

As long as the user's code remains similar in terms of complexity, I don't think this would be that big of a deal.


I'd like to thank you once more for the help you're providing, and apologize for my lack of experience. I'm eager to see your work in action.

@dranikpg
Copy link
Member

dranikpg commented Feb 3, 2022

@The-EDev Hi! I've filed a PR for this issue, would you mind checking it out 😅 I haven't tested it with MSCV as for now

@The-EDev
Copy link
Member Author

The-EDev commented Feb 3, 2022

Hello @dranikpg, I've already taken a look at your PR briefly and it seems to be great in terms of code quality, concepts, execution, and even documentation. Thank you very much for the incredible work you've done!!

Unfortunately it'll take me some time to look at and test it in depth. Between an issue that took me 3 weeks of research and testing to fix, which burned me out more than I'd like to admit, and personal matters that are critical. I'm not able to work on Crow beyond leaving comments and organizing issues/PRs.

One thing I'd like to note (for the future) is that you can have multiple comments in 1 GitHub review, you don't need a new review for every comment :).

Thank you very much for your work once more, and how thoroughly you explained it, I promise to look at and merge it ASAP.

@dranikpg
Copy link
Member

dranikpg commented Feb 4, 2022

Oh, I just flagged it to be ready for review and wanted to let you know 😓 By no means do I want you to hurry - I really appreciate you keeping the project alive. I've only noticed by the end that I've used the wrong button - I recollected it into a single review - sorry for all the emails 😬

@The-EDev
Copy link
Member Author

The-EDev commented Feb 4, 2022

no worries 😁

@The-EDev The-EDev linked a pull request Feb 9, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Code based project improvement help wanted Contributions are requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants