Repository navigation
feat(store): group delete keeps the row(soft delete) - #2016
Conversation
The group row now gets deleted_at instead of being removed. Deleting a group that is already deleted, or does not exist, still returns not found. Tests check that the row stays, reads skip it, no live policy is left on the group or names it, and the name can be used again, after DeleteGroup and after an org delete.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 Summary
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Coverage Report for CI Build 39775872666Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.06%) to 55.65%Details
Uncovered Changes
Coverage Regressions275 previously-covered lines in 7 files lost coverage.
Coverage Stats
💛 - Coveralls |
rohilsurana
left a comment
There was a problem hiding this comment.
Two more points:
core/deleter/service.go:341: the group loop inDeleteOrganizationdoes not skipgroup.ErrNotExist, while the resource and domain loops do. Please handleErrNotExistthe same way in every delete loop, so a retry behaves the same everywhere.- A failed group create now leaves a deleted group row, like the project case. Please add groups to #2009 if they are not already there.
The group loop in DeleteOrganization skips group.ErrNotExist, like the resource and domain loops. The group e2e asserts PermissionDenied for GetGroup and a second DeleteGroup on a deleted group.
|
On the two points in the review:
|
There was a problem hiding this comment.
🧹 Nitpick comments: 1
core/deleter/service_test.go: 1
454-456: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExercise the group repository not-found path.
core/deleter.Service.DeleteGroupreturns immediately whenOnGroupDeletedreturns an error. The current setup therefore skipsGroupService.DeleteModelforgrp-gone.Return
nilfromOnGroupDeletedand returngroup.ErrNotExistfromDeleteModel:Suggested test correction
- m.mbrSvc.EXPECT().OnGroupDeleted(mock.Anything, "grp-gone").Return(group.ErrNotExist) + m.mbrSvc.EXPECT().OnGroupDeleted(mock.Anything, "grp-gone").Return(nil) + m.grpSvc.EXPECT().DeleteModel(mock.Anything, "grp-gone").Return(group.ErrNotExist)This makes
DeleteGroupreturn the repository error, soDeleteOrganizationreaches its existinggroup.ErrNotExisthandling branch.
ℹ️ Review info
⚙️ Run configuration
- Source mode: Auto
- Repository: raystack/frontier
- Branch: main
- File: .coderabbit.yaml
- Commit: 358f8dd
- Layers: Repository YAML
- Review profile: CHILL
- Plan: Advanced
- Run ID:
61aa255e-1795-4eca-91be-a3018ef29349
📒 Files selected for processing: 3
core/deleter/service.gocore/deleter/service_test.gotest/e2e/regression/api_test.go
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review.
Summary
DeleteGroupnow keeps the group row and setsdeleted_aton it, instead of removing it. Group reads already skip deleted rows, and the name is free again becauseuq_groups_org_id_name_livecovers live rows only. No table has a foreign key togroups.The rest of the delete is unchanged:
OnGroupDeletedstill removes the members' policies and relations, the policies where the group is the principal, and the link between the org and the group. Theapp.group.deletedaudit log is still written. The org delete deletes every group of the org the same way, disabled ones included.Changes
internal/store/postgres/group_repository.go:DeleteusessoftDelete. When no live row matches, it returnsgroup.ErrNotExist, as before.internal/store/postgres/group_repository_test.go:TestDeletecovers the kept row, a second delete, an unknown id, an id that is not a uuid, and reusing the name.test/e2e/regression/api_test.go:TestGroupAPIcase 11 andTestOrganizationDeleteAlsoDeletesDisabledProjectsAndGroupscheck the kept rows. The org test's subtest "no group row of the org is left" is renamed to "no group of the org is listed", since the row now stays.Technical Details
group.ErrNotExistfrom the repository, as before. Through the API it is refused earlier, at the permission check, because the group's tuples are already gone.Test Plan
TestDelete: the row stays withdeleted_atset and reads no longer find it; a second delete and an unknown id returngroup.ErrNotExist; an id that is not a uuid, and an empty id, returngroup.ErrInvalidUUID; a new group in the same org can take the name.TestGroupAPIcase 11: afterDeleteGroup, the row hasdeleted_atset,GetGroupand a secondDeleteGroupare refused, and a new group with the same name is created and listed while the deleted one is not.TestOrganizationDeleteAlsoDeletesDisabledProjectsAndGroups: after the org delete, the enabled and the disabled group both havedeleted_atset, and no live policy is on either group or names it as principal. Before the delete, the disabled group holds a policy on the org as principal.go test ./internal/store/postgres/...passes.go test ./test/e2e/...passes.golangci-lint run ./internal/store/postgres/... ./test/e2e/...reports no issues.SQL Safety
goqu.Ex{}. The onlygoqu.Lisnow()insidesoftDelete, with no values.ToSQL()callers capture and forward params (query, params, err := stmt.ToSQL(); db.…Context(ctx, …, query, params...)). Neverquery, _, err := ….?placeholders inside single-quoted SQL literals ingoqu.L.//nolint:forbidigoor// #nosec G20xannotations.