🐛 Add support for build metadata precedence in bundle version comparison

internal/operator-controller/bundle/versionrelease.go

@@ -0,0 +1,89 @@
+package bundle
+
+import (
+	"errors"
+	"fmt"
+	"strings"
+
+	bsemver "github.com/blang/semver/v4"
+
+	slicesutil "github.com/operator-framework/operator-controller/internal/shared/util/slices"
+)
+
+func NewLegacyRegistryV1VersionRelease(vStr string) (*VersionRelease, error) {
+	vers, err := bsemver.Parse(vStr)
+	if err != nil {
+		return nil, err
+	}
+
+	rel, err := NewRelease(strings.Join(vers.Build, "."))
+	if err != nil {
+		return nil, err
+	}
+	vers.Build = nil
+
+	return &VersionRelease{
+		Version: vers,
+		Release: rel,
+	}, nil
+}
+
+type VersionRelease struct {
+	Version bsemver.Version
+	Release Release
+}
+
+func (vr *VersionRelease) Compare(other VersionRelease) int {
+	if vCmp := vr.Version.Compare(other.Version); vCmp != 0 {
+		return vCmp
+	}
+	return vr.Release.Compare(other.Release)
+}
+
+func (vr *VersionRelease) AsLegacyRegistryV1Version() bsemver.Version {
+	return bsemver.Version{
+		Major: vr.Version.Major,
+		Minor: vr.Version.Minor,
+		Patch: vr.Version.Patch,
+		Pre:   vr.Version.Pre,
+		Build: slicesutil.Map(vr.Release, func(i bsemver.PRVersion) string { return i.String() }),
+	}
+}
+
+type Release []bsemver.PRVersion
+
+func (r Release) Compare(other Release) int {
+	if len(r) == 0 && len(other) > 0 {
+		return -1
+	}
+	if len(other) == 0 && len(r) > 0 {
+		return 1
+	}
+	a := bsemver.Version{Pre: r}
+	b := bsemver.Version{Pre: other}
+	return a.Compare(b)
+}
+
+func NewRelease(relStr string) (Release, error) {
+	if relStr == "" {
+		return nil, nil
+	}
+
+	var (
+		segments = strings.Split(relStr, ".")
+		r        = make(Release, 0, len(segments))
+		errs     []error
+	)
+	for i, segment := range segments {
+		prVer, err := bsemver.NewPRVersion(segment)
+		if err != nil {
+			errs = append(errs, fmt.Errorf("segment %d: %v", i, err))
+			continue
+		}
+		r = append(r, prVer)
+	}
+	if err := errors.Join(errs...); err != nil {
+		return nil, fmt.Errorf("invalid release %q: %v", relStr, err)
+	}
+	return r, nil
+}

internal/operator-controller/bundle/versionrelease_test.go

@@ -0,0 +1,121 @@
+package bundle_test
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/operator-framework/operator-controller/internal/operator-controller/bundle"
+)
+
+func TestLegacyRegistryV1VersionRelease_Compare(t *testing.T) {
+	type testCase struct {
+		name   string
+		v1     string
+		v2     string
+		expect int
+	}
+	for _, tc := range []testCase{
+		{
+			name:   "lower major version",
+			v1:     "1.0.0-0+0",
+			v2:     "2.0.0-0+0",
+			expect: -1,
+		},
+		{
+			name:   "lower minor version",
+			v1:     "0.1.0-0+0",
+			v2:     "0.2.0-0+0",
+			expect: -1,
+		},
+		{
+			name:   "lower patch version",
+			v1:     "0.0.1-0+0",
+			v2:     "0.0.2-0+0",
+			expect: -1,
+		},
+		{
+			name:   "lower prerelease version",
+			v1:     "0.0.0-1+0",
+			v2:     "0.0.0-2+0",
+			expect: -1,
+		},
+		{
+			name:   "lower build metadata",
+			v1:     "0.0.0-0+1",
+			v2:     "0.0.0-0+2",
+			expect: -1,
+		},
+		{
+			name:   "same major version",
+			v1:     "1.0.0-0+0",
+			v2:     "1.0.0-0+0",
+			expect: 0,
+		},
+		{
+			name:   "same minor version",
+			v1:     "0.1.0-0+0",
+			v2:     "0.1.0-0+0",
+			expect: 0,
+		},
+		{
+			name:   "same patch version",
+			v1:     "0.0.1-0+0",
+			v2:     "0.0.1-0+0",
+			expect: 0,
+		},
+		{
+			name:   "same prerelease version",
+			v1:     "0.0.0-1+0",
+			v2:     "0.0.0-1+0",
+			expect: 0,
+		},
+		{
+			name:   "same build metadata",
+			v1:     "0.0.0-0+1",
+			v2:     "0.0.0-0+1",
+			expect: 0,
+		},
+		{
+			name:   "higher major version",
+			v1:     "2.0.0-0+0",
+			v2:     "1.0.0-0+0",
+			expect: 1,
+		},
+		{
+			name:   "higher minor version",
+			v1:     "0.2.0-0+0",
+			v2:     "0.1.0-0+0",
+			expect: 1,
+		},
+		{
+			name:   "higher patch version",
+			v1:     "0.0.2-0+0",
+			v2:     "0.0.1-0+0",
+			expect: 1,
+		},
+		{
+			name:   "higher prerelease version",
+			v1:     "0.0.0-2+0",
+			v2:     "0.0.0-1+0",
+			expect: 1,
+		},
+		{
+			name:   "higher build metadata",
+			v1:     "0.0.0-0+2",
+			v2:     "0.0.0-0+1",
+			expect: 1,
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			vr1, err1 := bundle.NewLegacyRegistryV1VersionRelease(tc.v1)
+			vr2, err2 := bundle.NewLegacyRegistryV1VersionRelease(tc.v2)
+			require.NoError(t, err1)
+			require.NoError(t, err2)
+
+			actual := vr1.Compare(*vr2)
+			assert.Equal(t, tc.expect, actual)
+		})
+	}
+}

