Refactor router for readability (#1796)

* refactor router tests to table driven (this way it is easier to debug test cases with breakpoints)
* refactor router variables to be more readable

router.go

@@ -17,12 +17,12 @@ type (
 		label          byte
 		prefix         string
 		parent         *node
-		staticChildrens children
+		staticChildren children
 		ppath          string
 		pnames         []string
 		methodHandler  *methodHandler
-		paramChildren   *node
-		anyChildren     *node
+		paramChild     *node
+		anyChild       *node
 	}
 	kind          uint8
 	children      []*node
@@ -42,9 +42,9 @@ type (
 )
 
 const (
-	skind kind = iota
-	pkind
-	akind
+	staticKind kind = iota
+	paramKind
+	anyKind
 
 	paramLabel = byte(':')
 	anyLabel   = byte('*')
@@ -73,137 +73,147 @@ func (r *Router) Add(method, path string, h HandlerFunc) {
 	pnames := []string{} // Param names
 	ppath := path        // Pristine path
 
-	for i, l := 0, len(path); i < l; i++ {
+	for i, lcpIndex := 0, len(path); i < lcpIndex; i++ {
 		if path[i] == ':' {
 			j := i + 1
 
-			r.insert(method, path[:i], nil, skind, "", nil)
-			for ; i < l && path[i] != '/'; i++ {
+			r.insert(method, path[:i], nil, staticKind, "", nil)
+			for ; i < lcpIndex && path[i] != '/'; i++ {
 			}
 
 			pnames = append(pnames, path[j:i])
 			path = path[:j] + path[i:]
-			i, l = j, len(path)
+			i, lcpIndex = j, len(path)
 
-			if i == l {
-				r.insert(method, path[:i], h, pkind, ppath, pnames)
+			if i == lcpIndex {
+				r.insert(method, path[:i], h, paramKind, ppath, pnames)
 			} else {
-				r.insert(method, path[:i], nil, pkind, "", nil)
+				r.insert(method, path[:i], nil, paramKind, "", nil)
 			}
 		} else if path[i] == '*' {
-			r.insert(method, path[:i], nil, skind, "", nil)
+			r.insert(method, path[:i], nil, staticKind, "", nil)
 			pnames = append(pnames, "*")
-			r.insert(method, path[:i+1], h, akind, ppath, pnames)
+			r.insert(method, path[:i+1], h, anyKind, ppath, pnames)
 		}
 	}
 
-	r.insert(method, path, h, skind, ppath, pnames)
+	r.insert(method, path, h, staticKind, ppath, pnames)
 }
 
 func (r *Router) insert(method, path string, h HandlerFunc, t kind, ppath string, pnames []string) {
 	// Adjust max param
-	l := len(pnames)
-	if *r.echo.maxParam < l {
-		*r.echo.maxParam = l
+	paramLen := len(pnames)
+	if *r.echo.maxParam < paramLen {
+		*r.echo.maxParam = paramLen
 	}
 
-	cn := r.tree // Current node as root
-	if cn == nil {
+	currentNode := r.tree // Current node as root
+	if currentNode == nil {
 		panic("echo: invalid method")
 	}
 	search := path
 
 	for {
-		sl := len(search)
-		pl := len(cn.prefix)
-		l := 0
+		searchLen := len(search)
+		prefixLen := len(currentNode.prefix)
+		lcpLen := 0
 
-		// LCP
-		max := pl
-		if sl < max {
-			max = sl
+		// LCP - Longest Common Prefix (https://en.wikipedia.org/wiki/LCP_array)
+		max := prefixLen
+		if searchLen < max {
+			max = searchLen
 		}
-		for ; l < max && search[l] == cn.prefix[l]; l++ {
+		for ; lcpLen < max && search[lcpLen] == currentNode.prefix[lcpLen]; lcpLen++ {
 		}
 
-		if l == 0 {
+		if lcpLen == 0 {
 			// At root node
-			cn.label = search[0]
-			cn.prefix = search
+			currentNode.label = search[0]
+			currentNode.prefix = search
 			if h != nil {
-				cn.kind = t
-				cn.addHandler(method, h)
-				cn.ppath = ppath
-				cn.pnames = pnames
+				currentNode.kind = t
+				currentNode.addHandler(method, h)
+				currentNode.ppath = ppath
+				currentNode.pnames = pnames
 			}
-		} else if l < pl {
+		} else if lcpLen < prefixLen {
 			// Split node
-			n := newNode(cn.kind, cn.prefix[l:], cn, cn.staticChildrens, cn.methodHandler, cn.ppath, cn.pnames, cn.paramChildren, cn.anyChildren)
+			n := newNode(
+				currentNode.kind,
+				currentNode.prefix[lcpLen:],
+				currentNode,
+				currentNode.staticChildren,
+				currentNode.methodHandler,
+				currentNode.ppath,
+				currentNode.pnames,
+				currentNode.paramChild,
+				currentNode.anyChild,
+			)
 
 			// Update parent path for all children to new node
-			for _, child := range cn.staticChildrens {
+			for _, child := range currentNode.staticChildren {
 				child.parent = n
 			}
-			if cn.paramChildren != nil {
-				cn.paramChildren.parent = n
+			if currentNode.paramChild != nil {
+				currentNode.paramChild.parent = n
 			}
-			if cn.anyChildren != nil {
-				cn.anyChildren.parent = n
+			if currentNode.anyChild != nil {
+				currentNode.anyChild.parent = n
 			}
 
 			// Reset parent node
-			cn.kind = skind
-			cn.label = cn.prefix[0]
-			cn.prefix = cn.prefix[:l]
-			cn.staticChildrens = nil
-			cn.methodHandler = new(methodHandler)
-			cn.ppath = ""
-			cn.pnames = nil
-			cn.paramChildren = nil
-			cn.anyChildren = nil
+			currentNode.kind = staticKind
+			currentNode.label = currentNode.prefix[0]
+			currentNode.prefix = currentNode.prefix[:lcpLen]
+			currentNode.staticChildren = nil
+			currentNode.methodHandler = new(methodHandler)
+			currentNode.ppath = ""
+			currentNode.pnames = nil
+			currentNode.paramChild = nil
+			currentNode.anyChild = nil
 
 			// Only Static children could reach here
-			cn.addStaticChild(n)
+			currentNode.addStaticChild(n)
 
-			if l == sl {
+			if lcpLen == searchLen {
 				// At parent node
-				cn.kind = t
-				cn.addHandler(method, h)
-				cn.ppath = ppath
-				cn.pnames = pnames
+				currentNode.kind = t
+				currentNode.addHandler(method, h)
+				currentNode.ppath = ppath
+				currentNode.pnames = pnames
 			} else {
 				// Create child node
-				n = newNode(t, search[l:], cn, nil, new(methodHandler), ppath, pnames, nil, nil)
+				n = newNode(t, search[lcpLen:], currentNode, nil, new(methodHandler), ppath, pnames, nil, nil)
 				n.addHandler(method, h)
 				// Only Static children could reach here
-				cn.addStaticChild(n)
+				currentNode.addStaticChild(n)
 			}
-		} else if l < sl {
-			search = search[l:]
-			c := cn.findChildWithLabel(search[0])
+		} else if lcpLen < searchLen {
+			search = search[lcpLen:]
+			c := currentNode.findChildWithLabel(search[0])
 			if c != nil {
 				// Go deeper
-				cn = c
+				currentNode = c
 				continue
 			}
 			// Create child node
-			n := newNode(t, search, cn, nil, new(methodHandler), ppath, pnames, nil, nil)
+			n := newNode(t, search, currentNode, nil, new(methodHandler), ppath, pnames, nil, nil)
 			n.addHandler(method, h)
 			switch t {
-			case skind:
-				cn.addStaticChild(n)
-			case pkind:
-				cn.paramChildren = n
-			case akind:
-				cn.anyChildren = n
+			case staticKind:
+				currentNode.addStaticChild(n)
+			case paramKind:
+				currentNode.paramChild = n
+			case anyKind:
+				currentNode.anyChild = n
 			}
 		} else {
 			// Node already exists
 			if h != nil {
-				cn.addHandler(method, h)
-				cn.ppath = ppath
-				if len(cn.pnames) == 0 { // Issue #729
-					cn.pnames = pnames
+				currentNode.addHandler(method, h)
+				currentNode.ppath = ppath
+				if len(currentNode.pnames) == 0 { // Issue #729
+					currentNode.pnames = pnames
 				}
 			}
 		}
@@ -217,21 +227,21 @@ func newNode(t kind, pre string, p *node, sc children, mh *methodHandler, ppath
 		label:          pre[0],
 		prefix:         pre,
 		parent:         p,
-		staticChildrens: sc,
+		staticChildren: sc,
 		ppath:          ppath,
 		pnames:         pnames,
 		methodHandler:  mh,
-		paramChildren:   paramChildren,
-		anyChildren:     anyChildren,
+		paramChild:     paramChildren,
+		anyChild:       anyChildren,
 	}
 }
 
 func (n *node) addStaticChild(c *node) {
-	n.staticChildrens = append(n.staticChildrens, c)
+	n.staticChildren = append(n.staticChildren, c)
 }
 
 func (n *node) findStaticChild(l byte) *node {
-	for _, c := range n.staticChildrens {
+	for _, c := range n.staticChildren {
 		if c.label == l {
 			return c
 		}
@@ -240,16 +250,16 @@ func (n *node) findStaticChild(l byte) *node {
 }
 
 func (n *node) findChildWithLabel(l byte) *node {
-	for _, c := range n.staticChildrens {
+	for _, c := range n.staticChildren {
 		if c.label == l {
 			return c
 		}
 	}
 	if l == paramLabel {
-		return n.paramChildren
+		return n.paramChild
 	}
 	if l == anyLabel {
-		return n.anyChildren
+		return n.anyChild
 	}
 	return nil
 }
@@ -330,13 +340,15 @@ func (n *node) checkMethodNotAllowed() HandlerFunc {
 func (r *Router) Find(method, path string, c Context) {
 	ctx := c.(*context)
 	ctx.path = path
-	cn := r.tree // Current node as root
+	currentNode := r.tree // Current node as root
 
 	var (
+		// search stores the remaining path to check for match. By each iteration we move from start of path to end of the path
+		// and search value gets shorter and shorter.
 		search      = path
 		searchIndex = 0
-		n           int           // Param counter
-		pvalues     = ctx.pvalues // Use the internal slice so the interface can keep the illusion of a dynamic slice
+		paramIndex  int           // Param counter
+		paramValues = ctx.pvalues // Use the internal slice so the interface can keep the illusion of a dynamic slice
 	)
 
 	// Backtracking is needed when a dead end (leaf node) is reached in the router tree.
@@ -345,9 +357,9 @@ func (r *Router) Find(method, path string, c Context) {
 	// For example if there is no static node match we should check parent next sibling by kind (param).
 	// Backtracking itself does not check if there is a next sibling, this is done by the router logic.
 	backtrackToNextNodeKind := func(fromKind kind) (nextNodeKind kind, valid bool) {
-		previous := cn
-		cn = previous.parent
-		valid = cn != nil
+		previous := currentNode
+		currentNode = previous.parent
+		valid = currentNode != nil
 
 		// Next node type by priority
 		// NOTE: With the current implementation we never backtrack from an `any` route, so `previous.kind` is
@@ -355,51 +367,57 @@ func (r *Router) Find(method, path string, c Context) {
 		// If this is changed then for any route next kind would be `static` and this statement should be changed
 		nextNodeKind = previous.kind + 1
 
-		if fromKind == skind {
+		if fromKind == staticKind {
 			// when backtracking is done from static kind block we did not change search so nothing to restore
 			return
 		}
 
 		// restore search to value it was before we move to current node we are backtracking from.
-		if previous.kind == skind {
+		if previous.kind == staticKind {
 			searchIndex -= len(previous.prefix)
 		} else {
-			n--
+			paramIndex--
 			// for param/any node.prefix value is always `:` so we can not deduce searchIndex from that and must use pValue
 			// for that index as it would also contain part of path we cut off before moving into node we are backtracking from
-			searchIndex -= len(pvalues[n])
+			searchIndex -= len(paramValues[paramIndex])
 		}
 		search = path[searchIndex:]
 		return
 	}
 
-	// Search order static > param > any
+	// Router tree is implemented by longest common prefix array (LCP array) https://en.wikipedia.org/wiki/LCP_array
+	// Tree search is implemented as for loop where one loop iteration is divided into 3 separate blocks
+	// Each of these blocks checks specific kind of node (static/param/any). Order of blocks reflex their priority in routing.
+	// Search order/priority is: static > param > any.
+	//
+	// Note: backtracking in tree is implemented by replacing/switching currentNode to previous node
+	// and hoping to (goto statement) next block by priority to check if it is the match.
 	for {
-		pl := 0 // Prefix length
-		l := 0  // LCP length
+		prefixLen := 0 // Prefix length
+		lcpLen := 0    // LCP (longest common prefix) length
 
-		if cn.label != ':' {
-			sl := len(search)
-			pl = len(cn.prefix)
+		if currentNode.kind == staticKind {
+			searchLen := len(search)
+			prefixLen = len(currentNode.prefix)
 
-			// LCP
-			max := pl
-			if sl < max {
-				max = sl
+			// LCP - Longest Common Prefix (https://en.wikipedia.org/wiki/LCP_array)
+			max := prefixLen
+			if searchLen < max {
+				max = searchLen
 			}
-			for ; l < max && search[l] == cn.prefix[l]; l++ {
+			for ; lcpLen < max && search[lcpLen] == currentNode.prefix[lcpLen]; lcpLen++ {
 			}
 		}
 
-		if l != pl {
+		if lcpLen != prefixLen {
 			// No matching prefix, let's backtrack to the first possible alternative node of the decision path
-			nk, ok := backtrackToNextNodeKind(skind)
+			nk, ok := backtrackToNextNodeKind(staticKind)
 			if !ok {
 				return // No other possibilities on the decision path
-			} else if nk == pkind {
+			} else if nk == paramKind {
 				goto Param
 				// NOTE: this case (backtracking from static node to previous any node) can not happen by current any matching logic. Any node is end of search currently
-				//} else if nk == akind {
+				//} else if nk == anyKind {
 				//	goto Any
 			} else {
 				// Not found (this should never be possible for static node we are looking currently)
@@ -408,31 +426,32 @@ func (r *Router) Find(method, path string, c Context) {
 		}
 
 		// The full prefix has matched, remove the prefix from the remaining search
-		search = search[l:]
-		searchIndex = searchIndex + l
+		search = search[lcpLen:]
+		searchIndex = searchIndex + lcpLen
 
 		// Finish routing if no remaining search and we are on an leaf node
-		if search == "" && cn.ppath != "" {
+		if search == "" && currentNode.ppath != "" {
 			break
 		}
 
 		// Static node
 		if search != "" {
-			if child := cn.findStaticChild(search[0]); child != nil {
-				cn = child
+			if child := currentNode.findStaticChild(search[0]); child != nil {
+				currentNode = child
 				continue
 			}
 		}
 
 	Param:
 		// Param node
-		if child := cn.paramChildren; search != "" && child != nil {
-			cn = child
+		if child := currentNode.paramChild; search != "" && child != nil {
+			currentNode = child
+			// FIXME: when param node does not have any children then param node should act similarly to any node - consider all remaining search as match
 			i, l := 0, len(search)
 			for ; i < l && search[i] != '/'; i++ {
 			}
-			pvalues[n] = search[:i]
-			n++
+			paramValues[paramIndex] = search[:i]
+			paramIndex++
 			search = search[i:]
 			searchIndex = searchIndex + i
 			continue
@@ -440,20 +459,20 @@ func (r *Router) Find(method, path string, c Context) {
 
 	Any:
 		// Any node
-		if child := cn.anyChildren; child != nil {
-			// If any node is found, use remaining path for pvalues
-			cn = child
-			pvalues[len(cn.pnames)-1] = search
+		if child := currentNode.anyChild; child != nil {
+			// If any node is found, use remaining path for paramValues
+			currentNode = child
+			paramValues[len(currentNode.pnames)-1] = search
 			break
 		}
 
 		// Let's backtrack to the first possible alternative node of the decision path
-		nk, ok := backtrackToNextNodeKind(akind)
+		nk, ok := backtrackToNextNodeKind(anyKind)
 		if !ok {
 			return // No other possibilities on the decision path
-		} else if nk == pkind {
+		} else if nk == paramKind {
 			goto Param
-		} else if nk == akind {
+		} else if nk == anyKind {
 			goto Any
 		} else {
 			// Not found
@@ -461,12 +480,12 @@ func (r *Router) Find(method, path string, c Context) {
 		}
 	}
 
-	ctx.handler = cn.findHandler(method)
-	ctx.path = cn.ppath
-	ctx.pnames = cn.pnames
+	ctx.handler = currentNode.findHandler(method)
+	ctx.path = currentNode.ppath
+	ctx.pnames = currentNode.pnames
 
 	if ctx.handler == nil {
-		ctx.handler = cn.checkMethodNotAllowed()
+		ctx.handler = currentNode.checkMethodNotAllowed()
 	}
 	return
 }