diff --git a/CONTEXT.md b/CONTEXT.md index 64ee7c6..f69f91e 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -340,6 +340,12 @@ segment. It is legal anywhere in a pattern, not only in the final position, so a gap in its middle. _Avoid_: a wildcard confined to one segment, a catch-all valid only as the last segment +**Whole-piece parameter**: +A `{name}` route segment names the whole piece between two slashes, never a part of one. A segment +mixing literal text with a parameter — `{id}c`, `x{id}`, `{a}-{b}` — is not a pattern govalin can +express, and a route carrying one fails at registration rather than matching nothing. +_Avoid_: a parameter embedded in a segment, a segment holding two parameters, a partial-piece capture + **Lifecycle breadth**: Before and After handlers run for *every* route whose pattern matches the URL, while the request is served by exactly one. Matching for the lifecycle is a collection; matching for the handler is a @@ -455,3 +461,4 @@ _Avoid_: a before handler shadowed by an earlier match, lifecycle handlers resol - `Vary` was read as naming the headers a response carries; resolved: it names the request headers the response was selected from, and a response header there is a cache key of nothing. - Declaring a `Vary` was read as last-call-wins, the way a lifetime is; resolved: a lifetime is a policy one caller chooses for the response and a selecting header is a fact each layer knows a piece of, so declarations accumulate. - "Who may store this" was read as something a cache key could answer; resolved: a key selects between stored responses, and a response that must not be shared at all is a cache scope decision. +- A route index was read as requiring most-specific-wins, the way the Go routers built on this shape resolve overlaps; resolved: **Registration-order match** is the rule an index has to preserve, so the tree carries the lowest registration order beneath each node and explores every branch that could beat the best it has. diff --git a/docs/adr/0015-segment-tree-route-index.md b/docs/adr/0015-segment-tree-route-index.md new file mode 100644 index 0000000..a8be11a --- /dev/null +++ b/docs/adr/0015-segment-tree-route-index.md @@ -0,0 +1,70 @@ +# A segment tree that keeps registration order + +## Status + +accepted + +## Context and decision + +Matching a URL asked every registered route whether it matched. Each route held a compiled regexp, +so a 200 route table ran 200 regexp executions per request: 63% of the request's time, plus the +`sync.Pool` traffic of taking a regexp machine out and putting it back 200 times. What a route cost +depended on where its author had happened to register it — 331ns at the top of the table, 4611ns at +the bottom. + +Routes are now indexed by their segments. A literal segment is a key, a parameter segment is the one +child that takes any piece, and a route sits at the node its last segment reaches; the walk descends +the URL's pieces rather than the table. A match costs what the path is, not what the table holds. + +### Registration order is the constraint, not a detail to trade away + +Every other Go router of this shape resolves overlapping routes by specificity: a static segment +beats a parameter, a parameter beats a wildcard. Govalin's **Registration-order match** says the +opposite — the first route registered whose pattern matches is the one that serves, so a literal +registered after a parameter route that also matches is unreachable *by design*. That rule is the +author's tool for deciding which of two overlapping patterns wins, and an index that quietly replaced +it with specificity would change which handler answers an existing app's requests. + +So the tree preserves it. Each node carries the lowest registration order of any route beneath it, +and the walk carries the best order it has found; a subtree that cannot beat it is never entered. +Parameter and literal children are both explored, because either may hold the earlier route. + +### The tree indexes segments, so what is not a segment stays in a list + +A wildcard spans separators, which is not a shape a per-segment tree can key on. Rather than +complicate the tree with a construct that appears a handful of times in an app — a static mount, a +CORS `Before`, an HTTPS redirect — routes holding a wildcard stay in an ordered list scanned after +the walk, where the lowest order still wins. An app that registers many wildcard routes gets the +linear scan it always had, now over a segment matcher rather than a regexp. + +The same reasoning removed the regexps: the patterns the parser built were only ever literals, +`([^/]+?)` and `.+?` joined by slashes, which is a walk over slash-separated pieces expressed in a +language that cannot know that. `PathMatcher` walks its segments directly and captures parameters on +the same pass instead of running the pattern a second time for submatches. + +## Considered options + +- **A segment tree ordered by registration (chosen)** — see above. +- **A specificity tree, as httprouter and gin use** — simpler, no order bookkeeping, and faster still + because only one branch is ever explored. Rejected because it silently changes which handler serves + a URL in any app with overlapping routes. +- **Keeping the linear scan and only replacing the regexp** — this was measured: it took the bottom + of a 200 route table from 4350ns to 1950ns for a fraction of the code. Rejected as the endpoint + rather than the step it became, because the cost still scales with the table. +- **Making the tree the whole answer and confirming nothing** — the segments settle everything about + a match except how the path spells its end. Rather than re-run the full match to check, the matcher + reports whether its trailing slash is optional and the walk asks at the node. +- **A slice of literal children instead of a map** — measured 5ns faster per request on small tables + and 40ns slower on a node with 40 children. Rejected: the map is the one that does not reintroduce + a scan proportional to the route table, which is the thing being fixed. + +## Consequences + +- A route table's shape no longer changes what a request costs: 120ns at the top of a 200 route + table and 271ns at the bottom, against 331ns and 4611ns before, and a one route app is unchanged. +- Every route now exists twice, in `pathHandlers` and in the index. `getOrCreatePathHandlerByPath` is + the single place a route is created, so it is the single place the two can diverge. +- The index is only correct if it agrees with a scan of the table. That is a property rather than a + case list, so it is tested as one, against 20k generated route tables. +- Before and after handlers still scan, over lists holding only the routes that have one. They match + every route they hit rather than the first, so the index's answer is not the question they ask. diff --git a/handlers.go b/handlers.go index b13e023..7d8c40f 100644 --- a/handlers.go +++ b/handlers.go @@ -8,6 +8,7 @@ import ( ) type pathHandler struct { + order int PathFragment string PathMatcher routing.PathMatcher Before BeforeFunc @@ -21,15 +22,15 @@ type pathHandler struct { Options HandlerFunc } -func newPathHandlerFromPathFragment(pathFragment string) (pathHandler, error) { +func newPathHandlerFromPathFragment(pathFragment string) (*pathHandler, error) { pathMatcher, err := routing.NewPathMatcherFromString(pathFragment) if err != nil { - return pathHandler{}, fmt.Errorf( + return nil, fmt.Errorf( "failed to create path matcher for pathFragment '%s'. Err: %w", pathFragment, err, ) } - return pathHandler{ + return &pathHandler{ PathFragment: pathFragment, PathMatcher: pathMatcher, Head: nil, diff --git a/internal/routing/path-parser.go b/internal/routing/path-parser.go index c11f46a..76eb17d 100644 --- a/internal/routing/path-parser.go +++ b/internal/routing/path-parser.go @@ -3,15 +3,19 @@ package routing import ( "fmt" "log/slog" - "regexp" "strings" ) +// PathMatcher matches request URLs against one compiled route path. type PathMatcher struct { - pathParamNames []string - regexp *regexp.Regexp + segments []Segment + pathParamNames []string + optionalTrailingSlash bool + matchesEverything bool } +// NewPathMatcherFromString compiles a route path into a matcher. It fails on a +// path segment that is neither a literal, a '{name}' parameter nor a wildcard. func NewPathMatcherFromString(path string) (PathMatcher, error) { // A path of nothing but slashes is the root spelled oddly, and a route fragment // joined to a path produces it. The matcher has to match what the warning says it became. @@ -20,17 +24,11 @@ func NewPathMatcherFromString(path string) (PathMatcher, error) { slog.Warn(fmt.Sprintf("The path '%s' was converted to /", path)) } - return PathMatcher{ - pathParamNames: []string{}, - regexp: regexp.MustCompile("^/$"), - }, nil + return PathMatcher{}, nil } - if path == "*" { - return PathMatcher{ - pathParamNames: []string{}, - regexp: regexp.MustCompile(".*?"), - }, nil + if path == wildcard { + return PathMatcher{matchesEverything: true}, nil } pathSegments, err := getPathSegments(path) @@ -39,33 +37,21 @@ func NewPathMatcherFromString(path string) (PathMatcher, error) { } pathParamNames := []string{} - for _, ps := range pathSegments { - pathParamNames = append(pathParamNames, ps.PathNames...) - } - - groupRegexpParts := []string{} - - for _, ps := range pathSegments { - groupRegexpParts = append(groupRegexpParts, ps.GroupedRegex) - } - - // Appended after the join, not as a segment: a segment carries a mandatory slash in - // front of the optional one, leaving the pattern matching only the doubled form. - optionalTrailingSlash := "" - if strings.HasSuffix(path, "/") { - optionalTrailingSlash = "/?" + for _, segment := range pathSegments { + if segment.Kind == ParameterSegment { + pathParamNames = append(pathParamNames, segment.Text) + } } - fullGroupedRegexpString := "^/" + strings.Join(groupRegexpParts, "/") + optionalTrailingSlash + "$" - return PathMatcher{ - pathParamNames: pathParamNames, - regexp: regexp.MustCompile(fullGroupedRegexpString), + segments: pathSegments, + pathParamNames: pathParamNames, + optionalTrailingSlash: strings.HasSuffix(path, "/"), }, nil } -func getPathSegments(path string) ([]pathSegment, error) { - pathSegments := []pathSegment{} +func getPathSegments(path string) ([]Segment, error) { + pathSegments := []Segment{} pathParts := strings.Split(path, "/") for _, pathPiece := range pathParts { @@ -86,7 +72,9 @@ func getPathSegments(path string) ([]pathSegment, error) { // MatchesURL checks whether given string URL matches the path. func (path *PathMatcher) MatchesURL(url string) bool { - return path.regexp.MatchString(url) + _, matches := path.match(url, nil) + + return matches } // PathParams extracts the path parameters from given string url according @@ -100,17 +88,117 @@ func (path *PathMatcher) PathParams(url string) map[string]string { return nil } - pathparamMap := map[string]string{} - pathParams := path.regexp.FindStringSubmatch(url) + values, matches := path.match(url, make([]string, 0, len(path.pathParamNames))) + if !matches { + slog.Error(fmt.Sprintf("The URL '%s' does not match the path it was asked for path params on", url)) + return map[string]string{} + } + + pathParamMap := make(map[string]string, len(values)) + for i, name := range path.pathParamNames { + pathParamMap[name] = values[i] + } + + return pathParamMap +} + +// Segments returns the pieces the path compiled to, and whether the path is +// nothing but those pieces. A caller that indexes routes by their segments can +// key on the ones it gets; a path holding a wildcard, or one that matches every +// URL, matches by a rule its segments do not carry and reports false. +func (path *PathMatcher) Segments() ([]Segment, bool) { + if path.matchesEverything { + return nil, false + } + + for _, segment := range path.segments { + if segment.Kind == WildcardSegment { + return nil, false + } + } + + return path.segments, true +} + +// OptionalTrailingSlash reports whether the path was registered with a trailing +// slash, and so matches a URL spelled either way. A caller that decided a match +// by the segments alone still has to ask: the slash is a rule about the end of +// the path rather than a piece of it. +func (path *PathMatcher) OptionalTrailingSlash() bool { + return path.optionalTrailingSlash +} + +// match walks the URL across the compiled segments, appending the values it +// captures to values. A nil values skips capture, so the routes a request does +// not match cost nothing to rule out. +func (path *PathMatcher) match(url string, values []string) ([]string, bool) { + if path.matchesEverything { + return values, true + } + + if len(url) == 0 || url[0] != '/' { + return values, false + } + + return path.matchSegments(path.segments, url[1:], values) +} + +// matchSegments matches segments against a URL positioned at the first +// character of the first segment. +func (path *PathMatcher) matchSegments(segments []Segment, url string, values []string) ([]string, bool) { + if len(segments) == 0 { + return values, url == "" || (path.optionalTrailingSlash && url == "/") + } + + head, rest := segments[0], segments[1:] + + switch head.Kind { + case LiteralSegment: + if !strings.HasPrefix(url, head.Text) { + return values, false + } + + return path.matchSeparator(rest, url[len(head.Text):], values) + + case ParameterSegment: + end := strings.IndexByte(url, '/') + if end < 0 { + end = len(url) + } + if end == 0 { + return values, false + } + if values != nil { + values = append(values, url[:end]) + } + + return path.matchSeparator(rest, url[end:], values) + } + + // A wildcard spans separators and takes as little as it can, so the shortest + // remainder that lets the segments after it match is the one that wins. + if len(rest) == 0 { + return values, url != "" + } + + for consumed := 1; consumed < len(url); consumed++ { + if captured, matches := path.matchSeparator(rest, url[consumed:], values); matches { + return captured, true + } + } + + return values, false +} - if len(pathParams) != len(path.pathParamNames)+1 { - slog.Error("The number of path params is not the same as configured path names") - return pathparamMap +// matchSeparator consumes the slash between two segments. +func (path *PathMatcher) matchSeparator(segments []Segment, url string, values []string) ([]string, bool) { + if len(segments) == 0 { + return path.matchSegments(segments, url, values) } - for i, v := range path.pathParamNames { - pathparamMap[v] = pathParams[i+1] + if len(url) == 0 || url[0] != '/' { + return values, false } - return pathparamMap + return path.matchSegments(segments, url[1:], values) } diff --git a/internal/routing/path-parser_test.go b/internal/routing/path-parser_test.go index 62f3337..c636f35 100644 --- a/internal/routing/path-parser_test.go +++ b/internal/routing/path-parser_test.go @@ -76,3 +76,56 @@ func TestNestedWildcardMatch(t *testing.T) { assert.Equal(t, false, pathMatcher.MatchesURL("/baz/baz/foo"), "Should not match on mismatched wildcard") assert.Equal(t, false, pathMatcher.MatchesURL("/foo/baz"), "Should not match on mismatched wildcard") } + +// A segment mixing literal text and a parameter used to silently compile to a +// pattern matching nothing its author meant; now it fails at registration. +func TestMalformedParameterSegmentIsRejected(t *testing.T) { + for _, path := range []string{"/a/{b}c", "/a/x{b}", "/a/{b}-{c}", "/a/{}", "/a/{{b}}", "/a/{b", "/a/b}"} { + _, err := routing.NewPathMatcherFromString(path) + + assert.Error(t, err, "'%s' is not a path the matcher can express", path) + } +} + +// TestParameterSegmentSpansTheWholePiece keeps the shapes either side of the +// rejection above matching what they always did. +func TestParameterSegmentSpansTheWholePiece(t *testing.T) { + pathMatcher, err := routing.NewPathMatcherFromString("/a/{b}") + + assert.Nil(t, err) + assert.Equal(t, true, pathMatcher.MatchesURL("/a/foo"), "Should match a value in the parameter position") + assert.Equal(t, map[string]string{"b": "foo"}, pathMatcher.PathParams("/a/foo"), "Should capture the value") + assert.Equal(t, false, pathMatcher.MatchesURL("/a/"), "Should not match an empty parameter") + assert.Equal(t, false, pathMatcher.MatchesURL("/a/foo/bar"), "Should not span a separator") +} + +// TestWildcardBacktracksToTheSegmentsAfterIt covers the case a segment walk has +// to get right and a left-to-right scan does not: a wildcard takes as little as +// it can, so how much of the URL it swallows is only settled by whether the +// segments after it still match. +func TestWildcardBacktracksToTheSegmentsAfterIt(t *testing.T) { + pathMatcher, err := routing.NewPathMatcherFromString("/a/*/b/{id}") + assert.Nil(t, err) + + assert.Equal(t, true, pathMatcher.MatchesURL("/a/x/b/1"), "Should match with the wildcard taking one piece") + assert.Equal(t, true, pathMatcher.MatchesURL("/a/x/y/z/b/1"), "Should match with the wildcard spanning separators") + assert.Equal(t, true, pathMatcher.MatchesURL("/a/b/b/1"), "Should pass over a piece the later literal also matches") + assert.Equal(t, false, pathMatcher.MatchesURL("/a/b/1"), "Should not match with nothing left for the wildcard") + assert.Equal(t, false, pathMatcher.MatchesURL("/a/x/b/1/2"), "Should not let the parameter span a separator") + + assert.Equal( + t, + map[string]string{"id": "1"}, + pathMatcher.PathParams("/a/x/y/b/1"), + "Should capture from the match the wildcard backtracked into", + ) +} + +// TestWildcardMatchesAnyCharacter pins the wildcard as any character: the +// regexp it replaced skipped a newline a parameter in the same position accepted. +func TestWildcardMatchesAnyCharacter(t *testing.T) { + pathMatcher, err := routing.NewPathMatcherFromString("/a/*") + + assert.Nil(t, err) + assert.Equal(t, true, pathMatcher.MatchesURL("/a/x\ny"), "Should match a path holding a newline") +} diff --git a/internal/routing/path-segment.go b/internal/routing/path-segment.go index 79ba88c..f11f2eb 100644 --- a/internal/routing/path-segment.go +++ b/internal/routing/path-segment.go @@ -2,64 +2,57 @@ package routing import ( "fmt" - "regexp" "strings" ) -type pathSegment struct { - PathPiece string - GroupedRegex string - PathNames []string +// SegmentKind is what one piece of a route path matches. +type SegmentKind uint8 + +const ( + // LiteralSegment matches exactly its text. + LiteralSegment SegmentKind = iota + // ParameterSegment matches any whole piece and captures it. + ParameterSegment + // WildcardSegment matches one or more characters across separators. + WildcardSegment +) + +// Segment is one slash-separated piece of a compiled route path. +type Segment struct { + // Kind says what the segment matches. + Kind SegmentKind + // Text is the literal a literal segment matches, or the name a parameter + // segment captures under. A wildcard segment has neither. + Text string } -var ( +const ( delimiterStart = "{" delimiterEnd = "}" + delimiters = delimiterStart + delimiterEnd wildcard = "*" - - wildcardPathSegment = pathSegment{ - PathPiece: "*", - PathNames: []string{}, - GroupedRegex: ".+?", - } ) -func newPathSegment(pathPiece string) (pathSegment, error) { - delimiterStartCount := strings.Count(pathPiece, delimiterStart) - delimiterEndCount := strings.Count(pathPiece, delimiterEnd) - totalDelimiters := delimiterStartCount + delimiterEndCount - - if delimiterStartCount != delimiterEndCount { - return pathSegment{}, fmt.Errorf("number of '%d' and '%d' is not the same", delimiterStartCount, delimiterEndCount) - } - +// newPathSegment reads one slash-separated piece of a route path. A piece is a +// wildcard, a literal, or a single '{name}' parameter that spans the whole +// piece; anything else is a path the matcher has no meaning for. +func newPathSegment(pathPiece string) (Segment, error) { if pathPiece == wildcard { - return wildcardPathSegment, nil - } - - if totalDelimiters == 0 { - return createNormalPathSegment(pathPiece), nil + return Segment{Kind: WildcardSegment}, nil } - if totalDelimiters == 2 && pathPiece[0:1] == delimiterStart && pathPiece[len(pathPiece)-1:] == delimiterEnd { - return createParameterPathSegment(pathPiece), nil + if !strings.ContainsAny(pathPiece, delimiters) { + return Segment{Kind: LiteralSegment, Text: pathPiece}, nil } - return pathSegment{}, nil -} + name, hasStart := strings.CutPrefix(pathPiece, delimiterStart) + name, hasEnd := strings.CutSuffix(name, delimiterEnd) -func createNormalPathSegment(pathPiece string) pathSegment { - return pathSegment{ - PathPiece: pathPiece, - PathNames: []string{}, - GroupedRegex: regexp.QuoteMeta(pathPiece), + if !hasStart || !hasEnd || name == "" || strings.ContainsAny(name, delimiters) { + return Segment{}, fmt.Errorf( + "path segment '%s' is neither a literal nor a '{name}' parameter", pathPiece, + ) } -} -func createParameterPathSegment(pathPiece string) pathSegment { - return pathSegment{ - PathPiece: pathPiece, - PathNames: []string{strings.Trim(strings.Trim(pathPiece, delimiterStart), delimiterEnd)}, - GroupedRegex: "([^/]+?)", - } + return Segment{Kind: ParameterSegment, Text: name}, nil } diff --git a/routeindex.go b/routeindex.go new file mode 100644 index 0000000..b880b21 --- /dev/null +++ b/routeindex.go @@ -0,0 +1,173 @@ +package govalin + +import ( + "math" + "strings" + + "github.com/pkkummermo/govalin/internal/routing" +) + +// routeIndex answers which route serves a URL without asking every route: a +// tree over path segments, walked by the URL's pieces. Registration order +// still decides which of several matching routes wins, and wildcard routes, +// which the tree cannot key on, stay in an ordered list — see ADR 0015. +type routeIndex struct { + root *routeNode + wildcards []*pathHandler +} + +type routeNode struct { + literals map[string]*routeNode + param *routeNode + routes []*pathHandler + minOrder int +} + +func (index *routeIndex) add(handler *pathHandler) { + segments, indexable := handler.PathMatcher.Segments() + if !indexable { + index.wildcards = append(index.wildcards, handler) + return + } + + if index.root == nil { + index.root = &routeNode{minOrder: handler.order} + } + + node := index.root + for _, segment := range segments { + node = node.child(segment, handler.order) + } + + node.routes = append(node.routes, handler) +} + +// child returns the node a segment leads to, creating it if this is the first +// route to take that step. Routes arrive in registration order, so the order +// that creates a node is the lowest any route below it can have. +func (node *routeNode) child(segment routing.Segment, order int) *routeNode { + if segment.Kind == routing.ParameterSegment { + if node.param == nil { + node.param = &routeNode{minOrder: order} + } + + return node.param + } + + if child, exists := node.literals[segment.Text]; exists { + return child + } + + child := &routeNode{minOrder: order} + if node.literals == nil { + node.literals = map[string]*routeNode{} + } + node.literals[segment.Text] = child + + return child +} + +// match returns the route that serves the URL for the method — the first +// registered of those that match — or nil when none does. +func (index *routeIndex) match(url string, method string) *pathHandler { + walk := routeWalk{method: method, bestOrder: math.MaxInt} + + if index.root != nil && len(url) > 0 && url[0] == '/' { + pieces := url[1:] + + // A route path holds its trailing slash as a rule about how the path ends + // rather than as a piece of its own, so the URL's is read off before the walk. + walk.trailingSlash = strings.HasSuffix(pieces, "/") + if walk.trailingSlash { + pieces = pieces[:len(pieces)-1] + } + + if pieces == "" { + walk.acceptAt(index.root) + } else { + walk.visit(index.root, pieces) + } + } + + for _, handler := range index.wildcards { + if handler.order >= walk.bestOrder { + break + } + + if handler.GetHandlerByMethod(method) != nil && handler.PathMatcher.MatchesURL(url) { + walk.best = handler + walk.bestOrder = handler.order + + break + } + } + + return walk.best +} + +type routeWalk struct { + method string + best *pathHandler + bestOrder int + trailingSlash bool +} + +// visit descends the node by the first of the URL pieces it has left. +func (walk *routeWalk) visit(node *routeNode, pieces string) { + if node.minOrder >= walk.bestOrder { + return + } + + piece, rest, more := strings.Cut(pieces, "/") + child := node.literals[piece] + + // A parameter takes any piece there is, but never the absence of one. + param := node.param + if piece == "" { + param = nil + } + + if !more { + if child != nil { + walk.acceptAt(child) + } + if param != nil { + walk.acceptAt(param) + } + + return + } + + if child != nil { + walk.visit(child, rest) + } + if param != nil { + walk.visit(param, rest) + } +} + +// acceptAt takes the best route ending at a node the walk reached, which means +// every segment of it matched. What the segments do not carry is how the path +// spells its end, so the trailing slash is settled here. +func (walk *routeWalk) acceptAt(node *routeNode) { + if node.minOrder >= walk.bestOrder { + return + } + + for _, handler := range node.routes { + if handler.order >= walk.bestOrder { + return + } + + if walk.trailingSlash && !handler.PathMatcher.OptionalTrailingSlash() { + continue + } + + if handler.GetHandlerByMethod(walk.method) != nil { + walk.best = handler + walk.bestOrder = handler.order + + return + } + } +} diff --git a/routeindex_test.go b/routeindex_test.go new file mode 100644 index 0000000..57f1d24 --- /dev/null +++ b/routeindex_test.go @@ -0,0 +1,97 @@ +package govalin + +import ( + "math/rand/v2" + "net/http" + "strings" + "testing" +) + +// linearMatch is the scan the index replaced: walk the route table in +// registration order and take the first route that matches and answers the +// method. +func linearMatch(handlers []*pathHandler, url string, method string) *pathHandler { + for _, handler := range handlers { + if handler.GetHandlerByMethod(method) != nil && handler.PathMatcher.MatchesURL(url) { + return handler + } + } + + return nil +} + +// TestRouteIndexAgreesWithAScanOfTheTable is the property the index has to +// hold: it returns the route the table walk would have, whatever the table. +// Random tables reach the overlaps a written-out case list does not — a literal +// registered behind a parameter, two routes on the same node differing only in +// a trailing slash, a wildcard cutting in ahead of a tree route. +func TestRouteIndexAgreesWithAScanOfTheTable(t *testing.T) { + pieces := []string{"a", "b", "ab", "{id}", "{name}", "*", "c"} + urlPieces := []string{"a", "b", "ab", "c", "x", "yy", ""} + methods := []string{http.MethodGet, http.MethodPost, http.MethodDelete} + + random := rand.New(rand.NewPCG(1, 2)) + + for range 20000 { + app := New(func(config *Config) { + config.EnableAccessLog(false) + config.EnableStartupLog(false) + }) + + registered := map[string]bool{} + for range random.IntN(8) + 1 { + path := "" + for range random.IntN(3) + 1 { + path += "/" + pieces[random.IntN(len(pieces))] + } + if random.IntN(4) == 0 { + path += "/" + } + + method := methods[random.IntN(len(methods))] + if registered[method+" "+path] { + continue + } + registered[method+" "+path] = true + + app.addMethod(method, path, func(_ *Call) {}) + } + + url := "" + for range random.IntN(4) + 1 { + url += "/" + urlPieces[random.IntN(len(urlPieces))] + } + if random.IntN(4) == 0 { + url += "/" + } + + for _, method := range methods { + want := linearMatch(app.pathHandlers, url, method) + got := app.routes.match(url, method) + + if want != got { + t.Fatalf( + "%s %s over table [%s]: table walk picked %v, index picked %v", + method, url, describe(app.pathHandlers), fragmentOf(want), fragmentOf(got), + ) + } + } + } +} + +func describe(handlers []*pathHandler) string { + fragments := make([]string, 0, len(handlers)) + for _, handler := range handlers { + fragments = append(fragments, handler.PathFragment) + } + + return strings.Join(fragments, " ") +} + +func fragmentOf(handler *pathHandler) string { + if handler == nil { + return "" + } + + return handler.PathFragment +} diff --git a/server.go b/server.go index 81901d1..422e7c3 100644 --- a/server.go +++ b/server.go @@ -8,6 +8,7 @@ import ( "net" "net/http" "os" + "slices" "time" "github.com/pkkummermo/govalin/internal/http/headers" @@ -29,7 +30,10 @@ type App struct { mux *http.ServeMux server http.Server currentFragment string - pathHandlers []pathHandler + pathHandlers []*pathHandler + routes routeIndex + beforeHandlers []*pathHandler + afterHandlers []*pathHandler } // New creates a new Govalin App instance. @@ -152,6 +156,12 @@ func (server *App) addMethod(method string, fullPath string, methodHandler Handl // short circuited. func (server *App) Before(path string, beforeFunc BeforeFunc) { fullPath := server.currentFragment + path + + if beforeFunc == nil { + slog.Error(fmt.Sprintf("Before on path %s is nil.", fullPath)) + os.Exit(1) + } + handler := server.getOrCreatePathHandlerByPath(fullPath) if handler.Before != nil { @@ -160,6 +170,7 @@ func (server *App) Before(path string, beforeFunc BeforeFunc) { } handler.Before = beforeFunc + server.beforeHandlers = insertInRegistrationOrder(server.beforeHandlers, handler) } // Add an after handler to given path @@ -168,14 +179,21 @@ func (server *App) Before(path string, beforeFunc BeforeFunc) { // the same request. func (server *App) After(path string, afterFunc AfterFunc) { fullPath := server.currentFragment + path + + if afterFunc == nil { + slog.Error(fmt.Sprintf("After on path %s is nil.", fullPath)) + os.Exit(1) + } + handler := server.getOrCreatePathHandlerByPath(fullPath) if handler.After != nil { - slog.Error(fmt.Sprintf("Before already exists on path %s.", fullPath)) + slog.Error(fmt.Sprintf("After already exists on path %s.", fullPath)) os.Exit(1) } handler.After = afterFunc + server.afterHandlers = insertInRegistrationOrder(server.afterHandlers, handler) } // Add a GET handler @@ -337,44 +355,51 @@ func (server *App) Shutdown() error { } func (server *App) getOrCreatePathHandlerByPath(path string) *pathHandler { - if existingPathHandler, pathNotFoundErr := server.getPathHandlerByPath(path); pathNotFoundErr == nil { + if existingPathHandler := server.getPathHandlerByPath(path); existingPathHandler != nil { return existingPathHandler } + newHandler, pathHandlerErr := newPathHandlerFromPathFragment(path) if pathHandlerErr != nil { slog.Error(fmt.Sprintf("Failed to create handler for path '%s'. Err: %v", path, pathHandlerErr)) os.Exit(1) } + newHandler.order = len(server.pathHandlers) server.pathHandlers = append(server.pathHandlers, newHandler) - handler, err := server.getPathHandlerByPath(path) - if err != nil { - slog.Error(fmt.Sprintf("Failed to retrieve newly created handler for path '%s'. Err %v", path, err)) - os.Exit(1) + server.routes.add(newHandler) + + return newHandler +} + +// insertInRegistrationOrder places a handler in a lifecycle list at the position +// its path holds in the route table, which is not where it was appended: Before +// and After can be registered on a path some earlier route already created. +func insertInRegistrationOrder(handlers []*pathHandler, handler *pathHandler) []*pathHandler { + position := len(handlers) + for position > 0 && handlers[position-1].order > handler.order { + position-- } - return handler + + return slices.Insert(handlers, position, handler) } -func (server *App) getPathHandlerByPath(path string) (*pathHandler, error) { - for i := range server.pathHandlers { - if server.pathHandlers[i].PathFragment == path { - return &server.pathHandlers[i], nil +func (server *App) getPathHandlerByPath(path string) *pathHandler { + for _, pathHandler := range server.pathHandlers { + if pathHandler.PathFragment == path { + return pathHandler } } - return &pathHandler{}, fmt.Errorf( - "no pathHandler found for given path %s", path, - ) + return nil } func (server *App) matchBeforeHandlers(call *Call) bool { - for i := range server.pathHandlers { - pathHandler := &server.pathHandlers[i] - + for _, pathHandler := range server.beforeHandlers { if call.bypassLifecycle { return false } - if pathHandler.Before != nil && pathHandler.PathMatcher.MatchesURL(call.URL().Path) { + if pathHandler.PathMatcher.MatchesURL(call.URL().Path) { call.pathParams = pathHandler.PathMatcher.PathParams(call.URL().Path) if !pathHandler.Before(call) { @@ -401,33 +426,23 @@ func (server *App) matchHandlers(call *Call) { // callHandlerByMethod runs the first registered handler for the method whose // path matches the request, and reports whether the request was handled. func (server *App) callHandlerByMethod(call *Call, method string) bool { - for i := range server.pathHandlers { - pathHandler := &server.pathHandlers[i] - - if call.bypassLifecycle { - return true - } - - handler := pathHandler.GetHandlerByMethod(method) - if handler != nil && pathHandler.PathMatcher.MatchesURL(call.URL().Path) { - call.pathParams = pathHandler.PathMatcher.PathParams(call.URL().Path) - handler(call) - - return true - } + pathHandler := server.routes.match(call.URL().Path, method) + if pathHandler == nil { + return false } - return false + call.pathParams = pathHandler.PathMatcher.PathParams(call.URL().Path) + pathHandler.GetHandlerByMethod(method)(call) + + return true } func (server *App) matchAfterHandlers(call *Call) { - for i := range server.pathHandlers { - pathHandler := &server.pathHandlers[i] - + for _, pathHandler := range server.afterHandlers { if call.bypassLifecycle { return } - if pathHandler.After != nil && pathHandler.PathMatcher.MatchesURL(call.URL().Path) { + if pathHandler.PathMatcher.MatchesURL(call.URL().Path) { call.pathParams = pathHandler.PathMatcher.PathParams(call.URL().Path) pathHandler.After(call) } diff --git a/server_test.go b/server_test.go index 51d974d..56dfa8f 100644 --- a/server_test.go +++ b/server_test.go @@ -344,6 +344,40 @@ func TestBefore(t *testing.T) { }) } +// TestLifecycleHandlersRunInRouteTableOrder pins which order overlapping before +// and after handlers run in when one of them is registered on a path an earlier +// route already put in the table: the table's order, not the order the Before +// and After calls were made in. +func TestLifecycleHandlersRunInRouteTableOrder(t *testing.T) { + app := newTestApp() + app.Get("/test", func(call *govalin.Call) { + call.Text(" handler") + }) + app.Before("/*", func(call *govalin.Call) bool { + call.Text(" wildcard-before") + return true + }) + app.Before("/test", func(call *govalin.Call) bool { + call.Text("test-before") + return true + }) + app.After("/*", func(call *govalin.Call) { + call.Text(" wildcard-after") + }) + app.After("/test", func(call *govalin.Call) { + call.Text(" test-after") + }) + + govalintest.Test(t, app, func(client *govalintest.Client) { + assert.Equal( + t, + "test-before wildcard-before handler test-after wildcard-after", + client.Get("/test"), + "Should run the handlers in the order their paths sit in the route table", + ) + }) +} + func TestAfter(t *testing.T) { app := newTestApp() app.Get("/test", func(call *govalin.Call) {