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 2 commits into
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 (7)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughSummary by CodeRabbit
WalkthroughList query parsing and validation move from Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ListHandler
participant parseListParams
participant ListService
Client->>ListHandler: Send list request with query parameters
ListHandler->>parseListParams: Parse and validate query
parseListParams-->>ListHandler: Return ListArguments or validation error
ListHandler->>ListService: Execute list operation with ListArguments
ListService-->>ListHandler: 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: 3 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 1094 lines (>500) | +2 |
| Sensitive paths | none | +0 |
| Test coverage | Missing tests for: pkg/api | +1 |
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
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.
| RefType string `validate:"required_with=RefTargetID"` | ||
| RefTargetID string `validate:"required_with=RefType"` | ||
| Size int64 `validate:"min=1,max=100"` | ||
| Page int64 `validate:"min=1,max=10000000"` |
There was a problem hiding this comment.
Why max - 10000000? Is that a realistic max? What about like 100000? What's the reason?
There was a problem hiding this comment.
Well, there shouldn't be any max there, but (Page-1)*Size can overflow int, and a negative number will be passed to the database, so it's just-a-big-number.
But honestly, I agree with you, 10000000 might be confusing, 100000 is not 🙂
2 participants
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