From 9557c4579a068bac10b2d5ac05fe97d977099c0b Mon Sep 17 00:00:00 2001 From: Dmitrii Andreev Date: Wed, 8 Jul 2026 13:33:22 -0500 Subject: [PATCH 1/2] HYPERFLEET-1156 - feat: add resource references (non-ownership associations) Add a resource_references table with composite PK (source_id, ref_type, target_id) and FK constraints (CASCADE on source, RESTRICT on target). Supports CRUD: - Create/Patch persist references with validation (min/max, target existence, duplicates) - Delete blocks when target is referenced (409), force-delete clears inbound refs - List supports ref_type + ref_target_id query params for filtering by reference - Presenter renders references as grouped ObjectReference map with href links --- pkg/api/presenters/resource.go | 23 + pkg/api/presenters/resource_test.go | 45 ++ pkg/api/resource.go | 1 + pkg/api/resource_reference.go | 20 + pkg/dao/mocks/resource.go | 16 + pkg/dao/resource.go | 71 ++- .../202607021925_add_resource_references.go | 33 ++ pkg/db/migrations/migration_structs.go | 1 + pkg/handlers/resource_handler.go | 6 +- pkg/handlers/resource_handler_test.go | 8 +- pkg/handlers/root_resource_handler.go | 6 +- pkg/registry/registry.go | 1 + pkg/services/resource.go | 220 ++++++++- pkg/services/resource_test.go | 288 +++++++++++- pkg/services/types.go | 23 +- pkg/services/types_test.go | 35 ++ test/integration/channels_test.go | 8 +- test/integration/resource_conditions_test.go | 16 +- test/integration/resource_delete_test.go | 14 +- .../integration/resource_force_delete_test.go | 4 +- test/integration/resource_references_test.go | 426 ++++++++++++++++++ test/integration/root_resources_test.go | 8 +- test/integration/versions_test.go | 12 +- 23 files changed, 1224 insertions(+), 61 deletions(-) create mode 100644 pkg/api/resource_reference.go create mode 100644 pkg/db/migrations/202607021925_add_resource_references.go create mode 100644 test/integration/resource_references_test.go diff --git a/pkg/api/presenters/resource.go b/pkg/api/presenters/resource.go index e5d28e6d..06a68590 100644 --- a/pkg/api/presenters/resource.go +++ b/pkg/api/presenters/resource.go @@ -8,6 +8,7 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/registry" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/util" ) @@ -90,6 +91,11 @@ func PresentResource(r *api.Resource) openapi.Resource { } } + if len(r.References) > 0 { + refs := presentResourceReferences(r.References) + resp.References = &refs + } + return resp } @@ -107,6 +113,23 @@ func PresentResourceList(resources api.ResourceList, paging *api.PagingMeta) ope } } +func presentResourceReferences(refs []api.ResourceReference) map[string][]openapi.ObjectReference { + result := make(map[string][]openapi.ObjectReference) + for _, ref := range refs { + id := ref.TargetID + objRef := openapi.ObjectReference{ + Id: &id, + Kind: ref.TargetKind, + } + if targetDesc, ok := registry.Get(ref.TargetKind); ok { + href := fmt.Sprintf("/api/hyperfleet/v1/%s/%s", targetDesc.Plural, ref.TargetID) + objRef.Href = &href + } + result[ref.RefType] = append(result[ref.RefType], objRef) + } + return result +} + func presentResourceConditions(conditions []api.ResourceCondition) []openapi.ResourceCondition { if len(conditions) == 0 { return []openapi.ResourceCondition{} diff --git a/pkg/api/presenters/resource_test.go b/pkg/api/presenters/resource_test.go index b52b7215..3817a573 100644 --- a/pkg/api/presenters/resource_test.go +++ b/pkg/api/presenters/resource_test.go @@ -10,6 +10,7 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/registry" ) func TestConvertResource(t *testing.T) { @@ -217,6 +218,50 @@ func TestPresentResource_WithEmptyConditions(t *testing.T) { Expect(string(body)).To(ContainSubstring(`"status":{"conditions":[]}`)) } +func TestPresentResource_WithReferences(t *testing.T) { + RegisterTestingT(t) + + registry.Reset() + registry.Register(registry.EntityDescriptor{ + Kind: "WifConfig", + Plural: "wifconfigs", + }) + + now := time.Now() + resource := &api.Resource{ + Meta: api.Meta{ID: "res-1", CreatedTime: now, UpdatedTime: now}, + Kind: "Cluster", + Name: "my-cluster", + Spec: datatypes.JSON(`{}`), + CreatedBy: "user@test.com", + UpdatedBy: "user@test.com", + References: []api.ResourceReference{ + { + SourceID: "res-1", + RefType: "wif_config", + TargetID: "wif-1", + TargetKind: "WifConfig", + }, + { + SourceID: "res-1", + RefType: "wif_config", + TargetID: "wif-2", + TargetKind: "WifConfig", + }, + }, + } + + resp := PresentResource(resource) + Expect(resp.References).ToNot(BeNil()) + refs := *resp.References + Expect(refs).To(HaveKey("wif_config")) + Expect(refs["wif_config"]).To(HaveLen(2)) + Expect(*refs["wif_config"][0].Id).To(Equal("wif-1")) + Expect(refs["wif_config"][0].Kind).To(Equal("WifConfig")) + Expect(*refs["wif_config"][0].Href).To(Equal("/api/hyperfleet/v1/wifconfigs/wif-1")) + Expect(*refs["wif_config"][1].Id).To(Equal("wif-2")) +} + func TestPresentResourceList(t *testing.T) { RegisterTestingT(t) diff --git a/pkg/api/resource.go b/pkg/api/resource.go index 75c42da7..46e80e52 100644 --- a/pkg/api/resource.go +++ b/pkg/api/resource.go @@ -30,6 +30,7 @@ type Resource struct { Labels datatypes.JSON `json:"labels,omitempty" gorm:"type:jsonb"` Spec datatypes.JSON `json:"spec" gorm:"type:jsonb;not null"` Conditions []ResourceCondition `json:"-" gorm:"foreignKey:ResourceID;references:ID"` + References []ResourceReference `json:"-" gorm:"foreignKey:SourceID;references:ID"` Generation int32 `json:"generation" gorm:"default:1;not null"` } diff --git a/pkg/api/resource_reference.go b/pkg/api/resource_reference.go new file mode 100644 index 00000000..1821e79c --- /dev/null +++ b/pkg/api/resource_reference.go @@ -0,0 +1,20 @@ +package api + +// ResourceReference stores a single non-ownership association between two resources. +// Table: resource_references. Natural composite PK (source_id, ref_type, target_id). +type ResourceReference struct { + SourceID string `json:"source_id" gorm:"primaryKey;column:source_id;size:255"` + RefType string `json:"ref_type" gorm:"primaryKey;column:ref_type;size:255"` + TargetID string `json:"target_id" gorm:"primaryKey;column:target_id;size:255"` + TargetKind string `json:"target_kind" gorm:"column:target_kind;size:100;not null"` +} + +func (ResourceReference) TableName() string { + return "resource_references" +} + +// ResourceSummary carries kind + name for error messages (e.g. FindReferencers 409). +type ResourceSummary struct { + Kind string + Name string +} diff --git a/pkg/dao/mocks/resource.go b/pkg/dao/mocks/resource.go index ad91ddff..53824cea 100644 --- a/pkg/dao/mocks/resource.go +++ b/pkg/dao/mocks/resource.go @@ -122,3 +122,19 @@ func (d *resourceDaoMock) GetByID(_ context.Context, id string) (*api.Resource, } return nil, gorm.ErrRecordNotFound } + +func (d *resourceDaoMock) ReplaceReferences(_ context.Context, _ string, _ []api.ResourceReference) error { + return nil +} + +func (d *resourceDaoMock) FindReferencers(_ context.Context, _ string) ([]api.ResourceSummary, error) { + return nil, nil +} + +func (d *resourceDaoMock) ClearTargetReferences(_ context.Context, _ string) error { + return nil +} + +func (d *resourceDaoMock) FindSourceIDsByRef(_ context.Context, _, _ string) ([]string, error) { + return nil, nil +} diff --git a/pkg/dao/resource.go b/pkg/dao/resource.go index 18db1cdb..9a6d3e45 100644 --- a/pkg/dao/resource.go +++ b/pkg/dao/resource.go @@ -23,6 +23,10 @@ type ResourceDao interface { FindByKindAndOwner(ctx context.Context, kind, ownerID string) (api.ResourceList, error) FindByKindAndOwnerForUpdate(ctx context.Context, kind, ownerID string) (api.ResourceList, error) GetByID(ctx context.Context, id string) (*api.Resource, error) + ReplaceReferences(ctx context.Context, sourceID string, refs []api.ResourceReference) error + FindReferencers(ctx context.Context, targetID string) ([]api.ResourceSummary, error) + ClearTargetReferences(ctx context.Context, targetID string) error + FindSourceIDsByRef(ctx context.Context, refType, targetID string) ([]string, error) } var _ ResourceDao = &sqlResourceDao{} @@ -38,7 +42,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("References"). + Take(&resource, "kind = ? AND id = ?", kind, id).Error; err != nil { return nil, err } return &resource, nil @@ -57,7 +62,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("References"). Take(&resource, "kind = ? AND id = ? AND owner_id = ?", kind, id, ownerID).Error; err != nil { return nil, err } @@ -148,7 +153,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("References").Take(&resource, "id = ?", id).Error; err != nil { return nil, err } return &resource, nil @@ -165,3 +170,63 @@ func (d *sqlResourceDao) FindByKindAndOwnerForUpdate( } return resources, nil } + +func (d *sqlResourceDao) ReplaceReferences( + ctx context.Context, sourceID string, refs []api.ResourceReference, +) error { + g2 := d.sessionFactory.New(ctx) + if err := g2.Where("source_id = ?", sourceID).Delete(&api.ResourceReference{}).Error; err != nil { + db.MarkForRollback(ctx, err) + return err + } + if len(refs) > 0 { + if err := g2.Create(&refs).Error; err != nil { + db.MarkForRollback(ctx, err) + return err + } + } + return nil +} + +func (d *sqlResourceDao) FindReferencers( + ctx context.Context, targetID string, +) ([]api.ResourceSummary, error) { + g2 := d.sessionFactory.New(ctx) + var summaries []api.ResourceSummary + if err := g2.Raw(` + SELECT r.kind, r.name + FROM resource_references rr + JOIN resources r ON rr.source_id = r.id + WHERE rr.target_id = ? AND r.deleted_time IS NULL + LIMIT 1`, + targetID, + ).Scan(&summaries).Error; err != nil { + return nil, err + } + return summaries, nil +} + +// ClearTargetReferences removes all inbound references pointing at targetID. +// Called by forceDeleteResourceTree before hard-deleting a referenced target, +// because the target_id FK uses ON DELETE RESTRICT. +func (d *sqlResourceDao) ClearTargetReferences(ctx context.Context, targetID string) error { + g2 := d.sessionFactory.New(ctx) + if err := g2.Where("target_id = ?", targetID).Delete(&api.ResourceReference{}).Error; err != nil { + db.MarkForRollback(ctx, err) + return err + } + return nil +} + +func (d *sqlResourceDao) FindSourceIDsByRef( + ctx context.Context, refType, targetID string, +) ([]string, error) { + g2 := d.sessionFactory.New(ctx) + var ids []string + if err := g2.Model(&api.ResourceReference{}). + Where("ref_type = ? AND target_id = ?", refType, targetID). + Pluck("source_id", &ids).Error; err != nil { + return nil, err + } + return ids, nil +} diff --git a/pkg/db/migrations/202607021925_add_resource_references.go b/pkg/db/migrations/202607021925_add_resource_references.go new file mode 100644 index 00000000..1ddbb3ca --- /dev/null +++ b/pkg/db/migrations/202607021925_add_resource_references.go @@ -0,0 +1,33 @@ +package migrations + +import ( + "github.com/go-gormigrate/gormigrate/v2" + "gorm.io/gorm" +) + +func addResourceReferences() *gormigrate.Migration { + return &gormigrate.Migration{ + ID: "202607021925", + Migrate: func(tx *gorm.DB) error { + if err := tx.Exec(`CREATE TABLE IF NOT EXISTS resource_references ( + source_id VARCHAR(255) NOT NULL, + ref_type VARCHAR(255) NOT NULL, + target_id VARCHAR(255) NOT NULL, + target_kind VARCHAR(100) NOT NULL, + PRIMARY KEY (source_id, ref_type, target_id), + FOREIGN KEY (source_id) REFERENCES resources(id) ON DELETE CASCADE, + FOREIGN KEY (target_id) REFERENCES resources(id) ON DELETE RESTRICT + );`).Error; err != nil { + return err + } + + if err := tx.Exec( + `CREATE INDEX idx_resource_references_target ON resource_references (target_id);`, + ).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..e6a2c04e 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(), + addResourceReferences(), } // Model represents the base model struct. All entities will have this struct embedded. diff --git a/pkg/handlers/resource_handler.go b/pkg/handlers/resource_handler.go index e4b90f32..cfd6d437 100644 --- a/pkg/handlers/resource_handler.go +++ b/pkg/handlers/resource_handler.go @@ -65,7 +65,11 @@ func (h *ResourceHandler) Create(w http.ResponseWriter, r *http.Request) { return nil, errors.GeneralError("failed to convert resource: %v", err) } - resource, svcErr := h.service.Create(ctx, h.descriptor.Kind, resource) + var refs map[string][]openapi.ObjectReference + if req.References != nil { + refs = *req.References + } + resource, svcErr := h.service.Create(ctx, h.descriptor.Kind, resource, refs) if svcErr != nil { return nil, svcErr } diff --git a/pkg/handlers/resource_handler_test.go b/pkg/handlers/resource_handler_test.go index ef6b3117..85dc20e9 100644 --- a/pkg/handlers/resource_handler_test.go +++ b/pkg/handlers/resource_handler_test.go @@ -65,7 +65,9 @@ func TestResourceHandler_Create(t *testing.T) { name: "Success - creates channel", body: `{"kind":"Channel","name":"stable","spec":{"is_default":true}}`, setupMock: func(mock *services.MockResourceService) { - mock.EXPECT().Create(gomock.Any(), "Channel", gomock.AssignableToTypeOf(&api.Resource{})).Return(&api.Resource{ + mock.EXPECT().Create( + gomock.Any(), "Channel", gomock.AssignableToTypeOf(&api.Resource{}), gomock.Any(), + ).Return(&api.Resource{ Meta: api.Meta{ID: "ch-123", CreatedTime: now, UpdatedTime: now}, Kind: "Channel", Name: "stable", @@ -96,7 +98,7 @@ func TestResourceHandler_Create(t *testing.T) { name: "Error 409 - duplicate name", body: `{"kind":"Channel","name":"stable","spec":{"is_default":true}}`, setupMock: func(mock *services.MockResourceService) { - mock.EXPECT().Create(gomock.Any(), "Channel", gomock.AssignableToTypeOf(&api.Resource{})). + mock.EXPECT().Create(gomock.Any(), "Channel", gomock.AssignableToTypeOf(&api.Resource{}), gomock.Any()). Return(nil, errors.Conflict("Channel with name 'stable' already exists")) }, expectedStatusCode: http.StatusConflict, @@ -378,7 +380,7 @@ func TestResourceHandler_CreateWithOwner(t *testing.T) { Href: "/api/hyperfleet/v1/channels/ch-1", Spec: datatypes.JSON(`{}`), CreatedBy: "u@t.com", UpdatedBy: "u@t.com", }, nil) - mock.EXPECT().Create(gomock.Any(), "Version", gomock.AssignableToTypeOf(&api.Resource{})). + mock.EXPECT().Create(gomock.Any(), "Version", gomock.AssignableToTypeOf(&api.Resource{}), gomock.Any()). Return(&api.Resource{ Meta: api.Meta{ID: "v-1", CreatedTime: now, UpdatedTime: now}, Kind: "Version", Name: "4-17-3", diff --git a/pkg/handlers/root_resource_handler.go b/pkg/handlers/root_resource_handler.go index 5f4bc570..dcbb7c54 100644 --- a/pkg/handlers/root_resource_handler.go +++ b/pkg/handlers/root_resource_handler.go @@ -104,7 +104,11 @@ func (h *RootResourceHandler) Create(w http.ResponseWriter, r *http.Request) { if convErr != nil { return nil, errors.GeneralError("failed to convert resource: %v", convErr) } - resource, svcErr := h.service.Create(r.Context(), descriptor.Kind, resource) + var refs map[string][]openapi.ObjectReference + if req.References != nil { + refs = *req.References + } + resource, svcErr := h.service.Create(r.Context(), descriptor.Kind, resource, refs) if svcErr != nil { return nil, svcErr } diff --git a/pkg/registry/registry.go b/pkg/registry/registry.go index 2d84a3d4..7deb65d8 100644 --- a/pkg/registry/registry.go +++ b/pkg/registry/registry.go @@ -94,6 +94,7 @@ func ChildrenOf(parentKind string) []EntityDescriptor { // - ReferenceDescriptor with TargetKind that doesn't resolve // - duplicate RefType within a single entity's References // - Max < Min (when Max > 0) +// - circular required references (Min > 0 cycle between two or more kinds) func Validate() { plurals := make(map[string]string, len(descriptors)) diff --git a/pkg/services/resource.go b/pkg/services/resource.go index c2c9804e..2931841a 100644 --- a/pkg/services/resource.go +++ b/pkg/services/resource.go @@ -4,9 +4,11 @@ import ( "context" "encoding/json" "fmt" + "strings" "time" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/dao" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" @@ -20,7 +22,7 @@ import ( type ResourceService interface { Get(ctx context.Context, kind, id string) (*api.Resource, *errors.ServiceError) - Create(ctx context.Context, kind string, resource *api.Resource) (*api.Resource, *errors.ServiceError) + Create(ctx context.Context, kind string, resource *api.Resource, refs map[string][]openapi.ObjectReference) (*api.Resource, *errors.ServiceError) //nolint:lll Patch(ctx context.Context, kind, id string, patch *api.ResourcePatch) (*api.Resource, *errors.ServiceError) Delete(ctx context.Context, kind, id string) (*api.Resource, *errors.ServiceError) List(ctx context.Context, kind string, args *ListArguments) (api.ResourceList, *api.PagingMeta, *errors.ServiceError) @@ -70,8 +72,14 @@ func (s *sqlResourceService) Get(ctx context.Context, kind, id string) (*api.Res // 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. +// +// refs carries the non-ownership references from the API request. nil means "no references +// supplied" (no validation of required refs is skipped only when the entity has no Min>0 +// descriptors). An empty map {} means "clear all references" — Min>0 descriptors will +// reject this with 422. func (s *sqlResourceService) Create( ctx context.Context, kind string, resource *api.Resource, + refs map[string][]openapi.ObjectReference, ) (*api.Resource, *errors.ServiceError) { resource.Kind = kind @@ -91,6 +99,10 @@ func (s *sqlResourceService) Create( } } + if svcErr := s.validateReferences(ctx, kind, refs); svcErr != nil { + return nil, svcErr + } + username := actorFromContext(ctx) if resource.CreatedBy == "" { resource.CreatedBy = username @@ -103,6 +115,16 @@ func (s *sqlResourceService) Create( if err != nil { return nil, handleCreateError(kind, err) } + + // Persist references after the resource row exists (FK requires source_id). + if len(refs) > 0 { + refRows := convertRefs(kind, resource.ID, refs) + if refErr := s.resourceDao.ReplaceReferences(ctx, resource.ID, refRows); refErr != nil { + return nil, errors.GeneralError("failed to save references: %s", refErr) + } + resource.References = refRows + } + return resource, nil } @@ -134,7 +156,23 @@ func (s *sqlResourceService) Patch( return nil, errors.Validation("Invalid patch data: %v", applyErr) } - if jsonBytesEqual(oldSpec, resource.Spec) && jsonBytesEqual(oldLabels, resource.Labels) { + specChanged := !jsonBytesEqual(oldSpec, resource.Spec) + labelsChanged := !jsonBytesEqual(oldLabels, resource.Labels) + refsChanged := patch.References != nil + + // Validate and persist references when the patch includes them (nil = skip, {} = clear). + if refsChanged { + if svcErr := s.validateReferences(ctx, kind, patch.References); svcErr != nil { + return nil, svcErr + } + if refErr := s.resourceDao.ReplaceReferences( + ctx, resource.ID, convertRefs(kind, resource.ID, patch.References), + ); refErr != nil { + return nil, errors.GeneralError("failed to save references: %s", refErr) + } + } + + if !specChanged && !labelsChanged && !refsChanged { return resource, nil } @@ -146,6 +184,10 @@ func (s *sqlResourceService) Patch( return nil, handleUpdateError(kind, saveErr) } + if refsChanged { + resource.References = convertRefs(kind, resource.ID, patch.References) + } + return resource, nil } @@ -160,6 +202,22 @@ func (s *sqlResourceService) Delete(ctx context.Context, kind, id string) (*api. return nil, handleSoftDeleteError(kind, err) } + // Reject first-time deletion if other resources still reference this one. + // Skip on re-evaluation (already soft-deleted) so the re-delete path can + // re-assess whether to hard-delete after children are finalized. + if resource.DeletedTime == nil { + referencers, refErr := s.resourceDao.FindReferencers(ctx, resource.ID) + if refErr != nil { + return nil, errors.GeneralError("failed to check references: %s", refErr) + } + if len(referencers) > 0 { + return nil, errors.ConflictState( + "cannot delete %s %q: referenced by %s %q — remove the reference before deleting", + kind, id, referencers[0].Kind, referencers[0].Name, + ) + } + } + deletedBy := actorFromContext(ctx) deletedAt := time.Now().UTC().Truncate(time.Microsecond) @@ -212,6 +270,18 @@ func (s *sqlResourceService) deleteResourceTree( } } + // Check if other resources reference this one before any deletion. + referencers, refErr := s.resourceDao.FindReferencers(ctx, resource.ID) + if refErr != nil { + return errors.GeneralError("failed to check references: %s", refErr) + } + if len(referencers) > 0 { + return errors.ConflictState( + "cannot delete %s %q: referenced by %s %q — remove the reference before deleting", + resource.Kind, resource.ID, referencers[0].Kind, referencers[0].Name, + ) + } + shouldSoftDelete, svcErr := s.shouldSoftDelete(ctx, resource, children) if svcErr != nil { return svcErr @@ -315,6 +385,12 @@ func (s *sqlResourceService) List( scopedArgs.Search = "(" + scopedArgs.Search + ") AND " + kindFilter } + if svcErr := s.applyRefFilter(ctx, kind, &scopedArgs); svcErr != nil { + return nil, nil, svcErr + } + + scopedArgs.Preloads = append(scopedArgs.Preloads, "Conditions", "References") + var resources api.ResourceList paging, svcErr := s.generic.List(ctx, &scopedArgs, &resources) if svcErr != nil { @@ -343,6 +419,12 @@ func (s *sqlResourceService) ListByOwner( scopedArgs.Search = "(" + scopedArgs.Search + ") AND " + kindFilter } + if svcErr := s.applyRefFilter(ctx, kind, &scopedArgs); svcErr != nil { + return nil, nil, svcErr + } + + scopedArgs.Preloads = append(scopedArgs.Preloads, "Conditions", "References") + var resources []api.Resource paging, svcErr := s.generic.List(ctx, &scopedArgs, &resources) if svcErr != nil { @@ -639,6 +721,46 @@ func (s *sqlResourceService) hasActiveChildren( return false, nil } +func (s *sqlResourceService) applyRefFilter( + ctx context.Context, kind string, args *ListArguments, +) *errors.ServiceError { + if args.RefType == "" { + return nil + } + desc := registry.MustGet(kind) + found := false + for _, ref := range desc.References { + if ref.RefType == args.RefType { + found = true + break + } + } + if !found { + return errors.Validation("Unknown ref_type %q for entity %s", args.RefType, kind) + } + const maxRefFilterIDs = 1000 + sourceIDs, err := s.resourceDao.FindSourceIDsByRef(ctx, args.RefType, args.RefTargetID) + if err != nil { + return errors.GeneralError("failed to query references: %s", err) + } + if len(sourceIDs) > maxRefFilterIDs { + return errors.Validation( + "ref_type filter matches %d resources (limit %d); "+ + "use pagination on the referencing entity directly", + len(sourceIDs), maxRefFilterIDs, + ) + } + if len(sourceIDs) == 0 { + args.Search += ` AND id = ""` + return nil + } + quoted := make([]string, len(sourceIDs)) + for i, sid := range sourceIDs { + quoted[i] = `"` + sid + `"` + } + args.Search += " AND id in [" + strings.Join(quoted, ", ") + "]" + return 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 { @@ -690,8 +812,6 @@ func applyResourcePatch(resource *api.Resource, patch *api.ResourcePatch) error } resource.Labels = labelsJSON } - // TODO: handle patch.References — three-way semantics (nil=skip, {}=clear, map=replace) - // via dao.ReplaceReferences per generic-resource-registry-design.md §9.2 return nil } @@ -752,9 +872,101 @@ func (s *sqlResourceService) forceDeleteResourceTree( if err := s.resourceConditionDao.DeleteByResource(ctx, resource.ID); err != nil { return errors.GeneralError("Failed to delete resource conditions during force-delete: %s", err) } + // Clear inbound references before hard-deleting (FK uses ON DELETE RESTRICT). + if err := s.resourceDao.ClearTargetReferences(ctx, resource.ID); err != nil { + return errors.GeneralError("failed to clear references: %s", err) + } if err := s.resourceDao.Delete(ctx, resource.Kind, resource.ID); err != nil { return handleDeleteError(resource.Kind, err) } return nil } + +// validateReferences checks that refs satisfies the ReferenceDescriptors on the entity: +// - required ref types (Min > 0) must be present +// - unknown ref types are rejected +// - per-type count must not exceed Max (when Max > 0) +// - every referenced target must exist in the database +func (s *sqlResourceService) validateReferences( + ctx context.Context, kind string, refs map[string][]openapi.ObjectReference, +) *errors.ServiceError { + desc := registry.MustGet(kind) + + descByType := make(map[string]registry.ReferenceDescriptor, len(desc.References)) + for _, rd := range desc.References { + descByType[rd.RefType] = rd + if rd.Min > 0 { + if len(refs[rd.RefType]) < rd.Min { + return errors.Validation( + "required reference type %q missing for %s (min %d)", + rd.RefType, kind, rd.Min, + ) + } + } + } + + // Validate each supplied ref type. + for refType, objRefs := range refs { + rd, ok := descByType[refType] + if !ok { + return errors.Validation("unknown reference type %q for %s", refType, kind) + } + if rd.Max > 0 && len(objRefs) > rd.Max { + return errors.Validation( + "reference type %q for %s exceeds max count %d (got %d)", + refType, kind, rd.Max, len(objRefs), + ) + } + seen := make(map[string]bool, len(objRefs)) + for _, ref := range objRefs { + if ref.Id == nil || *ref.Id == "" { + return errors.Validation("reference type %q: id is required", refType) + } + if seen[*ref.Id] { + return errors.Validation( + "reference type %q: duplicate target id %q", + refType, *ref.Id, + ) + } + seen[*ref.Id] = true + target, err := s.resourceDao.Get(ctx, rd.TargetKind, *ref.Id) + if err != nil { + return errors.Validation( + "reference type %q: target %s %q not found", + refType, rd.TargetKind, *ref.Id, + ) + } + if target.DeletedTime != nil { + return errors.Validation( + "reference type %q: target %s %q is marked for deletion", + refType, rd.TargetKind, *ref.Id, + ) + } + } + } + + return nil +} + +// convertRefs flattens the API reference map into a slice of ResourceReference rows for the DAO. +// Uses the registry's TargetKind (not the client-supplied Kind) so the stored value is always authoritative. +func convertRefs(kind, sourceID string, refs map[string][]openapi.ObjectReference) []api.ResourceReference { + desc := registry.MustGet(kind) + targetKindByRef := make(map[string]string, len(desc.References)) + for _, rd := range desc.References { + targetKindByRef[rd.RefType] = rd.TargetKind + } + var result []api.ResourceReference + for refType, objRefs := range refs { + for _, ref := range objRefs { + result = append(result, api.ResourceReference{ + SourceID: sourceID, + RefType: refType, + TargetID: *ref.Id, + TargetKind: targetKindByRef[refType], + }) + } + } + return result +} diff --git a/pkg/services/resource_test.go b/pkg/services/resource_test.go index 640ec061..5febbc34 100644 --- a/pkg/services/resource_test.go +++ b/pkg/services/resource_test.go @@ -11,6 +11,7 @@ import ( "gorm.io/gorm" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/auth" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/dao" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" @@ -48,6 +49,10 @@ type mockResourceDao struct { saveErr error deleteErr error existsSoftDeletedByOwnerErr error + replaceRefsErr error + findReferencersResult []api.ResourceSummary + lastReplacedRefs []api.ResourceReference + replaceRefsCalled bool } func newMockResourceDao() *mockResourceDao { @@ -165,6 +170,40 @@ func (d *mockResourceDao) GetByID(_ context.Context, id string) (*api.Resource, return nil, gorm.ErrRecordNotFound } +func (d *mockResourceDao) FindByIDs(_ context.Context, kind string, ids []string) (api.ResourceList, error) { + idSet := make(map[string]bool, len(ids)) + for _, id := range ids { + idSet[id] = true + } + var result api.ResourceList + for _, r := range d.resources { + if r.Kind == kind && idSet[r.ID] { + result = append(result, r) + } + } + return result, nil +} + +func (d *mockResourceDao) ReplaceReferences(_ context.Context, _ string, refs []api.ResourceReference) error { + d.replaceRefsCalled = true + d.lastReplacedRefs = refs + if d.replaceRefsErr != nil { + return d.replaceRefsErr + } + return nil +} + +func (d *mockResourceDao) FindReferencers(_ context.Context, _ string) ([]api.ResourceSummary, error) { + return d.findReferencersResult, nil +} + +func (d *mockResourceDao) ClearTargetReferences(_ context.Context, _ string) error { + return nil +} + +func (d *mockResourceDao) FindSourceIDsByRef(_ context.Context, _, _ string) ([]string, error) { + return nil, nil +} func (d *mockResourceDao) addResource(r *api.Resource) { d.resources[resourceKey(r.Kind, r.ID)] = r } @@ -285,7 +324,7 @@ func TestResourceService_Create_SetsDefaults(t *testing.T) { resource.CreatedBy = "" resource.UpdatedBy = "" - result, svcErr := svc.Create(context.Background(), "Channel", resource) + result, svcErr := svc.Create(context.Background(), "Channel", resource, nil) Expect(svcErr).To(BeNil()) Expect(result.Kind).To(Equal("Channel")) Expect(result.CreatedBy).To(Equal(defaultSystemUser)) @@ -304,7 +343,7 @@ func TestResourceService_Create_SetsUserFromAuthContext(t *testing.T) { resource.CreatedBy = "" resource.UpdatedBy = "" - result, svcErr := svc.Create(ctx, "Channel", resource) + result, svcErr := svc.Create(ctx, "Channel", resource, nil) Expect(svcErr).To(BeNil()) Expect(result.CreatedBy).To(Equal("user@test.com")) Expect(result.UpdatedBy).To(Equal("user@test.com")) @@ -322,7 +361,7 @@ func TestResourceService_Create_PreservesExplicitValues(t *testing.T) { resource.UpdatedBy = "explicit@test.com" resource.Generation = 5 - result, svcErr := svc.Create(context.Background(), "Channel", resource) + result, svcErr := svc.Create(context.Background(), "Channel", resource, nil) Expect(svcErr).To(BeNil()) Expect(result.CreatedBy).To(Equal("explicit@test.com")) Expect(result.Generation).To(Equal(int32(5))) @@ -336,7 +375,7 @@ func TestResourceService_Create_ValidName(t *testing.T) { svc, _, _ := newTestResourceService(mockDao) resource := testResource("Channel", "ch-1", "stable") - result, svcErr := svc.Create(context.Background(), "Channel", resource) + result, svcErr := svc.Create(context.Background(), "Channel", resource, nil) Expect(svcErr).To(BeNil()) Expect(result.Name).To(Equal("stable")) } @@ -350,7 +389,7 @@ func TestResourceService_Create_UniqueConstraint(t *testing.T) { svc, _, _ := newTestResourceService(mockDao) resource := testResource("Channel", "ch-1", "stable") - result, svcErr := svc.Create(context.Background(), "Channel", resource) + result, svcErr := svc.Create(context.Background(), "Channel", resource, nil) Expect(result).To(BeNil()) Expect(svcErr).ToNot(BeNil()) Expect(svcErr.HTTPCode).To(Equal(409)) @@ -364,7 +403,7 @@ func TestResourceService_Create_EmptyName(t *testing.T) { svc, _, _ := newTestResourceService(mockDao) resource := testResource("WifConfig", "wif-1", "") - result, svcErr := svc.Create(context.Background(), "WifConfig", resource) + result, svcErr := svc.Create(context.Background(), "WifConfig", resource, nil) Expect(result).To(BeNil()) Expect(svcErr).ToNot(BeNil()) Expect(svcErr.HTTPCode).To(Equal(400)) @@ -379,7 +418,7 @@ func TestResourceService_Create_UnknownKind(t *testing.T) { svc, _, _ := newTestResourceService(mockDao) resource := testResource("Bogus", "b-1", "test") - result, svcErr := svc.Create(context.Background(), "Bogus", resource) + result, svcErr := svc.Create(context.Background(), "Bogus", resource, nil) Expect(result).To(BeNil()) Expect(svcErr).ToNot(BeNil()) Expect(svcErr.HTTPCode).To(Equal(400)) @@ -399,7 +438,7 @@ func TestResourceService_Create_ChildLocksParent(t *testing.T) { child := testResource("Version", "v-1", "4.18") child.OwnerID = &parent.ID - result, svcErr := svc.Create(context.Background(), "Version", child) + result, svcErr := svc.Create(context.Background(), "Version", child, nil) Expect(svcErr).To(BeNil()) Expect(result).ToNot(BeNil()) } @@ -421,7 +460,7 @@ func TestResourceService_Create_ChildRejectsDeletedParent(t *testing.T) { child := testResource("Version", "v-1", "4.18") child.OwnerID = &parent.ID - result, svcErr := svc.Create(context.Background(), "Version", child) + result, svcErr := svc.Create(context.Background(), "Version", child, nil) Expect(result).To(BeNil()) Expect(svcErr).ToNot(BeNil()) Expect(svcErr.HTTPCode).To(Equal(409)) @@ -438,7 +477,7 @@ func TestResourceService_Create_ChildRejectsMissingParent(t *testing.T) { nonexistent := "no-such-id" child.OwnerID = &nonexistent - result, svcErr := svc.Create(context.Background(), "Version", child) + result, svcErr := svc.Create(context.Background(), "Version", child, nil) Expect(result).To(BeNil()) Expect(svcErr).ToNot(BeNil()) Expect(svcErr.HTTPCode).To(Equal(404)) @@ -1350,8 +1389,8 @@ func TestResourceService_Delete_CascadeParentSoftDeletedWhileChildSoftDeleted(t task := testResourceWithOwner("Task", "t-1", "build", "ws-1") mockDao.addResource(task) - // Delete Workspace → cascade-deletes Task (soft-delete because Task has RequiredAdapters) - // → Workspace should be soft-deleted (soft-deleted child exists) + // Delete Workspace -> cascade-deletes Task (soft-delete because Task has RequiredAdapters) + // -> Workspace should be soft-deleted (soft-deleted child exists) _, svcErr := svc.Delete(context.Background(), "Workspace", "ws-1") Expect(svcErr).To(BeNil()) @@ -1860,4 +1899,229 @@ func TestProcessAdapterStatus_SoftDeleted_ChildrenExist_NoHardDelete(t *testing. // Parent should NOT be hard-deleted because child exists _, exists := mockDao.resources[resourceKey("Parent", "p-1")] Expect(exists).To(BeTrue(), "Parent should still exist — active child blocks hard-delete") +// --- Resource References --- + +func setupRefDescriptors() { + registry.Reset() + registry.Register(registry.EntityDescriptor{ + Kind: "Target", + Plural: "targets", + }) + registry.Register(registry.EntityDescriptor{ + Kind: "Parent", + Plural: "parents", + References: []registry.ReferenceDescriptor{ + {RefType: "dep", TargetKind: "Target", Min: 1, Max: 1}, + }, + }) +} + +func setupOptionalRefDescriptors() { + registry.Reset() + registry.Register(registry.EntityDescriptor{ + Kind: "Target", + Plural: "targets", + }) + registry.Register(registry.EntityDescriptor{ + Kind: "Parent", + Plural: "parents", + References: []registry.ReferenceDescriptor{ + {RefType: "dep", TargetKind: "Target", Min: 0, Max: 3}, + }, + }) +} + +func TestResourceService_Create_RequiredRefMissing_Returns400(t *testing.T) { + RegisterTestingT(t) + setupRefDescriptors() + + mockDao := newMockResourceDao() + svc, _, _ := newTestResourceService(mockDao) + + resource := testResource("Parent", "p-1", "parent-1") + result, svcErr := svc.Create(context.Background(), "Parent", resource, nil) + Expect(result).To(BeNil()) + Expect(svcErr).ToNot(BeNil()) + Expect(svcErr.HTTPCode).To(Equal(400)) + Expect(svcErr.Reason).To(ContainSubstring("required reference type")) + Expect(svcErr.Reason).To(ContainSubstring("dep")) +} + +func TestResourceService_Create_RefExceedsMax_Returns400(t *testing.T) { + RegisterTestingT(t) + setupRefDescriptors() + + mockDao := newMockResourceDao() + svc, _, _ := newTestResourceService(mockDao) + + // Add two targets + mockDao.addResource(testResource("Target", "t-1", "target-1")) + mockDao.addResource(testResource("Target", "t-2", "target-2")) + + resource := testResource("Parent", "p-1", "parent-1") + refs := map[string][]openapi.ObjectReference{ + "dep": { + {Id: strPtr("t-1"), Kind: "Target"}, + {Id: strPtr("t-2"), Kind: "Target"}, + }, + } + + result, svcErr := svc.Create(context.Background(), "Parent", resource, refs) + Expect(result).To(BeNil()) + Expect(svcErr).ToNot(BeNil()) + Expect(svcErr.HTTPCode).To(Equal(400)) + Expect(svcErr.Reason).To(ContainSubstring("exceeds max count")) +} + +func TestResourceService_Create_WithValidRefs_Persists(t *testing.T) { + RegisterTestingT(t) + setupRefDescriptors() + + mockDao := newMockResourceDao() + svc, _, _ := newTestResourceService(mockDao) + + target := testResource("Target", "t-1", "target-1") + mockDao.addResource(target) + + resource := testResource("Parent", "p-1", "parent-1") + refs := map[string][]openapi.ObjectReference{ + "dep": { + {Id: strPtr("t-1"), Kind: "Target"}, + }, + } + + result, svcErr := svc.Create(context.Background(), "Parent", resource, refs) + Expect(svcErr).To(BeNil()) + Expect(result).ToNot(BeNil()) + Expect(mockDao.replaceRefsCalled).To(BeTrue()) + Expect(mockDao.lastReplacedRefs).To(HaveLen(1)) + Expect(mockDao.lastReplacedRefs[0].RefType).To(Equal("dep")) + Expect(mockDao.lastReplacedRefs[0].TargetID).To(Equal("t-1")) +} + +func TestResourceService_Create_RefTargetNotFound_Returns400(t *testing.T) { + RegisterTestingT(t) + setupRefDescriptors() + + mockDao := newMockResourceDao() + svc, _, _ := newTestResourceService(mockDao) + + resource := testResource("Parent", "p-1", "parent-1") + refs := map[string][]openapi.ObjectReference{ + "dep": { + {Id: strPtr("nonexistent"), Kind: "Target"}, + }, + } + + result, svcErr := svc.Create(context.Background(), "Parent", resource, refs) + Expect(result).To(BeNil()) + Expect(svcErr).ToNot(BeNil()) + Expect(svcErr.HTTPCode).To(Equal(400)) + Expect(svcErr.Reason).To(ContainSubstring("not found")) +} + +func TestResourceService_Patch_ReferencesReplaced(t *testing.T) { + RegisterTestingT(t) + setupOptionalRefDescriptors() + + mockDao := newMockResourceDao() + svc, _, _ := newTestResourceService(mockDao) + + existing := testResource("Parent", "p-1", "parent-1") + mockDao.addResource(existing) + + target := testResource("Target", "t-1", "target-1") + mockDao.addResource(target) + + patch := &api.ResourcePatch{ + References: map[string][]openapi.ObjectReference{ + "dep": { + {Id: strPtr("t-1"), Kind: "Target"}, + }, + }, + } + + result, svcErr := svc.Patch(context.Background(), "Parent", "p-1", patch) + Expect(svcErr).To(BeNil()) + Expect(result).ToNot(BeNil()) + Expect(mockDao.replaceRefsCalled).To(BeTrue()) + Expect(mockDao.lastReplacedRefs).To(HaveLen(1)) +} + +func TestResourceService_Patch_NilReferences_NoOp(t *testing.T) { + RegisterTestingT(t) + setupOptionalRefDescriptors() + + mockDao := newMockResourceDao() + svc, _, _ := newTestResourceService(mockDao) + + existing := testResource("Parent", "p-1", "parent-1") + mockDao.addResource(existing) + + patch := &api.ResourcePatch{ + References: nil, + } + + _, svcErr := svc.Patch(context.Background(), "Parent", "p-1", patch) + Expect(svcErr).To(BeNil()) + Expect(mockDao.replaceRefsCalled).To(BeFalse()) +} + +func TestResourceService_Patch_EmptyMapClearsAndValidatesMin(t *testing.T) { + RegisterTestingT(t) + setupRefDescriptors() // Min: 1 on "dep" + + mockDao := newMockResourceDao() + svc, _, _ := newTestResourceService(mockDao) + + existing := testResource("Parent", "p-1", "parent-1") + mockDao.addResource(existing) + + patch := &api.ResourcePatch{ + References: map[string][]openapi.ObjectReference{}, + } + + result, svcErr := svc.Patch(context.Background(), "Parent", "p-1", patch) + Expect(result).To(BeNil()) + Expect(svcErr).ToNot(BeNil()) + Expect(svcErr.HTTPCode).To(Equal(400)) + Expect(svcErr.Reason).To(ContainSubstring("required reference type")) +} + +func TestResourceService_Delete_ReferencedResource_Returns409(t *testing.T) { + RegisterTestingT(t) + setupRefDescriptors() + + mockDao := newMockResourceDao() + svc, _, _ := newTestResourceService(mockDao) + + target := testResource("Target", "t-1", "target-1") + mockDao.addResource(target) + mockDao.findReferencersResult = []api.ResourceSummary{ + {Kind: "Parent", Name: "parent-1"}, + } + + result, svcErr := svc.Delete(context.Background(), "Target", "t-1") + Expect(result).To(BeNil()) + Expect(svcErr).ToNot(BeNil()) + Expect(svcErr.HTTPCode).To(Equal(409)) + Expect(svcErr.Reason).To(ContainSubstring("Parent")) + Expect(svcErr.Reason).To(ContainSubstring("parent-1")) +} + +func TestResourceService_Delete_UnreferencedResource_Succeeds(t *testing.T) { + RegisterTestingT(t) + setupRefDescriptors() + + mockDao := newMockResourceDao() + svc, _, _ := newTestResourceService(mockDao) + + target := testResource("Target", "t-1", "target-1") + mockDao.addResource(target) + mockDao.findReferencersResult = nil + + result, svcErr := svc.Delete(context.Background(), "Target", "t-1") + Expect(svcErr).To(BeNil()) + Expect(result).ToNot(BeNil()) + Expect(result.DeletedTime).ToNot(BeNil()) } diff --git a/pkg/services/types.go b/pkg/services/types.go index 1f51ae74..2591f703 100755 --- a/pkg/services/types.go +++ b/pkg/services/types.go @@ -11,12 +11,14 @@ import ( // ListArguments are arguments relevant for listing objects. // This struct is common to all service List funcs in this package type ListArguments struct { - Search string - Preloads []string - Order []string - Fields []string - Page int - Size int64 + Search string + RefType string + RefTargetID string + Preloads []string + Order []string + Fields []string + Size int64 + Page int } // MaxListSize defines the PostgreSQL WHERE IN clause parameter limit (~65500). @@ -133,5 +135,14 @@ func NewListArguments(params url.Values) (*ListArguments, *errors.ServiceError) // Parse fields parameter using shared logic listArgs.Fields = ParseFieldsParameter(params) + listArgs.RefType = strings.TrimSpace(params.Get("ref_type")) + listArgs.RefTargetID = strings.TrimSpace(params.Get("ref_target_id")) + if (listArgs.RefType == "") != (listArgs.RefTargetID == "") { + return nil, errors.New( + errors.CodeValidationFormat, + "ref_type and ref_target_id must be provided together", + ) + } + return listArgs, nil } diff --git a/pkg/services/types_test.go b/pkg/services/types_test.go index fb1192bf..db20150a 100644 --- a/pkg/services/types_test.go +++ b/pkg/services/types_test.go @@ -197,6 +197,41 @@ func TestNewListArguments_Fields(t *testing.T) { } } +func TestNewListArguments_RefTypeWithoutTargetID_Returns400(t *testing.T) { + RegisterTestingT(t) + + params := url.Values{"ref_type": []string{"dep"}} + listArgs, err := NewListArguments(params) + Expect(listArgs).To(BeNil()) + Expect(err).ToNot(BeNil()) + Expect(err.HTTPCode).To(Equal(400)) + Expect(err.Reason).To(ContainSubstring("ref_type and ref_target_id must be provided together")) +} + +func TestNewListArguments_RefTargetIDWithoutRefType_Returns400(t *testing.T) { + RegisterTestingT(t) + + params := url.Values{"ref_target_id": []string{"some-id"}} + listArgs, err := NewListArguments(params) + Expect(listArgs).To(BeNil()) + Expect(err).ToNot(BeNil()) + Expect(err.HTTPCode).To(Equal(400)) + Expect(err.Reason).To(ContainSubstring("ref_type and ref_target_id must be provided together")) +} + +func TestNewListArguments_RefTypePairValid(t *testing.T) { + RegisterTestingT(t) + + params := url.Values{ + "ref_type": []string{"dep"}, + "ref_target_id": []string{"target-1"}, + } + listArgs, err := NewListArguments(params) + Expect(err).To(BeNil()) + Expect(listArgs.RefType).To(Equal("dep")) + Expect(listArgs.RefTargetID).To(Equal("target-1")) +} + // TestNewListArguments_Validation tests pagination parameter validation (HYPERFLEET-1241) func TestNewListArguments_Validation(t *testing.T) { RegisterTestingT(t) diff --git a/test/integration/channels_test.go b/test/integration/channels_test.go index 7e3441c8..02406459 100644 --- a/test/integration/channels_test.go +++ b/test/integration/channels_test.go @@ -18,7 +18,7 @@ import ( func createChannel(t *testing.T, svc services.ResourceService, name string) *api.Resource { t.Helper() channel := newChannelResource(name) - created, err := svc.Create(t.Context(), "Channel", channel) + created, err := svc.Create(t.Context(), "Channel", channel, nil) if err != nil { t.Fatalf("Failed to create channel: %v", err) } @@ -36,7 +36,7 @@ func TestChannelCreate(t *testing.T) { // Attempt to create duplicate - should fail duplicate := newChannelResource(channelName) - _, svcErr := svc.Create(t.Context(), "Channel", duplicate) + _, svcErr := svc.Create(t.Context(), "Channel", duplicate, nil) Expect(svcErr).ToNot(BeNil(), "duplicate channel name should fail") Expect(svcErr.HTTPCode).To(Equal(409)) }) @@ -45,7 +45,7 @@ func TestChannelCreate(t *testing.T) { svc, _ := setupResourceTest(t) channel := newChannelResource("") - _, svcErr := svc.Create(t.Context(), "Channel", channel) + _, svcErr := svc.Create(t.Context(), "Channel", channel, nil) Expect(svcErr).ToNot(BeNil(), "empty channel name should fail") Expect(svcErr.HTTPCode).To(Equal(400)) }) @@ -63,7 +63,7 @@ func TestChannelCreate(t *testing.T) { var err error channel.Labels, err = json.Marshal(labels) Expect(err).To(BeNil(), "should marshal labels") - createdChannel, svcErr := svc.Create(t.Context(), "Channel", channel) + createdChannel, svcErr := svc.Create(t.Context(), "Channel", channel, nil) Expect(svcErr).To(BeNil()) Expect(createdChannel.Labels).NotTo(BeNil()) diff --git a/test/integration/resource_conditions_test.go b/test/integration/resource_conditions_test.go index c51c4b42..98138fa6 100644 --- a/test/integration/resource_conditions_test.go +++ b/test/integration/resource_conditions_test.go @@ -17,7 +17,7 @@ func TestResourceConditions_UpdateAndPreload(t *testing.T) { svc, h := setupResourceTest(t) ctx := context.Background() - channel, svcErr := svc.Create(ctx, "Channel", newChannelResource("cond-test-update")) + channel, svcErr := svc.Create(ctx, "Channel", newChannelResource("cond-test-update"), nil) Expect(svcErr).To(BeNil()) condDao := dao.NewResourceConditionDao(h.DBFactory) @@ -65,7 +65,7 @@ func TestResourceConditions_LastTransitionTimePreserved(t *testing.T) { svc, h := setupResourceTest(t) ctx := context.Background() - channel, svcErr := svc.Create(ctx, "Channel", newChannelResource("cond-test-transition")) + channel, svcErr := svc.Create(ctx, "Channel", newChannelResource("cond-test-transition"), nil) Expect(svcErr).To(BeNil()) condDao := dao.NewResourceConditionDao(h.DBFactory) @@ -140,7 +140,7 @@ func TestResourceConditions_AtomicReplace(t *testing.T) { svc, h := setupResourceTest(t) ctx := context.Background() - channel, svcErr := svc.Create(ctx, "Channel", newChannelResource("cond-test-replace")) + channel, svcErr := svc.Create(ctx, "Channel", newChannelResource("cond-test-replace"), nil) Expect(svcErr).To(BeNil()) condDao := dao.NewResourceConditionDao(h.DBFactory) @@ -176,7 +176,7 @@ func TestResourceConditions_SaveDoesNotTouchConditions(t *testing.T) { svc, h := setupResourceTest(t) ctx := context.Background() - channel, svcErr := svc.Create(ctx, "Channel", newChannelResource("cond-test-save")) + channel, svcErr := svc.Create(ctx, "Channel", newChannelResource("cond-test-save"), nil) Expect(svcErr).To(BeNil()) condDao := dao.NewResourceConditionDao(h.DBFactory) @@ -209,10 +209,10 @@ func TestResourceConditions_GetByOwnerPreload(t *testing.T) { svc, h := setupResourceTest(t) ctx := context.Background() - channel, svcErr := svc.Create(ctx, "Channel", newChannelResource("cond-test-owner-preload")) + channel, svcErr := svc.Create(ctx, "Channel", newChannelResource("cond-test-owner-preload"), nil) Expect(svcErr).To(BeNil()) - version, svcErr := svc.Create(ctx, "Version", newVersionResource("v1", channel.ID)) + version, svcErr := svc.Create(ctx, "Version", newVersionResource("v1", channel.ID), nil) Expect(svcErr).To(BeNil()) condDao := dao.NewResourceConditionDao(h.DBFactory) @@ -238,7 +238,7 @@ func TestResourceConditions_ClearWithEmptySlice(t *testing.T) { svc, h := setupResourceTest(t) ctx := context.Background() - channel, svcErr := svc.Create(ctx, "Channel", newChannelResource("cond-test-clear")) + channel, svcErr := svc.Create(ctx, "Channel", newChannelResource("cond-test-clear"), nil) Expect(svcErr).To(BeNil()) condDao := dao.NewResourceConditionDao(h.DBFactory) @@ -266,7 +266,7 @@ func TestResourceConditions_CreatedTimePreserved(t *testing.T) { svc, h := setupResourceTest(t) ctx := context.Background() - channel, svcErr := svc.Create(ctx, "Channel", newChannelResource("cond-test-created")) + channel, svcErr := svc.Create(ctx, "Channel", newChannelResource("cond-test-created"), nil) Expect(svcErr).To(BeNil()) condDao := dao.NewResourceConditionDao(h.DBFactory) diff --git a/test/integration/resource_delete_test.go b/test/integration/resource_delete_test.go index 9735a045..27afd75d 100644 --- a/test/integration/resource_delete_test.go +++ b/test/integration/resource_delete_test.go @@ -32,14 +32,14 @@ func TestResourceDelete_ParentChildWithRequiredAdapters(t *testing.T) { // Create Channel (parent) channelName := fmt.Sprintf("test-delete-channel-%s", uuid.NewString()[:8]) channel := newChannelResource(channelName) - createdChannel, svcErr := svc.Create(t.Context(), "Channel", channel) + createdChannel, svcErr := svc.Create(t.Context(), "Channel", channel, nil) Expect(svcErr).To(BeNil(), "Channel creation should succeed") Expect(createdChannel.ID).NotTo(BeEmpty()) // Create Version (child with RequiredAdapters) versionName := fmt.Sprintf("v1.0.0-%s", uuid.NewString()[:8]) version := newVersionResource(versionName, createdChannel.ID) - createdVersion, svcErr := svc.Create(t.Context(), "Version", version) + createdVersion, svcErr := svc.Create(t.Context(), "Version", version, nil) Expect(svcErr).To(BeNil(), "Version creation should succeed") Expect(createdVersion.ID).NotTo(BeEmpty()) @@ -125,7 +125,7 @@ func TestResourceDelete_ParentChildWithRequiredAdapters(t *testing.T) { // Create Channel without children channelName := fmt.Sprintf("test-delete-orphan-%s", uuid.NewString()[:8]) channel := newChannelResource(channelName) - createdChannel, svcErr := svc.Create(t.Context(), "Channel", channel) + createdChannel, svcErr := svc.Create(t.Context(), "Channel", channel, nil) Expect(svcErr).To(BeNil(), "Channel creation should succeed") // Delete Channel (no children, no RequiredAdapters) - should be HARD-DELETED @@ -151,13 +151,13 @@ func TestResourceDelete_ParentChildWithRequiredAdapters(t *testing.T) { // Create Channel channelName := fmt.Sprintf("test-restrict-%s", uuid.NewString()[:8]) channel := newChannelResource(channelName) - createdChannel, svcErr := svc.Create(t.Context(), "Channel", channel) + createdChannel, svcErr := svc.Create(t.Context(), "Channel", channel, nil) Expect(svcErr).To(BeNil()) // Create active Version versionName := fmt.Sprintf("v1.0.0-%s", uuid.NewString()[:8]) version := newVersionResource(versionName, createdChannel.ID) - _, svcErr = svc.Create(t.Context(), "Version", version) + _, svcErr = svc.Create(t.Context(), "Version", version, nil) Expect(svcErr).To(BeNil()) // Try to delete Channel - should fail (OnParentDelete=Restrict) @@ -191,13 +191,13 @@ func TestResourceDelete_WithoutRequiredAdapters(t *testing.T) { // Create Channel channelName := fmt.Sprintf("test-harddelete-%s", uuid.NewString()[:8]) channel := newChannelResource(channelName) - createdChannel, svcErr := svc.Create(t.Context(), "Channel", channel) + createdChannel, svcErr := svc.Create(t.Context(), "Channel", channel, nil) Expect(svcErr).To(BeNil()) // Create Version (no RequiredAdapters) versionName := fmt.Sprintf("v1.0.0-%s", uuid.NewString()[:8]) version := newVersionResource(versionName, createdChannel.ID) - createdVersion, svcErr := svc.Create(t.Context(), "Version", version) + createdVersion, svcErr := svc.Create(t.Context(), "Version", version, nil) Expect(svcErr).To(BeNil()) // Delete Version - should be HARD-DELETED (no RequiredAdapters) diff --git a/test/integration/resource_force_delete_test.go b/test/integration/resource_force_delete_test.go index 28e473b0..b281eac1 100644 --- a/test/integration/resource_force_delete_test.go +++ b/test/integration/resource_force_delete_test.go @@ -18,7 +18,7 @@ func createVersionForChannel( ) { t.Helper() version := newVersionResource(name, channelID) - _, err := svc.Create(t.Context(), "Version", version) + _, err := svc.Create(t.Context(), "Version", version, nil) if err != nil { t.Fatalf("Failed to create version: %v", err) } @@ -221,7 +221,7 @@ func TestResourceForceDeleteHTTP(t *testing.T) { token := test.GetAccessTokenFromContext(ctx) wifConfig := newWifConfigResource(fmt.Sprintf("fd-wif-%s", prefix)) - created, createErr := svc.Create(t.Context(), "WifConfig", wifConfig) + created, createErr := svc.Create(t.Context(), "WifConfig", wifConfig, nil) Expect(createErr).To(BeNil()) markFinalizing(t, h, created.ID) diff --git a/test/integration/resource_references_test.go b/test/integration/resource_references_test.go new file mode 100644 index 00000000..25b11a2b --- /dev/null +++ b/test/integration/resource_references_test.go @@ -0,0 +1,426 @@ +package integration + +import ( + "fmt" + "sync" + "testing" + + "github.com/google/uuid" + . "github.com/onsi/gomega" + + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/registry" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/services" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/util" + "github.com/openshift-hyperfleet/hyperfleet-api/test" +) + +var registerRefsOnce sync.Once + +// registerRefTestDescriptors registers entity descriptors needed for resource +// reference integration tests. Must be called before setupResourceTest so +// that the registry knows about these kinds when services/DAO try to resolve them. +func registerRefTestDescriptors() { + registerRefsOnce.Do(func() { + registry.Register(registry.EntityDescriptor{ + Kind: "RefTarget", + Plural: "reftargets", + }) + registry.Register(registry.EntityDescriptor{ + Kind: "RefSource", + Plural: "refsources", + References: []registry.ReferenceDescriptor{ + {RefType: "dep", TargetKind: "RefTarget", Min: 1, Max: 1}, + }, + }) + registry.Register(registry.EntityDescriptor{ + Kind: "OptSource", + Plural: "optsources", + References: []registry.ReferenceDescriptor{ + {RefType: "link", TargetKind: "RefTarget", Min: 0, Max: 0}, + }, + }) + }) +} + +func setupRefTest(t *testing.T) (services.ResourceService, *test.Helper) { + t.Helper() + registerRefTestDescriptors() + return setupResourceTest(t) +} + +func newRefTestResource(kind, name string) *api.Resource { + return &api.Resource{ + Kind: kind, + Name: name, + Spec: []byte(`{"key": "value"}`), + CreatedBy: "test@example.com", + UpdatedBy: "test@example.com", + } +} + +// makeRefs builds a reference map with a single ref type and target(s). +func makeRefs(refType string, targets ...struct{ id, kind string }) map[string][]openapi.ObjectReference { + refs := make([]openapi.ObjectReference, len(targets)) + for i, t := range targets { + refs[i] = openapi.ObjectReference{Id: util.ToPtr(t.id), Kind: t.kind} + } + return map[string][]openapi.ObjectReference{refType: refs} +} + +// --- Create --- + +func TestResourceReferences_CreateWithValidRef(t *testing.T) { + RegisterTestingT(t) + svc, _ := setupRefTest(t) + + targetName := fmt.Sprintf("target-%s", uuid.NewString()[:8]) + target, svcErr := svc.Create(t.Context(), "RefTarget", newRefTestResource("RefTarget", targetName), nil) + Expect(svcErr).To(BeNil(), "creating target should succeed") + + sourceName := fmt.Sprintf("source-%s", uuid.NewString()[:8]) + refs := makeRefs("dep", struct{ id, kind string }{target.ID, "RefTarget"}) + source, svcErr := svc.Create(t.Context(), "RefSource", newRefTestResource("RefSource", sourceName), refs) + Expect(svcErr).To(BeNil(), "creating source with valid ref should succeed") + Expect(source.ID).NotTo(BeEmpty()) + + // GET should return the resource with references preloaded. + retrieved, svcErr := svc.Get(t.Context(), "RefSource", source.ID) + Expect(svcErr).To(BeNil(), "get source should succeed") + Expect(retrieved.References).To(HaveLen(1), "should have exactly one reference row") + Expect(retrieved.References[0].RefType).To(Equal("dep")) + Expect(retrieved.References[0].TargetID).To(Equal(target.ID)) + Expect(retrieved.References[0].TargetKind).To(Equal("RefTarget")) + Expect(retrieved.References[0].SourceID).To(Equal(source.ID)) +} + +func TestResourceReferences_CreateMissingRequiredRef(t *testing.T) { + RegisterTestingT(t) + svc, _ := setupRefTest(t) + + sourceName := fmt.Sprintf("source-noreq-%s", uuid.NewString()[:8]) + // RefSource has Min=1 on "dep", so creating without refs should fail. + _, svcErr := svc.Create(t.Context(), "RefSource", newRefTestResource("RefSource", sourceName), nil) + Expect(svcErr).NotTo(BeNil(), "create without required ref should fail") + Expect(svcErr.HTTPCode).To(Equal(400)) +} + +func TestResourceReferences_CreateRefToNonExistentTarget(t *testing.T) { + RegisterTestingT(t) + svc, _ := setupRefTest(t) + + fakeID := uuid.NewString() + sourceName := fmt.Sprintf("source-ghost-%s", uuid.NewString()[:8]) + refs := makeRefs("dep", struct{ id, kind string }{fakeID, "RefTarget"}) + + _, svcErr := svc.Create(t.Context(), "RefSource", newRefTestResource("RefSource", sourceName), refs) + Expect(svcErr).NotTo(BeNil(), "ref to non-existent target should fail") + Expect(svcErr.HTTPCode).To(Equal(400)) +} + +func TestResourceReferences_CreateTooManyRefs(t *testing.T) { + RegisterTestingT(t) + svc, _ := setupRefTest(t) + + // Create two targets. + target1, svcErr := svc.Create(t.Context(), "RefTarget", + newRefTestResource("RefTarget", fmt.Sprintf("target1-%s", uuid.NewString()[:8])), nil) + Expect(svcErr).To(BeNil()) + + target2, svcErr := svc.Create(t.Context(), "RefTarget", + newRefTestResource("RefTarget", fmt.Sprintf("target2-%s", uuid.NewString()[:8])), nil) + Expect(svcErr).To(BeNil()) + + // RefSource has Max=1 on "dep" — supplying 2 should fail. + sourceName := fmt.Sprintf("source-toomany-%s", uuid.NewString()[:8]) + refs := makeRefs("dep", + struct{ id, kind string }{target1.ID, "RefTarget"}, + struct{ id, kind string }{target2.ID, "RefTarget"}, + ) + + _, svcErr = svc.Create(t.Context(), "RefSource", newRefTestResource("RefSource", sourceName), refs) + Expect(svcErr).NotTo(BeNil(), "exceeding Max refs should fail") + Expect(svcErr.HTTPCode).To(Equal(400)) +} + +// --- Patch --- + +func TestResourceReferences_PatchReplacesAtomically(t *testing.T) { + RegisterTestingT(t) + svc, _ := setupRefTest(t) + + target1, svcErr := svc.Create(t.Context(), "RefTarget", + newRefTestResource("RefTarget", fmt.Sprintf("target1-%s", uuid.NewString()[:8])), nil) + Expect(svcErr).To(BeNil()) + + target2, svcErr := svc.Create(t.Context(), "RefTarget", + newRefTestResource("RefTarget", fmt.Sprintf("target2-%s", uuid.NewString()[:8])), nil) + Expect(svcErr).To(BeNil()) + + // Create source pointing to target1. + sourceName := fmt.Sprintf("source-swap-%s", uuid.NewString()[:8]) + refs := makeRefs("dep", struct{ id, kind string }{target1.ID, "RefTarget"}) + source, svcErr := svc.Create(t.Context(), "RefSource", newRefTestResource("RefSource", sourceName), refs) + Expect(svcErr).To(BeNil()) + + // Patch to point to target2. + patchRefs := map[string][]openapi.ObjectReference{ + "dep": {{Id: util.ToPtr(target2.ID), Kind: "RefTarget"}}, + } + _, svcErr = svc.Patch(t.Context(), "RefSource", source.ID, &api.ResourcePatch{ + References: patchRefs, + }) + Expect(svcErr).To(BeNil(), "patch should succeed") + + // GET should now show only target2. + retrieved, svcErr := svc.Get(t.Context(), "RefSource", source.ID) + Expect(svcErr).To(BeNil()) + Expect(retrieved.References).To(HaveLen(1)) + Expect(retrieved.References[0].TargetID).To(Equal(target2.ID)) +} + +func TestResourceReferences_PatchNilRefsIsNoOp(t *testing.T) { + RegisterTestingT(t) + svc, _ := setupRefTest(t) + + target, svcErr := svc.Create(t.Context(), "RefTarget", + newRefTestResource("RefTarget", fmt.Sprintf("target-noop-%s", uuid.NewString()[:8])), nil) + Expect(svcErr).To(BeNil()) + + sourceName := fmt.Sprintf("source-noop-%s", uuid.NewString()[:8]) + refs := makeRefs("dep", struct{ id, kind string }{target.ID, "RefTarget"}) + source, svcErr := svc.Create(t.Context(), "RefSource", newRefTestResource("RefSource", sourceName), refs) + Expect(svcErr).To(BeNil()) + + // Patch with spec change only — no references field (nil). + _, svcErr = svc.Patch(t.Context(), "RefSource", source.ID, &api.ResourcePatch{ + Spec: map[string]interface{}{"key": "updated"}, + }) + Expect(svcErr).To(BeNil(), "patch spec-only should succeed") + + // References should be unchanged. + retrieved, svcErr := svc.Get(t.Context(), "RefSource", source.ID) + Expect(svcErr).To(BeNil()) + Expect(retrieved.References).To(HaveLen(1)) + Expect(retrieved.References[0].TargetID).To(Equal(target.ID)) +} + +func TestResourceReferences_PatchEmptyMapViolatesMin(t *testing.T) { + RegisterTestingT(t) + svc, _ := setupRefTest(t) + + target, svcErr := svc.Create(t.Context(), "RefTarget", + newRefTestResource("RefTarget", fmt.Sprintf("target-empty-%s", uuid.NewString()[:8])), nil) + Expect(svcErr).To(BeNil()) + + sourceName := fmt.Sprintf("source-empty-%s", uuid.NewString()[:8]) + refs := makeRefs("dep", struct{ id, kind string }{target.ID, "RefTarget"}) + source, svcErr := svc.Create(t.Context(), "RefSource", newRefTestResource("RefSource", sourceName), refs) + Expect(svcErr).To(BeNil()) + + // Patch with empty references map — Min=1 should reject. + _, svcErr = svc.Patch(t.Context(), "RefSource", source.ID, &api.ResourcePatch{ + References: map[string][]openapi.ObjectReference{}, + }) + Expect(svcErr).NotTo(BeNil(), "clearing required refs should fail") + Expect(svcErr.HTTPCode).To(Equal(400)) +} + +// --- Delete --- + +func TestResourceReferences_DeleteTargetWhileReferenced(t *testing.T) { + RegisterTestingT(t) + svc, _ := setupRefTest(t) + + target, svcErr := svc.Create(t.Context(), "RefTarget", + newRefTestResource("RefTarget", fmt.Sprintf("target-block-%s", uuid.NewString()[:8])), nil) + Expect(svcErr).To(BeNil()) + + sourceName := fmt.Sprintf("source-block-%s", uuid.NewString()[:8]) + refs := makeRefs("dep", struct{ id, kind string }{target.ID, "RefTarget"}) + _, svcErr = svc.Create(t.Context(), "RefSource", newRefTestResource("RefSource", sourceName), refs) + Expect(svcErr).To(BeNil()) + + // Attempt to delete target while referenced — expect 409. + _, svcErr = svc.Delete(t.Context(), "RefTarget", target.ID) + Expect(svcErr).NotTo(BeNil(), "delete of referenced target should fail") + Expect(svcErr.HTTPCode).To(Equal(409)) +} + +func TestResourceReferences_DeleteSourceSucceeds(t *testing.T) { + RegisterTestingT(t) + svc, h := setupRefTest(t) + + target, svcErr := svc.Create(t.Context(), "RefTarget", + newRefTestResource("RefTarget", fmt.Sprintf("target-delsrc-%s", uuid.NewString()[:8])), nil) + Expect(svcErr).To(BeNil()) + + sourceName := fmt.Sprintf("source-delsrc-%s", uuid.NewString()[:8]) + refs := makeRefs("dep", struct{ id, kind string }{target.ID, "RefTarget"}) + source, svcErr := svc.Create(t.Context(), "RefSource", newRefTestResource("RefSource", sourceName), refs) + Expect(svcErr).To(BeNil()) + + // Deleting the source should succeed — ON DELETE CASCADE cleans ref rows. + deleted, svcErr := svc.Delete(t.Context(), "RefSource", source.ID) + Expect(svcErr).To(BeNil(), "delete source should succeed") + Expect(deleted.DeletedTime).NotTo(BeNil()) + + // Source row should be gone (hard delete, no required adapters). + dbErr := checkResourceCount(t.Context(), h, []string{source.ID}, 0) + Expect(dbErr).To(BeNil()) + + // Target should still exist. + _, svcErr = svc.Get(t.Context(), "RefTarget", target.ID) + Expect(svcErr).To(BeNil(), "target should still exist after source delete") +} + +// --- List with ref_type filter --- + +func TestResourceReferences_ListByRefTypeAndTarget(t *testing.T) { + RegisterTestingT(t) + svc, _ := setupRefTest(t) + + // Create one target and two sources that reference it, plus one that does not. + target, svcErr := svc.Create(t.Context(), "RefTarget", + newRefTestResource("RefTarget", fmt.Sprintf("target-list-%s", uuid.NewString()[:8])), nil) + Expect(svcErr).To(BeNil()) + + otherTarget, svcErr := svc.Create(t.Context(), "RefTarget", + newRefTestResource("RefTarget", fmt.Sprintf("target-other-%s", uuid.NewString()[:8])), nil) + Expect(svcErr).To(BeNil()) + + // source1 references target + refs1 := makeRefs("dep", struct{ id, kind string }{target.ID, "RefTarget"}) + source1, svcErr := svc.Create(t.Context(), "RefSource", + newRefTestResource("RefSource", fmt.Sprintf("source-list1-%s", uuid.NewString()[:8])), refs1) + Expect(svcErr).To(BeNil()) + + // source2 references target + refs2 := makeRefs("dep", struct{ id, kind string }{target.ID, "RefTarget"}) + source2, svcErr := svc.Create(t.Context(), "RefSource", + newRefTestResource("RefSource", fmt.Sprintf("source-list2-%s", uuid.NewString()[:8])), refs2) + Expect(svcErr).To(BeNil()) + + // source3 references otherTarget — should NOT appear in results. + refs3 := makeRefs("dep", struct{ id, kind string }{otherTarget.ID, "RefTarget"}) + source3, svcErr := svc.Create(t.Context(), "RefSource", + newRefTestResource("RefSource", fmt.Sprintf("source-list3-%s", uuid.NewString()[:8])), refs3) + Expect(svcErr).To(BeNil()) + + args := &services.ListArguments{ + Page: 1, + Size: 100, + RefType: "dep", + RefTargetID: target.ID, + } + list, _, svcErr := svc.List(t.Context(), "RefSource", args) + Expect(svcErr).To(BeNil(), "list with ref filter should succeed") + + // Build set of returned IDs. + foundIDs := make(map[string]bool, len(list)) + for _, item := range list { + foundIDs[item.ID] = true + } + + Expect(foundIDs).To(HaveKey(source1.ID), "source1 should be in results") + Expect(foundIDs).To(HaveKey(source2.ID), "source2 should be in results") + Expect(foundIDs).NotTo(HaveKey(source3.ID), "source3 should NOT be in results") +} + +func TestResourceReferences_ListUnknownRefType(t *testing.T) { + RegisterTestingT(t) + svc, _ := setupRefTest(t) + + // RefSource has ref_type "dep" — querying "unknown" should fail. + args := &services.ListArguments{ + Page: 1, + Size: 20, + RefType: "unknown", + RefTargetID: "some-id", + } + _, _, svcErr := svc.List(t.Context(), "RefSource", args) + Expect(svcErr).NotTo(BeNil(), "unknown ref_type should fail") + Expect(svcErr.HTTPCode).To(Equal(400)) +} + +// --- Optional references --- + +func TestResourceReferences_OptionalRefCreate(t *testing.T) { + RegisterTestingT(t) + svc, _ := setupRefTest(t) + + // OptSource has Min=0 — creating without refs should succeed. + sourceName := fmt.Sprintf("optsource-%s", uuid.NewString()[:8]) + source, svcErr := svc.Create(t.Context(), "OptSource", newRefTestResource("OptSource", sourceName), nil) + Expect(svcErr).To(BeNil(), "optional ref entity should be created without refs") + Expect(source.ID).NotTo(BeEmpty()) + + // Create with a ref should also succeed. + target, svcErr := svc.Create(t.Context(), "RefTarget", + newRefTestResource("RefTarget", fmt.Sprintf("target-opt-%s", uuid.NewString()[:8])), nil) + Expect(svcErr).To(BeNil()) + + source2Name := fmt.Sprintf("optsource2-%s", uuid.NewString()[:8]) + refs := makeRefs("link", struct{ id, kind string }{target.ID, "RefTarget"}) + source2, svcErr := svc.Create(t.Context(), "OptSource", newRefTestResource("OptSource", source2Name), refs) + Expect(svcErr).To(BeNil()) + + retrieved, svcErr := svc.Get(t.Context(), "OptSource", source2.ID) + Expect(svcErr).To(BeNil()) + Expect(retrieved.References).To(HaveLen(1)) + Expect(retrieved.References[0].RefType).To(Equal("link")) + Expect(retrieved.References[0].TargetID).To(Equal(target.ID)) +} + +func TestResourceReferences_ForceDeleteReferencedTarget(t *testing.T) { + RegisterTestingT(t) + svc, h := setupRefTest(t) + prefix := uuid.NewString()[:8] + + target, svcErr := svc.Create(t.Context(), "RefTarget", + newRefTestResource("RefTarget", fmt.Sprintf("target-fd-%s", prefix)), nil) + Expect(svcErr).To(BeNil()) + + refs := makeRefs("dep", struct{ id, kind string }{target.ID, "RefTarget"}) + _, svcErr = svc.Create(t.Context(), "RefSource", + newRefTestResource("RefSource", fmt.Sprintf("source-fd-%s", prefix)), refs) + Expect(svcErr).To(BeNil()) + + _, svcErr = svc.Delete(t.Context(), "RefTarget", target.ID) + Expect(svcErr).ToNot(BeNil(), "regular delete should fail — target is referenced") + Expect(svcErr.HTTPCode).To(Equal(409)) + + markFinalizing(t, h, target.ID) + + svcErr = svc.ForceDelete(t.Context(), "RefTarget", target.ID, "integration test cleanup") + Expect(svcErr).To(BeNil(), "force-delete should bypass reference restriction") + + _, getErr := svc.Get(t.Context(), "RefTarget", target.ID) + Expect(getErr).ToNot(BeNil()) + Expect(getErr.HTTPCode).To(Equal(404), "target should be gone after force-delete") +} + +func TestResourceReferences_PatchClearOptionalRefs(t *testing.T) { + RegisterTestingT(t) + svc, _ := setupRefTest(t) + + target, svcErr := svc.Create(t.Context(), "RefTarget", + newRefTestResource("RefTarget", fmt.Sprintf("target-clropt-%s", uuid.NewString()[:8])), nil) + Expect(svcErr).To(BeNil()) + + sourceName := fmt.Sprintf("optsource-clr-%s", uuid.NewString()[:8]) + refs := makeRefs("link", struct{ id, kind string }{target.ID, "RefTarget"}) + source, svcErr := svc.Create(t.Context(), "OptSource", newRefTestResource("OptSource", sourceName), refs) + Expect(svcErr).To(BeNil()) + + // Patch with empty references map — Min=0 should allow clearing. + _, svcErr = svc.Patch(t.Context(), "OptSource", source.ID, &api.ResourcePatch{ + References: map[string][]openapi.ObjectReference{}, + }) + Expect(svcErr).To(BeNil(), "clearing optional refs should succeed") + + retrieved, svcErr := svc.Get(t.Context(), "OptSource", source.ID) + Expect(svcErr).To(BeNil()) + Expect(retrieved.References).To(BeEmpty(), "references should be cleared") +} diff --git a/test/integration/root_resources_test.go b/test/integration/root_resources_test.go index a0298df4..36cc5bc4 100644 --- a/test/integration/root_resources_test.go +++ b/test/integration/root_resources_test.go @@ -268,7 +268,7 @@ func TestFlatChildRouteList(t *testing.T) { ch := createChannel(t, svc, fmt.Sprintf("flat-parent-%s", uuid.NewString()[:8])) v1Name := fmt.Sprintf("flat-v1-%s", uuid.NewString()[:8]) v1 := newVersionResource(v1Name, ch.ID) - created1, svcErr := svc.Create(t.Context(), "Version", v1) + created1, svcErr := svc.Create(t.Context(), "Version", v1, nil) Expect(svcErr).To(BeNil()) resp, err := rootResourceRequest(ctx). @@ -296,7 +296,7 @@ func TestFlatChildRouteGetByID(t *testing.T) { ch := createChannel(t, svc, fmt.Sprintf("flat-get-parent-%s", uuid.NewString()[:8])) v := newVersionResource(fmt.Sprintf("flat-get-v-%s", uuid.NewString()[:8]), ch.ID) - created, svcErr := svc.Create(t.Context(), "Version", v) + created, svcErr := svc.Create(t.Context(), "Version", v, nil) Expect(svcErr).To(BeNil()) resp, err := rootResourceRequest(ctx). @@ -319,7 +319,7 @@ func TestFlatChildRoutePatch(t *testing.T) { ch := createChannel(t, svc, fmt.Sprintf("flat-patch-parent-%s", uuid.NewString()[:8])) v := newVersionResource(fmt.Sprintf("flat-patch-v-%s", uuid.NewString()[:8]), ch.ID) - created, svcErr := svc.Create(t.Context(), "Version", v) + created, svcErr := svc.Create(t.Context(), "Version", v, nil) Expect(svcErr).To(BeNil()) patchSpec := map[string]interface{}{ @@ -351,7 +351,7 @@ func TestFlatChildRouteDelete(t *testing.T) { ch := createChannel(t, svc, fmt.Sprintf("flat-del-parent-%s", uuid.NewString()[:8])) v := newVersionResource(fmt.Sprintf("flat-del-v-%s", uuid.NewString()[:8]), ch.ID) - _, svcErr := svc.Create(t.Context(), "Version", v) + _, svcErr := svc.Create(t.Context(), "Version", v, nil) Expect(svcErr).To(BeNil()) retrieved, getErr := svc.Get(t.Context(), "Version", v.ID) diff --git a/test/integration/versions_test.go b/test/integration/versions_test.go index b0a81a9b..1f879195 100644 --- a/test/integration/versions_test.go +++ b/test/integration/versions_test.go @@ -20,7 +20,7 @@ func createTestChannel(t *testing.T, svc services.ResourceService) *api.Resource channelName := fmt.Sprintf("channel-%s", uniqueSuffix) channel := newChannelResource(channelName) - created, err := svc.Create(t.Context(), "Channel", channel) + created, err := svc.Create(t.Context(), "Channel", channel, nil) if err != nil { t.Fatalf("Failed to create channel: %v", err) } @@ -30,7 +30,7 @@ func createTestChannel(t *testing.T, svc services.ResourceService) *api.Resource func createTestVersion(t *testing.T, svc services.ResourceService, name, channelID string) *api.Resource { t.Helper() version := newVersionResource(name, channelID) - created, err := svc.Create(t.Context(), "Version", version) + created, err := svc.Create(t.Context(), "Version", version, nil) if err != nil { t.Fatalf("Failed to create version: %v", err) } @@ -41,7 +41,7 @@ func expectCreateError(t *testing.T, svc services.ResourceService, resource *api.Resource, expectedCode int, msg string, ) { t.Helper() - _, svcErr := svc.Create(t.Context(), resource.Kind, resource) + _, svcErr := svc.Create(t.Context(), resource.Kind, resource, nil) Expect(svcErr).ToNot(BeNil(), msg) Expect(svcErr.HTTPCode).To(Equal(expectedCode)) } @@ -121,7 +121,7 @@ func TestVersionCreate(t *testing.T) { var err error version.Labels, err = json.Marshal(labels) Expect(err).To(BeNil(), "should marshal labels") - createdVersion, svcErr := svc.Create(t.Context(), "Version", version) + createdVersion, svcErr := svc.Create(t.Context(), "Version", version, nil) Expect(svcErr).To(BeNil()) Expect(createdVersion.Labels).NotTo(BeNil()) @@ -308,7 +308,7 @@ func TestVersionList(t *testing.T) { var err error version1.Labels, err = json.Marshal(labels) Expect(err).To(BeNil(), "should marshal labels") - created1, svcErr := svc.Create(t.Context(), "Version", version1) + created1, svcErr := svc.Create(t.Context(), "Version", version1, nil) Expect(svcErr).To(BeNil()) Expect(created1.Labels).NotTo(BeNil()) @@ -316,7 +316,7 @@ func TestVersionList(t *testing.T) { version2 := newVersionResource("version-with-label-2", channel.ID) version2.Labels, err = json.Marshal(labels) Expect(err).To(BeNil(), "should marshal labels") - created2, svcErr := svc.Create(t.Context(), "Version", version2) + created2, svcErr := svc.Create(t.Context(), "Version", version2, nil) Expect(svcErr).To(BeNil()) Expect(created2.Labels).NotTo(BeNil()) From ff7b6f0fb45e6d7ec78c09ade7b46367d3a99641 Mon Sep 17 00:00:00 2001 From: Dmitrii Andreev Date: Wed, 8 Jul 2026 14:41:50 -0500 Subject: [PATCH 2/2] HYPERFLEET-1156 - fix: address review feedback - Reuse convertRefs result in Patch (eliminate duplicate call) - Add Preload Conditions+References to GetForUpdate (fixes Patch response dropping existing refs/conditions) - Add Conditions+References preloads to ListAll for parity - CREATE INDEX IF NOT EXISTS in migration - Add Rollback to migration - Fix doc comment 422 -> 400 - Extract shared extractReferences helper from handlers - Add logging to ClearTargetReferences in forceDeleteResourceTree - Replace raw SQL with GORM query builder in FindReferencers - Add unit tests for duplicate ref target ID and soft-deleted target rejection - Add integration tests for duplicate refs and soft-deleted targets --- pkg/api/presenters/resource.go | 4 +- pkg/api/resource.go | 6 +- pkg/api/resource_reference.go | 2 +- pkg/dao/mocks/resource.go | 2 +- pkg/dao/resource.go | 40 ++++---- .../202607021925_add_resource_references.go | 5 +- pkg/handlers/resource_handler.go | 14 ++- pkg/handlers/root_resource_handler.go | 5 +- pkg/registry/registry.go | 45 +++++++++ pkg/registry/registry_test.go | 96 +++++++++++++++++++ pkg/services/resource.go | 63 +++++------- pkg/services/resource_test.go | 76 ++++++++++++--- test/integration/resource_references_test.go | 56 ++++++++++- test/integration/resource_status_test.go | 4 +- 14 files changed, 327 insertions(+), 91 deletions(-) diff --git a/pkg/api/presenters/resource.go b/pkg/api/presenters/resource.go index 06a68590..3b30db23 100644 --- a/pkg/api/presenters/resource.go +++ b/pkg/api/presenters/resource.go @@ -113,8 +113,8 @@ func PresentResourceList(resources api.ResourceList, paging *api.PagingMeta) ope } } -func presentResourceReferences(refs []api.ResourceReference) map[string][]openapi.ObjectReference { - result := make(map[string][]openapi.ObjectReference) +func presentResourceReferences(refs []api.ResourceReference) api.ReferenceMap { + result := make(api.ReferenceMap) for _, ref := range refs { id := ref.TargetID objRef := openapi.ObjectReference{ diff --git a/pkg/api/resource.go b/pkg/api/resource.go index 46e80e52..dbb0832a 100644 --- a/pkg/api/resource.go +++ b/pkg/api/resource.go @@ -34,10 +34,14 @@ type Resource struct { Generation int32 `json:"generation" gorm:"default:1;not null"` } +// ReferenceMap is the API-level representation of resource references, +// keyed by ref type (e.g. "wif_config") with a list of object references per type. +type ReferenceMap = map[string][]openapi.ObjectReference + type ResourcePatch struct { Spec map[string]interface{} Labels map[string]string - References map[string][]openapi.ObjectReference + References ReferenceMap } type ResourceList []*Resource diff --git a/pkg/api/resource_reference.go b/pkg/api/resource_reference.go index 1821e79c..2c6a5fc7 100644 --- a/pkg/api/resource_reference.go +++ b/pkg/api/resource_reference.go @@ -13,7 +13,7 @@ func (ResourceReference) TableName() string { return "resource_references" } -// ResourceSummary carries kind + name for error messages (e.g. FindReferencers 409). +// ResourceSummary carries kind + name for error messages (e.g. FindReferencer 409). type ResourceSummary struct { Kind string Name string diff --git a/pkg/dao/mocks/resource.go b/pkg/dao/mocks/resource.go index 53824cea..0f369e7a 100644 --- a/pkg/dao/mocks/resource.go +++ b/pkg/dao/mocks/resource.go @@ -127,7 +127,7 @@ func (d *resourceDaoMock) ReplaceReferences(_ context.Context, _ string, _ []api return nil } -func (d *resourceDaoMock) FindReferencers(_ context.Context, _ string) ([]api.ResourceSummary, error) { +func (d *resourceDaoMock) FindReferencer(_ context.Context, _ string) (*api.ResourceSummary, error) { return nil, nil } diff --git a/pkg/dao/resource.go b/pkg/dao/resource.go index 9a6d3e45..051570dd 100644 --- a/pkg/dao/resource.go +++ b/pkg/dao/resource.go @@ -24,7 +24,7 @@ type ResourceDao interface { FindByKindAndOwnerForUpdate(ctx context.Context, kind, ownerID string) (api.ResourceList, error) GetByID(ctx context.Context, id string) (*api.Resource, error) ReplaceReferences(ctx context.Context, sourceID string, refs []api.ResourceReference) error - FindReferencers(ctx context.Context, targetID string) ([]api.ResourceSummary, error) + FindReferencer(ctx context.Context, targetID string) (*api.ResourceSummary, error) ClearTargetReferences(ctx context.Context, targetID string) error FindSourceIDsByRef(ctx context.Context, refType, targetID string) ([]string, error) } @@ -52,8 +52,9 @@ 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( - &resource, "kind = ? AND id = ?", kind, id).Error; err != nil { + if err := g2.Clauses(clause.Locking{Strength: "UPDATE"}). + Preload("Conditions").Preload("References"). + Take(&resource, "kind = ? AND id = ?", kind, id).Error; err != nil { return nil, err } return &resource, nil @@ -179,6 +180,9 @@ func (d *sqlResourceDao) ReplaceReferences( db.MarkForRollback(ctx, err) return err } + for i := range refs { + refs[i].SourceID = sourceID + } if len(refs) > 0 { if err := g2.Create(&refs).Error; err != nil { db.MarkForRollback(ctx, err) @@ -188,22 +192,26 @@ func (d *sqlResourceDao) ReplaceReferences( return nil } -func (d *sqlResourceDao) FindReferencers( +// FindReferencer returns the first non-deleted resource that references targetID, +// or nil if none exists. Used as an existence check for 409 conflict responses. +func (d *sqlResourceDao) FindReferencer( ctx context.Context, targetID string, -) ([]api.ResourceSummary, error) { - g2 := d.sessionFactory.New(ctx) - var summaries []api.ResourceSummary - if err := g2.Raw(` - SELECT r.kind, r.name - FROM resource_references rr - JOIN resources r ON rr.source_id = r.id - WHERE rr.target_id = ? AND r.deleted_time IS NULL - LIMIT 1`, - targetID, - ).Scan(&summaries).Error; err != nil { +) (*api.ResourceSummary, error) { + g2 := d.sessionFactory.New(ctx) + var summary api.ResourceSummary + err := g2.Model(&api.ResourceReference{}). + Select("resources.kind, resources.name"). + Joins("JOIN resources ON resource_references.source_id = resources.id"). + Where("resource_references.target_id = ? AND resources.deleted_time IS NULL", targetID). + Limit(1). + Scan(&summary).Error + if err != nil { return nil, err } - return summaries, nil + if summary.Kind == "" { + return nil, nil + } + return &summary, nil } // ClearTargetReferences removes all inbound references pointing at targetID. diff --git a/pkg/db/migrations/202607021925_add_resource_references.go b/pkg/db/migrations/202607021925_add_resource_references.go index 1ddbb3ca..868abfdc 100644 --- a/pkg/db/migrations/202607021925_add_resource_references.go +++ b/pkg/db/migrations/202607021925_add_resource_references.go @@ -22,12 +22,15 @@ func addResourceReferences() *gormigrate.Migration { } if err := tx.Exec( - `CREATE INDEX idx_resource_references_target ON resource_references (target_id);`, + `CREATE INDEX IF NOT EXISTS idx_resource_references_target ON resource_references (target_id);`, ).Error; err != nil { return err } return nil }, + Rollback: func(tx *gorm.DB) error { + return tx.Exec(`DROP TABLE IF EXISTS resource_references`).Error + }, } } diff --git a/pkg/handlers/resource_handler.go b/pkg/handlers/resource_handler.go index cfd6d437..8abaf4b9 100644 --- a/pkg/handlers/resource_handler.go +++ b/pkg/handlers/resource_handler.go @@ -65,10 +65,7 @@ func (h *ResourceHandler) Create(w http.ResponseWriter, r *http.Request) { return nil, errors.GeneralError("failed to convert resource: %v", err) } - var refs map[string][]openapi.ObjectReference - if req.References != nil { - refs = *req.References - } + refs := extractReferences(req.References) resource, svcErr := h.service.Create(ctx, h.descriptor.Kind, resource, refs) if svcErr != nil { return nil, svcErr @@ -211,6 +208,15 @@ func convertResourcePatch(req *openapi.ResourcePatchRequest) *api.ResourcePatch return patch } +// extractReferences unwraps the optional references pointer from an API request. +// Returns nil when no references are supplied (nil pointer), or the map value. +func extractReferences(refs *api.ReferenceMap) api.ReferenceMap { + if refs == nil { + return nil + } + return *refs +} + func (h *ResourceHandler) ForceDelete(w http.ResponseWriter, r *http.Request) { var req openapi.ForceDeleteRequest cfg := &handlerConfig{ diff --git a/pkg/handlers/root_resource_handler.go b/pkg/handlers/root_resource_handler.go index dcbb7c54..91862705 100644 --- a/pkg/handlers/root_resource_handler.go +++ b/pkg/handlers/root_resource_handler.go @@ -104,10 +104,7 @@ func (h *RootResourceHandler) Create(w http.ResponseWriter, r *http.Request) { if convErr != nil { return nil, errors.GeneralError("failed to convert resource: %v", convErr) } - var refs map[string][]openapi.ObjectReference - if req.References != nil { - refs = *req.References - } + refs := extractReferences(req.References) resource, svcErr := h.service.Create(r.Context(), descriptor.Kind, resource, refs) if svcErr != nil { return nil, svcErr diff --git a/pkg/registry/registry.go b/pkg/registry/registry.go index 7deb65d8..5d22c900 100644 --- a/pkg/registry/registry.go +++ b/pkg/registry/registry.go @@ -147,6 +147,51 @@ func Validate() { } } } + + // Detect cycles among required references (Min > 0). + // A cycle means two or more kinds mutually require each other, making + // Create impossible (each resource needs the other to exist first). + // Uses DFS with three-color marking: 0 = unvisited, 1 = in-stack, 2 = done. + color := make(map[string]int, len(descriptors)) + var path []string + var dfs func(kind string) + dfs = func(kind string) { + color[kind] = 1 + path = append(path, kind) + d := descriptors[kind] + for _, ref := range d.References { + if ref.Min <= 0 { + continue + } + switch color[ref.TargetKind] { + case 1: // back-edge → cycle + // Find the cycle start in path for a clear error message. + start := 0 + for i, k := range path { + if k == ref.TargetKind { + start = i + break + } + } + cycle := make([]string, len(path[start:])+1) + copy(cycle, path[start:]) + cycle[len(cycle)-1] = ref.TargetKind + panic(fmt.Sprintf( + "circular required references (Min > 0): %v", + cycle, + )) + case 0: + dfs(ref.TargetKind) + } + } + path = path[:len(path)-1] + color[kind] = 2 + } + for kind := range descriptors { + if color[kind] == 0 { + dfs(kind) + } + } } // ValidateSpecSchemas checks descriptors that set RequireSpecSchema and panics if diff --git a/pkg/registry/registry_test.go b/pkg/registry/registry_test.go index c97606a0..2e764699 100644 --- a/pkg/registry/registry_test.go +++ b/pkg/registry/registry_test.go @@ -474,3 +474,99 @@ func TestValidate_ValidReferences_Success(t *testing.T) { Validate() }).ToNot(Panic()) } + +func TestValidate_CircularRequiredRefs_DirectCycle_Panics(t *testing.T) { + RegisterTestingT(t) + Reset() + + Register(EntityDescriptor{ + Kind: "Alpha", + Plural: "alphas", + References: []ReferenceDescriptor{ + {RefType: "needs_beta", TargetKind: "Beta", Min: 1, Max: 1}, + }, + }) + Register(EntityDescriptor{ + Kind: "Beta", + Plural: "betas", + References: []ReferenceDescriptor{ + {RefType: "needs_alpha", TargetKind: "Alpha", Min: 1, Max: 1}, + }, + }) + + Expect(func() { + Validate() + }).To(PanicWith(ContainSubstring("circular required references"))) +} + +func TestValidate_CircularRequiredRefs_TransitiveCycle_Panics(t *testing.T) { + RegisterTestingT(t) + Reset() + + Register(EntityDescriptor{ + Kind: "A", + Plural: "as", + References: []ReferenceDescriptor{ + {RefType: "to_b", TargetKind: "B", Min: 1, Max: 1}, + }, + }) + Register(EntityDescriptor{ + Kind: "B", + Plural: "bs", + References: []ReferenceDescriptor{ + {RefType: "to_c", TargetKind: "C", Min: 1, Max: 1}, + }, + }) + Register(EntityDescriptor{ + Kind: "C", + Plural: "cs", + References: []ReferenceDescriptor{ + {RefType: "to_a", TargetKind: "A", Min: 1, Max: 1}, + }, + }) + + Expect(func() { + Validate() + }).To(PanicWith(ContainSubstring("circular required references"))) +} + +func TestValidate_OptionalRefCycle_NoPanic(t *testing.T) { + RegisterTestingT(t) + Reset() + + Register(EntityDescriptor{ + Kind: "Foo", + Plural: "foos", + References: []ReferenceDescriptor{ + {RefType: "to_bar", TargetKind: "Bar", Min: 0, Max: 1}, + }, + }) + Register(EntityDescriptor{ + Kind: "Bar", + Plural: "bars", + References: []ReferenceDescriptor{ + {RefType: "to_foo", TargetKind: "Foo", Min: 0, Max: 1}, + }, + }) + + Expect(func() { + Validate() + }).ToNot(Panic()) +} + +func TestValidate_CircularRequiredRefs_SelfCycle_Panics(t *testing.T) { + RegisterTestingT(t) + Reset() + + Register(EntityDescriptor{ + Kind: "Self", + Plural: "selfs", + References: []ReferenceDescriptor{ + {RefType: "self_ref", TargetKind: "Self", Min: 1, Max: 1}, + }, + }) + + Expect(func() { + Validate() + }).To(PanicWith(ContainSubstring("circular required references"))) +} diff --git a/pkg/services/resource.go b/pkg/services/resource.go index 2931841a..3d4089e0 100644 --- a/pkg/services/resource.go +++ b/pkg/services/resource.go @@ -8,7 +8,6 @@ import ( "time" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/dao" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" @@ -22,7 +21,7 @@ import ( type ResourceService interface { Get(ctx context.Context, kind, id string) (*api.Resource, *errors.ServiceError) - Create(ctx context.Context, kind string, resource *api.Resource, refs map[string][]openapi.ObjectReference) (*api.Resource, *errors.ServiceError) //nolint:lll + Create(ctx context.Context, kind string, resource *api.Resource, refs api.ReferenceMap) (*api.Resource, *errors.ServiceError) //nolint:lll Patch(ctx context.Context, kind, id string, patch *api.ResourcePatch) (*api.Resource, *errors.ServiceError) Delete(ctx context.Context, kind, id string) (*api.Resource, *errors.ServiceError) List(ctx context.Context, kind string, args *ListArguments) (api.ResourceList, *api.PagingMeta, *errors.ServiceError) @@ -72,14 +71,12 @@ func (s *sqlResourceService) Get(ctx context.Context, kind, id string) (*api.Res // 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. -// // refs carries the non-ownership references from the API request. nil means "no references -// supplied" (no validation of required refs is skipped only when the entity has no Min>0 -// descriptors). An empty map {} means "clear all references" — Min>0 descriptors will -// reject this with 422. +// supplied" — required ref types (Min > 0) will still be validated and rejected if missing. +// An empty map {} means "clear all references" — Min>0 descriptors will reject this with 400. func (s *sqlResourceService) Create( ctx context.Context, kind string, resource *api.Resource, - refs map[string][]openapi.ObjectReference, + refs api.ReferenceMap, ) (*api.Resource, *errors.ServiceError) { resource.Kind = kind @@ -165,11 +162,13 @@ func (s *sqlResourceService) Patch( if svcErr := s.validateReferences(ctx, kind, patch.References); svcErr != nil { return nil, svcErr } + refRows := convertRefs(kind, resource.ID, patch.References) if refErr := s.resourceDao.ReplaceReferences( - ctx, resource.ID, convertRefs(kind, resource.ID, patch.References), + ctx, resource.ID, refRows, ); refErr != nil { return nil, errors.GeneralError("failed to save references: %s", refErr) } + resource.References = refRows } if !specChanged && !labelsChanged && !refsChanged { @@ -184,10 +183,6 @@ func (s *sqlResourceService) Patch( return nil, handleUpdateError(kind, saveErr) } - if refsChanged { - resource.References = convertRefs(kind, resource.ID, patch.References) - } - return resource, nil } @@ -202,22 +197,6 @@ func (s *sqlResourceService) Delete(ctx context.Context, kind, id string) (*api. return nil, handleSoftDeleteError(kind, err) } - // Reject first-time deletion if other resources still reference this one. - // Skip on re-evaluation (already soft-deleted) so the re-delete path can - // re-assess whether to hard-delete after children are finalized. - if resource.DeletedTime == nil { - referencers, refErr := s.resourceDao.FindReferencers(ctx, resource.ID) - if refErr != nil { - return nil, errors.GeneralError("failed to check references: %s", refErr) - } - if len(referencers) > 0 { - return nil, errors.ConflictState( - "cannot delete %s %q: referenced by %s %q — remove the reference before deleting", - kind, id, referencers[0].Kind, referencers[0].Name, - ) - } - } - deletedBy := actorFromContext(ctx) deletedAt := time.Now().UTC().Truncate(time.Microsecond) @@ -271,14 +250,14 @@ func (s *sqlResourceService) deleteResourceTree( } // Check if other resources reference this one before any deletion. - referencers, refErr := s.resourceDao.FindReferencers(ctx, resource.ID) + referencer, refErr := s.resourceDao.FindReferencer(ctx, resource.ID) if refErr != nil { return errors.GeneralError("failed to check references: %s", refErr) } - if len(referencers) > 0 { + if referencer != nil { return errors.ConflictState( "cannot delete %s %q: referenced by %s %q — remove the reference before deleting", - resource.Kind, resource.ID, referencers[0].Kind, referencers[0].Name, + resource.Kind, resource.ID, referencer.Kind, referencer.Name, ) } @@ -449,6 +428,8 @@ func (s *sqlResourceService) GetByID(ctx context.Context, id string) (*api.Resou func (s *sqlResourceService) ListAll( ctx context.Context, args *ListArguments, ) (api.ResourceList, *api.PagingMeta, *errors.ServiceError) { + args.Preloads = append(args.Preloads, "Conditions", "References") + var resources []api.Resource paging, svcErr := s.generic.List(ctx, args, &resources) if svcErr != nil { @@ -738,22 +719,15 @@ func (s *sqlResourceService) applyRefFilter( if !found { return errors.Validation("Unknown ref_type %q for entity %s", args.RefType, kind) } - const maxRefFilterIDs = 1000 sourceIDs, err := s.resourceDao.FindSourceIDsByRef(ctx, args.RefType, args.RefTargetID) if err != nil { return errors.GeneralError("failed to query references: %s", err) } - if len(sourceIDs) > maxRefFilterIDs { - return errors.Validation( - "ref_type filter matches %d resources (limit %d); "+ - "use pagination on the referencing entity directly", - len(sourceIDs), maxRefFilterIDs, - ) - } if len(sourceIDs) == 0 { args.Search += ` AND id = ""` return nil } + // sourceIDs are server-generated UUIDs from the database, so manual quoting is safe. quoted := make([]string, len(sourceIDs)) for i, sid := range sourceIDs { quoted[i] = `"` + sid + `"` @@ -761,6 +735,7 @@ func (s *sqlResourceService) applyRefFilter( args.Search += " AND id in [" + strings.Join(quoted, ", ") + "]" return 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 { @@ -873,9 +848,15 @@ func (s *sqlResourceService) forceDeleteResourceTree( return errors.GeneralError("Failed to delete resource conditions during force-delete: %s", err) } // Clear inbound references before hard-deleting (FK uses ON DELETE RESTRICT). + // Note: referencing resources with Min>0 on this ref type will silently + // violate their required-reference invariant after this operation. if err := s.resourceDao.ClearTargetReferences(ctx, resource.ID); err != nil { return errors.GeneralError("failed to clear references: %s", err) } + logger.With(ctx, + "resource_kind", resource.Kind, + "resource_id", resource.ID, + ).Info("Cleared inbound references for force-delete") if err := s.resourceDao.Delete(ctx, resource.Kind, resource.ID); err != nil { return handleDeleteError(resource.Kind, err) } @@ -889,7 +870,7 @@ func (s *sqlResourceService) forceDeleteResourceTree( // - per-type count must not exceed Max (when Max > 0) // - every referenced target must exist in the database func (s *sqlResourceService) validateReferences( - ctx context.Context, kind string, refs map[string][]openapi.ObjectReference, + ctx context.Context, kind string, refs api.ReferenceMap, ) *errors.ServiceError { desc := registry.MustGet(kind) @@ -951,7 +932,7 @@ func (s *sqlResourceService) validateReferences( // convertRefs flattens the API reference map into a slice of ResourceReference rows for the DAO. // Uses the registry's TargetKind (not the client-supplied Kind) so the stored value is always authoritative. -func convertRefs(kind, sourceID string, refs map[string][]openapi.ObjectReference) []api.ResourceReference { +func convertRefs(kind, sourceID string, refs api.ReferenceMap) []api.ResourceReference { desc := registry.MustGet(kind) targetKindByRef := make(map[string]string, len(desc.References)) for _, rd := range desc.References { diff --git a/pkg/services/resource_test.go b/pkg/services/resource_test.go index 5febbc34..3dd0103f 100644 --- a/pkg/services/resource_test.go +++ b/pkg/services/resource_test.go @@ -11,7 +11,6 @@ import ( "gorm.io/gorm" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/auth" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/dao" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" @@ -50,7 +49,7 @@ type mockResourceDao struct { deleteErr error existsSoftDeletedByOwnerErr error replaceRefsErr error - findReferencersResult []api.ResourceSummary + findReferencerResult *api.ResourceSummary lastReplacedRefs []api.ResourceReference replaceRefsCalled bool } @@ -193,8 +192,8 @@ func (d *mockResourceDao) ReplaceReferences(_ context.Context, _ string, refs [] return nil } -func (d *mockResourceDao) FindReferencers(_ context.Context, _ string) ([]api.ResourceSummary, error) { - return d.findReferencersResult, nil +func (d *mockResourceDao) FindReferencer(_ context.Context, _ string) (*api.ResourceSummary, error) { + return d.findReferencerResult, nil } func (d *mockResourceDao) ClearTargetReferences(_ context.Context, _ string) error { @@ -1899,6 +1898,8 @@ func TestProcessAdapterStatus_SoftDeleted_ChildrenExist_NoHardDelete(t *testing. // Parent should NOT be hard-deleted because child exists _, exists := mockDao.resources[resourceKey("Parent", "p-1")] Expect(exists).To(BeTrue(), "Parent should still exist — active child blocks hard-delete") +} + // --- Resource References --- func setupRefDescriptors() { @@ -1959,7 +1960,7 @@ func TestResourceService_Create_RefExceedsMax_Returns400(t *testing.T) { mockDao.addResource(testResource("Target", "t-2", "target-2")) resource := testResource("Parent", "p-1", "parent-1") - refs := map[string][]openapi.ObjectReference{ + refs := api.ReferenceMap{ "dep": { {Id: strPtr("t-1"), Kind: "Target"}, {Id: strPtr("t-2"), Kind: "Target"}, @@ -1984,7 +1985,7 @@ func TestResourceService_Create_WithValidRefs_Persists(t *testing.T) { mockDao.addResource(target) resource := testResource("Parent", "p-1", "parent-1") - refs := map[string][]openapi.ObjectReference{ + refs := api.ReferenceMap{ "dep": { {Id: strPtr("t-1"), Kind: "Target"}, }, @@ -2007,7 +2008,7 @@ func TestResourceService_Create_RefTargetNotFound_Returns400(t *testing.T) { svc, _, _ := newTestResourceService(mockDao) resource := testResource("Parent", "p-1", "parent-1") - refs := map[string][]openapi.ObjectReference{ + refs := api.ReferenceMap{ "dep": { {Id: strPtr("nonexistent"), Kind: "Target"}, }, @@ -2034,7 +2035,7 @@ func TestResourceService_Patch_ReferencesReplaced(t *testing.T) { mockDao.addResource(target) patch := &api.ResourcePatch{ - References: map[string][]openapi.ObjectReference{ + References: api.ReferenceMap{ "dep": { {Id: strPtr("t-1"), Kind: "Target"}, }, @@ -2078,7 +2079,7 @@ func TestResourceService_Patch_EmptyMapClearsAndValidatesMin(t *testing.T) { mockDao.addResource(existing) patch := &api.ResourcePatch{ - References: map[string][]openapi.ObjectReference{}, + References: api.ReferenceMap{}, } result, svcErr := svc.Patch(context.Background(), "Parent", "p-1", patch) @@ -2097,9 +2098,7 @@ func TestResourceService_Delete_ReferencedResource_Returns409(t *testing.T) { target := testResource("Target", "t-1", "target-1") mockDao.addResource(target) - mockDao.findReferencersResult = []api.ResourceSummary{ - {Kind: "Parent", Name: "parent-1"}, - } + mockDao.findReferencerResult = &api.ResourceSummary{Kind: "Parent", Name: "parent-1"} result, svcErr := svc.Delete(context.Background(), "Target", "t-1") Expect(result).To(BeNil()) @@ -2118,10 +2117,61 @@ func TestResourceService_Delete_UnreferencedResource_Succeeds(t *testing.T) { target := testResource("Target", "t-1", "target-1") mockDao.addResource(target) - mockDao.findReferencersResult = nil + mockDao.findReferencerResult = nil result, svcErr := svc.Delete(context.Background(), "Target", "t-1") Expect(svcErr).To(BeNil()) Expect(result).ToNot(BeNil()) Expect(result.DeletedTime).ToNot(BeNil()) } + +func TestResourceService_Create_DuplicateRefTargetID_Returns400(t *testing.T) { + RegisterTestingT(t) + setupOptionalRefDescriptors() // Max: 3 on "dep" + + mockDao := newMockResourceDao() + svc, _, _ := newTestResourceService(mockDao) + + target := testResource("Target", "t-1", "target-1") + mockDao.addResource(target) + + resource := testResource("Parent", "p-1", "parent-1") + refs := api.ReferenceMap{ + "dep": { + {Id: strPtr("t-1"), Kind: "Target"}, + {Id: strPtr("t-1"), Kind: "Target"}, // duplicate + }, + } + + result, svcErr := svc.Create(context.Background(), "Parent", resource, refs) + Expect(result).To(BeNil()) + Expect(svcErr).ToNot(BeNil()) + Expect(svcErr.HTTPCode).To(Equal(400)) + Expect(svcErr.Reason).To(ContainSubstring("duplicate target id")) +} + +func TestResourceService_Create_SoftDeletedTarget_Returns400(t *testing.T) { + RegisterTestingT(t) + setupOptionalRefDescriptors() + + mockDao := newMockResourceDao() + svc, _, _ := newTestResourceService(mockDao) + + deletedAt := time.Now() + target := testResource("Target", "t-1", "target-1") + target.DeletedTime = &deletedAt + mockDao.addResource(target) + + resource := testResource("Parent", "p-1", "parent-1") + refs := api.ReferenceMap{ + "dep": { + {Id: strPtr("t-1"), Kind: "Target"}, + }, + } + + result, svcErr := svc.Create(context.Background(), "Parent", resource, refs) + Expect(result).To(BeNil()) + Expect(svcErr).ToNot(BeNil()) + Expect(svcErr.HTTPCode).To(Equal(400)) + Expect(svcErr.Reason).To(ContainSubstring("marked for deletion")) +} diff --git a/test/integration/resource_references_test.go b/test/integration/resource_references_test.go index 25b11a2b..8001a660 100644 --- a/test/integration/resource_references_test.go +++ b/test/integration/resource_references_test.go @@ -61,12 +61,12 @@ func newRefTestResource(kind, name string) *api.Resource { } // makeRefs builds a reference map with a single ref type and target(s). -func makeRefs(refType string, targets ...struct{ id, kind string }) map[string][]openapi.ObjectReference { +func makeRefs(refType string, targets ...struct{ id, kind string }) api.ReferenceMap { refs := make([]openapi.ObjectReference, len(targets)) for i, t := range targets { refs[i] = openapi.ObjectReference{Id: util.ToPtr(t.id), Kind: t.kind} } - return map[string][]openapi.ObjectReference{refType: refs} + return api.ReferenceMap{refType: refs} } // --- Create --- @@ -165,7 +165,7 @@ func TestResourceReferences_PatchReplacesAtomically(t *testing.T) { Expect(svcErr).To(BeNil()) // Patch to point to target2. - patchRefs := map[string][]openapi.ObjectReference{ + patchRefs := api.ReferenceMap{ "dep": {{Id: util.ToPtr(target2.ID), Kind: "RefTarget"}}, } _, svcErr = svc.Patch(t.Context(), "RefSource", source.ID, &api.ResourcePatch{ @@ -221,7 +221,7 @@ func TestResourceReferences_PatchEmptyMapViolatesMin(t *testing.T) { // Patch with empty references map — Min=1 should reject. _, svcErr = svc.Patch(t.Context(), "RefSource", source.ID, &api.ResourcePatch{ - References: map[string][]openapi.ObjectReference{}, + References: api.ReferenceMap{}, }) Expect(svcErr).NotTo(BeNil(), "clearing required refs should fail") Expect(svcErr.HTTPCode).To(Equal(400)) @@ -416,7 +416,7 @@ func TestResourceReferences_PatchClearOptionalRefs(t *testing.T) { // Patch with empty references map — Min=0 should allow clearing. _, svcErr = svc.Patch(t.Context(), "OptSource", source.ID, &api.ResourcePatch{ - References: map[string][]openapi.ObjectReference{}, + References: api.ReferenceMap{}, }) Expect(svcErr).To(BeNil(), "clearing optional refs should succeed") @@ -424,3 +424,49 @@ func TestResourceReferences_PatchClearOptionalRefs(t *testing.T) { Expect(svcErr).To(BeNil()) Expect(retrieved.References).To(BeEmpty(), "references should be cleared") } + +func TestResourceReferences_CreateDuplicateRefTarget(t *testing.T) { + RegisterTestingT(t) + svc, _ := setupRefTest(t) + + target, svcErr := svc.Create(t.Context(), "RefTarget", + newRefTestResource("RefTarget", fmt.Sprintf("target-dup-%s", uuid.NewString()[:8])), nil) + Expect(svcErr).To(BeNil()) + + // Use OptSource (link ref type, Max=0 i.e. unlimited) so the duplicate + // check is reached before any max-count validation. + sourceName := fmt.Sprintf("source-dup-%s", uuid.NewString()[:8]) + refs := api.ReferenceMap{ + "link": { + {Id: util.ToPtr(target.ID), Kind: "RefTarget"}, + {Id: util.ToPtr(target.ID), Kind: "RefTarget"}, // duplicate + }, + } + + _, svcErr = svc.Create(t.Context(), "OptSource", newRefTestResource("OptSource", sourceName), refs) + Expect(svcErr).NotTo(BeNil(), "duplicate ref target should fail") + Expect(svcErr.HTTPCode).To(Equal(400)) + Expect(svcErr.Reason).To(ContainSubstring("duplicate target id")) +} + +func TestResourceReferences_CreateRefToSoftDeletedTarget(t *testing.T) { + RegisterTestingT(t) + svc, h := setupRefTest(t) + + // Create a target, then soft-delete it via direct DB update + // (svc.Delete would hard-delete since RefTarget has no required adapters). + target, svcErr := svc.Create(t.Context(), "RefTarget", + newRefTestResource("RefTarget", fmt.Sprintf("target-del-%s", uuid.NewString()[:8])), nil) + Expect(svcErr).To(BeNil()) + + markFinalizing(t, h, target.ID) + + // Attempt to reference the soft-deleted target. + sourceName := fmt.Sprintf("source-del-%s", uuid.NewString()[:8]) + refs := makeRefs("dep", struct{ id, kind string }{target.ID, "RefTarget"}) + + _, svcErr = svc.Create(t.Context(), "RefSource", newRefTestResource("RefSource", sourceName), refs) + Expect(svcErr).NotTo(BeNil(), "ref to soft-deleted target should fail") + Expect(svcErr.HTTPCode).To(Equal(400)) + Expect(svcErr.Reason).To(ContainSubstring("marked for deletion")) +} diff --git a/test/integration/resource_status_test.go b/test/integration/resource_status_test.go index b2917786..72af1829 100644 --- a/test/integration/resource_status_test.go +++ b/test/integration/resource_status_test.go @@ -23,7 +23,7 @@ func createTestChannelForStatus(t *testing.T, svc services.ResourceService) *api t.Helper() name := fmt.Sprintf("status-ch-%s", uuid.NewString()[:8]) channel := newChannelResource(name) - created, svcErr := svc.Create(context.Background(), "Channel", channel) + created, svcErr := svc.Create(context.Background(), "Channel", channel, nil) if svcErr != nil { t.Fatalf("Failed to create channel for status test: %v", svcErr) } @@ -133,7 +133,7 @@ func TestResourceStatus_NestedEntityStatuses(t *testing.T) { versionName := fmt.Sprintf("status-ver-%s", uuid.NewString()[:8]) version := newVersionResource(versionName, channel.ID) - createdVersion, svcErr := svc.Create(context.Background(), "Version", version) + createdVersion, svcErr := svc.Create(context.Background(), "Version", version, nil) Expect(svcErr).To(BeNil()) account := h.NewRandAccount()