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

Improve Middleware documentation and compile time static assertions #864

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Guiorgy
Copy link

@Guiorgy Guiorgy commented Aug 6, 2024

Closes #861

Currently there is some ambiguity in the documentation surrounding middlewares:

  • CROW_MIDDLEWARES: In the reference docs the example uses crow::SimpleApp, yet the example in Crow/examples states // ALL middleware (including per handler) is listed and the guide doesn't mention this at all:

    auto app = [crow::SimpleApp](https://crowcpp.org/master/reference/namespacecrow.html#a3603179c9794548cac2c9990685178b4)(); // or crow::App()
    [CROW_ROUTE](https://crowcpp.org/master/reference/app_8h.html#a6aa7ec3a2f3ee3f17d5f9acd879e7381)(app, "/with_middleware")
    .CROW_MIDDLEWARES(app, LocalMiddleware) // Can be used more than one
    ([]() {                                 // middleware.
        return "Hello world!";
    });
    // ALL middleware (including per handler) is listed
    crow::App<RequestLogger, SecretContentGuard, RequestAppend> app;
    ...
    CROW_ROUTE(app, "/secret")
        // Enable SecretContentGuard for this handler
        .CROW_MIDDLEWARES(app, SecretContentGuard)([]() {
            return "";
        });

    It needs to be clarified, does CROW_MIDDLEWARES require the specified middleware be also specified in the referenced app template arguments?

  • The reference docs mention the Crow constructor Construct Crow with a subset of middleware., yet this is not explained in the Guide, and there's no example of the usage in the examples.

    The guide should include a section on configuring middlewares, which should include both with the constructor and with app.get_middleware<>.

    The examples should include an example on using the app constructor.

Also, we can reduce the confusion by implementing compile time static assertions:

  • When passing a middleware to the app constructor, it should be validated that the type of that middleware is within the middlewares defined in the app class template types.
  • When passing a middleware to CROW_MIDDLEWARES, it should be validated that the type of that middleware is within the middlewares defined in the app class template types.

TODO:

  • Guide: Local middleware need to be defined in the app middlewares.
  • Reference docs: CROW_MIDDLEWARES comment and example.
  • Guide: Middlewares configuration using app constructor and using app.get_middleware<>.
  • Ecamples: Middleware configuration using app constructor.
  • Validate middlewares passed to the app constructor.
  • Validate middlewares passed to CROW_MIDDLEWARES.

As of writing, the PR includes an implementation that validates middlewares passed to the app constructor, however, the current implementation uses std::decay when comparing types. This needs to be discussed. It also needs more testing for edge cases IMO. Also, I'd like to do the same for CROW_MIDDLEWARES but I don't know where to do so, a suggestion would be appreciated.

a variant of contains that compares types using std::decay
it checks if a type pack is a subset of another type pack using contains_decayed
…the types of the arguments passed to the constructor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Middleware doesn't work when added through the constructor
1 participant