internal/frontend: refactor details handlers
At the moment, there are three handlers for serving the details page,
which are now combined into one. This reduces duplicated logic, and
makes it easier to support logic for requests to path@master.
Change-Id: Iedde7430ff5106ea97a4c062f9e83123fa12c5a5
Reviewed-on: https://team-review.git.corp.google.com/c/golang/discovery/+/769963
CI-Result: Cloud Build <devtools-proctor-result-processor@system.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
diff --git a/internal/frontend/details.go b/internal/frontend/details.go
index 149a6a7..138de81 100644
--- a/internal/frontend/details.go
+++ b/internal/frontend/details.go
@@ -36,16 +36,66 @@
PageType string
}
-func (s *Server) serveDetails(w http.ResponseWriter, r *http.Request) error {
+// serveDetails handles requests for package/directory/module details pages. It
+// expects paths of the form "[/mod]/<module-path>[@<version>?tab=<tab>]".
+// stdlib module pages are handled at "/std", and requests to "/mod/std" will
+// be redirected to that path.
+func (s *Server) serveDetails(w http.ResponseWriter, r *http.Request) (err error) {
if r.URL.Path == "/" {
s.staticPageHandler("index.tmpl", "")(w, r)
return nil
}
- parts := strings.SplitN(strings.TrimPrefix(r.URL.Path, "/"), "@", 2)
- if stdlib.Contains(parts[0]) {
- return s.serveStdLib(w, r)
+ if r.URL.Path == "/C" {
+ // Package "C" is a special case: redirect to the Go Blog article on cgo.
+ // (This is what godoc.org does.)
+ http.Redirect(w, r, "https://golang.org/doc/articles/c_go_cgo.html", http.StatusMovedPermanently)
+ return nil
}
- return s.servePackageDetails(w, r)
+ if r.URL.Path == "/mod/std" {
+ // The stdlib module page is hosted at "/std".
+ http.Redirect(w, r, "/std", http.StatusMovedPermanently)
+ return nil
+ }
+
+ var (
+ fullPath, modulePath, requestedVersion string
+ isModule bool
+ urlPath = r.URL.Path
+ )
+ if strings.HasPrefix(r.URL.Path, "/mod") {
+ urlPath = strings.TrimPrefix(r.URL.Path, "/mod")
+ isModule = true
+ }
+
+ // Parse the fullPath, modulePath and requestedVersion, based on whether
+ // the path is in the stdlib. If unable to parse these elements, return
+ // http.StatusBadRequest.
+ if parts := strings.SplitN(strings.TrimPrefix(urlPath, "/"), "@", 2); stdlib.Contains(parts[0]) {
+ fullPath, requestedVersion, err = parseStdLibURLPath(urlPath)
+ modulePath = stdlib.ModulePath
+ } else {
+ fullPath, modulePath, requestedVersion, err = parseDetailsURLPath(urlPath)
+ }
+ if err != nil {
+ return &serverError{
+ status: http.StatusBadRequest,
+ err: err,
+ }
+ }
+
+ ctx := r.Context()
+ // Validate the fullPath and requestedVersion that were parsed.
+ if err := checkPathAndVersion(ctx, s.ds, fullPath, requestedVersion); err != nil {
+ return err
+ }
+ // Depending on what the request was for, return the module or package page.
+ if isModule || fullPath == stdlib.ModulePath {
+ return s.serveModulePage(w, r, fullPath, requestedVersion)
+ }
+ if isActiveUseDirectories(ctx) {
+ return s.servePackagePageNew(w, r, fullPath, modulePath, requestedVersion)
+ }
+ return s.servePackagePage(w, r, fullPath, modulePath, requestedVersion)
}
// parseDetailsURLPath parses a URL path that refers (or may refer) to something
@@ -129,13 +179,13 @@
// checkPathAndVersion verifies that the requested path and version are
// acceptable. The given path may be a module or package path.
-func checkPathAndVersion(ctx context.Context, ds internal.DataSource, path, version string) error {
- if !isSupportedVersion(ctx, version) {
+func checkPathAndVersion(ctx context.Context, ds internal.DataSource, fullPath, requestedVersion string) error {
+ if !isSupportedVersion(ctx, requestedVersion) {
return &serverError{
status: http.StatusBadRequest,
epage: &errorPage{
- Message: fmt.Sprintf("%q is not a valid semantic version.", version),
- SecondaryMessage: suggestedSearch(path),
+ Message: fmt.Sprintf("%q is not a valid semantic version.", requestedVersion),
+ SecondaryMessage: suggestedSearch(fullPath),
},
}
}
@@ -143,7 +193,7 @@
if !ok {
return nil
}
- excluded, err := db.IsExcluded(ctx, path)
+ excluded, err := db.IsExcluded(ctx, fullPath)
if err != nil {
return err
}
@@ -228,3 +278,24 @@
},
}
}
+
+func parseStdLibURLPath(urlPath string) (path, version string, err error) {
+ defer derrors.Wrap(&err, "parseStdLibURLPath(%q)", urlPath)
+
+ // This splits urlPath into either:
+ // /<path>@<tag> or /<path>
+ parts := strings.SplitN(urlPath, "@", 2)
+ path = strings.TrimSuffix(strings.TrimPrefix(parts[0], "/"), "/")
+ if err := module.CheckImportPath(path); err != nil {
+ return "", "", err
+ }
+
+ if len(parts) == 1 {
+ return path, internal.LatestVersion, nil
+ }
+ version = stdlib.VersionForTag(parts[1])
+ if version == "" {
+ return "", "", fmt.Errorf("invalid Go tag for url: %q", urlPath)
+ }
+ return path, version, nil
+}
diff --git a/internal/frontend/module.go b/internal/frontend/module.go
index 71eb236..e068ecd 100644
--- a/internal/frontend/module.go
+++ b/internal/frontend/module.go
@@ -9,39 +9,15 @@
"errors"
"fmt"
"net/http"
- "strings"
"golang.org/x/pkgsite/internal"
"golang.org/x/pkgsite/internal/derrors"
"golang.org/x/pkgsite/internal/log"
)
-// handleModuleDetails handles requests for non-stdlib module details pages. It
-// expects paths of the form "/mod/<module-path>[@<version>?tab=<tab>]".
-// stdlib module pages are handled at "/std".
-func (s *Server) serveModuleDetails(w http.ResponseWriter, r *http.Request) error {
- if r.URL.Path == "/mod/std" {
- http.Redirect(w, r, "/std", http.StatusMovedPermanently)
- return nil
- }
- urlPath := strings.TrimPrefix(r.URL.Path, "/mod")
- path, _, version, err := parseDetailsURLPath(urlPath)
- if err != nil {
- return &serverError{
- status: http.StatusBadRequest,
- err: fmt.Errorf("handleModuleDetails: %v", err),
- }
- }
- return s.serveModulePage(w, r, path, version)
-}
-
// serveModulePage serves details pages for the module specified by modulePath
// and version.
func (s *Server) serveModulePage(w http.ResponseWriter, r *http.Request, modulePath, requestedVersion string) error {
- ctx := r.Context()
- if err := checkPathAndVersion(ctx, s.ds, modulePath, requestedVersion); err != nil {
- return err
- }
// This function handles top level behavior related to the existence of the
// requested modulePath@version:
// TODO: fix
@@ -52,6 +28,7 @@
// a. We don't know anything about this module: just serve a 404
// b. We have valid versions for this module path, but `version` isn't
// one of them. Serve a 404 but recommend the other versions.
+ ctx := r.Context()
mi, err := s.ds.GetModuleInfo(ctx, modulePath, requestedVersion)
if err == nil {
return s.serveModulePageWithModule(ctx, w, r, mi, requestedVersion)
diff --git a/internal/frontend/package.go b/internal/frontend/package.go
index 5335ccc..8a052c6 100644
--- a/internal/frontend/package.go
+++ b/internal/frontend/package.go
@@ -18,19 +18,6 @@
"golang.org/x/pkgsite/internal/stdlib"
)
-// handlePackageDetails handles requests for package details pages. It expects
-// paths of the form "/<path>[@<version>?tab=<tab>]".
-func (s *Server) servePackageDetails(w http.ResponseWriter, r *http.Request) error {
- pkgPath, modulePath, version, err := parseDetailsURLPath(r.URL.Path)
- if err != nil {
- return &serverError{
- status: http.StatusBadRequest,
- err: fmt.Errorf("handlePackageDetails: %v", err),
- }
- }
- return s.servePackagePage(w, r, pkgPath, modulePath, version)
-}
-
// handlePackageDetailsRedirect redirects all redirects to "/pkg" to "/".
func (s *Server) handlePackageDetailsRedirect(w http.ResponseWriter, r *http.Request) {
urlPath := strings.TrimPrefix(r.URL.Path, "/pkg")
@@ -41,12 +28,6 @@
// pkgPath, in the module specified by modulePath and version.
func (s *Server) servePackagePage(w http.ResponseWriter, r *http.Request, pkgPath, modulePath, version string) (err error) {
ctx := r.Context()
- if err := checkPathAndVersion(ctx, s.ds, pkgPath, version); err != nil {
- return err
- }
- if isActiveUseDirectories(ctx) {
- return s.servePackagePageNew(w, r, pkgPath, modulePath, version)
- }
// This function handles top level behavior related to the existence of the
// requested pkgPath@version.
diff --git a/internal/frontend/server.go b/internal/frontend/server.go
index b20e7a8..c85a855 100644
--- a/internal/frontend/server.go
+++ b/internal/frontend/server.go
@@ -87,13 +87,11 @@
// Install registers server routes using the given handler registration func.
func (s *Server) Install(handle func(string, http.Handler), redisClient *redis.Client) {
var (
- modHandler http.Handler = s.errorHandler(s.serveModuleDetails)
detailHandler http.Handler = s.errorHandler(s.serveDetails)
searchHandler http.Handler = s.errorHandler(s.serveSearch)
)
if redisClient != nil {
- modHandler = middleware.Cache("module-details", redisClient, moduleTTL)(modHandler)
- detailHandler = middleware.Cache("package-details", redisClient, packageTTL)(detailHandler)
+ detailHandler = middleware.Cache("details", redisClient, detailsTTL)(detailHandler)
searchHandler = middleware.Cache("search", redisClient, middleware.TTL(defaultTTL))(searchHandler)
}
handle("/static/", http.StripPrefix("/static/", http.FileServer(http.Dir(s.staticPath))))
@@ -102,7 +100,6 @@
http.ServeFile(w, r, fmt.Sprintf("%s/img/favicon.ico", http.Dir(s.staticPath)))
}))
handle("/fetch/", http.HandlerFunc(s.fetchHandler))
- handle("/mod/", modHandler)
handle("/pkg/", http.HandlerFunc(s.handlePackageDetailsRedirect))
handle("/search", searchHandler)
handle("/search-help", s.staticPageHandler("search_help.tmpl", "Search Help - go.dev"))
@@ -129,21 +126,18 @@
longTTL = 24 * time.Hour
)
-// packageTTL assigns the cache TTL for package detail requests.
-func packageTTL(r *http.Request) time.Duration {
- return detailsTTL(r.Context(), r.URL.Path, r.FormValue("tab"))
+// detailsTTL assigns the cache TTL for package detail requests.
+func detailsTTL(r *http.Request) time.Duration {
+ return detailsTTLForPath(r.Context(), r.URL.Path, r.FormValue("tab"))
}
-// moduleTTL assigns the cache TTL for /mod/ requests.
-func moduleTTL(r *http.Request) time.Duration {
- urlPath := strings.TrimPrefix(r.URL.Path, "/mod")
- return detailsTTL(r.Context(), urlPath, r.FormValue("tab"))
-}
-
-func detailsTTL(ctx context.Context, urlPath, tab string) time.Duration {
+func detailsTTLForPath(ctx context.Context, urlPath, tab string) time.Duration {
if urlPath == "/" {
return defaultTTL
}
+ if strings.HasPrefix(urlPath, "/mod") {
+ urlPath = strings.TrimPrefix(urlPath, "/mod")
+ }
_, _, version, err := parseDetailsURLPath(urlPath)
if err != nil {
log.Errorf(ctx, "falling back to default TTL: %v", err)
diff --git a/internal/frontend/server_test.go b/internal/frontend/server_test.go
index 542437b..871e946 100644
--- a/internal/frontend/server_test.go
+++ b/internal/frontend/server_test.go
@@ -177,7 +177,7 @@
t.Run("no experiments", func(t *testing.T) {
testServer(t)
})
- t.Run("use-paths-table", func(t *testing.T) {
+ t.Run("use directories", func(t *testing.T) {
testServer(t, internal.ExperimentUseDirectories, internal.ExperimentInsertDirectories)
})
}
@@ -712,7 +712,7 @@
in(".Directories", text(`This is a package synopsis`))),
},
{
- name: "module@version overview tab",
+ name: "module at version overview tab",
urlPath: fmt.Sprintf("/mod/%s@%s?tab=overview", sample.ModulePath, sample.VersionString),
wantStatusCode: http.StatusOK,
want: in("",
@@ -726,7 +726,7 @@
})),
},
{
- name: "module@version overview tab, pseudoversion",
+ name: "module at version overview tab, pseudoversion",
urlPath: fmt.Sprintf("/mod/%s@%s?tab=overview", sample.ModulePath, pseudoVersion),
wantStatusCode: http.StatusOK,
want: in("",
@@ -740,7 +740,7 @@
})),
},
{
- name: "module@version packages tab",
+ name: "module at version packages tab",
urlPath: fmt.Sprintf("/mod/%s@%s?tab=packages", sample.ModulePath, sample.VersionString),
wantStatusCode: http.StatusOK,
want: in("",
@@ -748,7 +748,7 @@
in(".Directories", text(`This is a package synopsis`))),
},
{
- name: "module@version versions tab",
+ name: "module at version versions tab",
urlPath: fmt.Sprintf("/mod/%s@%s?tab=versions", sample.ModulePath, sample.VersionString),
wantStatusCode: http.StatusOK,
want: in("",
@@ -762,7 +762,7 @@
text("v1.0.0")))),
},
{
- name: "module@version licenses tab",
+ name: "module at version licenses tab",
urlPath: fmt.Sprintf("/mod/%s@%s?tab=licenses", sample.ModulePath, sample.VersionString),
wantStatusCode: http.StatusOK,
want: in("",
@@ -863,7 +863,7 @@
return r
}
-func TestPackageTTL(t *testing.T) {
+func TestDetailsTTL(t *testing.T) {
tests := []struct {
r *http.Request
want time.Duration
@@ -873,19 +873,6 @@
{mustRequest("/host.com/module@v1.2.3/suffix?tab=overview", t), longTTL},
{mustRequest("/host.com/module@v1.2.3/suffix?tab=versions", t), defaultTTL},
{mustRequest("/host.com/module@v1.2.3/suffix?tab=importedby", t), defaultTTL},
- }
- for _, test := range tests {
- if got := packageTTL(test.r); got != test.want {
- t.Errorf("packageTTL(%v) = %v, want %v", test.r, got, test.want)
- }
- }
-}
-
-func TestModuleTTL(t *testing.T) {
- tests := []struct {
- r *http.Request
- want time.Duration
- }{
{mustRequest("/mod/host.com/module@v1.2.3/suffix", t), longTTL},
{mustRequest("/mod/host.com/module/suffix", t), shortTTL},
{mustRequest("/mod/host.com/module@v1.2.3/suffix?tab=overview", t), longTTL},
@@ -893,8 +880,8 @@
{mustRequest("/mod/host.com/module@v1.2.3/suffix?tab=importedby", t), defaultTTL},
}
for _, test := range tests {
- if got := moduleTTL(test.r); got != test.want {
- t.Errorf("packageTTL(%v) = %v, want %v", test.r, got, test.want)
+ if got := detailsTTL(test.r); got != test.want {
+ t.Errorf("detailsTTL(%v) = %v, want %v", test.r, got, test.want)
}
}
}
diff --git a/internal/frontend/stdlib.go b/internal/frontend/stdlib.go
deleted file mode 100644
index fe16e63..0000000
--- a/internal/frontend/stdlib.go
+++ /dev/null
@@ -1,59 +0,0 @@
-// Copyright 2019 The Go Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style
-// license that can be found in the LICENSE file.
-
-package frontend
-
-import (
- "fmt"
- "net/http"
- "strings"
-
- "golang.org/x/mod/module"
- "golang.org/x/pkgsite/internal"
- "golang.org/x/pkgsite/internal/derrors"
- "golang.org/x/pkgsite/internal/stdlib"
-)
-
-// serveStdLib handles a request for a stdlib package or module.
-func (s *Server) serveStdLib(w http.ResponseWriter, r *http.Request) error {
- path, version, err := parseStdLibURLPath(r.URL.Path)
- if err != nil {
- return &serverError{
- status: http.StatusBadRequest,
- err: fmt.Errorf("handleStdLib: %v", err),
- }
- }
- if path == stdlib.ModulePath {
- return s.serveModulePage(w, r, stdlib.ModulePath, version)
- }
-
- // Package "C" is a special case: redirect to the Go Blog article on cgo.
- // (This is what godoc.org does.)
- if path == "C" {
- http.Redirect(w, r, "https://golang.org/doc/articles/c_go_cgo.html", http.StatusMovedPermanently)
- return nil
- }
- return s.servePackagePage(w, r, path, stdlib.ModulePath, version)
-}
-
-func parseStdLibURLPath(urlPath string) (path, version string, err error) {
- defer derrors.Wrap(&err, "parseStdLibURLPath(%q)", urlPath)
-
- // This splits urlPath into either:
- // /<path>@<tag> or /<path>
- parts := strings.SplitN(urlPath, "@", 2)
- path = strings.TrimSuffix(strings.TrimPrefix(parts[0], "/"), "/")
- if err := module.CheckImportPath(path); err != nil {
- return "", "", err
- }
-
- if len(parts) == 1 {
- return path, internal.LatestVersion, nil
- }
- version = stdlib.VersionForTag(parts[1])
- if version == "" {
- return "", "", fmt.Errorf("invalid Go tag for url: %q", urlPath)
- }
- return path, version, nil
-}
