HYPERFLEET-1160 - feat: separate resource_labels table for generic resource labels

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
 }

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",

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"`

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
+}

pkg/api/resource_label_test.go

@@ -0,0 +1,75 @@
+package api
+
+import (
+	"strings"
+	"testing"
+
+	. "github.com/onsi/gomega"
+)
+
+func TestValidateLabel(t *testing.T) {
+	tests := []struct {
+		name        string
+		key         string
+		value       string
+		expectError bool
+	}{
+		{
+			name:        "empty key returns error",
+			key:         "",
+			value:       "v",
+			expectError: true,
+		},
+		{
+			name:        "valid key and value",
+			key:         "env",
+			value:       "prod",
+			expectError: false,
+		},
+		{
+			name:        "key at exactly 255 chars",
+			key:         strings.Repeat("k", MaxLabelKeyLen),
+			value:       "v",
+			expectError: false,
+		},
+		{
+			name:        "key at 256 chars returns error",
+			key:         strings.Repeat("k", MaxLabelKeyLen+1),
+			value:       "v",
+			expectError: true,
+		},
+		{
+			name:        "value at exactly 255 chars",
+			key:         "env",
+			value:       strings.Repeat("v", MaxLabelValueLen),
+			expectError: false,
+		},
+		{
+			name:        "value at 256 chars returns error",
+			key:         "env",
+			value:       strings.Repeat("v", MaxLabelValueLen+1),
+			expectError: true,
+		},
+		{
+			name:        "empty value is allowed",
+			key:         "env",
+			value:       "",
+			expectError: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			RegisterTestingT(t)
+
+			err := ValidateLabel(tt.key, tt.value)
+
+			if tt.expectError {
+				Expect(err).ToNot(BeNil())
+				return
+			}
+
+			Expect(err).To(BeNil())
+		})
+	}
+}

pkg/dao/resource.go

@@ -38,7 +38,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
@@ -47,7 +48,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.Preload("Conditions").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
 	}
@@ -57,7 +58,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
 	}
@@ -130,7 +131,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
@@ -139,7 +140,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
@@ -148,7 +150,7 @@ func (d *sqlResourceDao) FindByKindAndOwner(ctx context.Context, kind, ownerID s
 func (d *sqlResourceDao) GetByID(ctx context.Context, id string) (*api.Resource, error) {
 	g2 := d.sessionFactory.New(ctx)
 	var resource api.Resource
-	if err := g2.Preload("Conditions").Take(&resource, "id = ?", id).Error; err != nil {
+	if err := g2.Preload("Conditions").Preload("Labels").Take(&resource, "id = ?", id).Error; err != nil {
 		return nil, err
 	}
 	return &resource, nil
@@ -159,7 +161,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
 	}

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
+}

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
+		},
+	}
+}

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.