fix(pagination): resolve page number conversion and key-based pagination issues#2003
fix(pagination): resolve page number conversion and key-based pagination issues#2003
Conversation
WalkthroughAdds a new pagination helper ReadPageRequest in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI command
participant CU as clientutils.ReadPageRequest
participant GRPC as gRPC query handler
User->>CLI: run list command with pagination flags
CLI->>CU: ReadPageRequest(cmd.Flags())
CU-->>CLI: PageRequest or error
CLI->>GRPC: Query with PageRequest
GRPC-->>CLI: Response (items + pagination)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
C:x/cert
C:x/deployment
C:x/audit
C:x/market
C:x/provider
C:CLI
labels
Zblocker64
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
x/market/client/cli/lease.go (1)
63-66: Misleading help text: “Query order” for a lease getter.Minor polish for UX.
Apply:
- Short: "Query order", + Short: "Query lease",x/market/client/cli/bid.go (1)
63-66: Misleading help text: “Query order” for a bid getter.Apply:
- Short: "Query order", + Short: "Query bid",x/market/keeper/grpc_query.go (4)
33-41: Defensively reject requests that set both Key and Offset.Client fixes help, but gRPC should also enforce mutual exclusivity to protect server invariants.
Apply this diff:
if req.Pagination == nil { req.Pagination = &sdkquery.PageRequest{} } else if req.Pagination != nil && req.Pagination.Offset > 0 && req.Filters.State == "" { return nil, status.Error(codes.InvalidArgument, "invalid request parameters. if offset is set, filter.state must be provided") } +// Defensive: page key and numeric offset are mutually exclusive +if len(req.Pagination.Key) > 0 && req.Pagination.Offset > 0 { + return nil, status.Error(codes.InvalidArgument, "invalid request parameters. set either page key or offset, not both") +}
189-196: Reverse-search decode fix (unsolicited[0]) is correct.This removes the off‑by‑index bug and matches the encoder which sets a single flag byte.
For readability, consider a named constant:
+const unsolicitedReverseSearch byte = 1 @@ - if unsolicited[0] == 1 { + if unsolicited[0] == unsolicitedReverseSearch { reverseSearch = true }
164-172: Add the same key/offset mutual‑exclusion guard for Bids.Mirror the Orders validation to block ambiguous pagination requests.
Apply this diff:
if req.Pagination == nil { req.Pagination = &sdkquery.PageRequest{} } else if req.Pagination != nil && req.Pagination.Offset > 0 && req.Filters.State == "" { return nil, status.Error(codes.InvalidArgument, "invalid request parameters. if offset is set, filter.state must be provided") } +if len(req.Pagination.Key) > 0 && req.Pagination.Offset > 0 { + return nil, status.Error(codes.InvalidArgument, "invalid request parameters. set either page key or offset, not both") +}
318-326: Also enforce key/offset mutual exclusivity for Leases.Keeps all three query handlers consistent and resilient against malformed clients.
Apply this diff:
if req.Pagination == nil { req.Pagination = &sdkquery.PageRequest{} } else if req.Pagination != nil && req.Pagination.Offset > 0 && req.Filters.State == "" { return nil, status.Error(codes.InvalidArgument, "invalid request parameters. if offset is set, filter.state must be provided") } +if len(req.Pagination.Key) > 0 && req.Pagination.Offset > 0 { + return nil, status.Error(codes.InvalidArgument, "invalid request parameters. set either page key or offset, not both") +}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
client/utils.go(1 hunks)x/audit/client/cli/query.go(2 hunks)x/cert/client/cli/query.go(2 hunks)x/deployment/client/cli/query.go(2 hunks)x/market/client/cli/bid.go(2 hunks)x/market/client/cli/lease.go(2 hunks)x/market/client/cli/order.go(2 hunks)x/market/keeper/grpc_query.go(3 hunks)x/provider/client/cli/query.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
x/audit/client/cli/query.go (1)
client/utils.go (1)
ReadPageRequest(13-46)
x/provider/client/cli/query.go (1)
client/utils.go (1)
ReadPageRequest(13-46)
x/market/client/cli/bid.go (1)
client/utils.go (1)
ReadPageRequest(13-46)
x/market/client/cli/lease.go (1)
client/utils.go (1)
ReadPageRequest(13-46)
x/deployment/client/cli/query.go (1)
client/utils.go (1)
ReadPageRequest(13-46)
x/cert/client/cli/query.go (1)
client/utils.go (1)
ReadPageRequest(13-46)
x/market/client/cli/order.go (1)
client/utils.go (1)
ReadPageRequest(13-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: lint
- GitHub Check: build-bins
- GitHub Check: tests
- GitHub Check: coverage
- GitHub Check: release-dry-run
🔇 Additional comments (2)
x/market/keeper/grpc_query.go (2)
276-291: Keeping Bids NextKey raw and encoding with flags at the end is correct.LGTM. The token now round‑trips cleanly across boundary pages and reverse index scans.
128-147: Approve — keep SDK NextKey rawVerified: EncodePaginationKey/DecodePaginationKey are used consistently across grpc_query.go files, util/query/pagination.go provides implementations and tests (util/query/pagination_test.go), and no NextKey+searchPrefix mutations were found. LGTM.
2 participants
Description
Closes: #XXXX
This PR resolves critical pagination bugs that were preventing proper page navigation in CLI queries and breaking provider startup order discovery. The issues included:
--page Nflags were not converting to proper offset valuesKey Changes:
Client-side fixes:
ReadPageRequestfunction with page-to-offset conversion (--page 2→offset = (2-1) * limit)x/provider/client/cli/query.gox/market/client/cli/{order,bid,lease}.gox/deployment/client/cli/query.gox/cert/client/cli/query.gox/audit/client/cli/query.goServer-side fixes:
unsolicited[1]→unsolicited[0])Testing:
--page Nand--page-keymethods return identical results across all query typesAuthor Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow-up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.md