From 8395e956e49f5e97cd8e4264f69c69168792354d Mon Sep 17 00:00:00 2001 From: Chris Banks Date: Sun, 20 Aug 2023 22:52:16 +0100 Subject: [PATCH] Shrink and simplify the route table structures. - Use generics (introduced in Go 1.18) to provide compile-time type checking and eliminate type introspection at runtime. - Drop the unused `prefix` flag in the trie node objects; just point directly to the http.Handler (interface). Improves readability and saves an unnecessary indirection on every lookup. - Make the clever table-driven unit tests a bit more readable by using more common terminology. - Unexport some symbols which shouldn't have been exported. - Minor editorial improvements to documentation. - Update the copyright assertions in the LICENCE files to keep them accurate. --- LICENCE | 5 +- trie/LICENSE | 4 +- trie/README.md | 6 +- trie/trie.go | 141 ++++++++++++++++++++------------------------ trie/trie_test.go | 124 +++++++++++++++++++------------------- triemux/LICENSE | 4 +- triemux/mux.go | 58 +++++++----------- triemux/mux_test.go | 12 ++-- 8 files changed, 166 insertions(+), 188 deletions(-) diff --git a/LICENCE b/LICENCE index 083c4110..3678e01e 100644 --- a/LICENCE +++ b/LICENCE @@ -1,5 +1,6 @@ -The MIT License (MIT) -Copyright (c) 2013 Government Digital Service +MIT Licence + +Copyright © 2013-2023 Crown Copyright (Government Digital Service) 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 diff --git a/trie/LICENSE b/trie/LICENSE index bda38427..c53aaa23 100644 --- a/trie/LICENSE +++ b/trie/LICENSE @@ -1,4 +1,6 @@ -Copyright (c) 2013 Government Digital Service +MIT Licence + +Copyright © 2013, 2023 Crown Copyright (Government Digital Service) 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 diff --git a/trie/README.md b/trie/README.md index 2a4e70bb..dfceec27 100644 --- a/trie/README.md +++ b/trie/README.md @@ -7,8 +7,8 @@ rather than just strings. This makes it suitable for efficiently storing information about hierarchical systems in general, rather than being specifically geared towards string lookup. -Read the documentation on [godoc.org][docs] for details of how to use `trie`. +Read the [documentation] for details of how to use `trie`. -[docs]: http://godoc.org/github.com/alphagov/router/trie -[go]: http://golang.org +[documentation]: https://pkg.go.dev/github.com/alphagov/router/trie +[go]: https://go.dev/ [trie]: https://en.wikipedia.org/wiki/Trie diff --git a/trie/trie.go b/trie/trie.go index 11eda389..67a10c98 100644 --- a/trie/trie.go +++ b/trie/trie.go @@ -1,136 +1,121 @@ // Package trie implements a simple trie data structure that maps "paths" (which -// are slices of strings) to arbitrary data values (type interface{}). +// are slices of strings) to values of some type T. package trie -type trieChildren map[string]*Trie +type trieChildren[T interface{}] map[string]*Trie[T] -type Trie struct { - Leaf bool - Entry interface{} - Children trieChildren +type Trie[T interface{}] struct { + leaf bool + entry T + children trieChildren[T] } -// NewTrie makes a new empty Trie -func NewTrie() *Trie { - return &Trie{ - Children: make(trieChildren), - } +// NewTrie makes a new, empty Trie. +func NewTrie[T interface{}]() *Trie[T] { + return &Trie[T]{children: make(trieChildren[T])} } -// Get retrieves an element from the Trie +// Get retrieves an entry from the Trie. If there is no fully-matching entry, +// Get returns `(nil, false)`. `path` can be empty, to denote the root node. // -// Takes a path (which can be empty, to denote the root element of the Trie), -// and returns the object if the path exists in the Trie, or nil and a status of -// false. Example: +// Example: // -// if res, ok := trie.Get([]string{"foo", "bar"}); ok { -// fmt.Println("Value at /foo/bar was", res) -// } -func (t *Trie) Get(path []string) (entry interface{}, ok bool) { +// if res, ok := trie.Get([]string{"foo", "bar"}); ok { +// fmt.Println("Value at /foo/bar was", res) +// } +func (t *Trie[T]) Get(path []string) (entry T, ok bool) { if len(path) == 0 { - return t.getentry() + return t.getEntry() } - key := path[0] - newpath := path[1:] + key, newPath := path[0], path[1:] - res, ok := t.Children[key] + res, ok := t.children[key] if !ok { - // Path doesn't exist: shortcut return value - return nil, false + return } - - return res.Get(newpath) + return res.Get(newPath) } -// GetLongestPrefix retrieves an element from the Trie +// GetLongestPrefix retrieves the longest matching entry from the Trie. +// +// GetLongestPrefix returns a full match if there is one, or the entry with the +// longest matching prefix. If there is no match at all, GetLongestPrefix +// returns `(nil, false)`. `path` can be empty, to denote the root node. // -// Takes a path (which can be empty, to denote the root element of the Trie). -// If a matching object exists, it is returned. Otherwise the object with the -// longest matching prefix is returned. If nothing matches at all, nil and a -// status of false is returned. Example: +// Example: // -// if res, ok := trie.GetLongestPrefix([]string{"foo", "bar"}); ok { -// fmt.Println("Value at /foo/bar was", res) -// } -func (t *Trie) GetLongestPrefix(path []string) (entry interface{}, ok bool) { +// if res, ok := trie.GetLongestPrefix([]string{"foo", "bar"}); ok { +// fmt.Println("Value at /foo/bar was", res) +// } +func (t *Trie[T]) GetLongestPrefix(path []string) (entry T, ok bool) { if len(path) == 0 { - return t.getentry() + return t.getEntry() } - key := path[0] - newpath := path[1:] + key, newPath := path[0], path[1:] - res, ok := t.Children[key] + res, ok := t.children[key] if !ok { - // Path doesn't exist: return this node as possible best match - return t.getentry() + return t.getEntry() // Full path not found, but this is the longest match. } - entry, ok = res.GetLongestPrefix(newpath) + entry, ok = res.GetLongestPrefix(newPath) if ok { return entry, ok } - // We haven't found a match yet, return this node - return t.getentry() + return t.getEntry() // No match yet, so return this node. } -// Set creates an element in the Trie -// -// Takes a path (which can be empty, to denote the root element of the Trie), -// and an arbitrary value (interface{}) to use as the leaf data. -func (t *Trie) Set(path []string, value interface{}) { +// Set adds an entry to the Trie. `path` can be empty, to denote the root node. +func (t *Trie[T]) Set(path []string, value T) { if len(path) == 0 { - t.setentry(value) + t.setEntry(value) return } - key := path[0] - newpath := path[1:] + key, newPath := path[0], path[1:] - res, ok := t.Children[key] + res, ok := t.children[key] if !ok { - // Trie node that should hold entry doesn't already exist, so let's create it - res = NewTrie() - t.Children[key] = res + res = NewTrie[T]() + t.children[key] = res } - res.Set(newpath, value) + res.Set(newPath, value) } -// Del removes an element from the Trie. Returns a boolean indicating whether an -// element was actually deleted. -func (t *Trie) Del(path []string) bool { +// Del removes an entry from the Trie, returning true if it deleted an entry. +func (t *Trie[T]) Del(path []string) bool { if len(path) == 0 { - return t.delentry() + return t.delEntry() } - key := path[0] - newpath := path[1:] + key, newPath := path[0], path[1:] - res, ok := t.Children[key] + res, ok := t.children[key] if !ok { return false } - - return res.Del(newpath) + return res.Del(newPath) } -func (t *Trie) setentry(value interface{}) { - t.Leaf = true - t.Entry = value +func (t *Trie[T]) setEntry(value T) { + t.leaf = true + t.entry = value } -func (t *Trie) getentry() (entry interface{}, ok bool) { - if t.Leaf { - return t.Entry, true +func (t *Trie[T]) getEntry() (entry T, ok bool) { + if t.leaf { + return t.entry, true } - return nil, false + return } -func (t *Trie) delentry() (ok bool) { - ok = t.Leaf - t.Leaf = false - t.Entry = nil +func (t *Trie[T]) delEntry() (ok bool) { + ok = t.leaf + t.leaf = false + var zero T + t.entry = zero return } diff --git a/trie/trie_test.go b/trie/trie_test.go index bf6dfa8c..32c282b3 100644 --- a/trie/trie_test.go +++ b/trie/trie_test.go @@ -4,52 +4,54 @@ import ( "testing" ) -type Meth int +type method int const ( - Set Meth = iota + Set method = iota Del ) -type Pair struct { - meth Meth - path []string - val interface{} +type FixtureRow struct { + method method + path []string + val interface{} } -type Check struct { +type Expectation struct { path []string val interface{} ok bool } type Example struct { - pairs []Pair - checks []Check + fixtures []FixtureRow + expectations []Expectation } -// Data driven testing. Each Example consists of a slice of Pairs which are -// "set" in the Trie, and a set of Checks, which consist of a Trie path and -// expected return values and "ok" checks. +// In these table-driven tests, each Example consists of: +// +// - a slice of FixtureRows which are inserted into the Trie +// - a set of Expectations, which consist of a Trie path and expected return +// values and "ok" checks // -// TestExamples iterates through the examples slice and runs all the Checks -// having set up a Trie with the appropriate Pairs. +// TestExamples iterates over each test case in `examples`, setting up the +// fixture and asserting the corresponding Expectations. var examples = []Example{ { // Simple setting and getting - []Pair{ + []FixtureRow{ {Set, []string{"foo"}, "bar"}, }, - []Check{ + []Expectation{ {[]string{"foo"}, "bar", true}, {[]string{"baz"}, nil, false}, {[]string{"foo", "bar"}, nil, false}, }, }, { // Multiple path components - []Pair{ + []FixtureRow{ {Set, []string{"foo", "bar"}, 123}, }, - []Check{ + []Expectation{ {[]string{"foo", "bar"}, 123, true}, {[]string{"baz"}, nil, false}, {[]string{"foo", "baz"}, nil, false}, @@ -58,11 +60,11 @@ var examples = []Example{ }, }, { // Multiple values on the same path - []Pair{ + []FixtureRow{ {Set, []string{"foo"}, "hello"}, {Set, []string{"foo", "bar"}, 123}, }, - []Check{ + []Expectation{ {[]string{"foo", "bar"}, 123, true}, {[]string{"foo"}, "hello", true}, {[]string{"foo", "baz"}, nil, false}, @@ -71,27 +73,27 @@ var examples = []Example{ }, }, { // Deleting - []Pair{ + []FixtureRow{ {Set, []string{"foo"}, "hello"}, {Del, []string{"foo"}, nil}, }, - []Check{ + []Expectation{ {[]string{"foo"}, nil, false}, }, }, { // Setting at the root - []Pair{ + []FixtureRow{ {Set, []string{}, "hello"}, }, - []Check{ + []Expectation{ {[]string{}, "hello", true}, }, }, { // Setting nil - []Pair{ + []FixtureRow{ {Set, []string{"foo"}, nil}, }, - []Check{ + []Expectation{ {[]string{"foo"}, nil, true}, }, }, @@ -99,11 +101,11 @@ var examples = []Example{ var prefixExamples = []Example{ { // Multiple values on the same path - []Pair{ + []FixtureRow{ {Set, []string{"foo"}, "hello"}, {Set, []string{"foo", "bar"}, 123}, }, - []Check{ + []Expectation{ {[]string{"foo", "bar"}, 123, true}, {[]string{"foo"}, "hello", true}, {[]string{"foo", "baz"}, "hello", true}, @@ -113,11 +115,11 @@ var prefixExamples = []Example{ }, }, { // Multiple values on the same path with gaps - []Pair{ + []FixtureRow{ {Set, []string{"foo"}, "hello"}, {Set, []string{"foo", "bar", "baz"}, 123}, }, - []Check{ + []Expectation{ {[]string{"foo", "bar", "baz"}, 123, true}, {[]string{"foo"}, "hello", true}, {[]string{"foo", "baz"}, "hello", true}, @@ -127,12 +129,12 @@ var prefixExamples = []Example{ }, }, { // Deleting - []Pair{ + []FixtureRow{ {Set, []string{"foo"}, "hello"}, {Set, []string{"foo", "bar", "baz"}, 123}, {Del, []string{"foo", "bar", "baz"}, nil}, }, - []Check{ + []Expectation{ {[]string{"foo", "bar", "baz"}, "hello", true}, }, }, @@ -140,7 +142,7 @@ var prefixExamples = []Example{ func TestNew(t *testing.T) { path := []string{"hello", "world"} - trie := NewTrie() + trie := NewTrie[interface{}]() _, ok := trie.Get(path) if ok { t.Error("An empty Trie should not contain any entries") @@ -154,15 +156,15 @@ func TestExamples(t *testing.T) { } func testExample(t *testing.T, i int, ex Example) { - trie := buildExampleTrie(t, ex.pairs) - for _, c := range ex.checks { - val, ok := trie.Get(c.path) - t.Logf("trie.Get(path:%v) -> val:%v, ok:%v", c.path, val, ok) - if ok != c.ok { - t.Errorf("Example %d check %+v: trie.Get ok was %v (expected %v)", i, c, ok, c.ok) + trie := buildExampleTrie(t, ex.fixtures) + for _, e := range ex.expectations { + val, ok := trie.Get(e.path) + t.Logf("trie.Get(path:%v) -> val:%v, ok:%v", e.path, val, ok) + if ok != e.ok { + t.Errorf("Example %d check %+v: trie.Get ok was %v (expected %v)", i, e, ok, e.ok) } - if val != c.val { - t.Errorf("Example %d check %+v: trie.Get val was %v (expected %v)", i, c, val, c.val) + if val != e.val { + t.Errorf("Example %d check %+v: trie.Get val was %v (expected %v)", i, e, val, e.val) } } } @@ -174,45 +176,45 @@ func TestPrefixExamples(t *testing.T) { } func testPrefixExample(t *testing.T, i int, ex Example) { - trie := buildExampleTrie(t, ex.pairs) - for _, c := range ex.checks { - val, ok := trie.GetLongestPrefix(c.path) - t.Logf("trie.GetLongestPrefix(path:%v) -> val:%v, ok:%v", c.path, val, ok) - if ok != c.ok { - t.Errorf("Example %d check %+v: trie.Get ok was %v (expected %v)", i, c, ok, c.ok) + trie := buildExampleTrie(t, ex.fixtures) + for _, e := range ex.expectations { + val, ok := trie.GetLongestPrefix(e.path) + t.Logf("trie.GetLongestPrefix(path:%v) -> val:%v, ok:%v", e.path, val, ok) + if ok != e.ok { + t.Errorf("Example %d check %+v: trie.Get ok was %v (expected %v)", i, e, ok, e.ok) } - if val != c.val { - t.Errorf("Example %d check %+v: trie.Get val was %v (expected %v)", i, c, val, c.val) + if val != e.val { + t.Errorf("Example %d check %+v: trie.Get val was %v (expected %v)", i, e, val, e.val) } } } func TestDelReturnsStatus(t *testing.T) { - trie := NewTrie() + trie := NewTrie[interface{}]() path := []string{"foo"} trie.Set(path, "bar") ok := trie.Del(path) if !ok { - t.Error("trie.Del didn't return true when deleting a key that exists!") + t.Error("trie.Del didn't return true when deleting a key that exists") } ok = trie.Del(path) if ok { - t.Error("trie.Del didn't return false when deleting a key that didn't exist!") + t.Error("trie.Del didn't return false when deleting a key that didn't exist") } } -func buildExampleTrie(t *testing.T, pairs []Pair) *Trie { - trie := NewTrie() - for _, p := range pairs { - switch p.meth { +func buildExampleTrie(t *testing.T, fixtures []FixtureRow) *Trie[interface{}] { + trie := NewTrie[interface{}]() + for _, f := range fixtures { + switch f.method { case Set: - trie.Set(p.path, p.val) - t.Logf("trie.Set(path:%v, val:%v)", p.path, p.val) + trie.Set(f.path, f.val) + t.Logf("trie.Set(path:%v, val:%v)", f.path, f.val) case Del: - trie.Del(p.path) - t.Logf("trie.Del(path:%v)", p.path) + trie.Del(f.path) + t.Logf("trie.Del(path:%v)", f.path) default: - t.Errorf("Unrecognised method %v in Pair %v", p.meth, p) + t.Errorf("Unrecognised method %v in FixtureRow %v", f.method, f) } } return trie diff --git a/triemux/LICENSE b/triemux/LICENSE index bda38427..c53aaa23 100644 --- a/triemux/LICENSE +++ b/triemux/LICENSE @@ -1,4 +1,6 @@ -Copyright (c) 2013 Government Digital Service +MIT Licence + +Copyright © 2013, 2023 Crown Copyright (Government Digital Service) 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 diff --git a/triemux/mux.go b/triemux/mux.go index da2f9295..a669c9b0 100644 --- a/triemux/mux.go +++ b/triemux/mux.go @@ -4,7 +4,6 @@ package triemux import ( - "log" "net/http" "strings" "sync" @@ -15,19 +14,17 @@ import ( type Mux struct { mu sync.RWMutex - exactTrie *trie.Trie - prefixTrie *trie.Trie + exactTrie *trie.Trie[http.Handler] + prefixTrie *trie.Trie[http.Handler] count int } -type muxEntry struct { - prefix bool - handler http.Handler -} - // NewMux makes a new empty Mux. func NewMux() *Mux { - return &Mux{exactTrie: trie.NewTrie(), prefixTrie: trie.NewTrie()} + return &Mux{ + exactTrie: trie.NewTrie[http.Handler](), + prefixTrie: trie.NewTrie[http.Handler](), + } } // ServeHTTP dispatches the request to a backend with a registered route @@ -50,59 +47,48 @@ func (mux *Mux) ServeHTTP(w http.ResponseWriter, r *http.Request) { http.NotFound(w, r) return } - handler.ServeHTTP(w, r) } -// lookup takes a path and looks up its registered entry in the mux trie, -// returning the handler for that path, if any matches. +// lookup finds a URL path in the Mux and returns the corresponding handler. func (mux *Mux) lookup(path string) (handler http.Handler, ok bool) { mux.mu.RLock() defer mux.mu.RUnlock() - pathSegments := splitpath(path) - val, ok := mux.exactTrie.Get(pathSegments) - if !ok { - val, ok = mux.prefixTrie.GetLongestPrefix(pathSegments) + pathSegments := splitPath(path) + if handler, ok = mux.exactTrie.Get(pathSegments); !ok { + handler, ok = mux.prefixTrie.GetLongestPrefix(pathSegments) } if !ok { entryNotFoundCountMetric.Inc() return nil, false } - - entry, ok := val.(muxEntry) - if !ok { - log.Printf("lookup: got value (%v) from trie that wasn't a muxEntry!", val) - entryNotFoundCountMetric.Inc() - return nil, false - } - - return entry.handler, ok + return } -// Handle registers the specified route (either an exact or a prefix route) -// and associates it with the specified handler. Requests through the mux for -// paths matching the route will be passed to that handler. +// Handle adds a route (either an exact path or a path prefix) to the Mux and +// and associates it with a handler, so that the Mux will pass matching +// requests to that handler. func (mux *Mux) Handle(path string, prefix bool, handler http.Handler) { mux.mu.Lock() defer mux.mu.Unlock() - mux.count++ + t := mux.exactTrie if prefix { - mux.prefixTrie.Set(splitpath(path), muxEntry{prefix, handler}) - } else { - mux.exactTrie.Set(splitpath(path), muxEntry{prefix, handler}) + t = mux.prefixTrie } + t.Set(splitPath(path), handler) + mux.count++ } func (mux *Mux) RouteCount() int { return mux.count } -// splitpath turns a slash-delimited string into a lookup path (a slice -// containing the strings between slashes). Empty items produced by -// leading, trailing, or adjacent slashes are removed. -func splitpath(path string) []string { +// splitPath turns a slash-delimited string into a lookup path (a slice +// containing the strings between slashes). splitPath omits empty items +// produced by leading, trailing, or adjacent slashes. +func splitPath(path string) []string { partsWithBlanks := strings.Split(path, "/") parts := make([]string, 0, len(partsWithBlanks)) diff --git a/triemux/mux_test.go b/triemux/mux_test.go index 6452b9be..209a1265 100644 --- a/triemux/mux_test.go +++ b/triemux/mux_test.go @@ -29,20 +29,20 @@ var splitExamples = []SplitExample{ {"/foo/////bar/", []string{"foo", "bar"}}, } -func TestSplitpath(t *testing.T) { +func TestSplitPath(t *testing.T) { for _, ex := range splitExamples { - testSplitpath(t, ex) + testSplitPath(t, ex) } } -func testSplitpath(t *testing.T, ex SplitExample) { - out := splitpath(ex.in) +func testSplitPath(t *testing.T, ex SplitExample) { + out := splitPath(ex.in) if len(out) != len(ex.out) { - t.Errorf("splitpath(%v) was not %v", ex.in, ex.out) + t.Errorf("splitPath(%v) was not %v", ex.in, ex.out) } for i := range ex.out { if out[i] != ex.out[i] { - t.Errorf("splitpath(%v) differed from %v at component %d "+ + t.Errorf("splitPath(%v) differed from %v at component %d "+ "(expected %v, got %v)", out, ex.out, i, ex.out[i], out[i]) } }