internal/postgres: use FieldSet in GetDirectory

GetDirectory now supports the WithDocumentation, WithReadme, and AllFields fieldsets. At the moment, only one fieldset can be specified at a time. Additional support will be updated in future CLs. For golang/go#39629 Change-Id: I42b15d2d06a0b36aa0298a79ea1c06303b682926 Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/250946 Run-TryBot: Julie Qiu <julie@golang.org> TryBot-Result: kokoro <noreply+kokoro@google.com> Reviewed-by: Jonathan Amsterdam <jba@google.com>
golang · Aug 28, 2020 · db8f07c · db8f07c
1 parent ddad9a9
commit db8f07c
Show file tree

Hide file tree

Showing 7 changed files with 177 additions and 58 deletions.
diff --git a/internal/datasource.go b/internal/datasource.go
@@ -17,7 +17,7 @@ type DataSource interface {
 
 	// GetDirectory returns information about a directory, which may also be a module and/or package.
 	// The module and version must both be known.
-	GetDirectory(ctx context.Context, dirPath, modulePath, version string, pathID int, fields ...FieldSet) (_ *Directory, err error)
+	GetDirectory(ctx context.Context, dirPath, modulePath, version string, pathID int, field FieldSet) (_ *Directory, err error)
 	// GetDirectoryMeta returns information about a directory.
 	GetDirectoryMeta(ctx context.Context, dirPath, modulePath, version string) (_ *DirectoryMeta, err error)
 	// GetImports returns a slice of import paths imported by the package

diff --git a/internal/frontend/overview.go b/internal/frontend/overview.go
@@ -46,6 +46,7 @@ func fetchOverviewDetails(ctx context.Context, ds internal.DataSource, dmeta *in
 	if err != nil {
 		return nil, err
 	}
+	dir.DirectoryMeta = *dmeta
 	var readme *internal.Readme
 	if dir.Readme != nil {
 		readme = &internal.Readme{Filepath: dir.Readme.Filepath, Contents: dir.Readme.Contents}

diff --git a/internal/nonredist.go b/internal/nonredist.go
@@ -35,7 +35,7 @@ func (d *Directory) RemoveNonRedistributableData() {
 	if !d.IsRedistributable {
 		d.Readme = nil
 		if d.Package != nil {
-			d.Package.Documentation = &Documentation{}
+			d.Package.Documentation = nil
 		}
 	}
 }

diff --git a/internal/postgres/directory.go b/internal/postgres/directory.go
@@ -81,50 +81,98 @@ func (db *DB) GetPackagesInDirectory(ctx context.Context, dirPath, modulePath, r
 	return packages, nil
 }
 
-func (db *DB) GetDirectory(ctx context.Context, dirPath, modulePath, version string, pathID int, fields ...internal.FieldSet) (_ *internal.Directory, err error) {
-	return db.getDirectory(ctx, dirPath, modulePath, version)
-}
-
-// getDirectory returns a directory from the database, along with all of the
-// data associated with that directory, including the package, imports, readme,
-// documentation, and licenses.
-func (db *DB) getDirectory(ctx context.Context, path, modulePath, version string) (_ *internal.Directory, err error) {
-	defer derrors.Wrap(&err, "GetDirectory(ctx, %q, %q, %q)", path, modulePath, version)
-	dmeta, err := db.GetDirectoryMeta(ctx, path, modulePath, version)
+// GetDirectory returns a directory from the database, along with all of the
+// data associated with that directory.
+// TODO(golang/go#39629): remove pID.
+func (db *DB) GetDirectory(ctx context.Context, fullPath, modulePath, version string, pID int, field internal.FieldSet) (_ *internal.Directory, err error) {
+	defer derrors.Wrap(&err, "GetDirectory(ctx, %q, %q, %q)", fullPath, modulePath, version)
+	pathID, isRedistributable, err := db.getPathIDAndIsRedistributable(ctx, fullPath, modulePath, version)
 	if err != nil {
 		return nil, err
 	}
-	dir := &internal.Directory{DirectoryMeta: *dmeta}
 
-	if dir.IsRedistributable || db.bypassLicenseCheck {
+	dir := &internal.Directory{
+		DirectoryMeta: internal.DirectoryMeta{
+			Path:              fullPath,
+			PathID:            pathID,
+			IsRedistributable: isRedistributable,
+			ModuleInfo: internal.ModuleInfo{
+				ModulePath: modulePath,
+				Version:    version,
+			},
+		},
+	}
+	if field&internal.WithReadme != 0 {
 		readme, err := db.getReadme(ctx, dir.ModulePath, dir.Version)
 		if err != nil && !errors.Is(err, derrors.NotFound) {
 			return nil, err
 		}
 		dir.Readme = readme
 	}
-	if dir.Name != "" {
-		pkg := &internal.Package{
-			Path: dir.Path,
-			Name: dir.Name,
+
+	if field&internal.WithDocumentation != 0 {
+		doc, err := db.getDocumentation(ctx, dir.PathID)
+		if err != nil && !errors.Is(err, derrors.NotFound) {
+			return nil, err
 		}
-		if dir.IsRedistributable || db.bypassLicenseCheck {
-			doc, err := db.getDocumentation(ctx, dir.PathID)
-			if err != nil && !(db.bypassLicenseCheck && err == sql.ErrNoRows) {
-				return nil, err
+		if doc != nil {
+			dir.Package = &internal.Package{
+				Path:          dir.Path,
+				Documentation: doc,
 			}
-			pkg.Documentation = doc
+		}
+	}
+	if field == internal.AllFields {
+		dmeta, err := db.GetDirectoryMeta(ctx, fullPath, modulePath, version)
+		if err != nil {
+			return nil, err
+		}
+		dir.DirectoryMeta = *dmeta
+		if dir.Name != "" {
+			if dir.Package == nil {
+				dir.Package = &internal.Package{Path: dir.Path}
+			}
+			dir.Package.Name = dmeta.Name
 		}
 		imports, err := db.getImports(ctx, dir.PathID)
 		if err != nil {
 			return nil, err
 		}
-		pkg.Imports = imports
-		dir.Package = pkg
+		if len(imports) > 0 {
+			dir.Package.Imports = imports
+		}
+	}
+	if !db.bypassLicenseCheck {
+		dir.RemoveNonRedistributableData()
 	}
 	return dir, nil
 }
 
+func (db *DB) getPathIDAndIsRedistributable(ctx context.Context, fullPath, modulePath, version string) (_ int, _ bool, err error) {
+	defer derrors.Wrap(&err, "getPathID(ctx, %q, %q, %q)", fullPath, modulePath, version)
+	var (
+		pathID            int
+		isRedistributable bool
+	)
+	query := `
+		SELECT p.id, p.redistributable
+		FROM paths p
+		INNER JOIN modules m ON (p.module_id = m.id)
+		WHERE
+		    p.path = $1
+		    AND m.module_path = $2
+		    AND m.version = $3;`
+	err = db.db.QueryRow(ctx, query, fullPath, modulePath, version).Scan(&pathID, &isRedistributable)
+	switch err {
+	case sql.ErrNoRows:
+		return 0, false, derrors.NotFound
+	case nil:
+		return pathID, isRedistributable, nil
+	default:
+		return 0, false, err
+	}
+}
+
 // GetDirectoryMeta information about a directory from the database.
 func (db *DB) GetDirectoryMeta(ctx context.Context, path, modulePath, version string) (_ *internal.DirectoryMeta, err error) {
 	defer derrors.Wrap(&err, "GetDirectoryMeta(ctx, %q, %q, %q)", path, modulePath, version)

diff --git a/internal/postgres/directory_test.go b/internal/postgres/directory_test.go
@@ -487,34 +487,6 @@ func TestGetDirectory(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	newVdir := func(path, modulePath, version string, readme *internal.Readme, pkg *internal.Package) *internal.Directory {
-		return &internal.Directory{
-			DirectoryMeta: internal.DirectoryMeta{
-				ModuleInfo:        *sample.ModuleInfo(modulePath, version),
-				Path:              path,
-				V1Path:            path,
-				IsRedistributable: true,
-				Licenses:          sample.LicenseMetadata,
-			},
-			Readme:  readme,
-			Package: pkg,
-		}
-	}
-
-	newPackage := func(name, path string) *internal.Package {
-		return &internal.Package{
-			Name: name,
-			Path: path,
-			Documentation: &internal.Documentation{
-				Synopsis: sample.Synopsis,
-				HTML:     sample.DocumentationHTML,
-				GOOS:     sample.GOOS,
-				GOARCH:   sample.GOARCH,
-			},
-			Imports: sample.Imports,
-		}
-	}
-
 	for _, tc := range []struct {
 		name, dirPath, modulePath, version string
 		want                               *internal.Directory
@@ -598,7 +570,7 @@ func TestGetDirectory(t *testing.T) {
 		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
-			got, err := testDB.getDirectory(ctx, tc.dirPath, tc.modulePath, tc.version)
+			got, err := testDB.GetDirectory(ctx, tc.dirPath, tc.modulePath, tc.version, 0, internal.AllFields)
 			if tc.wantNotFoundErr {
 				if !errors.Is(err, derrors.NotFound) {
 					t.Fatalf("want %v; got = \n%+v, %v", derrors.NotFound, got, err)
@@ -630,6 +602,103 @@ func TestGetDirectory(t *testing.T) {
 	}
 }
 
+func TestGetDirectoryFieldSet(t *testing.T) {
+	ctx, cancel := context.WithTimeout(context.Background(), testTimeout)
+	defer cancel()
+
+	defer ResetTestDB(testDB, t)
+
+	// Add a module that has READMEs in a directory and a package.
+	m := sample.Module("a.com/m", "v1.2.3", "dir/p")
+	dir := findDirectory(m, "a.com/m/dir/p")
+	if err := testDB.InsertModule(ctx, m); err != nil {
+		t.Fatal(err)
+	}
+
+	cleanFields := func(dir *internal.Directory) {
+		// Remove fields based on the FieldSet specified.
+		dir.DirectoryMeta = internal.DirectoryMeta{
+			Path:              dir.Path,
+			IsRedistributable: true,
+			ModuleInfo: internal.ModuleInfo{
+				ModulePath: dir.ModulePath,
+				Version:    dir.Version,
+			},
+		}
+		if dir.Package != nil {
+			dir.Package.Imports = nil
+			dir.Package.Name = ""
+		}
+	}
+
+	for _, test := range []struct {
+		name  string
+		field internal.FieldSet
+		want  *internal.Directory
+	}{
+		{
+			name:  "WithDocumentation",
+			field: internal.WithDocumentation,
+			want: newVdir("a.com/m/dir/p", "a.com/m", "v1.2.3",
+				nil, newPackage("p", "a.com/m/dir/p")),
+		},
+		{
+			name:  "WithReadme",
+			field: internal.WithReadme,
+			want: newVdir("a.com/m/dir/p", "a.com/m", "v1.2.3",
+				&internal.Readme{
+					Filepath: "README.md",
+					Contents: "readme",
+				}, nil),
+		},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			got, err := testDB.GetDirectory(ctx, dir.Path, dir.ModulePath, dir.Version, 0, test.field)
+			if err != nil {
+				t.Fatal(err)
+			}
+			opts := []cmp.Option{
+				cmp.AllowUnexported(source.Info{}, safehtml.HTML{}),
+				// The packages table only includes partial license information; it omits the Coverage field.
+				cmpopts.IgnoreFields(licenses.Metadata{}, "Coverage"),
+				cmpopts.IgnoreFields(internal.DirectoryMeta{}, "PathID"),
+			}
+			cleanFields(test.want)
+			if diff := cmp.Diff(test.want, got, opts...); diff != "" {
+				t.Errorf("mismatch (-want, +got):\n%s", diff)
+			}
+		})
+	}
+}
+
+func newVdir(path, modulePath, version string, readme *internal.Readme, pkg *internal.Package) *internal.Directory {
+	return &internal.Directory{
+		DirectoryMeta: internal.DirectoryMeta{
+			ModuleInfo:        *sample.ModuleInfo(modulePath, version),
+			Path:              path,
+			V1Path:            path,
+			IsRedistributable: true,
+			Licenses:          sample.LicenseMetadata,
+		},
+		Readme:  readme,
+		Package: pkg,
+	}
+}
+
+func newPackage(name, path string) *internal.Package {
+	return &internal.Package{
+		Name: name,
+		Path: path,
+		Documentation: &internal.Documentation{
+			Synopsis: sample.Synopsis,
+			HTML:     sample.DocumentationHTML,
+			GOOS:     sample.GOOS,
+			GOARCH:   sample.GOARCH,
+		},
+		Imports: sample.Imports,
+	}
+}
+
 func TestGetDirectoryBypass(t *testing.T) {
 	ctx, cancel := context.WithTimeout(context.Background(), testTimeout)
 	defer cancel()
@@ -648,7 +717,7 @@ func TestGetDirectoryBypass(t *testing.T) {
 		{testDB, true},
 		{bypassDB, false},
 	} {
-		d, err := test.db.getDirectory(ctx, m.ModulePath, m.ModulePath, m.Version)
+		d, err := test.db.GetDirectory(ctx, m.ModulePath, m.ModulePath, m.Version, 0, internal.AllFields)
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -677,6 +746,7 @@ func TestGetDirectoryBypass(t *testing.T) {
 
 func findDirectory(m *internal.Module, path string) *internal.Directory {
 	for _, d := range m.Directories {
+		d.ModuleInfo = m.ModuleInfo
 		if d.Path == path {
 			return d
 		}

diff --git a/internal/postgres/insert_module_test.go b/internal/postgres/insert_module_test.go
@@ -111,7 +111,7 @@ func checkModule(ctx context.Context, t *testing.T, want *internal.Module) {
 	}
 
 	for _, dir := range want.Directories {
-		got, err := testDB.getDirectory(ctx, dir.Path, want.ModulePath, want.Version)
+		got, err := testDB.GetDirectory(ctx, dir.Path, want.ModulePath, want.Version, 0, internal.AllFields)
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -185,7 +185,7 @@ func TestInsertModuleLicenseCheck(t *testing.T) {
 			checkHasRedistData(mi.LegacyReadmeContents, pkg.DocumentationHTML, bypass)
 
 			// New model
-			dir, err := db.getDirectory(ctx, mod.ModulePath, mod.ModulePath, mod.Version)
+			dir, err := db.GetDirectory(ctx, mod.ModulePath, mod.ModulePath, mod.Version, 0, internal.AllFields)
 			if err != nil {
 				t.Fatal(err)
 			}

diff --git a/internal/proxydatasource/details.go b/internal/proxydatasource/details.go
@@ -19,7 +19,7 @@ import (
 )
 
 // GetDirectory returns information about a directory at a path.
-func (ds *DataSource) GetDirectory(ctx context.Context, fullPath, modulePath, version string, pathID int, fields ...internal.FieldSet) (_ *internal.Directory, err error) {
+func (ds *DataSource) GetDirectory(ctx context.Context, fullPath, modulePath, version string, pathID int, field internal.FieldSet) (_ *internal.Directory, err error) {
 	defer derrors.Wrap(&err, "GetDirectory(%q, %q, %q)", fullPath, modulePath, version)
 	return ds.directoryFromVersion(ctx, fullPath, modulePath, version)
 }