Skip to content

Commit 00b48a4

Browse files
benluddybenluddy
committedAug 11, 2020
Return empty properties and dependencies in ListBundles responses.
Bundles that don't have any properties/dependencies should return nil or [] for those fields, but due to an error in handling NULL column values, a single zero-valued entry was being returned.
·
v1.51.0goreleaser-all-os
1 parent 018a040 commit 00b48a4

File tree

5 files changed

+720
-64
lines changed

5 files changed

+720
-64
lines changed
 

‎go.mod

+1
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ require (
4747
golang.org/x/mod v0.2.0
4848
golang.org/x/net v0.0.0-20200625001655-4c5254603344
4949
golang.org/x/oauth2 v0.0.0-20190604053449-0f29369cfe45
50+
golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e
5051
golang.org/x/sys v0.0.0-20200625212154-ddb9806d33ae // indirect
5152
golang.org/x/text v0.3.3 // indirect
5253
google.golang.org/genproto v0.0.0-20200701001935-0939c5918c31 // indirect

‎pkg/sqlite/query.go

+83-64
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -o sqlitefakes/fake_rowscanner.go . RowScanner
2+
//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -o sqlitefakes/fake_querier.go . Querier
13
package sqlite
24

35
import (
@@ -13,10 +15,28 @@ import (
1315
"github.com/operator-framework/operator-registry/pkg/registry"
1416
)
1517

16-
type SQLQuerier struct {
18+
type RowScanner interface {
19+
Next() bool
20+
Close() error
21+
Scan(dest ...interface{}) error
22+
}
23+
24+
type Querier interface {
25+
QueryContext(ctx context.Context, query string, args ...interface{}) (RowScanner, error)
26+
}
27+
28+
type dbQuerierAdapter struct {
1729
db *sql.DB
1830
}
1931

32+
func (a dbQuerierAdapter) QueryContext(ctx context.Context, query string, args ...interface{}) (RowScanner, error) {
33+
return a.db.QueryContext(ctx, query, args...)
34+
}
35+
36+
type SQLQuerier struct {
37+
db Querier
38+
}
39+
2040
var _ registry.Query = &SQLQuerier{}
2141

2242
func NewSQLLiteQuerier(dbFilename string) (*SQLQuerier, error) {
@@ -25,11 +45,15 @@ func NewSQLLiteQuerier(dbFilename string) (*SQLQuerier, error) {
2545
return nil, err
2646
}
2747

28-
return &SQLQuerier{db}, nil
48+
return &SQLQuerier{dbQuerierAdapter{db}}, nil
2949
}
3050

3151
func NewSQLLiteQuerierFromDb(db *sql.DB) *SQLQuerier {
32-
return &SQLQuerier{db}
52+
return &SQLQuerier{dbQuerierAdapter{db}}
53+
}
54+
55+
func NewSQLLiteQuerierFromDBQuerier(q Querier) *SQLQuerier {
56+
return &SQLQuerier{q}
3357
}
3458

3559
func (s *SQLQuerier) ListTables(ctx context.Context) ([]string, error) {
@@ -900,7 +924,7 @@ func (s *SQLQuerier) GetCurrentCSVNameForChannel(ctx context.Context, pkgName, c
900924
return "", nil
901925
}
902926

903-
func (s *SQLQuerier) ListBundles(ctx context.Context) (bundles []*api.Bundle, err error) {
927+
func (s *SQLQuerier) ListBundles(ctx context.Context) ([]*api.Bundle, error) {
904928
query := `SELECT DISTINCT channel_entry.entry_id, operatorbundle.bundle, operatorbundle.bundlepath,
905929
channel_entry.operatorbundle_name, channel_entry.package_name, channel_entry.channel_name, operatorbundle.replaces, operatorbundle.skips,
906930
operatorbundle.version, operatorbundle.skiprange,
@@ -918,23 +942,25 @@ func (s *SQLQuerier) ListBundles(ctx context.Context) (bundles []*api.Bundle, er
918942
}
919943
defer rows.Close()
920944

921-
bundles = []*api.Bundle{}
945+
var bundles []*api.Bundle
922946
bundlesMap := map[string]*api.Bundle{}
923947
for rows.Next() {
924-
var entryID sql.NullInt64
925-
var bundle sql.NullString
926-
var bundlePath sql.NullString
927-
var bundleName sql.NullString
928-
var pkgName sql.NullString
929-
var channelName sql.NullString
930-
var replaces sql.NullString
931-
var skips sql.NullString
932-
var version sql.NullString
933-
var skipRange sql.NullString
934-
var depType sql.NullString
935-
var depValue sql.NullString
936-
var propType sql.NullString
937-
var propValue sql.NullString
948+
var (
949+
entryID sql.NullInt64
950+
bundle sql.NullString
951+
bundlePath sql.NullString
952+
bundleName sql.NullString
953+
pkgName sql.NullString
954+
channelName sql.NullString
955+
replaces sql.NullString
956+
skips sql.NullString
957+
version sql.NullString
958+
skipRange sql.NullString
959+
depType sql.NullString
960+
depValue sql.NullString
961+
propType sql.NullString
962+
propValue sql.NullString
963+
)
938964
if err := rows.Scan(&entryID, &bundle, &bundlePath, &bundleName, &pkgName, &channelName, &replaces, &skips, &version, &skipRange, &depType, &depValue, &propType, &propValue); err != nil {
939965
return nil, err
940966
}
@@ -946,29 +972,18 @@ func (s *SQLQuerier) ListBundles(ctx context.Context) (bundles []*api.Bundle, er
946972
bundleKey := fmt.Sprintf("%s/%s/%s/%s", bundleName.String, version.String, bundlePath.String, channelName.String)
947973
bundleItem, ok := bundlesMap[bundleKey]
948974
if ok {
949-
// Create new dependency object
950975
if depType.Valid && depValue.Valid {
951-
dep := &api.Dependency{}
952-
dep.Type = depType.String
953-
dep.Value = depValue.String
954-
955-
// Add new dependency to the existing list
956-
existingDeps := bundleItem.Dependencies
957-
existingDeps = append(existingDeps, dep)
958-
bundleItem.Dependencies = existingDeps
976+
bundleItem.Dependencies = append(bundleItem.Dependencies, &api.Dependency{
977+
Type: depType.String,
978+
Value: depValue.String,
979+
})
959980
}
960981

961-
962-
// Create new property object
963982
if propType.Valid && propValue.Valid {
964-
prop := &api.Property{}
965-
prop.Type = propType.String
966-
prop.Value = propValue.String
967-
968-
// Add new property to the existing list
969-
existingProps := bundleItem.Properties
970-
existingProps = append(existingProps, prop)
971-
bundleItem.Properties = existingProps
983+
bundleItem.Properties = append(bundleItem.Properties, &api.Property{
984+
Type: propType.String,
985+
Value: propValue.String,
986+
})
972987
}
973988
} else {
974989
// Create new bundle
@@ -987,30 +1002,34 @@ func (s *SQLQuerier) ListBundles(ctx context.Context) (bundles []*api.Bundle, er
9871002
out.Version = version.String
9881003
out.SkipRange = skipRange.String
9891004
out.Replaces = replaces.String
990-
out.Skips = strings.Split(skips.String, ",")
1005+
if skips.Valid {
1006+
out.Skips = strings.Split(skips.String, ",")
1007+
}
9911008

9921009
provided, required, err := s.GetApisForEntry(ctx, entryID.Int64)
9931010
if err != nil {
9941011
return nil, err
9951012
}
996-
out.ProvidedApis = provided
997-
out.RequiredApis = required
998-
999-
// Create new dependency and dependency list
1000-
dep := &api.Dependency{}
1001-
dependencies := []*api.Dependency{}
1002-
dep.Type = depType.String
1003-
dep.Value = depValue.String
1004-
dependencies = append(dependencies, dep)
1005-
out.Dependencies = dependencies
1006-
1007-
// Create new property and property list
1008-
prop := &api.Property{}
1009-
properties := []*api.Property{}
1010-
prop.Type = propType.String
1011-
prop.Value = propValue.String
1012-
properties = append(properties, prop)
1013-
out.Properties = properties
1013+
if len(provided) > 0 {
1014+
out.ProvidedApis = provided
1015+
}
1016+
if len(required) > 0 {
1017+
out.RequiredApis = required
1018+
}
1019+
1020+
if depType.Valid && depValue.Valid {
1021+
out.Dependencies = []*api.Dependency{{
1022+
Type: depType.String,
1023+
Value: depValue.String,
1024+
}}
1025+
}
1026+
1027+
if propType.Valid && propValue.Valid {
1028+
out.Properties = []*api.Property{{
1029+
Type: propType.String,
1030+
Value: propValue.String,
1031+
}}
1032+
}
10141033

10151034
bundlesMap[bundleKey] = out
10161035
}
@@ -1028,29 +1047,29 @@ func (s *SQLQuerier) ListBundles(ctx context.Context) (bundles []*api.Bundle, er
10281047
bundles = append(bundles, v)
10291048
}
10301049

1031-
return
1050+
return bundles, nil
10321051
}
10331052

10341053
func unique(deps []*api.Dependency) []*api.Dependency {
1035-
keys := make(map[string]bool)
1036-
list := []*api.Dependency{}
1054+
keys := make(map[string]struct{})
1055+
var list []*api.Dependency
10371056
for _, entry := range deps {
10381057
depKey := fmt.Sprintf("%s/%s", entry.Type, entry.Value)
10391058
if _, value := keys[depKey]; !value {
1040-
keys[depKey] = true
1059+
keys[depKey] = struct{}{}
10411060
list = append(list, entry)
10421061
}
10431062
}
10441063
return list
10451064
}
10461065

10471066
func uniqueProps(props []*api.Property) []*api.Property {
1048-
keys := make(map[string]bool)
1049-
list := []*api.Property{}
1067+
keys := make(map[string]struct{})
1068+
var list []*api.Property
10501069
for _, entry := range props {
10511070
propKey := fmt.Sprintf("%s/%s", entry.Type, entry.Value)
10521071
if _, value := keys[propKey]; !value {
1053-
keys[propKey] = true
1072+
keys[propKey] = struct{}{}
10541073
list = append(list, entry)
10551074
}
10561075
}

‎pkg/sqlite/query_test.go

+278
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,278 @@
1+
package sqlite_test
2+
3+
import (
4+
"context"
5+
"database/sql"
6+
"fmt"
7+
"reflect"
8+
"testing"
9+
10+
"github.com/operator-framework/operator-registry/pkg/api"
11+
"github.com/operator-framework/operator-registry/pkg/sqlite"
12+
"github.com/operator-framework/operator-registry/pkg/sqlite/sqlitefakes"
13+
"github.com/stretchr/testify/assert"
14+
)
15+
16+
func TestListBundles(t *testing.T) {
17+
type Columns struct {
18+
EntryID sql.NullInt64
19+
Bundle sql.NullString
20+
BundlePath sql.NullString
21+
BundleName sql.NullString
22+
PackageName sql.NullString
23+
ChannelName sql.NullString
24+
Replaces sql.NullString
25+
Skips sql.NullString
26+
Version sql.NullString
27+
SkipRange sql.NullString
28+
DependencyType sql.NullString
29+
DependencyValue sql.NullString
30+
PropertyType sql.NullString
31+
PropertyValue sql.NullString
32+
}
33+
34+
var NoRows sqlitefakes.FakeRowScanner
35+
NoRows.NextReturns(false)
36+
37+
ScanFromColumns := func(t *testing.T, dsts []interface{}, cols Columns) {
38+
ct := reflect.TypeOf(cols)
39+
if len(dsts) != ct.NumField() {
40+
t.Fatalf("expected %d columns, got %d", ct.NumField(), len(dsts))
41+
}
42+
for i, dst := range dsts {
43+
f := ct.Field(i)
44+
dv := reflect.ValueOf(dst)
45+
if dv.Kind() != reflect.Ptr {
46+
t.Fatalf("scan argument at index %d is not a pointer", i)
47+
}
48+
if !f.Type.AssignableTo(dv.Elem().Type()) {
49+
t.Fatalf("%s is not assignable to argument %s at index %d", f.Type, dv.Elem().Type(), i)
50+
}
51+
dv.Elem().Set(reflect.ValueOf(cols).Field(i))
52+
}
53+
}
54+
55+
for _, tc := range []struct {
56+
Name string
57+
Querier func(t *testing.T) sqlite.Querier
58+
Bundles []*api.Bundle
59+
ErrorMessage string
60+
}{
61+
{
62+
Name: "returns error when query returns error",
63+
Querier: func(t *testing.T) sqlite.Querier {
64+
var q sqlitefakes.FakeQuerier
65+
q.QueryContextReturns(nil, fmt.Errorf("test"))
66+
return &q
67+
},
68+
ErrorMessage: "test",
69+
},
70+
{
71+
Name: "returns error when scan returns error",
72+
Querier: func(t *testing.T) sqlite.Querier {
73+
var (
74+
q sqlitefakes.FakeQuerier
75+
r sqlitefakes.FakeRowScanner
76+
)
77+
q.QueryContextReturns(&r, nil)
78+
r.NextReturnsOnCall(0, true)
79+
r.ScanReturns(fmt.Errorf("test"))
80+
return &q
81+
},
82+
ErrorMessage: "test",
83+
},
84+
{
85+
Name: "skips row without valid bundle name",
86+
Querier: func(t *testing.T) sqlite.Querier {
87+
var (
88+
q sqlitefakes.FakeQuerier
89+
r sqlitefakes.FakeRowScanner
90+
)
91+
q.QueryContextReturns(&r, nil)
92+
r.NextReturnsOnCall(0, true)
93+
r.ScanCalls(func(args ...interface{}) error {
94+
ScanFromColumns(t, args, Columns{
95+
Version: sql.NullString{Valid: true},
96+
BundlePath: sql.NullString{Valid: true},
97+
ChannelName: sql.NullString{Valid: true},
98+
})
99+
return nil
100+
})
101+
return &q
102+
},
103+
},
104+
{
105+
Name: "skips row without valid version",
106+
Querier: func(t *testing.T) sqlite.Querier {
107+
var (
108+
q sqlitefakes.FakeQuerier
109+
r sqlitefakes.FakeRowScanner
110+
)
111+
q.QueryContextReturns(&r, nil)
112+
r.NextReturnsOnCall(0, true)
113+
r.ScanCalls(func(args ...interface{}) error {
114+
ScanFromColumns(t, args, Columns{
115+
BundleName: sql.NullString{Valid: true},
116+
BundlePath: sql.NullString{Valid: true},
117+
ChannelName: sql.NullString{Valid: true},
118+
})
119+
return nil
120+
})
121+
return &q
122+
},
123+
},
124+
{
125+
Name: "skips row without valid bundle path",
126+
Querier: func(t *testing.T) sqlite.Querier {
127+
var (
128+
q sqlitefakes.FakeQuerier
129+
r sqlitefakes.FakeRowScanner
130+
)
131+
q.QueryContextReturns(&r, nil)
132+
r.NextReturnsOnCall(0, true)
133+
r.ScanCalls(func(args ...interface{}) error {
134+
ScanFromColumns(t, args, Columns{
135+
BundleName: sql.NullString{Valid: true},
136+
Version: sql.NullString{Valid: true},
137+
ChannelName: sql.NullString{Valid: true},
138+
})
139+
return nil
140+
})
141+
return &q
142+
},
143+
},
144+
{
145+
Name: "skips row without valid channel name",
146+
Querier: func(t *testing.T) sqlite.Querier {
147+
var (
148+
q sqlitefakes.FakeQuerier
149+
r sqlitefakes.FakeRowScanner
150+
)
151+
q.QueryContextReturns(&r, nil)
152+
r.NextReturnsOnCall(0, true)
153+
r.ScanCalls(func(args ...interface{}) error {
154+
ScanFromColumns(t, args, Columns{
155+
BundleName: sql.NullString{Valid: true},
156+
Version: sql.NullString{Valid: true},
157+
BundlePath: sql.NullString{Valid: true},
158+
})
159+
return nil
160+
})
161+
return &q
162+
},
163+
},
164+
{
165+
Name: "bundle dependencies are null when dependency type or value is null",
166+
Querier: func(t *testing.T) sqlite.Querier {
167+
var (
168+
q sqlitefakes.FakeQuerier
169+
r sqlitefakes.FakeRowScanner
170+
)
171+
q.QueryContextReturns(&r, nil)
172+
r.NextReturnsOnCall(0, true)
173+
r.ScanCalls(func(args ...interface{}) error {
174+
ScanFromColumns(t, args, Columns{
175+
BundleName: sql.NullString{Valid: true},
176+
Version: sql.NullString{Valid: true},
177+
ChannelName: sql.NullString{Valid: true},
178+
BundlePath: sql.NullString{Valid: true},
179+
})
180+
return nil
181+
})
182+
return &q
183+
},
184+
Bundles: []*api.Bundle{
185+
{},
186+
},
187+
},
188+
{
189+
Name: "all dependencies and properties are returned",
190+
Querier: func(t *testing.T) sqlite.Querier {
191+
var (
192+
q sqlitefakes.FakeQuerier
193+
r sqlitefakes.FakeRowScanner
194+
)
195+
q.QueryContextReturns(&NoRows, nil)
196+
q.QueryContextReturnsOnCall(0, &r, nil)
197+
r.NextReturnsOnCall(0, true)
198+
r.NextReturnsOnCall(1, true)
199+
cols := []Columns{
200+
{
201+
BundleName: sql.NullString{Valid: true, String: "BundleName"},
202+
Version: sql.NullString{Valid: true, String: "Version"},
203+
ChannelName: sql.NullString{Valid: true, String: "ChannelName"},
204+
BundlePath: sql.NullString{Valid: true, String: "BundlePath"},
205+
DependencyType: sql.NullString{Valid: true, String: "Dependency1Type"},
206+
DependencyValue: sql.NullString{Valid: true, String: "Dependency1Value"},
207+
PropertyType: sql.NullString{Valid: true, String: "Property1Type"},
208+
PropertyValue: sql.NullString{Valid: true, String: "Property1Value"},
209+
},
210+
{
211+
BundleName: sql.NullString{Valid: true, String: "BundleName"},
212+
Version: sql.NullString{Valid: true, String: "Version"},
213+
ChannelName: sql.NullString{Valid: true, String: "ChannelName"},
214+
BundlePath: sql.NullString{Valid: true, String: "BundlePath"},
215+
DependencyType: sql.NullString{Valid: true, String: "Dependency2Type"},
216+
DependencyValue: sql.NullString{Valid: true, String: "Dependency2Value"},
217+
PropertyType: sql.NullString{Valid: true, String: "Property2Type"},
218+
PropertyValue: sql.NullString{Valid: true, String: "Property2Value"},
219+
},
220+
}
221+
var i int
222+
r.ScanCalls(func(args ...interface{}) error {
223+
if i < len(cols) {
224+
ScanFromColumns(t, args, cols[i])
225+
i++
226+
}
227+
return nil
228+
})
229+
return &q
230+
},
231+
Bundles: []*api.Bundle{
232+
{
233+
CsvName: "BundleName",
234+
ChannelName: "ChannelName",
235+
BundlePath: "BundlePath",
236+
Version: "Version",
237+
Dependencies: []*api.Dependency{
238+
{
239+
Type: "Dependency1Type",
240+
Value: "Dependency1Value",
241+
},
242+
{
243+
Type: "Dependency2Type",
244+
Value: "Dependency2Value",
245+
},
246+
},
247+
Properties: []*api.Property{
248+
{
249+
Type: "Property1Type",
250+
Value: "Property1Value",
251+
},
252+
{
253+
Type: "Property2Type",
254+
Value: "Property2Value",
255+
},
256+
},
257+
},
258+
},
259+
},
260+
} {
261+
t.Run(tc.Name, func(t *testing.T) {
262+
var q sqlite.Querier
263+
if tc.Querier != nil {
264+
q = tc.Querier(t)
265+
}
266+
sq := sqlite.NewSQLLiteQuerierFromDBQuerier(q)
267+
bundles, err := sq.ListBundles(context.Background())
268+
269+
assert := assert.New(t)
270+
assert.Equal(tc.Bundles, bundles)
271+
if tc.ErrorMessage == "" {
272+
assert.NoError(err)
273+
} else {
274+
assert.EqualError(err, tc.ErrorMessage)
275+
}
276+
})
277+
}
278+
}

‎pkg/sqlite/sqlitefakes/fake_querier.go

+120
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎pkg/sqlite/sqlitefakes/fake_rowscanner.go

+238
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)
Please sign in to comment.