From 79d23008502eee143ad7c8dd7f4061f860f6766f Mon Sep 17 00:00:00 2001 From: Chung Wu Date: Mon, 21 Mar 2022 23:10:59 +0800 Subject: [PATCH] fix: filtered pagination NextKey off-by-one when Offset != 0 and CountTotal == true (#11408) ## Description Closes: [#11407](https://github.com/cosmos/cosmos-sdk/issues/11407) In the original code, when CountTotal == true, the iteration goes even after the `end+1`-th hit. The `end+1`-th hit is expected to be `NextKey`. However, if there is a no-hit after after that, `numHits == end+1` is still true, and `NextKey` will be set to key beyond the `end+1`-th hit, resulting in consecutive queries missing the `end+1`-th hit. --- ### 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... - [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers 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.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable) --- types/query/filtered_pagination.go | 4 +- types/query/filtered_pagination_test.go | 63 +++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/types/query/filtered_pagination.go b/types/query/filtered_pagination.go index 2856d513f2..47dd31aaba 100644 --- a/types/query/filtered_pagination.go +++ b/types/query/filtered_pagination.go @@ -98,7 +98,9 @@ func FilteredPaginate( } if numHits == end+1 { - nextKey = iterator.Key() + if nextKey == nil { + nextKey = iterator.Key() + } if !countTotal { break diff --git a/types/query/filtered_pagination_test.go b/types/query/filtered_pagination_test.go index 49bfe34860..694d0c3cbb 100644 --- a/types/query/filtered_pagination_test.go +++ b/types/query/filtered_pagination_test.go @@ -253,3 +253,66 @@ func execFilterPaginate(store sdk.KVStore, pageReq *query.PageRequest, appCodec return balResult, res, err } + +func (s *paginationTestSuite) TestFilteredPaginationsNextKey() { + app, ctx, appCodec := setupTest(s.T()) + + var balances sdk.Coins + + for i := 1; i <= 10; i++ { + denom := fmt.Sprintf("test%ddenom", i) + balances = append(balances, sdk.NewInt64Coin(denom, int64(i))) + } + + balances = balances.Sort() + addr1 := sdk.AccAddress([]byte("addr1")) + acc1 := app.AccountKeeper.NewAccountWithAddress(ctx, addr1) + app.AccountKeeper.SetAccount(ctx, acc1) + s.Require().NoError(testutil.FundAccount(app.BankKeeper, ctx, addr1, balances)) + store := ctx.KVStore(app.GetKey(types.StoreKey)) + + execFilterPaginate := func(store sdk.KVStore, pageReq *query.PageRequest, appCodec codec.Codec) (balances sdk.Coins, res *query.PageResponse, err error) { + balancesStore := prefix.NewStore(store, types.BalancesPrefix) + accountStore := prefix.NewStore(balancesStore, address.MustLengthPrefix(addr1)) + + var balResult sdk.Coins + res, err = query.FilteredPaginate(accountStore, pageReq, func(key []byte, value []byte, accumulate bool) (bool, error) { + var amount sdk.Int + err := amount.Unmarshal(value) + if err != nil { + return false, err + } + + // filter odd amounts + if amount.Int64()%2 == 1 { + if accumulate { + balResult = append(balResult, sdk.NewCoin(string(key), amount)) + } + + return true, nil + } + + return false, nil + }) + + return balResult, res, err + } + + s.T().Log("verify next key of offset query") + pageReq := &query.PageRequest{Key: nil, Limit: 1, CountTotal: true} + balances, res, err := execFilterPaginate(store, pageReq, appCodec) + s.Require().NoError(err) + s.Require().NotNil(res) + s.Require().Equal(1, len(balances)) + s.Require().Equal(balances[0].Amount.Int64(), int64(1)) + s.Require().Equal(uint64(5), res.Total) + s.Require().NotNil(res.NextKey) + + pageReq = &query.PageRequest{Key: res.NextKey, Limit: 1} + balances, res, err = execFilterPaginate(store, pageReq, appCodec) + s.Require().NoError(err) + s.Require().NotNil(res) + s.Require().Equal(1, len(balances)) + s.Require().Equal(balances[0].Amount.Int64(), int64(3)) + s.Require().NotNil(res.NextKey) +}