HYPERFLEET-1328 - refactor: simplify resource handlers and add registry cycle detection#286
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 (8)
🔗 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 (6)
📝 WalkthroughSummary by CodeRabbit
WalkthroughResource handlers now branch on Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ResourceHandler
participant RootResourceHandler
participant Registry
Client->>ResourceHandler: POST /versions without parent_id
ResourceHandler->>ResourceHandler: extract parentID, hasParent=false
ResourceHandler->>RootResourceHandler: create child-kind request
RootResourceHandler->>Registry: MustGet(parent kind)
Registry-->>RootResourceHandler: parent descriptor
RootResourceHandler-->>Client: errors.Validation with parent route guidance
Related issues: None identified from provided data. Related PRs: None identified from provided data. Suggested labels: security, validation, routing Suggested reviewers: None identified from provided data. Compact metadata: repo CWE-863, CWE-284. 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/registry/registry.go (1)
109-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMerge duplicate
ParentKind != ""guards and extract cycle check.Two separate
if d.ParentKind != ""blocks run back-to-back (Lines 109 and 118). Combine them and extract the traversal into a helper (e.g.checkParentKindCycle(d, descriptors)) —Validate()is now ~66 lines with well over 5 independent panic branches, exceeding the decomposition threshold.♻️ Proposed refactor
- if d.ParentKind != "" { - if _, ok := descriptors[d.ParentKind]; !ok { - panic(fmt.Sprintf( - "entity kind %q references unregistered parent kind %q", - d.Kind, d.ParentKind, - )) - } - } - - if d.ParentKind != "" { - visited := map[string]bool{d.Kind: true} - for cur := d.ParentKind; cur != ""; { - if visited[cur] { - panic(fmt.Sprintf( - "ownership cycle detected: kind %q participates in a ParentKind cycle", - d.Kind, - )) - } - visited[cur] = true - cur = descriptors[cur].ParentKind - } - } + if d.ParentKind != "" { + if _, ok := descriptors[d.ParentKind]; !ok { + panic(fmt.Sprintf( + "entity kind %q references unregistered parent kind %q", + d.Kind, d.ParentKind, + )) + } + checkParentKindCycle(d, descriptors) + }As per path instructions, "Functions >50 lines or >5 branching paths — flag for decomposition."
🤖 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/registry/registry.go` around lines 109 - 131, In Validate, merge the duplicated ParentKind presence checks into a single guard and move the ancestry traversal into a helper such as checkParentKindCycle. Use the existing descriptors map and d.Kind/d.ParentKind in the helper to preserve the same panic behavior, but keep Validate focused by delegating the cycle detection logic instead of having two back-to-back branches.Source: Path instructions
🤖 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/resource_handler.go`:
- Around line 45-63: The child-kind create rejection is currently using
errors.Validation(), which maps to 400 instead of the expected 422 response.
Update the shared child-create validation logic used by ResourceHandler and
RootResourceHandler so both the flat and nested create paths return
Unprocessable Entity for invalid child-kind creates, preserving the API contract
expected by the root_resources integration tests. Locate the duplicated
rejection code in ResourceHandler and RootResourceHandler and route both through
the same helper that produces the 422 response.
In `@pkg/handlers/root_resource_handler.go`:
- Around line 92-97: The child-kind root create path in rootResourceHandler
should return the dedicated 422 contract instead of the generic validation
status. In the branch that checks descriptor.ParentKind and builds the “create
it via /{id}/...” message, either set the HTTPCode explicitly to 422 on the
returned error or switch to the existing child-create error helper so POST
/api/hyperfleet/v1/resources preserves the child-create behavior.
---
Nitpick comments:
In `@pkg/registry/registry.go`:
- Around line 109-131: In Validate, merge the duplicated ParentKind presence
checks into a single guard and move the ancestry traversal into a helper such as
checkParentKindCycle. Use the existing descriptors map and d.Kind/d.ParentKind
in the helper to preserve the same panic behavior, but keep Validate focused by
delegating the cycle detection logic instead of having two back-to-back
branches.
🪄 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: 992f9348-4315-439a-9194-d319079ce1e2
📒 Files selected for processing (7)
pkg/handlers/resource_handler.gopkg/handlers/resource_handler_test.gopkg/handlers/root_resource_handler.gopkg/middleware/schema_validation.gopkg/middleware/schema_validation_match_test.gopkg/registry/registry.gopkg/registry/registry_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)
Risk Score: 0 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 170 lines | +0 |
| Sensitive paths | none | +0 |
| Test coverage | Tests cover changed packages | +0 |
Computed by hyperfleet-risk-scorer
…ry cycle detection Consolidate parent-kind checks in resource handlers with actionable error messages, add ParentKind cycle detection to registry startup validation, and harden schema validation path matching.
kuudori
force-pushed
the
HYPERFLEET-1328-refactor-resource-handler-validation
branch
from
1cba700 to
2fe4a11
Compare
1 participant
Summary
ResourceHandlerandRootResourceHandlerwith actionable error messages that point to the exact nested route (e.g.create it via /channels/{id}/versions)ParentKindcycle detection toregistry.Validate()so misconfigured ownership chains are caught at startup instead of causing infinite recursion at runtimeTest plan
TestValidate_ParentKindCycle_Panics)go vetclean across all modified packages