HYPERFLEET-1155 - feat: add force-delete endpoint for generic resources#266
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as ResourceHandler
participant Service as ResourceService
participant DAO as resourceDao
Client->>Handler: POST /{plural}/{id}/force-delete {reason}
Handler->>Handler: validate reason length
Handler->>Service: ForceDelete(kind, id, reason)
Service->>DAO: GetForUpdate(kind, id)
Service->>Service: verify DeletedTime != nil
Service->>Service: forceDeleteResourceTree (recursive on children)
Service->>DAO: Delete(resource)
Service-->>Handler: nil or *ServiceError
Handler-->>Client: 204 No Content or error status
Related PRs: No related PRs identified from provided information. Suggested labels: enhancement, needs-security-review Suggested reviewers: No reviewer information provided. CWE-284 (Improper Access Control): CWE-863 (Incorrect Authorization): No RBAC/role gating visible in the handler code for force-delete routes — confirm authorization middleware is applied upstream of these new routes; force-deletion is destructive and irreversible (hard delete, not soft delete). CWE-362 (Race Condition): No dependency, CI/CD, IDE config, or 🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 3 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 816 lines (>500) | +2 |
| Sensitive paths | none | +0 |
| Test coverage | Missing tests for: plugins/entities | +1 |
Computed by hyperfleet-risk-scorer
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/services/resource_test.go`:
- Around line 1318-1328: The tripwire test is only validating local fixtures
because setupTestDescriptors() rebuilds the registry, so it never checks real
plugin descriptors. Move the RequiredAdapters assertion out of
TestAllGenericDescriptors_HaveNoRequiredAdapters and into a test that loads the
actual generic-resource plugins, then iterate registry.All() to verify the
registered descriptors from the real registry. Keep the existing Expect check
and failure message, but ensure it runs against the production plugin
registration path rather than the test fixture registry.
In `@test/integration/resource_force_delete_test.go`:
- Around line 110-139: The NestedVersionForceDelete subtest in the resource
force-delete integration test only verifies that the Version identified by
versionID is removed, but it does not confirm that the parent channel remains
intact. Update this test around the existing ForceDelete and checkResourceCount
assertions to also assert that channel.ID still exists after the nested version
delete, using the existing helpers in setupResourceTest, createChannel, and
checkResourceCount so the parent scope is explicitly validated.
- Around line 22-141: The current `TestResourceForceDelete` suite only calls
`svc.ForceDelete`, so it misses regressions in
`pkg/handlers/resource_handler.go` and the route/plugin wiring for `POST
/{plural}/{id}/force-delete` and the nested owner-scoped force-delete path.
Update these subtests to use the integration HTTP setup from
`test.RegisterIntegration(t)` (helper, client) with Resty and Gomega, send real
requests through the handler routes, and assert on response status/body plus
`reason` validation instead of only checking `ServiceError.HTTPCode`. Keep the
DB cleanup assertions, but locate the tests by `TestResourceForceDelete`,
`setupResourceTest`, and `ForceDelete` when refactoring.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: cd5c2875-03f9-424c-a4fe-6dd27eb0ccdd
📒 Files selected for processing (8)
pkg/handlers/resource_handler.gopkg/handlers/resource_handler_test.gopkg/services/resource.gopkg/services/resource_test.goplugins/channels/plugin.goplugins/versions/plugin.goplugins/wifconfigs/plugin.gotest/integration/resource_force_delete_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
kuudori
force-pushed
the
HYPERFLEET-1155-feat-force-delete-generic-resources
branch
from
8779683 to
c0d96a5
Compare
|
JIRA description says "soft-deleting" but per ADR-0013 the intent is hard-delete. Worth updating the ticket wording to avoid future confusion. |
kuudori
force-pushed
the
HYPERFLEET-1155-feat-force-delete-generic-resources
branch
from
c0d96a5 to
1c2f75c
Compare
| if _, err := h.service.GetByOwner(r.Context(), h.descriptor.Kind, id, parentID); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if err := h.service.ForceDelete(r.Context(), h.descriptor.Kind, id, req.Reason); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
nit: Wdyt about creating a ForceDeleteByOwner(ctx, kind, id, parentID, reason) for this one?
The two-call approach works correctly (no TOCTOU — ForceDelete re-locks via GetForUpdate), but breaks the handler's "validate request, delegate once" pattern.
There was a problem hiding this comment.
I'd stick with the current implementation. The handler just calls the service layer, no additional logic 🙂
A dedicated service method would also create drift if the delete flow changes later...
kuudori
force-pushed
the
HYPERFLEET-1155-feat-force-delete-generic-resources
branch
from
1c2f75c to
8da9e50
Compare
Add POST /{plural}/{id}/force-delete to ResourceHandler for all generic
entity types (channels, versions, wifconfigs). The endpoint bypasses
OnParentDelete policies and recursively hard-deletes the entire ownership
tree for resources in Finalizing state (deleted_time IS NOT NULL).
Key design decisions per ADR-0013 (DB-only force-delete):
- Requires Finalizing state and a reason field in the request body
- Bottom-up hard-delete ordering per ADR-0012
- Fail-loud guard if RequiredAdapters non-empty (tripwire for HYPERFLEET-1154)
- MarkForRollback on error prevents partial tree deletion from committing
- Audit log with structured fields before each resource deletion
Includes unit tests (handler + service + tripwire) and integration tests
confirming force-delete succeeds where normal DELETE returns 409 due to
Restrict policy.
kuudori
force-pushed
the
HYPERFLEET-1155-feat-force-delete-generic-resources
branch
from
8da9e50 to
9a28571
Compare
|
/test unit |
|
/test presubmits-integration |
3 participants
Summary
POST /{plural}/{id}/force-deletetoResourceHandlerfor all generic entity types (channels, versions, wifconfigs)Design
Per ADR-0013 (DB-only force-delete) and ADR-0012 (bottom-up deletion ordering):
deleted_time IS NOT NULL(409 otherwise)validateNotEmpty+validateMaxLengthforceDeleteResourceTreewalks all children regardless of OnParentDelete policy, hard-deletes bottom-upMarkForRollbackon any error prevents partial commitsTestAllGenericDescriptors_HaveNoRequiredAdaptersbreaks CI when assumption changesTest plan
TestAllGenericDescriptors_HaveNoRequiredAdaptersmake verify-allpasses (1322 tests)make test-integrationpasses (force-delete scenarios)