Path collision for handlers on different methods leading to 404 or Method not allowed responses

labstack / echo

High performance, minimalist Go web framework

https://echo.labstack.com

MIT License

29.86k stars 2.23k forks source link

Path collision for handlers on different methods leading to 404 or Method not allowed responses #1225

Closed arussellsaw closed 5 years ago

arussellsaw commented 5 years ago

Issue Description

Hi!

i've encountered a bug where the following routes collide and lead to 404 or Method not allowed responses

DELETE /:id/foo/:scheme PUT /:id/foo/bar PUT /:id/foo/baz

i've opened a PR with failing tests that reproduce this bug in our framework here: https://github.com/monzo/typhon/pull/103

(direct link to echo-only test here: https://github.com/monzo/typhon/pull/103/files#diff-ed15360a5b63dcacb420fca89f41239cR193 ) thanks! 🙏

Checklist

[x] Dependencies installed
[x] No typos
[x] Searched existing issues and docs

Expected behaviour

i should be able to call DELETE /23124/foo/bar and hit the DELETE handler registered

Actual behaviour

i get a Method not allowed response

Steps to reproduce

Working code to debug

https://github.com/monzo/typhon/pull/103

Version/commit

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

alexaandru commented 5 years ago

Re-opening this as I think it is important to investigate. Thank you for your report @arussellsaw

alexaandru commented 5 years ago

I can reproduce this. Not sure if we can fix it since it may change routing in backward incompatible ways. I do consider it a bug, but people may rely (even if not knowing it) on this bug to craft their routes by now, which makes it tricky to fix it. @vishr @im-kulikov what are your thoughts?

im-kulikov commented 5 years ago

What if we fix the bug and add a description to the release (docs) that describes the problem? In one of the previous releases of Go changed the json.Unmarshal / Marshal thus breaking backwards compatibility, but described the issue in the release/docs. It was quite normal.

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.