ipkn / crow

Crow is very fast and easy to use C++ micro web framework (inspired by Python Flask)
BSD 3-Clause "New" or "Revised" License
7.47k stars 888 forks source link

Remove CROW_ROUTE macro. #13

Open acron0 opened 10 years ago

acron0 commented 10 years ago

The desire is to move back to a simple method call that looks like

app.route("/hello/<int>", [](int count){ ... });

But to maintain compile-time checking of url parameters and argument lists.

ipkn commented 10 years ago

Any implementation idea?

alexkarpenko commented 10 years ago

If we changed the route function signature to:

template <typename U>
void route(U route, U::callbackType callback);

Then wouldn't the following work?

app.route("/hello/<int>"_route, [](int count){ ... });

Where _route is a custom string literal that returns an object of similar type to

crow::black_magic::get_parameter_tag(url)

Alternatively we could also do something like:

app.route(crow::r("/hello/<int>"), [](int count){ ... });
acron0 commented 10 years ago
app.route(crow::r("/hello/<int>"), [](int count){ ... });

I like this one. No oddness to the syntax, clear and explicit.

ipkn commented 10 years ago

The problem was that return type from get_parameter_tag or crow::r should be always same type because the argument type is the same. C++ don't support function overloading of different return types.

I bypassed this issue by returning int from get_parameter_tag and reconstruct template type list from the integer. The integer should be computed at compile time (because it is used as template argument), so it cannot be given as a function argument (constexpr still have many restrictions).

But I did some researches and I think I found some clues for removing macros by using the type erasure technique. I will show you soon if I succeeded.

alexkarpenko commented 10 years ago

Ah gotcha. I'm not a C++ meta-programming expert :)

tnicolson commented 10 years ago

Perhaps rather than writing the type in the url, you could just define a placeholder and take the argument types from the function, e.g.

app.route("/add/__/__", [](int a, int b) { ... });

Flask requires typenames in the url because python's function aren't strongly typed. If the type is specified once, you don't have to check for consistency at compile time. Ideally, you'd still be able to check the number of arguments matched at compile time, but given that you can't eliminate all runtime checking anyway (type conversion for example), perhaps you could just throw a runtime error from app.route during startup. Mismatches aren't going to get far.

This has a few advantages:

Perhaps you could allow the user to define the placeholder (everyone has different preferences):

"/add/<_>/<_>"
"/add/{}/{}"
"/add/_1/_2"  // c.f. std::placeholders to allow reordering... ;-) 
acron0 commented 10 years ago

Some very interesting ideas here. However, this would depart from one of the core intentions of the current implementation, which is to compile-time validate that your handler function and URL parameters match types.

How would I differentiate...

app.route("/add/__/__", [](int a, int b) { ... });
app.route("/add/__/__", [](const char* a, int b) { ... });
app.route("/add/__/__", [](const char* a, const char* b) { ... });

(Not that this is necessarily good design, but at least the current system is prescriptive enough to cope with it.)

tnicolson commented 10 years ago

You haven't lost anything, these routes could be handled in just the same way that crow currently handles them (i.e. just pass the argument list from the handler through to the Router::new_rule_tagged method, instead of looking up the same argument list using the black magic tag value).

The number of placeholders could easily be validated at compile-time. What type-safety do you gain by asking the user to specify the same parameter list twice?

alexkarpenko commented 10 years ago

I really like this proposal. Also follows the DRY principle.

ipkn commented 10 years ago

I agree on the proposal. My purpose of describing the type in the url was make route look like Flask, and it is much better if we can remove macros. Also I want to find a way to add an argument with user defined type in handle function.

Crow is already checking types at the run time. It gives 400 Bad Request to the user if non-number is given to url argument(ex, request /add/abc/def to /add/<int>/<int>).

For user defined type conversion, I think it is good to add a template type like std::hash for it. One possible name is crow::to_argument, and if the argument type is int, crow::to_argument<int>(std::string) is called. Any user want to add a type conversion for a CustomType can specialize crow::to_argument<CustomType>::operator().

My preferred placeholder string is "%".

acron0 commented 10 years ago

My preferred placeholder would involve a numeral: %1%, {2}, <3>, _4

Like most mature string formatters, it seems reasonable to allow users to specify a route that doesn't necessarily specify arguments left-to-right.

ipkn commented 9 years ago

I added route_dynamic and can be used as follows:

    app.route_dynamic("/set_int/<int>")
    ([&](int y){
        x = y;
        return "";
    });

It checks handler type at run-time and throws exception if there's a mismatch.

acron0 commented 9 years ago

Awesome, nice one.

For reference, added in https://github.com/ipkn/crow/commit/5d8d52763989f8839644d64791ea0a47f2b28799

ipkn commented 9 years ago

CROW_ROUTE is still kept; Compile-time checking is better to use I think.

tghosgor commented 9 years ago

Isn't compile time checking a bit pointless here? There is the DRY principle issue and the browser user is free to put any input to the address bar, so we still need to check the handler arguments against the input at run-time and it should be enough to do it once instead of both compile-time and run-time.

acron0 commented 9 years ago

The point of compile-time checking is to help protect developers from themselves, not against users necessarily. Developers choose C++ because of compile-time safety (amongst other things) and so the point of this feature was to extend this safety to routes. I believe ipkn will admit that it was a bit of a personal goal, to leverage the new C++ features in order to achieve this however. If we get this 'for free' then I can't see the harm in it.

jeaye commented 9 years ago

Isn't compile time checking a bit pointless here?

Absolutely not. A huge draw to Crow is its compile-time checking. Following up on the previous suggestions, why not keep the route as a template parameter? Without the macro, we can have something along the lines of:

app.add<"/hello/<int>"_route>([](int count){ ... });

// or, without the UDL

app.add<crow::route("/hello/<int>")>([](int count){ ... });

The integer black magic for rebuilding the types is working well; we should keep it and just remove the icky macro.

ipkn commented 9 years ago

The main reason that I keep both CROW_ROUTE and route_dynamic is that I can't find a way to implement the route function as @jeaye suggested.

If the route is only given to template part, the returned object of app.route or app.add can only see the parameter_tag (the integer value) and don't know the route itself. That's why I added url parameter and pass it to new_rule_tagged.

It seems that the only workaround is converting the string literal into variadic template of chars. But it needs much longer compile time, so I want to avoid it.

Please let me know if anyone can find a better way to do this.

jeaye commented 9 years ago

On Thu, Apr 16, 2015 at 04:06:56AM -0700, ipkn wrote:

It seems that the only workaround is converting the string literal into variadic template of chars. But it needs much longer compile time, so I want to avoid it.

This is a trade off I'd absolutely be willing to make, given that it's C++. Not everyone agrees though, no doubt.