fix: include pagination.key at reverse mode #20939
Conversation
WalkthroughWalkthroughThe changes address a bug in the Cosmos SDK where the collection iterator did not correctly include the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
beer-1
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: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- types/query/collections_pagination.go (1 hunks)
- types/query/collections_pagination_test.go (1 hunks)
Additional context used
Path-based instructions (3)
types/query/collections_pagination_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"types/query/collections_pagination.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Additional comments not posted (3)
types/query/collections_pagination_test.go (1)
86-95: LGTM! Ensure sufficient test coverage.The new test case "with key and reverse" is well-structured and appears to correctly verify the desired behavior.
However, ensure that this test case sufficiently covers all edge cases related to the reverse mode with a key.
Verification successful
LGTM! Ensure sufficient test coverage.
The new test case "with key and reverse" is well-structured and appears to correctly verify the desired behavior. The presence of multiple test cases in
collections_pagination_test.goindicates that pagination functionality, including reverse mode and key handling, is being tested in various scenarios.
- "nil pagination"
- "with key and limit"
- "with reverse"
- "with key and reverse"
- "with offset and count total"
- "filtered no key"
- "filtered with key"
This suggests that edge cases are likely covered.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the new test case "with key and reverse" sufficiently covers all edge cases. # Test: Search for all test cases in the file. Expect: Only occurances of the test case "with key and reverse". rg --type go -A 5 $'with key and reverse'Length of output: 431
Script:
#!/bin/bash # Description: List all test case names in collections_pagination_test.go to check for coverage of reverse mode with a key. # List all test cases in the file rg --type go '": {' collections_pagination_test.goLength of output: 124
Script:
#!/bin/bash # Description: List all test case names in collections_pagination_test.go to check for coverage of reverse mode with a key. # List all test cases in the file rg --type go '": \{' collections_pagination_test.goLength of output: 124
Script:
#!/bin/bash # Description: Find the correct path for the collections_pagination_test.go file and list all test case names in it to check for coverage of reverse mode with a key. # Find all Go test files fd --type f --extension go --search-path . # List all test cases in the file once the correct path is identified rg --type go '": \{' $(fd collections_pagination_test.go)Length of output: 90416
types/query/collections_pagination.go (1)
335-342: LGTM! But verify the function usage in the codebase.The changes in
getCollItercorrectly adjust the start key by including the prefix and increasing the start key to include it in the iteration.However, ensure that all function calls to
getCollIterare consistent with the updated logic.CHANGELOG.md (1)
121-121: Changelog entry looks good!The changelog entry clearly describes the fix for the collection reverse iterator to include
pagination.keyin the result.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- types/query/collections_pagination.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- types/query/collections_pagination.go
| // if we are in reverse mode, we need to increase the start key | ||
| // to include the start key in the iteration. | ||
| start = storetypes.PrefixEndBytes(append(prefix, start...)) | ||
| end := prefix |
There was a problem hiding this comment.
nit: this is not needed, just pass prefix to IterateRaw in the place of end. Although we may want to keep it for readability
There was a problem hiding this comment.
yea I also think it's good for readability
Description
Closes: #20938
Author 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...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
pagination.keyin the result, ensuring more accurate data retrieval.