Skip to content

fix: clean up project-level policies on project deletion#1530

Merged
AmanGIT07 merged 3 commits intomainfrom
fix/cleanup-project-policies-on-project-deletion
Apr 10, 2026
Merged

fix: clean up project-level policies on project deletion#1530
AmanGIT07 merged 3 commits intomainfrom
fix/cleanup-project-policies-on-project-deletion

Conversation

@AmanGIT07
Copy link
Copy Markdown
Contributor

Summary

  • DeleteProject in the cascade deleter was not cleaning up project-level policies before deleting the project model
  • This left orphaned policy rows in Postgres and orphaned rolebinding tuples in SpiceDB (bearer + role relations) for all principal types (users, service users, PATs)
  • The grant relation (project → granted → rolebinding) was cleaned by DeleteModel, but the rolebinding's own relations (rolebinding → bearer → principal, rolebinding → role → role) and the policy row were not

The fix adds a policyService.List + policyService.Delete loop to DeleteProject, matching the existing pattern used in DeleteOrganization for org-level policies.

What was orphaned (before fix)

rolebinding:X → bearer → user/serviceuser/pat ← orphaned in SpiceDB + Postgres
rolebinding:X → role → role:Y ← orphaned in SpiceDB + Postgres
policies table row (resource_id=projectID) ← orphaned in Postgres

Test plan

  • Manually tested by script that creates a project with user + PAT policies, deletes the project, verifies all SpiceDB tuples are cleaned up and org-level PAT access is preserved

@vercel
Copy link
Copy Markdown

vercel bot commented Apr 10, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
frontier Ready Ready Preview, Comment Apr 10, 2026 10:05am

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 10, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: de0794c0-b15e-4476-898a-40f79cfa4182

📥 Commits

Reviewing files that changed from the base of the PR and between 4ef720c and 8e03d0d.

📒 Files selected for processing (1)
  • core/deleter/service_test.go
✅ Files skipped from review due to trivial changes (1)
  • core/deleter/service_test.go

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes

    • Project deletion now removes all associated project-level policies before deleting resources and the project record, preventing orphaned policies.
  • Tests

    • Added comprehensive cascade-deletion tests for project, organization, customer, and user deletion flows, including error and edge cases.
  • Chores

    • Added autogenerated test mocks for multiple service interfaces to support the new tests.

Walkthrough

Service.DeleteProject now lists and deletes project-level policies before deleting resources and the project model. Many autogenerated testify/mock mocks were added under core/deleter/mocks/. New tests exercise cascade deletion flows and error cases for project, organization, customers, and user deletions.

Changes

Cohort / File(s) Summary
Service deletion logic
core/deleter/service.go
DeleteProject now calls policyService.List to enumerate project policies and invokes policyService.Delete for each before deleting resources and the project model; returns listing errors or wrapped errors identifying any failed policy deletion.
Autogenerated mocks
core/deleter/mocks/customer_service.go, core/deleter/mocks/group_service.go, core/deleter/mocks/invitation_service.go, core/deleter/mocks/invoice_service.go, core/deleter/mocks/organization_service.go, core/deleter/mocks/policy_service.go, core/deleter/mocks/project_service.go, core/deleter/mocks/resource_service.go, core/deleter/mocks/role_service.go, core/deleter/mocks/subscription_service.go, core/deleter/mocks/user_service.go
Adds mock implementations (testify/mock) for multiple deleter service interfaces: mock types embedding mock.Mock, EXPECT() helpers, typed call builders (Run/Return/RunAndReturn), and NewXxxService constructors that register test cleanup. Reviewers should scan generated code for correctness of signatures and test cleanup hooks.
Tests
core/deleter/service_test.go
New test suite validating cascade deletion: TestDeleteProject, TestDeleteOrganization, TestDeleteCustomers, and TestDeleteUser; covers ordering, success cases, and failure propagation (policy listing/deletion, billed-customer invoice blocking).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • whoAbhishekSah
  • rohilsurana

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@AmanGIT07 AmanGIT07 enabled auto-merge (squash) April 10, 2026 08:14
@coveralls
Copy link
Copy Markdown

coveralls commented Apr 10, 2026

Coverage Report for CI Build 24237674629

Warning

Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes.
Quick fix: rebase this PR. Learn more →

Coverage increased (+0.3%) to 41.606%

Details

  • Coverage increased (+0.3%) from the base build.
  • Patch coverage: 11 of 11 lines across 1 file are fully covered (100%).
  • 58 coverage regressions across 2 files.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

58 previously-covered lines in 2 files lost coverage.

File Lines Losing Coverage Coverage
core/organization/service.go 48 51.3%
core/project/service.go 10 80.69%

Coverage Stats

Coverage Status
Relevant Lines: 36442
Covered Lines: 15162
Line Coverage: 41.61%
Coverage Strength: 11.89 hits per line

💛 - Coveralls

Comment thread core/deleter/service.go
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
core/deleter/service_test.go (1)

50-63: Add explicit ordering assertions to enforce cascade sequence intent.

The test name and structure imply a strict deletion order (policies → resources → project), but the mock expectations don't enforce this constraint. Without InOrder() or sequence assertions, testify allows expectations to be satisfied in any order. If the implementation is later refactored (e.g., to concurrent deletes), this test would still pass even if the order changes. Use mock.InOrder() to wrap the expectations and make the ordering requirement explicit.

Also applies to: 122-131


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d11c6266-92f3-4259-a387-6ba0e4f4a711

📥 Commits

Reviewing files that changed from the base of the PR and between 8dc47ff and 4ef720c.

📒 Files selected for processing (12)
  • core/deleter/mocks/customer_service.go
  • core/deleter/mocks/group_service.go
  • core/deleter/mocks/invitation_service.go
  • core/deleter/mocks/invoice_service.go
  • core/deleter/mocks/organization_service.go
  • core/deleter/mocks/policy_service.go
  • core/deleter/mocks/project_service.go
  • core/deleter/mocks/resource_service.go
  • core/deleter/mocks/role_service.go
  • core/deleter/mocks/subscription_service.go
  • core/deleter/mocks/user_service.go
  • core/deleter/service_test.go
✅ Files skipped from review due to trivial changes (2)
  • core/deleter/mocks/user_service.go
  • core/deleter/mocks/subscription_service.go

Comment thread core/deleter/service_test.go
@AmanGIT07 AmanGIT07 disabled auto-merge April 10, 2026 10:07
@AmanGIT07 AmanGIT07 merged commit 03d3faa into main Apr 10, 2026
8 checks passed
@AmanGIT07 AmanGIT07 deleted the fix/cleanup-project-policies-on-project-deletion branch April 10, 2026 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants