HYPERFLEET-1369 - refactor: Flatten handlerConfig - #316
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:
📝 WalkthroughWalkthroughHandlers replace Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant Helpers
participant Service
Client->>Handler: send request
Handler->>Helpers: decode and validate
Helpers-->>Handler: validated request or error
Handler->>Service: execute operation
Service-->>Handler: result or error
Handler-->>Client: write HTTP response
Suggested reviewers: 🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 2 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 2002 lines (>500) | +2 |
| Sensitive paths | none | +0 |
| Test coverage | Tests cover changed packages | +0 |
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
🧹 Nitpick comments (1)
pkg/handlers/framework.go (1)
22-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMagic string
"strict"used as switch discriminator with silent default fallback.Any typo'd mode value (
"Strict","STRICT", etc.) silently falls through todefault(non-strict) decoding — no compile-time or runtime signal. Since strict mode is what rejects unknown/immutable fields on PATCH/PUT, a future typo at a call site quietly weakens input validation instead of failing loudly. Define a package constant instead of relying on the literal.As per path instructions, "Magic numbers/strings SHOULD be named constants" (QUAL-02).
♻️ Proposed fix
+const StrictMode = "strict" + func decodeAndValidate(r *http.Request, req any, validateFuncs []validate, decodeType ...string) *errors.ServiceError { ... switch mode { - case "strict": + case StrictMode:And update call sites (
decodeAndValidate(r, &req, validateFuncs, "strict")) to usehandlers.StrictMode.🤖 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/framework.go` around lines 22 - 50, Define an exported package constant such as StrictMode for the strict decoding discriminator, and replace the literal "strict" in decodeAndValidate and all call sites with that constant. Preserve the existing strict behavior while ensuring callers use the shared symbol instead of a magic string.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 `@AGENTS.md`:
- Around line 87-88: Update the framework.go entry in AGENTS.md to describe the
current decodeAndValidate-based handling flow instead of the removed handle,
handleGet, handleList, and handleDelete functions. Keep the documentation
aligned with the symbols that actually exist in framework.go.
In `@pkg/handlers/root_resource_handler.go`:
- Around line 202-214: Add an immediate return after handleError in
RootResourceHandler.Delete when GetByID returns svcErr, preventing access to
resource.Kind after a failed lookup. Preserve the existing delete flow for
successful lookups and the current error handling for Delete failures.
- Around line 61-71: Update the List handler flow around presenters.SliceFilter
so it returns immediately after successfully writing the filtered response when
listArgs.Fields is set. Keep the existing error handling and unfiltered write
for requests without fields, preventing both responses from being written.
---
Nitpick comments:
In `@pkg/handlers/framework.go`:
- Around line 22-50: Define an exported package constant such as StrictMode for
the strict decoding discriminator, and replace the literal "strict" in
decodeAndValidate and all call sites with that constant. Preserve the existing
strict behavior while ensuring callers use the shared symbol instead of a magic
string.
🪄 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: b8f18c51-fa59-4a74-a75b-ea8e733fc96d
📒 Files selected for processing (7)
AGENTS.mdCLAUDE.mdpkg/handlers/CLAUDE.mdpkg/handlers/framework.gopkg/handlers/resource_handler.gopkg/handlers/resource_status_handler.gopkg/handlers/root_resource_handler.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 with no reviewable changes (1)
- CLAUDE.md
There was a problem hiding this comment.
♻️ Duplicate comments (2)
pkg/handlers/root_resource_handler.go (2)
202-214: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winNil dereference:
handleErrorwithoutreturnonGetByIDfailure (CWE-476).On a failed lookup
resourceis nil and line 208 readsresource.Kind, panicking the handler goroutine instead of returning the error response. Violates ERR-03 (error response must be followed byreturn).🐛 Proposed fix
if svcErr != nil { handleError(r, w, svcErr) + return }🤖 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 202 - 214, Update RootResourceHandler.Delete so the GetByID error branch calls handleError and immediately returns before accessing resource.Kind or invoking Delete; preserve the existing Delete error handling and successful response flow.Source: Path instructions
61-71: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winMissing
returnafter the filtered write: double-write plus unfiltered payload on the wire (CWE-200).Line 68 writes the field-filtered list, then execution falls through to line 70 and writes the full
result. Two concatenated JSON bodies, and?fields=withholds nothing.🐛 Proposed fix
writeJSONResponse(w, r, http.StatusOK, filtered) + return } writeJSONResponse(w, r, http.StatusOK, result)🤖 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 61 - 71, Update the filtered-response branch in the resource handler to return immediately after writing the successful filtered payload. Keep the existing error return and unfiltered response path unchanged so only one JSON response is written.
🤖 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.
Duplicate comments:
In `@pkg/handlers/root_resource_handler.go`:
- Around line 202-214: Update RootResourceHandler.Delete so the GetByID error
branch calls handleError and immediately returns before accessing resource.Kind
or invoking Delete; preserve the existing Delete error handling and successful
response flow.
- Around line 61-71: Update the filtered-response branch in the resource handler
to return immediately after writing the successful filtered payload. Keep the
existing error return and unfiltered response path unchanged so only one JSON
response is written.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: aa1fb51f-33e6-417a-bb90-2bb86c8f18b9
📒 Files selected for processing (7)
AGENTS.mdCLAUDE.mdpkg/handlers/CLAUDE.mdpkg/handlers/framework.gopkg/handlers/resource_handler.gopkg/handlers/resource_status_handler.gopkg/handlers/root_resource_handler.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 (6)
- CLAUDE.md
- pkg/handlers/resource_status_handler.go
- pkg/handlers/CLAUDE.md
- AGENTS.md
- pkg/handlers/resource_handler.go
- pkg/handlers/framework.go
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/root_resource_handler.go`:
- Around line 253-258: Update the response construction in
FindByResourcePaginated’s caller so the total count remains int64 end-to-end and
cannot be silently truncated by int32(total). Prefer changing the OpenAPI
response contract and related Page/Size/Total fields to int64; otherwise
validate total against an explicit supported maximum and handle overflow before
assigning it.
🪄 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: e952cfc9-e6b3-4180-9702-05a48ba607a6
📒 Files selected for processing (7)
AGENTS.mdCLAUDE.mdpkg/handlers/CLAUDE.mdpkg/handlers/framework.gopkg/handlers/resource_handler.gopkg/handlers/resource_status_handler.gopkg/handlers/root_resource_handler.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 (6)
- pkg/handlers/resource_status_handler.go
- CLAUDE.md
- AGENTS.md
- pkg/handlers/CLAUDE.md
- pkg/handlers/resource_handler.go
- pkg/handlers/framework.go
|
|
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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/CLAUDE.md`:
- Around line 3-13: Update the “Request Processing” pipeline documentation to
require JWT-authenticated POST, PATCH, PUT, and DELETE handlers to resolve
caller identity and return 401 Unauthorized when resolution fails, while keeping
GET and LIST handlers permitted without identity. Place this requirement
alongside the existing decode/validation and service-call guidance.
- Around line 17-18: Update the ResourceHandler route example to use the
parent_id placeholder consistently with the mux.Vars(r)["parent_id"] lookup,
replacing the inconsistent cluster_id placeholder while preserving the
documented nested route structure.
- Around line 66-73: Update the “Error constructor functions” section in
CLAUDE.md to document the required errors. Add entries for errors.Conflict and
errors.ValidationWithDetails alongside the existing errors.NotFound,
errors.Validation, errors.GeneralError, errors.MalformedRequest, and
errors.BadRequest entries, preserving their signatures and HTTP error semantics
from pkg/errors.
In `@pkg/handlers/helpers_test.go`:
- Around line 334-370: Extend TestApplyFieldFilter with an invalid-fields
subtest using a rejected field such as “nonexistent”, then assert
applyFieldFilter returns the expected non-nil *errors.ServiceError and its
HTTPCode. Keep the existing no-fields and valid-fields coverage unchanged.
- Around line 150-162: Fix the “undefined decodeType” subtest by either
supplying a validator that sets validateCalled or removing the unused flag and
its assertion; retain assertions for successful decoding. Align the test’s
expected error behavior with decodeAndValidate’s handling of unknown decodeType,
updating it if the helper rejects unrecognized modes instead of accepting them.
- Around line 302-324: Extend the table-driven tests for describeJSONKind with a
reflect.Kind that is not explicitly handled, such as reflect.Invalid, and expect
the fallback text “the correct type”. Keep the existing mappings unchanged and
ensure the new case exercises the default branch.
In `@pkg/handlers/helpers.go`:
- Around line 57-60: Update the request-body handling helper around io.ReadAll
to wrap r.Body with http.MaxBytesReader using the established request size limit
before reading, ensuring oversized bodies are rejected with HTTP 413 while
preserving malformed-request handling for other read failures.
- Around line 62-84: Update the decode-mode handling around the strict/default
switch to use a compile-checked mode type or explicitly reject any unrecognized
decodeType value instead of silently falling back to permissive decoding;
preserve strict behavior for the strict mode and return an appropriate error for
invalid values. In both the strict decoder and permissive path, pass req
directly to Decode and Unmarshal rather than taking its address.
In `@pkg/handlers/resource_handler_test.go`:
- Around line 1139-1144: Update the test cases for validateName to exercise the
dbNameMaxLen fallback when both configured lengths are zero. Replace or
supplement the “no validation when zero lengths” case with a name longer than
dbNameMaxLen and expect http.StatusBadRequest, while retaining coverage for a
valid name if needed.
🪄 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: af0ef66b-be99-4480-8fb6-f2aa043ab0cf
📒 Files selected for processing (13)
AGENTS.mdCLAUDE.mdpkg/handlers/CLAUDE.mdpkg/handlers/framework.gopkg/handlers/framework_test.gopkg/handlers/helpers.gopkg/handlers/helpers_test.gopkg/handlers/name_validation_handler_test.gopkg/handlers/resource_handler.gopkg/handlers/resource_handler_test.gopkg/handlers/resource_status_handler.gopkg/handlers/root_resource_handler.gopkg/handlers/validation.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 with no reviewable changes (3)
- pkg/handlers/framework_test.go
- pkg/handlers/framework.go
- pkg/handlers/name_validation_handler_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- CLAUDE.md
- AGENTS.md
- pkg/handlers/resource_status_handler.go
- pkg/handlers/resource_handler.go
475e10a to
479bea8
Compare
| @@ -127,12 +126,6 @@ Use `pkg/logger/` — `logger.Info(ctx, "msg")`, `logger.With(ctx, "key", val).E | |||
|
|
|||
| ### Handlers | |||
There was a problem hiding this comment.
### Handlers heading is empty, falls straight through to ### Services. Either fill it in (see CLAUDE.md's ### Handler Pipeline) or remove it.
| Apply `?fields=` query parameter filtering: | ||
|
|
||
| ```go | ||
| result, svcErr := applyFieldFilter(r, presenters.PresentResource(resource)) |
There was a problem hiding this comment.
if err != nil checks an undefined variable, the assignment above is svcErr. Would fail to compile if copied verbatim.
2 participants
Summary
Removes the
handlerConfigstruct and handle* runner functions from the handler layer, flattening all HTTP handlers to callhandleErrorandwriteJSONResponsedirectly. The previous approach wrapped handler logic in closures that were immediately unwrapped and executed by generic runner functions, adding indirection without value. This refactor follows the standard Go HTTP handler pattern, improves readability with top-to-bottom execution flow, and eliminates per-request closure allocations.HYPERFLEET-1369
Changes
Handler framework simplification:
handlerConfigstruct that wrapped handler logic in closureshandle,handleSoftDelete,handleGet,handleList,handleForceDelete,handleCreateWithNoContent, and one additional runner)framework.gotohelpers.go(254 lines deleted from framework.go, 119 lines added to helpers.go)decodeAndValidatehelper for shared decode+validate pattern with optional "strict" mode for PATCH requestshandleError,writeJSONResponse, andapplyFieldFilterutility functionsHandler refactoring:
resource_handler.go(318 lines),root_resource_handler.go(436 lines), andresource_status_handler.go(80 lines) to flat implementationshandleError(r, w, err)andwriteJSONResponse(w, r, statusCode, data)directly instead of wrapping in config structsvalidatetype definition tovalidation.gowhere its implementations liveTesting:
helpers.gofunctions (400 new lines inhelpers_test.go)TestResourceHandler_Create_NameValidationfrom separate file intoresource_handler_test.gofor better organizationname_validation_handler_test.go(94 lines) - tests merged into main test fileDocumentation:
pkg/handlers/CLAUDE.mdwith comprehensive handler patterns, validation patterns, error handling guidance, and field filtering examples (120 lines)CLAUDE.mdto reflect new handler structureTest Plan
make lintpassesmake test-allpasses