Fix #1224 by searching routes after failed match #2616
Conversation
|
(Also, compared to earlier PRs, this approach only adds runtime overhead when routes fail to match.) |
core/lib/src/router/router.rs
Outdated
| .filter(|method| *method != &req.method()) | ||
| .filter_map(|method| self.routes.get(method)) | ||
| .flatten() | ||
| .any(|route| paths_match(route, req)) |
There was a problem hiding this comment.
It should be possible to simplify this using the Router::routes method:
self.routes()
.any(|route| route.method != req.method() && paths_match(route, req))There was a problem hiding this comment.
Thanks, I'll check how that works out.
core/lib/src/server.rs
Outdated
| status = Status::UnsupportedMediaType; | ||
| } else { | ||
| status = Status::NotAcceptable; | ||
| } |
There was a problem hiding this comment.
I don’t think that returning NotAcceptable is correct here. This status code is about content negotiation via the request’s Accept header – e.g. the client wants application/json but the server can only deliver text/html. See https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/406
IMHO this branch should always return UnsupportedMediaType regardless of whether payload is supported.
There was a problem hiding this comment.
I don't quite follow your reasoning here. Rocket's route "format" specifier switches on exactly whether or not the method expects payload or not ( https://rocket.rs/v0.5/guide/requests/#format )
The logic here merely mirrors that same logic, and returns 406 if the route can only produce payload which the client doesn't want.
It would be wrong to return 415 for a GET route which can't return the desired content.
There was a problem hiding this comment.
My bad, I didn’t realize that this restriction had different meanings depending on the method. Your implementation is correct.
|
@jespersm Thank you for your contribution! Is there still something missing except rebasing? |
Ouch, I'm not sure, and I'm AFK for a few days, so I can't really check or rebase at the moment. |
|
No stress, thank you for your work when you have time 😅 |
jespersm
force-pushed
the
fix_issue_1224
branch
from
35669cc
to
96f2727
Compare
|
@reneleonhardt : I have now ported my PR to the latest main, and it seems to pass the CI builds and tests: https://github.com/jespersm/Rocket/actions/runs/10726665635/job/29747224230 |
jespersm
force-pushed
the
fix_issue_1224
branch
from
3b43a03
to
be52278
Compare
|
Cool, I hope your improvements get merged soon! 🚀 |
This PR fixes #1224 by introducing more detailed HTTP status reporting.
If the request is matched, this patch does nothing. That way, routes which set their own HTTP status code are not affected even if they return 404.
If no route matches the request, this PR checks for "near matches", first looking to see if any routes failed only the format check, and afterwards checks for matching paths, but on a different HTTP method.
For format-check failures for POST and PUT, the HTTP status code will be 415 Unsupported Media Type (since this means that the Content-Type of the request didn't match what the route expected). For GET, etc., a failed format check will be 406 Not Acceptable, since the route couldn't produce what the client requested.
If the method and path didn't match at all, but the path is present as a route for one of the other HTTP methods, 405 Method Not Allowed is returned.
Otherwise, the 404 Not Found is left as is.
Tests were corrected as required.