internal/postgres: change versions sort order

In getPath versions, sort first by version,then by module path.

Module path came first in order to group results by module when a package
appears in more than one module. But it also results in out-of-order
versions, because v11 sorts before v2. Now we only use the module path
to break ties when versions are the same.

Incompatible versions still come last.

Fixes #79632.

Change-Id: I1b66135685a0b36478bb0380da03e515ae64b4cb
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/784460
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
kokoro-CI: kokoro <noreply+kokoro@google.com>

Author: jba

internal/postgres/version.go

@@ -94,11 +94,12 @@ func getPathVersions(ctx context.Context, db *DB, path string, startPageToken st
 			LIMIT 1
 		)
 		AND version_type in (%s)
-		AND ($3 = '' OR (NOT m.incompatible, m.module_path, m.sort_version) <= (NOT $2, $3, $4))
+		AND ($3 = '' OR (NOT m.incompatible, m.sort_version, m.module_path) <= (NOT $2, $3, $4))
 	ORDER BY
 		m.incompatible,
-		m.module_path DESC,
-		m.sort_version DESC %s`
+		m.sort_version DESC,
+		m.module_path DESC
+	%s`
 
 	if len(versionTypes) == 0 {
 		return nil, "", fmt.Errorf("error: must specify at least one version type")
@@ -132,17 +133,18 @@ func getPathVersions(ctx context.Context, db *DB, path string, startPageToken st
 	// Construct the page token for the next page.
 	// See the comment near the top of this function for the format.
 	if len(versions) > 0 {
-		nextPageToken = makePageToken(lastIncompatible, versions[len(versions)-1].ModulePath, lastSortVersion)
+		nextPageToken = makePageToken(lastIncompatible, lastSortVersion, versions[len(versions)-1].ModulePath)
 	}
 	return versions, nextPageToken, nil
 }
 
 // parsePageToken parses a page token for getPathVersions.
 // It return a slice of query args.
 func parsePageToken(s string) (queryArgs []any, err error) {
-	// A page token has the form "I P S"
-	// where I is a bool for incompatible version, P is a module path, and S
-	// is a sort version. Spaces suffice to separate these since none can contain a space.
+	// A page token has the form "I S P"
+	// where I is a bool for incompatible version, S
+	// is a sort version and P is a module path.
+	// Spaces suffice to separate these since none can contain a space.
 	parts := strings.Fields(s)
 	if len(parts) != 3 {
 		return nil, errors.New("invalid page token (wrong # parts)")
@@ -155,8 +157,8 @@ func parsePageToken(s string) (queryArgs []any, err error) {
 }
 
 // makePageToken constructs a page token for getPathVersions.
-func makePageToken(inc bool, mpath, version string) string {
-	return fmt.Sprintf("%t %s %s", inc, mpath, version)
+func makePageToken(inc bool, version, mpath string) string {
+	return fmt.Sprintf("%t %s %s", inc, version, mpath)
 }
 
 // versionTypeExpr returns a comma-separated list of version types,

internal/postgres/version_test.go

@@ -19,7 +19,7 @@ import (
 	"golang.org/x/pkgsite/internal/version"
 )
 
-func TestGetVersions(t *testing.T) {
+func TestGetVersionsForPath(t *testing.T) {
 	t.Parallel()
 	var (
 		taggedAndPseudoModule = "path.to/foo"
@@ -266,6 +266,59 @@ func TestGetVersions(t *testing.T) {
 	}
 }
 
+func TestGetPathVersionsSorting(t *testing.T) {
+	// Validate that getPathVersions sorts by version,
+	// then by module path, with incompatible versions last.
+	testDB, release := acquire(t)
+	defer release()
+	testModules := []*internal.Module{
+		sample.Module("m.com/a", "v1.0.0", "pkg"),
+		sample.Module("m.com/a", "v1.2.3", "pkg"),
+		sample.Module("m.com/a", "v1.2.3-pre", "pkg"),
+		sample.Module("m.com/a", "v2.1.1+incompatible", "pkg"),
+		sample.Module("m.com/a", "v2.2.2+incompatible", "pkg"),
+		sample.Module("m.com/a", "v0.0.0-20200101120012-000000000000", "pkg"),
+		sample.Module("m.com/a/v2", "v2.2.2", "pkg"),
+		sample.Module("m.com/a/v2", "v2.1.1", "pkg"),
+		sample.Module("m.com/a/v11", "v11.0.0", "pkg"),
+		// same package path, different module
+		sample.Module("m.com", "v1.2.3", "a/pkg"),
+		sample.Module("m.com", "v1.3.0", "a/pkg"),
+	}
+	for _, m := range testModules {
+		testDB.MustInsertModule(t, m)
+	}
+	gotmods, _, err := getPathVersions(t.Context(), testDB, "m.com/a", "", 0, version.TypePrerelease, version.TypeRelease, version.TypePseudo)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	type modver struct {
+		ModulePath, Version string
+	}
+	want := []modver{
+		{"m.com/a/v11", "v11.0.0"},
+		{"m.com/a/v2", "v2.2.2"},
+		{"m.com/a/v2", "v2.1.1"},
+		{"m.com", "v1.3.0"},
+		// version ties are broken by module path
+		{"m.com/a", "v1.2.3"},
+		{"m.com", "v1.2.3"},
+		{"m.com/a", "v1.2.3-pre"},
+		{"m.com/a", "v1.0.0"},
+		{"m.com/a", "v0.0.0-20200101120012-000000000000"},
+		{"m.com/a", "v2.2.2+incompatible"},
+		{"m.com/a", "v2.1.1+incompatible"},
+	}
+	var got []modver
+	for _, mi := range gotmods {
+		got = append(got, modver{mi.ModulePath, mi.Version})
+	}
+	if diff := cmp.Diff(want, got); diff != "" {
+		t.Errorf("mismatch (-want, +got):\n%s", diff)
+	}
+}
+
 func TestGetLatestInfo(t *testing.T) {
 	t.Parallel()
 	testDB, release := acquire(t)