Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #1224 by searching routes after failed match #2616

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jespersm
Copy link

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.

@jespersm
Copy link
Author

(Also, compared to earlier PRs, this approach only adds runtime overhead when routes fail to match.)

Copy link

@palant palant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I am not the maintainer of this repository, so I’m only nit picking here and you are free to ignore my comments.

.filter(|method| *method != &req.method())
.filter_map(|method| self.routes.get(method))
.flatten()
.any(|route| paths_match(route, req))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be possible to simplify this using the Router::routes method:

self.routes()
    .any(|route| route.method != req.method() && paths_match(route, req))

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll check how that works out.

status = Status::UnsupportedMediaType;
} else {
status = Status::NotAcceptable;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, I didn’t realize that this restriction had different meanings depending on the method. Your implementation is correct.

@reneleonhardt
Copy link

@jespersm Thank you for your contribution! Is there still something missing except rebasing?

@jespersm
Copy link
Author

@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.

@reneleonhardt
Copy link

No stress, thank you for your work when you have time 😅

@jespersm
Copy link
Author

jespersm commented Sep 5, 2024

@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
I added some extra checks to make the hints more precise.

@reneleonhardt
Copy link

Cool, I hope your improvements get merged soon! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rocket should show 405 instead of 404 for method not allowed
3 participants