HYPERFLEET-1157 - feat: generic root endpoints and flat child routes#272
HYPERFLEET-1157 - feat: generic root endpoints and flat child routes#272kuudori wants to merge 2 commits into
Conversation
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.
|
Skipping CI for Draft Pull Request. |
|
[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 |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds DAO/service lookup by ID, resource force-delete with recursive child deletion, root Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Plugin as RegisterEntityRoutes
participant Handler as RootResourceHandler
participant Service as sqlResourceService
participant DAO as sqlResourceDao
Client->>Plugin: startup with routes + schema validator
Plugin->>Handler: wire /resources and /{plural} routes
Client->>Handler: POST /resources/{id}/force-delete
Handler->>Service: ForceDelete(kind, id, reason)
Service->>DAO: GetForUpdate(id)
Service->>DAO: Delete(resource tree)
Service-->>Handler: result or error
Handler-->>Client: HTTP response
CWE-284 (Improper Access Control): CWE-362 (Race Condition): Recursive delete uses row locking across parent/child traversal; verify all mutations stay inside one transaction boundary. CWE-770 (Uncontrolled Resource Consumption): Recursive tree deletion can traverse unbounded depth/fanout. CWE-1321 (Prototype Pollution of Route Space / Reserved Namespace Collision): Rejecting the reserved plural 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 3 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 1572 lines (>500) | +2 |
| Sensitive paths | none | +0 |
| Test coverage | Missing tests for: pkg/dao pkg/dao/mocks pkg/middleware | +1 |
Computed by hyperfleet-risk-scorer
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/middleware/schema_validation.go`:
- Around line 122-129: PATCH requests to the root resources route are bypassing
schema validation because RootResourceHandler.Patch sends a body without kind,
so resolveRootResourcePlural in schema_validation.go always returns empty and
exits early. Update the validation flow in the middleware to derive the resource
plural from the path/{id} or another lookup tied to the root resource pattern
instead of relying on requestData.kind, so the existing
matchers/rootResourcePattern path still validates PATCH bodies before
next.ServeHTTP runs.
In `@pkg/services/resource.go`:
- Around line 443-449: The Force-deleting resource audit log in resource.go is
currently writing the authenticated caller value directly, which exposes PII.
Update the logging in the resource deletion flow around the logger.With call to
redact or omit caller before it is emitted, using the caller value from
actorFromContext only in a sanitized form. Keep the existing resource_kind,
resource_id, reason, and child_resource_ids fields, but ensure the caller field
is replaced with a redacted value before the Info("Force-deleting resource")
call.
- Around line 413-441: Bound the recursive hard-delete walk in
forceDeleteResourceTree so it cannot traverse arbitrarily deep or wide subtrees
under one transaction. Add a clear depth/fan-out safeguard or recursion limit
before descending through registry.ChildrenOf and
resourceDao.FindByKindAndOwnerForUpdate, and return a ServiceError when the
limit is exceeded. Keep the guard near the recursive call path so the tree walk
fails fast on pathological resource graphs.
🪄 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: e3d57f27-2153-4b3f-aea3-9f2367242b5d
📒 Files selected for processing (12)
pkg/dao/mocks/resource.gopkg/dao/resource.gopkg/handlers/resource_handler.gopkg/handlers/resource_handler_test.gopkg/handlers/root_resource_handler.gopkg/middleware/schema_validation.gopkg/services/resource.gopkg/services/resource_test.goplugins/entities/plugin.gotest/integration/resource_force_delete_test.gotest/integration/resource_helpers.gotest/integration/root_resources_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)
| // For root /resources endpoint, resolve the entity plural from body's kind field | ||
| if resourcePlural == "" { | ||
| resourcePlural = resolveRootResourcePlural(requestData) | ||
| if resourcePlural == "" { | ||
| next.ServeHTTP(w, r) | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
PATCH /resources/{id} silently skips spec schema validation — CWE-20 (Improper Input Validation).
rootResourcePattern matches /resources/{id} for PATCH (line 194-196), requiring resolveRootResourcePlural to determine the plural from the body's kind field. But RootResourceHandler.Patch uses openapi.ResourcePatchRequest, which per root_resource_handler.go only carries Spec/Labels — it never includes kind. So resolveRootResourcePlural returns "" for every PATCH to /resources/{id}, and validation is bypassed unconditionally (lines 125-128) — not just for unresolved/unknown kinds.
This directly contradicts the standard: "HTTP middleware must validate request bodies for every POST/PATCH before service/DB logic runs." It also creates an inconsistency: the same entity's PATCH is validated when hit via the flat child route (/versions/{id}, matched directly by matchers) but not when hit via /resources/{id} — an attacker (or buggy client) can bypass schema validation simply by using the root path.
Resolve the plural from the path {id} (e.g. a lightweight resource lookup) rather than relying on kind being present in the PATCH body, or explicitly document/gate this as an accepted risk with compensating validation at the service layer.
[medium_effort_and_high_reward]
As per path instructions ("hyperfleet/standards/code-review/README.md"): "HTTP middleware must validate request bodies for every POST/PATCH before service/DB logic runs."
Also applies to: 194-212
🤖 Prompt for 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.
In `@pkg/middleware/schema_validation.go` around lines 122 - 129, PATCH requests
to the root resources route are bypassing schema validation because
RootResourceHandler.Patch sends a body without kind, so
resolveRootResourcePlural in schema_validation.go always returns empty and exits
early. Update the validation flow in the middleware to derive the resource
plural from the path/{id} or another lookup tied to the root resource pattern
instead of relying on requestData.kind, so the existing
matchers/rootResourcePattern path still validates PATCH bodies before
next.ServeHTTP runs.
Source: Path instructions
| func (s *sqlResourceService) forceDeleteResourceTree( | ||
| ctx context.Context, resource *api.Resource, caller, reason string, | ||
| ) *errors.ServiceError { | ||
| desc := registry.MustGet(resource.Kind) | ||
| if len(desc.RequiredAdapters) > 0 { | ||
| return errors.GeneralError( | ||
| "force-delete not implemented for resources with required adapters (kind=%s)"+ | ||
| " — adapter_status cleanup needed, see HYPERFLEET-1154", | ||
| resource.Kind, | ||
| ) | ||
| } | ||
|
|
||
| children := registry.ChildrenOf(resource.Kind) | ||
|
|
||
| childIDs := make([]string, 0) | ||
| for _, child := range children { | ||
| items, err := s.resourceDao.FindByKindAndOwnerForUpdate(ctx, child.Kind, resource.ID) | ||
| if err != nil { | ||
| logger.With(ctx, "resource_id", resource.ID, "child_kind", child.Kind). | ||
| WithError(err).Error("Failed to find children for force-delete") | ||
| return errors.GeneralError("Unable to find %s children for force-delete", child.Kind) | ||
| } | ||
| for _, item := range items { | ||
| childIDs = append(childIDs, item.ID) | ||
| if svcErr := s.forceDeleteResourceTree(ctx, item, caller, reason); svcErr != nil { | ||
| return svcErr | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'func (NotFound|Validation|GeneralError|Conflict|ValidationWithDetails|NotImplemented)\(' pkg/errors/errors.goRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 635
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== errors.go excerpts =='
sed -n '300,380p' pkg/errors/errors.go
echo
echo '== resource.go excerpts =='
sed -n '360,470p' pkg/services/resource.go
echo
echo '== forceDelete references =='
rg -n 'forceDeleteResourceTree|force-delete not implemented|required adapters' pkg/services pkg -g'*.go'
echo
echo '== tests around required adapters / force-delete =='
rg -n 'required adapters|force-delete|NotImplemented|GeneralError|HTTPCode' pkg/services pkg -g'*_test.go'Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 24184
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== resource_test force-delete block around 1280-1365 =='
sed -n '1280,1365p' pkg/services/resource_test.go
echo
echo '== resource_test earlier force-delete block around 1210-1265 =='
sed -n '1210,1265p' pkg/services/resource_test.go
echo
echo '== resource_handler tests for force-delete =='
sed -n '640,840p' pkg/handlers/resource_handler_test.go
echo
echo '== cluster/nodepool hard-delete patterns =='
sed -n '410,470p' pkg/services/cluster.go
echo
sed -n '490,540p' pkg/services/node_pool.goRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 14201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== NotImplemented usage in repo =='
rg -n '\bNotImplemented\(' pkg handlers plugins test -g'*.go'
echo
echo '== resource error mapping helpers =='
rg -n 'func handle(Get|List|Delete)?Error|ConflictState|BadRequest|MalformedRequest|NotImplemented' pkg/services pkg/handlers -g'*.go'
echo
echo '== HTTP 500 tests around force-delete and delete state errors =='
sed -n '1200,1365p' pkg/services/resource_test.goRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 797
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== registry parent/child validation =='
rg -n 'ParentKind|ChildrenOf|RequiredAdapters|OnParentDelete|cycle|acyclic|validate.*kind|Register\(registry.EntityDescriptor' pkg plugins -g'*.go'
echo
echo '== resource schema / DAO constraints on owner relationships =='
rg -n 'owner_id|OwnerID|parent_id|ParentID|FOREIGN KEY|cycle|recursive' pkg test -g'*.go' -g'*.sql' -g'*.yaml'
echo
echo '== registry implementation outline =='
fd -a 'registry*.go' pkg pluginsRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 38040
Bound the recursive hard-delete walk. forceDeleteResourceTree walks the entire subtree under one transaction and acquires row locks at every level with no explicit depth/fan-out guard. Large or pathological trees can hold locks long enough to block concurrent writes and create a CWE-400 availability risk.
🤖 Prompt for 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.
In `@pkg/services/resource.go` around lines 413 - 441, Bound the recursive
hard-delete walk in forceDeleteResourceTree so it cannot traverse arbitrarily
deep or wide subtrees under one transaction. Add a clear depth/fan-out safeguard
or recursion limit before descending through registry.ChildrenOf and
resourceDao.FindByKindAndOwnerForUpdate, and return a ServiceError when the
limit is exceeded. Keep the guard near the recursive call path so the tree walk
fails fast on pathological resource graphs.
| logger.With(ctx, | ||
| "resource_kind", resource.Kind, | ||
| "resource_id", resource.ID, | ||
| "caller", caller, | ||
| "reason", reason, | ||
| "child_resource_ids", childIDs, | ||
| ).Info("Force-deleting resource") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -B2 -A8 'func actorFromContext' --type=goRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 173
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## resource.go around force-delete"
grep -n "forceDeleteResourceTree\|Force-deleting resource\|actorFromContext" -n pkg/services/resource.go || true
sed -n '380,470p' pkg/services/resource.go
echo
echo "## actorFromContext search"
rg -n "actorFromContext|DeletedBy|caller" pkg -g'*.go' || true
echo
echo "## read resource tests around required adapters"
rg -n "required adapters|Force-delete|force-delete" test pkg -g'*.go' || trueRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 17488
Redact caller before logging it
actorFromContext returns the authenticated username, and tests already use email-like values such as admin@test.com, so this log line writes PII into the audit trail. CWE-532.
🤖 Prompt for 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.
In `@pkg/services/resource.go` around lines 443 - 449, The Force-deleting resource
audit log in resource.go is currently writing the authenticated caller value
directly, which exposes PII. Update the logging in the resource deletion flow
around the logger.With call to redact or omit caller before it is emitted, using
the caller value from actorFromContext only in a sanitized form. Keep the
existing resource_kind, resource_id, reason, and child_resource_ids fields, but
ensure the caller field is replaced with a redacted value before the
Info("Force-deleting resource") call.
Add cross-kind /resources root endpoints (GET list with ?kind= filter, GET/PATCH/DELETE by ID, POST with child-kind 422 rejection) and flat top-level routes for child entities (GET/PATCH/DELETE + POST 422). Extend schema validation middleware to validate spec on /resources POST by extracting kind from request body. Signed-off-by: Dmitrii Andreev <dandreev@redhat.com>
02ab984 to
b69d8f7
Compare
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/handlers/root_resource_handler.go`:
- Around line 110-145: The Patch action in RootResourceHandler can panic when
registry.MustGet(resource.Kind) is called for a resource whose kind is missing
from the registry. Replace the MustGet lookup with the safe registry.Get path
inside RootResourceHandler.Patch, and if the kind is unknown, return a
ServiceError instead of proceeding. Keep the fix localized around the descriptor
lookup before h.validator.Validate so stale or orphaned resources fail cleanly.
In `@plugins/entities/plugin.go`:
- Around line 26-36: The schema validator initialization in the plugin setup is
swallowing errors from validators.NewSchemaValidator with no operator signal. In
the code that sets schemaValidator before RegisterEntityRoutes, add an explicit
log of the failure (including schemaPath and the returned error) and keep the
fallback only if the degraded behavior is intentional. If you choose to continue
without validation, add a clear comment explaining the intentional degradation;
otherwise, return/stop route registration so misconfiguration is not silent.
In `@test/integration/root_resources_test.go`:
- Around line 212-229: The test name in
TestRootResourcePatchSoftDeletedReturns409 conflicts with the asserted status,
since it expects http.StatusNotFound rather than 409. Update the test to match
the intended contract by either renaming the test to reflect a 404/not-found
outcome or changing the expectation if PATCH on a soft-deleted resource should
באמת return 409. Use the symbols TestRootResourcePatchSoftDeletedReturns409,
rootResourceRequest, and http.StatusNotFound to locate and align the test with
the documented behavior.
🪄 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: 6814a773-99d3-4d44-a74d-be1ac47dd258
📒 Files selected for processing (9)
pkg/dao/mocks/resource.gopkg/dao/resource.gopkg/handlers/root_resource_handler.gopkg/middleware/schema_validation.gopkg/services/resource.gopkg/services/resource_test.goplugins/entities/plugin.goplugins/entities/plugin_test.gotest/integration/root_resources_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)
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/dao/mocks/resource.go
- pkg/dao/resource.go
- pkg/middleware/schema_validation.go
- pkg/services/resource_test.go
- pkg/services/resource.go
| func (h *RootResourceHandler) Patch(w http.ResponseWriter, r *http.Request) { | ||
| var req openapi.ResourcePatchRequest | ||
| cfg := &handlerConfig{ | ||
| MarshalInto: &req, | ||
| StrictUnmarshal: true, | ||
| Validate: []validate{ | ||
| validatePatchRequest(&req), | ||
| validateLabels(&req, "Labels"), | ||
| }, | ||
| Action: func() (interface{}, *errors.ServiceError) { | ||
| id := mux.Vars(r)["id"] | ||
| resource, err := h.service.GetByID(r.Context(), id) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if req.Spec != nil && h.validator != nil { | ||
| descriptor := registry.MustGet(resource.Kind) | ||
| if validationErr := h.validator.Validate(descriptor.Plural, *req.Spec); validationErr != nil { | ||
| if svcErr, ok := validationErr.(*errors.ServiceError); ok { | ||
| return nil, svcErr | ||
| } | ||
| return nil, errors.Validation("Spec validation failed: %v", validationErr) | ||
| } | ||
| } | ||
|
|
||
| patch := convertResourcePatch(&req) | ||
| resource, err = h.service.Patch(r.Context(), resource.Kind, id, patch) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return presenters.PresentResource(resource), nil | ||
| }, | ||
| } | ||
| handle(w, r, cfg, http.StatusOK) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## root_resource_handler.go\n'
ast-grep outline pkg/handlers/root_resource_handler.go --view expanded || true
printf '\n## relevant registry symbols\n'
rg -n "func (.*MustGet|func (.*Get\\(|type Descriptor|MustGet\\(|registry.Get\\(" pkg plugins -g'*.go' || true
printf '\n## root resource handler snippets\n'
cat -n pkg/handlers/root_resource_handler.go | sed -n '1,240p'Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 8317
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## registry implementation files\n'
fd -a 'registry*.go' . || true
rg -n "MustGet|func Get\\(|type Descriptor|LoadDescriptors|RegisterKind|Plural" pkg plugins -g'*.go' || trueRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 13288
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## root_resource_handler.go around Patch/Create\n'
cat -n pkg/handlers/root_resource_handler.go | sed -n '1,220p'
printf '\n## registry APIs\n'
cat -n pkg/registry/*.go | sed -n '1,260p' 2>/dev/null || true
printf '\n## any tests or docs mentioning PATCH /resources/{id}\n'
rg -n "PATCH /api/.*/resources|PATCH .*/resources/\\{id\\}|schema validation|MustGet\\(" . -g'*.go' -g'*.md' -g'*.yaml' || trueRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 19848
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## startup registry validation and route setup\n'
rg -n "registry.Validate\\(|ValidateSpecSchemas\\(|LoadDescriptors\\(|MustGet\\(" cmd pkg plugins -g'*.go' || true
cat -n cmd/hyperfleet-api/server/routes.go | sed -n '1,220p'
cat -n cmd/hyperfleet-api/servecmd/cmd.go | sed -n '1,180p'
printf '\n## schema validation middleware\n'
cat -n pkg/middleware/schema_validation.go | sed -n '1,260p'
printf '\n## resource service kind handling\n'
cat -n pkg/services/resource.go | sed -n '1,260p'Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 35012
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## tests/docs around generic PATCH validation\n'
rg -n "PATCH.*resources|spec validation|Validate\\(resourcePlural|h.validator.Validate|SchemaValidationMiddleware" test pkg docs plugins -g'*.go' -g'*.md' -g'*.yaml' || true
printf '\n## route registrations for root resources\n'
rg -n "RootResourceHandler|ForceDelete|handleForceDelete|handleSoftDelete|HandleFunc\\(\"/resources" cmd pkg plugins -g'*.go' || true
printf '\n## config-driven entities that can be removed/reconfigured\n'
rg -n "entities:|LoadDescriptors|Reset\\(|registry.Reset\\(" . -g'*.go' -g'*.md' -g'*.yaml' || trueRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 23502
🏁 Script executed (no clone):
# no-op
Length of output: 125
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## ResourcePatchRequest shape and PATCH tests\n'
rg -n "type ResourcePatchRequest|ResourcePatchRequest struct|kind.*patch|PatchRequest" pkg/api/openapi pkg/middleware pkg/handlers -g'*.go' || true
cat -n pkg/api/openapi/*.go 2>/dev/null | rg -n "type ResourcePatchRequest|ResourcePatchRequest struct|Kind|Spec|Labels" -n || true
printf '\n## root PATCH handler tests\n'
rg -n "Test.*Patch|Patch\\(" pkg/handlers/root_resource_handler_test.go pkg/handlers/resource_handler_test.go -g'*.go' || true
cat -n pkg/handlers/resource_handler_test.go | sed -n '1,260p'Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 12873
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## ResourcePatchRequest definition\n'
rg -n "type ResourcePatchRequest struct" pkg/api/openapi -g'*.go' || true
printf '\n## resource patch handler tests focused snippets\n'
rg -n "Patch" pkg/handlers/resource_handler_test.go pkg/handlers/root_resource_handler_test.go -g'*.go' || trueRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 1408
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=$(rg -l "type ResourcePatchRequest struct" pkg/api/openapi -g'*.go' | head -n 1)
echo "FILE=$file"
if [ -n "${file:-}" ]; then
line=$(rg -n "type ResourcePatchRequest struct" "$file" | cut -d: -f1 | head -n 1)
start=$((line-20))
[ "$start" -lt 1 ] && start=1
end=$((line+80))
cat -n "$file" | sed -n "${start},${end}p"
fiRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 229
Guard registry.MustGet in Patch. pkg/handlers/root_resource_handler.go:127 can panic on a row whose Kind no longer exists in the registry/config (CWE-248). Use registry.Get and return a service error instead so PATCH on stale/orphaned resources fails cleanly.
🤖 Prompt for 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.
In `@pkg/handlers/root_resource_handler.go` around lines 110 - 145, The Patch
action in RootResourceHandler can panic when registry.MustGet(resource.Kind) is
called for a resource whose kind is missing from the registry. Replace the
MustGet lookup with the safe registry.Get path inside RootResourceHandler.Patch,
and if the kind is unknown, return a ServiceError instead of proceeding. Keep
the fix localized around the descriptor lookup before h.validator.Validate so
stale or orphaned resources fail cleanly.
| schemaPath := environments.Environment().Config.Server.OpenAPISchemaPath | ||
| var schemaValidator *validators.SchemaValidator | ||
| if schemaPath != "" { | ||
| var err error | ||
| schemaValidator, err = validators.NewSchemaValidator(schemaPath) | ||
| if err != nil { | ||
| schemaValidator = nil | ||
| } | ||
| } | ||
|
|
||
| RegisterEntityRoutes(apiV1Router, resourceService, schemaValidator) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Schema validator load failure silently discarded.
validators.NewSchemaValidator(schemaPath) error is swallowed with no log and no comment explaining the degradation (CWE-390: Detection of Error Condition Without Action). If schemaPath is misconfigured, /resources POST/PATCH spec validation silently disables server-wide with zero operator signal — data can enter unvalidated. As per path instructions, "Log-and-continue MUST be intentional degradation with a comment, not a missing return."
🛡️ Proposed fix
if schemaPath != "" {
var err error
schemaValidator, err = validators.NewSchemaValidator(schemaPath)
if err != nil {
+ logger.With(context.Background(), "schema_path", schemaPath).
+ Error(fmt.Sprintf("failed to load OpenAPI schema validator, spec validation disabled: %v", err))
schemaValidator = nil
}
}🤖 Prompt for 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.
In `@plugins/entities/plugin.go` around lines 26 - 36, The schema validator
initialization in the plugin setup is swallowing errors from
validators.NewSchemaValidator with no operator signal. In the code that sets
schemaValidator before RegisterEntityRoutes, add an explicit log of the failure
(including schemaPath and the returned error) and keep the fallback only if the
degraded behavior is intentional. If you choose to continue without validation,
add a clear comment explaining the intentional degradation; otherwise,
return/stop route registration so misconfiguration is not silent.
Source: Path instructions
| func TestRootResourcePatchSoftDeletedReturns409(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| svc, h := setupResourceTest(t) | ||
|
|
||
| account := h.NewRandAccount() | ||
| ctx := h.NewAuthenticatedContext(account) | ||
|
|
||
| channel := createChannel(t, svc, fmt.Sprintf("del409-ch-%s", uuid.NewString()[:8])) | ||
| _, delErr := svc.Delete(t.Context(), "Channel", channel.ID) | ||
| Expect(delErr).To(BeNil()) | ||
|
|
||
| patchBody := `{"spec": {"is_default": true, "enabled_regex": ".*"}}` | ||
| resp, err := rootResourceRequest(ctx). | ||
| SetBody(patchBody). | ||
| Patch(h.RestURL(fmt.Sprintf("%s/%s", resourcesPath, channel.ID))) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(resp.StatusCode()).To(Equal(http.StatusNotFound)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test name contradicts its own assertion.
TestRootResourcePatchSoftDeletedReturns409 asserts http.StatusNotFound (Line 228), not 409. Either the name is stale or the expected contract for PATCH-on-soft-deleted-resource is wrong. This ambiguity matters — force-deletion-design.md requires adapters to handle 404 gracefully post-deletion, so downstream consumers need the correct documented status for this path.
♻️ Proposed fix (rename to match asserted behavior)
-func TestRootResourcePatchSoftDeletedReturns409(t *testing.T) {
+func TestRootResourcePatchSoftDeletedReturns404(t *testing.T) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestRootResourcePatchSoftDeletedReturns409(t *testing.T) { | |
| RegisterTestingT(t) | |
| svc, h := setupResourceTest(t) | |
| account := h.NewRandAccount() | |
| ctx := h.NewAuthenticatedContext(account) | |
| channel := createChannel(t, svc, fmt.Sprintf("del409-ch-%s", uuid.NewString()[:8])) | |
| _, delErr := svc.Delete(t.Context(), "Channel", channel.ID) | |
| Expect(delErr).To(BeNil()) | |
| patchBody := `{"spec": {"is_default": true, "enabled_regex": ".*"}}` | |
| resp, err := rootResourceRequest(ctx). | |
| SetBody(patchBody). | |
| Patch(h.RestURL(fmt.Sprintf("%s/%s", resourcesPath, channel.ID))) | |
| Expect(err).NotTo(HaveOccurred()) | |
| Expect(resp.StatusCode()).To(Equal(http.StatusNotFound)) | |
| } | |
| func TestRootResourcePatchSoftDeletedReturns404(t *testing.T) { | |
| RegisterTestingT(t) | |
| svc, h := setupResourceTest(t) | |
| account := h.NewRandAccount() | |
| ctx := h.NewAuthenticatedContext(account) | |
| channel := createChannel(t, svc, fmt.Sprintf("del409-ch-%s", uuid.NewString()[:8])) | |
| _, delErr := svc.Delete(t.Context(), "Channel", channel.ID) | |
| Expect(delErr).To(BeNil()) | |
| patchBody := `{"spec": {"is_default": true, "enabled_regex": ".*"}}` | |
| resp, err := rootResourceRequest(ctx). | |
| SetBody(patchBody). | |
| Patch(h.RestURL(fmt.Sprintf("%s/%s", resourcesPath, channel.ID))) | |
| Expect(err).NotTo(HaveOccurred()) | |
| Expect(resp.StatusCode()).To(Equal(http.StatusNotFound)) | |
| } |
🤖 Prompt for 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.
In `@test/integration/root_resources_test.go` around lines 212 - 229, The test
name in TestRootResourcePatchSoftDeletedReturns409 conflicts with the asserted
status, since it expects http.StatusNotFound rather than 409. Update the test to
match the intended contract by either renaming the test to reflect a
404/not-found outcome or changing the expectation if PATCH on a soft-deleted
resource should באמת return 409. Use the symbols
TestRootResourcePatchSoftDeletedReturns409, rootResourceRequest, and
http.StatusNotFound to locate and align the test with the documented behavior.
Source: Path instructions
Summary
/resourcesroot endpoints:GET(list with?kind=filter),GET/PATCH/DELETEby ID,POST(top-level only, child kinds return 422),POST force-deleteGET/PATCH/DELETE /{childPlural}+POSTreturns 422)/resourcesPOST by extractingkindfrom request bodyPlural: "resources"from shadowing root endpointDepends on: PR #266 (HYPERFLEET-1155 force-delete) — rebased on top of it
AC Coverage
TestRootResourceListTestRootResourceGetByIDTestRootResourcePatchTestRootResourceDeleteTestRootResourceCreateChildKindReturns422TestRootResourceCreateTestFlatChildRouteListTestFlatChildRouteGetByIDTestFlatChildRoutePatchTestFlatChildRouteDeleteTestFlatChildRoutePostReturns422TODO: HYPERFLEET-1154in codeGate results
build ✓ · lint ✓ · tests 1336 pass / 0 fail · verify ✓ · integration 11 new tests pass
Design decisions
RootResourceHandler— separate fromResourceHandler(cross-kind, resolves kind per-request)GetByIDDAO/service — kind-agnostic lookup using PK index (UUID v7 globally unique per ADR-0009)ListAllservice — separate fromList(no overloaded kind parameter)rootResourcePattern+resolveRootResourcePluralto extract kind from body; PATCH via/resources/{id}unvalidated (use entity-specific path for validated PATCH)descriptor.Kindfrom registry, not raw user inputKnown limitations
PATCH /resources/{id}skips spec schema validation (body has nokindfield for middleware to resolve)