internal/api: use the new DB.GetVersionsForPath method Update the versions route to use the new DB method, which supports paging and changes the sort order. Add query parameters for token and limit. Also add a query to request pseudo-versions, which are omitted by default. One problem: there is no cheap way to get the total number of versions in general; we only know for sure when there's just one page. We use -1 in the API and document that this means we don't know. Change-Id: I58ca49a0e3f8b228a0aa2626e0e2982ec217540f Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/786041 Reviewed-by: Ethan Lee <ethanalee@google.com> kokoro-CI: kokoro <noreply+kokoro@google.com> LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
diff --git a/internal/api/api.go b/internal/api/api.go index 6e2beee..4fb41c9 100644 --- a/internal/api/api.go +++ b/internal/api/api.go
@@ -198,10 +198,12 @@ // ServeModuleVersions handles requests for the v1beta module versions endpoint. // api:route /v1beta/versions/{path} // api:desc Versions of the module at {path}. -// api:desc If there are tagged versions, they are returned. -// api:desc Otherwise, the 10 most recent pseudo-versions are returned. -// api:desc The versions are in descending order. +// api:desc The versions are returned in descending semantic version order, +// api:desc with compatible versions listed first, followed by incompatible versions. +// api:desc Only tagged versions are returned, unless the pseudo query parameter is true. // api:desc Only results that match the filter query parameter are returned. +// api:desc The total in the response is -1 to indicate that the total number of results is unknown, +// api:desc unless all results fit on a single page. // api:example /v1beta/versions/golang.org/x/time?limit=3 func ServeModuleVersions(w http.ResponseWriter, r *http.Request, ds internal.DataSource) (err error) { defer derrors.Wrap(&err, "ServeModuleVersions") @@ -222,14 +224,51 @@ if err != nil { return fmt.Errorf("module %q: %w", path, err) } + // TODO: generalize this endpoint to packages. + // That is how the DB query works. if err := checkModulePath(path, um.ModulePath); err != nil { return err } - infos, err := ds.GetVersionsForPath(r.Context(), path) + limit := params.Limit + if limit <= 0 { + limit = defaultLimit + } + if limit > maxLimit { + limit = maxLimit + } + + start := "" + if params.Token != "" { + var err error + start, err = decodeStringPageToken(params.Token) + if err != nil { + return BadRequest(fmt.Sprintf("invalid next-page token: %v", err), "try again from the beginning, with no token") + } + } + + // Determine version types to fetch: either just the tagged ones, + // or all of them. + vts := []version.Type{version.TypeRelease, version.TypePrerelease} + if params.PseudoVersions { + vts = append(vts, version.TypePseudo) + } + infos, next, err := ds.GetPathVersions(r.Context(), path, start, limit+1, vts...) if err != nil { return err } + nextToken := "" + if len(infos) > limit { + if len(infos) != limit+1 { + return InternalServerError("len(infos)=%d, expected %d", len(infos), limit+1) + } + infos = infos[:limit] + nextToken, err = encodeStringPageToken(next) + if err != nil { + return err + } + } + var mvs []ModuleVersion for _, in := range infos { mvs = append(mvs, ModuleVersion{ @@ -250,10 +289,19 @@ return err } + // In general, we don't know the total number of versions. The only time we can be + // sure is if this is the first page, and there is no next page. + // In that case, we count the number of filtered results. + total := -1 + if start == "" && nextToken == "" { + total = len(mvs) + } + // api:response PaginatedResponse[ModuleVersion] - resp, err := paginate(mvs, params.ListParams, defaultLimit) - if err != nil { - return err + resp := PaginatedResponse[ModuleVersion]{ + Items: mvs, + Total: total, + NextPageToken: nextToken, } // The response is never immutable, because a new version can arrive at any time. @@ -519,17 +567,16 @@ } modulePath := um.ModulePath + // TODO(jba): share limit and start code between this + // and versions. limit := params.Limit - // If the user doesn't provide a limit, use a default. if limit <= 0 { limit = defaultLimit } - // Cap the user-supplied limit so we don't do too much work. if limit > maxLimit { limit = maxLimit } - // Resolve start path from token start := "" if params.Token != "" { var err error
diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 81cf5c1..292efd2 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml
@@ -549,7 +549,7 @@ }, "/versions/{path}": { "get": { - "description": "Versions of the module at {path}.\nIf there are tagged versions, they are returned.\nOtherwise, the 10 most recent pseudo-versions are returned.\nThe versions are in descending order.\nOnly results that match the filter query parameter are returned.", + "description": "Versions of the module at {path}.\nThe versions are returned in descending semantic version order,\nwith compatible versions listed first, followed by incompatible versions.\nOnly tagged versions are returned, unless the pseudo query parameter is true.\nOnly results that match the filter query parameter are returned.\nThe total in the response is -1 to indicate that the total number of results is unknown,\nunless all results fit on a single page.", "operationId": "getVersions", "parameters": [ { @@ -584,6 +584,14 @@ "schema": { "type": "string" } + }, + { + "description": "Whether to include pseudo-versions in the result.", + "in": "query", + "name": "pseudo", + "schema": { + "type": "boolean" + } } ], "responses": {
diff --git a/internal/api/params.go b/internal/api/params.go index 32b8403..d2f4e6e 100644 --- a/internal/api/params.go +++ b/internal/api/params.go
@@ -81,6 +81,8 @@ // VersionsParams are query parameters for /v1beta/versions/{path}. type VersionsParams struct { ListParams + // Whether to include pseudo-versions in the result. + PseudoVersions bool `form:"pseudo"` } // PackagesParams are query parameters for /v1beta/packages/{path}.
diff --git a/internal/datasource.go b/internal/datasource.go index 73a561e..a8ee7bf 100644 --- a/internal/datasource.go +++ b/internal/datasource.go
@@ -8,6 +8,8 @@ "context" "testing" "time" + + "golang.org/x/pkgsite/internal/version" ) // SearchOptions provide information used by db.Search. @@ -98,8 +100,8 @@ // GetLatestInfo gets information about the latest versions of a unit and module. // See LatestInfo for documentation. GetLatestInfo(ctx context.Context, unitPath, modulePath string, latestUnitMeta *UnitMeta) (LatestInfo, error) - // GetVersionsForPath returns a list of versions for the given path. - GetVersionsForPath(ctx context.Context, path string) ([]*ModuleInfo, error) + // GetPathVersions returns a list of versions for the given path. + GetPathVersions(ctx context.Context, path, start string, limit int, versionTypes ...version.Type) (_ []*ModuleInfo, next string, err error) // GetModulePackages returns a list of packages in the given module version. GetModulePackages(ctx context.Context, modulePath, version string) ([]*PackageMeta, error) // GetSymbols returns symbols for the given unit and build context.
diff --git a/internal/fetchdatasource/fetchdatasource.go b/internal/fetchdatasource/fetchdatasource.go index 500b082..11cb79c 100644 --- a/internal/fetchdatasource/fetchdatasource.go +++ b/internal/fetchdatasource/fetchdatasource.go
@@ -334,9 +334,9 @@ return nil, nil } -// GetVersionsForPath is not implemented. -func (ds *FetchDataSource) GetVersionsForPath(ctx context.Context, path string) ([]*internal.ModuleInfo, error) { - return nil, nil +// GetPathVersions is not implemented. +func (ds *FetchDataSource) GetPathVersions(ctx context.Context, path, start string, limit int, versionTypes ...version.Type) (_ []*internal.ModuleInfo, next string, err error) { + return nil, "", nil } // GetModuleReadme is not implemented.
diff --git a/internal/interfaces.go b/internal/interfaces.go index 5bb217b..329a664 100644 --- a/internal/interfaces.go +++ b/internal/interfaces.go
@@ -20,7 +20,7 @@ GetSymbolHistory(ctx context.Context, packagePath, modulePath string) (_ *SymbolHistory, err error) GetVersionMap(ctx context.Context, modulePath, requestedVersion string) (_ *VersionMap, err error) GetVersionMaps(ctx context.Context, paths []string, requestedVersion string) (_ []*VersionMap, err error) - GetVersionsForPath(ctx context.Context, path string) (_ []*ModuleInfo, err error) InsertModule(ctx context.Context, m *Module, lmv *LatestModuleVersions) (isLatest bool, err error) UpsertVersionMap(ctx context.Context, vm *VersionMap) (err error) + GetVersionsForPath(ctx context.Context, path string) ([]*ModuleInfo, error) }
diff --git a/internal/postgres/delete_test.go b/internal/postgres/delete_test.go index 78d3a90..6fa0b6f 100644 --- a/internal/postgres/delete_test.go +++ b/internal/postgres/delete_test.go
@@ -178,14 +178,14 @@ if err := testDB.DeletePseudoversionsExcept(ctx, sample.ModulePath, pseudo1); err != nil { t.Fatal(err) } - mods, _, err := getPathVersions(ctx, testDB, sample.ModulePath, "", 800, version.TypeRelease) + mods, _, err := testDB.GetPathVersions(ctx, sample.ModulePath, "", 800, version.TypeRelease) if err != nil { t.Fatal(err) } if len(mods) != 1 && mods[0].Version != sample.VersionString { t.Errorf("module version %q was not found", sample.VersionString) } - mods, _, err = getPathVersions(ctx, testDB, sample.ModulePath, "", 10, version.TypePseudo) + mods, _, err = testDB.GetPathVersions(ctx, sample.ModulePath, "", 10, version.TypePseudo) if err != nil { t.Fatal(err) }
diff --git a/internal/postgres/version.go b/internal/postgres/version.go index b6000c2..ae27638 100644 --- a/internal/postgres/version.go +++ b/internal/postgres/version.go
@@ -9,7 +9,6 @@ "database/sql" "errors" "fmt" - "io" "strconv" "strings" @@ -28,9 +27,9 @@ // GetVersionsForPath returns a list of tagged versions sorted in // descending semver order if any exist. If none, it returns the 10 most // recent from a list of pseudo-versions sorted in descending semver order. +// TODO: move this and its test to internal/frontend, the only place it's used. func (db *DB) GetVersionsForPath(ctx context.Context, path string) (_ []*internal.ModuleInfo, err error) { - defer derrors.WrapStack(&err, "GetVersionsForPath(ctx, %q)", path) - defer stats.Elapsed(ctx, "GetVersionsForPath")() + defer derrors.WrapStack(&err, "getVersionsForPath(ctx, %q)", path) // When a page shows too many versions, it can result in a Chrome CSS // bug: https://bugs.chromium.org/p/chromium/issues/detail?id=688640. @@ -38,31 +37,29 @@ // https://pkg.go.dev/github.com/aws/aws-sdk-go/aws/signer/v4?tab=versions. // It's not that useful to see that many versions on a page anyway, so // just limit to 800 versions. - versions, lsv, err := getPathVersions(ctx, db, path, "", 800, version.TypeRelease, version.TypePrerelease) + versions, _, err := db.GetPathVersions(ctx, path, "", 800, version.TypeRelease, version.TypePrerelease) if err != nil { return nil, err } if len(versions) != 0 { return versions, nil } - versions, _, err = getPathVersions(ctx, db, path, "", 10, version.TypePseudo) + versions, _, err = db.GetPathVersions(ctx, path, "", 10, version.TypePseudo) if err != nil { return nil, err } - // To satisfy unparam until a subsequent CL. - // TODO(jba); remove this. - fmt.Fprint(io.Discard, lsv) return versions, nil } -// getPathVersions returns a list of versions sorted in descending semver +// GetPathVersions returns a list of versions sorted in descending semver // order. The version types included in the list are specified by a list of // VersionTypes. // The result can be paginated by passing pageToken, which should be either the empty // string or a value returned from a previous call. // Each subsequent page will begin with the last value of the previous page. -func getPathVersions(ctx context.Context, db *DB, path string, startPageToken string, limit int, versionTypes ...version.Type) (_ []*internal.ModuleInfo, nextPageToken string, err error) { - defer derrors.WrapStack(&err, "getPathVersions(ctx, db, %q, %q, %d, %v)", path, startPageToken, limit, versionTypes) +func (db *DB) GetPathVersions(ctx context.Context, path string, startPageToken string, limit int, versionTypes ...version.Type) (_ []*internal.ModuleInfo, nextPageToken string, err error) { + defer derrors.WrapStack(&err, "DB.GetPathVersions(ctx, db, %q, %q, %d, %v)", path, startPageToken, limit, versionTypes) + defer stats.Elapsed(ctx, "GetPathVersions")() // Get previous values from pageToken. var pageTokenArgs = []any{false, "", ""} @@ -78,7 +75,6 @@ m.redistributable, m.has_go_mod, m.source_info, - -- to construct the page token m.incompatible, m.sort_version FROM modules m @@ -102,7 +98,7 @@ %s` if len(versionTypes) == 0 { - return nil, "", fmt.Errorf("error: must specify at least one version type") + return nil, "", errors.New("error: must specify at least one version type") } queryEnd := ";" if limit > 0 {
diff --git a/internal/postgres/version_test.go b/internal/postgres/version_test.go index 296d07a..88e655e 100644 --- a/internal/postgres/version_test.go +++ b/internal/postgres/version_test.go
@@ -288,7 +288,7 @@ for _, m := range testModules { testDB.MustInsertModule(t, m) } - gotmods, _, err := getPathVersions(t.Context(), testDB, "m.com/a", "", 0, version.TypePrerelease, version.TypeRelease, version.TypePseudo) + gotmods, _, err := testDB.GetPathVersions(t.Context(), "m.com/a", "", 0, version.TypePrerelease, version.TypeRelease, version.TypePseudo) if err != nil { t.Fatal(err) } @@ -765,7 +765,7 @@ versionIndex := 0 for { - mods, nextStart, err := getPathVersions(t.Context(), testDB, packagePath, start, limit, version.TypeRelease, version.TypePrerelease, version.TypePseudo) + mods, nextStart, err := testDB.GetPathVersions(t.Context(), packagePath, start, limit, version.TypeRelease, version.TypePrerelease, version.TypePseudo) if err != nil { t.Fatal(err) }
diff --git a/internal/testing/fakedatasource/fakedatasource.go b/internal/testing/fakedatasource/fakedatasource.go index 442490a..49cfd2e 100644 --- a/internal/testing/fakedatasource/fakedatasource.go +++ b/internal/testing/fakedatasource/fakedatasource.go
@@ -8,6 +8,7 @@ import ( "context" "fmt" + "slices" "sort" "strings" "testing" @@ -453,10 +454,15 @@ return nil, errNotImplemented } -// GetVersionsForPath returns a list of tagged versions sorted in -// descending semver order if any exist. If none, it returns the 10 most -// recent from a list of pseudo-versions sorted in descending semver order. -func (ds *FakeDataSource) GetVersionsForPath(ctx context.Context, path string) ([]*internal.ModuleInfo, error) { +// GetPathVersions returns a list of versions for the given path. +func (ds *FakeDataSource) GetPathVersions(ctx context.Context, path, start string, limit int, versionTypes ...version.Type) (_ []*internal.ModuleInfo, next string, err error) { + + // Find the v1 path for the argument path. + // For example, if path is example.com/foo/v2, then the + // v1 path is example.com/foo. + // If path is a package, the v1 path is the v1 path of the module + // followed by the relative package path. + // For example: example.com/foo/v2/pkg -> example.com/foo/pkg. var targetV1Path string for _, m := range ds.modules { if findUnit(m, path) != nil { @@ -465,9 +471,11 @@ } } if targetV1Path == "" { - return nil, nil + return nil, "", nil } + // Find all modules with a package whose v1 path matches + // the target v1 path. var infos []*internal.ModuleInfo for _, m := range ds.modules { for _, u := range m.Units { @@ -478,26 +486,66 @@ } } - // Only keep pseudoversions if we only have pseudoversions. - var nonPseudo []*internal.ModuleInfo - for _, info := range infos { - if !version.IsPseudo(info.Version) { - nonPseudo = append(nonPseudo, info) + // Keep only those modules whose version type is one of the + // requested ones. + var filtered []*internal.ModuleInfo + for _, mi := range infos { + t, err := version.ParseType(mi.Version) + if err != nil { + return nil, "", err + } + if slices.Contains(versionTypes, t) { + filtered = append(filtered, mi) } } - if len(nonPseudo) > 0 { - infos = nonPseudo + + if len(filtered) == 0 { + return nil, "", nil } - sort.Slice(infos, func(i, j int) bool { - return version.ForSorting(infos[i].Version) > version.ForSorting(infos[j].Version) + sort.Slice(filtered, func(i, j int) bool { + return semver.Compare(filtered[i].Version, filtered[j].Version) > 0 }) - if len(nonPseudo) == 0 && len(infos) > 10 { - infos = infos[:10] + // Apply start and limit to the list. + if start != "" { + startIndex := slices.IndexFunc(filtered, func(mi *internal.ModuleInfo) bool { + return semver.Compare(mi.Version, start) <= 0 + }) + fmt.Println("sx", startIndex) + if startIndex == -1 { + return nil, "", nil + } + filtered = filtered[startIndex:] } - return infos, nil + if limit > 0 && len(filtered) > limit { + filtered = filtered[:limit] + } + + if len(filtered) == 0 { + return nil, "", nil + } + + return filtered, filtered[len(filtered)-1].Version, nil +} + +// GetVersionsForPath returns a list of tagged versions sorted in +// descending semver order if any exist. If none, it returns the 10 most +// recent from a list of pseudo-versions sorted in descending semver order. +func (ds *FakeDataSource) GetVersionsForPath(ctx context.Context, path string) ([]*internal.ModuleInfo, error) { + versions, _, err := ds.GetPathVersions(ctx, path, "", 800, version.TypeRelease, version.TypePrerelease) + if err != nil { + return nil, err + } + if len(versions) != 0 { + return versions, nil + } + versions, _, err = ds.GetPathVersions(ctx, path, "", 10, version.TypePseudo) + if err != nil { + return nil, err + } + return versions, nil } // InsertModule inserts m into the FakeDataSource. It is only implemented for
diff --git a/internal/tests/api/api_test.go b/internal/tests/api/api_test.go index fb6a9de..d5b22a9 100644 --- a/internal/tests/api/api_test.go +++ b/internal/tests/api/api_test.go
@@ -680,6 +680,7 @@ ds.MustInsertModule(t, newMod("example.com", "v1.0.0", "v1.1.0")) ds.MustInsertModule(t, newMod("example.com", "v1.1.0", "v1.1.0")) ds.MustInsertModule(t, newMod("example.com/v2", "v2.0.0", "v2.0.0")) + ds.MustInsertModule(t, newMod("example.com", "v0.0.0-20140414041502-3c2ca4d52544", "v1.1.0")) for _, test := range []struct { name string @@ -714,6 +715,39 @@ }, }, { + name: "pseudo=true", + url: "/v1beta/versions/example.com?pseudo=true", + want: &api.PaginatedResponse[api.ModuleVersion]{ + Total: 4, + Items: []api.ModuleVersion{ + { + ModulePath: "example.com/v2", + Version: "v2.0.0", + LatestVersion: "v2.0.0", + IsRedistributable: true, + }, + { + ModulePath: "example.com", + Version: "v1.1.0", + LatestVersion: "v1.1.0", + IsRedistributable: true, + }, + { + ModulePath: "example.com", + Version: "v1.0.0", + LatestVersion: "v1.1.0", + IsRedistributable: true, + }, + { + ModulePath: "example.com", + Version: "v0.0.0-20140414041502-3c2ca4d52544", + LatestVersion: "v1.1.0", + IsRedistributable: true, + }, + }, + }, + }, + { name: "module not found", url: "/v1beta/versions/nonexistent.com", want: &api.Error{Code: 404, Message: "not found"}, @@ -819,15 +853,15 @@ }) } - testPagination[api.PaginatedResponse[api.ModuleVersion]](t, ds, "/v1beta/versions/example.com?limit=1", + testPagination(t, ds, "/v1beta/versions/example.com?limit=1", api.ServeModuleVersions, func(r *api.PaginatedResponse[api.ModuleVersion]) (int, int, string) { return len(r.Items), r.Total, r.NextPageToken }, []wantPage{ - {wantCount: 1, wantTotal: 3}, - {wantCount: 1, wantTotal: 3}, - {wantCount: 1, wantTotal: 3}, + {wantCount: 1, wantTotal: -1}, + {wantCount: 1, wantTotal: -1}, + {wantCount: 1, wantTotal: -1}, }) }