internal/operator-controller/bundleutil/bundle.go

@@ -10,20 +10,26 @@ import (
 	"github.com/operator-framework/operator-registry/alpha/property"
 
 	ocv1 "github.com/operator-framework/operator-controller/api/v1"
+	"github.com/operator-framework/operator-controller/internal/operator-controller/bundle"
 )
 
-func GetVersion(b declcfg.Bundle) (*bsemver.Version, error) {
+func GetVersionAndRelease(b declcfg.Bundle) (*bundle.VersionRelease, error) {
 	for _, p := range b.Properties {
 		if p.Type == property.TypePackage {
 			var pkg property.Package
 			if err := json.Unmarshal(p.Value, &pkg); err != nil {
 				return nil, fmt.Errorf("error unmarshalling package property: %w", err)
 			}
-			vers, err := bsemver.Parse(pkg.Version)
+
+			// TODO: For now, we assume that all bundles are registry+v1 bundles.
+			//   In the future, when we support other bundle formats, we should stop
+			//   using the legacy mechanism (i.e. using build metadata in the version)
+			//   to determine the bundle's release.
+			vr, err := bundle.NewLegacyRegistryV1VersionRelease(pkg.Version)
 			if err != nil {
 				return nil, err
 			}
-			return &vers, nil
+			return vr, nil
 		}
 	}
 	return nil, fmt.Errorf("no package property found in bundle %q", b.Name)

internal/operator-controller/bundleutil/bundle_test.go

@@ -12,7 +12,7 @@ import (
 	"github.com/operator-framework/operator-controller/internal/operator-controller/bundleutil"
 )
 
-func TestGetVersion(t *testing.T) {
+func TestGetVersionAndRelease(t *testing.T) {
 	tests := []struct {
 		name        string
 		pkgProperty *property.Property
@@ -22,7 +22,7 @@ func TestGetVersion(t *testing.T) {
 			name: "valid version",
 			pkgProperty: &property.Property{
 				Type:  property.TypePackage,
-				Value: json.RawMessage(`{"version": "1.0.0"}`),
+				Value: json.RawMessage(`{"version": "1.0.0-pre+1.alpha.2"}`),
 			},
 			wantErr: false,
 		},
@@ -34,6 +34,14 @@ func TestGetVersion(t *testing.T) {
 			},
 			wantErr: true,
 		},
+		{
+			name: "invalid release",
+			pkgProperty: &property.Property{
+				Type:  property.TypePackage,
+				Value: json.RawMessage(`{"version": "1.0.0+001"}`),
+			},
+			wantErr: true,
+		},
 		{
 			name: "invalid json",
 			pkgProperty: &property.Property{
@@ -61,7 +69,7 @@ func TestGetVersion(t *testing.T) {
 				Properties: properties,
 			}
 
-			_, err := bundleutil.GetVersion(bundle)
+			_, err := bundleutil.GetVersionAndRelease(bundle)
 			if tc.wantErr {
 				require.Error(t, err)
 			} else {

internal/operator-controller/catalogmetadata/compare/compare.go

@@ -1,25 +1,71 @@
 package compare
 
 import (
+	"slices"
+	"strings"
+
+	mmsemver "github.com/Masterminds/semver/v3"
+	bsemver "github.com/blang/semver/v4"
 	"k8s.io/apimachinery/pkg/util/sets"
 
 	"github.com/operator-framework/operator-registry/alpha/declcfg"
 
 	"github.com/operator-framework/operator-controller/internal/operator-controller/bundleutil"
+	slicesutil "github.com/operator-framework/operator-controller/internal/shared/util/slices"
 )
 
-// ByVersion is a sort "less" function that orders bundles
-// in inverse version order (higher versions on top).
-func ByVersion(b1, b2 declcfg.Bundle) int {
-	v1, err1 := bundleutil.GetVersion(b1)
-	v2, err2 := bundleutil.GetVersion(b2)
-	if err1 != nil || err2 != nil {
-		return compareErrors(err1, err2)
+// NewVersionRange returns a function that tests whether a semver version is in the
+// provided versionRange. The versionRange provided to this function can be any valid semver
+// version string or any range that matches the syntax defined in the Masterminds semver library.
+//
+// When the provided version range is a valid semver version that includes build metadata, then the
+// returned function will only match an identical version with the same build metadata.
+//
+// When the provided version range is a valid semver version that does NOT include build metadata,
+// then the returned function will match any version that matches the semver version, ignoring the
+// build metadata of matched versions.
+//
+// Otherwise, Masterminds semver constraints semantics are used to match versions.
+// See https://github.com/Masterminds/semver#checking-version-constraints
+func NewVersionRange(versionRange string) (bsemver.Range, error) {
+	if versionPin, err := bsemver.Parse(versionRange); err == nil && len(versionPin.Build) > 0 {
+		return exactVersionMatcher(versionPin), nil
 	}
+	return newMastermindsRange(versionRange)
+}
 
-	// Check for "greater than" because
-	// we want higher versions on top
-	return v2.Compare(*v1)
+func exactVersionMatcher(pin bsemver.Version) bsemver.Range {
+	return func(v bsemver.Version) bool {
+		return pin.Compare(v) == 0 && slices.Compare(pin.Build, v.Build) == 0
+	}
+}
+
+func newMastermindsRange(versionRange string) (bsemver.Range, error) {
+	constraint, err := mmsemver.NewConstraint(versionRange)
+	if err != nil {
+		return nil, err
+	}
+	return func(in bsemver.Version) bool {
+		pre := slicesutil.Map(in.Pre, func(pr bsemver.PRVersion) string { return pr.String() })
+		mmVer := mmsemver.New(in.Major, in.Minor, in.Patch, strings.Join(pre, "."), strings.Join(in.Build, "."))
+		return constraint.Check(mmVer)
+	}, nil
+}
+
+// ByVersionAndRelease is a comparison function that compares bundles by
+// version and release. Bundles with lower versions/releases are
+// considered less than bundles with higher versions/releases.
+func ByVersionAndRelease(b1, b2 declcfg.Bundle) int {
+	vr1, err1 := bundleutil.GetVersionAndRelease(b1)
+	vr2, err2 := bundleutil.GetVersionAndRelease(b2)
+
+	// We don't really expect errors, because we expect well-formed/validated
+	// FBC as input. However, just in case we'll check the errors and sort
+	// invalid bundles as "lower" than valid bundles.
+	if err1 != nil || err2 != nil {
+		return compareErrors(err2, err1)
+	}
+	return vr2.Compare(*vr1)
 }
 
 func ByDeprecationFunc(deprecation declcfg.Deprecation) func(a, b declcfg.Bundle) int {
@@ -42,16 +88,16 @@ func ByDeprecationFunc(deprecation declcfg.Deprecation) func(a, b declcfg.Bundle
 	}
 }
 
-// compareErrors returns 0 if both errors are either nil or not nil
-// -1 if err1 is nil and err2 is not nil
-// +1 if err1 is not nil and err2 is nil
+// compareErrors returns 0 if both errors are either nil or not nil,
+// -1 if err1 is not nil and err2 is nil, and
+// +1 if err1 is nil and err2 is not nil
+// The semantic is that errors are "less than" non-errors.
 func compareErrors(err1 error, err2 error) int {
 	if err1 != nil && err2 == nil {
-		return 1
+		return -1
 	}
-
 	if err1 == nil && err2 != nil {
-		return -1
+		return 1
 	}
 	return 0
 }