diff --git a/pkg/api/resource_reference.go b/pkg/api/resource_reference.go index 2c6a5fc7..1821e79c 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. FindReferencer 409). +// 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 0f369e7a..53824cea 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) FindReferencer(_ context.Context, _ string) (*api.ResourceSummary, error) { +func (d *resourceDaoMock) FindReferencers(_ context.Context, _ string) ([]api.ResourceSummary, error) { return nil, nil } diff --git a/pkg/dao/resource.go b/pkg/dao/resource.go index 7365f602..1c529eca 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 - FindReferencer(ctx context.Context, targetID string) (*api.ResourceSummary, 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) } @@ -196,26 +196,22 @@ func (d *sqlResourceDao) ReplaceReferences( return nil } -// FindReferencer returns the first non-deleted resource that references targetID, +// FindReferencers returns the list of resources that references targetID, // or nil if none exists. Used as an existence check for 409 conflict responses. -func (d *sqlResourceDao) FindReferencer( +func (d *sqlResourceDao) FindReferencers( ctx context.Context, targetID string, -) (*api.ResourceSummary, error) { +) ([]api.ResourceSummary, error) { g2 := d.sessionFactory.New(ctx) - var summary api.ResourceSummary + var summaries []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 + Scan(&summaries).Error if err != nil { return nil, err } - if summary.Kind == "" { - return nil, nil - } - return &summary, nil + return summaries, nil } // ClearTargetReferences removes all inbound references pointing at targetID. diff --git a/pkg/services/resource.go b/pkg/services/resource.go index b4987f30..6c1010b0 100644 --- a/pkg/services/resource.go +++ b/pkg/services/resource.go @@ -286,15 +286,19 @@ func (s *sqlResourceService) deleteResourceTree( } // Check if other resources reference this one before any deletion. - referencer, refErr := s.resourceDao.FindReferencer(ctx, resource.ID) + referencers, refErr := s.resourceDao.FindReferencers(ctx, resource.ID) if refErr != nil { return errors.GeneralError("failed to check references: %s", refErr) } - if referencer != nil { + // List out all the references for a specific resource + if len(referencers) > 0 { + names := make([]string, len(referencers)) + for i, r := range referencers { + names[i] = fmt.Sprintf("%s %q", r.Kind, r.Name) + } return errors.ConflictState( - "cannot delete %s %q: referenced by %s %q — remove the reference before deleting", - resource.Kind, resource.ID, referencer.Kind, referencer.Name, - ) + "cannot delete %s %q: referenced by %s — remove the reference(s) before deleting", + resource.Kind, resource.Name, strings.Join(names, ", ")) } shouldSoftDelete, svcErr := s.shouldSoftDelete(ctx, resource, children) @@ -979,6 +983,12 @@ func (s *sqlResourceService) validateReferences( ) } seen[*ref.Id] = true + if ref.Kind != rd.TargetKind { + return errors.Validation( + "reference type %q: kind %q does not match expected target kind %q", + refType, ref.Kind, rd.TargetKind, + ) + } target, err := s.resourceDao.Get(ctx, rd.TargetKind, *ref.Id) if err != nil { return errors.Validation( diff --git a/pkg/services/resource_test.go b/pkg/services/resource_test.go index 6b3157a4..13a7bb55 100644 --- a/pkg/services/resource_test.go +++ b/pkg/services/resource_test.go @@ -51,7 +51,7 @@ type mockResourceDao struct { deleteErr error existsSoftDeletedByOwnerErr error replaceRefsErr error - findReferencerResult *api.ResourceSummary + findReferencersResult []api.ResourceSummary lastReplacedRefs []api.ResourceReference replaceRefsCalled bool } @@ -194,8 +194,8 @@ func (d *mockResourceDao) ReplaceReferences(_ context.Context, _ string, refs [] return nil } -func (d *mockResourceDao) FindReferencer(_ context.Context, _ string) (*api.ResourceSummary, error) { - return d.findReferencerResult, nil +func (d *mockResourceDao) FindReferencers(_ context.Context, _ string) ([]api.ResourceSummary, error) { + return d.findReferencersResult, nil } func (d *mockResourceDao) ClearTargetReferences(_ context.Context, _ string) error { @@ -2196,7 +2196,7 @@ func TestResourceService_Delete_ReferencedResource_Returns409(t *testing.T) { target := testResource("Target", "t-1", "target-1") mockDao.addResource(target) - mockDao.findReferencerResult = &api.ResourceSummary{Kind: "Parent", Name: "parent-1"} + mockDao.findReferencersResult = []api.ResourceSummary{{Kind: "Parent", Name: "parent-1"}} result, svcErr := svc.Delete(context.Background(), "Target", "t-1") Expect(result).To(BeNil()) @@ -2215,7 +2215,7 @@ func TestResourceService_Delete_UnreferencedResource_Succeeds(t *testing.T) { target := testResource("Target", "t-1", "target-1") mockDao.addResource(target) - mockDao.findReferencerResult = nil + mockDao.findReferencersResult = nil result, svcErr := svc.Delete(context.Background(), "Target", "t-1") Expect(svcErr).To(BeNil()) diff --git a/test/integration/resource_references_test.go b/test/integration/resource_references_test.go index ab958ff4..97fc856f 100644 --- a/test/integration/resource_references_test.go +++ b/test/integration/resource_references_test.go @@ -2,6 +2,7 @@ package integration import ( "fmt" + "strings" "sync" "testing" @@ -516,6 +517,37 @@ func TestResourceReferences_CreateDuplicateRefTarget(t *testing.T) { Expect(svcErr.HTTPCode).To(Equal(400)) Expect(svcErr.Reason).To(ContainSubstring("duplicate target id")) } +func TestResourceReferences_CreateInvalidKind(t *testing.T) { + testCases := []struct { + name string + kind string + description string + }{ + {"WrongKind", "WrongKind", "wrong kind should fail validation"}, + {"EmptyKind", "", "empty kind should fail validation"}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + RegisterTestingT(t) + svc, _ := setupRefTest(t) + + target, svcErr := svc.Create(t.Context(), "RefTarget", + newRefTestResource("RefTarget", + fmt.Sprintf("target-%s-%s", strings.ToLower(tc.name[:2]), uuid.NewString()[:8])), + nil) + Expect(svcErr).To(BeNil()) + + sourceName := fmt.Sprintf("source-%s-%s", strings.ToLower(tc.name[:2]), uuid.NewString()[:8]) + refs := makeRefs("dep", struct{ id, kind string }{target.ID, tc.kind}) + + _, svcErr = svc.Create(t.Context(), "RefSource", newRefTestResource("RefSource", sourceName), refs) + Expect(svcErr).NotTo(BeNil(), tc.description) + Expect(svcErr.HTTPCode).To(Equal(400)) + Expect(svcErr.Reason).To(ContainSubstring("does not match expected target kind")) + }) + } +} func TestResourceReferences_CreateRefToSoftDeletedTarget(t *testing.T) { RegisterTestingT(t)