Skip to content

Commit 06f97d1

Browse files
m1kolam1kola
authored
Fixes reading properties with the same type (#427)
Some properties like `olm.package` only can appear in a bundle once and we currently handle this correctly. However properties such as `olm.gvk` and `olm.gvk.required` can be present multiple times in the list. The current implementation overrides all the instances with the one which is the last in the list. This modifies the code to be able to read in both cases. Signed-off-by: Mikalai Radchuk <[email protected]>
1 parent 52a1e28 commit 06f97d1

File tree

4 files changed

+58
-37
lines changed

4 files changed

+58
-37
lines changed

‎internal/catalogmetadata/types.go‎

Lines changed: 37 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ type Bundle struct {
4141

4242
mu sync.RWMutex
4343
// these properties are lazy loaded as they are requested
44-
propertiesMap map[string]property.Property
44+
propertiesMap map[string][]*property.Property
4545
bundlePackage *property.Package
4646
semVersion *bsemver.Version
4747
requiredPackages []PackageRequired
@@ -74,7 +74,7 @@ func (b *Bundle) loadPackage() error {
7474
b.mu.Lock()
7575
defer b.mu.Unlock()
7676
if b.bundlePackage == nil {
77-
bundlePackage, err := loadFromProps[property.Package](b, property.TypePackage, true)
77+
bundlePackage, err := loadOneFromProps[property.Package](b, property.TypePackage, true)
7878
if err != nil {
7979
return err
8080
}
@@ -94,7 +94,7 @@ func (b *Bundle) loadRequiredPackages() error {
9494
b.mu.Lock()
9595
defer b.mu.Unlock()
9696
if b.requiredPackages == nil {
97-
requiredPackages, err := loadFromProps[[]PackageRequired](b, property.TypePackageRequired, false)
97+
requiredPackages, err := loadFromProps[PackageRequired](b, property.TypePackageRequired, false)
9898
if err != nil {
9999
return fmt.Errorf("error determining bundle required packages for bundle %q: %s", b.Name, err)
100100
}
@@ -119,7 +119,7 @@ func (b *Bundle) loadMediaType() error {
119119
b.mu.Lock()
120120
defer b.mu.Unlock()
121121
if b.mediaType == nil {
122-
mediaType, err := loadFromProps[string](b, PropertyBundleMediaType, false)
122+
mediaType, err := loadOneFromProps[string](b, PropertyBundleMediaType, false)
123123
if err != nil {
124124
return fmt.Errorf("error determining bundle mediatype for bundle %q: %s", b.Name, err)
125125
}
@@ -128,31 +128,48 @@ func (b *Bundle) loadMediaType() error {
128128
return nil
129129
}
130130

131-
func (b *Bundle) propertyByType(propType string) *property.Property {
131+
func (b *Bundle) propertiesByType(propType string) []*property.Property {
132132
if b.propertiesMap == nil {
133-
b.propertiesMap = make(map[string]property.Property)
134-
for _, prop := range b.Properties {
135-
b.propertiesMap[prop.Type] = prop
133+
b.propertiesMap = make(map[string][]*property.Property)
134+
for i := range b.Properties {
135+
prop := b.Properties[i]
136+
b.propertiesMap[prop.Type] = append(b.propertiesMap[prop.Type], &prop)
136137
}
137138
}
138139

139-
prop, ok := b.propertiesMap[propType]
140-
if !ok {
141-
return nil
140+
return b.propertiesMap[propType]
141+
}
142+
143+
func loadOneFromProps[T any](bundle *Bundle, propType string, required bool) (T, error) {
144+
r, err := loadFromProps[T](bundle, propType, required)
145+
if err != nil {
146+
return *new(T), err
147+
}
148+
if len(r) > 1 {
149+
return *new(T), fmt.Errorf("expected 1 instance of property with type %q, got %d", propType, len(r))
142150
}
143-
return &prop
151+
if !required && len(r) == 0 {
152+
return *new(T), nil
153+
}
154+
155+
return r[0], nil
144156
}
145157

146-
func loadFromProps[T any](bundle *Bundle, propType string, required bool) (T, error) {
147-
parsedProp := *new(T)
148-
prop := bundle.propertyByType(propType)
149-
if prop != nil {
150-
if err := json.Unmarshal(prop.Value, &parsedProp); err != nil {
151-
return parsedProp, fmt.Errorf("property %q with value %q could not be parsed: %s", propType, prop.Value, err)
158+
func loadFromProps[T any](bundle *Bundle, propType string, required bool) ([]T, error) {
159+
props := bundle.propertiesByType(propType)
160+
if len(props) != 0 {
161+
result := []T{}
162+
for i := range props {
163+
parsedProp := *new(T)
164+
if err := json.Unmarshal(props[i].Value, &parsedProp); err != nil {
165+
return nil, fmt.Errorf("property %q with value %q could not be parsed: %s", propType, props[i].Value, err)
166+
}
167+
result = append(result, parsedProp)
152168
}
169+
return result, nil
153170
} else if required {
154-
return parsedProp, fmt.Errorf("bundle property with type %q not found", propType)
171+
return nil, fmt.Errorf("bundle property with type %q not found", propType)
155172
}
156173

157-
return parsedProp, nil
174+
return nil, nil
158175
}

‎internal/catalogmetadata/types_test.go‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,11 @@ func TestBundleRequiredPackages(t *testing.T) {
8383
Properties: []property.Property{
8484
{
8585
Type: property.TypePackageRequired,
86-
Value: json.RawMessage(`[{"packageName": "packageA", "versionRange": ">1.0.0"}, {"packageName": "packageB", "versionRange": ">0.5.0 <0.8.6"}]`),
86+
Value: json.RawMessage(`{"packageName": "packageA", "versionRange": ">1.0.0"}`),
87+
},
88+
{
89+
Type: property.TypePackageRequired,
90+
Value: json.RawMessage(`{"packageName": "packageB", "versionRange": ">0.5.0 <0.8.6"}`),
8791
},
8892
},
8993
}},
@@ -113,7 +117,7 @@ func TestBundleRequiredPackages(t *testing.T) {
113117
Properties: []property.Property{
114118
{
115119
Type: property.TypePackageRequired,
116-
Value: json.RawMessage(`[{"packageName": "packageA", "versionRange": "invalid"}]`),
120+
Value: json.RawMessage(`{"packageName": "packageA", "versionRange": "invalid"}`),
117121
},
118122
},
119123
}},

‎internal/resolution/variablesources/bundles_and_dependencies_test.go‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,8 @@ var _ = Describe("BundlesAndDepsVariableSource", func() {
4848
Package: "test-package",
4949
Properties: []property.Property{
5050
{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "2.0.0"}`)},
51-
{Type: property.TypeGVKRequired, Value: json.RawMessage(`[{"group":"foo.io","kind":"Foo","version":"v1"}]`)},
52-
{Type: property.TypePackageRequired, Value: json.RawMessage(`[{"packageName": "some-package", "versionRange": ">=1.0.0 <2.0.0"}]`)},
51+
{Type: property.TypeGVKRequired, Value: json.RawMessage(`{"group":"foo.io","kind":"Foo","version":"v1"}`)},
52+
{Type: property.TypePackageRequired, Value: json.RawMessage(`{"packageName": "some-package", "versionRange": ">=1.0.0 <2.0.0"}`)},
5353
},
5454
},
5555
InChannels: []*catalogmetadata.Channel{&channel},
@@ -108,9 +108,9 @@ var _ = Describe("BundlesAndDepsVariableSource", func() {
108108
Package: "some-other-package",
109109
Properties: []property.Property{
110110
{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "some-other-package", "version": "1.5.0"}`)},
111-
{Type: property.TypeGVK, Value: json.RawMessage(`[{"group":"foo.io","kind":"Foo","version":"v1"}]`)},
112-
{Type: property.TypeGVKRequired, Value: json.RawMessage(`[{"group":"bar.io","kind":"Bar","version":"v1"}]`)},
113-
{Type: property.TypePackageRequired, Value: json.RawMessage(`[{"packageName": "another-package", "versionRange": "< 2.0.0"}]`)},
111+
{Type: property.TypeGVK, Value: json.RawMessage(`{"group":"foo.io","kind":"Foo","version":"v1"}`)},
112+
{Type: property.TypeGVKRequired, Value: json.RawMessage(`{"group":"bar.io","kind":"Bar","version":"v1"}`)},
113+
{Type: property.TypePackageRequired, Value: json.RawMessage(`{"packageName": "another-package", "versionRange": "< 2.0.0"}`)},
114114
},
115115
},
116116
InChannels: []*catalogmetadata.Channel{&channel},
@@ -236,8 +236,8 @@ var _ = Describe("BundlesAndDepsVariableSource", func() {
236236
Package: "test-package",
237237
Properties: []property.Property{
238238
{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "2.0.0"}`)},
239-
{Type: property.TypeGVKRequired, Value: json.RawMessage(`[{"group":"foo.io","kind":"Foo","version":"v1"}]`)},
240-
{Type: property.TypePackageRequired, Value: json.RawMessage(`[{"packageName": "some-package", "versionRange": ">=1.0.0 <2.0.0"}]`)},
239+
{Type: property.TypeGVKRequired, Value: json.RawMessage(`{"group":"foo.io","kind":"Foo","version":"v1"}`)},
240+
{Type: property.TypePackageRequired, Value: json.RawMessage(`{"packageName": "some-package", "versionRange": ">=1.0.0 <2.0.0"}`)},
241241
},
242242
},
243243
InChannels: []*catalogmetadata.Channel{&channel},
@@ -350,8 +350,8 @@ var _ = Describe("BundlesAndDepsVariableSource", func() {
350350
Package: "test-package",
351351
Properties: []property.Property{
352352
{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "2.0.0"}`)},
353-
{Type: property.TypeGVKRequired, Value: json.RawMessage(`[{"group":"foo.io","kind":"Foo","version":"v1"}]`)},
354-
{Type: property.TypePackageRequired, Value: json.RawMessage(`[{"packageName": "some-package", "versionRange": ">=1.0.0 <2.0.0"}]`)},
353+
{Type: property.TypeGVKRequired, Value: json.RawMessage(`{"group":"foo.io","kind":"Foo","version":"v1"}`)},
354+
{Type: property.TypePackageRequired, Value: json.RawMessage(`{"packageName": "some-package", "versionRange": ">=1.0.0 <2.0.0"}`)},
355355
},
356356
},
357357
InChannels: []*catalogmetadata.Channel{{Channel: declcfg.Channel{Name: "stable"}}},

‎internal/resolution/variablesources/crd_constraints_test.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,9 @@ var bundleSet = map[string]*catalogmetadata.Bundle{
3535
Package: "test-package",
3636
Properties: []property.Property{
3737
{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "2.0.0"}`)},
38-
{Type: property.TypeGVKRequired, Value: json.RawMessage(`[{"group":"foo.io","kind":"Foo","version":"v1"}]`)},
39-
{Type: property.TypePackageRequired, Value: json.RawMessage(`[{"packageName": "some-package", "versionRange": ">=1.0.0 <2.0.0"}]`)},
40-
{Type: property.TypeGVK, Value: json.RawMessage(`[{"group":"bit.io","kind":"Bit","version":"v1"}]`)},
38+
{Type: property.TypeGVKRequired, Value: json.RawMessage(`{"group":"foo.io","kind":"Foo","version":"v1"}`)},
39+
{Type: property.TypePackageRequired, Value: json.RawMessage(`{"packageName": "some-package", "versionRange": ">=1.0.0 <2.0.0"}`)},
40+
{Type: property.TypeGVK, Value: json.RawMessage(`{"group":"bit.io","kind":"Bit","version":"v1"}`)},
4141
}},
4242
InChannels: []*catalogmetadata.Channel{&channel},
4343
},
@@ -84,9 +84,9 @@ var bundleSet = map[string]*catalogmetadata.Bundle{
8484
Package: "some-other-package",
8585
Properties: []property.Property{
8686
{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "some-other-package", "version": "1.5.0"}`)},
87-
{Type: property.TypeGVK, Value: json.RawMessage(`[{"group":"foo.io","kind":"Foo","version":"v1"}]`)},
88-
{Type: property.TypeGVKRequired, Value: json.RawMessage(`[{"group":"bar.io","kind":"Bar","version":"v1"}]`)},
89-
{Type: property.TypePackageRequired, Value: json.RawMessage(`[{"packageName": "another-package", "versionRange": "< 2.0.0"}]`)},
87+
{Type: property.TypeGVK, Value: json.RawMessage(`{"group":"foo.io","kind":"Foo","version":"v1"}`)},
88+
{Type: property.TypeGVKRequired, Value: json.RawMessage(`{"group":"bar.io","kind":"Bar","version":"v1"}`)},
89+
{Type: property.TypePackageRequired, Value: json.RawMessage(`{"packageName": "another-package", "versionRange": "< 2.0.0"}`)},
9090
}},
9191
InChannels: []*catalogmetadata.Channel{&channel},
9292
},

0 commit comments

Comments
 (0)