From f1b86cc4fdb0e0e689a81e1fdb13d90de28e250b Mon Sep 17 00:00:00 2001 From: Bo-Yi Wu Date: Sat, 9 May 2020 19:57:08 +0800 Subject: [PATCH] chore: refactor --- gin.go | 33 +------- tree.go | 222 ++++++--------------------------------------------- tree_test.go | 68 ++++++++-------- 3 files changed, 63 insertions(+), 260 deletions(-) diff --git a/gin.go b/gin.go index 159646a4ec..7175061c7a 100644 --- a/gin.go +++ b/gin.go @@ -113,8 +113,7 @@ type Engine struct { noMethod HandlersChain pool sync.Pool trees methodTrees - // paramsPool sync.Pool - maxParams uint16 + maxParams uint16 } var _ IRouter = &Engine{} @@ -254,7 +253,6 @@ func (engine *Engine) rebuild405Handlers() { } func (engine *Engine) addRoute(method, path string, handlers HandlersChain) { - // log.Println(method, path) assert1(path[0] == '/', "path must begin with '/'") assert1(method != "", "HTTP method can not be empty") assert1(len(handlers) > 0, "there must be at least one handler") @@ -271,19 +269,9 @@ func (engine *Engine) addRoute(method, path string, handlers HandlersChain) { root.addRoute(path, handlers) // Update maxParams - // log.Println("countParams(path):", countParams(path)) if paramsCount := countParams(path); paramsCount+varsCount > engine.maxParams { engine.maxParams = paramsCount + varsCount } - - // Lazy-init paramsPool alloc func - // if engine.paramsPool.New == nil && engine.maxParams > 0 { - // engine.paramsPool.New = func() interface{} { - // // log.Println("engine.maxParams:", engine.maxParams) - // ps := make(Params, 0, engine.maxParams) - // return &ps - // } - // } } // Routes returns a slice of registered routes, including some useful information, such as: @@ -402,21 +390,6 @@ func (engine *Engine) HandleContext(c *Context) { c.index = oldIndexValue } -// func (engine *Engine) getParams() *Params { -// ps := engine.paramsPool.Get().(*Params) -// *ps = (*ps)[0:0] // reset slice -// return ps -// } - -// func (engine *Engine) putParams(ps *Params) { -// c := engine.pool.Get().(*Context) -// c.Params = *ps -// engine.pool.Put(c) -// if ps != nil { -// engine.paramsPool.Put(ps) -// } -// } - func (engine *Engine) handleHTTPRequest(c *Context) { httpMethod := c.Request.Method rPath := c.Request.URL.Path @@ -440,14 +413,10 @@ func (engine *Engine) handleHTTPRequest(c *Context) { // Find route in tree value := root.getValue(rPath, c.params, unescape) if value.params != nil { - // engine.putParams(value.params) c.Params = *value.params } if value.handlers != nil { c.handlers = value.handlers - // if value.params != nil { - // c.Params = *value.params - // } c.fullPath = value.fullPath c.Next() c.writermem.WriteHeaderNow() diff --git a/tree.go b/tree.go index 0a6ad013d4..8f33390c33 100644 --- a/tree.go +++ b/tree.go @@ -95,16 +95,15 @@ const ( type node struct { path string indices string + wildChild bool + nType nodeType + priority uint32 children []*node handlers HandlersChain - priority uint32 - nType nodeType - maxParams uint16 - wildChild bool fullPath string } -// increments priority of the given child and reorders if necessary. +// Increments priority of the given child and reorders if necessary func (n *node) incrementChildPrio(pos int) int { cs := n.children cs[pos].priority++ @@ -115,13 +114,14 @@ func (n *node) incrementChildPrio(pos int) int { for ; newPos > 0 && cs[newPos-1].priority < prio; newPos-- { // Swap node positions cs[newPos-1], cs[newPos] = cs[newPos], cs[newPos-1] + } - // build new index char string + // Build new index char string if newPos != pos { - n.indices = n.indices[:newPos] + // unchanged prefix, might be empty - n.indices[pos:pos+1] + // the index char we move - n.indices[newPos:pos] + n.indices[pos+1:] // rest without char at 'pos' + n.indices = n.indices[:newPos] + // Unchanged prefix, might be empty + n.indices[pos:pos+1] + // The index char we move + n.indices[newPos:pos] + n.indices[pos+1:] // Rest without char at 'pos' } return newPos @@ -132,7 +132,6 @@ func (n *node) incrementChildPrio(pos int) int { func (n *node) addRoute(path string, handlers HandlersChain) { fullPath := path n.priority++ - // numParams := countParams(path) // Empty tree if len(n.path) == 0 && len(n.children) == 0 { @@ -145,11 +144,6 @@ func (n *node) addRoute(path string, handlers HandlersChain) { walk: for { - // Update maxParams of the current node - // if numParams > n.maxParams { - // n.maxParams = numParams - // } - // Find the longest common prefix. // This also implies that the common prefix contains no ':' or '*' // since the existing key can't contain those chars. @@ -167,13 +161,6 @@ walk: fullPath: n.fullPath, } - // Update maxParams (max of all children) - // for _, v := range child.children { - // if v.maxParams > child.maxParams { - // child.maxParams = v.maxParams - // } - // } - n.children = []*node{&child} // []byte for proper unicode char conversion, see #65 n.indices = string([]byte{n.path[i]}) @@ -192,18 +179,13 @@ walk: n = n.children[0] n.priority++ - // Update maxParams of the child node - // if numParams > n.maxParams { - // n.maxParams = numParams - // } - // numParams-- - // Check if the wildcard matches - if len(path) >= len(n.path) && n.path == path[:len(n.path)] { - // check for longer wildcard, e.g. :name and :names - if len(n.path) >= len(path) || path[len(n.path)] == '/' { - continue walk - } + if len(path) >= len(n.path) && n.path == path[:len(n.path)] && + // Adding a child to a catchAll is not possible + n.nType != catchAll && + // Check for longer wildcard, e.g. :name and :names + (len(n.path) >= len(path) || path[len(n.path)] == '/') { + continue walk } pathSeg := path @@ -243,7 +225,6 @@ walk: // []byte for proper unicode char conversion, see #65 n.indices += string([]byte{c}) child := &node{ - // maxParams: numParams, fullPath: fullPath, } n.children = append(n.children, child) @@ -323,9 +304,8 @@ func (n *node) insertChild(path string, fullPath string, handlers HandlersChain) n.wildChild = true child := &node{ - nType: param, - path: wildcard, - // maxParams: numParams, + nType: param, + path: wildcard, fullPath: fullPath, } n.children = []*node{child} @@ -339,7 +319,6 @@ func (n *node) insertChild(path string, fullPath string, handlers HandlersChain) path = path[len(wildcard):] child := &node{ - // maxParams: numParams, priority: 1, fullPath: fullPath, } @@ -374,13 +353,9 @@ func (n *node) insertChild(path string, fullPath string, handlers HandlersChain) child := &node{ wildChild: true, nType: catchAll, - // maxParams: 1, - fullPath: fullPath, + fullPath: fullPath, } - // update maxParams of the parent node - // if n.maxParams < 1 { - // n.maxParams = 1 - // } + n.children = []*node{child} n.indices = string('/') n = child @@ -392,7 +367,6 @@ func (n *node) insertChild(path string, fullPath string, handlers HandlersChain) nType: catchAll, handlers: handlers, priority: 1, - // maxParams: 1, fullPath: fullPath, } n.children = []*node{child} @@ -414,13 +388,12 @@ type nodeValue struct { fullPath string } -// getValue returns the handle registered with the given path (key). The values of +// Returns the handle registered with the given path (key). The values of // wildcards are saved to a map. // If no handle can be found, a TSR (trailing slash redirect) recommendation is // made if a handle exists with an extra (without the) trailing slash for the // given path. func (n *node) getValue(path string, params *Params, unescape bool) (value nodeValue) { - value = nodeValue{} walk: // Outer loop for walking the tree for { prefix := n.path @@ -446,32 +419,17 @@ walk: // Outer loop for walking the tree return } - // handle wildcard child + // Handle wildcard child n = n.children[0] switch n.nType { case param: - // find param end (either '/' or path end) + // Find param end (either '/' or path end) end := 0 for end < len(path) && path[end] != '/' { end++ } - // save param value - // if cap(value.params) < int(n.maxParams) { - // value.params = make(Params, 0, n.maxParams) - // } - // i := len(value.params) - // value.params = value.params[:i+1] // expand slice within preallocated capacity - // value.params[i].Key = n.path[1:] - // val := path[:end] - // if unescape { - // var err error - // if value.params[i].Value, err = url.QueryUnescape(val); err != nil { - // value.params[i].Value = val // fallback, in case of error - // } - // } else { - // value.params[i].Value = val - // } + // Save param value if params != nil { if value.params == nil { value.params = params @@ -517,22 +475,6 @@ walk: // Outer loop for walking the tree return case catchAll: - // // save param value - // if cap(value.params) < int(n.maxParams) { - // value.params = make(Params, 0, n.maxParams) - // } - // i := len(value.params) - // value.params = value.params[:i+1] // expand slice within preallocated capacity - // value.params[i].Key = n.path[2:] - // if unescape { - // var err error - // if value.params[i].Value, err = url.QueryUnescape(path); err != nil { - // value.params[i].Value = path // fallback, in case of error - // } - // } else { - // value.params[i].Value = path - // } - // Save param value if params != nil { if value.params == nil { @@ -561,7 +503,9 @@ walk: // Outer loop for walking the tree panic("invalid node type") } } - } else if path == prefix { + } + + if path == prefix { // We should have reached the node containing the handle. // Check if this node has a handle registered. if value.handlers = n.handlers; value.handlers != nil { @@ -622,120 +566,6 @@ func (n *node) findCaseInsensitivePath(path string, fixTrailingSlash bool) ([]by ) return ciPath, ciPath != nil - // ciPath = make([]byte, 0, len(path)+1) // preallocate enough memory - - // // Outer loop for walking the tree - // for len(path) >= len(n.path) && strings.EqualFold(path[:len(n.path)], n.path) { - // path = path[len(n.path):] - // ciPath = append(ciPath, n.path...) - - // if len(path) == 0 { - // // We should have reached the node containing the handle. - // // Check if this node has a handle registered. - // if n.handlers != nil { - // return ciPath, true - // } - - // // No handle found. - // // Try to fix the path by adding a trailing slash - // if fixTrailingSlash { - // for i := 0; i < len(n.indices); i++ { - // if n.indices[i] == '/' { - // n = n.children[i] - // if (len(n.path) == 1 && n.handlers != nil) || - // (n.nType == catchAll && n.children[0].handlers != nil) { - // return append(ciPath, '/'), true - // } - // return - // } - // } - // } - // return - // } - - // // If this node does not have a wildcard (param or catchAll) child, - // // we can just look up the next child node and continue to walk down - // // the tree - // if !n.wildChild { - // r := unicode.ToLower(rune(path[0])) - // for i, index := range n.indices { - // // must use recursive approach since both index and - // // ToLower(index) could exist. We must check both. - // if r == unicode.ToLower(index) { - // out, found := n.children[i].findCaseInsensitivePath(path, fixTrailingSlash) - // if found { - // return append(ciPath, out...), true - // } - // } - // } - - // // Nothing found. We can recommend to redirect to the same URL - // // without a trailing slash if a leaf exists for that path - // found = fixTrailingSlash && path == "/" && n.handlers != nil - // return - // } - - // n = n.children[0] - // switch n.nType { - // case param: - // // Find param end (either '/' or path end) - // end := 0 - // for end < len(path) && path[end] != '/' { - // end++ - // } - - // // add param value to case insensitive path - // ciPath = append(ciPath, path[:end]...) - - // // we need to go deeper! - // if end < len(path) { - // if len(n.children) > 0 { - // path = path[end:] - // n = n.children[0] - // continue - // } - - // // ... but we can't - // if fixTrailingSlash && len(path) == end+1 { - // return ciPath, true - // } - // return - // } - - // if n.handlers != nil { - // return ciPath, true - // } - // if fixTrailingSlash && len(n.children) == 1 { - // // No handle found. Check if a handle for this path + a - // // trailing slash exists - // n = n.children[0] - // if n.path == "/" && n.handlers != nil { - // return append(ciPath, '/'), true - // } - // } - // return - - // case catchAll: - // return append(ciPath, path...), true - - // default: - // panic("invalid node type") - // } - // } - - // // Nothing found. - // // Try to fix the path by adding / removing a trailing slash - // if fixTrailingSlash { - // if path == "/" { - // return ciPath, true - // } - // if len(path)+1 == len(n.path) && n.path[len(path)] == '/' && - // strings.EqualFold(path, n.path[:len(path)]) && - // n.handlers != nil { - // return append(ciPath, n.path...), true - // } - // } - // return } // Shift bytes in array by n bytes left diff --git a/tree_test.go b/tree_test.go index a1bd223986..1cb4f5598d 100644 --- a/tree_test.go +++ b/tree_test.go @@ -84,35 +84,13 @@ func checkPriorities(t *testing.T, n *node) uint32 { return prio } -// func checkMaxParams(t *testing.T, n *node) uint16 { -// var maxParams uint16 -// for i := range n.children { -// params := checkMaxParams(t, n.children[i]) -// if params > maxParams { -// maxParams = params -// } -// } -// if n.nType > root && !n.wildChild { -// maxParams++ -// } - -// if n.maxParams != maxParams { -// t.Errorf( -// "maxParams mismatch for node '%s': is %d, should be %d", -// n.path, n.maxParams, maxParams, -// ) -// } - -// return maxParams -// } - func TestCountParams(t *testing.T) { - if countParams("/path/:param1/static/*catch-all") != uint16(2) { + if countParams("/path/:param1/static/*catch-all") != 2 { + t.Fail() + } + if countParams(strings.Repeat("/:param", 256)) != 256 { t.Fail() } - // if countParams(strings.Repeat("/:param", 256)) != 255 { - // t.Fail() - // } } func TestTreeAddAndGet(t *testing.T) { @@ -150,7 +128,6 @@ func TestTreeAddAndGet(t *testing.T) { }) checkPriorities(t, tree) - // checkMaxParams(t, tree) } func TestTreeWildcard(t *testing.T) { @@ -194,7 +171,6 @@ func TestTreeWildcard(t *testing.T) { }) checkPriorities(t, tree) - // checkMaxParams(t, tree) } func TestUnescapeParameters(t *testing.T) { @@ -232,7 +208,6 @@ func TestUnescapeParameters(t *testing.T) { }, unescape) checkPriorities(t, tree) - // checkMaxParams(t, tree) } func catchPanic(testFunc func()) (recv interface{}) { @@ -366,6 +341,8 @@ func TestTreeCatchAllConflict(t *testing.T) { {"/src/*filepath/x", true}, {"/src2/", false}, {"/src2/*filepath/x", true}, + {"/src3/*filepath", false}, + {"/src3/*filepath/x", true}, } testRoutes(t, routes) } @@ -382,7 +359,6 @@ func TestTreeCatchMaxParams(t *testing.T) { tree := &node{} var route = "/cmd/*filepath" tree.addRoute(route, fakeHandler(route)) - // checkMaxParams(t, tree) } func TestTreeDoubleWildcard(t *testing.T) { @@ -518,6 +494,9 @@ func TestTreeRootTrailingSlashRedirect(t *testing.T) { func TestTreeFindCaseInsensitivePath(t *testing.T) { tree := &node{} + longPath := "/l" + strings.Repeat("o", 128) + "ng" + lOngPath := "/l" + strings.Repeat("O", 128) + "ng/" + routes := [...]string{ "/hi", "/b/", @@ -541,6 +520,17 @@ func TestTreeFindCaseInsensitivePath(t *testing.T) { "/doc/go/away", "/no/a", "/no/b", + "/Π", + "/u/apfêl/", + "/u/äpfêl/", + "/u/öpfêl", + "/v/Äpfêl/", + "/v/Öpfêl", + "/w/♬", // 3 byte + "/w/♭/", // 3 byte, last byte differs + "/w/𠜎", // 4 byte + "/w/𠜏/", // 4 byte + longPath, } for _, route := range routes { @@ -619,6 +609,21 @@ func TestTreeFindCaseInsensitivePath(t *testing.T) { {"/DOC/", "/doc", true, true}, {"/NO", "", false, true}, {"/DOC/GO", "", false, true}, + {"/π", "/Π", true, false}, + {"/π/", "/Π", true, true}, + {"/u/ÄPFÊL/", "/u/äpfêl/", true, false}, + {"/u/ÄPFÊL", "/u/äpfêl/", true, true}, + {"/u/ÖPFÊL/", "/u/öpfêl", true, true}, + {"/u/ÖPFÊL", "/u/öpfêl", true, false}, + {"/v/äpfêL/", "/v/Äpfêl/", true, false}, + {"/v/äpfêL", "/v/Äpfêl/", true, true}, + {"/v/öpfêL/", "/v/Öpfêl", true, true}, + {"/v/öpfêL", "/v/Öpfêl", true, false}, + {"/w/♬/", "/w/♬", true, true}, + {"/w/♭", "/w/♭/", true, true}, + {"/w/𠜎/", "/w/𠜎", true, true}, + {"/w/𠜏", "/w/𠜏/", true, true}, + {lOngPath, longPath, true, true}, } // With fixTrailingSlash = true for _, test := range tests { @@ -706,8 +711,7 @@ func TestTreeWildcardConflictEx(t *testing.T) { tree.addRoute(conflict.route, fakeHandler(conflict.route)) }) - if !regexp.MustCompile(fmt.Sprintf("'%s' in new path .* conflicts with existing wildcard '%s' in existing prefix '%s'", - conflict.segPath, conflict.existSegPath, conflict.existPath)).MatchString(fmt.Sprint(recv)) { + if !regexp.MustCompile(fmt.Sprintf("'%s' in new path .* conflicts with existing wildcard '%s' in existing prefix '%s'", conflict.segPath, conflict.existSegPath, conflict.existPath)).MatchString(fmt.Sprint(recv)) { t.Fatalf("invalid wildcard conflict error (%v)", recv) } }