feat(resource): soft-delete resources and keep the URN unique over live rows only - #1937
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: raystack/frontier/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughResource deletion now soft-deletes resources and records an audit event after successful service deletion. Repository queries exclude deleted resources by default and allow live resources to reuse deleted URNs. Project cleanup permanently purges both live and deleted resources. ChangesResource lifecycle
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Suggested reviewers: Merge Risk: 🟡 Moderate · up to Resources can be deleted and recreated under the same URN, but that state blocks migration rollback. Resolve or explicitly accept this rollback limitation before merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Resource creation and deletion now depend on a coordinated database and application rollout. An incompatible deployment order can interrupt resource creation, and reusing deleted resources’ identifiers can prevent a straightforward database rollback. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
Coverage Report for CI Build 36678992556Coverage increased (+0.08%) to 52.86%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: raystack/frontier/.coderabbit.yaml
Review profile: CHILL
Plan: Advanced
Run ID: 80fa62af-722a-46c0-a014-7eada53e34b6
📒 Files selected for processing (8)
core/resource/service.gocore/resource/service_test.gointernal/store/postgres/migrations/20260918100000_resources_urn_live_unique.down.sqlinternal/store/postgres/migrations/20260918100000_resources_urn_live_unique.up.sqlinternal/store/postgres/postgres.gointernal/store/postgres/resource_repository.gointernal/store/postgres/resource_repository_test.gopkg/auditrecord/consts.go
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.
|
End-to-end run of the resource RPCs on this branch (commit 0cd9a44), against a local Frontier with Postgres 15 and SpiceDB 1.34 in Docker. Calls were made as a super admin, with a regular user as the resource owner. 31 checks, all passed.
Note on the 403: the authorization rule for |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: raystack/frontier/.coderabbit.yaml
Review profile: CHILL
Plan: Advanced
Run ID: c20f423b-c9fb-41af-9fa2-9febe9d49f43
📒 Files selected for processing (9)
core/deleter/mocks/resource_service.gocore/deleter/service.gocore/deleter/service_test.gocore/resource/mocks/repository.gocore/resource/resource.gocore/resource/service.gocore/resource/service_test.gointernal/store/postgres/resource_repository.gointernal/store/postgres/resource_repository_test.go
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.
8ffcf75 to
fe03156
Compare
…ve rows only DeleteProjectResource now sets deleted_at instead of removing the row, and reads, updates, and deletes skip rows that have it set. The URN unique constraint becomes a partial unique index over live rows, and the upsert names it, so a deleted URN can be used again. The delete also writes a resource.deleted audit record.
…r lists The project delete cascade removes the project row for good, and a resource row cannot outlive the project it points to, so the cascade purges resources instead of soft-deleting them until project delete is soft. The resource list is ordered by created_at so its order no longer depends on the query plan.
The project cleanup listed only live resources, so a resource deleted through the API kept pointing at the project and the project row could not be removed. The cleanup now lists deleted rows as well and purges them. A new e2e case covers the soft delete of a resource and the org delete that follows.
…ict target helper
…n main The rebase onto main brought in a second TestSkipsSoftDeletedResources from the read-filter change. Keep that one and add the IncludeDeleted list check to it.
…Delete A unit test asserts the exact ON CONFLICT clause the helper produces, for a one-column and a two-column target, so a goqu change fails there instead of at runtime. The resource delete now uses the shared softDelete helper.
fe03156 to
52530f1
Compare
What
DeleteProjectResourcesetsdeleted_aton the resource row instead of removing it. SpiceDB tuples are still removed.Deleteonly touches live rows, so a second delete returns not-found. Reads and updates already skip deleted rows onmain.resources_urn_keyconstraint is replaced by the partial unique indexuq_resources_urn_liveover live rows. The upsert's conflict target names that index, through a newliveConflictTargethelper inpostgres.go. Migration and query change ship together.resource.deletedaudit record. Theapp.resource.deletedaudit log is unchanged.IncludeDeletedon the resource filter, so they are purged as well. Both carry aTODO(fix)to switch to the soft delete once project delete is soft.created_at, so its order no longer depends on the query plan.Why
Soft delete keeps the row. A deleted resource must stop appearing in reads, and must not hold its URN forever. With the old plain constraint, a URN could never be used again once its resource was deleted.
Behaviour change
A deleted resource stays in the table and is hidden from the API. A create with the URN of a deleted resource inserts a new row, and the deleted row stays as history. A create with the URN of a live resource still updates it in place.
Rollout
Between the migration running and the new binary starting, the old binary's
ON CONFLICT (urn)no longer matches an index, so resource creates fail for that window. Once a resource has been soft-deleted in an environment, a release without this change would show it as live again, and the down migration fails if a deleted row and a live row share a URN.Tested
deleted_at, and later gets, lists, updates, and deletes skip the row.liveConflictTargetasserting the exactON CONFLICT (urn) WHERE (deleted_at IS NULL)clause, and the two-column form.TestOrganizationAPIandTestResourceAPIpass locally against Docker.golangci-lintreports no issues on the changed packages.SQL Safety
goqu.Ex{}andgoqu.Record{}. TheON CONFLICTtarget is built with+inliveConflictTargetfrom a constant column list, never from caller input, and a unit test pins the exact clause it produces.now()is a constant.ToSQL()params are forwarded unchanged.?placeholders inside quoted SQL literals.//nolintor#nosecannotations.