Open hoeken opened 1 month ago
hoeken
commented
1 month ago Having typed this all out, I'm leaning more towards option 3 as an approach and not adding any extra callbacks or definitions. you can either get the response object from the request or by passing the request into the constructor of the derived response object.
hitecSmartHome
commented
1 month ago In express.js a middleware can be added in two ways. One is when added as a new cb param to the endpoint and runs before the main handler cb. This allows the middleware to peek in to the request, extend the response and act accordingly. This allows the middleware to reply to the request early and the main cb might not be called at all. It can check for options or whatever. The second way is to append the middleware globally so it will be called before every request. In my mind, its just another list of handler function pointers in a struct which should be iterated over when a request comes.
hitecSmartHome
commented
1 month ago I would imagine something like this
// Global middleware. Will be called on every request
server.use("*",[](PsychicRequest* req){
// Access request
// Can be reply early if conditions are met
if( req->method() == HTTP_OPTIONS ){
request->addResponseHeader("Access-Control-Allow-Origin", "*");
request->addResponseHeader("Access-Control-Allow-Methods", "GET, POST, PUT, DELETE, OPTIONS");
request->addResponseHeader("Access-Control-Allow-Headers", "Content-Type, Authorization, Accept");
request->addResponseHeader("Access-Control-Max-Age", "86400");
return request->reply(200);
}
// Will call the next middleware if it does not reply
// Should return false?
return false;
});// Global middleware. Will be called on every request which method is matching
server.use("*", HTTP_OPTIONS, [](PsychicRequest* req){
// Access request
request->addResponseHeader("Access-Control-Allow-Origin", "*");
request->addResponseHeader("Access-Control-Allow-Methods", "GET, POST, PUT, DELETE, OPTIONS");
request->addResponseHeader("Access-Control-Allow-Headers", "Content-Type, Authorization, Accept");
request->addResponseHeader("Access-Control-Max-Age", "86400");
return request->reply(200);
// Or return false;
});// Endpoint middleware only. Only called when "/test" is called
bool testMiddleware(PsychicRequest* req){
if( req->method() == HTTP_OPTIONS ){
request->addResponseHeader("Access-Control-Allow-Origin", "*");
request->addResponseHeader("Access-Control-Allow-Methods", "GET, POST, PUT, DELETE, OPTIONS");
request->addResponseHeader("Access-Control-Allow-Headers", "Content-Type, Authorization, Accept");
request->addResponseHeader("Access-Control-Max-Age", "86400");
return request->reply(200);
}
return false;
}
// Pass middleware function pointer
server.on("/test", testMiddleware, HTTP_GET, [this](PsychicRequest *request) {
// Process the request...
});// Endpoint middleware only ( other style )
server.use("/test",[](PsychicRequest* req){
if( req->method() == HTTP_OPTIONS ){
request->addResponseHeader("Access-Control-Allow-Origin", "*");
request->addResponseHeader("Access-Control-Allow-Methods", "GET, POST, PUT, DELETE, OPTIONS");
request->addResponseHeader("Access-Control-Allow-Headers", "Content-Type, Authorization, Accept");
request->addResponseHeader("Access-Control-Max-Age", "86400");
return request->reply(200);
}
return false;
});
// DO NOT pass middleware function pointer, let the global "/test" middleware run first
server.on("/test", HTTP_GET, [this](PsychicRequest *request) {
// Process the request...
});This would introduce a new use method for the library which would push the callback to a list.
This list would be then iterated over before calling any request cb. If parameters are matching ( for example the endpoint end/or the method ). Since the request is a pointer it could be passed forward to the main endpoint cb.
What do you guys think?
mathieucarbou
commented
1 month ago @hoeken : I agree with this , which is what most frameworks do.
Current Filter API should be deprecated IMO (or request parameter removed at least).
We will see after for the ESPAsync compat: it should not pose constrain to have a better design otherwise we will move nowhere.
server.on("/json", HTTP_GET, [](PsychicRequest* request, PsychicRequest* response) {
PsychicJsonResponse response = PsychicJsonResponse(response);
...
return response.send();
});```But more importantly, we need a filter chain capability to reuse common piece of codes. Something like:
// request filters are added to the filter chain on each endpoint
RequestFilter corsFilter = new CorsRequestFilter(params);
RequestFilter authc = new AuthcRequestFilter(username, pass);
RequestFilter authz = new AuthzRequestFilter(roleMap);
RequestFilter csrf = new AuthzRequestFilter(params);
[...]
server.on("/json", HTTP_GET, [](PsychicRequest* request, PsychicRequest* response) {
PsychicJsonResponse response = PsychicJsonResponse(response);
return response.send();
}).addRequestFilter(csrf).addRequestFilter(authc).addRequestFilter(authz)Here are different ways to write an interface for a RequestFilter:
class RequestFilter {
public:
typedef enum class {CONTINUE, ABORT} Result;
virtual Result filter(PsychicRequest* request, PsychicRequest* response);
}class RequestFilter {
public:
virtual bool filter(PsychicRequest* request, PsychicRequest* response);
}And the last one which is more flexible because it allowed composition of filter chain and it also allows a request filter to control the execution order of the filter chain by calling chain.filter(request, response) before or after its processing. This is what all web frameworks are doing in Java.
class RequestFilter {
public:
// implementation must call at the end chain.filter(request, response) in orde to continue the filter chain processing
virtual void filter(FilterChain* chain, PsychicRequest* request, PsychicRequest* response);
}
class FilterChain {
public:
void filter(PsychicRequest* request, PsychicRequest* response);
}@hitecSmartHome great, that is more or less exactly what i'm thinking.
@mathieucarbou is there any reason that we need to differentiate between filters and middleware? from what i can tell, we basically want a way of chaining some sort of filter/middleware/handler that receives a (request, response) that can modify the request, adding headers or even sending a full response, and then return some sort of flow control value (currently either continue or stop).
i'm going to refer to it as middlware for now, just for clarity. open to other names, (and we should do some sort of API review before we go live with v2.0.)
from a high level, you can add a middleware chain at:
lastly, we have a control flow enum being returned. i can see how this is useful as we could extend this in later releases for more advanced functionality, but realistically are there any other potential responses here? if we are simply iterating over a list of middleware until we are stopped, a boolean would be more straightforward and potentially less confusing for the end user. if anyone has examples from existing frameworks, i'd be keen to see how it works elsewhere.
if we extend this to its logical conclusion, we could probably even modify our handlers to become middleware. after all, what is a handler but something that takes a (request, response), calls some application specific user callbacks (such as onUpload(), onConnect, onDisconnect(), or for websocket onMessage()) and then generates an entire output and exits on completion. this is probably out of scope for this current refactor, but its definitely worth thinking about as we start down this road.
mathieucarbou
commented
1 month ago is there any reason that we need to differentiate between filters and middleware?
This is because (using my terms) a Filter (which activates or not a handler) does not receive any parameter and does not need some pre-processing or the request (load params), while the request should be minimally preprocessed before entering a filter chain because most use cases of filters are about looking at parameters and headers and reply depending on them.
Cors is no exclusion to this rule a real cors filter will have to look at the headers to reply. The cors "allow all" filter is just an exception.
i'm going to refer to it as middlware for now, just for clarity.
Ok (like ExpessJS).
- server level (run on every single request.) eg. server.addMiddleware(authMiddleware);
This is equivalent as /* with HTTP_ANY, but this shortcut is nice to have for authc. Authz is a bit different though.
- handler level (run on every request that gets to the handler level.) eg. WebsocketHandler or StaticFileHandle
Yes
- endpoint level (run on every endpoint that matches... which actually would be on the filter level as internally endpoints have a new generic handler assigned with the server.on() shortcut) eg. server.on("/admin", HTTP_GET, adminCallback).addMiddleware(adminAuthMiddleware)
Yes! Add would add to the filter chain.
lastly, we have a control flow enum being returned.
No we do not need. See how this is done in express js and also Java (my example above). Each middleware needs to call the second one, or ask the chain to run the next one. Thanks to this call action a middleware can decide to execute the other ones before itself, after itself, or just don't execute the next middleware.
if we extend this to its logical conclusion, we could probably even modify our handlers to become middleware.
No because a handler does not have access to the next middleware or the chain of middleware (see example API above).
Also a handler is specialized to the request type / content it needs to serve while a middleware is transparent to that and does not process the request but just filters it. It is application agnostic in the sense that its execution is tight to features like cors, security, csrf, etc that are not application specific.
Hope it helps :+1:
hitecSmartHome
commented
1 month ago It would not process things twice. The library is passing around pointers. Body and param parsing will be needed anyway. Doesnt matter where it is parsed. If the lib is parsing it before the handler it would not parse it again if there is a middleware. If there are middlewares, there is no need for filter. You can filter the request in a middleware too. You can even modify it to suit your cb needs. I dont see big value in filtering if we have middlewares
mathieucarbou
commented
1 month ago @hitecSmartHome : completely agree with you, but in previous discussion It looked as if @hoeken really wanted to optionally parse the params and headers for perf reasons...
80% of use cases of filters that return true/false is to active or not a filter chain / handler on external conditions (ap mode, sta mode, etc) => no need to parse
80-90% of use cases of middleware is to act on headers and parameters => I would parse params and headers by default
If really we are on the case of an exception app that is not requiring parsing, and / or perf is really a concern, then I would add a macro PSYCHIC_NO_REQUEST_PREPROCESS that, if set does not preload the parameter and headers and let the user do that inside the callback.
But by default I would ALWAYS provide to the users a completely parsed request because this is what they expect.
If we can put that in place then yes I agree with you that we can only have middlewares.
hitecSmartHome
commented
1 month ago
- 80% of use cases of filters that return true/false is to active or not a filter chain / handler on external conditions (ap mode, sta mode, etc) => no need to parse
I see. I completelly forget about ap and sta filtering. I never use them, just serving the same web app on ap too. But these really have their value.
hitecSmartHome
commented
1 month ago Maybe keep filters for these reasons but implement full on middlewares too :D
hoeken
commented
1 month ago @mathieucarbou headers + uri query params are automatically parsed in the request object. it is only for POST form data that you need to call loadParams() as the request body could be massive, or it could be an upload that the handler expects to process byte by byte, etc. the loadBody() function reads from an esp-idf buffer and i'm pretty sure there is no way to peek into that.
hoeken
commented
1 month ago @hitecSmartHome i'm going to convert everything under the hood to the new middleware style system. i'll keep the old filter stuff around for compatability, possibly with some depreciated preprocessor stuff. I'll see how the implementation goes.
hoeken
commented
1 month ago Here's my todo list / game plan for implementing middleware: https://github.com/hoeken/PsychicHttp/blob/v2-dev/middleware.md
mathieucarbou
commented
1 month ago @mathieucarbou headers + uri query params are automatically parsed in the request object. it is only for POST form data that you need to call loadParams() as the request body could be massive, or it could be an upload that the handler expects to process byte by byte, etc. the loadBody() function reads from an esp-idf buffer and i'm pretty sure there is no way to peek into that.
you are right... I completely forgotten again about this upload lol... Let's do it this way then with the required loadParams() call and we'll see how to handle that for the ESPAsync migration.
Quick question though...
What is then happening if a filter calls loadParams() to get the POST parameters of a multipart content, but this multipart content has a file upload, that needs to wait for the final handler to handle it ?
mathieucarbou
commented
1 month ago Here's my todo list / game plan for implementing middleware: https://github.com/hoeken/PsychicHttp/blob/v2-dev/middleware.md
@hoeken : for the middleware... I am really not in favor of returning enum (request, response). this will block the flexibility of the filter chain design.
Please have a look at other web frameworks implementations:
The right way to do that is to have a filter chain object controlling the sequence of filters and have a callback like this:
void (*chain, *request, *response)
inside a filter, here are all the possiblities:
chain.next();
// process filter// process filter
chain.next();// process filter and do not call chain.next() - response has to be committed - it won't fo go the handler// process filter first
chain.next();
// process filter afetrThe ability to control (surround) the code inside the filter with the chain makes a lot of designs possible like intercepting filters, etc but also filters that will act after a response is sent (logging filters for example). It allows also for compositions and patterns, like request scoped object:
Example of a Session filter:
session = getOrCreateSession();
chain.next(); // during all the processing further, the session object will be made available to other filters and handler
session.commit(); // save session or remove it from current "thread"@hoeken : also, note that for now I didn't want too far in the details. I sticked with these basic concepts of filter and filter chain . But after, It might be nice / required to introduce the concepts of Session, request.attributes, etc like a real web server would provide. I keep that for later, but the design above is REALLY a no brainer and required to implement further concept.
Just returning enum from a filter does not help control the sequence of the code executed in your filter in relation to the request processing (filter chain) (none, before, after, both).
hoeken
commented
1 month ago @mathieucarbou if you call loadParams it will load the body into a string (assuming it passes contentLength() > server()->maxRequestBodySize) if there is a file upload, currently it will ignore the uploaded file. if the handler is an upload handler, it will most likely fail. we could probably have that upload handler try and check the body string and process it that way.
its a bug, but fixable.
hoeken
commented
1 month ago @mathieucarbou i'll do some reading on this concept. it looks useful.
with this concept of middleware chains, how does the server get the final pass/fail result to decide if it will process the request? or are we still taking about having separate filters and middleware?
can you give me a basic example of how someone would use this? like for example to check ON_STA_FILTER and then something like BASIC_AUTH?
mathieucarbou
commented
1 month ago @mathieucarbou i'll do some reading on this concept. it looks useful.
with this concept of middleware chains, how does the server get the final pass/fail result to decide if it will process the request? or are we still taking about having separate filters and middleware?
can you give me a basic example of how someone would use this? like for example to check ON_STA_FILTER and then something like BASIC_AUTH?
Providing we get rid of the curent Filer API (we do not need it):
Middleware authc = new AuthcMiddleware(BASE64, username, pass) // could also be Authcv(DIGEST)
server.on("/dashboard").setFilter(ON_STA_FILTER).addMiddleware(authc)
class MiddlewareChain {
void next(Req*req, Res* res);
}
// Middleware with default implementation
class Middleware {
void handle(MiddlewareChain* chain, Req*req, Res* res) { chain.next(req, res) }
}
class AuthcMiddleware {
void handle(...) {
if(authenticated(...)) { chain.next(req, res) }
else req.send(401)
}
}
// this is an activation filter ! not a middleware
OnSTAFilter == []() { return WiFi.mode() == STA; } Also do not take the short path to convert the middleware chain class in a closure and pass a closure to the middleware:
void handle(<void>(req, res) MiddlewareNext, Req*req, Res* res) { chain.next(req, res) }
because the middleware chain has to be a class that may be extended.
There are some use cased where One would create its own FilterChain implemenation to "hook" inside an existing one. This pattern allows that.
hoeken
commented
1 month ago I'm confused. You say we would eliminate the filter API, but your example shows calling server.setFilter()
Is the filter concept completely separate from the middleware concept?
Or does middleware stop a request from processing by simply not calling chain.next()?
On Tue, Aug 13, 2024, 18:45 Mathieu Carbou @.***> wrote:
@mathieucarbou https://github.com/mathieucarbou i'll do some reading on this concept. it looks useful.
with this concept of middleware chains, how does the server get the final pass/fail result to decide if it will process the request? or are we still taking about having separate filters and middleware?
- with middleware chains: this is the middleware which decided if the processing continues
- I think we still need the filter with true return code to activate a endpoint
can you give me a basic example of how someone would use this? like for example to check ON_STA_FILTER and then something like BASIC_AUTH?
Providing we get rid of the curent Filer API (we do not need it):
Middleware authc = new AuthcMiddleware(BASE64, username, pass) // could also be Authcv(DIGEST) server.on("/dashboard").setFilter(ON_STA_FILTER).addMiddleware(authc)
class MiddlewareChain { void next(Reqreq, Res res); }
// RequestFilter with default implementation class Middleware { void handle(MiddlewareChain chain, Reqreq, Res* res) { chain.next(req, res) } }
class AuthcMiddleware { void handle(...) { if(authenticated(...)) { chain.next(req, res) } else req.send(401) } }
// this is an activation filter ! not a middleware OnSTAFilter == []() { return WiFi.mode() == STA; }
- The chain implementation has to keep track (pointer ?) to the current middleware and next would increment
- the last middleware delegates to the final handler
- a filter chain had always a final next(req, res) call that is valid and goes to the handler
— Reply to this email directly, view it on GitHub https://github.com/hoeken/PsychicHttp/issues/161#issuecomment-2287398512, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABEHSVACNHPXXGLRNNRITLZRKLAJAVCNFSM6AAAAABMNKQ5PCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOBXGM4TQNJRGI . You are receiving this because you were mentioned.Message ID: @.***>
mathieucarbou
commented
1 month ago Yes I think we should completely separate it and also remove the request parameter from Filter.
A filter acts on activating / deactivating a whole middleware chain + handler on a sever based on external conditions.
This is the equivalent of doing
on().. or removeEndpoint()
but this is just more dynamic.
A middleware chain is a request processing set of filters.
So the 2 concepts are completely different and act on different objects.
does middleware stop a request from processing by simply not calling chain.next()?
Yes ! This is also possible to not call chain.next(req, res) from a middle ware to stop processing further. This is my example above with authc. In that case the middleware is responsible for committing the response ( send (401) in the example)
hoeken
commented
1 month ago @mathieucarbou okay, back with another question :)
Should a middleware be allowed to stop the chain without sending a response? meaning, can they reject processing of a request so that another handler can take it? if they can, how do we go about determining the reason for the middleware chain not finishing?
since we are keeping filters, i would say that no, middleware is only for responding to a request or modifying the response. filters are for determining if we should handle the request at all.
btw, this is what the main request processing function is currently looking like:
esp_err_t PsychicHttpServer::requestHandler(httpd_req_t* req)
{
PsychicHttpServer* server = (PsychicHttpServer*)httpd_get_global_user_ctx(req->handle);
PsychicRequest request(server, req);
PsychicResponse response(&request);
// process any URL rewrites
server->_rewriteRequest(&request);
// run it through our global server filter list
if (!server->filter(&request))
return request.reply(400);
// run it through our global server middleware.
// false means the chain didnt complete and a response was sent.
if (!server->runMiddleware(&request, &response))
return ESP_OK;
// loop through our endpoints and see if anyone wants it.
for (auto* endpoint : server->_endpoints) {
if (endpoint->matches(request.uri().c_str())) {
if (endpoint->_method == request.method() || endpoint->_method == HTTP_ANY) {
request.setEndpoint(endpoint);
PsychicHandler* handler = endpoint->handler();
if (handler->filter(&request)) {
if (handler->runMiddleware(&request, &response))
return handler->handleRequest(&request);
else
return ESP_OK;
}
}
}
}
// loop through our global handlers and see if anyone wants it
for (auto* handler : server->_handlers) {
if (handler->filter(&request)) {
if (handler->runMiddleware(&request, &response))
return handler->handleRequest(&request);
else
return ESP_OK;
}
}
//if nothing hits, then try our 404 handler.
return PsychicHttpServer::notFoundHandler(req, HTTPD_404_NOT_FOUND);
}
For a variety of reasons, we need to implement some 'middleware' functionality where the user can define a number of functions to be called that work with a request + response. This would enable flexible and powerful functionality like:
The main limitation preventing us from doing this currently, is that the
onRequestcallback only gets passed in a pointer to thePsychicRequestobject, and then internally creates its ownPsychicResponseobject. In order to allow the middleware to modify the response (eg. by setting headers), we need to somehow create thePsychicResponseahead of time to be able to pass it to the middleware/filters.There are a number of ways we could achieve this:
1. Modify the onRequest callback and filter() callback to include a
PsychicResponseobject.This feels like the cleanest api to me - a filter or request handler can simply access the object needed without any extra work or declarations.
Currently, the all
PsychicResponseand other derivative classes take aPsychicRequestobject as the first parameter in the constructor. That could be changed to pass in the rootPsychicResponseobject for the advanced class to inherit from, then work continues on that.For example this simple json endpoint:
Becomes:
Another example, this time with a CORS middleware 'filter'
Becomes:
This could be implemented in parallel with the existing setup by creating new overloads to
server->on(),setFilter(), etc. The old functions could give #pragma warnings, and eventually be #depreciated.Biggest downside obviously is breaking compatibility, which could be alleviated.
2. Convert completely to the ESPAsyncWebserver style of request->beginReply()
This would have the advantage of keeping all the callback function definitions the same. It would also keep Psychic more in line with ESPAsyncWebserver styling. One disadvantage is that it would reduce the ability for users to extend the base PsychicResponse class with their own customized class which I think is a pretty nice feature.
3. Some sort of hybrid approach
I think it might be possible to do both ways. Something like this:
Since the
PsychicRequestobject now holds a pointer to thePsychicResponsethen all the following calls would be able to reference that and take advantage of the filter/middleware functionality:request->beginReply()would re-use internal response pointer instead of creating a new objectPsychicJsonResponse(request);would ultimately copy the data from the internalrequest->responseobject.Conclusion
This is me thinking out loud here about how to implement this in a way that makes sense and doesn't limit us in the future. Ideally we don't want to break anything either. Any thoughts or suggestions are welcome.