From 06f97d1335ae09e7f0820670fe48b976c400f6f9 Mon Sep 17 00:00:00 2001 From: Mikalai Radchuk <509198+m1kola@users.noreply.github.com> Date: Thu, 21 Sep 2023 15:51:39 +0100 Subject: [PATCH] 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 --- internal/catalogmetadata/types.go | 57 ++++++++++++------- internal/catalogmetadata/types_test.go | 8 ++- .../bundles_and_dependencies_test.go | 18 +++--- .../variablesources/crd_constraints_test.go | 12 ++-- 4 files changed, 58 insertions(+), 37 deletions(-) diff --git a/internal/catalogmetadata/types.go b/internal/catalogmetadata/types.go index a1eede3fb..4eb72bcf4 100644 --- a/internal/catalogmetadata/types.go +++ b/internal/catalogmetadata/types.go @@ -41,7 +41,7 @@ type Bundle struct { mu sync.RWMutex // these properties are lazy loaded as they are requested - propertiesMap map[string]property.Property + propertiesMap map[string][]*property.Property bundlePackage *property.Package semVersion *bsemver.Version requiredPackages []PackageRequired @@ -74,7 +74,7 @@ func (b *Bundle) loadPackage() error { b.mu.Lock() defer b.mu.Unlock() if b.bundlePackage == nil { - bundlePackage, err := loadFromProps[property.Package](b, property.TypePackage, true) + bundlePackage, err := loadOneFromProps[property.Package](b, property.TypePackage, true) if err != nil { return err } @@ -94,7 +94,7 @@ func (b *Bundle) loadRequiredPackages() error { b.mu.Lock() defer b.mu.Unlock() if b.requiredPackages == nil { - requiredPackages, err := loadFromProps[[]PackageRequired](b, property.TypePackageRequired, false) + requiredPackages, err := loadFromProps[PackageRequired](b, property.TypePackageRequired, false) if err != nil { return fmt.Errorf("error determining bundle required packages for bundle %q: %s", b.Name, err) } @@ -119,7 +119,7 @@ func (b *Bundle) loadMediaType() error { b.mu.Lock() defer b.mu.Unlock() if b.mediaType == nil { - mediaType, err := loadFromProps[string](b, PropertyBundleMediaType, false) + mediaType, err := loadOneFromProps[string](b, PropertyBundleMediaType, false) if err != nil { return fmt.Errorf("error determining bundle mediatype for bundle %q: %s", b.Name, err) } @@ -128,31 +128,48 @@ func (b *Bundle) loadMediaType() error { return nil } -func (b *Bundle) propertyByType(propType string) *property.Property { +func (b *Bundle) propertiesByType(propType string) []*property.Property { if b.propertiesMap == nil { - b.propertiesMap = make(map[string]property.Property) - for _, prop := range b.Properties { - b.propertiesMap[prop.Type] = prop + b.propertiesMap = make(map[string][]*property.Property) + for i := range b.Properties { + prop := b.Properties[i] + b.propertiesMap[prop.Type] = append(b.propertiesMap[prop.Type], &prop) } } - prop, ok := b.propertiesMap[propType] - if !ok { - return nil + return b.propertiesMap[propType] +} + +func loadOneFromProps[T any](bundle *Bundle, propType string, required bool) (T, error) { + r, err := loadFromProps[T](bundle, propType, required) + if err != nil { + return *new(T), err + } + if len(r) > 1 { + return *new(T), fmt.Errorf("expected 1 instance of property with type %q, got %d", propType, len(r)) } - return &prop + if !required && len(r) == 0 { + return *new(T), nil + } + + return r[0], nil } -func loadFromProps[T any](bundle *Bundle, propType string, required bool) (T, error) { - parsedProp := *new(T) - prop := bundle.propertyByType(propType) - if prop != nil { - if err := json.Unmarshal(prop.Value, &parsedProp); err != nil { - return parsedProp, fmt.Errorf("property %q with value %q could not be parsed: %s", propType, prop.Value, err) +func loadFromProps[T any](bundle *Bundle, propType string, required bool) ([]T, error) { + props := bundle.propertiesByType(propType) + if len(props) != 0 { + result := []T{} + for i := range props { + parsedProp := *new(T) + if err := json.Unmarshal(props[i].Value, &parsedProp); err != nil { + return nil, fmt.Errorf("property %q with value %q could not be parsed: %s", propType, props[i].Value, err) + } + result = append(result, parsedProp) } + return result, nil } else if required { - return parsedProp, fmt.Errorf("bundle property with type %q not found", propType) + return nil, fmt.Errorf("bundle property with type %q not found", propType) } - return parsedProp, nil + return nil, nil } diff --git a/internal/catalogmetadata/types_test.go b/internal/catalogmetadata/types_test.go index b42ec9a94..8ecb92508 100644 --- a/internal/catalogmetadata/types_test.go +++ b/internal/catalogmetadata/types_test.go @@ -83,7 +83,11 @@ func TestBundleRequiredPackages(t *testing.T) { Properties: []property.Property{ { Type: property.TypePackageRequired, - Value: json.RawMessage(`[{"packageName": "packageA", "versionRange": ">1.0.0"}, {"packageName": "packageB", "versionRange": ">0.5.0 <0.8.6"}]`), + Value: json.RawMessage(`{"packageName": "packageA", "versionRange": ">1.0.0"}`), + }, + { + Type: property.TypePackageRequired, + Value: json.RawMessage(`{"packageName": "packageB", "versionRange": ">0.5.0 <0.8.6"}`), }, }, }}, @@ -113,7 +117,7 @@ func TestBundleRequiredPackages(t *testing.T) { Properties: []property.Property{ { Type: property.TypePackageRequired, - Value: json.RawMessage(`[{"packageName": "packageA", "versionRange": "invalid"}]`), + Value: json.RawMessage(`{"packageName": "packageA", "versionRange": "invalid"}`), }, }, }}, diff --git a/internal/resolution/variablesources/bundles_and_dependencies_test.go b/internal/resolution/variablesources/bundles_and_dependencies_test.go index 3c0db1c9d..78805f75e 100644 --- a/internal/resolution/variablesources/bundles_and_dependencies_test.go +++ b/internal/resolution/variablesources/bundles_and_dependencies_test.go @@ -48,8 +48,8 @@ var _ = Describe("BundlesAndDepsVariableSource", func() { Package: "test-package", Properties: []property.Property{ {Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "2.0.0"}`)}, - {Type: property.TypeGVKRequired, Value: json.RawMessage(`[{"group":"foo.io","kind":"Foo","version":"v1"}]`)}, - {Type: property.TypePackageRequired, Value: json.RawMessage(`[{"packageName": "some-package", "versionRange": ">=1.0.0 <2.0.0"}]`)}, + {Type: property.TypeGVKRequired, Value: json.RawMessage(`{"group":"foo.io","kind":"Foo","version":"v1"}`)}, + {Type: property.TypePackageRequired, Value: json.RawMessage(`{"packageName": "some-package", "versionRange": ">=1.0.0 <2.0.0"}`)}, }, }, InChannels: []*catalogmetadata.Channel{&channel}, @@ -108,9 +108,9 @@ var _ = Describe("BundlesAndDepsVariableSource", func() { Package: "some-other-package", Properties: []property.Property{ {Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "some-other-package", "version": "1.5.0"}`)}, - {Type: property.TypeGVK, Value: json.RawMessage(`[{"group":"foo.io","kind":"Foo","version":"v1"}]`)}, - {Type: property.TypeGVKRequired, Value: json.RawMessage(`[{"group":"bar.io","kind":"Bar","version":"v1"}]`)}, - {Type: property.TypePackageRequired, Value: json.RawMessage(`[{"packageName": "another-package", "versionRange": "< 2.0.0"}]`)}, + {Type: property.TypeGVK, Value: json.RawMessage(`{"group":"foo.io","kind":"Foo","version":"v1"}`)}, + {Type: property.TypeGVKRequired, Value: json.RawMessage(`{"group":"bar.io","kind":"Bar","version":"v1"}`)}, + {Type: property.TypePackageRequired, Value: json.RawMessage(`{"packageName": "another-package", "versionRange": "< 2.0.0"}`)}, }, }, InChannels: []*catalogmetadata.Channel{&channel}, @@ -236,8 +236,8 @@ var _ = Describe("BundlesAndDepsVariableSource", func() { Package: "test-package", Properties: []property.Property{ {Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "2.0.0"}`)}, - {Type: property.TypeGVKRequired, Value: json.RawMessage(`[{"group":"foo.io","kind":"Foo","version":"v1"}]`)}, - {Type: property.TypePackageRequired, Value: json.RawMessage(`[{"packageName": "some-package", "versionRange": ">=1.0.0 <2.0.0"}]`)}, + {Type: property.TypeGVKRequired, Value: json.RawMessage(`{"group":"foo.io","kind":"Foo","version":"v1"}`)}, + {Type: property.TypePackageRequired, Value: json.RawMessage(`{"packageName": "some-package", "versionRange": ">=1.0.0 <2.0.0"}`)}, }, }, InChannels: []*catalogmetadata.Channel{&channel}, @@ -350,8 +350,8 @@ var _ = Describe("BundlesAndDepsVariableSource", func() { Package: "test-package", Properties: []property.Property{ {Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "2.0.0"}`)}, - {Type: property.TypeGVKRequired, Value: json.RawMessage(`[{"group":"foo.io","kind":"Foo","version":"v1"}]`)}, - {Type: property.TypePackageRequired, Value: json.RawMessage(`[{"packageName": "some-package", "versionRange": ">=1.0.0 <2.0.0"}]`)}, + {Type: property.TypeGVKRequired, Value: json.RawMessage(`{"group":"foo.io","kind":"Foo","version":"v1"}`)}, + {Type: property.TypePackageRequired, Value: json.RawMessage(`{"packageName": "some-package", "versionRange": ">=1.0.0 <2.0.0"}`)}, }, }, InChannels: []*catalogmetadata.Channel{{Channel: declcfg.Channel{Name: "stable"}}}, diff --git a/internal/resolution/variablesources/crd_constraints_test.go b/internal/resolution/variablesources/crd_constraints_test.go index 200d673f8..d9c02c608 100644 --- a/internal/resolution/variablesources/crd_constraints_test.go +++ b/internal/resolution/variablesources/crd_constraints_test.go @@ -35,9 +35,9 @@ var bundleSet = map[string]*catalogmetadata.Bundle{ Package: "test-package", Properties: []property.Property{ {Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "2.0.0"}`)}, - {Type: property.TypeGVKRequired, Value: json.RawMessage(`[{"group":"foo.io","kind":"Foo","version":"v1"}]`)}, - {Type: property.TypePackageRequired, Value: json.RawMessage(`[{"packageName": "some-package", "versionRange": ">=1.0.0 <2.0.0"}]`)}, - {Type: property.TypeGVK, Value: json.RawMessage(`[{"group":"bit.io","kind":"Bit","version":"v1"}]`)}, + {Type: property.TypeGVKRequired, Value: json.RawMessage(`{"group":"foo.io","kind":"Foo","version":"v1"}`)}, + {Type: property.TypePackageRequired, Value: json.RawMessage(`{"packageName": "some-package", "versionRange": ">=1.0.0 <2.0.0"}`)}, + {Type: property.TypeGVK, Value: json.RawMessage(`{"group":"bit.io","kind":"Bit","version":"v1"}`)}, }}, InChannels: []*catalogmetadata.Channel{&channel}, }, @@ -84,9 +84,9 @@ var bundleSet = map[string]*catalogmetadata.Bundle{ Package: "some-other-package", Properties: []property.Property{ {Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "some-other-package", "version": "1.5.0"}`)}, - {Type: property.TypeGVK, Value: json.RawMessage(`[{"group":"foo.io","kind":"Foo","version":"v1"}]`)}, - {Type: property.TypeGVKRequired, Value: json.RawMessage(`[{"group":"bar.io","kind":"Bar","version":"v1"}]`)}, - {Type: property.TypePackageRequired, Value: json.RawMessage(`[{"packageName": "another-package", "versionRange": "< 2.0.0"}]`)}, + {Type: property.TypeGVK, Value: json.RawMessage(`{"group":"foo.io","kind":"Foo","version":"v1"}`)}, + {Type: property.TypeGVKRequired, Value: json.RawMessage(`{"group":"bar.io","kind":"Bar","version":"v1"}`)}, + {Type: property.TypePackageRequired, Value: json.RawMessage(`{"packageName": "another-package", "versionRange": "< 2.0.0"}`)}, }}, InChannels: []*catalogmetadata.Channel{&channel}, },