Skip to content

Commit b5ad462

Browse files
zhuxi0511thinkerou
zhuxi0511
authored and
thinkerou
committed
Update the code logic for latestNode in tree.go (#2897)
1 parent 3b555a5 commit b5ad462

File tree

5 files changed

+121
-53
lines changed

5 files changed

+121
-53
lines changed

‎context.go‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,9 @@ type Context struct {
5454
index int8
5555
fullPath string
5656

57-
engine *Engine
58-
params *Params
57+
engine *Engine
58+
params *Params
59+
skippedNodes *[]skippedNode
5960

6061
// This mutex protect Keys map
6162
mu sync.RWMutex
@@ -97,7 +98,8 @@ func (c *Context) reset() {
9798
c.Accepted = nil
9899
c.queryCache = nil
99100
c.formCache = nil
100-
*c.params = (*c.params)[0:0]
101+
*c.params = (*c.params)[:0]
102+
*c.skippedNodes = (*c.skippedNodes)[:0]
101103
}
102104

103105
// Copy returns a copy of the current context that can be safely used outside the request's scope.

‎gin.go‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,7 @@ type Engine struct {
144144
pool sync.Pool
145145
trees methodTrees
146146
maxParams uint16
147+
maxSections uint16
147148
trustedProxies []string
148149
trustedCIDRs []*net.IPNet
149150
}
@@ -200,7 +201,8 @@ func Default() *Engine {
200201

201202
func (engine *Engine) allocateContext() *Context {
202203
v := make(Params, 0, engine.maxParams)
203-
return &Context{engine: engine, params: &v}
204+
skippedNodes := make([]skippedNode, 0, engine.maxSections)
205+
return &Context{engine: engine, params: &v, skippedNodes: &skippedNodes}
204206
}
205207

206208
// Delims sets template left and right delims and returns a Engine instance.
@@ -306,6 +308,10 @@ func (engine *Engine) addRoute(method, path string, handlers HandlersChain) {
306308
if paramsCount := countParams(path); paramsCount > engine.maxParams {
307309
engine.maxParams = paramsCount
308310
}
311+
312+
if sectionsCount := countSections(path); sectionsCount > engine.maxSections {
313+
engine.maxSections = sectionsCount
314+
}
309315
}
310316

311317
// Routes returns a slice of registered routes, including some useful information, such as:
@@ -539,7 +545,7 @@ func (engine *Engine) handleHTTPRequest(c *Context) {
539545
}
540546
root := t[i].root
541547
// Find route in tree
542-
value := root.getValue(rPath, c.params, unescape)
548+
value := root.getValue(rPath, c.params, c.skippedNodes, unescape)
543549
if value.params != nil {
544550
c.Params = *value.params
545551
}
@@ -567,7 +573,7 @@ func (engine *Engine) handleHTTPRequest(c *Context) {
567573
if tree.method == httpMethod {
568574
continue
569575
}
570-
if value := tree.root.getValue(rPath, nil, unescape); value.handlers != nil {
576+
if value := tree.root.getValue(rPath, nil, c.skippedNodes, unescape); value.handlers != nil {
571577
c.handlers = engine.allNoMethod
572578
serveError(c, http.StatusMethodNotAllowed, default405Body)
573579
return

‎gin_integration_test.go‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -408,8 +408,13 @@ func TestTreeRunDynamicRouting(t *testing.T) {
408408
router.GET("/ab/*xx", func(c *Context) { c.String(http.StatusOK, "/ab/*xx") })
409409
router.GET("/", func(c *Context) { c.String(http.StatusOK, "home") })
410410
router.GET("/:cc", func(c *Context) { c.String(http.StatusOK, "/:cc") })
411+
router.GET("/c1/:dd/e", func(c *Context) { c.String(http.StatusOK, "/c1/:dd/e") })
412+
router.GET("/c1/:dd/e1", func(c *Context) { c.String(http.StatusOK, "/c1/:dd/e1") })
413+
router.GET("/c1/:dd/f1", func(c *Context) { c.String(http.StatusOK, "/c1/:dd/f1") })
414+
router.GET("/c1/:dd/f2", func(c *Context) { c.String(http.StatusOK, "/c1/:dd/f2") })
411415
router.GET("/:cc/cc", func(c *Context) { c.String(http.StatusOK, "/:cc/cc") })
412416
router.GET("/:cc/:dd/ee", func(c *Context) { c.String(http.StatusOK, "/:cc/:dd/ee") })
417+
router.GET("/:cc/:dd/f", func(c *Context) { c.String(http.StatusOK, "/:cc/:dd/f") })
413418
router.GET("/:cc/:dd/:ee/ff", func(c *Context) { c.String(http.StatusOK, "/:cc/:dd/:ee/ff") })
414419
router.GET("/:cc/:dd/:ee/:ff/gg", func(c *Context) { c.String(http.StatusOK, "/:cc/:dd/:ee/:ff/gg") })
415420
router.GET("/:cc/:dd/:ee/:ff/:gg/hh", func(c *Context) { c.String(http.StatusOK, "/:cc/:dd/:ee/:ff/:gg/hh") })
@@ -446,6 +451,10 @@ func TestTreeRunDynamicRouting(t *testing.T) {
446451
testRequest(t, ts.URL+"/all", "", "/:cc")
447452
testRequest(t, ts.URL+"/all/cc", "", "/:cc/cc")
448453
testRequest(t, ts.URL+"/a/cc", "", "/:cc/cc")
454+
testRequest(t, ts.URL+"/c1/d/e", "", "/c1/:dd/e")
455+
testRequest(t, ts.URL+"/c1/d/e1", "", "/c1/:dd/e1")
456+
testRequest(t, ts.URL+"/c1/d/ee", "", "/:cc/:dd/ee")
457+
testRequest(t, ts.URL+"/c1/d/f", "", "/:cc/:dd/f")
449458
testRequest(t, ts.URL+"/c/d/ee", "", "/:cc/:dd/ee")
450459
testRequest(t, ts.URL+"/c/d/e/ff", "", "/:cc/:dd/:ee/ff")
451460
testRequest(t, ts.URL+"/c/d/e/f/gg", "", "/:cc/:dd/:ee/:ff/gg")
@@ -528,6 +537,12 @@ func TestTreeRunDynamicRouting(t *testing.T) {
528537
testRequest(t, ts.URL+"/get/abc/123abf/testss", "", "/get/abc/123abf/:param")
529538
testRequest(t, ts.URL+"/get/abc/123abfff/te", "", "/get/abc/123abfff/:param")
530539
// 404 not found
540+
testRequest(t, ts.URL+"/c/d/e", "404 Not Found")
541+
testRequest(t, ts.URL+"/c/d/e1", "404 Not Found")
542+
testRequest(t, ts.URL+"/c/d/eee", "404 Not Found")
543+
testRequest(t, ts.URL+"/c1/d/eee", "404 Not Found")
544+
testRequest(t, ts.URL+"/c1/d/e2", "404 Not Found")
545+
testRequest(t, ts.URL+"/cc/dd/ee/ff/gg/hh1", "404 Not Found")
531546
testRequest(t, ts.URL+"/a/dd", "404 Not Found")
532547
testRequest(t, ts.URL+"/addr/dd/aa", "404 Not Found")
533548
testRequest(t, ts.URL+"/something/secondthing/121", "404 Not Found")

‎tree.go‎

Lines changed: 76 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import (
1717
var (
1818
strColon = []byte(":")
1919
strStar = []byte("*")
20+
strSlash = []byte("/")
2021
)
2122

2223
// Param is a single URL parameter, consisting of a key and a value.
@@ -98,6 +99,11 @@ func countParams(path string) uint16 {
9899
return n
99100
}
100101

102+
func countSections(path string) uint16 {
103+
s := bytesconv.StringToBytes(path)
104+
return uint16(bytes.Count(s, strSlash))
105+
}
106+
101107
type nodeType uint8
102108

103109
const (
@@ -394,16 +400,19 @@ type nodeValue struct {
394400
fullPath string
395401
}
396402

403+
type skippedNode struct {
404+
path string
405+
node *node
406+
paramsCount int16
407+
}
408+
397409
// Returns the handle registered with the given path (key). The values of
398410
// wildcards are saved to a map.
399411
// If no handle can be found, a TSR (trailing slash redirect) recommendation is
400412
// made if a handle exists with an extra (without the) trailing slash for the
401413
// given path.
402-
func (n *node) getValue(path string, params *Params, unescape bool) (value nodeValue) {
403-
var (
404-
skippedPath string
405-
latestNode = n // Caching the latest node
406-
)
414+
func (n *node) getValue(path string, params *Params, skippedNodes *[]skippedNode, unescape bool) (value nodeValue) {
415+
var globalParamsCount int16
407416

408417
walk: // Outer loop for walking the tree
409418
for {
@@ -418,15 +427,20 @@ walk: // Outer loop for walking the tree
418427
if c == idxc {
419428
// strings.HasPrefix(n.children[len(n.children)-1].path, ":") == n.wildChild
420429
if n.wildChild {
421-
skippedPath = prefix + path
422-
latestNode = &node{
423-
path: n.path,
424-
wildChild: n.wildChild,
425-
nType: n.nType,
426-
priority: n.priority,
427-
children: n.children,
428-
handlers: n.handlers,
429-
fullPath: n.fullPath,
430+
index := len(*skippedNodes)
431+
*skippedNodes = (*skippedNodes)[:index+1]
432+
(*skippedNodes)[index] = skippedNode{
433+
path: prefix + path,
434+
node: &node{
435+
path: n.path,
436+
wildChild: n.wildChild,
437+
nType: n.nType,
438+
priority: n.priority,
439+
children: n.children,
440+
handlers: n.handlers,
441+
fullPath: n.fullPath,
442+
},
443+
paramsCount: globalParamsCount,
430444
}
431445
}
432446

@@ -435,10 +449,22 @@ walk: // Outer loop for walking the tree
435449
}
436450
}
437451
// If the path at the end of the loop is not equal to '/' and the current node has no child nodes
438-
// the current node needs to be equal to the latest matching node
439-
matched := path != "/" && !n.wildChild
440-
if matched {
441-
n = latestNode
452+
// the current node needs to roll back to last vaild skippedNode
453+
454+
if path != "/" && !n.wildChild {
455+
for l := len(*skippedNodes); l > 0; {
456+
skippedNode := (*skippedNodes)[l-1]
457+
*skippedNodes = (*skippedNodes)[:l-1]
458+
if strings.HasSuffix(skippedNode.path, path) {
459+
path = skippedNode.path
460+
n = skippedNode.node
461+
if value.params != nil {
462+
*value.params = (*value.params)[:skippedNode.paramsCount]
463+
}
464+
globalParamsCount = skippedNode.paramsCount
465+
continue walk
466+
}
467+
}
442468
}
443469

444470
// If there is no wildcard pattern, recommend a redirection
@@ -452,18 +478,12 @@ walk: // Outer loop for walking the tree
452478

453479
// Handle wildcard child, which is always at the end of the array
454480
n = n.children[len(n.children)-1]
481+
globalParamsCount++
455482

456483
switch n.nType {
457484
case param:
458485
// fix truncate the parameter
459486
// tree_test.go line: 204
460-
if matched {
461-
path = prefix + path
462-
// The saved path is used after the prefix route is intercepted by matching
463-
if n.indices == "/" {
464-
path = skippedPath[1:]
465-
}
466-
}
467487

468488
// Find param end (either '/' or path end)
469489
end := 0
@@ -549,9 +569,22 @@ walk: // Outer loop for walking the tree
549569

550570
if path == prefix {
551571
// If the current path does not equal '/' and the node does not have a registered handle and the most recently matched node has a child node
552-
// the current node needs to be equal to the latest matching node
553-
if latestNode.wildChild && n.handlers == nil && path != "/" {
554-
n = latestNode.children[len(latestNode.children)-1]
572+
// the current node needs to roll back to last vaild skippedNode
573+
if n.handlers == nil && path != "/" {
574+
for l := len(*skippedNodes); l > 0; {
575+
skippedNode := (*skippedNodes)[l-1]
576+
*skippedNodes = (*skippedNodes)[:l-1]
577+
if strings.HasSuffix(skippedNode.path, path) {
578+
path = skippedNode.path
579+
n = skippedNode.node
580+
if value.params != nil {
581+
*value.params = (*value.params)[:skippedNode.paramsCount]
582+
}
583+
globalParamsCount = skippedNode.paramsCount
584+
continue walk
585+
}
586+
}
587+
// n = latestNode.children[len(latestNode.children)-1]
555588
}
556589
// We should have reached the node containing the handle.
557590
// Check if this node has a handle registered.
@@ -582,19 +615,21 @@ walk: // Outer loop for walking the tree
582615
return
583616
}
584617

585-
if path != "/" && len(skippedPath) > 0 && strings.HasSuffix(skippedPath, path) {
586-
path = skippedPath
587-
// Reduce the number of cycles
588-
n, latestNode = latestNode, n
589-
// skippedPath cannot execute
590-
// example:
591-
// * /:cc/cc
592-
// call /a/cc expectations:match/200 Actual:match/200
593-
// call /a/dd expectations:unmatch/404 Actual: panic
594-
// call /addr/dd/aa expectations:unmatch/404 Actual: panic
595-
// skippedPath: It can only be executed if the secondary route is not found
596-
skippedPath = ""
597-
continue walk
618+
// roll back to last vaild skippedNode
619+
if path != "/" {
620+
for l := len(*skippedNodes); l > 0; {
621+
skippedNode := (*skippedNodes)[l-1]
622+
*skippedNodes = (*skippedNodes)[:l-1]
623+
if strings.HasSuffix(skippedNode.path, path) {
624+
path = skippedNode.path
625+
n = skippedNode.node
626+
if value.params != nil {
627+
*value.params = (*value.params)[:skippedNode.paramsCount]
628+
}
629+
globalParamsCount = skippedNode.paramsCount
630+
continue walk
631+
}
632+
}
598633
}
599634

600635
// Nothing found. We can recommend to redirect to the same URL with an

‎tree_test.go‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -33,14 +33,19 @@ func getParams() *Params {
3333
return &ps
3434
}
3535

36+
func getSkippedNodes() *[]skippedNode {
37+
ps := make([]skippedNode, 0, 20)
38+
return &ps
39+
}
40+
3641
func checkRequests(t *testing.T, tree *node, requests testRequests, unescapes ...bool) {
3742
unescape := false
3843
if len(unescapes) >= 1 {
3944
unescape = unescapes[0]
4045
}
4146

4247
for _, request := range requests {
43-
value := tree.getValue(request.path, getParams(), unescape)
48+
value := tree.getValue(request.path, getParams(), getSkippedNodes(), unescape)
4449

4550
if value.handlers == nil {
4651
if !request.nilHandler {
@@ -157,6 +162,8 @@ func TestTreeWildcard(t *testing.T) {
157162
"/aa/*xx",
158163
"/ab/*xx",
159164
"/:cc",
165+
"/c1/:dd/e",
166+
"/c1/:dd/e1",
160167
"/:cc/cc",
161168
"/:cc/:dd/ee",
162169
"/:cc/:dd/:ee/ff",
@@ -238,6 +245,9 @@ func TestTreeWildcard(t *testing.T) {
238245
{"/alldd", false, "/:cc", Params{Param{Key: "cc", Value: "alldd"}}},
239246
{"/all/cc", false, "/:cc/cc", Params{Param{Key: "cc", Value: "all"}}},
240247
{"/a/cc", false, "/:cc/cc", Params{Param{Key: "cc", Value: "a"}}},
248+
{"/c1/d/e", false, "/c1/:dd/e", Params{Param{Key: "dd", Value: "d"}}},
249+
{"/c1/d/e1", false, "/c1/:dd/e1", Params{Param{Key: "dd", Value: "d"}}},
250+
{"/c1/d/ee", false, "/:cc/:dd/ee", Params{Param{Key: "cc", Value: "c1"}, Param{Key: "dd", Value: "d"}}},
241251
{"/cc/cc", false, "/:cc/cc", Params{Param{Key: "cc", Value: "cc"}}},
242252
{"/ccc/cc", false, "/:cc/cc", Params{Param{Key: "cc", Value: "ccc"}}},
243253
{"/deedwjfs/cc", false, "/:cc/cc", Params{Param{Key: "cc", Value: "deedwjfs"}}},
@@ -605,7 +615,7 @@ func TestTreeTrailingSlashRedirect(t *testing.T) {
605615
"/doc/",
606616
}
607617
for _, route := range tsrRoutes {
608-
value := tree.getValue(route, nil, false)
618+
value := tree.getValue(route, nil, getSkippedNodes(), false)
609619
if value.handlers != nil {
610620
t.Fatalf("non-nil handler for TSR route '%s", route)
611621
} else if !value.tsr {
@@ -622,7 +632,7 @@ func TestTreeTrailingSlashRedirect(t *testing.T) {
622632
"/api/world/abc",
623633
}
624634
for _, route := range noTsrRoutes {
625-
value := tree.getValue(route, nil, false)
635+
value := tree.getValue(route, nil, getSkippedNodes(), false)
626636
if value.handlers != nil {
627637
t.Fatalf("non-nil handler for No-TSR route '%s", route)
628638
} else if value.tsr {
@@ -641,7 +651,7 @@ func TestTreeRootTrailingSlashRedirect(t *testing.T) {
641651
t.Fatalf("panic inserting test route: %v", recv)
642652
}
643653

644-
value := tree.getValue("/", nil, false)
654+
value := tree.getValue("/", nil, getSkippedNodes(), false)
645655
if value.handlers != nil {
646656
t.Fatalf("non-nil handler")
647657
} else if value.tsr {
@@ -821,7 +831,7 @@ func TestTreeInvalidNodeType(t *testing.T) {
821831

822832
// normal lookup
823833
recv := catchPanic(func() {
824-
tree.getValue("/test", nil, false)
834+
tree.getValue("/test", nil, getSkippedNodes(), false)
825835
})
826836
if rs, ok := recv.(string); !ok || rs != panicMsg {
827837
t.Fatalf("Expected panic '"+panicMsg+"', got '%v'", recv)
@@ -846,7 +856,7 @@ func TestTreeInvalidParamsType(t *testing.T) {
846856
params := make(Params, 0, 0)
847857

848858
// try to trigger slice bounds out of range with capacity 0
849-
tree.getValue("/test", &params, false)
859+
tree.getValue("/test", &params, getSkippedNodes(), false)
850860
}
851861

852862
func TestTreeWildcardConflictEx(t *testing.T) {

0 commit comments

Comments
 (0)