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

Don't redirect to slash route when method does not match #907

Merged
merged 2 commits into from
Aug 28, 2016

Conversation

JelteF
Copy link
Contributor

@JelteF JelteF commented Apr 19, 2016

This is a very simple fix for pallets/flask#1783, it simply only redirects to slash when the method matches as well. I think this would qualify as backwards compatible as it only changes behaviour for routes that previously returned a 405 where there were actual routes matching the route exactly.

@JelteF
Copy link
Contributor Author

JelteF commented Apr 19, 2016

Hmm, I see it's failing some tests. I'll look into those.

@jeffwidman
Copy link
Contributor

jeffwidman commented Apr 19, 2016

I think you're on the right track here. Thanks for working on this.

@JelteF JelteF force-pushed the fix-strict_slashes-methods branch from 69f0afa to 596bd0e Compare April 19, 2016 19:04
@JelteF JelteF force-pushed the fix-strict_slashes-methods branch from 596bd0e to 33258e1 Compare April 19, 2016 19:06
@JelteF
Copy link
Contributor Author

JelteF commented Apr 19, 2016

I think they should be fixed now. I didn't check if self.methods was defined.

@JelteF
Copy link
Contributor Author

JelteF commented Apr 19, 2016

BTW, I also changed the documentation of the method as it mentioned that the method could be supplied in the path, but that didn't work at all.

@@ -765,7 +765,9 @@ def match(self, path):
# tells the map to redirect to the same url but with a
# trailing slash
if self.strict_slashes and not self.is_leaf and \
not groups.pop('__suffix__'):
not groups.pop('__suffix__') and \
(method is None or self.methods is None or

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@untitaker
Copy link
Contributor

LGTM, but I'm unsure if I might be missing something. @mitsuhiko should look at this since I'm not very familiar with routing.

@davidism
Copy link
Member

Needs a test.

@JelteF
Copy link
Contributor Author

JelteF commented Apr 20, 2016

I just added tests, that I think cover all scenarios. I think the raised exceptions make sense as well.

@JelteF
Copy link
Contributor Author

JelteF commented Apr 20, 2016

Should I squash the commits together afterwards or should I leave them like this?

@JelteF
Copy link
Contributor Author

JelteF commented May 22, 2016

Any updates on merging this?

@untitaker
Copy link
Contributor

I've just pinged @mitsuhiko in IRC.

@untitaker
Copy link
Contributor

Sorry for the long wait. I've just merged this and it will land in 0.12. Thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants