From 6ffe8d7bbaa6f1a3713f9faabd4480264246cc01 Mon Sep 17 00:00:00 2001 From: Dmitrii Andreev Date: Thu, 2 Jul 2026 10:50:49 -0500 Subject: [PATCH] HYPERFLEET-1160 - feat: separate resource_labels table Replace JSONB labels column on resources with a dedicated resource_labels table (resource_id, key, value; composite PK, FK with ON DELETE CASCADE). - Add ResourceLabelDao with delete-all + bulk-insert ReplaceLabels - Wire label writes through ResourceService Create/Patch - Add Labels/Conditions preloads to all ResourceDao read methods - Add ExtractLabelQueries for EXISTS-subquery label filtering - Generalize extractMatchingQueries from condition-only to reusable - Restrict label filter operators to = and != (no lexicographic traps) - Remove validateLabelKey from new table path (key is parameterized) - Mark legacy JSONB label paths with TODO(HYPERFLEET-1159) - Enable ListByLabel integration test --- pkg/api/presenters/resource.go | 42 ++-- pkg/api/presenters/resource_test.go | 8 +- pkg/api/resource.go | 2 +- pkg/api/resource_label.go | 31 +++ pkg/dao/resource.go | 14 +- pkg/dao/resource_label.go | 49 +++++ .../202607010001_add_resource_labels.go | 29 +++ pkg/db/migrations/migration_structs.go | 1 + pkg/db/sql_helpers.go | 191 ++++++++++++++---- pkg/services/generic.go | 36 ++-- pkg/services/resource.go | 101 +++++---- pkg/services/resource_test.go | 21 +- plugins/resources/plugin.go | 1 + test/integration/channels_test.go | 21 +- test/integration/resource_delete_test.go | 49 +++++ test/integration/resource_helpers.go | 8 + test/integration/versions_test.go | 42 ++-- 17 files changed, 476 insertions(+), 170 deletions(-) create mode 100644 pkg/api/resource_label.go create mode 100644 pkg/dao/resource_label.go create mode 100644 pkg/db/migrations/202607010001_add_resource_labels.go diff --git a/pkg/api/presenters/resource.go b/pkg/api/presenters/resource.go index e5d28e6d..49ec63b0 100644 --- a/pkg/api/presenters/resource.go +++ b/pkg/api/presenters/resource.go @@ -4,8 +4,6 @@ import ( "encoding/json" "fmt" - "gorm.io/datatypes" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/util" @@ -19,16 +17,16 @@ func ConvertResource(req *openapi.ResourceCreateRequest) (*api.Resource, error) return nil, fmt.Errorf("failed to marshal spec: %w", err) } - labelsJSON, err := marshalLabels(req.Labels) - if err != nil { - return nil, fmt.Errorf("failed to marshal labels: %w", err) + labels, labelErr := convertLabelsToModel(req.Labels) + if labelErr != nil { + return nil, fmt.Errorf("invalid labels: %w", labelErr) } return &api.Resource{ Kind: req.Kind, Name: req.Name, Spec: specJSON, - Labels: labelsJSON, + Labels: labels, }, nil } @@ -54,7 +52,7 @@ func PresentResource(r *api.Resource) openapi.Resource { } } - labels := unmarshalLabels(r.Labels) + labels := presentLabels(r.Labels) resp := openapi.Resource{ Id: r.ID, @@ -127,27 +125,27 @@ func presentResourceConditions(conditions []api.ResourceCondition) []openapi.Res return result } -func marshalLabels(labels *map[string]string) (datatypes.JSON, error) { - if labels == nil { - return datatypes.JSON("{}"), nil +func convertLabelsToModel(labels *map[string]string) ([]api.ResourceLabel, error) { + if labels == nil || len(*labels) == 0 { + return nil, nil } - b, err := json.Marshal(*labels) - if err != nil { - return nil, err + result := make([]api.ResourceLabel, 0, len(*labels)) + for k, v := range *labels { + if err := api.ValidateLabel(k, v); err != nil { + return nil, err + } + result = append(result, api.ResourceLabel{Key: k, Value: v}) } - return datatypes.JSON(b), nil + return result, nil } -func unmarshalLabels(raw datatypes.JSON) *map[string]string { - if len(raw) == 0 { - return nil - } - var m map[string]string - if err := json.Unmarshal(raw, &m); err != nil { +func presentLabels(labels []api.ResourceLabel) *map[string]string { + if len(labels) == 0 { return nil } - if len(m) == 0 { - return nil + m := make(map[string]string, len(labels)) + for _, l := range labels { + m[l.Key] = l.Value } return &m } diff --git a/pkg/api/presenters/resource_test.go b/pkg/api/presenters/resource_test.go index b52b7215..738d02bb 100644 --- a/pkg/api/presenters/resource_test.go +++ b/pkg/api/presenters/resource_test.go @@ -28,7 +28,9 @@ func TestConvertResource(t *testing.T) { Expect(resource.Kind).To(Equal("Channel")) Expect(resource.Name).To(Equal("stable")) Expect(resource.Spec).NotTo(BeEmpty()) - Expect(string(resource.Labels)).To(ContainSubstring("env")) + Expect(resource.Labels).To(HaveLen(1)) + Expect(resource.Labels[0].Key).To(Equal("env")) + Expect(resource.Labels[0].Value).To(Equal("prod")) } func TestConvertResource_NilLabels(t *testing.T) { @@ -42,7 +44,7 @@ func TestConvertResource_NilLabels(t *testing.T) { resource, err := ConvertResource(req) Expect(err).NotTo(HaveOccurred()) - Expect(string(resource.Labels)).To(Equal("{}")) + Expect(resource.Labels).To(BeEmpty()) } func TestConvertResourceWithOwner(t *testing.T) { @@ -74,7 +76,7 @@ func TestPresentResource(t *testing.T) { Name: "stable", Href: "/api/hyperfleet/v1/channels/test-id", Spec: datatypes.JSON(`{"is_default":true}`), - Labels: datatypes.JSON(`{"env":"prod"}`), + Labels: []api.ResourceLabel{{Key: "env", Value: "prod"}}, Generation: 1, CreatedBy: "user@test.com", UpdatedBy: "user@test.com", diff --git a/pkg/api/resource.go b/pkg/api/resource.go index 75c42da7..fdaee95e 100644 --- a/pkg/api/resource.go +++ b/pkg/api/resource.go @@ -27,7 +27,7 @@ type Resource struct { Href string `json:"href,omitempty" gorm:"size:500"` CreatedBy string `json:"created_by" gorm:"size:255;not null"` UpdatedBy string `json:"updated_by" gorm:"size:255;not null"` - Labels datatypes.JSON `json:"labels,omitempty" gorm:"type:jsonb"` + Labels []ResourceLabel `json:"-" gorm:"foreignKey:ResourceID;references:ID"` Spec datatypes.JSON `json:"spec" gorm:"type:jsonb;not null"` Conditions []ResourceCondition `json:"-" gorm:"foreignKey:ResourceID;references:ID"` Generation int32 `json:"generation" gorm:"default:1;not null"` diff --git a/pkg/api/resource_label.go b/pkg/api/resource_label.go new file mode 100644 index 00000000..90e299af --- /dev/null +++ b/pkg/api/resource_label.go @@ -0,0 +1,31 @@ +package api + +import "fmt" + +const ( + MaxLabelKeyLen = 255 + MaxLabelValueLen = 255 +) + +type ResourceLabel struct { + ResourceID string `json:"-" gorm:"primaryKey;size:255;not null"` + Key string `json:"key" gorm:"primaryKey;size:255;not null"` + Value string `json:"value" gorm:"size:255;not null"` +} + +func (ResourceLabel) TableName() string { + return "resource_labels" +} + +func ValidateLabel(key, value string) error { + if key == "" { + return fmt.Errorf("label key cannot be empty") + } + if len(key) > MaxLabelKeyLen { + return fmt.Errorf("label key %q exceeds maximum length of %d", key, MaxLabelKeyLen) + } + if len(value) > MaxLabelValueLen { + return fmt.Errorf("label value for key %q exceeds maximum length of %d", key, MaxLabelValueLen) + } + return nil +} diff --git a/pkg/dao/resource.go b/pkg/dao/resource.go index 213a25a7..cd9b54d7 100644 --- a/pkg/dao/resource.go +++ b/pkg/dao/resource.go @@ -37,7 +37,8 @@ func NewResourceDao(sessionFactory db.SessionFactory) ResourceDao { func (d *sqlResourceDao) Get(ctx context.Context, kind, id string) (*api.Resource, error) { g2 := d.sessionFactory.New(ctx) var resource api.Resource - if err := g2.Preload("Conditions").Take(&resource, "kind = ? AND id = ?", kind, id).Error; err != nil { + if err := g2.Preload("Conditions").Preload("Labels"). + Take(&resource, "kind = ? AND id = ?", kind, id).Error; err != nil { return nil, err } return &resource, nil @@ -46,7 +47,7 @@ func (d *sqlResourceDao) Get(ctx context.Context, kind, id string) (*api.Resourc func (d *sqlResourceDao) GetForUpdate(ctx context.Context, kind, id string) (*api.Resource, error) { g2 := d.sessionFactory.New(ctx) var resource api.Resource - if err := g2.Clauses(clause.Locking{Strength: "UPDATE"}).Take( + if err := g2.Preload("Labels").Preload("Conditions").Clauses(clause.Locking{Strength: "UPDATE"}).Take( &resource, "kind = ? AND id = ?", kind, id).Error; err != nil { return nil, err } @@ -56,7 +57,7 @@ func (d *sqlResourceDao) GetForUpdate(ctx context.Context, kind, id string) (*ap func (d *sqlResourceDao) GetByOwner(ctx context.Context, kind, id, ownerID string) (*api.Resource, error) { g2 := d.sessionFactory.New(ctx) var resource api.Resource - if err := g2.Preload("Conditions"). + if err := g2.Preload("Conditions").Preload("Labels"). Take(&resource, "kind = ? AND id = ? AND owner_id = ?", kind, id, ownerID).Error; err != nil { return nil, err } @@ -129,7 +130,7 @@ func (d *sqlResourceDao) ExistsSoftDeletedByOwner(ctx context.Context, kinds []s func (d *sqlResourceDao) FindByKind(ctx context.Context, kind string) (api.ResourceList, error) { g2 := d.sessionFactory.New(ctx) var resources api.ResourceList - if err := g2.Where("kind = ?", kind).Find(&resources).Error; err != nil { + if err := g2.Preload("Labels").Preload("Conditions").Where("kind = ?", kind).Find(&resources).Error; err != nil { return nil, err } return resources, nil @@ -138,7 +139,8 @@ func (d *sqlResourceDao) FindByKind(ctx context.Context, kind string) (api.Resou func (d *sqlResourceDao) FindByKindAndOwner(ctx context.Context, kind, ownerID string) (api.ResourceList, error) { g2 := d.sessionFactory.New(ctx) var resources api.ResourceList - if err := g2.Where("kind = ? AND owner_id = ?", kind, ownerID).Find(&resources).Error; err != nil { + if err := g2.Preload("Labels").Preload("Conditions"). + Where("kind = ? AND owner_id = ?", kind, ownerID).Find(&resources).Error; err != nil { return nil, err } return resources, nil @@ -149,7 +151,7 @@ func (d *sqlResourceDao) FindByKindAndOwnerForUpdate( ) (api.ResourceList, error) { g2 := d.sessionFactory.New(ctx) var resources api.ResourceList - if err := g2.Clauses(clause.Locking{Strength: "UPDATE"}). + if err := g2.Preload("Labels").Preload("Conditions").Clauses(clause.Locking{Strength: "UPDATE"}). Where("kind = ? AND owner_id = ?", kind, ownerID).Find(&resources).Error; err != nil { return nil, err } diff --git a/pkg/dao/resource_label.go b/pkg/dao/resource_label.go new file mode 100644 index 00000000..46b75fb7 --- /dev/null +++ b/pkg/dao/resource_label.go @@ -0,0 +1,49 @@ +package dao + +import ( + "context" + + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db" +) + +//go:generate mockgen-v0.6.0 -source=resource_label.go -package=dao -destination=resource_label_mock.go + +type ResourceLabelDao interface { + ReplaceLabels(ctx context.Context, resourceID string, labels []api.ResourceLabel) error +} + +var _ ResourceLabelDao = &sqlResourceLabelDao{} + +type sqlResourceLabelDao struct { + sessionFactory db.SessionFactory +} + +func NewResourceLabelDao(sessionFactory db.SessionFactory) ResourceLabelDao { + return &sqlResourceLabelDao{sessionFactory: sessionFactory} +} + +func (d *sqlResourceLabelDao) ReplaceLabels( + ctx context.Context, resourceID string, labels []api.ResourceLabel, +) error { + g2 := d.sessionFactory.New(ctx) + + if err := g2.Where("resource_id = ?", resourceID).Delete(&api.ResourceLabel{}).Error; err != nil { + db.MarkForRollback(ctx, err) + return err + } + + if len(labels) > 0 { + rows := make([]api.ResourceLabel, len(labels)) + copy(rows, labels) + for i := range rows { + rows[i].ResourceID = resourceID + } + if err := g2.Create(&rows).Error; err != nil { + db.MarkForRollback(ctx, err) + return err + } + } + + return nil +} diff --git a/pkg/db/migrations/202607010001_add_resource_labels.go b/pkg/db/migrations/202607010001_add_resource_labels.go new file mode 100644 index 00000000..79b39831 --- /dev/null +++ b/pkg/db/migrations/202607010001_add_resource_labels.go @@ -0,0 +1,29 @@ +package migrations + +import ( + "github.com/go-gormigrate/gormigrate/v2" + "gorm.io/gorm" +) + +func addResourceLabels() *gormigrate.Migration { + return &gormigrate.Migration{ + ID: "202607010001", + Migrate: func(tx *gorm.DB) error { + if err := tx.Exec(`CREATE TABLE IF NOT EXISTS resource_labels ( + resource_id VARCHAR(255) NOT NULL, + key VARCHAR(255) NOT NULL, + value VARCHAR(255) NOT NULL, + PRIMARY KEY (resource_id, key), + FOREIGN KEY (resource_id) REFERENCES resources(id) ON DELETE CASCADE + );`).Error; err != nil { + return err + } + + if err := tx.Exec(`ALTER TABLE resources DROP COLUMN IF EXISTS labels;`).Error; err != nil { + return err + } + + return nil + }, + } +} diff --git a/pkg/db/migrations/migration_structs.go b/pkg/db/migrations/migration_structs.go index 1ae15820..85109f97 100755 --- a/pkg/db/migrations/migration_structs.go +++ b/pkg/db/migrations/migration_structs.go @@ -38,6 +38,7 @@ var MigrationList = []*gormigrate.Migration{ addResources(), removeReadyCondition(), addResourceConditions(), + addResourceLabels(), } // Model represents the base model struct. All entities will have this struct embedded. diff --git a/pkg/db/sql_helpers.go b/pkg/db/sql_helpers.go index 2c52ef6e..02c885d2 100755 --- a/pkg/db/sql_helpers.go +++ b/pkg/db/sql_helpers.go @@ -16,18 +16,15 @@ import ( "gorm.io/gorm" ) -// Label key validation pattern: only lowercase letters, digits, and underscores to prevent SQL injection -var labelKeyPattern = regexp.MustCompile(`^[a-z0-9_]+$`) +// jsonbKeyPattern guards keys interpolated into JSONB paths (spec->>'%s', properties->>'%s'). +var jsonbKeyPattern = regexp.MustCompile(`^[a-z0-9_]+$`) -// validateFieldKey validates a JSONB field key to prevent SQL injection through field -// name interpolation. Only allows lowercase letters, digits, and underscores. -// fieldType is used in error messages (e.g. "label key", "spec field segment"). -func validateFieldKey(key, fieldType string) *errors.ServiceError { +func validateJsonbKey(key, fieldType string) *errors.ServiceError { if key == "" { return errors.BadRequest("%s cannot be empty", fieldType) } - if !labelKeyPattern.MatchString(key) { + if !jsonbKeyPattern.MatchString(key) { return errors.BadRequest( "%s '%s' is invalid: must contain only lowercase letters, digits, and underscores", fieldType, key, ) @@ -36,8 +33,9 @@ func validateFieldKey(key, fieldType string) *errors.ServiceError { return nil } -func validateLabelKey(key string) *errors.ServiceError { - return validateFieldKey(key, "label key") +// TODO(HYPERFLEET-1159): remove once Cluster/NodePool are migrated to Resource-kind entities. +func validateLegacyLabelKey(key string) *errors.ServiceError { + return validateJsonbKey(key, "label key") } // Field mapping rules for user-friendly syntax to database columns @@ -55,7 +53,7 @@ func getField(name string, disallowedFields map[string]string) (field string, er return } key := strings.TrimPrefix(trimmedName, "properties.") - if validationErr := validateFieldKey(key, "property key"); validationErr != nil { + if validationErr := validateJsonbKey(key, "property key"); validationErr != nil { err = validationErr return } @@ -76,7 +74,7 @@ func getField(name string, disallowedFields map[string]string) (field string, er parts := strings.Split(strings.TrimPrefix(trimmedName, "spec."), ".") for _, part := range parts { - if validationErr := validateFieldKey(part, "spec field segment"); validationErr != nil { + if validationErr := validateJsonbKey(part, "spec field segment"); validationErr != nil { err = validationErr return } @@ -93,11 +91,16 @@ func getField(name string, disallowedFields map[string]string) (field string, er return } - // Map user-friendly labels.xxx syntax to JSONB query: labels->>'xxx' + // TODO(HYPERFLEET-1159): remove this legacy JSONB branch once Cluster/NodePool + // are migrated to Resource-kind entities with the resource_labels table. if strings.HasPrefix(trimmedName, "labels.") { + if _, disallowed := disallowedFields["labels"]; disallowed { + err = errors.BadRequest("%s is not a valid field name", name) + return + } key := strings.TrimPrefix(trimmedName, "labels.") - if validationErr := validateLabelKey(key); validationErr != nil { + if validationErr := validateLegacyLabelKey(key); validationErr != nil { err = validationErr return } @@ -339,66 +342,74 @@ func conditionSubfieldConverter(n *tsl.Node, conditionType, subfield string) (in // returning the modified tree (with condition nodes replaced) and the extracted conditions. func ExtractConditionQueries(n *tsl.Node) (*tsl.Node, []sq.Sqlizer, *errors.ServiceError) { var conditions []sq.Sqlizer - modifiedTree, err := extractConditionsWalk(n, &conditions) + modifiedTree, err := extractMatchingQueries( + n, hasCondition, conditionsNodeConverter, + "NOT operator is not supported with condition queries; "+ + "use the inverse condition instead (e.g., Reconciled='False')", + "OR operator is not supported with condition queries (status.conditions.*); "+ + "use separate requests or combine conditions with AND", + &conditions, + ) return modifiedTree, conditions, err } -// subtreeHasCondition returns true if any node in the subtree is a condition query -func subtreeHasCondition(n *tsl.Node) bool { +// subtreeHasMatch returns true if any node in the subtree satisfies predicate. +func subtreeHasMatch(n *tsl.Node, predicate func(*tsl.Node) bool) bool { if n == nil { return false } - if hasCondition(n) { + if predicate(n) { return true } - if subtreeHasCondition(n.Left) { + if subtreeHasMatch(n.Left, predicate) { return true } - if subtreeHasCondition(n.Right) { + if subtreeHasMatch(n.Right, predicate) { return true } - return slices.ContainsFunc(n.Children, subtreeHasCondition) + return slices.ContainsFunc(n.Children, func(c *tsl.Node) bool { return subtreeHasMatch(c, predicate) }) } -// extractConditionsWalk recursively walks the tree and extracts condition queries -func extractConditionsWalk(n *tsl.Node, conditions *[]sq.Sqlizer) (*tsl.Node, *errors.ServiceError) { +// extractMatchingQueries recursively walks the tree, replacing every node matching +// predicate with a `1=1` placeholder and collecting converter(node) into exprs. +// NOT and OR are rejected over a subtree containing a match: extracting a match from +// an OR/NOT branch and replacing it with 1=1 changes the query semantics (e.g. +// "name='x' OR condition" becomes "(name='x' OR 1=1) AND condition", which collapses +// the OR to always-true). notMsg/orMsg are the operator-specific error messages. +func extractMatchingQueries( + n *tsl.Node, + predicate func(*tsl.Node) bool, + converter func(*tsl.Node) (any, *errors.ServiceError), + notMsg, orMsg string, + exprs *[]sq.Sqlizer, +) (*tsl.Node, *errors.ServiceError) { if n == nil { return nil, nil } // NOT is unary in v6: child is in Right, Left is nil if n.Kind == tsl.KindUnaryExpr && n.Operator == tsl.OpNot { - if subtreeHasCondition(n.Right) { - return n, errors.BadRequest( - "NOT operator is not supported with condition queries; " + - "use the inverse condition instead (e.g., Reconciled='False')", - ) + if subtreeHasMatch(n.Right, predicate) { + return n, errors.BadRequest("%s", notMsg) } } - // OR with condition queries is not supported: extracting conditions from - // an OR branch and replacing them with 1=1 changes the query semantics - // (e.g. "name='x' OR condition" becomes "(name='x' OR 1=1) AND condition", - // which collapses the OR to always-true). if n.Kind == tsl.KindBinaryExpr && n.Operator == tsl.OpOr { - if subtreeHasCondition(n.Left) || subtreeHasCondition(n.Right) { - return n, errors.BadRequest( - "OR operator is not supported with condition queries (status.conditions.*); " + - "use separate requests or combine conditions with AND", - ) + if subtreeHasMatch(n.Left, predicate) || subtreeHasMatch(n.Right, predicate) { + return n, errors.BadRequest("%s", orMsg) } } - if hasCondition(n) { - expr, err := conditionsNodeConverter(n) + if predicate(n) { + expr, err := converter(n) if err != nil { return n, err } sqlizer, ok := expr.(sq.Sqlizer) if !ok { - return n, errors.GeneralError("unexpected type %T in condition expression", expr) + return n, errors.GeneralError("unexpected type %T in extracted expression", expr) } - *conditions = append(*conditions, sqlizer) + *exprs = append(*exprs, sqlizer) // Replace with a placeholder that always evaluates to true return &tsl.Node{ @@ -409,12 +420,12 @@ func extractConditionsWalk(n *tsl.Node, conditions *[]sq.Sqlizer) (*tsl.Node, *e }, nil } - // For non-condition nodes, recursively process children + // For non-matching nodes, recursively process children var newLeft, newRight *tsl.Node if n.Left != nil { var err *errors.ServiceError - newLeft, err = extractConditionsWalk(n.Left, conditions) + newLeft, err = extractMatchingQueries(n.Left, predicate, converter, notMsg, orMsg, exprs) if err != nil { return n, err } @@ -422,7 +433,7 @@ func extractConditionsWalk(n *tsl.Node, conditions *[]sq.Sqlizer) (*tsl.Node, *e if n.Right != nil { var err *errors.ServiceError - newRight, err = extractConditionsWalk(n.Right, conditions) + newRight, err = extractMatchingQueries(n.Right, predicate, converter, notMsg, orMsg, exprs) if err != nil { return n, err } @@ -430,7 +441,7 @@ func extractConditionsWalk(n *tsl.Node, conditions *[]sq.Sqlizer) (*tsl.Node, *e var newChildren []*tsl.Node for _, child := range n.Children { - newChild, childErr := extractConditionsWalk(child, conditions) + newChild, childErr := extractMatchingQueries(child, predicate, converter, notMsg, orMsg, exprs) if childErr != nil { return n, childErr } @@ -448,6 +459,98 @@ func extractConditionsWalk(n *tsl.Node, conditions *[]sq.Sqlizer) (*tsl.Node, *e }, nil } +// hasLabel returns true if node has a labels. identifier on the left hand side. +func hasLabel(n *tsl.Node) bool { + if n.Left == nil || n.Left.Kind != tsl.KindIdentifier { + return false + } + leftStr, ok := n.Left.Value.(string) + if !ok { + return false + } + return strings.HasPrefix(leftStr, "labels.") +} + +func labelsNodeConverter(n *tsl.Node, resourceTable string) (any, *errors.ServiceError) { + if n.Left == nil || n.Left.Kind != tsl.KindIdentifier { + return nil, errors.BadRequest("invalid label query structure") + } + + leftStr, ok := n.Left.Value.(string) + if !ok { + return nil, errors.BadRequest("expected string for left side of label query") + } + + parts := strings.Split(leftStr, ".") + if len(parts) != 2 || parts[0] != "labels" { + return nil, errors.BadRequest("invalid label field path: %s", leftStr) + } + + key := parts[1] + + if n.Operator != tsl.OpEQ && n.Operator != tsl.OpNE { + return nil, errors.BadRequest( + "operator '%s' is not supported for label queries; use = or !=", n.Operator, + ) + } + sqlOp := comparisonOperators[n.Operator] + + if n.Right == nil || n.Right.Kind != tsl.KindStringLiteral { + return nil, errors.BadRequest("expected string value for label '%s'", key) + } + rightStr, ok := n.Right.Value.(string) + if !ok { + return nil, errors.BadRequest("expected string value for label '%s'", key) + } + + query := fmt.Sprintf( + "EXISTS (SELECT 1 FROM resource_labels WHERE resource_labels.resource_id = %s.id "+ + "AND resource_labels.key = ? AND resource_labels.value %s ?)", + resourceTable, sqlOp, + ) + return sq.Expr(query, key, rightStr), nil +} + +// isLabelIdentifier returns true for a bare labels. identifier node, regardless +// of its position in the tree — unlike hasLabel, which only matches when the +// identifier is the direct left side of a comparison. +func isLabelIdentifier(n *tsl.Node) bool { + if n == nil || n.Kind != tsl.KindIdentifier { + return false + } + s, ok := n.Value.(string) + return ok && strings.HasPrefix(s, "labels.") +} + +func ExtractLabelQueries(n *tsl.Node, resourceTable string) (*tsl.Node, []sq.Sqlizer, *errors.ServiceError) { + var labels []sq.Sqlizer + converter := func(n *tsl.Node) (any, *errors.ServiceError) { + return labelsNodeConverter(n, resourceTable) + } + modifiedTree, err := extractMatchingQueries( + n, hasLabel, converter, + "NOT operator is not supported with label queries", + "OR operator is not supported with label queries (labels.*); "+ + "use separate requests or combine label filters with AND", + &labels, + ) + if err != nil { + return n, nil, err + } + + // "not labels.env='x'" parses as "(NOT labels.env) = 'x'" due to TSL precedence, + // so hasLabel never matches it and it survives into modifiedTree. Without this + // guard it falls through to getField's JSONB branch and hits a missing column. + if subtreeHasMatch(modifiedTree, isLabelIdentifier) { + return n, nil, errors.BadRequest( + "labels. must be used in a direct comparison, e.g. labels.env=\"prod\"; " + + "wrap NOT in parentheses, e.g. not (labels.env=\"prod\")", + ) + } + + return modifiedTree, labels, nil +} + // FieldNameWalk walks the filter tree, maps user-facing field names to SQL columns // via getField, then wraps spec JSONB numeric comparisons in CAST. func FieldNameWalk( diff --git a/pkg/services/generic.go b/pkg/services/generic.go index 25db050a..7f47466e 100755 --- a/pkg/services/generic.go +++ b/pkg/services/generic.go @@ -180,6 +180,17 @@ func (s *sqlGenericService) buildSearchValues( return "", nil, serviceErr } + // Extract label queries (labels.xxx) for Resource-kind entities before field name + // mapping — they're backed by a separate resource_labels table, not a JSONB column, + // so they need an EXISTS subquery rather than a simple field substitution. + var labelExprs []squirrel.Sqlizer + if listCtx.resourceType == "Resource" { + tslTree, labelExprs, serviceErr = db.ExtractLabelQueries(tslTree, (*d).GetTableName()) + if serviceErr != nil { + return "", nil, serviceErr + } + } + // apply field name mapping (status.xxx -> status_xxx, labels.xxx -> labels->>'xxx', spec.xxx -> spec->>'xxx') // also wraps spec JSONB fields in CAST(... AS numeric) when compared against a number tslTree, serviceErr = db.FieldNameWalk(tslTree, *listCtx.disallowedFields) @@ -210,20 +221,19 @@ func (s *sqlGenericService) buildSearchValues( return "", nil, errors.GeneralError("%s", err.Error()) } - // Combine the base SQL with condition expressions - if len(conditionExprs) > 0 { - for _, condExpr := range conditionExprs { - condSQL, condValues, err := condExpr.ToSql() - if err != nil { - return "", nil, errors.GeneralError("%s", err.Error()) - } - if sql == "" { - sql = condSQL - } else { - sql = fmt.Sprintf("(%s) AND (%s)", sql, condSQL) - } - values = append(values, condValues...) + // Combine the base SQL with extracted condition and label expressions + extractedExprs := append(append([]squirrel.Sqlizer{}, conditionExprs...), labelExprs...) + for _, expr := range extractedExprs { + exprSQL, exprValues, err := expr.ToSql() + if err != nil { + return "", nil, errors.GeneralError("%s", err.Error()) + } + if sql == "" { + sql = exprSQL + } else { + sql = fmt.Sprintf("(%s) AND (%s)", sql, exprSQL) } + values = append(values, exprValues...) } return sql, values, nil diff --git a/pkg/services/resource.go b/pkg/services/resource.go index e44794ee..20a7dce1 100644 --- a/pkg/services/resource.go +++ b/pkg/services/resource.go @@ -26,15 +26,24 @@ type ResourceService interface { ListByOwner(ctx context.Context, kind, ownerID string, args *ListArguments) (api.ResourceList, *api.PagingMeta, *errors.ServiceError) // nolint:lll } -func NewResourceService(resourceDao dao.ResourceDao, generic GenericService) ResourceService { - return &sqlResourceService{resourceDao: resourceDao, generic: generic} +func NewResourceService( + resourceDao dao.ResourceDao, + resourceLabelDao dao.ResourceLabelDao, + generic GenericService, +) ResourceService { + return &sqlResourceService{ + resourceDao: resourceDao, + resourceLabelDao: resourceLabelDao, + generic: generic, + } } var _ ResourceService = &sqlResourceService{} type sqlResourceService struct { - resourceDao dao.ResourceDao - generic GenericService + resourceDao dao.ResourceDao + resourceLabelDao dao.ResourceLabelDao + generic GenericService } // Get returns a single resource by kind and ID. Returns 404 if not found. @@ -49,9 +58,6 @@ func (s *sqlResourceService) Get(ctx context.Context, kind, id string) (*api.Res return resource, nil } -// Create validates name constraints from the EntityDescriptor, sets CreatedBy/UpdatedBy -// from the auth context, and persists a new resource. ID generation, timestamps, href -// computation, and generation initialisation are handled by the GORM BeforeCreate hook. func (s *sqlResourceService) Create( ctx context.Context, kind string, resource *api.Resource, ) (*api.Resource, *errors.ServiceError) { @@ -85,12 +91,16 @@ func (s *sqlResourceService) Create( if err != nil { return nil, handleCreateError(kind, err) } + + if len(resource.Labels) > 0 { + if labelErr := s.resourceLabelDao.ReplaceLabels(ctx, resource.ID, resource.Labels); labelErr != nil { + return nil, handleCreateError(kind, labelErr) + } + } + return resource, nil } -// Patch applies spec/label changes to a resource. Acquires a row-level lock via GetForUpdate -// to prevent concurrent modifications. Increments generation only when spec or labels actually -// change (compared via deep JSON equality). Rejects patches on soft-deleted resources with 409. func (s *sqlResourceService) Patch( ctx context.Context, kind, id string, patch *api.ResourcePatch, ) (*api.Resource, *errors.ServiceError) { @@ -102,37 +112,41 @@ func (s *sqlResourceService) Patch( return nil, handleGetError(kind, "id", id, err) } - // Check if resource is marked for deletion if resource.DeletedTime != nil { return nil, errors.ConflictState("%s '%s' is marked for deletion", kind, id) } - // Snapshot current values before applying the patch. Defensive copy required because - // applyResourcePatch replaces the slice reference, and we need the originals for comparison. oldSpec := append([]byte(nil), resource.Spec...) - oldLabels := append([]byte(nil), resource.Labels...) + oldLabels := resource.Labels if applyErr := applyResourcePatch(resource, patch); applyErr != nil { return nil, errors.Validation("Invalid patch data: %v", applyErr) } - if jsonBytesEqual(oldSpec, resource.Spec) && jsonBytesEqual(oldLabels, resource.Labels) { + specChanged := !jsonBytesEqual(oldSpec, resource.Spec) + labelsChanged := !labelsEqual(oldLabels, resource.Labels) + + if !specChanged && !labelsChanged { return resource, nil } resource.IncrementGeneration() - resource.UpdatedBy = actorFromContext(ctx) if saveErr := s.resourceDao.Save(ctx, resource); saveErr != nil { return nil, handleUpdateError(kind, saveErr) } + if labelsChanged { + if labelErr := s.resourceLabelDao.ReplaceLabels(ctx, resource.ID, resource.Labels); labelErr != nil { + return nil, handleUpdateError(kind, labelErr) + } + } + return resource, nil } -// Delete removes a resource and its cascade subtree. Resources with required -// adapters are soft-deleted; all others are hard-deleted. +// Resources with required adapters are soft-deleted; all others are hard-deleted. func (s *sqlResourceService) Delete(ctx context.Context, kind, id string) (*api.Resource, *errors.ServiceError) { if svcErr := validateKind(kind); svcErr != nil { return nil, svcErr @@ -264,8 +278,7 @@ func (s *sqlResourceService) checkCanDelete( return nil } -// GetByOwner returns a single child resource, validated as belonging to the specified owner. -// Returns 404 if the resource doesn't exist or belongs to a different owner. +// GetByOwner returns a single child resource scoped to the specified owner. Returns 404 if not found. func (s *sqlResourceService) GetByOwner( ctx context.Context, kind, id, ownerID string, ) (*api.Resource, *errors.ServiceError) { @@ -290,6 +303,7 @@ func (s *sqlResourceService) List( args = &ListArguments{Page: 1, Size: 20} } scopedArgs := *args + scopedArgs.Preloads = append(append([]string(nil), scopedArgs.Preloads...), "Labels", "Conditions") kindFilter := fmt.Sprintf("kind = '%s'", kind) if scopedArgs.Search == "" { scopedArgs.Search = kindFilter @@ -306,8 +320,6 @@ func (s *sqlResourceService) List( } // ListByOwner returns child resources of the given owner with pagination, search, and ordering. -// Injects kind and owner_id filters into the TSL search string before delegating to GenericService.List. -// A shallow copy of args is made to avoid mutating the caller's ListArguments. func (s *sqlResourceService) ListByOwner( ctx context.Context, kind, ownerID string, args *ListArguments, ) (api.ResourceList, *api.PagingMeta, *errors.ServiceError) { @@ -318,6 +330,7 @@ func (s *sqlResourceService) ListByOwner( args = &ListArguments{Page: 1, Size: 20} } scopedArgs := *args + scopedArgs.Preloads = append(append([]string(nil), scopedArgs.Preloads...), "Labels", "Conditions") kindFilter := fmt.Sprintf("kind = '%s' AND owner_id = '%s'", kind, ownerID) if scopedArgs.Search == "" { scopedArgs.Search = kindFilter @@ -338,8 +351,6 @@ func (s *sqlResourceService) ListByOwner( return result, paging, nil } -// validateKind checks that the kind is a registered entity type. -// Returns 400 if the kind is unknown, preventing invalid kinds from reaching the DAO. func validateKind(kind string) *errors.ServiceError { if _, ok := registry.Get(kind); !ok { return errors.Validation("Unknown entity kind: %s", kind) @@ -347,8 +358,7 @@ func validateKind(kind string) *errors.ServiceError { return nil } -// validateResourceName checks that the kind is registered and the name is non-empty. -// Name format and length validation is handled by OpenAPI spec validation middleware. +// Name format/length validation is handled by OpenAPI spec validation middleware. func validateResourceName(kind, name string) *errors.ServiceError { if svcErr := validateKind(kind); svcErr != nil { return svcErr @@ -359,10 +369,7 @@ func validateResourceName(kind, name string) *errors.ServiceError { return nil } -// jsonBytesEqual is a nil-safe wrapper around jsonEqual. Returns true if both slices are -// nil/empty, false if only one is, and delegates to jsonEqual for semantic JSON comparison. -// Needed because Resource.Labels is nullable (JSONB NULL), and jsonEqual(nil, nil) returns -// false due to json.Unmarshal(nil) error. +// jsonBytesEqual is a nil-safe wrapper around jsonEqual for comparing JSONB spec fields. func jsonBytesEqual(a, b []byte) bool { if len(a) == 0 && len(b) == 0 { return true @@ -373,7 +380,26 @@ func jsonBytesEqual(a, b []byte) bool { return jsonEqual(a, b) } -// applyResourcePatch merges non-nil patch fields into the resource by marshaling them to JSON. +// labelsEqual compares two label slices by key-value content (order-independent). +func labelsEqual(a, b []api.ResourceLabel) bool { + if len(a) == 0 && len(b) == 0 { + return true + } + if len(a) != len(b) { + return false + } + m := make(map[string]string, len(a)) + for _, l := range a { + m[l.Key] = l.Value + } + for _, l := range b { + if v, ok := m[l.Key]; !ok || v != l.Value { + return false + } + } + return true +} + func applyResourcePatch(resource *api.Resource, patch *api.ResourcePatch) error { if patch.Spec != nil { specJSON, err := json.Marshal(patch.Spec) @@ -383,13 +409,16 @@ func applyResourcePatch(resource *api.Resource, patch *api.ResourcePatch) error resource.Spec = specJSON } if patch.Labels != nil { - labelsJSON, err := json.Marshal(patch.Labels) - if err != nil { - return fmt.Errorf("failed to marshal resource labels: %w", err) + labels := make([]api.ResourceLabel, 0, len(patch.Labels)) + for k, v := range patch.Labels { + if err := api.ValidateLabel(k, v); err != nil { + return err + } + labels = append(labels, api.ResourceLabel{Key: k, Value: v}) } - resource.Labels = labelsJSON + resource.Labels = labels } - // TODO: handle patch.References — three-way semantics (nil=skip, {}=clear, map=replace) + // TODO(HYPERFLEET-1156): handle patch.References — three-way semantics (nil=skip, {}=clear, map=replace) // via dao.ReplaceReferences per generic-resource-registry-design.md §9.2 return nil } diff --git a/pkg/services/resource_test.go b/pkg/services/resource_test.go index 31d0cbd1..331b3229 100644 --- a/pkg/services/resource_test.go +++ b/pkg/services/resource_test.go @@ -171,6 +171,25 @@ func (d *mockResourceDao) addResource(r *api.Resource) { var _ dao.ResourceDao = &mockResourceDao{} +type mockResourceLabelDao struct { + labels map[string][]api.ResourceLabel + replaceErr error +} + +func newMockResourceLabelDao() *mockResourceLabelDao { + return &mockResourceLabelDao{labels: make(map[string][]api.ResourceLabel)} +} + +func (d *mockResourceLabelDao) ReplaceLabels(_ context.Context, resourceID string, labels []api.ResourceLabel) error { + if d.replaceErr != nil { + return d.replaceErr + } + d.labels[resourceID] = labels + return nil +} + +var _ dao.ResourceLabelDao = &mockResourceLabelDao{} + type resourceGenericMock struct { listErr *errors.ServiceError lastSearch string @@ -192,7 +211,7 @@ var _ GenericService = &resourceGenericMock{} func newTestResourceService(mockDao *mockResourceDao) (ResourceService, *mockResourceDao, *resourceGenericMock) { generic := &resourceGenericMock{} - svc := NewResourceService(mockDao, generic) + svc := NewResourceService(mockDao, newMockResourceLabelDao(), generic) return svc, mockDao, generic } diff --git a/plugins/resources/plugin.go b/plugins/resources/plugin.go index 1510f9f9..afb2702a 100644 --- a/plugins/resources/plugin.go +++ b/plugins/resources/plugin.go @@ -15,6 +15,7 @@ func NewServiceLocator(env *environments.Env) ServiceLocator { return func() services.ResourceService { return services.NewResourceService( dao.NewResourceDao(env.Database.SessionFactory), + dao.NewResourceLabelDao(env.Database.SessionFactory), generic.Service(&env.Services), ) } diff --git a/test/integration/channels_test.go b/test/integration/channels_test.go index 7e3441c8..f0a77e01 100644 --- a/test/integration/channels_test.go +++ b/test/integration/channels_test.go @@ -55,25 +55,19 @@ func TestChannelCreate(t *testing.T) { channelName := fmt.Sprintf("channel-with-labels-%s", uuid.NewString()[:8]) channel := newChannelResource(channelName) - labels := map[string]string{ - "environment": "test", - "team": "platform", + channel.Labels = []api.ResourceLabel{ + {Key: "environment", Value: "test"}, + {Key: "team", Value: "platform"}, } - var err error - channel.Labels, err = json.Marshal(labels) - Expect(err).To(BeNil(), "should marshal labels") createdChannel, svcErr := svc.Create(t.Context(), "Channel", channel) Expect(svcErr).To(BeNil()) - Expect(createdChannel.Labels).NotTo(BeNil()) + Expect(createdChannel.Labels).NotTo(BeEmpty()) - // Get the resource and verify labels persisted retrieved, getErr := svc.Get(t.Context(), "Channel", createdChannel.ID) Expect(getErr).To(BeNil(), "should retrieve channel") - var retrievedLabels map[string]string - err = json.Unmarshal(retrieved.Labels, &retrievedLabels) - Expect(err).To(BeNil(), "should unmarshal retrieved labels") - Expect(retrievedLabels).To(Equal(labels), "retrieved labels should match") + retrievedLabels := labelsToMap(retrieved.Labels) + Expect(retrievedLabels).To(Equal(map[string]string{"environment": "test", "team": "platform"})) }) t.Run("SetsTimestamps", func(t *testing.T) { @@ -337,8 +331,7 @@ func TestChannelPatch(t *testing.T) { Expect(patched.Generation).To(Equal(int32(2)), "generation should increment to 2") // Verify labels - var patchedLabels map[string]string - json.Unmarshal(patched.Labels, &patchedLabels) + patchedLabels := labelsToMap(patched.Labels) Expect(patchedLabels).To(Equal(newLabels)) }) diff --git a/test/integration/resource_delete_test.go b/test/integration/resource_delete_test.go index 9735a045..f063721d 100644 --- a/test/integration/resource_delete_test.go +++ b/test/integration/resource_delete_test.go @@ -170,6 +170,55 @@ func TestResourceDelete_ParentChildWithRequiredAdapters(t *testing.T) { }) } +func TestExistsSoftDeletedByOwner(t *testing.T) { + t.Run("DetectsSoftDeletedChild", func(t *testing.T) { + RegisterTestingT(t) + svc, h := setupResourceTest(t) + + registry.UpdateDescriptor("Version", func(d *registry.EntityDescriptor) { + d.RequiredAdapters = []string{"test-adapter"} + }) + t.Cleanup(func() { + registry.UpdateDescriptor("Version", func(d *registry.EntityDescriptor) { + d.RequiredAdapters = nil + }) + }) + + channel := createTestChannel(t, svc) + version := createTestVersion(t, svc, fmt.Sprintf("v-%s", uuid.NewString()[:8]), channel.ID) + + // Soft-delete the version (RequiredAdapters forces soft-delete) + _, svcErr := svc.Delete(t.Context(), "Version", version.ID) + Expect(svcErr).To(BeNil()) + + // ExistsSoftDeletedByOwner raw SQL: kind IN (?) AND owner_id = ? AND deleted_time IS NOT NULL + dbSession := h.DBFactory.New(t.Context()) + var exists bool + err := dbSession.Raw( + "SELECT EXISTS(SELECT 1 FROM resources WHERE kind IN (?) AND owner_id = ? AND deleted_time IS NOT NULL)", + []string{"Version"}, channel.ID, + ).Scan(&exists).Error + Expect(err).To(BeNil()) + Expect(exists).To(BeTrue(), "should detect soft-deleted child via raw SQL") + + // Also verify with multiple kinds (the slice binding path) + err = dbSession.Raw( + "SELECT EXISTS(SELECT 1 FROM resources WHERE kind IN (?) AND owner_id = ? AND deleted_time IS NOT NULL)", + []string{"Version", "NonexistentKind"}, channel.ID, + ).Scan(&exists).Error + Expect(err).To(BeNil()) + Expect(exists).To(BeTrue(), "should detect with multi-kind IN clause") + + // Negative case: no soft-deleted children for a random owner + err = dbSession.Raw( + "SELECT EXISTS(SELECT 1 FROM resources WHERE kind IN (?) AND owner_id = ? AND deleted_time IS NOT NULL)", + []string{"Version"}, "nonexistent-owner", + ).Scan(&exists).Error + Expect(err).To(BeNil()) + Expect(exists).To(BeFalse(), "should not detect for nonexistent owner") + }) +} + // TestResourceDelete_WithoutRequiredAdapters tests delete behavior when Version // does NOT have RequiredAdapters configured (hard-delete scenario). func TestResourceDelete_WithoutRequiredAdapters(t *testing.T) { diff --git a/test/integration/resource_helpers.go b/test/integration/resource_helpers.go index 1de8976f..ff8fd2b9 100644 --- a/test/integration/resource_helpers.go +++ b/test/integration/resource_helpers.go @@ -31,6 +31,14 @@ func checkResourceCount(ctx context.Context, h *test.Helper, ids []string, expec return nil } +func labelsToMap(labels []api.ResourceLabel) map[string]string { + m := make(map[string]string, len(labels)) + for _, l := range labels { + m[l.Key] = l.Value + } + return m +} + // newChannelResource creates a Channel resource struct with default spec. // Does NOT persist to database - use svc.Create() to persist. func newChannelResource(name string) *api.Resource { diff --git a/test/integration/versions_test.go b/test/integration/versions_test.go index b0a81a9b..27636b2f 100644 --- a/test/integration/versions_test.go +++ b/test/integration/versions_test.go @@ -113,25 +113,19 @@ func TestVersionCreate(t *testing.T) { channel := createTestChannel(t, svc) version := newVersionResource("version-with-labels", channel.ID) - labels := map[string]string{ - "environment": "test", - "team": "platform", + version.Labels = []api.ResourceLabel{ + {Key: "environment", Value: "test"}, + {Key: "team", Value: "platform"}, } - var err error - version.Labels, err = json.Marshal(labels) - Expect(err).To(BeNil(), "should marshal labels") createdVersion, svcErr := svc.Create(t.Context(), "Version", version) Expect(svcErr).To(BeNil()) - Expect(createdVersion.Labels).NotTo(BeNil()) + Expect(createdVersion.Labels).NotTo(BeEmpty()) - // Get the resource and verify labels persisted retrieved, getErr := svc.Get(t.Context(), "Version", createdVersion.ID) Expect(getErr).To(BeNil(), "should retrieve version") - var retrievedLabels map[string]string - jsonErr := json.Unmarshal(retrieved.Labels, &retrievedLabels) - Expect(jsonErr).To(BeNil(), "should unmarshal retrieved labels") - Expect(retrievedLabels).To(Equal(labels), "retrieved labels should match") + retrievedLabels := labelsToMap(retrieved.Labels) + Expect(retrievedLabels).To(Equal(map[string]string{"environment": "test", "team": "platform"})) }) t.Run("SetsTimestamps", func(t *testing.T) { @@ -302,23 +296,17 @@ func TestVersionList(t *testing.T) { // Create version with unique label version1 := newVersionResource("version-with-label-1", channel.ID) - labels := map[string]string{ - "environment": uniqueLabel, - } - var err error - version1.Labels, err = json.Marshal(labels) - Expect(err).To(BeNil(), "should marshal labels") + version1.Labels = []api.ResourceLabel{{Key: "environment", Value: uniqueLabel}} created1, svcErr := svc.Create(t.Context(), "Version", version1) Expect(svcErr).To(BeNil()) - Expect(created1.Labels).NotTo(BeNil()) + Expect(created1.Labels).NotTo(BeEmpty()) // Create another version with the same label version2 := newVersionResource("version-with-label-2", channel.ID) - version2.Labels, err = json.Marshal(labels) - Expect(err).To(BeNil(), "should marshal labels") + version2.Labels = []api.ResourceLabel{{Key: "environment", Value: uniqueLabel}} created2, svcErr := svc.Create(t.Context(), "Version", version2) Expect(svcErr).To(BeNil()) - Expect(created2.Labels).NotTo(BeNil()) + Expect(created2.Labels).NotTo(BeEmpty()) // Create version without the label version3 := createTestVersion(t, svc, "version-no-label", channel.ID) @@ -333,11 +321,8 @@ func TestVersionList(t *testing.T) { Expect(svcErr).To(BeNil(), "list by label should succeed") Expect(len(list)).To(BeNumerically(">=", 2), "should find at least 2 versions with the label") - // Verify all returned versions have the label for _, item := range list { - var itemLabels map[string]string - err := json.Unmarshal(item.Labels, &itemLabels) - Expect(err).To(BeNil()) + itemLabels := labelsToMap(item.Labels) Expect(itemLabels["environment"]).To(Equal(uniqueLabel)) } @@ -521,11 +506,8 @@ func TestVersionPatch(t *testing.T) { retrieved, getErr := svc.Get(t.Context(), "Version", version.ID) Expect(getErr).To(BeNil()) - // Verify updated version is incremented Expect(retrieved.Generation).To(Equal(int32(2))) - var retrievedLabelValues map[string]string - jsonErr := json.Unmarshal(retrieved.Labels, &retrievedLabelValues) - Expect(jsonErr).To(BeNil()) + retrievedLabelValues := labelsToMap(retrieved.Labels) Expect(retrievedLabelValues).To(Equal(newLabels), "patched labels should match retrieved labels") })