Skip to content

Commit ed9d06a

Browse files
jeanbzajba
jeanbza
authored and
jba
committed
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]>
1 parent 259676f commit ed9d06a

File tree

5 files changed

+157
-137
lines changed

5 files changed

+157
-137
lines changed

‎internal/postgres/insert_module.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -393,7 +393,7 @@ func (pdb *DB) insertUnits(ctx context.Context, tx *database.DB,
393393
u.Name,
394394
pq.Array(licenseTypes),
395395
pq.Array(licensePaths),
396-
u.IsRedistributable,
396+
pdb.bypassLicenseCheck || u.IsRedistributable,
397397
)
398398
if u.Readme != nil {
399399
pathToReadme[u.Path] = u.Readme

‎internal/postgres/licenses_test.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -196,7 +196,9 @@ func TestGetLicensesBypass(t *testing.T) {
196196
}
197197

198198
// Read with license bypass includes non-redistributable license contents.
199-
check(true, sample.NonRedistributableLicense)
199+
redist := *sample.NonRedistributableLicense
200+
redist.Contents = []uint8{}
201+
check(true, &redist)
200202

201203
// Read without license bypass does not include non-redistributable license contents.
202204
nonRedist := *sample.NonRedistributableLicense

‎internal/postgres/search.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -731,7 +731,7 @@ func (db *DB) GetPackagesForSearchDocumentUpsert(ctx context.Context, before tim
731731
a UpsertSearchDocumentArgs
732732
redist bool
733733
)
734-
if err := rows.Scan(&a.PackagePath, &a.ModulePath, &a.Version, &a.Synopsis, &redist,
734+
if err := rows.Scan(&a.PackagePath, &a.ModulePath, &a.Version, database.NullIsEmpty(&a.Synopsis), &redist,
735735
database.NullIsEmpty(&a.ReadmeFilePath), database.NullIsEmpty(&a.ReadmeContents)); err != nil {
736736
return err
737737
}

‎internal/postgres/search_test.go‎

Lines changed: 112 additions & 98 deletions
Original file line numberDiff line numberDiff line change
@@ -823,29 +823,34 @@ func TestExcludedFromSearch(t *testing.T) {
823823
}
824824

825825
func TestSearchBypass(t *testing.T) {
826-
t.Parallel()
827-
testDB, release := acquire(t)
828-
defer release()
829-
ctx := context.Background()
830-
bypassDB := NewBypassingLicenseCheck(testDB.db)
831-
832-
m := nonRedistributableModule()
833-
MustInsertModule(ctx, t, bypassDB, m)
834-
835826
for _, test := range []struct {
836-
db *DB
827+
bypass bool
837828
wantEmpty bool
838829
}{
839-
{testDB, true},
840-
{bypassDB, false},
830+
{false, true},
831+
{true, false},
841832
} {
842-
rs, err := test.db.Search(ctx, m.ModulePath, SearchOptions{MaxResults: 10})
843-
if err != nil {
844-
t.Fatal(err)
845-
}
846-
if got := (rs[0].Synopsis == ""); got != test.wantEmpty {
847-
t.Errorf("bypass %t: got empty %t, want %t", test.db == bypassDB, got, test.wantEmpty)
848-
}
833+
t.Run(fmt.Sprintf("bypass %t", test.bypass), func(t *testing.T) {
834+
t.Parallel()
835+
testDB, release := acquire(t)
836+
defer release()
837+
ctx := context.Background()
838+
839+
if test.bypass {
840+
testDB = NewBypassingLicenseCheck(testDB.db)
841+
}
842+
843+
m := nonRedistributableModule()
844+
MustInsertModule(ctx, t, testDB, m)
845+
846+
rs, err := testDB.Search(ctx, m.ModulePath, SearchOptions{MaxResults: 10})
847+
if err != nil {
848+
t.Fatal(err)
849+
}
850+
if got := (rs[0].Synopsis == ""); got != test.wantEmpty {
851+
t.Errorf("got empty %t, want %t", got, test.wantEmpty)
852+
}
853+
})
849854
}
850855
}
851856

@@ -1219,90 +1224,99 @@ func TestUpdateSearchDocumentsImportedByCount(t *testing.T) {
12191224
}
12201225

12211226
func TestGetPackagesForSearchDocumentUpsert(t *testing.T) {
1222-
t.Parallel()
1223-
testDB, release := acquire(t)
1224-
defer release()
1225-
ctx := context.Background()
1227+
for _, test := range []struct{ bypass bool }{{false}, {true}} {
1228+
t.Run(fmt.Sprintf("bypass %t", test.bypass), func(t *testing.T) {
1229+
ctx := context.Background()
12261230

1227-
moduleA := sample.Module("mod.com", "v1.2.3",
1228-
"A", "A/notinternal", "A/internal", "A/internal/B")
1231+
moduleA := sample.Module("mod.com", "v1.2.3",
1232+
"A", "A/notinternal", "A/internal", "A/internal/B")
12291233

1230-
// moduleA.Units[1] is mod.com/A.
1231-
moduleA.Units[1].Readme = &internal.Readme{
1232-
Filepath: sample.ReadmeFilePath,
1233-
Contents: sample.ReadmeContents,
1234-
}
1235-
// moduleA.Units[2] is mod.com/A/notinternal.
1236-
moduleA.Units[2].Readme = &internal.Readme{
1237-
Filepath: sample.ReadmeFilePath,
1238-
Contents: sample.ReadmeContents,
1239-
}
1240-
moduleN := nonRedistributableModule()
1241-
bypassDB := NewBypassingLicenseCheck(testDB.db)
1242-
for _, m := range []*internal.Module{moduleA, moduleN} {
1243-
MustInsertModule(ctx, t, bypassDB, m)
1244-
}
1234+
// moduleA.Units[1] is mod.com/A.
1235+
moduleA.Units[1].Readme = &internal.Readme{
1236+
Filepath: sample.ReadmeFilePath,
1237+
Contents: sample.ReadmeContents,
1238+
}
1239+
// moduleA.Units[2] is mod.com/A/notinternal.
1240+
moduleA.Units[2].Readme = &internal.Readme{
1241+
Filepath: sample.ReadmeFilePath,
1242+
Contents: sample.ReadmeContents,
1243+
}
1244+
moduleN := nonRedistributableModule()
12451245

1246-
// We are asking for all packages in search_documents updated before now, which is
1247-
// all the non-internal packages.
1248-
got, err := testDB.GetPackagesForSearchDocumentUpsert(ctx, time.Now(), 10)
1249-
if err != nil {
1250-
t.Fatal(err)
1251-
}
1252-
sort.Slice(got, func(i, j int) bool { return got[i].PackagePath < got[j].PackagePath })
1253-
want := []UpsertSearchDocumentArgs{
1254-
{
1255-
PackagePath: moduleN.ModulePath,
1256-
ModulePath: moduleN.ModulePath,
1257-
Version: "v1.2.3",
1258-
ReadmeFilePath: "",
1259-
ReadmeContents: "",
1260-
Synopsis: "",
1261-
},
1262-
{
1263-
PackagePath: "mod.com/A",
1264-
ModulePath: "mod.com",
1265-
Version: "v1.2.3",
1266-
ReadmeFilePath: "README.md",
1267-
ReadmeContents: "readme",
1268-
Synopsis: sample.Doc.Synopsis,
1269-
},
1270-
{
1271-
PackagePath: "mod.com/A/notinternal",
1272-
ModulePath: "mod.com",
1273-
Version: "v1.2.3",
1274-
ReadmeFilePath: "README.md",
1275-
ReadmeContents: "readme",
1276-
Synopsis: sample.Doc.Synopsis,
1277-
},
1278-
}
1279-
if diff := cmp.Diff(want, got); diff != "" {
1280-
t.Fatalf("testDB.GetPackagesForSearchDocumentUpsert mismatch(-want +got):\n%s", diff)
1281-
}
1246+
testDB, release := acquire(t)
1247+
defer release()
12821248

1283-
// Reading with license bypass should return the non-redistributable fields.
1284-
got, err = bypassDB.GetPackagesForSearchDocumentUpsert(ctx, time.Now(), 10)
1285-
if err != nil {
1286-
t.Fatal(err)
1287-
}
1288-
if len(got) == 0 {
1289-
t.Fatal("len(got)==0")
1290-
}
1291-
sort.Slice(got, func(i, j int) bool { return got[i].PackagePath < got[j].PackagePath })
1292-
gm := got[0]
1293-
for _, got := range []string{gm.ReadmeFilePath, gm.ReadmeContents, gm.Synopsis} {
1294-
if got == "" {
1295-
t.Errorf("got empty field, want non-empty")
1296-
}
1297-
}
1249+
if test.bypass {
1250+
testDB = NewBypassingLicenseCheck(testDB.db)
1251+
}
1252+
MustInsertModule(ctx, t, testDB, moduleA)
1253+
MustInsertModule(ctx, t, testDB, moduleN)
12981254

1299-
// pkgPaths should be an empty slice, all packages were inserted more recently than yesterday.
1300-
got, err = testDB.GetPackagesForSearchDocumentUpsert(ctx, time.Now().Add(-24*time.Hour), 10)
1301-
if err != nil {
1302-
t.Fatal(err)
1303-
}
1304-
if len(got) != 0 {
1305-
t.Fatalf("expected testDB.GetPackagesForSearchDocumentUpsert to return an empty slice; got %v", got)
1255+
// We are asking for all packages in search_documents updated before now, which is
1256+
// all the non-internal packages.
1257+
got, err := testDB.GetPackagesForSearchDocumentUpsert(ctx, time.Now(), 10)
1258+
if err != nil {
1259+
t.Fatal(err)
1260+
}
1261+
sort.Slice(got, func(i, j int) bool { return got[i].PackagePath < got[j].PackagePath })
1262+
wantNonRedistributable := UpsertSearchDocumentArgs{
1263+
PackagePath: moduleN.ModulePath,
1264+
ModulePath: moduleN.ModulePath,
1265+
Version: "v1.2.3",
1266+
ReadmeFilePath: "",
1267+
ReadmeContents: "",
1268+
Synopsis: "",
1269+
}
1270+
if test.bypass {
1271+
// If we inserted with bypass mode, these exist in the database
1272+
// and will be returned by search.
1273+
wantNonRedistributable.ReadmeFilePath = "README.md"
1274+
wantNonRedistributable.ReadmeContents = "readme"
1275+
wantNonRedistributable.Synopsis = sample.Doc.Synopsis
1276+
}
1277+
want := []UpsertSearchDocumentArgs{
1278+
wantNonRedistributable,
1279+
{
1280+
PackagePath: "mod.com/A",
1281+
ModulePath: "mod.com",
1282+
Version: "v1.2.3",
1283+
ReadmeFilePath: "README.md",
1284+
ReadmeContents: "readme",
1285+
Synopsis: sample.Doc.Synopsis,
1286+
},
1287+
{
1288+
PackagePath: "mod.com/A/notinternal",
1289+
ModulePath: "mod.com",
1290+
Version: "v1.2.3",
1291+
ReadmeFilePath: "README.md",
1292+
ReadmeContents: "readme",
1293+
Synopsis: sample.Doc.Synopsis,
1294+
},
1295+
}
1296+
if diff := cmp.Diff(want, got); diff != "" {
1297+
t.Fatalf("testDB.GetPackagesForSearchDocumentUpsert mismatch(-want +got):\n%s", diff)
1298+
}
1299+
1300+
// Reading with license bypass should return the non-redistributable fields.
1301+
if test.bypass {
1302+
sort.Slice(got, func(i, j int) bool { return got[i].PackagePath < got[j].PackagePath })
1303+
gm := got[0]
1304+
for _, got := range []string{gm.ReadmeFilePath, gm.ReadmeContents, gm.Synopsis} {
1305+
if got == "" {
1306+
t.Errorf("got empty field, want non-empty")
1307+
}
1308+
}
1309+
1310+
// pkgPaths should be an empty slice, all packages were inserted more recently than yesterday.
1311+
got, err = testDB.GetPackagesForSearchDocumentUpsert(ctx, time.Now().Add(-24*time.Hour), 10)
1312+
if err != nil {
1313+
t.Fatal(err)
1314+
}
1315+
if len(got) != 0 {
1316+
t.Fatalf("expected testDB.GetPackagesForSearchDocumentUpsert to return an empty slice; got %v", got)
1317+
}
1318+
}
1319+
})
13061320
}
13071321
}
13081322

‎internal/postgres/unit_test.go‎

Lines changed: 40 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -804,46 +804,50 @@ func subdirectories(modulePath string, suffixes []string) []*internal.PackageMet
804804
}
805805

806806
func TestGetUnitBypass(t *testing.T) {
807-
t.Parallel()
808-
testDB, release := acquire(t)
809-
defer release()
810-
ctx := context.Background()
811-
bypassDB := NewBypassingLicenseCheck(testDB.db)
812-
813-
m := nonRedistributableModule()
814-
MustInsertModule(ctx, t, bypassDB, m)
815-
816807
for _, test := range []struct {
817-
db *DB
808+
bypass bool
818809
wantEmpty bool
819810
}{
820-
{testDB, true},
821-
{bypassDB, false},
811+
{false, true},
812+
{true, false},
822813
} {
823-
pathInfo := newUnitMeta(m.ModulePath, m.ModulePath, m.Version)
824-
d, err := test.db.GetUnit(ctx, pathInfo, internal.AllFields, internal.BuildContext{})
825-
if err != nil {
826-
t.Fatal(err)
827-
}
828-
if got := (d.Readme == nil); got != test.wantEmpty {
829-
t.Errorf("readme empty: got %t, want %t", got, test.wantEmpty)
830-
}
831-
if got := (d.Documentation == nil); got != test.wantEmpty {
832-
t.Errorf("synopsis empty: got %t, want %t", got, test.wantEmpty)
833-
}
834-
if got := (d.Documentation == nil); got != test.wantEmpty {
835-
t.Errorf("doc empty: got %t, want %t", got, test.wantEmpty)
836-
}
837-
if got := d.IsRedistributable; got != !test.wantEmpty { // wantEmpty iff !IsRedistributable
838-
t.Errorf("IsRedistributable is %v: want %v", got, !test.wantEmpty)
839-
}
840-
pkgs := d.Subdirectories
841-
if len(pkgs) != 1 {
842-
t.Fatal("len(pkgs) != 1")
843-
}
844-
if got := (pkgs[0].Synopsis == ""); got != test.wantEmpty {
845-
t.Errorf("synopsis empty: got %t, want %t", got, test.wantEmpty)
846-
}
814+
t.Run(fmt.Sprintf("bypass %t", test.bypass), func(t *testing.T) {
815+
t.Parallel()
816+
testDB, release := acquire(t)
817+
defer release()
818+
ctx := context.Background()
819+
820+
if test.bypass {
821+
testDB = NewBypassingLicenseCheck(testDB.db)
822+
}
823+
824+
m := nonRedistributableModule()
825+
MustInsertModule(ctx, t, testDB, m)
826+
pathInfo := newUnitMeta(m.ModulePath, m.ModulePath, m.Version)
827+
d, err := testDB.GetUnit(ctx, pathInfo, internal.AllFields, internal.BuildContext{})
828+
if err != nil {
829+
t.Fatal(err)
830+
}
831+
if got := (d.Readme == nil); got != test.wantEmpty {
832+
t.Errorf("readme empty: got %t, want %t", got, test.wantEmpty)
833+
}
834+
if got := (d.Documentation == nil); got != test.wantEmpty {
835+
t.Errorf("synopsis empty: got %t, want %t", got, test.wantEmpty)
836+
}
837+
if got := (d.Documentation == nil); got != test.wantEmpty {
838+
t.Errorf("doc empty: got %t, want %t", got, test.wantEmpty)
839+
}
840+
if got := d.IsRedistributable; got != !test.wantEmpty { // wantEmpty iff !IsRedistributable
841+
t.Errorf("IsRedistributable is %v: want %v", got, !test.wantEmpty)
842+
}
843+
pkgs := d.Subdirectories
844+
if len(pkgs) != 1 {
845+
t.Fatal("len(pkgs) != 1")
846+
}
847+
if got := (pkgs[0].Synopsis == ""); got != test.wantEmpty {
848+
t.Errorf("synopsis empty: got %t, want %t", got, test.wantEmpty)
849+
}
850+
})
847851
}
848852
}
849853

0 commit comments

Comments
 (0)