fix the misplacement of adding slashes #2847

citizen233 · 2021-08-29T13:13:08Z

fix bug #2843

appleboy · 2021-08-29T13:53:59Z

Thanks for your effort. Can you also provide the benchmark result like this comment: #2767 (comment)

Hi @qm012 @rw-access, please also take a look if you have free time.

citizen233 · 2021-08-29T14:51:18Z

How to do benchmark test, please guide me

qm012

How to do benchmark test, please guide me

Please refer to the #2796 (comment)

fork https://github.com/gin-gonic/go-http-routing-benchmark
Go.mod changes the 'gin' branch version
Run the code for 'this PR' and 'master' using 'https://app.travis-ci.com/'
Use 'benchstat old.txt new.txt' to generate test reports

qm012 · 2021-08-30T01:15:14Z

routes_test.go

@@ -621,3 +621,24 @@ func TestRouteContextHoldsFullPath(t *testing.T) {
 	w := performRequest(router, http.MethodGet, "/not-found")
 	assert.Equal(t, http.StatusNotFound, w.Code)
 }
+
+// Reproduction test for the bug of issue #2843
+func TestRouteTrailingSlashRoute(t *testing.T) {


Perhaps we don't need a new function, could we integrate it into the function 'TestRouterNotFound'?

gin/routes_test.go

Lines 434 to 470 in 4e75841

func TestRouterNotFound(t *testing.T) {

router := New()

router.RedirectFixedPath = true

router.GET("/path", func(c *Context) {})

router.GET("/dir/", func(c *Context) {})

router.GET("/", func(c *Context) {})

testRoutes := []struct {

route string

code int

location string

}{

{"/path/", http.StatusMovedPermanently, "/path"}, // TSR -/

{"/dir", http.StatusMovedPermanently, "/dir/"}, // TSR +/

{"/PATH", http.StatusMovedPermanently, "/path"}, // Fixed Case

{"/DIR/", http.StatusMovedPermanently, "/dir/"}, // Fixed Case

{"/PATH/", http.StatusMovedPermanently, "/path"}, // Fixed Case -/

{"/DIR", http.StatusMovedPermanently, "/dir/"}, // Fixed Case +/

{"/../path", http.StatusMovedPermanently, "/path"}, // Without CleanPath

{"/nope", http.StatusNotFound, ""}, // NotFound

}

for _, tr := range testRoutes {

w := performRequest(router, http.MethodGet, tr.route)

assert.Equal(t, tr.code, w.Code)

if w.Code != http.StatusNotFound {

assert.Equal(t, tr.location, fmt.Sprint(w.Header().Get("Location")))

}

}

// Test custom not found handler

var notFound bool

router.NoRoute(func(c *Context) {

c.AbortWithStatus(http.StatusNotFound)

notFound = true

})

w := performRequest(router, http.MethodGet, "/nope")

assert.Equal(t, http.StatusNotFound, w.Code)

qm012 · 2021-08-30T01:22:33Z

tree.go

@@ -599,7 +599,7 @@ walk: // Outer loop for walking the tree
 		// Nothing found. We can recommend to redirect to the same URL with an
 		// extra trailing slash if a leaf exists for that path
 		value.tsr = path == "/" ||
-			(len(prefix) == len(path)+1 && n.handlers != nil)
+			(len(prefix) == len(path)+1 && path == prefix[:len(prefix) - 1] && n.handlers != nil)


Can I restore the code in red as is?

Yes, I run go test without any problems

citizen233 · 2021-08-30T03:56:02Z

The latest benchmarks:

branch	benchmark	running time
`master`	https://app.travis-ci.com/github/citizen233/go-http-routing-benchmark/builds/236467197	632.890s
`this PR`	https://app.travis-ci.com/github/citizen233/go-http-routing-benchmark/builds/236468715	633.684s

The latest benchstat report:

name              old time/op    new time/op    delta
Gin_Param           60.9ns ± 0%    61.4ns ± 1%   +0.87%  (p=0.000 n=9+9)
Gin_Param5           112ns ± 0%     116ns ± 2%   +3.66%  (p=0.000 n=8+9)
Gin_Param20          301ns ± 0%     305ns ± 1%   +1.60%  (p=0.000 n=8+9)
Gin_ParamWrite       115ns ± 2%     116ns ± 0%     ~     (p=0.115 n=10+8)
Gin_GithubStatic    75.9ns ± 2%    74.7ns ± 2%     ~     (p=0.435 n=10+9)
Gin_GithubParam      131ns ± 2%     132ns ± 2%     ~     (p=0.726 n=10+10)
Gin_GithubAll       27.1µs ± 1%    25.7µs ± 1%   -5.32%  (p=0.000 n=10+10)
Gin_GPlusStatic     57.4ns ± 0%    60.2ns ± 3%   +4.87%  (p=0.000 n=10+10)
Gin_GPlusParam      79.4ns ± 0%    92.9ns ± 4%  +16.95%  (p=0.000 n=10+10)
Gin_GPlus2Params     101ns ± 0%     117ns ± 1%  +15.76%  (p=0.000 n=10+10)
Gin_GPlusAll        1.13µs ± 0%    1.15µs ± 1%   +1.81%  (p=0.000 n=10+9)
Gin_ParseStatic     59.5ns ± 3%    58.0ns ± 2%     ~     (p=0.195 n=10+8)
Gin_ParseParam      68.0ns ± 3%    65.8ns ± 1%     ~     (p=0.076 n=10+9)
Gin_Parse2Params    83.4ns ± 2%    82.1ns ± 1%     ~     (p=0.194 n=10+8)
Gin_ParseAll        1.98µs ± 1%    1.96µs ± 2%     ~     (p=0.137 n=10+10)
Gin_StaticAll       19.2µs ± 1%    18.5µs ± 1%   -3.50%  (p=0.000 n=10+10)

citizen233 · 2021-08-30T04:57:35Z

Hi, @qm012 I modified the code based on the codereview comments and re-tested the benchmark ~

The latest benchmarks:

branch	benchmark	running time
`master`	https://app.travis-ci.com/github/citizen233/go-http-routing-benchmark/builds/236467197	632.890s
`this PR`	https://app.travis-ci.com/github/citizen233/go-http-routing-benchmark/builds/236470803	633.061s

The latest benchstat report:

name              old time/op    new time/op    delta
Gin_Param           60.9ns ± 0%    60.4ns ± 3%     ~     (p=0.151 n=9+10)
Gin_Param5           112ns ± 0%     115ns ± 1%   +2.39%  (p=0.000 n=8+10)
Gin_Param20          301ns ± 0%     303ns ± 1%   +0.86%  (p=0.000 n=8+9)
Gin_ParamWrite       115ns ± 2%     116ns ± 2%     ~     (p=0.085 n=10+10)
Gin_GithubStatic    75.9ns ± 2%    74.2ns ± 1%     ~     (p=0.089 n=10+10)
Gin_GithubParam      131ns ± 2%     128ns ± 0%   -2.24%  (p=0.000 n=10+8)
Gin_GithubAll       27.1µs ± 1%    25.8µs ± 1%   -4.66%  (p=0.000 n=10+10)
Gin_GPlusStatic     57.4ns ± 0%    56.8ns ± 1%   -1.06%  (p=0.000 n=10+8)
Gin_GPlusParam      79.4ns ± 0%    87.7ns ± 0%  +10.39%  (p=0.000 n=10+9)
Gin_GPlus2Params     101ns ± 0%     114ns ± 2%  +12.79%  (p=0.000 n=10+10)
Gin_GPlusAll        1.13µs ± 0%    1.13µs ± 1%   +0.64%  (p=0.023 n=10+10)
Gin_ParseStatic     59.5ns ± 3%    58.0ns ± 2%     ~     (p=0.239 n=10+10)
Gin_ParseParam      68.0ns ± 3%    65.5ns ± 1%   -3.66%  (p=0.000 n=10+9)
Gin_Parse2Params    83.4ns ± 2%    81.3ns ± 0%   -2.51%  (p=0.000 n=10+10)
Gin_ParseAll        1.98µs ± 1%    1.95µs ± 1%   -1.61%  (p=0.000 n=10+10)
Gin_StaticAll       19.2µs ± 1%    18.6µs ± 2%   -2.87%  (p=0.000 n=10+10)

codecov · 2021-08-31T01:44:15Z

Codecov Report

Merging #2847 (eb44623) into master (eb75ce0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2847   +/-   ##
=======================================
  Coverage   98.73%   98.73%           
=======================================
  Files          41       41           
  Lines        3079     3080    +1     
=======================================
+ Hits         3040     3041    +1     
  Misses         27       27           
  Partials       12       12

Flag	Coverage Δ
go-1.13	`98.73% <100.00%> (+<0.01%)`	⬆️
go-1.14	`98.57% <100.00%> (+<0.01%)`	⬆️
go-1.15	`98.57% <100.00%> (+<0.01%)`	⬆️
go-1.16	`98.57% <100.00%> (+<0.01%)`	⬆️
go-1.17	`98.47% <100.00%> (+<0.01%)`	⬆️
macos-latest	`98.73% <100.00%> (+<0.01%)`	⬆️
nomsgpack	`98.71% <100.00%> (+<0.01%)`	⬆️
ubuntu-latest	`98.73% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
tree.go	`100.00% <100.00%> (ø)`

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 eb75ce0...eb44623. Read the comment docs.

citizen233 · 2021-08-31T09:40:39Z

hi @qm012 If you have time, please help. Code review of my pr

citizen233 · 2021-09-03T09:13:38Z

@appleboy @rw-access If you have time, please help. Code review of my pr

Bisstocuz · 2021-10-25T13:40:29Z

Hi, the logic in tree.go has updated in #2897 , please merge from upstream and re-run the tests. 😀

citizen233 · 2021-10-26T03:18:45Z

Hi, @Bisstocuz This is the result of my retest

The latest benchmarks:

branch	benchmark	running time
`master`	https://app.travis-ci.com/github/citizen233/go-http-routing-benchmark/builds/240554803	636.481s
`this PR`	https://app.travis-ci.com/github/citizen233/go-http-routing-benchmark/builds/240554840	634.000s

The latest benchstat report:

name              old time/op    new time/op    delta
Gin_Param           62.2ns ± 5%    61.5ns ± 3%     ~     (p=0.436 n=10+10)
Gin_Param5           114ns ± 2%     119ns ± 0%   +4.17%  (p=0.000 n=10+9)
Gin_Param20          304ns ± 2%     301ns ± 0%   -0.98%  (p=0.003 n=10+9)
Gin_ParamWrite       115ns ± 3%     116ns ± 3%     ~     (p=0.366 n=10+9)
Gin_GithubStatic    76.0ns ± 0%    74.7ns ± 1%   -1.72%  (p=0.000 n=9+9)
Gin_GithubParam      132ns ± 1%     135ns ± 1%   +2.38%  (p=0.000 n=10+10)
Gin_GithubAll       27.5µs ± 1%    25.9µs ± 0%   -5.83%  (p=0.000 n=8+10)
Gin_GPlusStatic     58.6ns ± 3%    61.1ns ± 0%   +4.29%  (p=0.000 n=10+10)
Gin_GPlusParam      79.7ns ± 3%    88.3ns ± 0%  +10.69%  (p=0.000 n=9+10)
Gin_GPlus2Params     103ns ± 0%     115ns ± 0%  +11.46%  (p=0.000 n=9+9)
Gin_GPlusAll        1.15µs ± 1%    1.14µs ± 0%   -0.69%  (p=0.000 n=10+8)
Gin_ParseStatic     59.1ns ± 0%    62.0ns ± 1%   +5.01%  (p=0.000 n=10+10)
Gin_ParseParam      66.5ns ± 0%    69.3ns ± 0%   +4.34%  (p=0.000 n=10+10)
Gin_Parse2Params    81.8ns ± 0%    85.1ns ± 0%   +3.99%  (p=0.000 n=10+10)
Gin_ParseAll        1.95µs ± 1%    1.96µs ± 0%   +0.40%  (p=0.005 n=10+10)
Gin_StaticAll       19.2µs ± 1%    18.9µs ± 0%   -1.51%  (p=0.000 n=10+10)

appleboy

LGTM

appleboy · 2021-10-26T03:38:51Z

@thinkerou help to review.

thinkerou

lgtm

(cherry picked from commit 1c2aa59)

fix the misplacement of adding slashes

fc62633

citizen233 changed the title ~~fix: fix the misplacement of adding slashes~~ fix the misplacement of adding slashes Aug 29, 2021

feat: add fix gin-gonic#2843 test example

d58ba30

appleboy added the bug label Aug 29, 2021

appleboy added this to the v1.7.5 milestone Aug 29, 2021

qm012 reviewed Aug 30, 2021

View reviewed changes

feat: modify the code according to the codereview opinion

86831de

feat: format code

b33f331

daheige approved these changes Sep 6, 2021

View reviewed changes

appleboy requested review from appleboy and thinkerou September 14, 2021 22:38

Bisstocuz mentioned this pull request Oct 16, 2021

Update the code logic for latestNode in tree.go #2897

Merged

Merge branch 'master' of github.com:gin-gonic/gin into fix-tree

eb44623

appleboy approved these changes Oct 26, 2021

View reviewed changes

Bisstocuz mentioned this pull request Oct 26, 2021

Incorrect TSR with mixed static and wildcard paths #2918

Closed

thinkerou approved these changes Oct 26, 2021

View reviewed changes

appleboy merged commit 1c2aa59 into gin-gonic:master Oct 26, 2021

This was referenced Oct 26, 2021

value.tsr cause wrong redirect #2843

Closed

fix tsr with mixed static and wildcard paths #2924

Merged

Bisstocuz mentioned this pull request Nov 19, 2021

update version to v1.7.5 #2943

Closed

thinkerou pushed a commit that referenced this pull request Nov 20, 2021

fix the misplacement of adding slashes (#2847)

3df2d2c

Bisstocuz pushed a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021

fix the misplacement of adding slashes (gin-gonic#2847)

67626a5

(cherry picked from commit 1c2aa59)

Bisstocuz pushed a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021

fix the misplacement of adding slashes (gin-gonic#2847)

08239be

(cherry picked from commit 1c2aa59)

Bisstocuz pushed a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021

fix the misplacement of adding slashes (gin-gonic#2847)

26501ac

(cherry picked from commit 1c2aa59)

thinkerou pushed a commit that referenced this pull request Nov 23, 2021

fix the misplacement of adding slashes (#2847)

f45667b

Bisstocuz pushed a commit to Bisstocuz/gin that referenced this pull request Nov 23, 2021

fix the misplacement of adding slashes (gin-gonic#2847)

46dec28

(cherry picked from commit 1c2aa59)

thinkerou pushed a commit that referenced this pull request Nov 24, 2021

fix the misplacement of adding slashes (#2847)

83dff27

thinkerou pushed a commit that referenced this pull request Nov 24, 2021

fix the misplacement of adding slashes (#2847)

bb945cf

daheige pushed a commit to daheige/gin that referenced this pull request Apr 18, 2022

fix the misplacement of adding slashes (gin-gonic#2847)

bba3517

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix the misplacement of adding slashes #2847

fix the misplacement of adding slashes #2847

citizen233 commented Aug 29, 2021 •

edited

appleboy commented Aug 29, 2021

citizen233 commented Aug 29, 2021

qm012 left a comment •

edited

qm012 Aug 30, 2021

qm012 Aug 30, 2021

citizen233 Aug 30, 2021

citizen233 commented Aug 30, 2021

citizen233 commented Aug 30, 2021

codecov bot commented Aug 31, 2021 •

edited

citizen233 commented Aug 31, 2021

citizen233 commented Sep 3, 2021

Bisstocuz commented Oct 25, 2021

citizen233 commented Oct 26, 2021

appleboy left a comment

appleboy commented Oct 26, 2021

thinkerou left a comment

	func TestRouterNotFound(t *testing.T) {
	router := New()
	router.RedirectFixedPath = true
	router.GET("/path", func(c *Context) {})
	router.GET("/dir/", func(c *Context) {})
	router.GET("/", func(c *Context) {})

	testRoutes := []struct {
	route string
	code int
	location string
	}{
	{"/path/", http.StatusMovedPermanently, "/path"}, // TSR -/
	{"/dir", http.StatusMovedPermanently, "/dir/"}, // TSR +/
	{"/PATH", http.StatusMovedPermanently, "/path"}, // Fixed Case
	{"/DIR/", http.StatusMovedPermanently, "/dir/"}, // Fixed Case
	{"/PATH/", http.StatusMovedPermanently, "/path"}, // Fixed Case -/
	{"/DIR", http.StatusMovedPermanently, "/dir/"}, // Fixed Case +/
	{"/../path", http.StatusMovedPermanently, "/path"}, // Without CleanPath
	{"/nope", http.StatusNotFound, ""}, // NotFound
	}
	for _, tr := range testRoutes {
	w := performRequest(router, http.MethodGet, tr.route)
	assert.Equal(t, tr.code, w.Code)
	if w.Code != http.StatusNotFound {
	assert.Equal(t, tr.location, fmt.Sprint(w.Header().Get("Location")))
	}
	}

	// Test custom not found handler
	var notFound bool
	router.NoRoute(func(c *Context) {
	c.AbortWithStatus(http.StatusNotFound)
	notFound = true
	})
	w := performRequest(router, http.MethodGet, "/nope")
	assert.Equal(t, http.StatusNotFound, w.Code)

fix the misplacement of adding slashes #2847

fix the misplacement of adding slashes #2847

Conversation

citizen233 commented Aug 29, 2021 • edited

appleboy commented Aug 29, 2021

citizen233 commented Aug 29, 2021

qm012 left a comment • edited

Choose a reason for hiding this comment

qm012 Aug 30, 2021

Choose a reason for hiding this comment

qm012 Aug 30, 2021

Choose a reason for hiding this comment

citizen233 Aug 30, 2021

Choose a reason for hiding this comment

citizen233 commented Aug 30, 2021

citizen233 commented Aug 30, 2021

codecov bot commented Aug 31, 2021 • edited

Codecov Report

citizen233 commented Aug 31, 2021

citizen233 commented Sep 3, 2021

Bisstocuz commented Oct 25, 2021

citizen233 commented Oct 26, 2021

appleboy left a comment

Choose a reason for hiding this comment

appleboy commented Oct 26, 2021

thinkerou left a comment

Choose a reason for hiding this comment

citizen233 commented Aug 29, 2021 •

edited

qm012 left a comment •

edited

codecov bot commented Aug 31, 2021 •

edited