internal/postgres: apply bypassLicenseCheck to IsRedistributable column

This enables symbol search for non-LICENSE'd repos when -bypass_license_check is supplied.

I think the way this works is that when these get insert into units, we don't consider bypassLicenseCheck, and the IsRedistributable column gets set to false. That all seems reasonable by itself.

But later, in upsertSymbolSearchDocuments, we only upsert symbol docs for units that are redistributable. That check happens in SQL, so it's hard to factor in the bypassLicenseCheck flag.

It's not really clear to me whether we should "pollute" the database with this column set to true under the flag. If you run the program again with the flag turned off, your database keeps the values set under the old run. Anyways, I think this CL is probably the pragmatic take: it's a small code change that achieves the desired result without added complexity, and it's unlikely that anyone will run into the undesired outcome I describe above (who is running this program sometimes with the flag on, sometimes with it off?).

Change-Id: Ie15bcf0605b945e4af6574abf689827ea946ddae
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/685457
Reviewed-by: Jonathan Amsterdam <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: David Chase <[email protected]>
kokoro-CI: kokoro <[email protected]>
Commit-Queue: Jonathan Amsterdam <[email protected]>

Author: jeanbza

internal/postgres/insert_module.go

@@ -393,7 +393,7 @@ func (pdb *DB) insertUnits(ctx context.Context, tx *database.DB,
 			u.Name,
 			pq.Array(licenseTypes),
 			pq.Array(licensePaths),
-			u.IsRedistributable,
+			pdb.bypassLicenseCheck || u.IsRedistributable,
 		)
 		if u.Readme != nil {
 			pathToReadme[u.Path] = u.Readme

internal/postgres/licenses_test.go

@@ -196,7 +196,9 @@ func TestGetLicensesBypass(t *testing.T) {
 	}
 
 	// Read with license bypass includes non-redistributable license contents.
-	check(true, sample.NonRedistributableLicense)
+	redist := *sample.NonRedistributableLicense
+	redist.Contents = []uint8{}
+	check(true, &redist)
 
 	// Read without license bypass does not include non-redistributable license contents.
 	nonRedist := *sample.NonRedistributableLicense

internal/postgres/search.go

@@ -731,7 +731,7 @@ func (db *DB) GetPackagesForSearchDocumentUpsert(ctx context.Context, before tim
 			a      UpsertSearchDocumentArgs
 			redist bool
 		)
-		if err := rows.Scan(&a.PackagePath, &a.ModulePath, &a.Version, &a.Synopsis, &redist,
+		if err := rows.Scan(&a.PackagePath, &a.ModulePath, &a.Version, database.NullIsEmpty(&a.Synopsis), &redist,
 			database.NullIsEmpty(&a.ReadmeFilePath), database.NullIsEmpty(&a.ReadmeContents)); err != nil {
 			return err
 		}

internal/postgres/search_test.go

@@ -823,29 +823,34 @@ func TestExcludedFromSearch(t *testing.T) {
 }
 
 func TestSearchBypass(t *testing.T) {
-	t.Parallel()
-	testDB, release := acquire(t)
-	defer release()
-	ctx := context.Background()
-	bypassDB := NewBypassingLicenseCheck(testDB.db)
-
-	m := nonRedistributableModule()
-	MustInsertModule(ctx, t, bypassDB, m)
-
 	for _, test := range []struct {
-		db        *DB
+		bypass    bool
 		wantEmpty bool
 	}{
-		{testDB, true},
-		{bypassDB, false},
+		{false, true},
+		{true, false},
 	} {
-		rs, err := test.db.Search(ctx, m.ModulePath, SearchOptions{MaxResults: 10})
-		if err != nil {
-			t.Fatal(err)
-		}
-		if got := (rs[0].Synopsis == ""); got != test.wantEmpty {
-			t.Errorf("bypass %t: got empty %t, want %t", test.db == bypassDB, got, test.wantEmpty)
-		}
+		t.Run(fmt.Sprintf("bypass %t", test.bypass), func(t *testing.T) {
+			t.Parallel()
+			testDB, release := acquire(t)
+			defer release()
+			ctx := context.Background()
+
+			if test.bypass {
+				testDB = NewBypassingLicenseCheck(testDB.db)
+			}
+
+			m := nonRedistributableModule()
+			MustInsertModule(ctx, t, testDB, m)
+
+			rs, err := testDB.Search(ctx, m.ModulePath, SearchOptions{MaxResults: 10})
+			if err != nil {
+				t.Fatal(err)
+			}
+			if got := (rs[0].Synopsis == ""); got != test.wantEmpty {
+				t.Errorf("got empty %t, want %t", got, test.wantEmpty)
+			}
+		})
 	}
 }
 
@@ -1219,90 +1224,99 @@ func TestUpdateSearchDocumentsImportedByCount(t *testing.T) {
 }
 
 func TestGetPackagesForSearchDocumentUpsert(t *testing.T) {
-	t.Parallel()
-	testDB, release := acquire(t)
-	defer release()
-	ctx := context.Background()
+	for _, test := range []struct{ bypass bool }{{false}, {true}} {
+		t.Run(fmt.Sprintf("bypass %t", test.bypass), func(t *testing.T) {
+			ctx := context.Background()
 
-	moduleA := sample.Module("mod.com", "v1.2.3",
-		"A", "A/notinternal", "A/internal", "A/internal/B")
+			moduleA := sample.Module("mod.com", "v1.2.3",
+				"A", "A/notinternal", "A/internal", "A/internal/B")
 
-	// moduleA.Units[1] is mod.com/A.
-	moduleA.Units[1].Readme = &internal.Readme{
-		Filepath: sample.ReadmeFilePath,
-		Contents: sample.ReadmeContents,
-	}
-	// moduleA.Units[2] is mod.com/A/notinternal.
-	moduleA.Units[2].Readme = &internal.Readme{
-		Filepath: sample.ReadmeFilePath,
-		Contents: sample.ReadmeContents,
-	}
-	moduleN := nonRedistributableModule()
-	bypassDB := NewBypassingLicenseCheck(testDB.db)
-	for _, m := range []*internal.Module{moduleA, moduleN} {
-		MustInsertModule(ctx, t, bypassDB, m)
-	}
+			// moduleA.Units[1] is mod.com/A.
+			moduleA.Units[1].Readme = &internal.Readme{
+				Filepath: sample.ReadmeFilePath,
+				Contents: sample.ReadmeContents,
+			}
+			// moduleA.Units[2] is mod.com/A/notinternal.
+			moduleA.Units[2].Readme = &internal.Readme{
+				Filepath: sample.ReadmeFilePath,
+				Contents: sample.ReadmeContents,
+			}
+			moduleN := nonRedistributableModule()
 
-	// We are asking for all packages in search_documents updated before now, which is
-	// all the non-internal packages.
-	got, err := testDB.GetPackagesForSearchDocumentUpsert(ctx, time.Now(), 10)
-	if err != nil {
-		t.Fatal(err)
-	}
-	sort.Slice(got, func(i, j int) bool { return got[i].PackagePath < got[j].PackagePath })
-	want := []UpsertSearchDocumentArgs{
-		{
-			PackagePath:    moduleN.ModulePath,
-			ModulePath:     moduleN.ModulePath,
-			Version:        "v1.2.3",
-			ReadmeFilePath: "",
-			ReadmeContents: "",
-			Synopsis:       "",
-		},
-		{
-			PackagePath:    "mod.com/A",
-			ModulePath:     "mod.com",
-			Version:        "v1.2.3",
-			ReadmeFilePath: "README.md",
-			ReadmeContents: "readme",
-			Synopsis:       sample.Doc.Synopsis,
-		},
-		{
-			PackagePath:    "mod.com/A/notinternal",
-			ModulePath:     "mod.com",
-			Version:        "v1.2.3",
-			ReadmeFilePath: "README.md",
-			ReadmeContents: "readme",
-			Synopsis:       sample.Doc.Synopsis,
-		},
-	}
-	if diff := cmp.Diff(want, got); diff != "" {
-		t.Fatalf("testDB.GetPackagesForSearchDocumentUpsert mismatch(-want +got):\n%s", diff)
-	}
+			testDB, release := acquire(t)
+			defer release()
 
-	// Reading with license bypass should return the non-redistributable fields.
-	got, err = bypassDB.GetPackagesForSearchDocumentUpsert(ctx, time.Now(), 10)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if len(got) == 0 {
-		t.Fatal("len(got)==0")
-	}
-	sort.Slice(got, func(i, j int) bool { return got[i].PackagePath < got[j].PackagePath })
-	gm := got[0]
-	for _, got := range []string{gm.ReadmeFilePath, gm.ReadmeContents, gm.Synopsis} {
-		if got == "" {
-			t.Errorf("got empty field, want non-empty")
-		}
-	}
+			if test.bypass {
+				testDB = NewBypassingLicenseCheck(testDB.db)
+			}
+			MustInsertModule(ctx, t, testDB, moduleA)
+			MustInsertModule(ctx, t, testDB, moduleN)
 
-	// pkgPaths should be an empty slice, all packages were inserted more recently than yesterday.
-	got, err = testDB.GetPackagesForSearchDocumentUpsert(ctx, time.Now().Add(-24*time.Hour), 10)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if len(got) != 0 {
-		t.Fatalf("expected testDB.GetPackagesForSearchDocumentUpsert to return an empty slice; got %v", got)
+			// We are asking for all packages in search_documents updated before now, which is
+			// all the non-internal packages.
+			got, err := testDB.GetPackagesForSearchDocumentUpsert(ctx, time.Now(), 10)
+			if err != nil {
+				t.Fatal(err)
+			}
+			sort.Slice(got, func(i, j int) bool { return got[i].PackagePath < got[j].PackagePath })
+			wantNonRedistributable := UpsertSearchDocumentArgs{
+				PackagePath:    moduleN.ModulePath,
+				ModulePath:     moduleN.ModulePath,
+				Version:        "v1.2.3",
+				ReadmeFilePath: "",
+				ReadmeContents: "",
+				Synopsis:       "",
+			}
+			if test.bypass {
+				// If we inserted with bypass mode, these exist in the database
+				// and will be returned by search.
+				wantNonRedistributable.ReadmeFilePath = "README.md"
+				wantNonRedistributable.ReadmeContents = "readme"
+				wantNonRedistributable.Synopsis = sample.Doc.Synopsis
+			}
+			want := []UpsertSearchDocumentArgs{
+				wantNonRedistributable,
+				{
+					PackagePath:    "mod.com/A",
+					ModulePath:     "mod.com",
+					Version:        "v1.2.3",
+					ReadmeFilePath: "README.md",
+					ReadmeContents: "readme",
+					Synopsis:       sample.Doc.Synopsis,
+				},
+				{
+					PackagePath:    "mod.com/A/notinternal",
+					ModulePath:     "mod.com",
+					Version:        "v1.2.3",
+					ReadmeFilePath: "README.md",
+					ReadmeContents: "readme",
+					Synopsis:       sample.Doc.Synopsis,
+				},
+			}
+			if diff := cmp.Diff(want, got); diff != "" {
+				t.Fatalf("testDB.GetPackagesForSearchDocumentUpsert mismatch(-want +got):\n%s", diff)
+			}
+
+			// Reading with license bypass should return the non-redistributable fields.
+			if test.bypass {
+				sort.Slice(got, func(i, j int) bool { return got[i].PackagePath < got[j].PackagePath })
+				gm := got[0]
+				for _, got := range []string{gm.ReadmeFilePath, gm.ReadmeContents, gm.Synopsis} {
+					if got == "" {
+						t.Errorf("got empty field, want non-empty")
+					}
+				}
+
+				// pkgPaths should be an empty slice, all packages were inserted more recently than yesterday.
+				got, err = testDB.GetPackagesForSearchDocumentUpsert(ctx, time.Now().Add(-24*time.Hour), 10)
+				if err != nil {
+					t.Fatal(err)
+				}
+				if len(got) != 0 {
+					t.Fatalf("expected testDB.GetPackagesForSearchDocumentUpsert to return an empty slice; got %v", got)
+				}
+			}
+		})
 	}
 }
 

internal/postgres/unit_test.go

@@ -804,46 +804,50 @@ func subdirectories(modulePath string, suffixes []string) []*internal.PackageMet
 }
 
 func TestGetUnitBypass(t *testing.T) {
-	t.Parallel()
-	testDB, release := acquire(t)
-	defer release()
-	ctx := context.Background()
-	bypassDB := NewBypassingLicenseCheck(testDB.db)
-
-	m := nonRedistributableModule()
-	MustInsertModule(ctx, t, bypassDB, m)
-
 	for _, test := range []struct {
-		db        *DB
+		bypass    bool
 		wantEmpty bool
 	}{
-		{testDB, true},
-		{bypassDB, false},
+		{false, true},
+		{true, false},
 	} {
-		pathInfo := newUnitMeta(m.ModulePath, m.ModulePath, m.Version)
-		d, err := test.db.GetUnit(ctx, pathInfo, internal.AllFields, internal.BuildContext{})
-		if err != nil {
-			t.Fatal(err)
-		}
-		if got := (d.Readme == nil); got != test.wantEmpty {
-			t.Errorf("readme empty: got %t, want %t", got, test.wantEmpty)
-		}
-		if got := (d.Documentation == nil); got != test.wantEmpty {
-			t.Errorf("synopsis empty: got %t, want %t", got, test.wantEmpty)
-		}
-		if got := (d.Documentation == nil); got != test.wantEmpty {
-			t.Errorf("doc empty: got %t, want %t", got, test.wantEmpty)
-		}
-		if got := d.IsRedistributable; got != !test.wantEmpty { // wantEmpty iff !IsRedistributable
-			t.Errorf("IsRedistributable is %v: want %v", got, !test.wantEmpty)
-		}
-		pkgs := d.Subdirectories
-		if len(pkgs) != 1 {
-			t.Fatal("len(pkgs) != 1")
-		}
-		if got := (pkgs[0].Synopsis == ""); got != test.wantEmpty {
-			t.Errorf("synopsis empty: got %t, want %t", got, test.wantEmpty)
-		}
+		t.Run(fmt.Sprintf("bypass %t", test.bypass), func(t *testing.T) {
+			t.Parallel()
+			testDB, release := acquire(t)
+			defer release()
+			ctx := context.Background()
+
+			if test.bypass {
+				testDB = NewBypassingLicenseCheck(testDB.db)
+			}
+
+			m := nonRedistributableModule()
+			MustInsertModule(ctx, t, testDB, m)
+			pathInfo := newUnitMeta(m.ModulePath, m.ModulePath, m.Version)
+			d, err := testDB.GetUnit(ctx, pathInfo, internal.AllFields, internal.BuildContext{})
+			if err != nil {
+				t.Fatal(err)
+			}
+			if got := (d.Readme == nil); got != test.wantEmpty {
+				t.Errorf("readme empty: got %t, want %t", got, test.wantEmpty)
+			}
+			if got := (d.Documentation == nil); got != test.wantEmpty {
+				t.Errorf("synopsis empty: got %t, want %t", got, test.wantEmpty)
+			}
+			if got := (d.Documentation == nil); got != test.wantEmpty {
+				t.Errorf("doc empty: got %t, want %t", got, test.wantEmpty)
+			}
+			if got := d.IsRedistributable; got != !test.wantEmpty { // wantEmpty iff !IsRedistributable
+				t.Errorf("IsRedistributable is %v: want %v", got, !test.wantEmpty)
+			}
+			pkgs := d.Subdirectories
+			if len(pkgs) != 1 {
+				t.Fatal("len(pkgs) != 1")
+			}
+			if got := (pkgs[0].Synopsis == ""); got != test.wantEmpty {
+				t.Errorf("synopsis empty: got %t, want %t", got, test.wantEmpty)
+			}
+		})
 	}
 }