internal: delete experiment not-at-v1
Additionally, only redirect if the non-v1 path does not match the full
path, and is actually a non-v1 path.
Otherwise, this will redirect requests to <path>@<missing-version> to
<path>@latest, or <path>/v3 to <path>, instead of return a 404 that
allows the user to fetch the missing version.
Change-Id: I66946062d341477fdc19bfc2a0f18b848943807d
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/284580
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/experiment.go b/internal/experiment.go
index 1cb64a9..e45d0a2 100644
--- a/internal/experiment.go
+++ b/internal/experiment.go
@@ -8,7 +8,6 @@
const (
ExperimentDirectoryTree = "directory-tree"
ExperimentNotAtLatest = "not-at-latest"
- ExperimentNotAtV1 = "not-at-v1"
ExperimentRedirectedFromBanner = "redirected-from-banner"
)
@@ -17,7 +16,6 @@
var Experiments = map[string]string{
ExperimentDirectoryTree: "Enable the directory tree layout on the unit page.",
ExperimentNotAtLatest: "Enable the display of a 'not at latest' badge.",
- ExperimentNotAtV1: "Redirect requests to a path not at v1 to the highest major version of that path.",
ExperimentRedirectedFromBanner: "Display banner with path that request was redirected from.",
}
diff --git a/internal/frontend/404.go b/internal/frontend/404.go
index 5fcf653..e3f3718 100644
--- a/internal/frontend/404.go
+++ b/internal/frontend/404.go
@@ -16,7 +16,6 @@
"golang.org/x/pkgsite/internal"
"golang.org/x/pkgsite/internal/cookie"
"golang.org/x/pkgsite/internal/derrors"
- "golang.org/x/pkgsite/internal/experiment"
"golang.org/x/pkgsite/internal/log"
"golang.org/x/pkgsite/internal/postgres"
"golang.org/x/pkgsite/internal/stdlib"
@@ -141,27 +140,29 @@
}, fullPath, requestedVersion)
}
- if experiment.IsActive(ctx, internal.ExperimentNotAtV1) {
- // Check if the unit path exists at a higher major version.
- // For example, my.module might not exist, but my.module/v3 might.
- // Similarly, my.module/foo might not exist, but my.module/v3/foo might.
- // In either case, the user will be redirected to the highest major version
- // of the path.
- //
- // Do not bother to look for a specific version if this case. If
- // my.module/foo@v2.1.0 was requested, and my.module/foo/v2 exists, just
- // return the latest version of my.module/foo/v2.
- majPath, err := db.GetLatestMajorPathForV1Path(ctx, fullPath)
- if err != nil && err != derrors.NotFound {
- return nil, err
- }
- if majPath != "" {
- return &fetchResult{
- modulePath: majPath,
- goModPath: majPath,
- status: http.StatusFound,
- }, nil
- }
+ // Check if the unit path exists at a higher major version.
+ // For example, my.module might not exist, but my.module/v3 might.
+ // Similarly, my.module/foo might not exist, but my.module/v3/foo might.
+ // In either case, the user will be redirected to the highest major version
+ // of the path.
+ //
+ // Do not bother to look for a specific version if this case. If
+ // my.module/foo@v2.1.0 was requested, and my.module/foo/v2 exists, just
+ // return the latest version of my.module/foo/v2.
+ //
+ // Only redirect if the majPath returned is different from the fullPath, and
+ // the majPath is not at v1. We don't want to redirect my.module/foo/v3 to
+ // my.module/foo, or my.module/foo@v1.5.2 to my.module/foo@v1.0.0.
+ majPath, maj, err := db.GetLatestMajorPathForV1Path(ctx, fullPath)
+ if err != nil && err != derrors.NotFound {
+ return nil, err
+ }
+ if majPath != fullPath && maj != 1 && majPath != "" {
+ return &fetchResult{
+ modulePath: majPath,
+ goModPath: majPath,
+ status: http.StatusFound,
+ }, nil
}
// The full path does not exist in our database, but its module might.
diff --git a/internal/frontend/404_test.go b/internal/frontend/404_test.go
index aa30240..b95f2db 100644
--- a/internal/frontend/404_test.go
+++ b/internal/frontend/404_test.go
@@ -8,14 +8,12 @@
"context"
"errors"
"net/http"
- "strings"
"testing"
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"golang.org/x/pkgsite/internal"
"golang.org/x/pkgsite/internal/derrors"
- "golang.org/x/pkgsite/internal/experiment"
"golang.org/x/pkgsite/internal/postgres"
"golang.org/x/pkgsite/internal/testing/sample"
)
@@ -124,15 +122,15 @@
}
}
- checkPath := func(ctx context.Context, t *testing.T, testDB *postgres.DB, path, wantPath string, status int) {
- got, err := previousFetchStatusAndResponse(ctx, testDB, path, internal.LatestVersion)
+ checkPath := func(ctx context.Context, t *testing.T, testDB *postgres.DB, path, version, wantPath string, wantStatus int) {
+ got, err := previousFetchStatusAndResponse(ctx, testDB, path, version)
if err != nil {
t.Fatal(err)
}
want := &fetchResult{
modulePath: wantPath,
goModPath: wantPath,
- status: status,
+ status: wantStatus,
}
if diff := cmp.Diff(want, got,
cmp.AllowUnexported(fetchResult{}),
@@ -142,18 +140,26 @@
}
for _, test := range []struct {
- name, path, want string
+ name, path, version, wantPath string
+ wantStatus int
}{
- {"module path not at v1", sample.ModulePath, sample.ModulePath + "/v4"},
- {"import path not at v1", sample.ModulePath + "/foo", sample.ModulePath + "/v4/foo"},
+ {"module path not at v1", sample.ModulePath, internal.LatestVersion, sample.ModulePath + "/v4", http.StatusFound},
+ {"import path not at v1", sample.ModulePath + "/foo", internal.LatestVersion, sample.ModulePath + "/v4/foo", http.StatusFound},
} {
t.Run(test.name, func(t *testing.T) {
- want := strings.ReplaceAll(test.want, "/v4", "")
- checkPath(ctx, t, testDB, test.path, want, http.StatusNotFound)
+ checkPath(ctx, t, testDB, test.path, test.version, test.wantPath, test.wantStatus)
})
- t.Run(test.name+"not-at-v1", func(t *testing.T) {
- ctx := experiment.NewContext(ctx, internal.ExperimentNotAtV1)
- checkPath(ctx, t, testDB, test.path, test.want, http.StatusFound)
+ }
+ for _, test := range []struct {
+ name, path, version string
+ }{
+ {"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)
+ if !errors.Is(err, derrors.NotFound) {
+ t.Fatal(err)
+ }
})
}
}
diff --git a/internal/frontend/server_test.go b/internal/frontend/server_test.go
index 477e361..380a6ef 100644
--- a/internal/frontend/server_test.go
+++ b/internal/frontend/server_test.go
@@ -1220,8 +1220,7 @@
}
}
- _, handler, _ := newTestServer(t, nil, internal.ExperimentNotAtV1)
- _, handlerNoExp, _ := newTestServer(t, nil)
+ _, handler, _ := newTestServer(t, nil)
for _, test := range []struct {
name, path, flash string
@@ -1236,12 +1235,6 @@
t.Run(test.name, func(t *testing.T) {
w := httptest.NewRecorder()
handler.ServeHTTP(w, httptest.NewRequest("GET", test.path, nil))
- if w.Code != test.wantCode {
- t.Errorf("%q: got status code = %d, want %d", test.path, w.Code, test.wantCode)
- }
-
- w2 := httptest.NewRecorder()
- handlerNoExp.ServeHTTP(w2, httptest.NewRequest("GET", test.path, nil))
if test.path == v1modpath || test.path == v1path {
test.wantCode = http.StatusNotFound
}
diff --git a/internal/postgres/path.go b/internal/postgres/path.go
index 225c8d3..5312926 100644
--- a/internal/postgres/path.go
+++ b/internal/postgres/path.go
@@ -17,7 +17,7 @@
// GetLatestMajorPathForV1Path reports the latest unit path in the series for
// the given v1path.
-func (db *DB) GetLatestMajorPathForV1Path(ctx context.Context, v1path string) (_ string, err error) {
+func (db *DB) GetLatestMajorPathForV1Path(ctx context.Context, v1path string) (_ string, _ int, err error) {
defer derrors.Wrap(&err, "DB.GetLatestPathForV1Path(ctx, %q)", v1path)
q := `
SELECT p.path, m.series_path
@@ -42,7 +42,7 @@
return nil
}, v1path)
if err != nil {
- return "", err
+ return "", 0, err
}
var (
@@ -59,7 +59,7 @@
if v != "" {
i, err = strconv.Atoi(v)
if err != nil {
- return "", fmt.Errorf("strconv.Atoi(%q): %v", v, err)
+ return "", 0, fmt.Errorf("strconv.Atoi(%q): %v", v, err)
}
}
if maj <= i {
@@ -67,5 +67,9 @@
majPath = p
}
}
- return majPath, nil
+ if maj == 0 {
+ // Return 1 as the major version for all v0 or v1 majPaths.
+ maj = 1
+ }
+ return majPath, maj, nil
}
diff --git a/internal/postgres/path_test.go b/internal/postgres/path_test.go
index bdadb27..cf7dbac 100644
--- a/internal/postgres/path_test.go
+++ b/internal/postgres/path_test.go
@@ -6,6 +6,8 @@
import (
"context"
+ "strconv"
+ "strings"
"testing"
"golang.org/x/pkgsite/internal/testing/sample"
@@ -15,18 +17,27 @@
ctx, cancel := context.WithTimeout(context.Background(), testTimeout)
defer cancel()
- checkLatest := func(t *testing.T, versions []string, v1path string, wantVersion string) {
+ checkLatest := func(t *testing.T, versions []string, v1path string, version, suffix string) {
t.Helper()
- got, err := testDB.GetLatestMajorPathForV1Path(ctx, v1path)
+ gotPath, gotVer, err := testDB.GetLatestMajorPathForV1Path(ctx, v1path)
if err != nil {
t.Fatal(err)
}
want := sample.ModulePath
- if wantVersion != "" {
- want = want + "/" + wantVersion
+ if suffix != "" {
+ want = want + "/" + suffix
}
- if got != want {
- t.Errorf("GetLatestMajorPathForV1Path(%q) = %q, want %q", v1path, got, want)
+ var wantVer int
+ if version == "" {
+ wantVer = 1
+ } else {
+ wantVer, err = strconv.Atoi(strings.TrimPrefix(version, "v"))
+ if err != nil {
+ t.Fatal(err)
+ }
+ }
+ if gotPath != want || gotVer != wantVer {
+ t.Errorf("GetLatestMajorPathForV1Path(%q) = %q, %d, want %q, %d", v1path, gotPath, gotVer, want, wantVer)
}
}
@@ -71,14 +82,15 @@
}
t.Run("module", func(t *testing.T) {
v1path := sample.ModulePath
- checkLatest(t, test.versions, v1path, test.want)
+ checkLatest(t, test.versions, v1path, test.want, test.want)
})
t.Run("package", func(t *testing.T) {
+ want := test.want
if test.want != "" {
- test.want += "/"
+ want += "/"
}
v1path := sample.ModulePath + "/" + suffix
- checkLatest(t, test.versions, v1path, test.want+suffix)
+ checkLatest(t, test.versions, v1path, test.want, want+suffix)
})
})
}
