Implement a default OPTIONS handler and complement the handler for HTTP 405 in router #743
Conversation
| let registered_methods = self | ||
| .method_map | ||
| .iter() | ||
| .filter(|(_, r)| r.recognize(path).is_ok()) |
There was a problem hiding this comment.
It is inevitable to query every MethodRouter in the method_map to get all registered/supported methods which are used by the Allow header.
As is discussed in #51 (comment), it is somewhat inefficient. But now that L71 already do this to determine whether it falls to the branch handling 405 or not, I suppose it is fine for now (at least, it is not a new problem).
| Selection { | ||
| endpoint: &method_not_allowed, | ||
| params: Params::new(), | ||
| endpoint: if method == Method::Options { |
There was a problem hiding this comment.
I want to pass a closure here as the endpoint. But it seems impossible as Selection.endpoint takes a reference?
To circumvent the limitation, I am trying to pass it in the params from which it is then extracted in the endpoint function. I am not sure it is good or not as it is a little tricky and incurring unnecessary overhead. But if the way is acceptable, we can let the CorsMiddleware also extract the supported methods to fill Access-Control-Allow-Methods in CORS preflight requests without refactoring a lot.
| let allowed_origin = response | ||
| .header(http::headers::ACCESS_CONTROL_ALLOW_ORIGIN) | ||
| .map(|origin| Origin::from(origin.as_str())); | ||
| assert_eq!(allowed_origin.unwrap(), Origin::from(self_origin)); |
There was a problem hiding this comment.
The current implementation of CorsMiddleware responses directly to CORS preflight requests, without reaching the endpoint.
Line 155 in b0eafba
So the newly added endpoint handler won't interfere with the existing CORS handling flow.
Hello.
#388 did implement a handler for
405 Method Not Allowed. But it does not set theAllowheader, which is a must according to RFC7231.Following #51 (comment), I am drafting a supplementary implementation for handling
405 Method Not Allowed.Besides and based on that, it appears to be trivial to implement a default handler for the HTTP method
OPTIONS(unrelated to CORS) withAllowset. Compared to the former, the difference is just the response status.After finishing the commit and before submitting the PR, I notice it has an intersection with #104. But the latter has not been updated for a long while and the codebase has changed a lot since then, so I think it is worth opening another.
There are still some problems to solve. Let me add them in review comments below.