diff --git a/core/deleter/mocks/resource_service.go b/core/deleter/mocks/resource_service.go index 5240087e5..2cd6567f2 100644 --- a/core/deleter/mocks/resource_service.go +++ b/core/deleter/mocks/resource_service.go @@ -5,9 +5,8 @@ package mocks import ( context "context" - mock "github.com/stretchr/testify/mock" - resource "github.com/raystack/frontier/core/resource" + mock "github.com/stretchr/testify/mock" ) // ResourceService is an autogenerated mock type for the ResourceService type @@ -23,54 +22,6 @@ func (_m *ResourceService) EXPECT() *ResourceService_Expecter { return &ResourceService_Expecter{mock: &_m.Mock} } -// Delete provides a mock function with given fields: ctx, namespaceID, id -func (_m *ResourceService) Delete(ctx context.Context, namespaceID string, id string) error { - ret := _m.Called(ctx, namespaceID, id) - - if len(ret) == 0 { - panic("no return value specified for Delete") - } - - var r0 error - if rf, ok := ret.Get(0).(func(context.Context, string, string) error); ok { - r0 = rf(ctx, namespaceID, id) - } else { - r0 = ret.Error(0) - } - - return r0 -} - -// ResourceService_Delete_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Delete' -type ResourceService_Delete_Call struct { - *mock.Call -} - -// Delete is a helper method to define mock.On call -// - ctx context.Context -// - namespaceID string -// - id string -func (_e *ResourceService_Expecter) Delete(ctx interface{}, namespaceID interface{}, id interface{}) *ResourceService_Delete_Call { - return &ResourceService_Delete_Call{Call: _e.mock.On("Delete", ctx, namespaceID, id)} -} - -func (_c *ResourceService_Delete_Call) Run(run func(ctx context.Context, namespaceID string, id string)) *ResourceService_Delete_Call { - _c.Call.Run(func(args mock.Arguments) { - run(args[0].(context.Context), args[1].(string), args[2].(string)) - }) - return _c -} - -func (_c *ResourceService_Delete_Call) Return(_a0 error) *ResourceService_Delete_Call { - _c.Call.Return(_a0) - return _c -} - -func (_c *ResourceService_Delete_Call) RunAndReturn(run func(context.Context, string, string) error) *ResourceService_Delete_Call { - _c.Call.Return(run) - return _c -} - // List provides a mock function with given fields: ctx, flt func (_m *ResourceService) List(ctx context.Context, flt resource.Filter) ([]resource.Resource, error) { ret := _m.Called(ctx, flt) @@ -130,6 +81,54 @@ func (_c *ResourceService_List_Call) RunAndReturn(run func(context.Context, reso return _c } +// Purge provides a mock function with given fields: ctx, namespaceID, id +func (_m *ResourceService) Purge(ctx context.Context, namespaceID string, id string) error { + ret := _m.Called(ctx, namespaceID, id) + + if len(ret) == 0 { + panic("no return value specified for Purge") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, string, string) error); ok { + r0 = rf(ctx, namespaceID, id) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// ResourceService_Purge_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Purge' +type ResourceService_Purge_Call struct { + *mock.Call +} + +// Purge is a helper method to define mock.On call +// - ctx context.Context +// - namespaceID string +// - id string +func (_e *ResourceService_Expecter) Purge(ctx interface{}, namespaceID interface{}, id interface{}) *ResourceService_Purge_Call { + return &ResourceService_Purge_Call{Call: _e.mock.On("Purge", ctx, namespaceID, id)} +} + +func (_c *ResourceService_Purge_Call) Run(run func(ctx context.Context, namespaceID string, id string)) *ResourceService_Purge_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context), args[1].(string), args[2].(string)) + }) + return _c +} + +func (_c *ResourceService_Purge_Call) Return(_a0 error) *ResourceService_Purge_Call { + _c.Call.Return(_a0) + return _c +} + +func (_c *ResourceService_Purge_Call) RunAndReturn(run func(context.Context, string, string) error) *ResourceService_Purge_Call { + _c.Call.Return(run) + return _c +} + // NewResourceService creates a new instance of ResourceService. 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 NewResourceService(t interface { diff --git a/core/deleter/service.go b/core/deleter/service.go index 8ef5ca4fb..2eb6b0391 100644 --- a/core/deleter/service.go +++ b/core/deleter/service.go @@ -68,7 +68,7 @@ type PolicyService interface { type ResourceService interface { List(ctx context.Context, flt resource.Filter) ([]resource.Resource, error) - Delete(ctx context.Context, namespaceID, id string) error + Purge(ctx context.Context, namespaceID, id string) error } type GroupService interface { @@ -213,15 +213,18 @@ func (d Service) DeleteProject(ctx context.Context, id string) error { } } - // delete all related resources + // the project row is removed for good below, so every resource row of the + // project, deleted ones included, has to go with it + // TODO(fix): soft-delete the live resources instead once project delete is soft resources, err := d.resService.List(ctx, resource.Filter{ - ProjectID: id, + ProjectID: id, + IncludeDeleted: true, }) if err != nil { return err } for _, r := range resources { - if err = d.resService.Delete(ctx, r.NamespaceID, r.ID); err != nil { + if err = d.resService.Purge(ctx, r.NamespaceID, r.ID); err != nil { return fmt.Errorf("failed to delete project while deleting a resource[%s]: %w", r.Name, err) } } diff --git a/core/deleter/service_test.go b/core/deleter/service_test.go index f3955cb55..0aabc6378 100644 --- a/core/deleter/service_test.go +++ b/core/deleter/service_test.go @@ -109,9 +109,9 @@ func TestDeleteProject(t *testing.T) { m.polSvc.EXPECT().Delete(mock.Anything, "pol-1").Return(nil) m.polSvc.EXPECT().Delete(mock.Anything, "pol-2").Return(nil) - m.resSvc.EXPECT().List(mock.Anything, resource.Filter{ProjectID: "proj-1"}). + m.resSvc.EXPECT().List(mock.Anything, resource.Filter{ProjectID: "proj-1", IncludeDeleted: true}). Return([]resource.Resource{{ID: "res-1", NamespaceID: "ns-1", Name: "r1"}}, nil) - m.resSvc.EXPECT().Delete(mock.Anything, "ns-1", "res-1").Return(nil) + m.resSvc.EXPECT().Purge(mock.Anything, "ns-1", "res-1").Return(nil) m.projSvc.EXPECT().DeleteModel(mock.Anything, "proj-1").Return(nil) @@ -145,7 +145,7 @@ func TestDeleteProject(t *testing.T) { m.polSvc.EXPECT().List(mock.Anything, policy.Filter{ProjectID: "proj-1"}). Return([]policy.Policy{}, nil) - m.resSvc.EXPECT().List(mock.Anything, resource.Filter{ProjectID: "proj-1"}). + m.resSvc.EXPECT().List(mock.Anything, resource.Filter{ProjectID: "proj-1", IncludeDeleted: true}). Return([]resource.Resource{}, nil) m.projSvc.EXPECT().DeleteModel(mock.Anything, "proj-1").Return(nil) @@ -200,7 +200,7 @@ func TestDeleteOrganization(t *testing.T) { Return([]project.Project{{ID: "proj-1", Name: "p1"}}, nil) m.polSvc.EXPECT().List(mock.Anything, policy.Filter{ProjectID: "proj-1"}). Return([]policy.Policy{}, nil) - m.resSvc.EXPECT().List(mock.Anything, resource.Filter{ProjectID: "proj-1"}). + m.resSvc.EXPECT().List(mock.Anything, resource.Filter{ProjectID: "proj-1", IncludeDeleted: true}). Return([]resource.Resource{}, nil) m.projSvc.EXPECT().DeleteModel(mock.Anything, "proj-1").Return(nil) diff --git a/core/resource/filter.go b/core/resource/filter.go index 9e230d60a..5ff69eb3a 100644 --- a/core/resource/filter.go +++ b/core/resource/filter.go @@ -5,4 +5,7 @@ type Filter struct { UserID string ServiceUserID string NamespaceID string + // IncludeDeleted also returns soft-deleted rows. + // TODO(fix): remove once the project delete cascade no longer purges + IncludeDeleted bool } diff --git a/core/resource/mocks/repository.go b/core/resource/mocks/repository.go index 32cd3b036..533d6f8d0 100644 --- a/core/resource/mocks/repository.go +++ b/core/resource/mocks/repository.go @@ -299,6 +299,53 @@ func (_c *Repository_List_Call) RunAndReturn(run func(context.Context, resource. return _c } +// Purge provides a mock function with given fields: ctx, id +func (_m *Repository) Purge(ctx context.Context, id string) error { + ret := _m.Called(ctx, id) + + if len(ret) == 0 { + panic("no return value specified for Purge") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, string) error); ok { + r0 = rf(ctx, id) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// Repository_Purge_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Purge' +type Repository_Purge_Call struct { + *mock.Call +} + +// Purge is a helper method to define mock.On call +// - ctx context.Context +// - id string +func (_e *Repository_Expecter) Purge(ctx interface{}, id interface{}) *Repository_Purge_Call { + return &Repository_Purge_Call{Call: _e.mock.On("Purge", ctx, id)} +} + +func (_c *Repository_Purge_Call) Run(run func(ctx context.Context, id string)) *Repository_Purge_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context), args[1].(string)) + }) + return _c +} + +func (_c *Repository_Purge_Call) Return(_a0 error) *Repository_Purge_Call { + _c.Call.Return(_a0) + return _c +} + +func (_c *Repository_Purge_Call) RunAndReturn(run func(context.Context, string) error) *Repository_Purge_Call { + _c.Call.Return(run) + return _c +} + // Update provides a mock function with given fields: ctx, _a1 func (_m *Repository) Update(ctx context.Context, _a1 resource.Resource) (resource.Resource, error) { ret := _m.Called(ctx, _a1) diff --git a/core/resource/resource.go b/core/resource/resource.go index 344fa5057..dc31c7421 100644 --- a/core/resource/resource.go +++ b/core/resource/resource.go @@ -17,6 +17,7 @@ type Repository interface { List(ctx context.Context, flt Filter) ([]Resource, error) Update(ctx context.Context, resource Resource) (Resource, error) Delete(ctx context.Context, id string) error + Purge(ctx context.Context, id string) error } type Resource struct { diff --git a/core/resource/service.go b/core/resource/service.go index 11b5835af..7bc1ebceb 100644 --- a/core/resource/service.go +++ b/core/resource/service.go @@ -449,6 +449,35 @@ func (s Service) batchCheckWithScopeFilter(ctx context.Context, relations []rela } func (s Service) Delete(ctx context.Context, namespaceID, id string) error { + res, err := s.repository.GetByID(ctx, id) + if err != nil { + return err + } + resourceProject, err := s.projectService.Get(ctx, res.ProjectID) + if err != nil { + return fmt.Errorf("failed to get project: %w", err) + } + + if err := s.relationService.Delete(ctx, relation.Relation{ + Object: relation.Object{ + ID: id, + Namespace: namespaceID, + }, + }); err != nil && !errors.Is(err, relation.ErrNotExist) { + return err + } + if err := s.repository.Delete(ctx, id); err != nil { + return err + } + + s.createAuditRecord(ctx, pkgauditrecord.ResourceDeletedEvent, res, resourceProject) + return nil +} + +// Purge removes the resource and its SpiceDB tuples for good. Only the project +// delete cascade uses it; the API uses Delete, which keeps the row. +// TODO(fix): remove once project delete is soft +func (s Service) Purge(ctx context.Context, namespaceID, id string) error { if err := s.relationService.Delete(ctx, relation.Relation{ Object: relation.Object{ ID: id, @@ -457,7 +486,7 @@ func (s Service) Delete(ctx context.Context, namespaceID, id string) error { }); err != nil && !errors.Is(err, relation.ErrNotExist) { return err } - return s.repository.Delete(ctx, id) + return s.repository.Purge(ctx, id) } // RemovePrincipalAccess deletes every resource-level policy the principal holds diff --git a/core/resource/service_test.go b/core/resource/service_test.go index 90a8606d9..9e8a2d765 100644 --- a/core/resource/service_test.go +++ b/core/resource/service_test.go @@ -17,6 +17,7 @@ import ( "github.com/raystack/frontier/core/user" patmodels "github.com/raystack/frontier/core/userpat/models" "github.com/raystack/frontier/internal/bootstrap/schema" + pkgauditrecord "github.com/raystack/frontier/pkg/auditrecord" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" ) @@ -531,25 +532,100 @@ func TestUpdate(t *testing.T) { } func TestDelete(t *testing.T) { - t.Run("deletes relations then resource", func(t *testing.T) { - repo, relationSvc, _, _, _, _, _, _, svc := newTestService(t) + ctx := context.Background() + proj := project.Project{ + ID: uuid.New().String(), + Title: "Project One", + 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} + 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) relationSvc.EXPECT().Delete(mock.Anything, relation.Relation{ Object: relation.Object{ID: "r1", Namespace: "resource/item"}, }).Return(nil) repo.EXPECT().Delete(mock.Anything, "r1").Return(nil) + auditRepo.EXPECT().Create(mock.Anything, mock.MatchedBy(func(rec auditmodels.AuditRecord) bool { + return rec.Event == pkgauditrecord.ResourceDeletedEvent && + rec.Target != nil && rec.Target.ID == "r1" && rec.Target.Name == "Resource One" && + rec.Resource.ID == proj.ID && rec.OrgID == proj.Organization.ID + })).Return(auditmodels.AuditRecord{}, nil) - err := svc.Delete(context.Background(), "resource/item", "r1") + err := svc.Delete(ctx, "resource/item", "r1") assert.NoError(t, err) }) + t.Run("returns not found when the resource does not exist", func(t *testing.T) { + repo, _, _, _, _, _, _, _, svc := newTestService(t) + + repo.EXPECT().GetByID(mock.Anything, "r1").Return(resource.Resource{}, resource.ErrNotExist) + + err := svc.Delete(ctx, "resource/item", "r1") + assert.ErrorIs(t, err, resource.ErrNotExist) + }) + t.Run("ignores relation not exist error", func(t *testing.T) { - repo, relationSvc, _, _, _, _, _, _, svc := newTestService(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) 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")). + Return(auditmodels.AuditRecord{}, nil) + + err := svc.Delete(ctx, "resource/item", "r1") + assert.NoError(t, err) + }) + + t.Run("returns relation delete error", func(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) + relationSvc.EXPECT().Delete(mock.Anything, mock.Anything).Return(errors.New("spicedb down")) + + err := svc.Delete(ctx, "resource/item", "r1") + assert.ErrorContains(t, err, "spicedb down") + }) + + t.Run("writes no audit record when the row delete fails", func(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) + 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") + }) +} + +func TestPurge(t *testing.T) { + t.Run("removes relations then the row", func(t *testing.T) { + repo, relationSvc, _, _, _, _, _, _, svc := newTestService(t) + + relationSvc.EXPECT().Delete(mock.Anything, relation.Relation{ + Object: relation.Object{ID: "r1", Namespace: "resource/item"}, + }).Return(nil) + repo.EXPECT().Purge(mock.Anything, "r1").Return(nil) + + err := svc.Purge(context.Background(), "resource/item", "r1") + assert.NoError(t, err) + }) + + t.Run("ignores relation not exist error", func(t *testing.T) { + repo, relationSvc, _, _, _, _, _, _, svc := newTestService(t) + + relationSvc.EXPECT().Delete(mock.Anything, mock.Anything).Return(relation.ErrNotExist) + repo.EXPECT().Purge(mock.Anything, "r1").Return(nil) - err := svc.Delete(context.Background(), "resource/item", "r1") + err := svc.Purge(context.Background(), "resource/item", "r1") assert.NoError(t, err) }) @@ -558,7 +634,7 @@ func TestDelete(t *testing.T) { relationSvc.EXPECT().Delete(mock.Anything, mock.Anything).Return(errors.New("spicedb down")) - err := svc.Delete(context.Background(), "resource/item", "r1") + err := svc.Purge(context.Background(), "resource/item", "r1") assert.ErrorContains(t, err, "spicedb down") }) } diff --git a/internal/store/postgres/migrations/20260918100000_resources_urn_live_unique.down.sql b/internal/store/postgres/migrations/20260918100000_resources_urn_live_unique.down.sql new file mode 100644 index 000000000..c21df0c5f --- /dev/null +++ b/internal/store/postgres/migrations/20260918100000_resources_urn_live_unique.down.sql @@ -0,0 +1,2 @@ +ALTER TABLE resources ADD CONSTRAINT resources_urn_key UNIQUE (urn); +DROP INDEX IF EXISTS uq_resources_urn_live; diff --git a/internal/store/postgres/migrations/20260918100000_resources_urn_live_unique.up.sql b/internal/store/postgres/migrations/20260918100000_resources_urn_live_unique.up.sql new file mode 100644 index 000000000..2e17f9c1c --- /dev/null +++ b/internal/store/postgres/migrations/20260918100000_resources_urn_live_unique.up.sql @@ -0,0 +1,2 @@ +CREATE UNIQUE INDEX IF NOT EXISTS uq_resources_urn_live ON resources (urn) WHERE deleted_at IS NULL; +ALTER TABLE resources DROP CONSTRAINT IF EXISTS resources_urn_key; diff --git a/internal/store/postgres/postgres.go b/internal/store/postgres/postgres.go index bb85e16ea..215e56ce5 100644 --- a/internal/store/postgres/postgres.go +++ b/internal/store/postgres/postgres.go @@ -34,6 +34,15 @@ func fromLive(table string) *goqu.SelectDataset { return dialect.From(table).Where(live(table)) } +// liveConflictTarget builds the ON CONFLICT target for a unique index that +// covers only live rows. For example "urn" renders as +// ON CONFLICT (urn) WHERE (deleted_at IS NULL). Postgres uses such an index only +// when the clause names its condition. goqu wraps the target in parentheses as +// is, which is why the string ends open. +func liveConflictTarget(columns string) string { + return columns + ") WHERE (deleted_at IS NULL" +} + const ( TABLE_PERMISSIONS = "permissions" TABLE_GROUPS = "groups" diff --git a/internal/store/postgres/resource_repository.go b/internal/store/postgres/resource_repository.go index bfb324ee5..10fabe118 100644 --- a/internal/store/postgres/resource_repository.go +++ b/internal/store/postgres/resource_repository.go @@ -67,7 +67,7 @@ func (r ResourceRepository) Create(ctx context.Context, res resource.Resource) ( "principal_type": principalType, "metadata": marshaledMetadata, }).OnConflict( - goqu.DoUpdate("urn", goqu.Record{ + goqu.DoUpdate(liveConflictTarget("urn"), goqu.Record{ "name": res.Name, "title": res.Title, "project_id": res.ProjectID, @@ -90,6 +90,8 @@ func (r ResourceRepository) Create(ctx context.Context, res resource.Resource) ( return resource.Resource{}, fmt.Errorf("%w: %w", err, resource.ErrInvalidDetail) case errors.Is(err, ErrInvalidTextRepresentation): return resource.Resource{}, fmt.Errorf("%w: %w", err, resource.ErrInvalidUUID) + case errors.Is(err, ErrDuplicateKey): + return resource.Resource{}, resource.ErrConflict default: return resource.Resource{}, err } @@ -101,7 +103,11 @@ func (r ResourceRepository) Create(ctx context.Context, res resource.Resource) ( func (r ResourceRepository) List(ctx context.Context, flt resource.Filter) ([]resource.Resource, error) { var fetchedResources []Resource - sqlStatement := dialect.From(TABLE_RESOURCES) + sqlStatement := fromLive(TABLE_RESOURCES) + if flt.IncludeDeleted { + sqlStatement = dialect.From(TABLE_RESOURCES) + } + sqlStatement = sqlStatement.Order(goqu.C("created_at").Asc()) if flt.ProjectID != "" { sqlStatement = sqlStatement.Where(goqu.Ex{"project_id": flt.ProjectID}) } @@ -149,7 +155,7 @@ func (r ResourceRepository) GetByID(ctx context.Context, id string) (resource.Re return resource.Resource{}, resource.ErrInvalidID } - query, params, err := dialect.From(TABLE_RESOURCES).Where(goqu.Ex{ + query, params, err := fromLive(TABLE_RESOURCES).Where(goqu.Ex{ "id": id, }).ToSQL() if err != nil { @@ -189,7 +195,7 @@ func (r ResourceRepository) Update(ctx context.Context, res resource.Resource) ( "metadata": marshaledMetadata, "updated_at": goqu.L("now()"), }, - ).Where(goqu.Ex{"id": res.ID}).Returning(&ResourceCols{}).ToSQL() + ).Where(goqu.Ex{"id": res.ID}, live(TABLE_RESOURCES)).Returning(&ResourceCols{}).ToSQL() if err != nil { return resource.Resource{}, fmt.Errorf("%w: %s", errQuery, err) } @@ -221,7 +227,7 @@ func (r ResourceRepository) GetByURN(ctx context.Context, urn string) (resource. return resource.Resource{}, resource.ErrInvalidURN } - query, params, err := dialect.Select(&ResourceCols{}).From(TABLE_RESOURCES).Where( + query, params, err := fromLive(TABLE_RESOURCES).Select(&ResourceCols{}).Where( goqu.Ex{ "urn": urn, }).ToSQL() @@ -243,20 +249,23 @@ func (r ResourceRepository) GetByURN(ctx context.Context, urn string) (resource. } func (r ResourceRepository) Delete(ctx context.Context, id string) error { - query, params, err := dialect.Delete(TABLE_RESOURCES).Where( + query, params, err := dialect.Update(TABLE_RESOURCES).Set( + goqu.Record{ + "deleted_at": goqu.L("now()"), + }, + ).Where( goqu.Ex{ "id": id, }, - ).ToSQL() + live(TABLE_RESOURCES), + ).Returning(&ResourceCols{}).ToSQL() if err != nil { return fmt.Errorf("%w: %s", errQuery, err) } + var resourceModel Resource if err = r.dbc.WithTimeout(ctx, TABLE_RESOURCES, "Delete", func(ctx context.Context) error { - if _, err = r.dbc.DB.ExecContext(ctx, query, params...); err != nil { - return err - } - return nil + return r.dbc.QueryRowxContext(ctx, query, params...).StructScan(&resourceModel) }); err != nil { err = checkPostgresError(err) switch { @@ -268,3 +277,25 @@ func (r ResourceRepository) Delete(ctx context.Context, id string) error { } return nil } + +// Purge removes the row for good. The project delete cascade uses it, since a +// resource row cannot outlive its project row. +// TODO(fix): remove once project delete is soft +func (r ResourceRepository) Purge(ctx context.Context, id string) error { + query, params, err := dialect.Delete(TABLE_RESOURCES).Where( + goqu.Ex{ + "id": id, + }, + ).ToSQL() + if err != nil { + return fmt.Errorf("%w: %s", errQuery, err) + } + + if err = r.dbc.WithTimeout(ctx, TABLE_RESOURCES, "Purge", func(ctx context.Context) error { + _, err := r.dbc.DB.ExecContext(ctx, query, params...) + return err + }); err != nil { + return checkPostgresError(err) + } + return nil +} diff --git a/internal/store/postgres/resource_repository_test.go b/internal/store/postgres/resource_repository_test.go index 1bb565848..8ef259d29 100644 --- a/internal/store/postgres/resource_repository_test.go +++ b/internal/store/postgres/resource_repository_test.go @@ -291,6 +291,71 @@ func (s *ResourceRepositoryTestSuite) TestCreate() { } }) } + + s.Run("should update the live resource that already has the urn", func() { + existing := s.resources[0] + got, err := s.repository.Create(s.ctx, resource.Resource{ + URN: existing.URN, + Name: "renamed", + ProjectID: existing.ProjectID, + NamespaceID: existing.NamespaceID, + PrincipalID: existing.PrincipalID, + PrincipalType: existing.PrincipalType, + }) + if err != nil { + s.T().Fatal(err) + } + if got.ID != existing.ID || got.Name != "renamed" { + s.T().Fatalf("got %+v, expected row %s renamed in place", got, existing.ID) + } + }) + + s.Run("should create a new row when the urn belongs to a soft-deleted resource", func() { + deleted := s.resources[1] + if _, err := s.client.ExecContext(s.ctx, "UPDATE resources SET deleted_at = now() WHERE id = $1", deleted.ID); err != nil { + s.T().Fatal(err) + } + got, err := s.repository.Create(s.ctx, resource.Resource{ + URN: deleted.URN, + Name: deleted.Name, + ProjectID: deleted.ProjectID, + NamespaceID: deleted.NamespaceID, + PrincipalID: deleted.PrincipalID, + PrincipalType: deleted.PrincipalType, + }) + if err != nil { + s.T().Fatal(err) + } + if got.ID == deleted.ID { + s.T().Fatalf("got the deleted row %s back, expected a new row", deleted.ID) + } + var rows int + if err := s.client.QueryRowxContext(s.ctx, "SELECT count(*) FROM resources WHERE urn = $1", deleted.URN).Scan(&rows); err != nil { + s.T().Fatal(err) + } + if rows != 2 { + s.T().Fatalf("got %d rows with urn %s, expected the deleted row and the new one", rows, deleted.URN) + } + }) + + s.Run("should return conflict when the id belongs to a soft-deleted resource", func() { + deleted := s.resources[2] + if _, err := s.client.ExecContext(s.ctx, "UPDATE resources SET deleted_at = now() WHERE id = $1", deleted.ID); err != nil { + s.T().Fatal(err) + } + _, err := s.repository.Create(s.ctx, resource.Resource{ + ID: deleted.ID, + URN: "another-urn", + Name: deleted.Name, + ProjectID: deleted.ProjectID, + NamespaceID: deleted.NamespaceID, + PrincipalID: deleted.PrincipalID, + PrincipalType: deleted.PrincipalType, + }) + if !errors.Is(err, resource.ErrConflict) { + s.T().Fatalf("got error %v, expected %v", err, resource.ErrConflict) + } + }) } func (s *ResourceRepositoryTestSuite) TestList() { @@ -408,6 +473,68 @@ func (s *ResourceRepositoryTestSuite) TestUpdate() { }) } +func (s *ResourceRepositoryTestSuite) TestDelete() { + target := s.resources[0] + + err := s.repository.Delete(s.ctx, target.ID) + s.Require().NoError(err) + + var deleted bool + err = s.client.QueryRowxContext(s.ctx, "SELECT deleted_at IS NOT NULL FROM resources WHERE id = $1", target.ID).Scan(&deleted) + s.Require().NoError(err) + s.Assert().True(deleted) + + _, err = s.repository.GetByID(s.ctx, target.ID) + s.Assert().ErrorIs(err, resource.ErrNotExist) + + err = s.repository.Delete(s.ctx, target.ID) + s.Assert().ErrorIs(err, resource.ErrNotExist) + + err = s.repository.Delete(s.ctx, utils.NewString()) + s.Assert().ErrorIs(err, resource.ErrNotExist) +} + +func (s *ResourceRepositoryTestSuite) TestPurge() { + target := s.resources[0] + + err := s.repository.Purge(s.ctx, target.ID) + s.Require().NoError(err) + + var rows int + err = s.client.QueryRowxContext(s.ctx, "SELECT count(*) FROM resources WHERE id = $1", target.ID).Scan(&rows) + s.Require().NoError(err) + s.Assert().Equal(0, rows) + + err = s.repository.Purge(s.ctx, utils.NewString()) + s.Assert().NoError(err) +} + +func (s *ResourceRepositoryTestSuite) TestSkipsSoftDeletedResources() { + deleted := s.resources[0] + _, err := s.client.ExecContext(s.ctx, "UPDATE resources SET deleted_at = now() WHERE id = $1", deleted.ID) + s.Require().NoError(err) + + _, err = s.repository.GetByID(s.ctx, deleted.ID) + s.Assert().ErrorIs(err, resource.ErrNotExist) + + _, err = s.repository.GetByURN(s.ctx, deleted.URN) + s.Assert().ErrorIs(err, resource.ErrNotExist) + + got, err := s.repository.List(s.ctx, resource.Filter{}) + s.Assert().NoError(err) + s.Assert().Len(got, len(s.resources)-1) + for _, r := range got { + s.Assert().NotEqual(deleted.ID, r.ID) + } + + all, err := s.repository.List(s.ctx, resource.Filter{IncludeDeleted: true}) + s.Assert().NoError(err) + s.Assert().Len(all, len(s.resources)) + + _, err = s.repository.Update(s.ctx, resource.Resource{ID: deleted.ID, Title: "changed"}) + s.Assert().ErrorIs(err, resource.ErrNotExist) +} + func TestResourceRepository(t *testing.T) { suite.Run(t, new(ResourceRepositoryTestSuite)) } diff --git a/pkg/auditrecord/consts.go b/pkg/auditrecord/consts.go index db630dabb..0044154b7 100644 --- a/pkg/auditrecord/consts.go +++ b/pkg/auditrecord/consts.go @@ -75,6 +75,7 @@ const ( // Resource Events ResourceCreatedEvent Event = "resource.created" + ResourceDeletedEvent Event = "resource.deleted" // User Events UserConsentGrantedEvent Event = "user.consent_granted" diff --git a/test/e2e/regression/api_test.go b/test/e2e/regression/api_test.go index c8c3d5afd..e6c434634 100644 --- a/test/e2e/regression/api_test.go +++ b/test/e2e/regression/api_test.go @@ -2201,6 +2201,74 @@ func (s *APIRegressionTestSuite) TestResourceAPI() { s.Assert().NoError(err) s.Assert().False(checkCreatePermResp.Msg.GetStatus()) }) + s.Run("4. deleting a resource hides it and frees its urn", func() { + createOrgResp, err := s.testBench.Client.CreateOrganization(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateOrganizationRequest{ + Body: &frontierv1beta1.OrganizationRequestBody{ + Title: "org 4", + Name: "org-resource-4", + }, + })) + s.Require().NoError(err) + + createProjResp, err := s.testBench.Client.CreateProject(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateProjectRequest{ + Body: &frontierv1beta1.ProjectRequestBody{ + Name: "org-4-proj-1", + OrgId: createOrgResp.Msg.GetOrganization().GetId(), + }, + })) + s.Require().NoError(err) + projectID := createProjResp.Msg.GetProject().GetId() + + createResourceResp, err := s.testBench.Client.CreateProjectResource(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateProjectResourceRequest{ + ProjectId: projectID, + Body: &frontierv1beta1.ResourceRequestBody{ + Name: "res-1", + Namespace: computeOrderNamespace, + }, + })) + s.Require().NoError(err) + resourceID := createResourceResp.Msg.GetResource().GetId() + + _, err = s.testBench.Client.DeleteProjectResource(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteProjectResourceRequest{ + ProjectId: projectID, + Id: resourceID, + })) + s.Require().NoError(err) + + _, err = s.testBench.Client.GetProjectResource(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.GetProjectResourceRequest{ + ProjectId: projectID, + Id: resourceID, + })) + s.Assert().Equal(connect.CodeNotFound, connect.CodeOf(err)) + + listResp, err := s.testBench.Client.ListProjectResources(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.ListProjectResourcesRequest{ + ProjectId: projectID, + })) + s.Require().NoError(err) + s.Assert().Empty(listResp.Msg.GetResources()) + + _, err = s.testBench.Client.DeleteProjectResource(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteProjectResourceRequest{ + ProjectId: projectID, + Id: resourceID, + })) + s.Assert().Equal(connect.CodeNotFound, connect.CodeOf(err)) + + recreateResp, err := s.testBench.Client.CreateProjectResource(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateProjectResourceRequest{ + ProjectId: projectID, + Body: &frontierv1beta1.ResourceRequestBody{ + Name: "res-1", + Namespace: computeOrderNamespace, + }, + })) + s.Require().NoError(err) + s.Assert().NotEqual(resourceID, recreateResp.Msg.GetResource().GetId()) + + // the org now holds a live resource and a deleted one; both go with it + _, err = s.testBench.Client.DeleteOrganization(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteOrganizationRequest{ + Id: createOrgResp.Msg.GetOrganization().GetId(), + })) + s.Require().NoError(err) + }) } func (s *APIRegressionTestSuite) TestPolicyAPI() {