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

Fix #1526 trailing slash to any route #1563

Merged
merged 3 commits into from
May 6, 2020

Conversation

lammel
Copy link
Contributor

@lammel lammel commented May 2, 2020

This PR fixes #1526 to handle a corner case for a request with trailing slash that shall match an any route just below the request path.

GET /users/ will now correctly match the any route /users/*

@codecov
Copy link

codecov bot commented May 2, 2020

Codecov Report

Merging #1563 into master will increase coverage by 0.10%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1563      +/-   ##
==========================================
+ Coverage   84.85%   84.96%   +0.10%     
==========================================
  Files          28       28              
  Lines        2166     2168       +2     
==========================================
+ Hits         1838     1842       +4     
+ Misses        213      211       -2     
  Partials      115      115              
Impacted Files Coverage Δ
router.go 97.15% <100.00%> (+0.02%) ⬆️
response.go 76.31% <0.00%> (+5.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2207c37...d4bc3fb. Read the comment docs.

@lammel
Copy link
Contributor Author

lammel commented May 2, 2020

Benchmarks are showing slight improvements for complex routes (5 to 15% faster) and a slowdown for the ParseAPI routes (around +17%)

pkg: github.com/labstack/echo/v4
BenchmarkRouterStaticRoutes-8    	   67260	     16516 ns/op	       0 B/op	       0 allocs/op
BenchmarkRouterGitHubAPI-8       	   32832	     34217 ns/op	       2 B/op	       0 allocs/op
BenchmarkRouterParseAPI-8        	  199347	      5694 ns/op	       0 B/op	       0 allocs/op
BenchmarkRouterGooglePlusAPI-8   	  112566	     10480 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/labstack/echo/v4	5.279s

WIth this fix:

pkg: github.com/labstack/echo/v4
BenchmarkRouterStaticRoutes-8    	   68384	     16857 ns/op	       0 B/op	       0 allocs/op
BenchmarkRouterGitHubAPI-8       	   34982	     32503 ns/op	       2 B/op	       0 allocs/op
BenchmarkRouterParseAPI-8        	  169191	      6680 ns/op	       0 B/op	       0 allocs/op
BenchmarkRouterGooglePlusAPI-8   	  128191	      8870 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/labstack/echo/v4	7.078s

@lammel
Copy link
Contributor Author

lammel commented May 2, 2020

@vishr Comments welcome. A duplicate check for cn.findChildByKind(akind) when the route search is exhausted (search = "") and we need to lookup if a any child is still down the route tree is the cause for the slight performance decrease for some benchmarks.
Did not have time to optimize that away, not sure that is easily possible.

Overall I think it is acceptable as it is.

@lammel lammel mentioned this pull request May 2, 2020
3 tasks
@lammel
Copy link
Contributor Author

lammel commented May 6, 2020

Update for the benchmarks against current master. Again a little ups and downs.
Benchmarks do not to be very reliable as we have up to 10% deviation for the same code per run.

Results for master:

master$ /usr/bin/go test -benchmem -run=^$ github.com/labstack/echo/v4 -bench '^(BenchmarkRouterStaticRoutes|BenchmarkRouterGitHubAPI|BenchmarkRouterParseAPI|BenchmarkRouterGooglePlusAPI)$' -benchtime 10s -cpu 2
goos: linux
goarch: amd64
pkg: github.com/labstack/echo/v4
BenchmarkRouterStaticRoutes-2    	  779059	     14300 ns/op	       0 B/op	       0 allocs/op
BenchmarkRouterGitHubAPI-2       	  394016	     29629 ns/op	       0 B/op	       0 allocs/op
BenchmarkRouterParseAPI-2        	 2559226	      4633 ns/op	       0 B/op	       0 allocs/op
BenchmarkRouterGooglePlusAPI-2   	 1500585	      8078 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/labstack/echo/v4	60.088s

with the latest commits of this PR:

bugfix/1526-..$ /usr/bin/go test -benchmem -run=^$ github.com/labstack/echo/v4 -bench '^(BenchmarkRouterStaticRoutes|BenchmarkRouterGitHubAPI|BenchmarkRouterParseAPI|BenchmarkRouterGooglePlusAPI)$' -benchtime 10s
goos: linux
goarch: amd64
pkg: github.com/labstack/echo/v4
BenchmarkRouterStaticRoutes-16     	  774600	     14568 ns/op	       0 B/op	       0 allocs/op
BenchmarkRouterGitHubAPI-16        	  353102	     32341 ns/op	       0 B/op	       0 allocs/op
BenchmarkRouterParseAPI-16         	 2439986	      4926 ns/op	       0 B/op	       0 allocs/op
BenchmarkRouterGooglePlusAPI-16    	 1545140	      7864 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/labstack/echo/v4	60.229s

@vishr vishr merged commit 43e32ba into labstack:master May 6, 2020
@lammel lammel deleted the bugfix/1526-route-trailing-slash branch May 12, 2020 07:47
@jeanphilippe-mh
Copy link

Thanks you for this fix @lammel 🙏, i've had the same issue with echo v4.1.16 that breaks CORS policy. I've rollbacked to 4.1.14 through go.mod and it's working like a charm again!
Waiting for Echo release 4.1.17 now 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Routing Bug or Feature?
3 participants