Allow parameter routes to end with a dot (/foo/:id.json) #1101
Conversation
Currently a route in the form of `/foo/:id.json` means echo will detect the parameter name `id.json` instead of the expected `id`. I think this is rather counter-intuitive, as adding an extension to paths is a fairly common use case. With this change both a `/` and a `.` will be treated as the end of a parameter name. Benchmark before this change: $ go test -bench . [..] goos: linux goarch: amd64 pkg: github.com/labstack/echo BenchmarkRouterStaticRoutes-4 100000 17743 ns/op 0 B/op 0 allocs/op BenchmarkRouterGitHubAPI-4 50000 33081 ns/op 1 B/op 0 allocs/op BenchmarkRouterParseAPI-4 300000 5370 ns/op 0 B/op 0 allocs/op BenchmarkRouterGooglePlusAPI-4 200000 9183 ns/op 0 B/op 0 allocs/op PASS ok github.com/labstack/echo 8.565s After this change: goos: linux goarch: amd64 pkg: github.com/labstack/echo BenchmarkRouterStaticRoutes-4 100000 17699 ns/op 0 B/op 0 allocs/op BenchmarkRouterGitHubAPI-4 50000 32962 ns/op 1 B/op 0 allocs/op BenchmarkRouterParseAPI-4 300000 5450 ns/op 0 B/op 0 allocs/op BenchmarkRouterGooglePlusAPI-4 200000 9205 ns/op 0 B/op 0 allocs/op PASS ok github.com/labstack/echo 8.590s Fixes #599
Codecov Report
@@ Coverage Diff @@
## master #1101 +/- ##
=======================================
Coverage 81.14% 81.14%
=======================================
Files 25 25
Lines 1899 1899
=======================================
Hits 1541 1541
Misses 250 250
Partials 108 108
Continue to review full report at Codecov.
|
|
@vishr Any thoughts on this? We've had to add some unpleasant changes in our middleware and it would be great for the echo router to support this pattern out of the box. |
|
The caveat I see is that you can't have a param with |
Yeah, that's true. I don't know if anyone actually needs/does this though? Any change to this will be backwards-incompatible. For example someone mentioned routes in the form of |
|
It won’t affect us. Periods in param names are probably rare. If the allowed chars for param names has been well documented before now, I would not make a breaking change. If it hasn’t been, then this was always the intended behavior, and this is just fixing a bug ;) |
|
@Carpetsmoker @ansel1 thanks for your contribution 🎉 |
This is a follow-up to #1101. It turns out that that patch is incomplete, as a similar check also needs to be added in the `Router.Add()` method. I don't understand why the test works fine, but when using it in a real application. For example with this example: func main() { e := echo.New() e.GET("/xxx/:id.json", func(c echo.Context) error { return c.String(200, fmt.Sprintf("%#v: names: %#v; vals: %#v", c.Path(), c.ParamNames(), c.ParamValues())) }) log.Fatal(e.Start(":8000")) } Gives a 404 on `/xxx/42.json`, and for `/xxx/42` it gives the output: /xxx/:id.json": names: []string{"id.json"}; vals: []string{"42"} It makes sense to add the test there too; I just don't get why the test cases that I added in ##1101 *does* produce the correct output :-/
Currently a route in the form of
/foo/:id.jsonmeans echo will detectthe parameter name
id.jsoninstead of the expectedid. I think thisis rather counter-intuitive, as adding an extension to paths is a fairly
common use case.
With this change both a
/and a.will be treated as the end of aparameter name.
Benchmark before this change:
After this change:
Fixes #599