Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 Summary
Merge Risk: 🟡 Moderate · 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 39763193357Coverage increased (+0.03%) to 55.674%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
26f3314 to
da8dac4
Compare
|
Sandbox run: a local Frontier server with Postgres and SpiceDB in Docker, called over the Connect protocol. The binary was built from #2014 with #2011 applied, so it has main plus #2008, #2011, #2013 and #2014. Deletion config used: app:
deletion:
enabled: true
resources:
resource/aoi: { project: block, organization: allow }
resource/workflow: { project: block, organization: allow }
resource/orderitem: { project: block_unless_deletable, organization: block_unless_deletable }
resource/insight: { project: allow, organization: allow }
|
| Check | Expected | Observed |
|---|---|---|
| The refused project is untouched | row live, 5 live | row live, 5 live |
| Delete by an org viewer (no delete permission) | permission_denied | permission_denied |
| Delete an empty project (owner) | ok | ok |
| Delete the first project above after deleting its 4 blocking resources | ok | ok |
CheckProjectDelete on an org's project holding an AOI |
can_delete=false, blockers=[('resource/aoi', 1)] | can_delete=false, blockers=[('resource/aoi', 1)] |
DeleteOrganization on that org (project rules do not apply) |
ok | ok |
With enabled: false, delete that project |
ok | ok |
| if !utils.IsValidUUID(id) { | ||
| return project.ErrInvalidUUID | ||
| } | ||
| projects, err := d.projService.List(ctx, project.Filter{ProjectIDs: []string{id}, IncludeDisabled: true}) |
There was a problem hiding this comment.
DeleteProject should be safe to call twice. After the first delete, the project is gone from the live list, so a second call returns project.ErrNotExist, which the handler turns into not_found. The ErrNotExist check on DeleteModel below was added for retries, and this lookup makes it unreachable. Please return success when the project is already deleted.
The authorization check runs before this code. Once the project's relations are removed, a repeat call may be stopped there. Please make sure a project that is not found in authz is handled the same way, so a retry does not fail.
This RPC works with ids only, so the name based lookup for projects can go.
There was a problem hiding this comment.
The deleter now returns success when the project is not found, and skips a policy that is already gone.
The permission check stays as it is. After a finished delete the project has no SpiceDB links, so a repeat call through the API answers permission_denied before it reaches the deleter. Today a caller without access gets permission_denied for a live project id and for a missing one alike. Letting a missing id through would answer success for it, so such a caller could tell which ids are live projects.
A project name passes the permission check and is then refused by the uuid check in the deleter, so this RPC still deletes by id only.
DeleteProject runs the RESOURCES_EXIST check and returns a BlockedError naming the project, which the API answers with failed_precondition and one violation per blocking namespace. The org delete removes its projects without that check. An unknown project or an id that is not a uuid answers not found. The API e2e suite runs with the deletion config on.
DeleteProject returns success when no live project has the id, and the project delete skips a policy that is already gone, so a repeat delete does not fail in the deleter.
0568226 to
74cb6ad
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Source mode: Auto
- Repository: raystack/frontier
- Branch: main
- File: .coderabbit.yaml
- Commit: 8053fb3
- Layers: Repository YAML
- Review profile: CHILL
- Plan: Advanced
- Run ID:
48c82393-7a50-4614-bcc7-ee1b85e8f741
📒 Files selected for processing: 2
core/deleter/service.gocore/deleter/service_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.
| if len(projects) == 0 { | ||
| return nil | ||
| } | ||
| blockers, err := d.ProjectResourceBlockers(ctx, id) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
set -eu
printf '%s\n' '--- changed files ---'
git diff --name-only 8053fb367641cd0da8c37604f5b67a8c8b6fcfda 74cb6ad54ffc05e1a42857713b2c191b822f58f9
printf '%s\n' '--- deletion symbols and references ---'
rg -n -F --glob '*.go' -e 'ProjectResourceBlockers' -e 'func (d *Deleter) deleteProject' -e 'deleteProject(ctx' -e 'DeleteProject' core internal pkg
printf '%s\n' '--- service.go ---'
nl -ba core/deleter/service.go | sed -n '180,280p'
printf '%s\n' '--- deleter.go ---'
nl -ba core/deleter/deleter.go | sed -n '1,180p'
printf '%s\n' '--- resource-related directories ---'
find core internal pkg -type f -name '*.go' | grep -E '/(resource|aoi|workflow|order|namespace|project)/' | head -200Repository: raystack/frontier
Length of output: 18798
🏁 Script executed:
set -eu
printf '%s\n' '--- changed files ---'
git diff --name-only 8053fb367641cd0da8c37604f5b67a8c8b6fcfda 74cb6ad54ffc05e1a42857713b2c191b822f58f9
printf '%s\n' '--- deletion symbols and references ---'
rg -n -F --glob '*.go' -e 'ProjectResourceBlockers' -e 'func (d *Deleter) deleteProject' -e 'deleteProject(ctx' -e 'DeleteProject' core internal pkg || test "$?" -eq 1
printf '%s\n' '--- service.go ---'
nl -ba core/deleter/service.go | sed -n '180,280p'
printf '%s\n' '--- deleter.go ---'
nl -ba core/deleter/deleter.go | sed -n '1,180p'
printf '%s\n' '--- resource-related files ---'
find core internal pkg -type f -name '*.go' | grep -E '/(resource|aoi|workflow|order|namespace|project)/' | head -200Repository: raystack/frontier
Length of output: 18792
🏁 Script executed:
set -eu
printf '%s\n' '--- blocker implementation ---'
nl -ba core/deleter/resource_blockers.go | sed -n '1,220p'
printf '%s\n' '--- resource service declarations and create path ---'
rg -n -F --glob '*.go' -e 'func (.*) Create' -e 'Create(ctx' -e 'CreateResource' -e 'ResourceService' core/resource internal/api | head -240
nl -ba core/resource/service.go | sed -n '1,260p'
printf '%s\n' '--- resource repository declarations and implementations ---'
rg -n -F --glob '*.go' -e 'type Repository interface' -e 'func (.*) Create' -e 'INSERT INTO' -e 'ProjectID' core/resource internal pkg | head -300
printf '%s\n' '--- transaction and lock references around project/resource operations ---'
rg -n -i --glob '*.go' -e 'transaction' -e 'tx\.' -e 'for update' -e 'advisory' -e 'mutex' -e 'lock' core/deleter core/resource core/project internal/api | head -300Repository: raystack/frontier
Length of output: 42220
🏁 Script executed:
set -eu
printf '%s\n' '--- blocker implementation ---'
nl -ba core/deleter/resource_blockers.go | sed -n '1,180p'
printf '%s\n' '--- resource create symbols ---'
rg -n -F --glob '*.go' -e 'func (s Service) Create' -e 'func (.*) Create' -e 'Create(ctx' core/resource core/namespace internal/api | head -240
printf '%s\n' '--- resource service ---'
nl -ba core/resource/service.go | sed -n '1,280p'
printf '%s\n' '--- resource repository interfaces and implementations ---'
rg -n -F --glob '*.go' -e 'type Repository interface' -e 'Create(ctx context.Context' -e 'DeleteModel(ctx context.Context' -e 'Purge(ctx context.Context' core/resource core/project pkg | head -300
printf '%s\n' '--- synchronization and transaction references ---'
rg -n -i --glob '*.go' -e 'transaction' -e 'for update' -e 'advisory' -e 'mutex' -e 'lock' core/deleter core/resource core/project internal/api | head -300Repository: raystack/frontier
Length of output: 42043
🏁 Script executed:
set -eu
printf '%s\n' '--- complete resource Create block ---'
nl -ba core/resource/service.go | sed -n '80,175p'
printf '%s\n' '--- resource endpoint block ---'
nl -ba internal/api/v1beta1connect/resource.go | sed -n '205,240p'
printf '%s\n' '--- resource repository Create declaration ---'
nl -ba core/resource/resource.go | sed -n '1,45p'Repository: raystack/frontier
Length of output: 7080
🏁 Script executed:
set -eu
printf '%s\n' '--- concrete resource persistence ---'
rg -n -F --glob '*.go' -e 'type ResourceRepository' -e 'func (.*Resource.*) Create' -e 'func (.*resource.*) Create' -e 'func (.*) DeleteModel' internal/store core
printf '%s\n' '--- candidate resource repository ---'
nl -ba internal/store/postgres/resource_repository.go | sed -n '1,260p'
printf '%s\n' '--- project persistence declarations and implementations ---'
rg -n -F --glob '*.go' -e 'DeleteModel' -e 'type ProjectRepository' internal/store core/projectRepository: raystack/frontier
Length of output: 11255
Serialize resource creation with project deletion.
DeleteProject checks blockers, then deleteProject lists resources again. Resource creation remains possible between these calls. A newly created blocking resource can be found by the second list, purged, and omitted from the BlockedError, so the delete can return success. Coordinate resource creation and deletion so this interval cannot admit new resources.
This branch was successfully deployed
3 participants
Summary
DeleteProjectnow refuses while the project's resources block the delete, using theRESOURCES_EXISTcheck from #2005. The refusal isfailed_preconditionwith onegoogle.rpc.PreconditionFailureviolation per blocking namespace. The organization delete removes its projects without the project check, so a resource that blocks a project delete does not stop an organization delete. No delete is refused whileapp.deletionis disabled. A project that is not found counts as deleted, so a repeat delete does not fail in the deleter.Changes
core/deleter/service.go:DeleteProjectlooks the project up by id withIncludeDisabled. A project that is not found returns success and nothing is removed; an id that is not a uuid returnsproject.ErrInvalidUUID.ProjectResourceBlockers, and returns aBlockedErrornaming the project when anything blocks.deleteProjectholds the removal steps and skips the blocker check, because what blocks a project delete must not block an organization delete.DeleteOrganizationcalls it for each project. A policy that is already gone is skipped.core/deleter/deleter.go:BlockedErrorgainsProjectID.Error()names the project or the organization and leaves out empty messages, so a project refusal readsproject cannot be deleted yet.internal/api/v1beta1connect/deleter.go:DeleteProjectanswersfailed_preconditionwith the blockers, andnot_foundfor an id that is not a uuid. The blocked-delete log lines carryproject_idand no longer say organization.test/e2e/regression/api_test.go: the API suite runs withapp.deletionenabled and a rule forcompute/order(project: block,organization: allow).Technical Details
failed_preconditionis HTTP 400. A violation carries the blocker type and the namespace;PreconditionFailurehas no field for the count.DeleteProjectwith a project name answered 500 before; it now answersnot_found.DeleteProjectthrough the API is refused by the permission check withpermission_denied, because the delete removed the project's SpiceDB relations. It does not reach the deleter.OrganizationResourceBlockershas no caller yet, so the organization delete applies no resource rule and theorganizationrules of the deletion config are ignored for now. feat(deleter): org delete refuses while resources block it #2017 wires it into the organization delete.Test Plan
core/deleter/service_test.goTestDeleteProject: a blocked project returns theBlockedErrorand removes nothing; a project that is not found returns success and removes nothing; a policy that is already gone does not stop the delete; a name returnsproject.ErrInvalidUUID; the existing removal cases expect the lookup.TestDeleteOrganization: with the deletion config on, an org whose project holds a resource that blocks a project delete is deleted, and the resource is removed.internal/api/v1beta1connect/deleter_test.goTestHandler_DeleteProject: a blocked delete answersfailed_preconditionwith oneRESOURCES_EXISTviolation; a missing project and a name answernot_found.TestProjectDeleteRefusedWhileResourcesBlock: a project holding acompute/orderresource is refused with that violation and stays readable; after the resource is deleted, the project delete succeeds; an org whose project still holds the resource is deleted.go test ./core/... ./internal/api/... ./pkg/server/...passes.TestEndToEndAPIRegressionTestSuite,TestEndToEndServiceUsersRegressionTestSuiteandTestEndToEndPATRegressionTestSuitepass.golangci-lint run ./core/deleter/... ./internal/api/v1beta1connect/... ./test/e2e/regression/...reports no issues.