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

Implementing HTTP 405 if HTTP method doesn't match with required by the route #1276

Closed
wants to merge 13 commits into from

Conversation

Drunpy
Copy link

@Drunpy Drunpy commented Apr 14, 2020

This PR purpose is to meet RFC 7231 sec6.6.5 and #1224

@Drunpy Drunpy changed the title Returning HTTP 405 if HTTP method doesn't match with required by route [WIP] Return HTTP 405 if HTTP method doesn't match with required by route Apr 15, 2020
@Drunpy Drunpy marked this pull request as draft April 17, 2020 11:43
@SergioBenitez
Copy link
Member

It looks like the tests are failing due to a UI mismatch almost certainly caused by differing versions of rustc. Check that your nightly compiler is up to date as this is what the CI will use. Once it is, you should get the same failures locally.

@jebrosen
Copy link
Collaborator

CI has been failing on master (probably for a while, sorry) and I only got around to fixing it yesterday.

@Drunpy, if you rebase your branch on top of the latest master the failing ui-tests should start passing again.

@Drunpy Drunpy marked this pull request as ready for review April 25, 2020 01:00
@Drunpy Drunpy changed the title [WIP] Return HTTP 405 if HTTP method doesn't match with required by route Implementing HTTP 405 if HTTP method doesn't match with required by the route Apr 25, 2020
@Drunpy
Copy link
Author

Drunpy commented Apr 28, 2020

Ready for review ✔️ @jebrosen

Copy link
Collaborator

@jebrosen jebrosen left a comment

Choose a reason for hiding this comment

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

Drat, I should have looked at this earlier so I could notice the rustfmt. Thanks for keeping those in separate commits though - that should make it easier to separate those changes out again.

If I'm reading right, the bulk of the change is to have the router not use method as a filter when finding matching routes, and to instead handle method mismatch one level up in Rocket::route. The overall approach seems reasonable, but I only have a fuzzy understanding of the routing code and would prefer to have a confirmation from @SergioBenitez before doing a thorough review.

Copy link
Member

@SergioBenitez SergioBenitez left a comment

Choose a reason for hiding this comment

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

This is incredibly difficult to review with all of the formatting changes. Please resubmit a PR without any formatting changes to existing code, and if possible, follow the existing style where you can.

However, I'd like to note the following: at a very high level, I'm for this change. My concern is the performance impact that this will have. It's very nice to be able to filter by method: it means we use it as a prefix. We should not remove this from the router. At worst, we should simply iterate through all possible methods, checking to see if there's a match, in case no route was found. This means we take the performance hit only if there's no match initially.

@SergioBenitez SergioBenitez added the pr: closed This pull request was not merged label Jun 6, 2020
@Drunpy Drunpy mentioned this pull request Jun 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: closed This pull request was not merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants