feat(store): policy and relation deletes keep the row - #1968
Conversation
The plain unique constraints on policies and relations are replaced by partial unique indexes over rows where deleted_at is null, and both upserts name those indexes in their conflict target. A soft-deleted row no longer blocks the same tuple from being written again, and a duplicate of a live row still updates it in place.
Policy delete, the owner-guarded policy delete, and relation delete set deleted_at instead of removing the row. The SpiceDB tuples are still removed by the services as before. A delete that finds no live row reports not found, so a second delete of the same policy or relation no longer writes an audit record or passes silently.
Policy rows now stay after a delete, so the org PATs listing filters the policies join on deleted_at. A PAT whose policy was removed lists with no scope instead of its old role.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
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
WalkthroughPolicy and relation records now use live-row uniqueness and soft deletion. Upserts can create new rows after deletion, while repeated deletion returns not found. The migration, repository tests, PAT query test, and API regression test cover these changes. ChangesLive Policy and Relation Lifecycle
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature Suggested reviewers: Merge Risk: 🟡 Moderate · up to The change moves policy and relation deletes to soft deletes. During rollout, writes from the old binary can fail between the migration and the new binary starting. The migration can also block writes while it runs, and rolling it back can fail once policies or relations have been deleted and recreated. Plan the rollout (drain old writers, schedule the migration) and decide how rollback should work before merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The new schema can reject grant and membership writes from older running versions, and deletion followed by recreation can prevent the supplied rollback from completing. Existing resource-scoped authorization checks remain in place, but deployment ordering and recovery require an explicit plan. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
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 37427502063Coverage increased (+0.009%) to 54.353%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: raystack/frontier/.coderabbit.yaml
- Review profile: CHILL
- Plan: Advanced
- Run ID:
6c8a7ed3-6952-419e-8302-fe4b8506b571
📒 Files selected for processing (9)
internal/store/postgres/migrations/20261005100000_policies_relations_live_unique.down.sqlinternal/store/postgres/migrations/20261005100000_policies_relations_live_unique.up.sqlinternal/store/postgres/org_pats_repository.gointernal/store/postgres/org_pats_repository_test.gointernal/store/postgres/policy_repository.gointernal/store/postgres/policy_repository_test.gointernal/store/postgres/relation_repository.gointernal/store/postgres/relation_repository_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; 1 remain after this review.
Removed comment about PAT with no live policy listing.
What
deleted_atinstead of removing the row. The SpiceDB tuples are still removed by the services, in the same place as before.policiesandrelationsare replaced by partial unique indexes over live rows,uq_policies_role_resource_principal_liveanduq_relations_subject_object_relation_live. Both upserts name those indexes in their conflict target throughliveConflictTarget. Migration and query change ship together.Why
Postgres keeps the history and SpiceDB holds only live access. A permission check on a removed membership fails because no tuple is left, and nothing replays the relations table into SpiceDB. With the old plain constraints, a kept row would block the same tuple forever: a removed member could not rejoin, and a deleted policy could not be granted again. Reads already skip deleted rows on
main(#1936).Behaviour change
DeleteRelationorRemovePlatformUseris unchanged: the service finds nothing and returns success, as it does today for a relation that never existed.deleted_atset.Rollout
Between the migration running and the new binary starting, the old binary's plain
ON CONFLICTno longer matches an index, so policy and relation writes (membership changes, org and project creation) fail for that window with SQL state 42P10. Keeping the old constraint for one release is not an option: with both present, the new target still matches the old constraint, and a re-add would update the deleted row and return it. The down migration fails if a deleted and a live row share a tuple by then.Tested
GetandGetByFields.TestRelationAPI: a secondDeletePolicyreturns not found, the same policy can be created again with a new id, and the user's access returns.SQL Safety
goqu.Ex{}andgoqu.Record{}; the conflict target andnow()are constants.ToSQL()params are forwarded unchanged. The raw guarded-delete statement keeps its$1to$4parameters; only the verb changed.?inside quoted SQL literals.//nolintor#nosecannotations.