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 |
📝 WalkthroughWalkthroughResource 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 child kind without parent_id
ResourceHandler->>ResourceHandler: extract parentID, hasParent
ResourceHandler->>RootResourceHandler: reject child-kind root create
RootResourceHandler->>Registry: MustGet(parent kind)
Registry-->>RootResourceHandler: parent descriptor
RootResourceHandler-->>Client: errors.Validation with nested path guidance
🚥 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 | 167 lines | +0 |
| Sensitive paths | none | +0 |
| Test coverage | Tests cover changed packages | +0 |
Computed by hyperfleet-risk-scorer
kuudori
force-pushed
the
HYPERFLEET-1328-refactor-resource-handler-validation
branch
from
1cba700 to
2fe4a11
Compare
| if _, svcErr := h.service.Get(ctx, h.descriptor.ParentKind, parentID); svcErr != nil { | ||
| return nil, svcErr | ||
| } | ||
| if hasParent { |
There was a problem hiding this comment.
Warning
Blocking
Category: Bug
Removing the parent-existence pre-check changes observable API behavior for List:
| Scenario | Before | After |
|---|---|---|
GET /channels/{bad-id}/versions |
404 "Channel not found" | 200 with empty list |
GET /channels/{bad-id}/versions/{id} |
404 "Channel not found" | 404 "Version not found" |
ListByOwner just filters by owner_id in SQL — if the parent does not exist, no children match, so it returns an empty list (200). The old pre-check caught this case and returned 404.
If this is intentional, it should be documented in the PR description and the removed "Parent not found" test should be replaced with a test asserting the new behavior (200 empty list). If not, the parent-existence check should be kept for List and Get.
There was a problem hiding this comment.
good catch! updated PR description with rationale.
empty list is just a consistent answer, and showing that a parent wasn't found feels a little strange to me. if the user doesn't see the full path, he might think he's working with the parent when he sees that it wasn't found (maybe reads it as an id problem or wrong entity)
| if rootResourcePattern.MatchString(path) { | ||
| // Root /resources POST carries kind in body — resolve plural from body later. | ||
| // Root /resources PATCH has no kind; handler validates spec internally. | ||
| if method == http.MethodPost && rootResourcePattern.MatchString(path) { |
There was a problem hiding this comment.
Warning
Blocking
Category: JIRA
HYPERFLEET-1328 acceptance criteria #1, #2, and #3 are written for approach #1 (middleware DB lookup) but the PR implements approach #2 (exclude PATCH from middleware). Under approach #2:
- AC#1 ("PATCH rejected before any DB access"): validation still happens after
GetByIDin the handler - AC#2 ("consistent validation between root PATCH and flat child PATCH"): flat routes still validate in middleware, root PATCH does not
- AC#3 ("integration test for PATCH validation before GetByID"): no PATCH-specific integration test was added
The approach is valid per the ticket's technical notes, but the ACs should be updated to match approach #2 — or a follow-up ticket should capture the remaining PATCH validation gap. The ticket also notes approach #2 should "document this as the accepted layering for the root PATCH case."
| if _, err = h.service.Get(ctx, h.descriptor.ParentKind, parentID); err != nil { | ||
| return nil, err | ||
| } | ||
| if hasParent { |
There was a problem hiding this comment.
Tip
nit — non-blocking suggestion
Category: Bug
With the parent pre-check removed, GET /channels/{bad-id}/versions/{v-id} now returns 404 "Version not found" instead of 404 "Channel not found". The status code is correct either way, but the error message may confuse callers debugging ownership — they get a child-not-found error when the actual problem is a non-existent parent.
There was a problem hiding this comment.
covered above, "version not found" is correct for the resource being requested 🙂
…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
2fe4a11 to
6b5632f
Compare
|
/retest |
2 participants
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 runtimeBehavior change: parent-existence pre-check removed from Get/List
GET /channels/{bad-id}/versions/{v-id}GET /channels/{bad-id}/versionsTest plan
TestValidate_ParentKindCycle_Panics)go vetclean across all modified packages