A little optimisation in get() and add() methods.

It was a bug that when there are children in the next node, but no parameterised one,  having a non-existent path element caused the main loop to continue and firing an if pathLen == i+1 case like if there was a legit node found.

LICENSE

@@ -1,6 +1,6 @@
 MIT License
 
-Copyright (c) <year> <copyright holders>
+Copyright (c) 2023 Alexander "Arav" Andreev <me@arav.su>
 
 Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the "Software"), to deal in the Software without restriction, including without limitation the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is furnished to do so, subject to the following conditions:
 

README.md

@@ -24,6 +24,12 @@ This router is used like many others., example:
 		...
 	}
 
+	s := r.Sub("/api/v1")
+
+	s.Handler(http.MethodGet, "/", func(w http.ResponseWriter, r *http.Request) {
+		...
+	})
+
 	if err := http.ListenAndServe(":8000", r); err != nil {
 		...
 	}

httpr.go

@@ -9,18 +9,47 @@ import (
 
 type path []string
 
-// newPath splits a path and ensures that it starts with a slash (/) and doesn't
-// have more than 1 catch-all parameter.
+// newPath ensures that a path provided is correct and splits it.
 func newPath(path string) (path, error) {
+	pathLen := len(path)
+
+	if pathLen == 0 {
+		return nil, errors.New("empty path is not allowed")
+	}
+
 	if path[0] != '/' {
-		return nil, errors.New("path should start with a slash (/) symbol")
+		return nil, errors.New("path should start with a slash symbol \"/\"")
 	}
 
 	if strings.Count(path, "*") > 1 {
-		return nil, errors.New("there can be only one catch-all (*) parameter in path")
+		return nil, errors.New("path can have only one catch-all parameter \"*\"")
 	}
 
-	parts := strings.Split(strings.TrimSuffix(path, "/"), "/")
+	if path[pathLen-1] == '/' {
+		path = path[:pathLen-1]
+	}
+
+	parts := strings.Split(path, "/")
+
+	parts[0] = "/"
+
+	return parts, nil
+}
+
+// newServePath is a reduced version of newPath for ServeHTTP.
+func newServePath(path string) (path, error) {
+	if path[0] != '/' {
+		return nil, errors.New("path should start with a slash symbol \"/\"")
+	}
+
+	path = strings.ReplaceAll(path, "//", "/")
+
+	pathLen := len(path)
+	if path[pathLen-1] == '/' {
+		path = path[:pathLen-1]
+	}
+
+	parts := strings.Split(path, "/")
 
 	parts[0] = "/"
 
@@ -60,7 +89,9 @@ outer:
 			path[i] = curNode.endpoint + ":" + path[i]
 		}
 
-		if pathLen == i+1 {
+		pathNextIdx := i + 1
+
+		if pathLen == pathNextIdx {
 			var params Params = make(Params)
 
 			for _, part := range path {
@@ -73,10 +104,15 @@ outer:
 			return curNode.handler, params
 		}
 
-		if pathLen > i+1 {
+		if pathLen > pathNextIdx {
+			if len(curNode.children) == 0 {
+				break outer
+			}
+
 			var paramNode *node
+
 			for _, next := range curNode.children {
-				if next.endpoint == path[i+1] {
+				if next.endpoint == path[pathNextIdx] {
 					curNode = next
 					continue outer
 				}
@@ -88,8 +124,10 @@ outer:
 
 			if paramNode != nil {
 				curNode = paramNode
-				continue
+				continue outer
 			}
+
+			break outer
 		}
 	}
 
@@ -97,12 +135,12 @@ outer:
 }
 
 func (n *node) add(path path, handler http.HandlerFunc) error {
-	pathLen := len(path)
+	pathLastIdx := len(path) - 1
 	curNode := n
 
 outer:
 	for i := range path {
-		if pathLen == i+1 {
+		if pathLastIdx == i {
 			if curNode.handler != nil {
 				return errors.New("attempt to redefine a handler for already existing path")
 			}
@@ -111,28 +149,29 @@ outer:
 			return nil
 		}
 
+		pathNextIdx := i + 1
+
 		for _, child := range curNode.children {
-			firstChar := path[i+1][0]
+			firstChar := path[pathNextIdx][0]
 			if (firstChar == ':' || firstChar == '*') && firstChar == child.endpoint[0] {
 				// Do not allow different param names, because only the first one
 				// is saved, so a param won't be available by a new name.
-				//
-				// I am not the one to judge, but it is a little strange to
-				// expect different types of param in one place.
-				if path[i+1] != child.endpoint {
-					return errors.New("param names " + path[i+1] + " and " + child.endpoint + " are differ")
+				// Therefore, it is good to return an error because in this case
+				// you're doing something wrong.
+				if path[pathNextIdx] != child.endpoint {
+					return errors.New("param names " + path[pathNextIdx] + " and " + child.endpoint + " are differ")
 				}
 				curNode = child
 				continue outer
 			}
 
-			if child.endpoint == path[i+1] {
+			if child.endpoint == path[pathNextIdx] {
 				curNode = child
 				continue outer
 			}
 		}
 
-		newChild := &node{endpoint: path[i+1]}
+		newChild := &node{endpoint: path[pathNextIdx]}
 		curNode.children = append(curNode.children, newChild)
 		curNode = newChild
 	}
@@ -150,31 +189,34 @@ func New() *Router {
 }
 
 func (rr *Router) ServeHTTP(w http.ResponseWriter, r *http.Request) {
-	if tree, ok := rr.tree[r.Method]; ok {
-		path, err := newPath(r.URL.Path)
-		if err != nil {
-			http.Error(w, err.Error(), http.StatusNotAcceptable)
-			return
-		}
+	tree, ok := rr.tree[r.Method]
+	if !ok {
+		http.Error(w, http.StatusText(http.StatusNotImplemented), http.StatusNotImplemented)
+		return
+	}
 
-		if handler, params := tree.get(path); handler != nil {
-			if params != nil {
-				r = r.WithContext(context.WithValue(r.Context(), ParamsKey, params))
-			}
-			handler(w, r)
-		} else {
-			if rr.NotFoundHandler != nil {
-				rr.NotFoundHandler(w, r)
-			} else {
-				http.Error(w, "Not Found", http.StatusNotFound)
-			}
+	path, err := newServePath(r.URL.Path)
+	if err != nil {
+		http.Error(w, err.Error(), http.StatusNotAcceptable)
+		return
+	}
+
+	if handler, params := tree.get(path); handler != nil {
+		if params != nil {
+			r = r.WithContext(context.WithValue(r.Context(), ParamsKey, params))
 		}
+		handler(w, r)
 	} else {
-		http.Error(w, http.StatusText(http.StatusMethodNotAllowed), http.StatusMethodNotAllowed)
+		if rr.NotFoundHandler != nil {
+			rr.NotFoundHandler(w, r)
+		} else {
+			http.Error(w, "Not Found", http.StatusNotFound)
+		}
 	}
 }
 
 // Handler registers a handler for provided pattern for a given HTTP method.
+// Pattern must start with a slash (/) symbol.
 func (rr *Router) Handler(method, pattern string, handler http.HandlerFunc) error {
 	path, err := newPath(pattern)
 	if err != nil {
@@ -185,11 +227,7 @@ func (rr *Router) Handler(method, pattern string, handler http.HandlerFunc) erro
 		rr.tree[method] = &node{endpoint: "/"}
 	}
 
-	if err := rr.tree[method].add(path, handler); err != nil {
-		return err
-	}
-
-	return nil
+	return rr.tree[method].add(path, handler)
 }
 
 // ServeStatic serves a given file system.
@@ -204,8 +242,41 @@ func (rr *Router) ServeStatic(path string, root http.FileSystem) error {
 	})
 }
 
-// Param returns a URL parameter (that is set like `/a/b/:key/d`) with a key
-// or an empty string if no such parameter found.
+// subPath attaches a base path in front of a pattern.
+//
+// It is not a sub-router, it just passes a resulted pattern down to
+// a router instance.
+type subPath struct {
+	r    *Router
+	base string
+}
+
+// Sub creates a group of handlers with the same base path.
+//
+// How to use:
+//
+//	r := httpr.New()
+//	...
+//	s := r.Sub("/api/v1")
+//	s.Handler(http.MethodGet, "/", func(w, r) {...})
+//	s.Handler(http.MethodGet, "/section", func(w, r) {...})
+func (rr *Router) Sub(base string) *subPath {
+	if base[len(base)-1] == '/' {
+		base = base[:len(base)-1]
+	}
+
+	return &subPath{
+		r:    rr,
+		base: base,
+	}
+}
+
+// Handler registers a handler for a sub-path.
+func (sp *subPath) Handler(method, pattern string, handler http.HandlerFunc) error {
+	return sp.r.Handler(method, sp.base+pattern, handler)
+}
+
+// Param returns a URL parameter set with :key, or an empty string if not found.
 func Param(r *http.Request, key string) string {
 	if params := r.Context().Value(ParamsKey).(Params); params != nil {
 		return params[key]

httpr_test.go

@@ -1,15 +1,15 @@
-package httpr_test
+package httpr
 
 import (
 	"net/http"
+	"net/http/httptest"
 	"os"
+	"strings"
 	"testing"
-
-	"git.arav.su/Arav/httpr"
 )
 
 func Test(t *testing.T) {
-	r := httpr.New()
+	r := New()
 
 	err := r.Handler(http.MethodGet, "/", func(w http.ResponseWriter, r *http.Request) {})
 	if err != nil {
@@ -47,3 +47,110 @@ func Test(t *testing.T) {
 	}
 
 }
+
+func TestPaths(t *testing.T) {
+	found := false
+
+	r := New()
+
+	err := r.Handler(http.MethodGet, "/:lel", func(w http.ResponseWriter, r *http.Request) { found = true })
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	r.NotFoundHandler = func(w http.ResponseWriter, r *http.Request) { found = false }
+
+	w := httptest.NewRecorder()
+	p := "/xmpp://me@arav.su"
+	req := httptest.NewRequest(http.MethodGet, p, strings.NewReader(""))
+	r.ServeHTTP(w, req)
+	if found {
+		t.Error("Path", p, "should return 404")
+	}
+
+	p = "/lel"
+	req = httptest.NewRequest(http.MethodGet, p, strings.NewReader(""))
+	r.ServeHTTP(w, req)
+	if !found {
+		t.Error("Path", p, "should return 200")
+	}
+
+	p = "/lel/lol"
+	req = httptest.NewRequest(http.MethodGet, p, strings.NewReader(""))
+	r.ServeHTTP(w, req)
+	if found {
+		t.Error("Path", p, "should return 404")
+	}
+}
+
+func TestSubPaths(t *testing.T) {
+	found := true
+
+	r := New()
+
+	s := r.Sub("/api/v1")
+
+	err := s.Handler(http.MethodGet, "/", func(w http.ResponseWriter, r *http.Request) { found = true })
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	err = s.Handler(http.MethodGet, "/test", func(w http.ResponseWriter, r *http.Request) { found = true })
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	r.NotFoundHandler = func(w http.ResponseWriter, r *http.Request) { found = false }
+
+	w := httptest.NewRecorder()
+	p := "/api/v1/"
+	req := httptest.NewRequest(http.MethodGet, p, strings.NewReader(""))
+	r.ServeHTTP(w, req)
+	if !found {
+		t.Error("Path", p, "should return 200")
+	}
+
+	p = "/api/v1/test"
+	req = httptest.NewRequest(http.MethodGet, p, strings.NewReader(""))
+	r.ServeHTTP(w, req)
+	if !found {
+		t.Error("Path", p, "should return 200")
+	}
+
+	p = "/api/v1/nonexistent"
+	req = httptest.NewRequest(http.MethodGet, p, strings.NewReader(""))
+	r.ServeHTTP(w, req)
+	if found {
+		t.Error(found, "Path", p, "should return 404")
+	}
+}
+
+func TestPathParsing(t *testing.T) {
+	p, err := newPath("/api/v1/../.")
+	if err != nil {
+		t.Error(err)
+	}
+	t.Log(p)
+}
+
+const testStr = "/api/v1/foo/bar/baz/abc/def/fucc/b0y/of/a/local/dungeon/master/got/his/ass/fisted/for/free/and/he/was/absolutely/happy/with/that/"
+
+func BenchmarkPatternPathParsing(b *testing.B) {
+	for i := 0; i < b.N; i++ {
+		p, err := newPath(testStr)
+		if err != nil {
+			b.Fatal(err)
+		}
+		b.Log(len(p))
+	}
+}
+
+func BenchmarkServePathParsing(b *testing.B) {
+	for i := 0; i < b.N; i++ {
+		p, err := newServePath(testStr)
+		if err != nil {
+			b.Fatal(err)
+		}
+		b.Log(len(p))
+	}
+}