internal/frontend: return previous fetch result on 404
When a request results in a 404, show the user the previous fetch
result, instead of giving the option to refetch the path. The majority
of the time, clicking "Request <path>" will just show the user when is
in the database.
Change-Id: I652edd86e8ad236498eb78ffc205171309cbbee1
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/281675
Trust: Julie Qiu <julie@golang.org>
Run-TryBot: Julie Qiu <julie@golang.org>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
diff --git a/internal/frontend/404.go b/internal/frontend/404.go
index 452a321..6b98ca2 100644
--- a/internal/frontend/404.go
+++ b/internal/frontend/404.go
@@ -5,13 +5,18 @@
package frontend
import (
+ "context"
+ "errors"
"fmt"
+ "html"
"net/http"
"github.com/google/safehtml/template"
+ "github.com/google/safehtml/template/uncheckedconversions"
"golang.org/x/pkgsite/internal"
"golang.org/x/pkgsite/internal/derrors"
"golang.org/x/pkgsite/internal/log"
+ "golang.org/x/pkgsite/internal/postgres"
"golang.org/x/pkgsite/internal/stdlib"
)
@@ -46,7 +51,27 @@
if stdlib.Contains(fullPath) {
return &serverError{status: http.StatusNotFound}
}
- return pathNotFoundError(fullPath, requestedVersion)
+
+ db, ok := ds.(*postgres.DB)
+ if !ok {
+ return proxydatasourceNotSupportedErr()
+ }
+ fr, err := previousFetchStatusAndResponse(ctx, db, fullPath, requestedVersion)
+ if err != nil || fr.status == http.StatusInternalServerError {
+ if err != nil && !errors.Is(err, derrors.NotFound) {
+ log.Error(ctx, err)
+ }
+ return pathNotFoundError(fullPath, requestedVersion)
+ }
+ return &serverError{
+ status: fr.status,
+ epage: &errorPage{
+ messageTemplate: uncheckedconversions.TrustedTemplateFromStringKnownToSatisfyTypeContract(`
+ <h3 class="Error-message">{{.StatusText}}</h3>
+ <p class="Error-message">` + html.UnescapeString(fr.responseText) + `</p>`),
+ MessageData: struct{ StatusText string }{http.StatusText(fr.status)},
+ },
+ }
}
// pathNotFoundError returns a page with an option on how to
@@ -70,3 +95,51 @@
},
}
}
+
+// previousFetchStatusAndResponse returns the status and response text from a
+// previous fetch of the fullPath and requestedVersion.
+func previousFetchStatusAndResponse(ctx context.Context, db *postgres.DB, fullPath, requestedVersion string) (_ *fetchResult, err error) {
+ defer derrors.Wrap(&err, "fetchRedirectPath(w, r, %q, %q)", fullPath, requestedVersion)
+
+ vm, err := db.GetVersionMap(ctx, fullPath, requestedVersion)
+ if err != nil {
+ return nil, err
+ }
+ if vm.Status != http.StatusNotFound {
+ return resultFromFetchRequest([]*fetchResult{
+ {
+ modulePath: vm.ModulePath,
+ goModPath: vm.GoModPath,
+ status: vm.Status,
+ err: errors.New(vm.Error),
+ },
+ }, fullPath, requestedVersion)
+ }
+
+ // If the status is 404, it likely means that the fullPath is not a
+ // modulePath. Check all of the candidate module paths for the past result.
+ paths, err := candidateModulePaths(fullPath)
+ if err != nil {
+ return nil, err
+ }
+ vms, err := db.GetVersionMapsNon2xxStatus(ctx, paths, requestedVersion)
+ if err != nil {
+ return nil, err
+ }
+ if len(vms) == 0 {
+ return nil, nil
+ }
+ var fetchResults []*fetchResult
+ for _, vm := range vms {
+ fetchResults = append(fetchResults, &fetchResult{
+ modulePath: vm.ModulePath,
+ goModPath: vm.GoModPath,
+ status: vm.Status,
+ err: errors.New(vm.Error),
+ })
+ }
+ if len(fetchResults) == 0 {
+ return nil, derrors.NotFound
+ }
+ return resultFromFetchRequest(fetchResults, fullPath, requestedVersion)
+}
diff --git a/internal/frontend/404_test.go b/internal/frontend/404_test.go
new file mode 100644
index 0000000..5a6d2d7
--- /dev/null
+++ b/internal/frontend/404_test.go
@@ -0,0 +1,93 @@
+// Copyright 2020 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 (
+ "context"
+ "errors"
+ "testing"
+
+ "golang.org/x/pkgsite/internal"
+ "golang.org/x/pkgsite/internal/derrors"
+ "golang.org/x/pkgsite/internal/testing/sample"
+)
+
+func TestPreviousFetchStatusAndResponse(t *testing.T) {
+ ctx := context.Background()
+
+ for _, mod := range []struct {
+ path string
+ goModPath string
+ status int
+ }{
+ {"mvdan.cc/sh", "", 200},
+ {"mvdan.cc", "", 404},
+ {"400.mod/foo/bar", "", 400},
+ {"400.mod/foo", "", 404},
+ {"400.mod", "", 404},
+ {"github.com/alternative/ok", "github.com/vanity", 491},
+ {"github.com/alternative/ok/path", "", 404},
+ {"github.com/alternative/bad", "vanity", 491},
+ {"bad.mod/foo/bar", "", 490},
+ {"bad.mod/foo", "", 404},
+ {"bad.mod", "", 490},
+ {"500.mod/foo", "", 404},
+ {"500.mod", "", 500},
+ {"reprocess.mod/foo", "", 520},
+ } {
+ goModPath := mod.goModPath
+ if goModPath == "" {
+ goModPath = mod.path
+ }
+ if err := testDB.UpsertVersionMap(ctx, &internal.VersionMap{
+ ModulePath: mod.path,
+ RequestedVersion: internal.LatestVersion,
+ ResolvedVersion: sample.VersionString,
+ Status: mod.status,
+ GoModPath: goModPath,
+ }); err != nil {
+ t.Fatal(err)
+ }
+ }
+
+ for _, test := range []struct {
+ name, path string
+ status int
+ }{
+ {"bad request at path root", "400.mod/foo/bar", 404},
+ {"bad request at mod but 404 at path", "400.mod/foo", 404},
+ {"alternative mod", "github.com/alternative/ok", 491},
+ {"alternative mod package path", "github.com/alternative/ok/path", 491},
+ {"alternative mod bad module path", "github.com/alternative/bad", 404},
+ {"bad module at path", "bad.mod/foo/bar", 404},
+ {"bad module at mod but 404 at path", "bad.mod/foo", 404},
+ {"500", "500.mod/foo", 500},
+ {"mod to reprocess", "reprocess.mod/foo", 404},
+ } {
+ t.Run(test.name, func(t *testing.T) {
+ fr, err := previousFetchStatusAndResponse(ctx, testDB, test.path, internal.LatestVersion)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if fr.status != test.status {
+ t.Errorf("got %v; want %v", fr.status, test.status)
+ }
+ })
+ }
+
+ for _, test := range []struct {
+ name, path string
+ }{
+ {"path never fetched", "github.com/nonexistent"},
+ {"path never fetched, but top level mod fetched", "mvdan.cc/sh/v3"},
+ } {
+ t.Run(test.name, func(t *testing.T) {
+ _, err := previousFetchStatusAndResponse(ctx, testDB, test.path, internal.LatestVersion)
+ if !errors.Is(err, derrors.NotFound) {
+ t.Errorf("got %v; want %v", err, derrors.NotFound)
+ }
+ })
+ }
+}
diff --git a/internal/frontend/fetch.go b/internal/frontend/fetch.go
index ce90461..40f1eb8 100644
--- a/internal/frontend/fetch.go
+++ b/internal/frontend/fetch.go
@@ -158,6 +158,9 @@
log.Errorf(ctx, "fetchAndPoll(ctx, ds, q, %q, %q, %q): %v", modulePath, fullPath, requestedVersion, err)
return http.StatusInternalServerError, http.StatusText(http.StatusInternalServerError)
}
+ if fr.status == derrors.ToStatus(derrors.AlternativeModule) {
+ fr.status = http.StatusNotFound
+ }
return fr.status, fr.responseText
}
@@ -247,7 +250,7 @@
fr.responseText = "Oops! Something went wrong."
return fr, nil
case derrors.ToStatus(derrors.AlternativeModule):
- if err := module.CheckImportPath(fr.goModPath); err != nil {
+ if err := module.CheckPath(fr.goModPath); err != nil {
fr.status = http.StatusNotFound
fr.responseText = fmt.Sprintf(`%q does not have a valid module path (%q).`, fullPath, fr.goModPath)
return fr, nil
@@ -259,7 +262,6 @@
fr.status = http.StatusInternalServerError
return fr, err
}
- fr.status = http.StatusNotFound
fr.responseText = h.String()
return fr, nil
}
diff --git a/internal/frontend/server_test.go b/internal/frontend/server_test.go
index 36eb94f..005c5ff 100644
--- a/internal/frontend/server_test.go
+++ b/internal/frontend/server_test.go
@@ -19,6 +19,7 @@
"github.com/jba/templatecheck"
"golang.org/x/net/html"
"golang.org/x/pkgsite/internal"
+ "golang.org/x/pkgsite/internal/derrors"
"golang.org/x/pkgsite/internal/experiment"
"golang.org/x/pkgsite/internal/middleware"
"golang.org/x/pkgsite/internal/postgres"
@@ -1148,6 +1149,16 @@
if err := testDB.InsertModule(ctx, sampleModule); err != nil {
t.Fatal(err)
}
+ alternativeModule := &internal.VersionMap{
+ ModulePath: "module.path/alternative",
+ GoModPath: sample.ModulePath,
+ RequestedVersion: sample.VersionString,
+ ResolvedVersion: sample.VersionString,
+ Status: derrors.ToStatus(derrors.AlternativeModule),
+ }
+ if err := testDB.UpsertVersionMap(ctx, alternativeModule); err != nil {
+ t.Fatal(err)
+ }
_, handler, _ := newTestServer(t, nil)
for _, test := range []struct {
@@ -1156,6 +1167,7 @@
}{
{"not found", "/invalid-page", http.StatusNotFound},
{"bad request", "/gocloud.dev/@latest/blob", http.StatusBadRequest},
+ {"alternative module", "/" + alternativeModule.ModulePath, http.StatusNotFound},
} {
t.Run(test.name, func(t *testing.T) {
w := httptest.NewRecorder()
