From c76866da887dbe897088f5cab55e9230a9c53c4a Mon Sep 17 00:00:00 2001 From: Jonathan Amsterdam Date: Sun, 18 Aug 2019 04:33:41 -0400 Subject: [PATCH] internal/frontend: group imported-by paths Instead of displaying the paths imported by a module as a flat list, use
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 --- content/static/css/stylesheet.css | 7 + content/static/html/pages/pkg_importedby.tmpl | 30 +++- internal/frontend/details.go | 2 +- internal/frontend/imports.go | 32 +++-- internal/frontend/imports_test.go | 22 +-- internal/frontend/section.go | 134 ++++++++++++++++++ internal/frontend/section_test.go | 110 ++++++++++++++ internal/postgres/details.go | 46 +++--- internal/postgres/details_test.go | 82 +++-------- 9 files changed, 356 insertions(+), 109 deletions(-) create mode 100644 internal/frontend/section.go create mode 100644 internal/frontend/section_test.go diff --git a/content/static/css/stylesheet.css b/content/static/css/stylesheet.css index c5a6675b..4918f63e 100644 --- a/content/static/css/stylesheet.css +++ b/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 +} diff --git a/content/static/html/pages/pkg_importedby.tmpl b/content/static/html/pages/pkg_importedby.tmpl index 7a1a10e4..079b8b87 100644 --- a/content/static/html/pages/pkg_importedby.tmpl +++ b/content/static/html/pages/pkg_importedby.tmpl @@ -9,16 +9,32 @@ {{if .ImportedBy}}

Imported by

- {{template "pagination_summary" .Pagination}} {{pluralize .Pagination.TotalCount "importer"}} + Known {{pluralize .Total "importer"}}: {{.Total}}{{if not .TotalIsExact}}+{{end}}

-
    - {{range .ImportedBy}} -
  • {{.}}
    • - {{end}} -
- {{template "pagination_nav" .Pagination}} + {{template "sections" .ImportedBy}} {{else}} {{template "empty_content" "No known importers for this package!"}} {{end}} {{end}} + +{{define "sections"}} +
    + {{range .}} + {{template "section" .}} + {{end}} +
+{{end}} + +{{define "section"}} + {{if .Subs}} +
+ {{.Prefix}} ({{.NumLines}}) +
+ {{template "sections" .Subs}} +
+
+ {{else}} +
  • {{.Prefix}}
    • + {{end}} +{{end}} diff --git a/internal/frontend/details.go b/internal/frontend/details.go index 4a6ad129..1b1a1cbe 100644 --- a/internal/frontend/details.go +++ b/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": diff --git a/internal/frontend/imports.go b/internal/frontend/imports.go index 568d02a5..beeb79ee 100644 --- a/internal/frontend/imports.go +++ b/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 } diff --git a/internal/frontend/imports_test.go b/internal/frontend/imports_test.go index 7b8b6091..58d2fea1 100644 --- a/internal/frontend/imports_test.go +++ b/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) } }) } diff --git a/internal/frontend/section.go b/internal/frontend/section.go new file mode 100644 index 00000000..e428c8e6 --- /dev/null +++ b/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] +} diff --git a/internal/frontend/section_test.go b/internal/frontend/section_test.go new file mode 100644 index 00000000..23ee97a3 --- /dev/null +++ b/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) + } + } +} diff --git a/internal/postgres/details.go b/internal/postgres/details.go index 02155b9a..ad79c44b 100644 --- a/internal/postgres/details.go +++ b/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 diff --git a/internal/postgres/details_test.go b/internal/postgres/details_test.go index aeb54ac3..8047b42a 100644 --- a/internal/postgres/details_test.go +++ b/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) } }) }