fix: header in const request bug and improve code architecture #333
Conversation
src/executors/mod.rs
Outdated
| @@ -1,7 +1,7 @@ | |||
| /// This is the module that has all the executor functions | |||
| /// i.e. the functions that have the responsibility of parsing and executing functions. | |||
| use crate::io_helpers::read_file; | |||
| use crate::types::Request; | |||
There was a problem hiding this comment.
I think importing use crates::types::{Request, Response} would make more sense. Just a nit
There was a problem hiding this comment.
Yes, far better. Autoimport does some weird things sometimes
AntoineRR
force-pushed
the
fix/headers
branch
from
3c53aa6 to
26ce1ee
Compare
| pub fn apply_hashmap_headers( | ||
| response: &mut HttpResponseBuilder, | ||
| headers: &HashMap<String, String>, | ||
| ) { | ||
| for (key, val) in headers.iter() { | ||
| response.insert_header((key.clone(), val.clone())); | ||
| } | ||
| } | ||
|
|
||
| #[inline] | ||
| pub fn apply_dashmap_headers( | ||
| response: &mut HttpResponseBuilder, | ||
| headers: &DashMap<String, String>, | ||
| ) { | ||
| for h in headers.iter() { | ||
| response.insert_header((h.key().clone(), h.value().clone())); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This is very smart! Good job! 👍
There was a problem hiding this comment.
Thanks ;) This avoids merging the headers and then applying them, which creates an unnecessary copy of both headers list. I wanted to make only one function that works with either a hashmap or a dashmap but it doesn't seem to be easy
| async fn apply_middleware( | ||
| request: &mut Request, | ||
| middleware_type: MiddlewareRoute, | ||
| middleware_router: &MiddlewareRouter, | ||
| route_uri: &str, |
There was a problem hiding this comment.
@AntoineRR , can you please explain this function a bit?
There was a problem hiding this comment.
Sure!
We have two middlewares to apply, one before routing and another after it.
We first route on the middleware function and execute it, which gives us some modifications to apply to the request object. We only care about the headers for now => they become the request headers.
Therefore the middleware could modify the request (at least the "before" middleware for Robyn currently), which is why request is &mut.
For the "after" middleware, I think it cannot currently modify the request or the response, so the execute_midlleware_function should return an empty hashmap and we will just have executed the python function.
The idea here is just to factorize the code to execute both middlewares so it is easier to understand and maintain. Although if you think it makes sense to keep the execution of both middleware separate it is also possible
There was a problem hiding this comment.
Perfect. @AntoineRR do you think we can modify request body in before_request and response body in after_request?
There was a problem hiding this comment.
I think it would make sense yes. Maybe introducing a common trait for Request and Response could help us with this. The apply_middleware method would become generic and take either a Request or a Response, and the trait will then provide a method for parsing the hashmap returned by execute_middleware_function and apply it to the object.
There was a problem hiding this comment.
I could try to do this in an other PR, as it will add a new feature for Robyn, i.e. response modification in "after" middleware. What do you think of it?
There was a problem hiding this comment.
I could try to do this in an other PR, as it will add a new feature for Robyn, i.e. response modification in "after" middleware. What do you think of it?
Sounds great. We implemented some of it here too: #297
Just in case you need any reference.
Maybe introducing a common trait for Request and Response could help us with this
What kind of trait will it be?
There was a problem hiding this comment.
Thanks for the reference :)
This could be something like this I guess:
trait Data {
fn modify_from_hashmap(&mut self, modifications: Hashmap<String, String>) -> &mut self;
}
and then apply_middleware signature would become:
async fn apply_middleware<T: Data>(
data: &mut T,
middleware_type: MiddlewareRoute,
middleware_router: &MiddlewareRouter,
route_uri: &str,
)
After writing impl of Data for Request and Response, they could be used as the data parameter for the function. Then we call data.modify_from_hashmap with the hashmap from execute_middleware_function and we're good! Using generics in Rust is a zero cost abstraction due to monomorphization so there will be no performance impact from this. Using this solution makes modifying requests and response from a hashmap easier IMO, and everything is contained inside the Request and Response struct.
I don't like the name Data for the trait but it is just an example here.
There was a problem hiding this comment.
Alright, that makes sense. Perfect. 😄
There was a problem hiding this comment.
Using generics in Rust is a zero cost abstraction due to monomorphization so there will be no performance impact from this.
I wasn't aware of this. Thank you! 😄
* fix: allow response headers * fix: rename get_headers to get_request_headers * fix: header in const request bug and improve code architecture (#333) * test: add integration tests to verify headers in non const requests * docs: add documentation of per route headers Co-authored-by: Antoine Romero-Romero <[email protected]>
Description
This PR fixes #323
This is my suggestion for fixing the issue as well as improving the request handling
Responsestruct created from the hashmap returned by the python functions inexecute_http_request. I would have liked to return aResponsedirectly from the py function by mapping the Rust struct to a Python TypedDict but I'm not sure if it is doable? Maybe consider it for a later PR.request_handlermodule (which became obsolete with the previous point)