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 the misplacement of adding slashes #2847

Merged
merged 5 commits into from Oct 26, 2021
Merged

Conversation

citizen233
Copy link
Contributor

@citizen233 citizen233 commented Aug 29, 2021

fix bug #2843

@citizen233 citizen233 changed the title fix: fix the misplacement of adding slashes fix the misplacement of adding slashes Aug 29, 2021
@appleboy appleboy added the bug label Aug 29, 2021
@appleboy appleboy added this to the v1.7.5 milestone Aug 29, 2021
@appleboy
Copy link
Member

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
Copy link
Contributor Author

How to do benchmark test, please guide me

Copy link
Contributor

@qm012 qm012 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How to do benchmark test, please guide me

Please refer to the #2796 (comment)

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

routes_test.go Outdated
@@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

tree.go Outdated
@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I restore the code in red as is?
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I run go test without any problems

@citizen233
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link

codecov bot commented Aug 31, 2021

Codecov Report

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

Impacted file tree graph

@@           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
Copy link
Contributor Author

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

@citizen233
Copy link
Contributor Author

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

daheige
daheige approved these changes Sep 6, 2021
@Bisstocuz
Copy link
Contributor

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

@citizen233
Copy link
Contributor Author

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)

Copy link
Member

@appleboy appleboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@appleboy
Copy link
Member

@thinkerou help to review.

Copy link
Member

@thinkerou thinkerou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@appleboy appleboy merged commit 1c2aa59 into gin-gonic:master Oct 26, 2021
24 checks passed
Bisstocuz pushed a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021
Bisstocuz pushed a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021
Bisstocuz pushed a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021
Bisstocuz pushed a commit to Bisstocuz/gin that referenced this pull request Nov 23, 2021
daheige pushed a commit to daheige/gin that referenced this pull request Apr 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants