From 05c22e21ecb2b2b9b29c967fa661910a5e222f89 Mon Sep 17 00:00:00 2001 From: aman Date: Mon, 19 Oct 2026 12:15:45 +0530 Subject: [PATCH 1/2] fix(resource): a resource in a disabled project can be deleted Delete looked up the resource's project with Get, which skips disabled projects, so it failed before removing anything. It now lists the project by id with IncludeDisabled. Create still uses Get. --- core/resource/mocks/project_service.go | 59 +++++++++++++++++++ core/resource/service.go | 24 +++++++- core/resource/service_test.go | 36 +++++++++-- .../store/postgres/project_repository_test.go | 5 ++ 4 files changed, 118 insertions(+), 6 deletions(-) diff --git a/core/resource/mocks/project_service.go b/core/resource/mocks/project_service.go index 9fd776116b..d74d9d424c 100644 --- a/core/resource/mocks/project_service.go +++ b/core/resource/mocks/project_service.go @@ -79,6 +79,65 @@ func (_c *ProjectService_Get_Call) RunAndReturn(run func(context.Context, string return _c } +// List provides a mock function with given fields: ctx, f +func (_m *ProjectService) List(ctx context.Context, f project.Filter) ([]project.Project, error) { + ret := _m.Called(ctx, f) + + if len(ret) == 0 { + panic("no return value specified for List") + } + + var r0 []project.Project + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, project.Filter) ([]project.Project, error)); ok { + return rf(ctx, f) + } + if rf, ok := ret.Get(0).(func(context.Context, project.Filter) []project.Project); ok { + r0 = rf(ctx, f) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]project.Project) + } + } + + if rf, ok := ret.Get(1).(func(context.Context, project.Filter) error); ok { + r1 = rf(ctx, f) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// ProjectService_List_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'List' +type ProjectService_List_Call struct { + *mock.Call +} + +// List is a helper method to define mock.On call +// - ctx context.Context +// - f project.Filter +func (_e *ProjectService_Expecter) List(ctx interface{}, f interface{}) *ProjectService_List_Call { + return &ProjectService_List_Call{Call: _e.mock.On("List", ctx, f)} +} + +func (_c *ProjectService_List_Call) Run(run func(ctx context.Context, f project.Filter)) *ProjectService_List_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context), args[1].(project.Filter)) + }) + return _c +} + +func (_c *ProjectService_List_Call) Return(_a0 []project.Project, _a1 error) *ProjectService_List_Call { + _c.Call.Return(_a0, _a1) + return _c +} + +func (_c *ProjectService_List_Call) RunAndReturn(run func(context.Context, project.Filter) ([]project.Project, error)) *ProjectService_List_Call { + _c.Call.Return(run) + return _c +} + // NewProjectService creates a new instance of ProjectService. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. // The first argument is typically a *testing.T value. func NewProjectService(t interface { diff --git a/core/resource/service.go b/core/resource/service.go index 99cf4fabca..6d6033158b 100644 --- a/core/resource/service.go +++ b/core/resource/service.go @@ -34,6 +34,7 @@ type AuthnService interface { type ProjectService interface { Get(ctx context.Context, idOrName string) (project.Project, error) + List(ctx context.Context, f project.Filter) ([]project.Project, error) } type OrgService interface { @@ -181,6 +182,25 @@ func (s Service) getParent(ctx context.Context, res Resource) (parent, error) { if err != nil { return parent{}, fmt.Errorf("failed to get project: %w", err) } + return projectParent(proj), nil +} + +// getParentToDelete also finds a disabled project +func (s Service) getParentToDelete(ctx context.Context, res Resource) (parent, error) { + if res.ProjectID == "" { + return s.getParent(ctx, res) + } + projects, err := s.projectService.List(ctx, project.Filter{ProjectIDs: []string{res.ProjectID}, IncludeDisabled: true}) + if err != nil { + return parent{}, fmt.Errorf("failed to get project: %w", err) + } + if len(projects) == 0 { + return parent{}, fmt.Errorf("failed to get project: %w", project.ErrNotExist) + } + return projectParent(projects[0]), nil +} + +func projectParent(proj project.Project) parent { return parent{ projectID: proj.ID, org: proj.Organization, @@ -188,7 +208,7 @@ func (s Service) getParent(ctx context.Context, res Resource) (parent, error) { link: relation.Subject{ID: proj.ID, Namespace: schema.ProjectNamespace}, linkName: schema.ProjectRelationName, audit: auditmodels.Resource{ID: proj.ID, Type: pkgauditrecord.ProjectType, Name: proj.Title}, - }, nil + } } func (s Service) createAuditRecord(ctx context.Context, event pkgauditrecord.Event, res Resource, p parent) { @@ -481,7 +501,7 @@ func (s Service) Delete(ctx context.Context, namespaceID, id string) error { if err != nil { return err } - p, err := s.getParent(ctx, res) + p, err := s.getParentToDelete(ctx, res) if err != nil { return err } diff --git a/core/resource/service_test.go b/core/resource/service_test.go index 49b0a1411c..837acdadd9 100644 --- a/core/resource/service_test.go +++ b/core/resource/service_test.go @@ -665,12 +665,13 @@ func TestDelete(t *testing.T) { Organization: organization.Organization{ID: uuid.New().String(), Title: "Org One"}, } res := resource.Resource{ID: "r1", Name: "res-1", Title: "Resource One", NamespaceID: "resource/item", ProjectID: proj.ID} + lookup := project.Filter{ProjectIDs: []string{proj.ID}, IncludeDisabled: true} t.Run("deletes relations then the row, and writes the audit record", func(t *testing.T) { repo, relationSvc, _, projectSvc, _, _, auditRepo, _, svc := newTestService(t) repo.EXPECT().GetByID(mock.Anything, "r1").Return(res, nil) - projectSvc.EXPECT().Get(mock.Anything, proj.ID).Return(proj, nil) + projectSvc.EXPECT().List(mock.Anything, lookup).Return([]project.Project{proj}, nil) relationSvc.EXPECT().Delete(mock.Anything, relation.Relation{ Object: relation.Object{ID: "r1", Namespace: "resource/item"}, }).Return(nil) @@ -719,7 +720,7 @@ func TestDelete(t *testing.T) { repo, relationSvc, _, projectSvc, _, _, auditRepo, _, svc := newTestService(t) repo.EXPECT().GetByID(mock.Anything, "r1").Return(res, nil) - projectSvc.EXPECT().Get(mock.Anything, proj.ID).Return(proj, nil) + projectSvc.EXPECT().List(mock.Anything, lookup).Return([]project.Project{proj}, nil) relationSvc.EXPECT().Delete(mock.Anything, mock.Anything).Return(relation.ErrNotExist) repo.EXPECT().Delete(mock.Anything, "r1").Return(nil) auditRepo.EXPECT().Create(mock.Anything, mock.AnythingOfType("models.AuditRecord")). @@ -733,7 +734,7 @@ func TestDelete(t *testing.T) { repo, relationSvc, _, projectSvc, _, _, _, _, svc := newTestService(t) repo.EXPECT().GetByID(mock.Anything, "r1").Return(res, nil) - projectSvc.EXPECT().Get(mock.Anything, proj.ID).Return(proj, nil) + projectSvc.EXPECT().List(mock.Anything, lookup).Return([]project.Project{proj}, nil) relationSvc.EXPECT().Delete(mock.Anything, mock.Anything).Return(errors.New("spicedb down")) err := svc.Delete(ctx, "resource/item", "r1") @@ -744,13 +745,40 @@ func TestDelete(t *testing.T) { repo, relationSvc, _, projectSvc, _, _, _, _, svc := newTestService(t) repo.EXPECT().GetByID(mock.Anything, "r1").Return(res, nil) - projectSvc.EXPECT().Get(mock.Anything, proj.ID).Return(proj, nil) + projectSvc.EXPECT().List(mock.Anything, lookup).Return([]project.Project{proj}, nil) relationSvc.EXPECT().Delete(mock.Anything, mock.Anything).Return(nil) repo.EXPECT().Delete(mock.Anything, "r1").Return(errors.New("db down")) err := svc.Delete(ctx, "resource/item", "r1") assert.ErrorContains(t, err, "db down") }) + + t.Run("deletes a resource of a disabled project", func(t *testing.T) { + repo, relationSvc, _, projectSvc, _, _, auditRepo, _, svc := newTestService(t) + disabled := proj + disabled.State = project.Disabled + + repo.EXPECT().GetByID(mock.Anything, "r1").Return(res, nil) + projectSvc.EXPECT().List(mock.Anything, lookup).Return([]project.Project{disabled}, nil) + relationSvc.EXPECT().Delete(mock.Anything, mock.Anything).Return(nil) + repo.EXPECT().Delete(mock.Anything, "r1").Return(nil) + auditRepo.EXPECT().Create(mock.Anything, mock.MatchedBy(func(rec auditmodels.AuditRecord) bool { + return rec.Resource.ID == proj.ID + })).Return(auditmodels.AuditRecord{}, nil) + + err := svc.Delete(ctx, "resource/item", "r1") + assert.NoError(t, err) + }) + + t.Run("deletes nothing when the project is gone", func(t *testing.T) { + repo, _, _, projectSvc, _, _, _, _, svc := newTestService(t) + + repo.EXPECT().GetByID(mock.Anything, "r1").Return(res, nil) + projectSvc.EXPECT().List(mock.Anything, lookup).Return([]project.Project{}, nil) + + err := svc.Delete(ctx, "resource/item", "r1") + assert.ErrorIs(t, err, project.ErrNotExist) + }) } func TestPurge(t *testing.T) { diff --git a/internal/store/postgres/project_repository_test.go b/internal/store/postgres/project_repository_test.go index 575ab21763..6772950c1d 100644 --- a/internal/store/postgres/project_repository_test.go +++ b/internal/store/postgres/project_repository_test.go @@ -541,4 +541,9 @@ func (s *ProjectRepositoryTestSuite) TestListIncludeDisabled() { ids = append(ids, p.ID) } s.Assert().Contains(ids, disabled.ID) + + byID, err := s.repository.List(s.ctx, project.Filter{ProjectIDs: []string{disabled.ID}, IncludeDisabled: true}) + s.Require().NoError(err) + s.Require().Len(byID, 1) + s.Assert().Equal(disabled.ID, byID[0].ID) } From e40814b4e35331e35b4c8249d03e7992ff41b5f3 Mon Sep 17 00:00:00 2001 From: aman Date: Tue, 20 Oct 2026 15:48:10 +0530 Subject: [PATCH 2/2] test(store): List with IncludeDisabled skips a deleted project A project that is disabled and then soft-deleted does not come back from List, even with IncludeDisabled. Also drops the comment on getParentToDelete. --- core/resource/service.go | 1 - internal/store/postgres/project_repository_test.go | 6 ++++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/core/resource/service.go b/core/resource/service.go index 6d6033158b..7f20326a6c 100644 --- a/core/resource/service.go +++ b/core/resource/service.go @@ -185,7 +185,6 @@ func (s Service) getParent(ctx context.Context, res Resource) (parent, error) { return projectParent(proj), nil } -// getParentToDelete also finds a disabled project func (s Service) getParentToDelete(ctx context.Context, res Resource) (parent, error) { if res.ProjectID == "" { return s.getParent(ctx, res) diff --git a/internal/store/postgres/project_repository_test.go b/internal/store/postgres/project_repository_test.go index 6772950c1d..cd8373bbc8 100644 --- a/internal/store/postgres/project_repository_test.go +++ b/internal/store/postgres/project_repository_test.go @@ -546,4 +546,10 @@ func (s *ProjectRepositoryTestSuite) TestListIncludeDisabled() { s.Require().NoError(err) s.Require().Len(byID, 1) s.Assert().Equal(disabled.ID, byID[0].ID) + + _, err = s.client.ExecContext(s.ctx, "UPDATE projects SET deleted_at = now() WHERE id = $1", disabled.ID) + s.Require().NoError(err) + deleted, err := s.repository.List(s.ctx, project.Filter{ProjectIDs: []string{disabled.ID}, IncludeDisabled: true}) + s.Require().NoError(err) + s.Assert().Empty(deleted) }