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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughResource handlers now centralize parent and ownership checks for flat and nested routes. Child creation without parent context returns a validation error with nested-route guidance and HTTP 400. Root resource creation resolves parent and child paths from registry descriptors. Middleware tests cover root resource matching, and registry validation detects Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant RootResourceHandler
participant Registry
Client->>RootResourceHandler: POST child kind at root route
RootResourceHandler->>Registry: Resolve parent descriptor
Registry-->>RootResourceHandler: Return parent plural
RootResourceHandler-->>Client: Return HTTP 400 validation guidance
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 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: 1 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 233 lines (>200) | +1 |
| 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)
There was a problem hiding this comment.
IMO is quite misleading to have a 200 response
- In our case
versionsare entities on their own - but
versionscould be just a property of the parent resource - a 200 response for me makes the assumption of something is addressable under that
{bad-id}, so it means that exist - If we don't want to provide info about the parent, we could answer with
400 Bad Request
There was a problem hiding this comment.
I agree, from the other side of the API, you cannot differentiate if the parent resource exists or not when you receive 200. IMO there is a difference between an empty list and a list that does not exist.
| 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 🙂
kuudori
force-pushed
the
HYPERFLEET-1328-refactor-resource-handler-validation
branch
from
2fe4a11 to
6b5632f
Compare
|
/retest |
kuudori
force-pushed
the
HYPERFLEET-1328-refactor-resource-handler-validation
branch
from
6b5632f to
c75274e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/handlers/resource_handler_test.go (1)
845-847: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the actionable nested-route error detail.
This test passes even if the handler returns the wrong or empty message. Verify the problem-detail body includes guidance such as
/channels/{id}/versions, since that message is part of the behavior being changed.🧪 Proposed assertion
Expect(rr.Code).To(Equal(http.StatusBadRequest)) + Expect(rr.Body.String()).To(ContainSubstring(`/channels/{id}/versions`))🤖 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/resource_handler_test.go` around lines 845 - 847, The test around handler.Create must validate the response body, not only the HTTP status. After asserting http.StatusBadRequest, inspect the problem-detail response and verify it contains actionable nested-route guidance such as “/channels/{id}/versions”.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_test.go`:
- Around line 831-833: Restore the process-global registry after the test: in
the test containing the registry.Reset and registry.Register calls, capture the
existing registry state before mutation and register a cleanup handler to
restore it afterward; if snapshot/restore is unavailable, use test cleanup to
reset and re-register the original descriptors, and ensure the test does not run
in parallel.
---
Nitpick comments:
In `@pkg/handlers/resource_handler_test.go`:
- Around line 845-847: The test around handler.Create must validate the response
body, not only the HTTP status. After asserting http.StatusBadRequest, inspect
the problem-detail response and verify it contains actionable nested-route
guidance such as “/channels/{id}/versions”.
🪄 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: 0ac45817-4e09-416f-8eda-3e0c96c4dc38
📒 Files selected for processing (8)
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.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 due to trivial changes (1)
- pkg/middleware/schema_validation.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/handlers/root_resource_handler.go
- pkg/registry/registry_test.go
- pkg/registry/registry.go
- test/integration/root_resources_test.go
- pkg/middleware/schema_validation_match_test.go
- pkg/handlers/resource_handler.go
…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
c75274e to
7ea3eb3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 84-90: Remove the checkParentExists call from the Get and List
handlers. Branch directly on whether parent_id is present, then invoke
GetByOwner or ListByOwner so those methods determine the missing-parent
behavior, preserving an empty collection for List and the appropriate not-found
result for Get.
🪄 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: e614aa53-3aa3-4b42-8d42-911a956651e9
📒 Files selected for processing (8)
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.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 due to trivial changes (1)
- pkg/middleware/schema_validation.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/handlers/root_resource_handler.go
- pkg/middleware/schema_validation_match_test.go
- pkg/registry/registry.go
- pkg/handlers/resource_handler_test.go
- pkg/registry/registry_test.go
- test/integration/root_resources_test.go
| parentID, err := h.checkParentExists(r) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| var resource *api.Resource | ||
| var err *errors.ServiceError | ||
| if parentID, hasParent := vars["parent_id"]; hasParent { | ||
| if _, err = h.service.Get(ctx, h.descriptor.ParentKind, parentID); err != nil { | ||
| return nil, err | ||
| } | ||
| if parentID != "" { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove parent pre-checks from Get and List.
These calls restore the old behavior: invalid-parent Get reports the parent missing, while List returns 404 instead of the required empty collection. Branch directly on parent_id presence and let GetByOwner/ListByOwner determine the result (CWE-436).
Proposed fix
- parentID, err := h.checkParentExists(r)
- if err != nil {
- return nil, err
- }
+ parentID, hasParent := mux.Vars(r)["parent_id"]
var resource *api.Resource
- if parentID != "" {
+ var err *errors.ServiceError
+ if hasParent {
resource, err = h.service.GetByOwner(ctx, h.descriptor.Kind, id, parentID)
} else {
resource, err = h.service.Get(ctx, h.descriptor.Kind, id)
}- parentID, err := h.checkParentExists(r)
- if err != nil {
- return nil, err
- }
+ parentID, hasParent := mux.Vars(r)["parent_id"]
listArgs, err := services.NewListArguments(r.URL.Query())
if err != nil {
return nil, err
}
var resources api.ResourceList
var paging *api.PagingMeta
- if parentID != "" {
+ if hasParent {
resources, paging, err = h.service.ListByOwner(ctx, h.descriptor.Kind, parentID, listArgs)
} else {Also applies to: 110-122
🤖 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/resource_handler.go` around lines 84 - 90, Remove the
checkParentExists call from the Get and List handlers. Branch directly on
whether parent_id is present, then invoke GetByOwner or ListByOwner so those
methods determine the missing-parent behavior, preserving an empty collection
for List and the appropriate not-found result for Get.
| } | ||
| resource, err = presenters.ConvertResourceWithOwner(&req, parent.ID, parent.Kind, parent.Href) | ||
| } else if h.descriptor.ParentKind != "" { | ||
| return nil, childCreateRejection(h.descriptor) |
There was a problem hiding this comment.
This method was explicitly setting the resonse error code to 422 (as mentioned in the design generic-resource-registry-design.md), which is now missing. Can you please confirm and restore the explicit HTTPCode?
| 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.
Tip
nit — non-blocking suggestion
Category: JIRA
The unit tests here (TestShouldValidateRequest_PatchRootResourceSkipsMiddleware, TestShouldValidateRequest_PostRootResourceMatchesForBodyResolution) confirm routing, but AC#1 ("same 400 format")
and AC#3 ("no regression on POST /resources validation") are really end-to-end claims. Neither is exercised by an integration test — test/integration/root_resources_test.go's only PATCH cases are
the happy path and soft-deleted-404, and the only POST case is a valid spec.
Worth adding two integration tests before this ships:
POST /resourceswith an invalid spec → assert 400 (locks in AC#3)PATCH /resources/{id}with an invalid spec → assert the same problem-details shape asPATCH /{plural}/{id}(locks in AC#1)
Not blocking since the unit tests technically satisfy the AC text, but it's a real coverage gap for the behavior the ticket cares about.
| }).To(PanicWith(ContainSubstring("unregistered parent kind"))) | ||
| } | ||
|
|
||
| func TestValidate_ParentKindCycle_Panics(t *testing.T) { |
There was a problem hiding this comment.
Should we also add a 3+ node cycle test to here? Wdyt?
func TestValidate_ParentKindCycle_ThreeNode_Panics(t *testing.T) {
RegisterTestingT(t)
Reset()
Register(EntityDescriptor{Kind: "A", Plural: "as", ParentKind: "B"})
Register(EntityDescriptor{Kind: "B", Plural: "bs", ParentKind: "C"})
Register(EntityDescriptor{Kind: "C", Plural: "cs", ParentKind: "A"})
Expect(func() {
Validate()
}).To(PanicWith(ContainSubstring("ownership cycle detected")))
}| if parentID, hasParent := mux.Vars(r)["parent_id"]; hasParent { | ||
| // checkParentExists returns the parent_id if the parent exists, "" for flat | ||
| // routes, or a 404 if parent_id is present but the parent is missing. | ||
| func (h *ResourceHandler) checkParentExists(r *http.Request) (string, *errors.ServiceError) { |
There was a problem hiding this comment.
nit: the checkX pattern here usually returns only error convention. Thinking out loud, how does this sound?:
| func (h *ResourceHandler) checkParentExists(r *http.Request) (string, *errors.ServiceError) { | |
| func (h *ResourceHandler) parentIDIfExists(r *http.Request) (string, *errors.ServiceError) { |
5 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