Skip to content

Fix #1493 router loop for param routes #1501

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

Merged
merged 3 commits into from
Feb 19, 2020

Conversation

lammel
Copy link
Contributor

@lammel lammel commented Feb 8, 2020

This pull request fixes issue #1493, which refers to a router loop recently introduced with commit 5bf6888 to fix param values. The router Find function will now correctly set the parameters and avoid the endless routing loop.

An additional test for the loop has been added (it is reproducable) and some router tests have been corrected to use a fresh context for the tests (to avoid leaking state).

Benchmark

Benchmarks are looking good, only minimal changes (~1% for static route performance)

With this patch:

rl@slimboy branch:bugfix/1493-# /usr/bin/go test -benchmem -run=^$ github.com/labstack/echo/v4 -bench '^(BenchmarkRouterStaticRoutes|BenchmarkRouterGitHubAPI|BenchmarkRouterParseAPI|BenchmarkRouterGooglePlusAPI)$'
goos: linux
goarch: amd64
pkg: github.com/labstack/echo/v4
BenchmarkRouterStaticRoutes-8    	  91005	    11681 ns/op	      0 B/op	      0 allocs/op
BenchmarkRouterGitHubAPI-8       	  48444	    24682 ns/op	      1 B/op	      0 allocs/op
BenchmarkRouterParseAPI-8        	 293914	     3933 ns/op	      0 B/op	      0 allocs/op
BenchmarkRouterGooglePlusAPI-8   	 187198	     6247 ns/op	      0 B/op	      0 allocs/op
PASS
ok  	github.com/labstack/echo/v4	5.083s

Before the patch (master@75620e6, some commits after v4.1.14):

rl@slimboy branch:master# /usr/bin/go test -benchmem -run=^$ github.com/labstack/echo/v4 -bench '^(BenchmarkRouterStaticRoutes|BenchmarkRouterGitHubAPI|BenchmarkRouterParseAPI|BenchmarkRouterGooglePlusAPI)$'
goos: linux
goarch: amd64
pkg: github.com/labstack/echo/v4
BenchmarkRouterStaticRoutes-8    	  93324	    11502 ns/op	      0 B/op	      0 allocs/op
BenchmarkRouterGitHubAPI-8       	  46220	    25976 ns/op	      1 B/op	      0 allocs/op
BenchmarkRouterParseAPI-8        	 276850	     3974 ns/op	      0 B/op	      0 allocs/op
BenchmarkRouterGooglePlusAPI-8   	 193024	     6177 ns/op	      0 B/op	      0 allocs/op
PASS
ok  	github.com/labstack/echo/v4	5.075s

@codecov
Copy link

codecov bot commented Feb 8, 2020

Codecov Report

Merging #1501 into master will increase coverage by 0.12%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1501      +/-   ##
==========================================
+ Coverage   84.42%   84.54%   +0.12%     
==========================================
  Files          27       27              
  Lines        2093     2097       +4     
==========================================
+ Hits         1767     1773       +6     
+ Misses        212      211       -1     
+ Partials      114      113       -1     
Impacted Files Coverage Δ
router.go 97.13% <0.00%> (+0.76%) ⬆️

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 504f39a...1a717ee. Read the comment docs.

@lammel
Copy link
Contributor Author

lammel commented Feb 18, 2020

@vishr Any chance this can be merged?
It has already bitten some people upgrading to the latest version.

@lammel lammel force-pushed the bugfix/1493-router-loop branch from 6c4c861 to 1a717ee Compare February 18, 2020 15:15
@lammel
Copy link
Contributor Author

lammel commented Feb 18, 2020

Rebased the branch on current master.

@vishr vishr merged commit f4b5a90 into labstack:master Feb 19, 2020
@vishr
Copy link
Member

vishr commented Feb 19, 2020

@lammel thanks for your work!

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.

2 participants