OPRUN-4548: bundle version increased type safety / serialization support (#1938)

* ensure that new types minimize internal access and maximize safeness

Signed-off-by: grokspawn <jordan@nimblewidget.com>

* provide serialization support

Signed-off-by: grokspawn <jordan@nimblewidget.com>

* review comments

Signed-off-by: grokspawn <jordan@nimblewidget.com>

* more review updates

Signed-off-by: grokspawn <jordan@nimblewidget.com>

---------

Signed-off-by: grokspawn <jordan@nimblewidget.com>

Author: grokspawn

alpha/declcfg/declcfg.go

@@ -5,16 +5,27 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"strings"
 
 	"github.com/blang/semver/v4"
 	"golang.org/x/text/cases"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/sets"
 
+	"github.com/operator-framework/operator-registry/alpha/model"
 	"github.com/operator-framework/operator-registry/alpha/property"
 	prettyunmarshaler "github.com/operator-framework/operator-registry/pkg/prettyunmarshaler"
+	"github.com/operator-framework/operator-registry/pkg/registry"
 )
 
+// Re-export VersionRelease/Release types/functions from model package to make it possible for users to only include this package and avoid import cycles
+type (
+	Release        = model.Release
+	VersionRelease = model.VersionRelease
+)
+
+var NewRelease = model.NewRelease
+
 const (
 	SchemaPackage     = "olm.package"
 	SchemaChannel     = "olm.channel"
@@ -208,24 +219,52 @@ func (destination *DeclarativeConfig) Merge(src *DeclarativeConfig) {
 	destination.Deprecations = append(destination.Deprecations, src.Deprecations...)
 }
 
-type CompositeVersion struct {
-	Version semver.Version
-	Release semver.Version
-}
-
-func (cv *CompositeVersion) Compare(other *CompositeVersion) int {
-	if cv.Version.NE(other.Version) {
-		return cv.Version.Compare(other.Version)
-	}
-	hasrelease := len(cv.Release.Pre) > 0
-	otherhasrelease := len(other.Release.Pre) > 0
-	if hasrelease && !otherhasrelease {
-		return 1
+// usesLegacyReleaseVersion returns true if the bundle's CSV contains an olm.substitutesFor annotation.
+// It checks three possible sources in order:
+// 1. CsvJSON field
+// 2. olm.csv.metadata property
+// 3. olm.bundle.object property containing a CSV
+// Returns false if no substitutesFor annotation is found.
+// NB: this can only return true for registry+v1 bundles which always have a CSV
+func (b *Bundle) usesLegacyReleaseVersion() bool {
+	const substitutesForAnnotationKey = "olm.substitutesFor"
+
+	// Path 1: Check CsvJSON field if present
+	if b.CsvJSON != "" {
+		var csv registry.ClusterServiceVersion
+		if err := json.Unmarshal([]byte(b.CsvJSON), &csv); err == nil {
+			return csv.GetSubstitutesFor() != ""
+		}
+		// On error, fall through to check other sources
 	}
-	if !hasrelease && otherhasrelease {
-		return -1
+
+	// Path 2 & 3: Check properties
+	for _, prop := range b.Properties {
+		switch prop.Type {
+		case property.TypeCSVMetadata:
+			var csvMeta property.CSVMetadata
+			if err := json.Unmarshal(prop.Value, &csvMeta); err != nil {
+				continue
+			}
+			if csvMeta.Annotations != nil {
+				if substitutes, ok := csvMeta.Annotations[substitutesForAnnotationKey]; ok && substitutes != "" {
+					return true
+				}
+			}
+
+		case property.TypeBundleObject:
+			var bundleObj property.BundleObject
+			if err := json.Unmarshal(prop.Value, &bundleObj); err != nil {
+				continue
+			}
+			var csv registry.ClusterServiceVersion
+			if err := json.Unmarshal(bundleObj.Data, &csv); err == nil {
+				return csv.GetSubstitutesFor() != ""
+			}
+		}
 	}
-	return cv.Release.Compare(other.Release)
+
+	return false
 }
 
 // order by version, then
@@ -234,44 +273,69 @@ func (b *Bundle) Compare(other *Bundle) int {
 	if b.Name == other.Name {
 		return 0
 	}
-	acv, err := b.CompositeVersion()
+	avr, err := b.VersionRelease()
 	if err != nil {
 		return 0
 	}
-	otherCv, err := other.CompositeVersion()
+	otherVr, err := other.VersionRelease()
 	if err != nil {
 		return 0
 	}
-	return acv.Compare(otherCv)
+	return avr.Compare(otherVr)
 }
 
-func (b *Bundle) CompositeVersion() (*CompositeVersion, error) {
-	props, err := property.Parse(b.Properties)
-	if err != nil {
-		return nil, fmt.Errorf("parse properties for bundle %q: %v", b.Name, err)
-	}
-	if len(props.Packages) != 1 {
-		return nil, fmt.Errorf("bundle %q must have exactly 1 \"olm.package\" property, found %v", b.Name, len(props.Packages))
+// constructs a VersionRelease from the olm.package property of the bundle
+// this handles the cases where the property is present, missing, or duplicated
+// if a release field is present in the property, it is used as-is
+// if it is NOT present in the property, but the version field contains build metadata,
+// we attempt to convert the build metadata into a release and strip the build metadata from the version.
+// This is to support bundles that use the legacy approach of encoding release information in the build metadata field of the version
+func (b *Bundle) VersionRelease() (*VersionRelease, error) {
+	var (
+		vr *VersionRelease
+	)
+	// loop over all properties, and do not break if we find a package property, in order to check for duplicates
+	for _, prop := range b.Properties {
+		switch prop.Type {
+		case property.TypePackage:
+			var p property.Package
+
+			// if we encounter more than one olm.package property, return an error
+			if vr != nil {
+				return nil, fmt.Errorf("must be exactly one property of type %q", SchemaPackage)
+			}
+
+			if err := json.Unmarshal(prop.Value, &p); err != nil {
+				return nil, fmt.Errorf("unable to unmarshal \"olm.package\" property for bundle %q: %v", b.Name, err)
+			}
+			pv, err := semver.Parse(p.Version)
+			if err != nil {
+				return nil, fmt.Errorf("invalid semver version %q in \"olm.package\" property for bundle %q: %v", p.Version, b.Name, err)
+			}
+			pr, err := NewRelease(p.Release)
+			if err != nil {
+				return nil, fmt.Errorf("invalid release %q in \"olm.package\" property for bundle %q: %v", p.Release, b.Name, err)
+			}
+			vr = &VersionRelease{
+				Version: pv,
+				Release: pr,
+			}
+		}
 	}
-	v, err := semver.Parse(props.Packages[0].Version)
-	if err != nil {
-		return nil, fmt.Errorf("bundle %q has invalid version %q: %v", b.Name, props.Packages[0].Version, err)
+	if vr == nil {
+		return nil, fmt.Errorf("no \"olm.package\" property found for bundle %q", b.Name)
 	}
 
-	var r semver.Version
-	if props.Packages[0].Release != "" {
-		r, err = semver.Parse(fmt.Sprintf("0.0.0-%s", props.Packages[0].Release))
+	// if the bundle's release isn't provided, see if we can use the legacy build metadata release approach to identify a release
+	// if successful, remove the build metadata from the version. Only attempt for bundles using legacy release versioning.
+	if len(vr.Release) == 0 && vr.Version.Build != nil && b.usesLegacyReleaseVersion() {
+		newrel, err := NewRelease(strings.Join(vr.Version.Build, "."))
 		if err != nil {
-			return nil, fmt.Errorf("error parsing bundle %q release version %q: %v", b.Name, props.Packages[0].Release, err)
-		}
-		// only need to check for build metadata since we are using explicit zero major, minor, and patch versions above
-		if len(r.Build) != 0 {
-			return nil, fmt.Errorf("bundle %q release version %q cannot contain build metadata", b.Name, props.Packages[0].Release)
+			return nil, fmt.Errorf("unable to convert build metadata to release for bundle %q: %v", b.Name, err)
 		}
+		vr.Release = newrel
+		vr.Version.Build = nil
 	}
 
-	return &CompositeVersion{
-		Version: v,
-		Release: r,
-	}, nil
+	return vr, nil
 }

alpha/declcfg/declcfg_to_model.go

@@ -146,16 +146,12 @@ func ConvertToModel(cfg DeclarativeConfig) (model.Model, error) {
 		}
 
 		// Parse release version from the package property.
-		var relver semver.Version
+		var relver model.Release
 		if props.Packages[0].Release != "" {
-			relver, err = semver.Parse(fmt.Sprintf("0.0.0-%s", props.Packages[0].Release))
+			relver, err = model.NewRelease(props.Packages[0].Release)
 			if err != nil {
 				return nil, fmt.Errorf("error parsing bundle %q release version %q: %v", b.Name, props.Packages[0].Release, err)
 			}
-			// only need to check for build metadata since we are using explicit zero major, minor, and patch versions above
-			if len(relver.Build) != 0 {
-				return nil, fmt.Errorf("bundle %q release version %q cannot contain build metadata", b.Name, props.Packages[0].Release)
-			}
 		}
 
 		channelDefinedEntries[b.Package] = channelDefinedEntries[b.Package].Delete(b.Name)
@@ -170,7 +166,8 @@ func ConvertToModel(cfg DeclarativeConfig) (model.Model, error) {
 				mb.Objects = b.Objects
 				mb.PropertiesP = props
 				mb.Version = ver
-				mb.Release = relver
+				// TODO: Jordan: follow-up will evolve the internal types for more consistent use of VersionRelease
+				mb.Release = semver.Version{Pre: relver}
 			}
 		}
 		if !found {

alpha/declcfg/declcfg_to_model_test.go

@@ -444,7 +444,7 @@ func TestConvertToModel(t *testing.T) {
 		},
 		{
 			name:      "Error/InvalidReleaseVersion",
-			assertion: hasError(`error parsing bundle "foo.v0.1.0" release version "!!!": Invalid character(s) found in prerelease "!!!"`),
+			assertion: hasError(`error parsing bundle "foo.v0.1.0" release version "!!!": invalid release "!!!": segment 0: Invalid character(s) found in prerelease "!!!"`),
 			cfg: DeclarativeConfig{
 				Packages: []Package{newTestPackage("foo", "alpha", svgSmallCircle)},
 				Channels: []Channel{newTestChannel("foo", "alpha", ChannelEntry{Name: testBundleName("foo", "0.1.0")})},