search

buaazp / fasthttprouter

A high performance fasthttp request router that scales well
http://godoc.org/github.com/buaazp/fasthttprouter
BSD 3-Clause "New" or "Revised" License
876 stars 91 forks source link

Using original path in router #43

Closed GilbertGeliba closed 5 years ago

GilbertGeliba commented 6 years ago

Router's Handler method is using ctx.Path() to get the path, which is already parsed (normalized) by fasthttp. As a result, router attributes like RedirectTrailingSlash and RedirectFixedPath will never be used, since the paths it gets are always fixed. Also, for example, requests to /path//info can never get to the intended handler (which handles request on path /path/:param/info), since the path are getting normalized to /path/info by fasthttp.

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 99.708% when pulling 240194e839147c1a0e185f791097000a65718ae7 on GilbertGeliba:master into 28db72dff5a1512dab256b4b7f85c5aa8c36cf73 on buaazp:master.

erikdubbelboer commented 6 years ago

Looks like this change makes TestRouterNotFound fail because it expects the CleanPath behaviour.

The readme also contains the following:

Path auto-correction: Besides detecting the missing or additional trailing slash at no extra cost, the router can also fix wrong cases and remove superfluous path elements (like ../ or //).

So I guess the question is if this behavior should change or not. It would be a breaking change for people that depend on the behavior as it is documented and not a bug.

GilbertGeliba commented 6 years ago

The CleanPath test looks odd to me. It should not expect the code 200, since the router will do redirection, right? It should be 301 I think. For both RedirectTrailingSlash and RedirectFixedPath:

the router makes a redirection to the corrected path with status code 301 for GET requests and 307 for all other request methods

GilbertGeliba commented 6 years ago

I think it IS a bug. The behavior as it is documented to work on the original path, not the one has already been normalized. The CleanPath test should also be fixed. The test was passing because the path was normalized by fasthttp and the request was handled by the /path handler directly. So that's why it returned code 200. More importantly, the test was not even hitting the code it was intended to test.

GilbertGeliba commented 6 years ago

@buaazp Sorry to bother, but I really need some attention here. This is not a breaking change in my opinion or just in case we could have a patch version.

savsgio commented 5 years ago

Hi @GilbertGeliba ,

The official fasthttp organization is maintaining this repo in https://github.com/fasthttp/router.

Recently, I fixed this in addition to other optimizations. I invite you to use the official router of fasthttp.

Thanks.

oschwald commented 5 years ago

I tried switching to github.com/fasthttp/router but this behavior change made me back out of that switch.

savsgio commented 5 years ago

Could you say me more about that? Please

savsgio commented 5 years ago

I advise you to fix your code to works with router v0.3.0 and also benefit from performance improvements

savsgio commented 5 years ago

But I would like to know how it's your error.

oschwald commented 5 years ago

The issue is precisely what was described in https://github.com/buaazp/fasthttprouter/pull/43#issuecomment-405481766. Our users have come to rely on the normalization and we would either need to get them to update their code, which is always a big ask, or normalize the path elsewhere.

As far as "fixing" our code, if we were to do that, we probably would just get rid of the use of the router altogether.

savsgio commented 5 years ago

I read the PR again I think I will revert this fix to non break the api to most people that expect this behaviuor.

savsgio commented 5 years ago

@oschwald,

I reverted the fix in v0.3.1, try again to use it..

I think now is better to not change the behaviour of router.

I really prefer to use normalization of fasthttp, the truth.

oschwald commented 5 years ago

@savsgio, thanks! v0.3.1 seems to work well.

GilbertGeliba commented 5 years ago

@savsgio

Hi @GilbertGeliba ,

The official fasthttp organization is maintaining this repo in https://github.com/fasthttp/router.

Recently, I fixed this in addition to other optimizations. I invite you to use the official router of fasthttp.

Thanks.

Thanks, I would love to stick to the official fasthttp. I will try using v0.3.0, and let you know if it works.

buaazp commented 5 years ago

Sorry for late reply. @GilbertGeliba I have so many tough works and haven't deal github issues for a long time.

I agree use PathOriginal in router. And thanks for your PR very much.