lasselukkari / aWOT

Arduino web server library.
MIT License
283 stars 41 forks source link

Run middleware only on specific routes without using `res.end()`? #152

Closed leoshusar closed 3 days ago

leoshusar commented 6 days ago

Hi! Consider this made up example:

void setupWebApp() {
  webApp->use(&requestStartMiddleware);

  webApp->get("/ping", &pingHandler);
  webApp->get("/gpio/inputs", &getInputStatesHandler);

  webApp->use(&safetyMiddleware);
  webApp->post("/gpio/outputs/:pin", &setOutputStateHandler);

  webApp->use(&requestEndMiddleware);
}

My goal is to not have to duplicate anything, like I don't want to do the safety check in all relevant handlers nor I don't want to do path checking in the safetyMiddleware.

So I tried using two routers:

void setupWebApp() {
  safeRouter->get("/ping", &pingHandler);
  safeRouter->get("/gpio/inputs", &getInputStatesHandler);
  webApp->use(&safeRouter);

  unsafeRouter->use(&safetyMiddleware);
  unsafeRouter->post("/gpio/outputs/:pin", &setOutputStateHandler);
  webApp->use(&unsafeRouter);
}

but I quickly realized if I don't use separate path for routers, they are executed both, so after my pingHandler the safetyMiddleware is still being executed.

Is there any nice way to achieve this? Thanks!

lasselukkari commented 5 days ago

I'm not sure did I understand you correctly but would it work for you if you call res.end() in the pinHandler and in the safetyMiddleware if the check fails and mount the requestEndMiddleware with Application::finally instead of use? The finally is called always even if the res.end() has been called. You can't write anything to client if the response has been ended, but maybe that is not a problem in your use case.

leoshusar commented 5 days ago

Yeah, finally would work here. Thanks, I only checked the middleware docs page and didn't look thoroughly through the method list.

Although I'm still curious if there is a way to not need to call res.end() in all handlers and still skip the safetyMiddleware? For example here:

void getInputStateHandler(Request &req, Response &res) {
  char pinName[8];
  req.route("pin", pinName, sizeof(pinName));

  const auto pin = gpio::inputs::getPin(pinName);

  if (pin == gpio::inputs::Pin::unknown) {
    res.sendStatus(HttpStatus_NotFound);
    return;
  }

  const auto state = gpio::inputs::getState(pin);
  const auto json = com::json::serialize(state);

  if (!json) {
    res.sendStatus(HttpStatus_InternalServerError);
    return;
  }

  res.print(json.get());
}

void safetyMiddleware(Request &req, Response &res) {
  if (safety::isSafetyLockActive()) {
    res.status(HttpStatus_Conflict);
    res.print("Software lock active");

    res.end();
  }
}

I have multiple conditions from where I'm returning, so I need to remember to call res.end() everytime. That's also why I tried those two routers, I was expecting it to only use the one router with my requested path (later I also found this express.js issue with the same thing I ran into).

Did I miss anything else? Should I just create my own functions like printAndEnd() or something? I have no problem with that, I was just curious if I'm not missing anything.

Do you think the router isolation would be a good feature request? Or at least a route level middleware? Something like:

webApp->post("/gpio/outputs/:pin", &safetyMiddleware, ..., &setOutputStateHandler);

(not sure if this exact API would be possible in C++)

lasselukkari commented 4 days ago

My reasoning is pretty much the same as in the express issue you posted. There is a difference how the middleware chain is executed compared to express. In express you need to call the next() function in order to move to the next middleware. In this library the next middleware is called by default unless you call res.end.

There are basically two options how the router.use middleware can work and there would be pros on cons in each. The implementation is a lot simpler in this option that is now being used.

One solution to pass a state between the middleware is to use the RequestContext, You could in theory set some flags there and then make decisions based on those in other middleware, but I don't know will this be any less complicated.

This library works on all Arduino compatible boards starting from Uno. This creates some limitations on what can be done because the code needs to be compatible with ancient versions of C++ and there is only 2kb on RAM available in total and in practise about half of that when the networking libraries are included.

leoshusar commented 3 days ago

I see, I was actually thinking about how I could (ab)use the context, but in the end I didn't like that either. Then I remembered Go's defer statement, which lets you execute something at the end of the scope, and I found I can achieve something similar in C++.

I borrowed the implementation from here and now I start all my request handlers like this:

void getInputStateHandler(Request &req, Response &res) {
  util::defer([&res]() { res.end(); });

  // ...
}

and it calls the res.end(); everytime when I return from the handler. I think it could be written even cleaner, but for now this will be good enough.

Anyway thanks for your explanation. And thanks for the great library, this is the best one I found with higher level API and compatible with Ethernet.h (STM32 + Wiznet board) :)

lasselukkari commented 2 days ago

Nice simple and clean solution.

Thanks for the feedback!