Stiffstream / restinio

Cross-platform, efficient, customizable, and robust asynchronous HTTP(S)/WebSocket server C++ library with the right balance between performance and ease of use
Other
1.15k stars 93 forks source link

How best to return a 405 response from express router #82

Closed prince-chrismc closed 4 years ago

prince-chrismc commented 4 years ago

I am looking for suggestion, to create example for how someone can return 405 on endpoint.

For instance for any given route...

auto router = std::make_unique<router_t>();
router->http_post("/author/:author", [](auto req, auto param) {
    if(req->header().method() != http_method_post()) {
        return req->create_response(status_method_not_allowed())
            .connection_close().done();
    }
    // Happily process the request
}

What I would like to avoid is creating routes for every single method for each path and handling all the "bad methods" that I need to block.

prince-chrismc commented 4 years ago

Leaving my brain storming.

By extending the route matcher...

class method_equals_match_t // inspired by std_regex_engine_t
{
    //! Wrapper function for matching logic invokation.
    static auto try_match( http_method_id_t method_expected,
            http_method_id_t method_obtained )
      {
            return method_expected == method_obtained;
      }
};

template < typename Regex_Engine = std_regex_engine_t,
    typename Route_Engine = method_equals_match_t >
class route_matcher_t{
   // ...
        inline bool
        operator () (
            const http_request_header_t & h,
            target_path_holder_t & target_path,
            route_params_t & parameters ) const
        {
            //return m_method == h.method() 
                        return Route_Engine::try_match(m_method, h.method())
                                && match_route( target_path, parameters );
        }
};

I can then come along with...

class method_any_match_t 
{
    //! Wrapper function for matching logic invokation.
    static auto try_match( http_method_id_t method_expected,
            http_method_id_t method_obtained )
      {
            return true; // quick and easy
      }
};

Then the example above would not need to change to obtain the desired effect. This is however a bit awkward from the API since the routes are still all based on the method.

eao197 commented 4 years ago

I don't get the problem. And this code:

router->http_post("/author/:author", [](auto req, auto param) {
    if(req->header().method() != http_method_post()) {

has no sense because if a handler is set by http_post method then it can't be called for any other method except the POST.

Why not just use non_matched_request_handler?

prince-chrismc commented 4 years ago

The spec I need to implement has two clauses

  1. All endpoints MUST return a 405 when a incorrect method (not defined for use) is made
  2. Any request which does not match an endpoint MUST return a 404 response

Currently what I have is my none match handler returning a 404. Since there are 100+ routes with a large variety of methods I have no easy way to know that route "matched" the target_path but not the method.

You are correct my example is very weird... I tried to point that out in my closing statement. If I was able to do something like...

router->http_any("/author/:author", [](auto req, auto param) {
    if(req->header().method() != http_method_post())

Then I would get my desired effect.

eao197 commented 4 years ago

Ok, thanks. Now I see the problem.

It seems there should be something like:

auto method_disabled = [](const auto & req, const auto &) {
   return req->create_response(status_method_not_allowed())
            .connection_close().done();
};

router->http_get(first_path, normal_handler_1, restinio::other_methods(method_disabled));
router->http_post(second_path, normal_handler_1, restinio::other_methods(method_disabled));

or

auto method_disabled = ...;

router->add_handler(
   restinio::exact_match(restino::http_method_get()),
   first_path, normal_handler_1);
router->add_handler(
   restinio::if_not(restinio::http_method_get()),
   first_path, method_disabled);

router->add_handler(
   restinio::exact_match(restino::http_method_post()),
   second_path, normal_handler_2);
router->add_handler(
   restinio::if_not(restinio::http_method_post()),
   second_path, method_disabled);

router->add_handler(
   restinio::exact_match(restino::http_method_post(), restinio::http_method_put()),
   third_path, normal_handler_3);
router->add_handler(
   restinio::if_not(restinio::http_method_post(), restinio::http_method_put()),
   third_path, method_disabled);

I need to think which approach is better...

eao197 commented 4 years ago

It seems that this approach:

router->http_get(first_path, normal_handler_1, restinio::other_methods(method_disabled));

won't work because it makes impossible to handle different requests for the same path:

router->http_get("/author/:author", get_handler);
router->http_post("/author/:author", update_handler);
router->http_delete("/author/:author", delete_handler);

So the only way I see for now is something like that:

router->http_get("/author/:author", get_handler);
router->http_post("/author/:author", update_handler);
router->http_delete("/author/:author", delete_handler);
router->add_handler(
   restinio::if_not(restinio::http_method_get(), restinio::http_method_post(), restinio::http_method_delete()),
  "/author/:author", http_405_handler);
prince-chrismc commented 4 years ago

I must agree making the add_handler() more expressive is a better approach then expanding the router.

router->http_get(first_path, normal_handler_1, restinio::other_methods(method_disabled));

won't work because it makes impossible to handle different requests for the same path:

I would second that, it's a useful feature to strip complexity.

prince-chrismc commented 4 years ago

What would be the return type of restinio::if_not()?

eao197 commented 4 years ago

What would be the return type of restinio::if_not()?

I don't know yet. It seems that the addition of restinio::if_not() will require a redesign of express-router implementation. I plan to start this work after this weekend and it can take several days.

If you need a quick solution then you can think about this variant:

You can patch restinio::router::impl::route_matcher_t this way:

inline bool
operator () (
    const http_request_header_t & h,
    target_path_holder_t & target_path,
    route_params_t & parameters ) const
    {
        return (m_method == h.method() || m_method == http_method_unknown())
            && match_route( target_path, parameters );
    }

If will allow you to do calls like:

router->add_handler(restinio::http_method_unknown(), path, handler);

and your handler will be called for any request. So you can do method dispatching inside your handler:

router->add_handler(restinio::http_method_unknown(), "/author/:author",
   [](const auto & req, auto & params) {
      if(restinio::http_method_get() == req->header().method()) {
         ... // Handling of GET.
      }
      else if(restinio::http_method_post() == req->header().method()) {
         ... // Handling of POST.
      }
      else if(restinio::http_method_delete() == req->header().method()) {
         ... // Handling of DELETE.
      }
      else return req->create_response(status_method_not_allowed())
            .connection_close().done();
   });
prince-chrismc commented 4 years ago

Thank you very much for looking into this.

I wouldn't mind helping, but you know best what needs to be done.

For now I just loop over all the method, if it's not a support method, I add a 405 handler.

Meets my project requirements.

On Sun., Mar. 15, 2020, 1:48 a.m. eao197, notifications@github.com wrote:

What would be the return type of restinio::if_not()?

I don't know yet. It seems that the addition of restinio::if_not() will require a redesign of express-router implementation. I plan to start this work after this weekend and it can take several days.

If you need a quick solution then you can think about this variant:

You can patch restinio::router::impl::route_matcher_t https://github.com/Stiffstream/restinio/blob/v.0.6.5/dev/restinio/router/express.hpp#L363 this way:

inline booloperator () ( const http_request_header_t & h, target_path_holder_t & target_path, route_params_t & parameters ) const { return (m_method == h.method() || m_method == http_method_unknown()) && match_route( target_path, parameters ); }

If will allow you to do calls like:

router->add_handler(restinio::http_method_unknown(), path, handler);

and your handler will be called for any request. So you can do method dispatching inside your handler:

router->add_handler(restinio::http_method_unknown(), "/author/:author", [](const auto & req, auto & params) { if(restinio::http_method_get() == req->header().method()) { ... // Handling of GET. } else if(restinio::http_method_post() == req->header().method()) { ... // Handling of POST. } else if(restinio::http_method_delete() == req->header().method()) { ... // Handling of DELETE. } else return req->create_response(status_method_not_allowed()) .connection_close().done(); });

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Stiffstream/restinio/issues/82#issuecomment-599171154, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEAWA463UZZZEWS4BGC7KEDRHRTZDANCNFSM4LGYR66A .

eao197 commented 4 years ago

New functionality is working in a development branch. This is a modified express_router example. I hope it'll be a part of v.0.6.6.

prince-chrismc commented 4 years ago

Wow! I will try it out this week =)

prince-chrismc commented 4 years ago

New functionality is working in a development branch. This is a modified express_router example. I hope it'll be a part of v.0.6.6.

I added a comment, minor editorial, but it look spot on. Usage in the example is very expressive.

eao197 commented 4 years ago

The new method matchers are available now as a part of v.0.6.6.