HYPERFLEET-1274 - refactor: move list parameter parsing to handler layer#307
HYPERFLEET-1274 - refactor: move list parameter parsing to handler layer#307kuudori wants to merge 1 commit into
Conversation
Extract query parameter parsing from pkg/services/types.go into pkg/handlers/list_params.go. Use go-playground/validator for struct-level validation and return structured error details via ValidationWithDetails. Remove defensive size checks from generic.go service layer (now validated at the handler boundary) while keeping a guard for Size <= 0. Update stale mockgen example in docs/development.md.
|
[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 |
📝 WalkthroughWalkthroughList query parsing and validation move from Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ResourceHandler
participant parseListParams
participant sqlResourceService
Client->>ResourceHandler: Send list request with query parameters
ResourceHandler->>parseListParams: Parse and validate URL query
parseListParams-->>ResourceHandler: Return services.ListArguments
ResourceHandler->>sqlResourceService: Execute list operation
sqlResourceService-->>ResourceHandler: Return list results
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 2 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 1031 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: 1
🧹 Nitpick comments (1)
pkg/handlers/list_params_test.go (1)
246-255: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the
pageupper bound.Once
Pagegains amax(seepkg/handlers/list_params.goLine 20-21), add a range-error case (e.g.?page=99999999999999999999) so the overflow guard is regression-locked. Error paths are otherwise well covered.As per path instructions: "Error paths SHOULD be tested, not just happy paths".
🤖 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/list_params_test.go` around lines 246 - 255, Add a table-driven test case in the list-parameter validation tests covering a page value above the `Page` maximum, such as an excessively large numeric page query, and assert the expected range-validation error. Keep the existing valid large-page case unchanged and use the current error assertion pattern.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/list_params.go`:
- Around line 20-21: Cap Page or explicitly validate its multiplication with
Size before loadList computes the offset and calls Fetch. Update the Page
validation in list parameters and ensure (args.Page-1)*int(args.Size) cannot
overflow into a negative offset, while preserving valid pagination behavior.
---
Nitpick comments:
In `@pkg/handlers/list_params_test.go`:
- Around line 246-255: Add a table-driven test case in the list-parameter
validation tests covering a page value above the `Page` maximum, such as an
excessively large numeric page query, and assert the expected range-validation
error. Keep the existing valid large-page case unchanged and use the current
error assertion pattern.
🪄 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: 15f509e2-4d57-4412-9603-b9ad8c7ae08f
📒 Files selected for processing (11)
docs/development.mdpkg/handlers/framework.gopkg/handlers/list_params.gopkg/handlers/list_params_test.gopkg/handlers/resource_handler.gopkg/handlers/resource_status_handler.gopkg/handlers/root_resource_handler.gopkg/services/generic.gopkg/services/resource.gopkg/services/types.gopkg/services/types_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 with no reviewable changes (1)
- pkg/services/types_test.go
| Size int64 `validate:"min=1,max=100"` | ||
| Page int `validate:"min=1"` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
go-playground/validator v10 numeric max tag on int field
💡 Result:
In the go-playground/validator v10 library, the max tag behaves differently depending on the type of the field being validated [1][2]. When applied to an integer (int) field, the max tag ensures that the value of the integer is less than or equal to the parameter specified in the tag [1][2]. Example: type User struct { Age int validate:"max=100" } In this example, the validator will return an error if the Age field is greater than 100 [1][2]. Key points regarding the max tag: - Numeric comparison: For numbers (including various int sizes, uint, and float types), it performs a direct numeric comparison [1][2]. - Other types: The max tag is polymorphic. For strings, it checks the character length (number of characters); for slices, arrays, and maps, it validates the number of items [1][2]. - Order of operations: If multiple validation tags are used on the same field, they are processed in the order they are defined [1][2]. - Potential issues: Ensure that the validation tag is placed correctly. In some complex scenarios—such as when using custom types or specific struct-backed fields—improper ordering or unsupported types can lead to unexpected behavior or panics [3]. Always ensure your data types are compatible with the library's expected input formats [1][2].
Citations:
- 1: https://github.com/go-playground/validator/blob/v10.30.2/doc.go
- 2: https://github.com/go-playground/validator/blob/v10.12.0/doc.go
- 3: Panic on max, min Validation Tag with struct Fields in Version v10.20.0 and above go-playground/validator#1300
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files 'pkg/handlers/list_params.go' 'pkg/services/generic.go'
echo "== list_params.go =="
cat -n pkg/handlers/list_params.go
echo "== generic.go relevant slices =="
rg -n "loadList|args\.Page|args\.Size|offset|limit" pkg/services/generic.go
sed -n '240,320p' pkg/services/generic.goRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 8786
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== list-related call sites =="
rg -n "Fetch\(|Offset\(|Limit\(|Count\(" pkg | sed -n '1,200p'
echo "== DAO fetch implementation =="
rg -n "type GenericDao|func .*Fetch\(" pkgRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 1979
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Probe Go int overflow behavior for the offset expression shape.
python3 - <<'PY'
import sys
max_int = 2**63 - 1
size = 100
page = max_int // size + 2 # first page value that makes (page-1)*size overflow 64-bit signed
offset = (page - 1) * size
wrapped = ((offset + 2**63) % 2**64) - 2**63
print("page =", page)
print("exact offset =", offset)
print("wrapped int64 =", wrapped)
print("negative =", wrapped < 0)
PYRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 286
pkg/handlers/list_params.go:21 / pkg/services/generic.go:275 — Cap Page before computing the list offset. loadList multiplies (args.Page-1)*int(args.Size); with only min=1, a large valid page can overflow into a negative offset (CWE-190). Add an upper bound or explicit overflow check before Fetch.
🤖 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/list_params.go` around lines 20 - 21, Cap Page or explicitly
validate its multiplication with Size before loadList computes the offset and
calls Fetch. Update the Page validation in list parameters and ensure
(args.Page-1)*int(args.Size) cannot overflow into a negative offset, while
preserving valid pagination behavior.
1 participant
Summary
pkg/services/types.gointopkg/handlers/list_params.go, aligning with the handler-validates, service-executes patterngo-playground/validatorfor struct-level validation with structured error details viaValidationWithDetailsgeneric.go) while keeping aSize <= 0guardTest plan
make verify-allpasses (vet + lint + unit tests)make test-integrationpassespage=1,size=20,order=created_time desc)detailsarray?fields=nameresponse includesidfield automatically?ref_type=depwithout?ref_target_idreturns validation error