internal/frontend: group imported-by paths

Instead of displaying the paths imported by a module as a flat list,
use <details> elements to display a tree.

Make tree nodes for (roughly) "account" and "repo", so that
"github.com/google" is one level of the tree, and
"github.com/google/somerepo" is the next level. Don't group beyond
that on the assumption that users would want to see all the importing
packages in a repo as a flat list.

Also, get rid of pagination on the query. Instead use a large limit,
and only display paths up to that limit.

Fixes b/139411420.

Change-Id: I55e030e5f901dacf854f2e7f585a444d089f6ea1
Reviewed-on: https://team-review.git.corp.google.com/c/golang/discovery/+/537581
Reviewed-by: Julie Qiu <julieqiu@google.com>

content/static/css/stylesheet.css

@@ -582,3 +582,10 @@ code {
 .Directory-header {
   border-bottom: 1px solid var(--gray-8);
 }
+
+.Details-content {
+  margin-left: 40px
+}
+.Details-indent {
+  margin-left: 1.1rem
+}

content/static/html/pages/pkg_importedby.tmpl

@@ -9,16 +9,32 @@
     {{if .ImportedBy}}
       <h2>Imported by</h2>
       <p>
-        {{template "pagination_summary" .Pagination}} {{pluralize .Pagination.TotalCount "importer"}}
+        <b>Known {{pluralize .Total "importer"}}:</b> {{.Total}}{{if not .TotalIsExact}}+{{end}}
       </p>
-      <ul class="ImportedBy-list">
-        {{range .ImportedBy}}
-          <li><a class="u-breakWord" href="/pkg/{{.}}">{{.}}</a></li>
-        {{end}}
-      </ul>
-      {{template "pagination_nav" .Pagination}}
+      {{template "sections" .ImportedBy}}
     {{else}}
       {{template "empty_content" "No known importers for this package!"}}
     {{end}}
   </div>
 {{end}}
+
+{{define "sections"}}
+  <ul class="ImportedBy-list">
+    {{range .}}
+      {{template "section" .}}
+    {{end}}
+  </ul>
+{{end}}
+
+{{define "section"}}
+  {{if .Subs}}
+    <details>
+      <summary>{{.Prefix}} ({{.NumLines}})</summary>
+      <div class="Details-content">
+        {{template "sections" .Subs}}
+      </div>
+    </details>
+  {{else}}
+    <li class="Details-indent"><a class="u-breakWord" href="/pkg/{{.Prefix}}">{{.Prefix}}</a></li>
+  {{end}}
+{{end}}

internal/frontend/details.go

@@ -158,7 +158,7 @@ func fetchDetailsForPackage(ctx context.Context, r *http.Request, tab string, db
 	case "imports":
 		return fetchImportsDetails(ctx, db, pkg)
 	case "importedby":
-		return fetchImportedByDetails(ctx, db, pkg, newPaginationParams(r, 100))
+		return fetchImportedByDetails(ctx, db, pkg)
 	case "licenses":
 		return fetchPackageLicensesDetails(ctx, db, pkg)
 	case "readme":

internal/frontend/imports.go

@@ -59,26 +59,40 @@ func fetchImportsDetails(ctx context.Context, db *postgres.DB, pkg *internal.Ver
 // ImportedByDetails contains information for the collection of packages that
 // import a given package.
 type ImportedByDetails struct {
-	Pagination pagination
-
 	ModulePath string
 
 	// ImportedBy is the collection of packages that import the
 	// given package and are not part of the same module.
-	ImportedBy []string
+	// They are organized into a tree of sections by prefix.
+	ImportedBy []*Section
+
+	Total        int  // number of packages in ImportedBy
+	TotalIsExact bool // if false, then there may be more than Total
 }
 
+const importedByLimit = 7001
+
 // fetchImportedByDetails fetches importers for the package version specified by
 // path and version from the database and returns a ImportedByDetails.
-func fetchImportedByDetails(ctx context.Context, db *postgres.DB, pkg *internal.VersionedPackage, pageParams paginationParams) (*ImportedByDetails, error) {
-
-	importedBy, total, err := db.GetImportedBy(ctx, pkg.Path, pkg.ModulePath, pageParams.limit, pageParams.offset())
+func fetchImportedByDetails(ctx context.Context, db *postgres.DB, pkg *internal.VersionedPackage) (*ImportedByDetails, error) {
+	importedBy, err := db.GetImportedBy(ctx, pkg.Path, pkg.ModulePath, importedByLimit)
 	if err != nil {
 		return nil, err
 	}
+	// If we reached the query limit, then we don't know the total.
+	// Say so, and show one less than the limit.
+	// For example, if the limit is 101 and we get 101 results, then we'll
+	// say there are more than 100, and show the first 100.
+	totalIsExact := true
+	if len(importedBy) == importedByLimit {
+		importedBy = importedBy[:len(importedBy)-1]
+		totalIsExact = false
+	}
+	sections := Sections(importedBy, nextPrefixAccount)
 	return &ImportedByDetails{
-		ModulePath: pkg.VersionInfo.ModulePath,
-		ImportedBy: importedBy,
-		Pagination: newPagination(pageParams, len(importedBy), total),
+		ModulePath:   pkg.VersionInfo.ModulePath,
+		ImportedBy:   sections,
+		Total:        len(importedBy),
+		TotalIsExact: totalIsExact,
 	}, nil
 }

internal/frontend/imports_test.go

@@ -9,7 +9,6 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
-	"github.com/google/go-cmp/cmp/cmpopts"
 	"golang.org/x/discovery/internal"
 	"golang.org/x/discovery/internal/postgres"
 	"golang.org/x/discovery/internal/sample"
@@ -114,18 +113,25 @@ func TestFetchImportedByDetails(t *testing.T) {
 	}{
 		{
 			pkg:         pkg3,
-			wantDetails: &ImportedByDetails{},
+			wantDetails: &ImportedByDetails{TotalIsExact: true},
 		},
 		{
 			pkg: pkg2,
 			wantDetails: &ImportedByDetails{
-				ImportedBy: []string{pkg3.Path},
+				ImportedBy:   []*Section{{Prefix: pkg3.Path, NumLines: 0}},
+				Total:        1,
+				TotalIsExact: true,
 			},
 		},
 		{
 			pkg: pkg1,
 			wantDetails: &ImportedByDetails{
-				ImportedBy: []string{pkg2.Path, pkg3.Path},
+				ImportedBy: []*Section{
+					{Prefix: pkg2.Path, NumLines: 0},
+					{Prefix: pkg3.Path, NumLines: 0},
+				},
+				Total:        2,
+				TotalIsExact: true,
 			},
 		},
 	} {
@@ -133,15 +139,15 @@ func TestFetchImportedByDetails(t *testing.T) {
 			otherVersion := newVersion("path.to/foo", tc.pkg)
 			otherVersion.Version = "v1.0.5"
 			vp := firstVersionedPackage(otherVersion)
-			got, err := fetchImportedByDetails(ctx, testDB, vp, paginationParams{limit: 20, page: 1})
+			got, err := fetchImportedByDetails(ctx, testDB, vp)
 			if err != nil {
-				t.Fatalf("fetchImportedByDetails(ctx, db, %q, 1) = %v err = %v, want %v",
+				t.Fatalf("fetchImportedByDetails(ctx, db, %q) = %v err = %v, want %v",
 					tc.pkg.Path, got, err, tc.wantDetails)
 			}
 
 			tc.wantDetails.ModulePath = vp.VersionInfo.ModulePath
-			if diff := cmp.Diff(tc.wantDetails, got, cmpopts.IgnoreFields(ImportedByDetails{}, "Pagination")); diff != "" {
-				t.Errorf("fetchImportedByDetails(ctx, db, %q, 1) mismatch (-want +got):\n%s", tc.pkg.Path, diff)
+			if diff := cmp.Diff(tc.wantDetails, got); diff != "" {
+				t.Errorf("fetchImportedByDetails(ctx, db, %q) mismatch (-want +got):\n%s", tc.pkg.Path, diff)
 			}
 		})
 	}

internal/frontend/section.go

@@ -0,0 +1,134 @@
+// Copyright 2019 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 (
+	"strings"
+)
+
+// A Section represents a collection of lines with a common prefix. The
+// collection is itself divided into sections by prefix, forming a tree.
+type Section struct {
+	Prefix   string     // prefix for this section, or if Subs==nil, a single line
+	Subs     []*Section // subsections
+	NumLines int        // total number of lines in subsections
+}
+
+func newSection(prefix string) *Section {
+	return &Section{Prefix: prefix, NumLines: 0}
+}
+
+func (s *Section) add(sub *Section) {
+	s.Subs = append(s.Subs, sub)
+	if sub.Subs == nil {
+		s.NumLines++
+	} else {
+		s.NumLines += sub.NumLines
+	}
+}
+
+// A prefixFunc returns the next prefix of s, given the current prefix.
+// It should return the empty string if there are no more prefixes.
+type prefixFunc func(s, prefix string) string
+
+// Sections transforms a list of lines, which must be sorted, into a list
+// of Sections. Each Section in the result contains all the contiguous lines
+// with the same prefix.
+//
+// The nextPrefix function is responsible for extracting prefixes from lines.
+func Sections(lines []string, nextPrefix prefixFunc) []*Section {
+	s, _ := section("", lines, nextPrefix)
+	return s.Subs
+}
+
+// section collects all lines with the same prefix into a section. It assumes
+// that lines is sorted. It returns the section along with the remaining lines.
+func section(prefix string, lines []string, nextPrefix prefixFunc) (*Section, []string) {
+	s := newSection(prefix)
+	for len(lines) > 0 {
+		l := lines[0]
+		if !strings.HasPrefix(l, prefix) {
+			break
+		}
+
+		np := nextPrefix(l, prefix)
+		var sub *Section
+		if np == "" {
+			sub = newSection(l)
+			lines = lines[1:]
+		} else {
+			sub, lines = section(np, lines, nextPrefix)
+		}
+		s.add(sub)
+	}
+	// Collapse a section with a single subsection, except at top level.
+	if len(s.Subs) == 1 && prefix != "" {
+		s = s.Subs[0]
+	}
+	return s, lines
+}
+
+// nextPrefixAccount is a prefixFunc (see above). Its first argument
+// is an import path, and its second is the previous prefix that it returned
+// for that path, or "" if this is the first prefix.
+//
+// nextPrefixAccount tries to return an initial prefix for path
+// that consists of the "account": the entity controlling the
+// remainder of the path. In the most common case, paths beginning
+// with "github.com", the account is the second path element, the GitHub user or org.
+// So for example, the first prefix of "github.com/google/go-cmp/cmp" is
+// "github.com/google/".
+//
+// nextPrefixAccount returns a second prefix that is one path element past the
+// account. For github.com paths, this is the repo. Continuing the above example,
+// the second prefix is "github.com/google/go-cmp/".
+//
+// nextPrefixAccount does not return any prefixes beyond those two.
+func nextPrefixAccount(path, pre string) string {
+	// If the last prefix consisted of the entire path, then
+	// there is no next prefix.
+	if path == pre {
+		return ""
+	}
+	parts := strings.Split(path, "/")
+	prefix1, acctParts := accountPrefix(parts)
+	if pre == "" {
+		return prefix1
+	}
+	if pre == prefix1 {
+		// Second prefix: one element past the first.
+		// The +1 is safe because we know that pre is shorter than path from
+		// the first test of the function.
+		prefix2 := strings.Join(parts[:len(acctParts)+1], "/")
+		if prefix2 != path {
+			prefix2 += "/"
+		}
+		return prefix2
+	}
+	// No more prefixes after the first two.
+	return ""
+}
+
+// accountPrefix guesses the prefix of the path (split into parts at "/")
+// that corresponds to the account.
+func accountPrefix(parts []string) (string, []string) {
+	// TODO(jba): handle repo import paths like "example.org/foo/bar.hg".
+	var n int // index of account in parts
+	// The first two cases below handle the special cases that the go command does.
+	// See "go help importpath".
+	switch parts[0] {
+	case "github.com", "bitbucket.org", "launchpad.net":
+		n = 1
+	case "hub.jazz.net":
+		n = 2
+	default:
+		// For custom import paths, use the host as the first prefix.
+		n = 0
+	}
+	if n >= len(parts)-1 {
+		return strings.Join(parts, "/"), parts
+	}
+	return strings.Join(parts[:n+1], "/") + "/", parts[:n+1]
+}

internal/frontend/section_test.go

@@ -0,0 +1,110 @@
+// Copyright 2019 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 (
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+)
+
+func TestNextPrefixAccount(t *testing.T) {
+	for _, tc := range []struct {
+		path, want1, want2 string
+	}{
+		{"", "", ""},
+		{"github.com", "github.com", ""},
+		{"github.com/user", "github.com/user", ""},
+		{"github.com/user/repo", "github.com/user/", "github.com/user/repo"},
+		{"github.com/user/repo/more", "github.com/user/", "github.com/user/repo/"},
+		{"hub.jazz.net/git/user/project/more", "hub.jazz.net/git/user/", "hub.jazz.net/git/user/project/"},
+		{"k8s.io/a", "k8s.io/", "k8s.io/a"},
+		{"k8s.io/a/b", "k8s.io/", "k8s.io/a/"},
+		{"example.com", "example.com", ""},
+		{"example.com/foo", "example.com/", "example.com/foo"},
+		{"example.com/foo/bar", "example.com/", "example.com/foo/"},
+	} {
+		got1 := nextPrefixAccount(tc.path, "")
+		if got1 != tc.want1 {
+			t.Errorf(`nextPrefixAccount(%q, "") = %q, want %q`, tc.path, got1, tc.want1)
+			continue
+		}
+		got2 := nextPrefixAccount(tc.path, got1)
+		if got2 != tc.want2 {
+			t.Errorf(`nextPrefixAccount(%q, %q) = %q, want %q`, tc.path, got1, got2, tc.want2)
+			continue
+		}
+		if got2 == "" {
+			continue
+		}
+		got3 := nextPrefixAccount(tc.path, got2)
+		if got3 != "" {
+			t.Errorf(`nextPrefixAccount(%q, %q) = %q, want ""`, tc.path, got2, got3)
+		}
+	}
+}
+
+func TestPrefixSections(t *testing.T) {
+	for _, tc := range []struct {
+		lines []string
+		want  []*Section
+	}{
+		{
+			[]string{"foo.com/a", "bar.com/a", "baz.com/a"},
+			[]*Section{
+				{"foo.com/a", nil, 0},
+				{"bar.com/a", nil, 0},
+				{"baz.com/a", nil, 0},
+			},
+		},
+		{
+			[]string{"k8s.io/a", "k8s.io/b", "k8s.io/c"},
+			[]*Section{
+				{
+					"k8s.io/",
+					[]*Section{
+						{"k8s.io/a", nil, 0},
+						{"k8s.io/b", nil, 0},
+						{"k8s.io/c", nil, 0},
+					},
+					3,
+				},
+			},
+		},
+		{
+			[]string{
+				"github.com/eliben/gocdkx/blob",
+				"github.com/eliben/gocdkx/blob/azureblob",
+				"github.com/eliben/gocdkx/blob/fileblob",
+				"github.com/eliben/gocdkx/internal/docstore/dynamodocstore",
+				"github.com/eliben/gocdkx/internal/testing/octest",
+				"github.com/eliben/gocdkx/internal/trace",
+				"github.com/eliben/gocdkx/pubsub",
+				"github.com/eliben/gocdkx/pubsub/awspubsub",
+			},
+			[]*Section{
+				{
+					"github.com/eliben/gocdkx/",
+					[]*Section{
+						{"github.com/eliben/gocdkx/blob", nil, 0},
+						{"github.com/eliben/gocdkx/blob/azureblob", nil, 0},
+						{"github.com/eliben/gocdkx/blob/fileblob", nil, 0},
+						{"github.com/eliben/gocdkx/internal/docstore/dynamodocstore", nil, 0},
+						{"github.com/eliben/gocdkx/internal/testing/octest", nil, 0},
+						{"github.com/eliben/gocdkx/internal/trace", nil, 0},
+						{"github.com/eliben/gocdkx/pubsub", nil, 0},
+						{"github.com/eliben/gocdkx/pubsub/awspubsub", nil, 0},
+					},
+					8,
+				},
+			},
+		},
+	} {
+		got := Sections(tc.lines, nextPrefixAccount)
+		if diff := cmp.Diff(tc.want, got); diff != "" {
+			t.Errorf("%v: mismatch (-want, +got):\n%s", tc.lines, diff)
+		}
+	}
+}

internal/postgres/details.go

@@ -425,48 +425,46 @@ func (db *DB) GetImports(ctx context.Context, path, version string) (paths []str
 	return imports, nil
 }
 
+// The limit used in the GetImportedBy query.
+const ImportedByLimit = 7001
+
 // GetImportedBy fetches and returns all of the packages that import the
 // package with path.
 // The returned error may be checked with derrors.IsInvalidArgument to
 // determine if it resulted from an invalid package path or version.
-func (db *DB) GetImportedBy(ctx context.Context, path, modulePath string, limit, offset int) (paths []string, total int, err error) {
-	defer derrors.Wrap(&err, "GetImportedBy(ctx, %q, %q, %d, %d)", path, modulePath, limit, offset)
+//
+// Instead of supporting pagination, this query runs with a limit.
+func (db *DB) GetImportedBy(ctx context.Context, path, modulePath string, limit int) (paths []string, err error) {
+	defer derrors.Wrap(&err, "GetImportedBy(ctx, %q, %q)", path, modulePath)
 	if path == "" {
-		return nil, 0, xerrors.Errorf("path cannot be empty: %w", derrors.InvalidArgument)
+		return nil, xerrors.Errorf("path cannot be empty: %w", derrors.InvalidArgument)
 	}
-
 	query := `
 		SELECT
-			from_path,
-			COUNT(*) OVER() as total
-			FROM (
-				SELECT
-					DISTINCT ON (from_path) from_path
-				FROM
-					imports
-				WHERE
-					to_path = $1
-				AND
-					from_module_path <> $2
-				ORDER BY
-					from_path
-			) t
-		LIMIT $3
-		OFFSET $4;`
+			DISTINCT from_path
+		FROM
+			imports
+		WHERE
+			to_path = $1
+		AND
+			from_module_path <> $2
+		ORDER BY
+			from_path
+		LIMIT $3`
 
 	var importedby []string
 	collect := func(rows *sql.Rows) error {
 		var fromPath string
-		if err := rows.Scan(&fromPath, &total); err != nil {
+		if err := rows.Scan(&fromPath); err != nil {
 			return fmt.Errorf("row.Scan(): %v", err)
 		}
 		importedby = append(importedby, fromPath)
 		return nil
 	}
-	if err := db.runQuery(ctx, query, collect, path, modulePath, limit, offset); err != nil {
-		return nil, 0, err
+	if err := db.runQuery(ctx, query, collect, path, modulePath, limit); err != nil {
+		return nil, err
 	}
-	return importedby, total, nil
+	return importedby, nil
 }
 
 // GetModuleLicenses returns all licenses associated with the given module path and

internal/postgres/details_test.go

@@ -193,70 +193,36 @@ func TestPostgres_GetImportsAndImportedBy(t *testing.T) {
 
 	for _, tc := range []struct {
 		path, modulePath, version string
-		limit, offset             int
 		wantImports               []string
 		wantImportedBy            []string
-		wantTotalImportedBy       int
 	}{
 		{
-			path:                pkg3.Path,
-			modulePath:          modulePath3,
-			version:             "v1.3.0",
-			limit:               10,
-			offset:              0,
-			wantImports:         pkg3.Imports,
-			wantImportedBy:      nil,
-			wantTotalImportedBy: 0,
+			path:           pkg3.Path,
+			modulePath:     modulePath3,
+			version:        "v1.3.0",
+			wantImports:    pkg3.Imports,
+			wantImportedBy: nil,
 		},
 		{
-			path:                pkg2.Path,
-			modulePath:          modulePath2,
-			limit:               10,
-			offset:              0,
-			version:             "v1.2.0",
-			wantImports:         pkg2.Imports,
-			wantImportedBy:      []string{pkg3.Path},
-			wantTotalImportedBy: 1,
+			path:           pkg2.Path,
+			modulePath:     modulePath2,
+			version:        "v1.2.0",
+			wantImports:    pkg2.Imports,
+			wantImportedBy: []string{pkg3.Path},
 		},
 		{
-			path:                pkg1.Path,
-			modulePath:          modulePath1,
-			limit:               10,
-			offset:              0,
-			version:             "v1.1.0",
-			wantImports:         nil,
-			wantImportedBy:      []string{pkg2.Path, pkg3.Path},
-			wantTotalImportedBy: 2,
+			path:           pkg1.Path,
+			modulePath:     modulePath1,
+			version:        "v1.1.0",
+			wantImports:    nil,
+			wantImportedBy: []string{pkg2.Path, pkg3.Path},
 		},
 		{
-			path:                pkg1.Path,
-			modulePath:          modulePath1,
-			version:             "v1.1.0",
-			limit:               1,
-			offset:              0,
-			wantImports:         nil,
-			wantImportedBy:      []string{pkg2.Path},
-			wantTotalImportedBy: 2,
-		},
-		{
-			path:                pkg1.Path,
-			modulePath:          modulePath1,
-			version:             "v1.1.0",
-			limit:               1,
-			offset:              1,
-			wantImports:         nil,
-			wantImportedBy:      []string{pkg3.Path},
-			wantTotalImportedBy: 2,
-		},
-		{
-			path:                pkg1.Path,
-			modulePath:          modulePath2, // should cause pkg2 to be excluded.
-			version:             "v1.1.0",
-			limit:               10,
-			offset:              0,
-			wantImports:         nil,
-			wantImportedBy:      []string{pkg3.Path},
-			wantTotalImportedBy: 1,
+			path:           pkg1.Path,
+			modulePath:     modulePath2, // should cause pkg2 to be excluded.
+			version:        "v1.1.0",
+			wantImports:    nil,
+			wantImportedBy: []string{pkg3.Path},
 		},
 	} {
 		t.Run(tc.path, func(t *testing.T) {
@@ -282,16 +248,12 @@ func TestPostgres_GetImportsAndImportedBy(t *testing.T) {
 				t.Errorf("testDB.GetImports(%q, %q) mismatch (-want +got):\n%s", tc.path, tc.version, diff)
 			}
 
-			gotImportedBy, total, err := testDB.GetImportedBy(ctx, tc.path, tc.modulePath, tc.limit, tc.offset)
+			gotImportedBy, err := testDB.GetImportedBy(ctx, tc.path, tc.modulePath, 100)
 			if err != nil {
 				t.Fatal(err)
 			}
-			if total != tc.wantTotalImportedBy {
-				t.Errorf("testDB.GetImportedBy(%q, %q, %d, %d): total = %d, want %d", tc.path, tc.modulePath, tc.limit, tc.offset, total, tc.wantTotalImportedBy)
-			}
-
 			if diff := cmp.Diff(tc.wantImportedBy, gotImportedBy); diff != "" {
-				t.Errorf("testDB.GetImportedBy(%q, %q, %d, %d) mismatch (-want +got):\n%s", tc.path, tc.modulePath, tc.limit, tc.offset, diff)
+				t.Errorf("testDB.GetImportedBy(%q, %q) mismatch (-want +got):\n%s", tc.path, tc.modulePath, diff)
 			}
 		})
 	}