From 30ef30e8334d5954cbfda37d1b51c569141bd09d Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Mon, 16 Oct 2023 21:08:31 -0700 Subject: [PATCH] Simplify the implementation Switch back to http.ServeMux for package handlers, and move the scanning logic to the index handler. --- go.mod | 1 - go.sum | 2 - handler.go | 169 ++++++++++++++++---------- handler_test.go | 87 ++++++++++++- path_tree.go | 120 ------------------ path_tree_test.go | 304 ---------------------------------------------- 6 files changed, 189 insertions(+), 494 deletions(-) delete mode 100644 path_tree.go delete mode 100644 path_tree_test.go diff --git a/go.mod b/go.mod index 3d1c17d..91027f4 100644 --- a/go.mod +++ b/go.mod @@ -8,7 +8,6 @@ require ( github.com/stretchr/testify v1.8.4 golang.org/x/net v0.15.0 gopkg.in/yaml.v3 v3.0.1 - pgregory.net/rapid v1.1.0 ) require ( diff --git a/go.sum b/go.sum index d3d2117..e4ac672 100644 --- a/go.sum +++ b/go.sum @@ -16,5 +16,3 @@ gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127 h1:qIbj1fsPNlZgppZ+VLlY7N33 gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= -pgregory.net/rapid v1.1.0 h1:CMa0sjHSru3puNx+J0MIAuiiEV4N0qj8/cMWGBBCsjw= -pgregory.net/rapid v1.1.0/go.mod h1:PY5XlDGj0+V1FCq0o192FdRhpKHGTRIWBgqjDBTrq04= diff --git a/handler.go b/handler.go index 2540a98..f80ecdf 100644 --- a/handler.go +++ b/handler.go @@ -1,11 +1,12 @@ package main import ( - "errors" + "cmp" "fmt" "html/template" "net/http" "path" + "slices" "strings" "go.uber.org/sally/templates" @@ -18,27 +19,22 @@ var ( template.New("package.html").Parse(templates.Package)) ) -// Handler handles inbound HTTP requests. -// -// It provides the following endpoints: +// CreateHandler builds a new handler +// with the provided package configuration. +// The returned handler provides the following endpoints: // // GET / // Index page listing all packages. // GET / -// Package page for the given package. +// Package page for the given package. // GET / // Page listing packages under the given directory, // assuming that there's no package with the given name. // GET // // Package page for the given subpackage. -type Handler struct { - pkgs pathTree[*sallyPackage] -} - -// CreateHandler builds a new handler -// with the provided package configuration. -func CreateHandler(config *Config) *Handler { - var pkgs pathTree[*sallyPackage] +func CreateHandler(config *Config) http.Handler { + mux := http.NewServeMux() + pkgs := make([]*sallyPackage, 0, len(config.Packages)) for name, pkg := range config.Packages { baseURL := config.URL if pkg.URL != "" { @@ -48,22 +44,43 @@ func CreateHandler(config *Config) *Handler { modulePath := path.Join(baseURL, name) docURL := "https://" + path.Join(config.Godoc.Host, modulePath) - pkgs.Set(name, &sallyPackage{ + pkg := &sallyPackage{ + Name: name, Desc: pkg.Desc, ModulePath: modulePath, DocURL: docURL, GitURL: pkg.Repo, - }) - } + } + pkgs = append(pkgs, pkg) - return &Handler{ - pkgs: pkgs, + // Double-register so that "/foo" + // does not redirect to "/foo/" with a 300. + handler := &packageHandler{Pkg: pkg} + mux.Handle("/"+name, handler) + mux.Handle("/"+name+"/", handler) } + + mux.Handle("/", newIndexHandler(pkgs)) + return requireMethod(http.MethodGet, mux) } -var _ http.Handler = (*Handler)(nil) +func requireMethod(method string, handler http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != method { + http.NotFound(w, r) + return + } + + handler.ServeHTTP(w, r) + }) +} type sallyPackage struct { + // Name of the package. + // + // This is the part after the base URL. + Name string + // Canonical import path for the package. ModulePath string @@ -77,72 +94,94 @@ type sallyPackage struct { GitURL string } -func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { - if err := h.serveHTTP(w, r); err != nil { - if herr := new(httpError); errors.As(err, &herr) { - http.Error(w, herr.Message, herr.Code) - } else { - http.Error(w, err.Error(), 500) - } - } +type indexHandler struct { + pkgs []*sallyPackage // sorted by name } -// httpError indicates that an HTTP error occurred. -// -// The caller will write the error code and message to the response. -type httpError struct { - Code int // HTTP status code - Message string // error message -} +var _ http.Handler = (*indexHandler)(nil) + +func newIndexHandler(pkgs []*sallyPackage) *indexHandler { + slices.SortFunc(pkgs, func(a, b *sallyPackage) int { + return cmp.Compare(a.Name, b.Name) + }) -func httpErrorf(code int, format string, args ...interface{}) error { - return &httpError{ - Code: code, - Message: fmt.Sprintf(format, args...), + return &indexHandler{ + pkgs: pkgs, } } -func (e *httpError) Error() string { - return fmt.Sprintf("status %d: %s", e.Code, e.Message) -} +func (h *indexHandler) rangeOf(path string) (start, end int) { + if len(path) == 0 { + return 0, len(h.pkgs) + } -// serveHTTP is similar to ServeHTTP, except it returns an error. -// -// If it returns an httpError, -// the caller will write the error code and message to the response. -func (h *Handler) serveHTTP(w http.ResponseWriter, r *http.Request) error { - if r.Method != http.MethodGet { - return httpErrorf(http.StatusNotFound, "method %q not allowed", r.Method) + // If the packages are sorted by name, + // we can scan adjacent packages to find the range of packages + // whose name descends from path. + start, _ = slices.BinarySearchFunc(h.pkgs, path, func(pkg *sallyPackage, path string) int { + return cmp.Compare(pkg.Name, path) + }) + + for idx := start; idx < len(h.pkgs); idx++ { + if !descends(path, h.pkgs[idx].Name) { + // End of matching sequences. + // The next path is not a descendant of path. + return start, idx + } } + // All packages following start are descendants of path. + // Return the rest of the packages. + return start, len(h.pkgs) +} + +func (h *indexHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { path := strings.TrimPrefix(strings.TrimSuffix(r.URL.Path, "/"), "/") + start, end := h.rangeOf(path) - if pkg, suffix, ok := h.pkgs.Lookup(path); ok { - return h.servePackage(w, pkg, suffix) + // If start == end, then there are no packages + if start == end { + w.WriteHeader(http.StatusNotFound) + fmt.Fprintf(w, "no packages found under path: %v\n", path) + return + } + + err := indexTemplate.Execute(w, + struct{ Packages []*sallyPackage }{ + Packages: h.pkgs[start:end], + }) + if err != nil { + http.Error(w, err.Error(), 500) } - return h.serveIndex(w, path, h.pkgs.ListByPath(path)) } -func (h *Handler) servePackage(w http.ResponseWriter, pkg *sallyPackage, suffix string) error { - return packageTemplate.Execute(w, +type packageHandler struct { + Pkg *sallyPackage +} + +var _ http.Handler = (*packageHandler)(nil) + +func (h *packageHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { + // Extract the relative path to subpackages, if any. + // "/foo/bar" => "/bar" + // "/foo" => "" + relPath := strings.TrimPrefix(r.URL.Path, "/"+h.Pkg.Name) + + err := packageTemplate.Execute(w, struct { ModulePath string GitURL string DocURL string }{ - ModulePath: pkg.ModulePath, - GitURL: pkg.GitURL, - DocURL: pkg.DocURL + suffix, + ModulePath: h.Pkg.ModulePath, + GitURL: h.Pkg.GitURL, + DocURL: h.Pkg.DocURL + relPath, }) -} - -func (h *Handler) serveIndex(w http.ResponseWriter, path string, pkgs []*sallyPackage) error { - if len(pkgs) == 0 { - return httpErrorf(http.StatusNotFound, "no packages found under path: %s", path) + if err != nil { + http.Error(w, err.Error(), 500) } +} - return indexTemplate.Execute(w, - struct{ Packages []*sallyPackage }{ - Packages: pkgs, - }) +func descends(from, to string) bool { + return to == from || (strings.HasPrefix(to, from) && to[len(from)] == '/') } diff --git a/handler_test.go b/handler_test.go index a95a1f8..c6470f9 100644 --- a/handler_test.go +++ b/handler_test.go @@ -4,6 +4,7 @@ import ( "io" "net/http" "net/http/httptest" + "sort" "strings" "testing" @@ -83,10 +84,10 @@ func TestTrailingSlash(t *testing.T) { - + - Nothing to see here. Please move along. + Nothing to see here. Please move along. `) @@ -179,6 +180,88 @@ func TestPostRejected(t *testing.T) { } } +func TestIndexHandler_rangeOf(t *testing.T) { + tests := []struct { + desc string + pkgs []*sallyPackage + path string + want []string // names + }{ + { + desc: "empty", + pkgs: []*sallyPackage{ + {Name: "foo"}, + {Name: "bar"}, + }, + want: []string{"foo", "bar"}, + }, + { + desc: "single child", + pkgs: []*sallyPackage{ + {Name: "foo/bar"}, + {Name: "baz"}, + }, + path: "foo", + want: []string{"foo/bar"}, + }, + { + desc: "multiple children", + pkgs: []*sallyPackage{ + {Name: "foo/bar"}, + {Name: "foo/baz"}, + {Name: "qux"}, + {Name: "quux/quuz"}, + }, + path: "foo", + want: []string{"foo/bar", "foo/baz"}, + }, + { + desc: "to end of list", + pkgs: []*sallyPackage{ + {Name: "a"}, + {Name: "b"}, + {Name: "c/d"}, + {Name: "c/e"}, + }, + path: "c", + want: []string{"c/d", "c/e"}, + }, + { + desc: "similar name", + pkgs: []*sallyPackage{ + {Name: "foobar"}, + {Name: "foo/bar"}, + }, + path: "foo", + want: []string{"foo/bar"}, + }, + { + desc: "no match", + pkgs: []*sallyPackage{ + {Name: "foo"}, + {Name: "bar"}, + }, + path: "baz", + }, + } + + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + h := newIndexHandler(tt.pkgs) + start, end := h.rangeOf(tt.path) + + var got []string + for _, pkg := range tt.pkgs[start:end] { + got = append(got, pkg.Name) + } + sort.Strings(got) + sort.Strings(tt.want) + + assert.Equal(t, tt.want, got) + }) + } +} + func BenchmarkHandlerDispatch(b *testing.B) { handler := CreateHandler(&Config{ URL: "go.uberalt.org", diff --git a/path_tree.go b/path_tree.go deleted file mode 100644 index b6c4855..0000000 --- a/path_tree.go +++ /dev/null @@ -1,120 +0,0 @@ -package main - -import ( - "slices" - "strings" -) - -// pathTree holds values in a tree-like hierarchy -// defined by /-separate paths (e.g. import paths). -// -// It's expensive to add items to the tree, -// but lookups are fast. -// -// Its zero value is a valid empty tree. -type pathTree[T any] struct { - // We track two representations of the tree: - // - // 1. A list of key-value pairs, sorted by key. - // 2. A map of keys to values. - // - // (1) is used for fast scanning of all descendants of a path. - // - // (2) is used for fast lookups of specific paths. - - // paths is a list of all paths in the tree - // that have a value set for them. - paths []string // sorted - - // values[i] is the value for paths[i] - values []T - - // byPath is a map of all paths in the tree - // to their corresponding values. - byPath map[string]T // path => node -} - -// Set sets the value for path to value. -// If path already has a value, it is overwritten. -func (t *pathTree[T]) Set(path string, value T) { - if t.byPath == nil { - t.byPath = make(map[string]T) - } - - t.byPath[path] = value - idx, ok := slices.BinarySearch(t.paths, path) - if ok { - // t.paths[idx] already contains path. - t.values[idx] = value - } else { - t.paths = slices.Insert(t.paths, idx, path) - t.values = slices.Insert(t.values, idx, value) - } -} - -// Lookup retrieves the value for the given path. -// -// If the path doesn't have an explicit value set, -// the value for the closest ancestor with a value is returned. -// -// Suffix is the remaining, unmatched part of path. -// It has a leading '/' if the path wasn't an exact match. -// -// If no value is set for the path or its ancestors, -// ok is false. -func (t *pathTree[T]) Lookup(path string) (value T, suffix string, ok bool) { - idx := len(path) - for idx > 0 { - if value, ok := t.byPath[path[:idx]]; ok { - return value, path[idx:], true - } - - // No match. Trim the last path component. - // "foo/bar" => "foo" - // "foo" => "" - idx = strings.LastIndexByte(path[:idx], '/') - } - - return value, "", false -} - -// ListByPath returns all descendants of path in the tree, -// including the path itself if it exists. -// -// The values are returned in an unspecified order. -func (t *pathTree[T]) ListByPath(path string) []T { - start, end := t.rangeOf(path) - if start == end { - return nil - } - - descendants := make([]T, end-start) - for i := start; i < end; i++ { - descendants[i-start] = t.values[i] - } - return descendants -} - -func (t *pathTree[T]) rangeOf(path string) (start, end int) { - if len(path) == 0 { - return 0, len(t.paths) - } - - start, _ = slices.BinarySearch(t.paths, path) - for idx := start; idx < len(t.paths); idx++ { - if descends(path, t.paths[idx]) { - continue // path is an ancestor of p - } - - // End of matching sequences. - // The next path is not a descendant of path. - return start, idx - } - - // All paths following start are descendants of path. - return start, len(t.paths) -} - -func descends(from, to string) bool { - return to == from || (strings.HasPrefix(to, from) && to[len(from)] == '/') -} diff --git a/path_tree_test.go b/path_tree_test.go deleted file mode 100644 index e5303de..0000000 --- a/path_tree_test.go +++ /dev/null @@ -1,304 +0,0 @@ -package main - -import ( - "fmt" - "math/rand" - "slices" - "strconv" - "strings" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "pgregory.net/rapid" -) - -func TestPathTree_empty(t *testing.T) { - var tree pathTree[int] - - _, _, ok := tree.Lookup("") - assert.False(t, ok) - - _, _, ok = tree.Lookup("foo") - assert.False(t, ok) - - assert.Empty(t, tree.ListByPath("")) -} - -func TestPathTree(t *testing.T) { - var tree pathTree[int] - mustHave := func(path string, want int, wantSuffix string) { - t.Helper() - - v, suffix, ok := tree.Lookup(path) - require.True(t, ok, "path %q", path) - assert.Equal(t, v, want, "path %q", path) - assert.Equal(t, wantSuffix, suffix, "path %q", path) - } - - mustNotHave := func(path string) { - t.Helper() - - _, _, ok := tree.Lookup(path) - require.False(t, ok, "path %q", path) - } - - mustList := func(path string, want ...int) { - t.Helper() - slices.Sort(want) - - got := tree.ListByPath(path) - slices.Sort(got) - - assert.Equal(t, want, got, "path %q", path) - } - - tree.Set("foo", 10) - t.Run("single", func(t *testing.T) { - mustHave("foo", 10, "") - mustHave("foo/bar", 10, "/bar") - mustHave("foo/bar/baz", 10, "/bar/baz") - mustNotHave("") - mustNotHave("bar") - mustNotHave("bar/baz") - - t.Run("list", func(t *testing.T) { - mustList("", 10) - mustList("foo", 10) - mustList("foo/bar") - }) - }) - - // Override a descendant value. - t.Run("descendant", func(t *testing.T) { - tree.Set("foo/bar", 20) - mustHave("foo", 10, "") - mustHave("foo/bar", 20, "") - mustHave("foo/bar/baz", 20, "/baz") - - t.Run("list", func(t *testing.T) { - mustList("", 10, 20) - mustList("foo", 10, 20) - mustList("foo/bar", 20) - mustList("foo/bar/baz") - }) - }) - - // Add a sibling. - t.Run("sibling", func(t *testing.T) { - tree.Set("bar", 30) - mustHave("bar", 30, "") - mustHave("bar/baz", 30, "/baz") - - t.Run("list", func(t *testing.T) { - mustList("", 10, 20, 30) - mustList("foo", 10, 20) - mustList("bar", 30) - mustList("bar/baz") - }) - }) - - // Replace an existing value. - t.Run("replace", func(t *testing.T) { - tree.Set("bar", 40) - mustHave("bar", 40, "") - mustHave("bar/baz", 40, "/baz") - - t.Run("list", func(t *testing.T) { - mustList("", 10, 20, 40) - mustList("foo", 10, 20) - mustList("bar", 40) - mustList("bar/baz") - }) - }) -} - -func TestPathTreeRapid(t *testing.T) { - pathGen := rapid.StringMatching(`[a-z]+(/[a-z]+)*`) - valueGen := rapid.Int() - - rapid.Check(t, func(t *rapid.T) { - var tree pathTree[int] - - // Exact lookup table. - exact := make(map[string]int) - var paths []string // known paths - - knownPathGen := rapid.Deferred(func() *rapid.Generator[string] { - return rapid.SampledFrom(paths) - }) - - drawKnownPath := func(t *rapid.T) string { - if len(paths) == 0 { - t.Skip() - } - return knownPathGen.Draw(t, "knownPath") - } - - t.Repeat(map[string]func(*rapid.T){ - "Set": func(t *rapid.T) { - path := pathGen.Draw(t, "path") - if _, ok := exact[path]; ok { - // Already set. - // Overwrite will handle this. - t.Skip() - } - - value := valueGen.Draw(t, "value") - tree.Set(path, value) - exact[path] = value - paths = append(paths, path) - }, - "Overwrite": func(t *rapid.T) { - path := drawKnownPath(t) - value := valueGen.Draw(t, "value") - - tree.Set(path, value) - exact[path] = value - // paths already contains path. - }, - "ExactLookup": func(t *rapid.T) { - if len(paths) == 0 { - t.Skip() - } - - path := drawKnownPath(t) - want := exact[path] - - got, suffix, ok := tree.Lookup(path) - assert.True(t, ok, "path %q", path) - assert.Equal(t, want, got, "path %q", path) - assert.Empty(t, suffix, "path %q", path) - }, - "DescendantLookup": func(t *rapid.T) { - parentPath := drawKnownPath(t) - want := exact[parentPath] - - var childPath string - for { - childPath = "/" + pathGen.Draw(t, "childPath") - if _, ok := exact[parentPath+childPath]; !ok { - break // found a unique child path - } - } - - path := parentPath + childPath - got, suffix, ok := tree.Lookup(path) - assert.True(t, ok, "path %q", path) - assert.Equal(t, want, got, "path %q", path) - assert.Equal(t, childPath, suffix, "path %q", path) - }, - "ListAll": func(t *rapid.T) { - var want []int - for _, v := range exact { - want = append(want, v) - } - slices.Sort(want) - - got := tree.ListByPath("") - slices.Sort(got) - - assert.Equal(t, want, got) - }, - "ListSubset": func(t *rapid.T) { - path := drawKnownPath(t) - - var want []int - for p, v := range exact { - if descends(path, p) { - want = append(want, v) - } - } - slices.Sort(want) - - got := tree.ListByPath(path) - slices.Sort(got) - - if len(want) == 0 { - // Guard against nil != empty. - assert.Empty(t, got, "path %q", path) - } else { - assert.Equal(t, want, got, "path %q", path) - } - }, - }) - }) -} - -func BenchmarkPathTreeDeep(b *testing.B) { - depths := []int{10, 100} - widths := []int{10, 100} - for _, depth := range depths { - b.Run(fmt.Sprintf("depth=%d", depth), func(b *testing.B) { - for _, width := range widths { - b.Run(fmt.Sprintf("width=%d", width), func(b *testing.B) { - benchmarkPathTree(b, depth, width) - }) - } - }) - } -} - -func benchmarkPathTree(b *testing.B, Depth, Width int) { - var ( - tree pathTree[int] - depthpb strings.Builder - ) - paths := make([]string, 0, Depth*Width) - for i := 0; i < Depth; i++ { - if depthpb.Len() > 0 { - depthpb.WriteByte('/') - } - depthpb.WriteString("a") - - depthPath := depthpb.String() - for j := 0; j < Width; j++ { - path := depthPath + "/" + strconv.Itoa(j) - paths = append(paths, path) - tree.Set(path, i+i) - } - } - - b.ResetTimer() - - b.Run("LookupExact", func(b *testing.B) { - path := paths[rand.Intn(len(paths))] - b.RunParallel(func(pb *testing.PB) { - for pb.Next() { - _, _, ok := tree.Lookup(path) - require.True(b, ok) - } - }) - }) - - b.Run("LookupDescendant", func(b *testing.B) { - path := paths[rand.Intn(len(paths))] + strings.Repeat("/xyz", 10) - b.RunParallel(func(pb *testing.PB) { - for pb.Next() { - _, _, ok := tree.Lookup(path) - require.True(b, ok) - } - }) - }) - - b.Run("ListSubtree", func(b *testing.B) { - path := paths[rand.Intn(len(paths))] - b.RunParallel(func(pb *testing.PB) { - for pb.Next() { - if len(tree.ListByPath(path)) == 0 { - b.Fatal("unexpected empty list") - } - } - }) - }) - - b.Run("ListAll", func(b *testing.B) { - b.RunParallel(func(pb *testing.PB) { - for pb.Next() { - if len(tree.ListByPath("")) == 0 { - b.Fatal("unexpected empty list") - } - } - }) - }) -}