Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds a reusable paging parser with validation and tests, extends ChangesPaging Support
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant Route
participant PagingMiddleware
participant PagingParser
participant Handler
Client->>Route: Request with paging query parameters
Route->>PagingMiddleware: Invoke configured middleware
PagingMiddleware->>PagingParser: ParseAndValidate query values
PagingParser-->>PagingMiddleware: Validated paging or error
alt Invalid parameters
PagingMiddleware-->>Client: 400 Bad Request
else Valid parameters
PagingMiddleware->>Handler: Set context paging and continue
Handler-->>Client: Process request
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
cmd/api/src/api/middleware/paging.go (1)
39-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHoist
paging/errand drop the else-after-return.Per coding guidelines, variable initializations should be hoisted to the top of the function where possible; this also removes the unnecessary
elseafter an earlyreturn.♻️ Proposed refactor
return http.HandlerFunc(func(response http.ResponseWriter, request *http.Request) { - if paging, err := parser.ParseAndValidate(request.URL.Query()); err != nil { - api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusBadRequest, fmt.Sprintf(api.FmtErrorResponseDetailsBadQueryParameters, err), request), response) - return - } else { - bhCtx := bhctx.Get(request.Context()) - bhCtx.Skip = paging.Skip - bhCtx.Limit = paging.Limit - } + paging, err := parser.ParseAndValidate(request.URL.Query()) + if err != nil { + api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusBadRequest, fmt.Sprintf(api.FmtErrorResponseDetailsBadQueryParameters, err), request), response) + return + } + + bhCtx := bhctx.Get(request.Context()) + bhCtx.Skip = paging.Skip + bhCtx.Limit = paging.Limit next.ServeHTTP(response, request) })As per coding guidelines: "Group variable initializations in a
var ( ... )block and hoist them to the top of the function when possible."🤖 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 `@cmd/api/src/api/middleware/paging.go` around lines 39 - 46, Hoist the paging parse variables in the paging middleware and remove the unnecessary else after the early return. In the middleware function that calls parser.ParseAndValidate and writes the bad request response, declare paging and err together in a var block at the top when possible, then keep the validation/error handling as a straight if-return flow and assign bhCtx.Skip and bhCtx.Limit immediately after success.Source: Coding guidelines
🤖 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 `@packages/go/params/paging.go`:
- Around line 69-71: The PagingValidationError.Error method is only formatting
Err and Value, so it drops the actual Parameter and produces the wrong field
name for custom SkipKey values. Update Error() to include the Parameter when
present, and keep the message consistent with how
cmd/api/src/api/middleware/paging.go forwards the validation error into the 400
response. Use PagingValidationError, Parameter, and ErrInvalidSkip to locate the
fix.
---
Nitpick comments:
In `@cmd/api/src/api/middleware/paging.go`:
- Around line 39-46: Hoist the paging parse variables in the paging middleware
and remove the unnecessary else after the early return. In the middleware
function that calls parser.ParseAndValidate and writes the bad request response,
declare paging and err together in a var block at the top when possible, then
keep the validation/error handling as a straight if-return flow and assign
bhCtx.Skip and bhCtx.Limit immediately after success.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 90e236f3-9edb-4ca8-9f2f-0792337d63bb
📒 Files selected for processing (5)
cmd/api/src/api/middleware/paging.gocmd/api/src/api/router/router.gocmd/api/src/bhctx/bhctx.gopackages/go/params/paging.gopackages/go/params/paging_test.go
| func (s *PagingValidationError) Error() string { | ||
| return fmt.Sprintf("%s: %q", s.Err, s.Value) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Error() never surfaces the actual Parameter, causing misleading messages for custom SkipKey.
ErrInvalidSkip's wording hardcodes "skip", but SkipKey is configurable. When a caller configures a different key (e.g. "offset") and validation fails, Error() still returns "skip must be a non-negative integer: ...", even though the offending query parameter was offset. The middleware at cmd/api/src/api/middleware/paging.go (Line 40) forwards this string directly into the 400 response without using Parameter, so end users will see the wrong parameter name referenced in their error.
🐛 Proposed fix
func (s *PagingValidationError) Error() string {
- return fmt.Sprintf("%s: %q", s.Err, s.Value)
+ return fmt.Sprintf("%s: %s=%q", s.Err, s.Parameter, s.Value)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *PagingValidationError) Error() string { | |
| return fmt.Sprintf("%s: %q", s.Err, s.Value) | |
| } | |
| func (s *PagingValidationError) Error() string { | |
| return fmt.Sprintf("%s: %s=%q", s.Err, s.Parameter, s.Value) | |
| } |
🤖 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 `@packages/go/params/paging.go` around lines 69 - 71, The
PagingValidationError.Error method is only formatting Err and Value, so it drops
the actual Parameter and produces the wrong field name for custom SkipKey
values. Update Error() to include the Parameter when present, and keep the
message consistent with how cmd/api/src/api/middleware/paging.go forwards the
validation error into the 400 response. Use PagingValidationError, Parameter,
and ErrInvalidSkip to locate the fix.
mistahj67
force-pushed
the
BED-8597
branch
2 times, most recently
from
d1e927c to
efbdad9
Compare
2 participants
Description
Describe your changes in detail
Motivation and Context
Resolves BED-8597
Why is this change required? What problem does it solve?
How Has This Been Tested?
Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
skipandlimitquery parameters, with configurable defaults and validation.limit = -1(when enabled).400 Bad Requestwith clear error details.