internal/frontend: handle deep-linking on 404
Instead of always returning a 404 if the fullPath does not exist, check
the longest candidate module path does not exist instead. This matches
the frontend fetch logic.
For example, if a request is made to
github.com/kubernetes/client-go@v0.20.1/informers/admissionregistration/v1,
we now check if github.com/kubernetes/client-go@v0.20.1 has ever been
fetched, instead of
github.com/kubernetes/client-go/informers/admissionregistration/v1@v0.20.1
(since we know the latter is not a module).
Change-Id: I5ef37e4634c11900cf2225def01169dd0d2d73af
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/284575
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 e3f3718..d213b16 100644
--- a/internal/frontend/404.go
+++ b/internal/frontend/404.go
@@ -38,7 +38,7 @@
// servePathNotFoundPage serves a 404 page for the requested path, or redirects
// the user to an appropriate location.
func (s *Server) servePathNotFoundPage(w http.ResponseWriter, r *http.Request,
- ds internal.DataSource, fullPath, requestedVersion string) (err error) {
+ ds internal.DataSource, fullPath, modulePath, requestedVersion string) (err error) {
defer derrors.Wrap(&err, "servePathNotFoundPage(w, r, %q, %q)", fullPath, requestedVersion)
db, ok := ds.(*postgres.DB)
@@ -62,7 +62,7 @@
return &serverError{status: http.StatusNotFound}
}
- fr, err := previousFetchStatusAndResponse(ctx, db, fullPath, requestedVersion)
+ fr, err := previousFetchStatusAndResponse(ctx, db, fullPath, modulePath, requestedVersion)
if err != nil {
if err != nil {
log.Error(ctx, err)
@@ -73,7 +73,7 @@
case http.StatusFound, derrors.ToStatus(derrors.AlternativeModule):
u := constructUnitURL(fr.goModPath, fr.goModPath, internal.LatestVersion)
cookie.Set(w, cookie.AlternativeModuleFlash, fullPath, u)
- http.Redirect(w, r, constructUnitURL(fr.goModPath, fr.goModPath, internal.LatestVersion), http.StatusFound)
+ http.Redirect(w, r, u, http.StatusFound)
return
case http.StatusInternalServerError:
return pathNotFoundError(fullPath, requestedVersion)
@@ -114,22 +114,29 @@
// previousFetchStatusAndResponse returns the fetch result from a
// previous fetch of the fullPath and requestedVersion.
-func previousFetchStatusAndResponse(ctx context.Context, db *postgres.DB, fullPath, requestedVersion string) (_ *fetchResult, err error) {
+func previousFetchStatusAndResponse(ctx context.Context, db *postgres.DB, fullPath, modulePath, requestedVersion string) (_ *fetchResult, err error) {
defer derrors.Wrap(&err, "previousFetchStatusAndResponse(w, r, %q, %q)", fullPath, requestedVersion)
- // Check if a row exists in the version_map table for the requested path
- // and version. If not, this path may have never been fetched.
- // In that case, a derrors.NotFound will be returned.
- vm, err := db.GetVersionMap(ctx, fullPath, requestedVersion)
+ // Get all candidate module paths for this path.
+ paths, err := modulePathsToFetch(ctx, db, fullPath, modulePath)
if err != nil {
return nil, err
}
-
- // If the row has been fetched before, and the result was either a 490 or
- // 491, return that result, since it is a final state.
- if vm.Status >= 500 ||
- vm.Status == derrors.ToStatus(derrors.AlternativeModule) ||
- vm.Status == derrors.ToStatus(derrors.BadModule) {
+ // Check if a row exists in the version_map table for the longest candidate
+ // path and version.
+ //
+ // If we have not fetched the path before, a derrors.NotFound will be
+ // returned.
+ vm, err := db.GetVersionMap(ctx, paths[0], requestedVersion)
+ if err != nil {
+ return nil, err
+ }
+ // If the row has been fetched before, and the result was either a 490,
+ // 491, or 5xx, return that result, since it is a final state.
+ if vm != nil &&
+ (vm.Status >= 500 ||
+ vm.Status == derrors.ToStatus(derrors.AlternativeModule) ||
+ vm.Status == derrors.ToStatus(derrors.BadModule)) {
return resultFromFetchRequest([]*fetchResult{
{
modulePath: vm.ModulePath,
@@ -164,21 +171,10 @@
status: http.StatusFound,
}, nil
}
-
- // The full path does not exist in our database, but its module might.
- // This could be be because the path is in an alternative module or a bad
- // module, or it was fetched previously and 404ed.
- 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, fetchResultFromVersionMap(vm))
diff --git a/internal/frontend/404_test.go b/internal/frontend/404_test.go
index b95f2db..e181e15 100644
--- a/internal/frontend/404_test.go
+++ b/internal/frontend/404_test.go
@@ -20,6 +20,7 @@
func TestPreviousFetchStatusAndResponse(t *testing.T) {
ctx := context.Background()
+ defer postgres.ResetTestDB(testDB, t)
for _, mod := range []struct {
path string
@@ -34,6 +35,7 @@
{"github.com/alternative/ok", "github.com/vanity", 491},
{"github.com/alternative/ok/path", "", 404},
{"github.com/alternative/bad", "vanity", 491},
+ {"github.com/kubernetes/client-go", "k8s.io/client-go", 491},
{"bad.mod/foo/bar", "", 490},
{"bad.mod/foo", "", 404},
{"bad.mod", "", 490},
@@ -71,7 +73,7 @@
{"mod to reprocess", "reprocess.mod/foo", 404},
} {
t.Run(test.name, func(t *testing.T) {
- fr, err := previousFetchStatusAndResponse(ctx, testDB, test.path, internal.LatestVersion)
+ fr, err := previousFetchStatusAndResponse(ctx, testDB, test.path, internal.UnknownModulePath, internal.LatestVersion)
if err != nil {
t.Fatal(err)
}
@@ -84,11 +86,11 @@
for _, test := range []struct {
name, path string
}{
- {"path never fetched", "github.com/nonexistent"},
+ {"path never fetched", "github.com/non/existent"},
{"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)
+ _, err := previousFetchStatusAndResponse(ctx, testDB, test.path, internal.UnknownModulePath, internal.LatestVersion)
if !errors.Is(err, derrors.NotFound) {
t.Errorf("got %v; want %v", err, derrors.NotFound)
}
@@ -96,8 +98,67 @@
}
}
+func TestPreviousFetchStatusAndResponse_AlternativeModuleWithDeepLinking(t *testing.T) {
+ ctx := context.Background()
+ defer postgres.ResetTestDB(testDB, t)
+
+ for _, mod := range []struct {
+ path string
+ goModPath string
+ status int
+ }{
+ {"k8s.io/client-go", "k8s.io/client-go", 200},
+ {"github.com/kubernetes/client-go", "k8s.io/client-go", 491},
+ } {
+ 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, mod string
+ status int
+ }{
+ {"path with specified module", "github.com/kubernetes/client-go/informers/admissionregistration/v1", "github.com/kubernetes/client-go", 491},
+ } {
+ t.Run(test.name, func(t *testing.T) {
+ fr, err := previousFetchStatusAndResponse(ctx, testDB, test.path, test.mod, 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, mod string
+ }{
+ {"path with unknown module", "github.com/kubernetes/client-go/informers/admissionregistration/v1", internal.UnknownModulePath},
+ {"module nonexistent module", "github.com/kubernetes/client-go/typo", "github.com/kubernetes/client-go/typo"},
+ {"path with specified nonexistent module", "github.com/kubernetes/client-go/typo/informers/admissionregistration/v1", "github.com/kubernetes/client-go/typo"},
+ } {
+ t.Run(test.name, func(t *testing.T) {
+ if _, err := previousFetchStatusAndResponse(ctx, testDB, test.path, test.mod, internal.LatestVersion); !errors.Is(err, derrors.NotFound) {
+ t.Fatal(err)
+ }
+ })
+ }
+}
+
func TestPreviousFetchStatusAndResponse_PathExistsAtNonV1(t *testing.T) {
ctx := context.Background()
+ defer postgres.ResetTestDB(testDB, t)
if err := testDB.InsertModule(ctx, sample.Module(sample.ModulePath+"/v4", "v4.0.0", "foo")); err != nil {
t.Fatal(err)
@@ -123,7 +184,7 @@
}
checkPath := func(ctx context.Context, t *testing.T, testDB *postgres.DB, path, version, wantPath string, wantStatus int) {
- got, err := previousFetchStatusAndResponse(ctx, testDB, path, version)
+ got, err := previousFetchStatusAndResponse(ctx, testDB, path, internal.UnknownModulePath, version)
if err != nil {
t.Fatal(err)
}
@@ -156,7 +217,7 @@
{"import path v1 missing version", sample.ModulePath + "/foo", "v1.5.2"},
} {
t.Run(test.name, func(t *testing.T) {
- _, err := previousFetchStatusAndResponse(ctx, testDB, test.path, test.version)
+ _, err := previousFetchStatusAndResponse(ctx, testDB, test.path, internal.UnknownModulePath, test.version)
if !errors.Is(err, derrors.NotFound) {
t.Fatal(err)
}
diff --git a/internal/frontend/unit.go b/internal/frontend/unit.go
index e69fe38..6649397 100644
--- a/internal/frontend/unit.go
+++ b/internal/frontend/unit.go
@@ -93,7 +93,7 @@
if !errors.Is(err, derrors.NotFound) {
return err
}
- return s.servePathNotFoundPage(w, r, ds, info.fullPath, info.requestedVersion)
+ return s.servePathNotFoundPage(w, r, ds, info.fullPath, info.modulePath, info.requestedVersion)
}
recordVersionTypeMetric(ctx, info.requestedVersion)
