From 2bce66b9598240a1ef812a0f4cb6b000f9217308 Mon Sep 17 00:00:00 2001 From: Geoffrey Ragot Date: Fri, 8 Jul 2022 12:32:36 +0200 Subject: [PATCH 1/2] feat(api): Add configurable page_size on paginated request. This add the query parameter 'page_size' on both GET /transactions, GET /accounts, and GET /balances endpoints. The parameter is capped to a value of 1000. The default value still the same as before (15). The PR also fix some tests around pagination which was working but was not doing the right thing (see api/controllers/pagination_test.go line 209). --- pkg/api/controllers/account_controller.go | 15 ++- .../controllers/account_controller_test.go | 80 ++++++++++++ pkg/api/controllers/balance_controller.go | 16 ++- pkg/api/controllers/pagination_test.go | 118 ++++++++++++------ pkg/api/controllers/query.go | 38 ++++++ pkg/api/controllers/swagger.yaml | 18 +++ pkg/api/controllers/transaction_controller.go | 15 ++- .../transaction_controller_test.go | 87 +++++++++++++ .../opentelemetrytraces/storage_test.go | 4 +- pkg/storage/accounts.go | 11 +- pkg/storage/balances.go | 9 +- pkg/storage/sqlstorage/accounts.go | 16 +-- pkg/storage/sqlstorage/accounts_test.go | 8 +- pkg/storage/sqlstorage/balances.go | 12 +- pkg/storage/sqlstorage/balances_test.go | 14 +-- pkg/storage/sqlstorage/migrations_test.go | 2 +- pkg/storage/sqlstorage/pagination.go | 5 +- pkg/storage/sqlstorage/store_bench_test.go | 2 +- pkg/storage/sqlstorage/store_test.go | 28 ++--- pkg/storage/sqlstorage/transactions.go | 31 ++--- pkg/storage/storage.go | 2 +- pkg/storage/transactions.go | 11 +- 22 files changed, 419 insertions(+), 123 deletions(-) create mode 100644 pkg/api/controllers/query.go diff --git a/pkg/api/controllers/account_controller.go b/pkg/api/controllers/account_controller.go index c28d19f74..75b71d087 100644 --- a/pkg/api/controllers/account_controller.go +++ b/pkg/api/controllers/account_controller.go @@ -49,7 +49,8 @@ func (ctl *AccountController) GetAccounts(c *gin.Context) { c.Query("address") != "" || len(c.QueryMap("metadata")) > 0 || c.Query("balance") != "" || - c.Query("balance_operator") != "" { + c.Query("balance_operator") != "" || + c.Query("page_size") != "" { ResponseError(c, ledger.NewValidationError( "no other query params can be set with 'pagination_token'")) return @@ -75,7 +76,8 @@ func (ctl *AccountController) GetAccounts(c *gin.Context) { WithAddressFilter(token.AddressRegexpFilter). WithBalanceFilter(token.BalanceFilter). WithBalanceOperatorFilter(token.BalanceOperatorFilter). - WithMetadataFilter(token.MetadataFilter) + WithMetadataFilter(token.MetadataFilter). + WithPageSize(token.PageSize) } else { balance := c.Query("balance") @@ -97,12 +99,19 @@ func (ctl *AccountController) GetAccounts(c *gin.Context) { } } + pageSize, err := getPageSize(c) + if err != nil { + ResponseError(c, err) + return + } + accountsQuery = storage.NewAccountsQuery(). WithAfterAddress(c.Query("after")). WithAddressFilter(c.Query("address")). WithBalanceFilter(balance). WithBalanceOperatorFilter(balanceOperator). - WithMetadataFilter(c.QueryMap("metadata")) + WithMetadataFilter(c.QueryMap("metadata")). + WithPageSize(pageSize) } cursor, err = l.(*ledger.Ledger).GetAccounts(c.Request.Context(), *accountsQuery) diff --git a/pkg/api/controllers/account_controller_test.go b/pkg/api/controllers/account_controller_test.go index d35f89714..b4656cfd1 100644 --- a/pkg/api/controllers/account_controller_test.go +++ b/pkg/api/controllers/account_controller_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/base64" "encoding/json" + "fmt" "net/http" "net/url" "testing" @@ -13,6 +14,7 @@ import ( "github.com/numary/ledger/pkg/api/controllers" "github.com/numary/ledger/pkg/api/internal" "github.com/numary/ledger/pkg/core" + "github.com/numary/ledger/pkg/storage" "github.com/numary/ledger/pkg/storage/sqlstorage" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -336,6 +338,84 @@ func TestGetAccounts(t *testing.T) { })) } +func TestGetAccountsWithPageSize(t *testing.T) { + internal.RunTest(t, fx.Invoke(func(lc fx.Lifecycle, api *api.API, driver storage.Driver) { + lc.Append(fx.Hook{ + OnStart: func(ctx context.Context) error { + var previousLog *core.Log + logs := make([]core.Log, 0) + store := internal.GetStore(t, driver, context.Background()) + + for i := 0; i < 3*controllers.MaxPageSize; i++ { + log := core.NewSetMetadataLog(previousLog, core.SetMetadata{ + TargetID: fmt.Sprintf("accounts:%06d", i), + TargetType: core.MetaTargetTypeAccount, + Metadata: map[string]json.RawMessage{ + "foo": []byte("{}"), + }, + }) + logs = append(logs, log) + previousLog = &log + } + require.NoError(t, store.AppendLog(context.Background(), logs...)) + + t.Run("invalid page size", func(t *testing.T) { + rsp := internal.GetAccounts(api, url.Values{ + "page_size": []string{"nan"}, + }) + assert.Equal(t, http.StatusBadRequest, rsp.Result().StatusCode, rsp.Body.String()) + + err := sharedapi.ErrorResponse{} + internal.Decode(t, rsp.Body, &err) + assert.EqualValues(t, sharedapi.ErrorResponse{ + ErrorCode: controllers.ErrValidation, + ErrorMessage: controllers.ErrInvalidPageSize.Error(), + }, err) + }) + t.Run("page size over maximum", func(t *testing.T) { + httpResponse := internal.GetAccounts(api, url.Values{ + "page_size": []string{fmt.Sprintf("%d", 2*controllers.MaxPageSize)}, + }) + assert.Equal(t, http.StatusOK, httpResponse.Result().StatusCode, httpResponse.Body.String()) + + cursor := internal.DecodeCursorResponse[core.Account](t, httpResponse.Body) + assert.Len(t, cursor.Data, controllers.MaxPageSize) + assert.Equal(t, cursor.PageSize, controllers.MaxPageSize) + assert.NotEmpty(t, cursor.Next) + assert.True(t, cursor.HasMore) + }) + t.Run("with page size greater than max count", func(t *testing.T) { + httpResponse := internal.GetAccounts(api, url.Values{ + "page_size": []string{fmt.Sprintf("%d", controllers.MaxPageSize)}, + "after": []string{fmt.Sprintf("accounts:%06d", controllers.MaxPageSize-100)}, + }) + assert.Equal(t, http.StatusOK, httpResponse.Result().StatusCode, httpResponse.Body.String()) + + cursor := internal.DecodeCursorResponse[core.Account](t, httpResponse.Body) + assert.Len(t, cursor.Data, controllers.MaxPageSize-100) + assert.Equal(t, controllers.MaxPageSize, cursor.PageSize) + assert.Empty(t, cursor.Next) + assert.False(t, cursor.HasMore) + }) + t.Run("with page size lower than max count", func(t *testing.T) { + httpResponse := internal.GetAccounts(api, url.Values{ + "page_size": []string{fmt.Sprintf("%d", controllers.MaxPageSize/10)}, + }) + assert.Equal(t, http.StatusOK, httpResponse.Result().StatusCode, httpResponse.Body.String()) + + cursor := internal.DecodeCursorResponse[core.Account](t, httpResponse.Body) + assert.Len(t, cursor.Data, controllers.MaxPageSize/10) + assert.Equal(t, cursor.PageSize, controllers.MaxPageSize/10) + assert.NotEmpty(t, cursor.Next) + assert.True(t, cursor.HasMore) + }) + + return nil + }, + }) + })) +} + func TestGetAccount(t *testing.T) { internal.RunTest(t, fx.Invoke(func(lc fx.Lifecycle, api *api.API) { lc.Append(fx.Hook{ diff --git a/pkg/api/controllers/balance_controller.go b/pkg/api/controllers/balance_controller.go index a866c0e81..ef7e38ed2 100644 --- a/pkg/api/controllers/balance_controller.go +++ b/pkg/api/controllers/balance_controller.go @@ -46,7 +46,8 @@ func (ctl *BalanceController) GetBalances(c *gin.Context) { if c.Query("pagination_token") != "" { if c.Query("after") != "" || - c.Query("address") != "" { + c.Query("address") != "" || + c.Query("page_size") != "" { ResponseError(c, ledger.NewValidationError( "no other query params can be set with 'pagination_token'")) return @@ -69,12 +70,21 @@ func (ctl *BalanceController) GetBalances(c *gin.Context) { balancesQuery = balancesQuery. WithOffset(token.Offset). WithAfterAddress(token.AfterAddress). - WithAddressFilter(token.AddressRegexpFilter) + WithAddressFilter(token.AddressRegexpFilter). + WithPageSize(token.PageSize) } else { + + pageSize, err := getPageSize(c) + if err != nil { + ResponseError(c, err) + return + } + balancesQuery = balancesQuery. WithAfterAddress(c.Query("after")). - WithAddressFilter(c.Query("address")) + WithAddressFilter(c.Query("address")). + WithPageSize(pageSize) } cursor, err := l.(*ledger.Ledger).GetBalances(c.Request.Context(), *balancesQuery) diff --git a/pkg/api/controllers/pagination_test.go b/pkg/api/controllers/pagination_test.go index 90a3b7b96..ad3a8fa80 100644 --- a/pkg/api/controllers/pagination_test.go +++ b/pkg/api/controllers/pagination_test.go @@ -12,13 +12,14 @@ import ( "github.com/numary/ledger/pkg/api" "github.com/numary/ledger/pkg/api/internal" "github.com/numary/ledger/pkg/core" - "github.com/numary/ledger/pkg/storage" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/fx" ) +// This test makes sense if maxAdditionalTxs < pageSize const ( + pageSize = 10 maxTxsPages = 3 maxAdditionalTxs = 2 ) @@ -41,7 +42,7 @@ func getPagination(t *testing.T, api *api.API, txsPages, additionalTxs int) func return func(ctx context.Context) error { var rsp *httptest.ResponseRecorder - numTxs := txsPages*storage.QueryDefaultLimit + additionalTxs + numTxs := txsPages*pageSize + additionalTxs for i := 0; i < numTxs; i++ { rsp = internal.PostTransaction(t, api, core.TransactionData{ Postings: core.Postings{ @@ -63,25 +64,31 @@ func getPagination(t *testing.T, api *api.API, txsPages, additionalTxs int) func require.Equal(t, fmt.Sprintf("%d", numTxs), rsp.Header().Get("Count")) var paginationToken string - var cursor *sharedapi.Cursor[core.Transaction] + cursor := &sharedapi.Cursor[core.Transaction]{} // MOVING FORWARD for i := 0; i < txsPages; i++ { - rsp = internal.GetTransactions(api, url.Values{ - "pagination_token": []string{paginationToken}, - }) + + values := url.Values{} + if paginationToken == "" { + values.Set("page_size", fmt.Sprintf("%d", pageSize)) + } else { + values.Set("pagination_token", paginationToken) + } + + rsp = internal.GetTransactions(api, values) assert.Equal(t, http.StatusOK, rsp.Result().StatusCode) cursor = internal.DecodeCursorResponse[core.Transaction](t, rsp.Body) - assert.Len(t, cursor.Data, storage.QueryDefaultLimit) + assert.Len(t, cursor.Data, pageSize) assert.Equal(t, cursor.Next != "", cursor.HasMore) // First txid of the page assert.Equal(t, - uint64((txsPages-i)*storage.QueryDefaultLimit+additionalTxs-1), cursor.Data[0].ID) + uint64((txsPages-i)*pageSize+additionalTxs-1), cursor.Data[0].ID) // Last txid of the page assert.Equal(t, - uint64((txsPages-i-1)*storage.QueryDefaultLimit+additionalTxs), cursor.Data[len(cursor.Data)-1].ID) + uint64((txsPages-i-1)*pageSize+additionalTxs), cursor.Data[len(cursor.Data)-1].ID) paginationToken = cursor.Next } @@ -104,27 +111,38 @@ func getPagination(t *testing.T, api *api.API, txsPages, additionalTxs int) func uint64(0), cursor.Data[len(cursor.Data)-1].ID) } + assert.Empty(t, cursor.Next) + // MOVING BACKWARD if txsPages > 0 { - for i := 0; i < txsPages; i++ { + back := 0 + for cursor.Previous != "" { paginationToken = cursor.Previous rsp = internal.GetTransactions(api, url.Values{ "pagination_token": []string{paginationToken}, }) assert.Equal(t, http.StatusOK, rsp.Result().StatusCode) cursor = internal.DecodeCursorResponse[core.Transaction](t, rsp.Body) - assert.Len(t, cursor.Data, storage.QueryDefaultLimit) + assert.Len(t, cursor.Data, pageSize) assert.Equal(t, cursor.Next != "", cursor.HasMore) + back++ + } + if additionalTxs > 0 { + assert.Equal(t, txsPages, back) + } else { + assert.Equal(t, txsPages-1, back) } // First txid of the first page assert.Equal(t, - uint64(txsPages*storage.QueryDefaultLimit+additionalTxs-1), cursor.Data[0].ID) + uint64(txsPages*pageSize+additionalTxs-1), cursor.Data[0].ID) // Last txid of the first page assert.Equal(t, - uint64((txsPages-1)*storage.QueryDefaultLimit+additionalTxs), cursor.Data[len(cursor.Data)-1].ID) + uint64((txsPages-1)*pageSize+additionalTxs), cursor.Data[len(cursor.Data)-1].ID) } + + assert.Empty(t, cursor.Previous) }) t.Run("accounts", func(t *testing.T) { @@ -136,17 +154,26 @@ func getPagination(t *testing.T, api *api.API, txsPages, additionalTxs int) func require.Equal(t, http.StatusOK, rsp.Result().StatusCode) require.Equal(t, fmt.Sprintf("%d", numAcc), rsp.Header().Get("Count")) + accPages := numAcc / pageSize + additionalAccs := numAcc % pageSize + var paginationToken string - var cursor *sharedapi.Cursor[core.Account] + cursor := &sharedapi.Cursor[core.Account]{} // MOVING FORWARD - for i := 0; i < txsPages; i++ { - rsp = internal.GetAccounts(api, url.Values{ - "pagination_token": []string{paginationToken}, - }) + for i := 0; i < accPages; i++ { + + values := url.Values{} + if paginationToken == "" { + values.Set("page_size", fmt.Sprintf("%d", pageSize)) + } else { + values.Set("pagination_token", paginationToken) + } + + rsp = internal.GetAccounts(api, values) assert.Equal(t, http.StatusOK, rsp.Result().StatusCode) cursor = internal.DecodeCursorResponse[core.Account](t, rsp.Body) - assert.Len(t, cursor.Data, storage.QueryDefaultLimit) + assert.Len(t, cursor.Data, pageSize) assert.Equal(t, cursor.Next != "", cursor.HasMore) // First account of the page @@ -155,34 +182,34 @@ func getPagination(t *testing.T, api *api.API, txsPages, additionalTxs int) func cursor.Data[0].Address) } else { assert.Equal(t, - fmt.Sprintf("accounts:%06d", (txsPages-i)*storage.QueryDefaultLimit+additionalTxs), + fmt.Sprintf("accounts:%06d", (accPages-i)*pageSize+additionalAccs-1), cursor.Data[0].Address) } // Last account of the page assert.Equal(t, - fmt.Sprintf("accounts:%06d", (txsPages-i-1)*storage.QueryDefaultLimit+additionalTxs+1), + fmt.Sprintf("accounts:%06d", (accPages-i-1)*pageSize+additionalAccs), cursor.Data[len(cursor.Data)-1].Address) paginationToken = cursor.Next } - if additionalTxs > 0 { + if additionalAccs > 0 { rsp = internal.GetAccounts(api, url.Values{ "pagination_token": []string{paginationToken}, }) assert.Equal(t, http.StatusOK, rsp.Result().StatusCode, rsp.Body.String()) cursor = internal.DecodeCursorResponse[core.Account](t, rsp.Body) - assert.Len(t, cursor.Data, additionalTxs+1) + assert.Len(t, cursor.Data, additionalAccs) assert.Equal(t, cursor.Next != "", cursor.HasMore) // First account of the last page - if txsPages == 0 { + if accPages == 0 { assert.Equal(t, "world", cursor.Data[0].Address) } else { assert.Equal(t, - fmt.Sprintf("accounts:%06d", additionalTxs), + fmt.Sprintf("accounts:%06d", additionalAccs-1), cursor.Data[0].Address) } @@ -192,17 +219,26 @@ func getPagination(t *testing.T, api *api.API, txsPages, additionalTxs int) func cursor.Data[len(cursor.Data)-1].Address) } + assert.Empty(t, cursor.Next) + // MOVING BACKWARD - if txsPages > 0 { - for i := 0; i < txsPages; i++ { + if accPages > 0 { + back := 0 + for cursor.Previous != "" { paginationToken = cursor.Previous rsp = internal.GetAccounts(api, url.Values{ "pagination_token": []string{paginationToken}, }) assert.Equal(t, http.StatusOK, rsp.Result().StatusCode, rsp.Body.String()) cursor = internal.DecodeCursorResponse[core.Account](t, rsp.Body) - assert.Len(t, cursor.Data, storage.QueryDefaultLimit) + assert.Len(t, cursor.Data, pageSize) assert.Equal(t, cursor.Next != "", cursor.HasMore) + back++ + } + if additionalAccs > 0 { + assert.Equal(t, accPages, back) + } else { + assert.Equal(t, accPages-1, back) } // First account of the first page @@ -211,9 +247,11 @@ func getPagination(t *testing.T, api *api.API, txsPages, additionalTxs int) func // Last account of the first page assert.Equal(t, - fmt.Sprintf("accounts:%06d", (txsPages-1)*storage.QueryDefaultLimit+additionalTxs+1), + fmt.Sprintf("accounts:%06d", (txsPages-1)*pageSize+additionalTxs+1), cursor.Data[len(cursor.Data)-1].Address) } + + assert.Empty(t, cursor.Previous) }) t.Run("balances", func(t *testing.T) { @@ -222,12 +260,16 @@ func getPagination(t *testing.T, api *api.API, txsPages, additionalTxs int) func // MOVING FORWARD for i := 0; i < txsPages; i++ { - rsp = internal.GetBalances(api, url.Values{ - "pagination_token": []string{paginationToken}, - }) + values := url.Values{} + if paginationToken == "" { + values.Set("page_size", fmt.Sprintf("%d", pageSize)) + } else { + values.Set("pagination_token", paginationToken) + } + rsp = internal.GetBalances(api, values) assert.Equal(t, http.StatusOK, rsp.Result().StatusCode) cursor = internal.DecodeCursorResponse[core.AccountsBalances](t, rsp.Body) - assert.Len(t, cursor.Data, storage.QueryDefaultLimit) + assert.Len(t, cursor.Data, pageSize) assert.Equal(t, cursor.Next != "", cursor.HasMore) // First account balances of the page @@ -236,13 +278,13 @@ func getPagination(t *testing.T, api *api.API, txsPages, additionalTxs int) func assert.True(t, ok) } else { _, ok := cursor.Data[0][fmt.Sprintf( - "accounts:%06d", (txsPages-i)*storage.QueryDefaultLimit+additionalTxs)] + "accounts:%06d", (txsPages-i)*pageSize+additionalTxs)] assert.True(t, ok) } // Last account balances of the page _, ok := cursor.Data[len(cursor.Data)-1][fmt.Sprintf( - "accounts:%06d", (txsPages-i-1)*storage.QueryDefaultLimit+additionalTxs+1)] + "accounts:%06d", (txsPages-i-1)*pageSize+additionalTxs+1)] assert.True(t, ok) paginationToken = cursor.Next @@ -275,14 +317,14 @@ func getPagination(t *testing.T, api *api.API, txsPages, additionalTxs int) func // MOVING BACKWARD if txsPages > 0 { - for i := 0; i < txsPages; i++ { + for cursor.Previous != "" { paginationToken = cursor.Previous rsp = internal.GetBalances(api, url.Values{ "pagination_token": []string{paginationToken}, }) assert.Equal(t, http.StatusOK, rsp.Result().StatusCode, rsp.Body.String()) cursor = internal.DecodeCursorResponse[core.AccountsBalances](t, rsp.Body) - assert.Len(t, cursor.Data, storage.QueryDefaultLimit) + assert.Len(t, cursor.Data, pageSize) assert.Equal(t, cursor.Next != "", cursor.HasMore) } @@ -292,7 +334,7 @@ func getPagination(t *testing.T, api *api.API, txsPages, additionalTxs int) func // Last account balances of the first page _, ok = cursor.Data[len(cursor.Data)-1][fmt.Sprintf( - "accounts:%06d", (txsPages-1)*storage.QueryDefaultLimit+additionalTxs+1)] + "accounts:%06d", (txsPages-1)*pageSize+additionalTxs+1)] assert.True(t, ok) } }) diff --git a/pkg/api/controllers/query.go b/pkg/api/controllers/query.go new file mode 100644 index 000000000..84d66abfd --- /dev/null +++ b/pkg/api/controllers/query.go @@ -0,0 +1,38 @@ +package controllers + +import ( + "strconv" + + "github.com/gin-gonic/gin" + "github.com/numary/ledger/pkg/ledger" + "github.com/numary/ledger/pkg/storage" +) + +const ( + MaxPageSize = 1000 + DefaultPageSize = storage.QueryDefaultPageSize +) + +var ( + ErrInvalidPageSize = ledger.NewValidationError("invalid query value 'page_size'") +) + +func getPageSize(c *gin.Context) (uint, error) { + var ( + pageSize uint64 + err error + ) + if pageSizeParam := c.Query("page_size"); pageSizeParam != "" { + pageSize, err = strconv.ParseUint(pageSizeParam, 10, 32) + if err != nil { + return 0, ErrInvalidPageSize + } + + if pageSize > MaxPageSize { + pageSize = MaxPageSize + } + } else { + pageSize = DefaultPageSize + } + return uint(pageSize), nil +} diff --git a/pkg/api/controllers/swagger.yaml b/pkg/api/controllers/swagger.yaml index 8bdd41a63..c8d218933 100644 --- a/pkg/api/controllers/swagger.yaml +++ b/pkg/api/controllers/swagger.yaml @@ -69,6 +69,15 @@ paths: schema: type: string example: ledger001 + - name: page_size + in: query + description: 'The maximum number of results to return per page' + example: 100 + schema: + type: integer + maximum: 1000 + minimum: 1 + default: 15 - name: after in: query description: Pagination cursor, will return accounts after given address, in descending order. @@ -432,6 +441,15 @@ paths: schema: type: string example: ledger001 + - name: page_size + in: query + description: 'The maximum number of results to return per page' + example: 100 + schema: + type: integer + maximum: 1000 + minimum: 1 + default: 15 - name: after in: query description: Pagination cursor, will return transactions after given txid diff --git a/pkg/api/controllers/transaction_controller.go b/pkg/api/controllers/transaction_controller.go index 9d33ef81d..97098478f 100644 --- a/pkg/api/controllers/transaction_controller.go +++ b/pkg/api/controllers/transaction_controller.go @@ -55,7 +55,7 @@ func (ctl *TransactionController) GetTransactions(c *gin.Context) { if c.Query("after") != "" || c.Query("reference") != "" || c.Query("account") != "" || c.Query("source") != "" || c.Query("destination") != "" || c.Query("start_time") != "" || - c.Query("end_time") != "" { + c.Query("end_time") != "" || c.Query("page_size") != "" { ResponseError(c, ledger.NewValidationError( "no other query params can be set with 'pagination_token'")) return @@ -81,8 +81,8 @@ func (ctl *TransactionController) GetTransactions(c *gin.Context) { WithDestinationFilter(token.DestinationFilter). WithStartTimeFilter(token.StartTime). WithEndTimeFilter(token.EndTime). - WithMetadataFilter(token.MetadataFilter) - + WithMetadataFilter(token.MetadataFilter). + WithPageSize(token.PageSize) } else { var afterTxIDParsed uint64 if c.Query("after") != "" { @@ -110,6 +110,12 @@ func (ctl *TransactionController) GetTransactions(c *gin.Context) { } } + pageSize, err := getPageSize(c) + if err != nil { + ResponseError(c, err) + return + } + txQuery = storage.NewTransactionsQuery(). WithAfterTxID(afterTxIDParsed). WithReferenceFilter(c.Query("reference")). @@ -118,7 +124,8 @@ func (ctl *TransactionController) GetTransactions(c *gin.Context) { WithDestinationFilter(c.Query("destination")). WithStartTimeFilter(startTimeParsed). WithEndTimeFilter(endTimeParsed). - WithMetadataFilter(c.QueryMap("metadata")) + WithMetadataFilter(c.QueryMap("metadata")). + WithPageSize(pageSize) } cursor, err = l.(*ledger.Ledger).GetTransactions(c.Request.Context(), *txQuery) diff --git a/pkg/api/controllers/transaction_controller_test.go b/pkg/api/controllers/transaction_controller_test.go index 62ad3db7e..46cde61be 100644 --- a/pkg/api/controllers/transaction_controller_test.go +++ b/pkg/api/controllers/transaction_controller_test.go @@ -606,6 +606,93 @@ func TestGetTransactions(t *testing.T) { })) } +func TestGetTransactionsWithPageSize(t *testing.T) { + internal.RunTest(t, fx.Invoke(func(lc fx.Lifecycle, api *api.API, driver storage.Driver) { + lc.Append(fx.Hook{ + OnStart: func(ctx context.Context) error { + now := time.Now().UTC() + var previousLog *core.Log + logs := make([]core.Log, 0) + store := internal.GetStore(t, driver, context.Background()) + + for i := 0; i < 3*controllers.MaxPageSize; i++ { + tx := core.Transaction{ + ID: uint64(i), + TransactionData: core.TransactionData{ + Postings: core.Postings{ + { + Source: "world", + Destination: fmt.Sprintf("account:%d", i), + Amount: 1000, + Asset: "USD", + }, + }, + }, + Timestamp: now.Format(time.RFC3339), + } + log := core.NewTransactionLog(previousLog, tx) + logs = append(logs, log) + previousLog = &log + } + require.NoError(t, store.AppendLog(context.Background(), logs...)) + + t.Run("invalid page size", func(t *testing.T) { + rsp := internal.GetTransactions(api, url.Values{ + "page_size": []string{"nan"}, + }) + assert.Equal(t, http.StatusBadRequest, rsp.Result().StatusCode, rsp.Body.String()) + + err := sharedapi.ErrorResponse{} + internal.Decode(t, rsp.Body, &err) + assert.EqualValues(t, sharedapi.ErrorResponse{ + ErrorCode: controllers.ErrValidation, + ErrorMessage: controllers.ErrInvalidPageSize.Error(), + }, err) + }) + t.Run("page size over maximum", func(t *testing.T) { + httpResponse := internal.GetTransactions(api, url.Values{ + "page_size": []string{fmt.Sprintf("%d", 2*controllers.MaxPageSize)}, + }) + assert.Equal(t, http.StatusOK, httpResponse.Result().StatusCode, httpResponse.Body.String()) + + cursor := internal.DecodeCursorResponse[core.Transaction](t, httpResponse.Body) + assert.Len(t, cursor.Data, controllers.MaxPageSize) + assert.Equal(t, cursor.PageSize, controllers.MaxPageSize) + assert.NotEmpty(t, cursor.Next) + assert.True(t, cursor.HasMore) + }) + t.Run("with page size greater than max count", func(t *testing.T) { + httpResponse := internal.GetTransactions(api, url.Values{ + "page_size": []string{fmt.Sprintf("%d", controllers.MaxPageSize)}, + "after": []string{fmt.Sprintf("%d", controllers.MaxPageSize-100)}, + }) + assert.Equal(t, http.StatusOK, httpResponse.Result().StatusCode, httpResponse.Body.String()) + + cursor := internal.DecodeCursorResponse[core.Transaction](t, httpResponse.Body) + assert.Len(t, cursor.Data, controllers.MaxPageSize-100) + assert.Equal(t, cursor.PageSize, controllers.MaxPageSize) + assert.Empty(t, cursor.Next) + assert.False(t, cursor.HasMore) + }) + t.Run("with page size lower than max count", func(t *testing.T) { + httpResponse := internal.GetTransactions(api, url.Values{ + "page_size": []string{fmt.Sprintf("%d", controllers.MaxPageSize/10)}, + }) + assert.Equal(t, http.StatusOK, httpResponse.Result().StatusCode, httpResponse.Body.String()) + + cursor := internal.DecodeCursorResponse[core.Transaction](t, httpResponse.Body) + assert.Len(t, cursor.Data, controllers.MaxPageSize/10) + assert.Equal(t, cursor.PageSize, controllers.MaxPageSize/10) + assert.NotEmpty(t, cursor.Next) + assert.True(t, cursor.HasMore) + }) + + return nil + }, + }) + })) +} + type transaction struct { core.Transaction PreCommitVolumes accountsVolumes `json:"preCommitVolumes,omitempty"` diff --git a/pkg/opentelemetry/opentelemetrytraces/storage_test.go b/pkg/opentelemetry/opentelemetrytraces/storage_test.go index b4a083949..995827bcc 100644 --- a/pkg/opentelemetry/opentelemetrytraces/storage_test.go +++ b/pkg/opentelemetry/opentelemetrytraces/storage_test.go @@ -94,7 +94,7 @@ func testAggregateVolumes(t *testing.T, store storage.Store) { func testGetAccounts(t *testing.T, store storage.Store) { _, err := store.GetAccounts(context.Background(), storage.AccountsQuery{ - Limit: 1, + PageSize: 1, }) assert.NoError(t, err) } @@ -106,7 +106,7 @@ func testCountTransactions(t *testing.T, store storage.Store) { func testGetTransactions(t *testing.T, store storage.Store) { _, err := store.GetTransactions(context.Background(), storage.TransactionsQuery{ - Limit: 1, + PageSize: 1, }) assert.NoError(t, err) } diff --git a/pkg/storage/accounts.go b/pkg/storage/accounts.go index 18414645a..b6fbd867e 100644 --- a/pkg/storage/accounts.go +++ b/pkg/storage/accounts.go @@ -1,7 +1,7 @@ package storage type AccountsQuery struct { - Limit uint + PageSize uint Offset uint AfterAddress string Filters AccountsQueryFilters @@ -48,15 +48,14 @@ func NewBalanceOperator(s string) (BalanceOperator, bool) { } func NewAccountsQuery() *AccountsQuery { - return &AccountsQuery{ - Limit: QueryDefaultLimit, + PageSize: QueryDefaultPageSize, } } -func (a *AccountsQuery) WithLimit(limit uint) *AccountsQuery { - if limit != 0 { - a.Limit = limit +func (a *AccountsQuery) WithPageSize(pageSize uint) *AccountsQuery { + if pageSize != 0 { + a.PageSize = pageSize } return a diff --git a/pkg/storage/balances.go b/pkg/storage/balances.go index fd0b17e21..a8e9ac9ee 100644 --- a/pkg/storage/balances.go +++ b/pkg/storage/balances.go @@ -1,7 +1,7 @@ package storage type BalancesQuery struct { - Limit uint + PageSize uint Offset uint AfterAddress string Filters BalancesQueryFilters @@ -13,7 +13,7 @@ type BalancesQueryFilters struct { func NewBalancesQuery() *BalancesQuery { return &BalancesQuery{ - Limit: QueryDefaultLimit, + PageSize: QueryDefaultPageSize, } } @@ -34,3 +34,8 @@ func (b *BalancesQuery) WithAddressFilter(address string) *BalancesQuery { return b } + +func (b *BalancesQuery) WithPageSize(pageSize uint) *BalancesQuery { + b.PageSize = pageSize + return b +} diff --git a/pkg/storage/sqlstorage/accounts.go b/pkg/storage/sqlstorage/accounts.go index 28fad1ced..c7566e2b1 100644 --- a/pkg/storage/sqlstorage/accounts.go +++ b/pkg/storage/sqlstorage/accounts.go @@ -89,7 +89,7 @@ func (s *Store) buildAccountsQuery(p storage.AccountsQuery) (*sqlbuilder.SelectB func (s *Store) getAccounts(ctx context.Context, exec executor, q storage.AccountsQuery) (sharedapi.Cursor[core.Account], error) { accounts := make([]core.Account, 0) - if q.Limit == 0 { + if q.PageSize == 0 { return sharedapi.Cursor[core.Account]{Data: accounts}, nil } @@ -103,8 +103,8 @@ func (s *Store) getAccounts(ctx context.Context, exec executor, q storage.Accoun } // We fetch an additional account to know if there is more - sb.Limit(int(q.Limit + 1)) - t.Limit = q.Limit + sb.Limit(int(q.PageSize + 1)) + t.PageSize = q.PageSize sb.Offset(int(q.Offset)) sqlq, args := sb.BuildWithFlavor(s.schema.Flavor()) @@ -133,8 +133,8 @@ func (s *Store) getAccounts(ctx context.Context, exec executor, q storage.Accoun } var previous, next string - if int(q.Offset)-int(q.Limit) >= 0 { - t.Offset = q.Offset - q.Limit + if int(q.Offset)-int(q.PageSize) >= 0 { + t.Offset = q.Offset - q.PageSize raw, err := json.Marshal(t) if err != nil { return sharedapi.Cursor[core.Account]{}, s.error(err) @@ -142,9 +142,9 @@ func (s *Store) getAccounts(ctx context.Context, exec executor, q storage.Accoun previous = base64.RawURLEncoding.EncodeToString(raw) } - if len(accounts) == int(q.Limit+1) { + if len(accounts) == int(q.PageSize+1) { accounts = accounts[:len(accounts)-1] - t.Offset = q.Offset + q.Limit + t.Offset = q.Offset + q.PageSize raw, err := json.Marshal(t) if err != nil { return sharedapi.Cursor[core.Account]{}, s.error(err) @@ -153,7 +153,7 @@ func (s *Store) getAccounts(ctx context.Context, exec executor, q storage.Accoun } return sharedapi.Cursor[core.Account]{ - PageSize: int(q.Limit), + PageSize: int(q.PageSize), HasMore: next != "", Previous: previous, Next: next, diff --git a/pkg/storage/sqlstorage/accounts_test.go b/pkg/storage/sqlstorage/accounts_test.go index eff3eb7fb..ec17beb20 100644 --- a/pkg/storage/sqlstorage/accounts_test.go +++ b/pkg/storage/sqlstorage/accounts_test.go @@ -30,7 +30,7 @@ func TestAccounts(t *testing.T) { t.Run("success balance", func(t *testing.T) { q := storage.AccountsQuery{ - Limit: 10, + PageSize: 10, Filters: storage.AccountsQueryFilters{ Balance: "50", }, @@ -42,7 +42,7 @@ func TestAccounts(t *testing.T) { t.Run("panic invalid balance", func(t *testing.T) { q := storage.AccountsQuery{ - Limit: 10, + PageSize: 10, Filters: storage.AccountsQueryFilters{ Balance: "TEST", }, @@ -59,7 +59,7 @@ func TestAccounts(t *testing.T) { t.Run("panic invalid balance_operator", func(t *testing.T) { assert.PanicsWithValue(t, "invalid balance_operator parameter", func() { q := storage.AccountsQuery{ - Limit: 10, + PageSize: 10, Filters: storage.AccountsQueryFilters{ Balance: "50", BalanceOperator: "TEST", @@ -72,7 +72,7 @@ func TestAccounts(t *testing.T) { t.Run("success balance_operator", func(t *testing.T) { q := storage.AccountsQuery{ - Limit: 10, + PageSize: 10, Filters: storage.AccountsQueryFilters{ Balance: "50", BalanceOperator: storage.BalanceOperatorGte, diff --git a/pkg/storage/sqlstorage/balances.go b/pkg/storage/sqlstorage/balances.go index ad77fcf9a..cd5dd8791 100644 --- a/pkg/storage/sqlstorage/balances.go +++ b/pkg/storage/sqlstorage/balances.go @@ -100,8 +100,8 @@ func (s *Store) getBalances(ctx context.Context, exec executor, q storage.Balanc t.AddressRegexpFilter = q.Filters.AddressRegexp } - sb.Limit(int(q.Limit + 1)) - t.Limit = q.Limit + sb.Limit(int(q.PageSize + 1)) + t.PageSize = q.PageSize sb.Offset(int(q.Offset)) balanceQuery, args := sb.BuildWithFlavor(s.schema.Flavor()) @@ -151,8 +151,8 @@ func (s *Store) getBalances(ctx context.Context, exec executor, q storage.Balanc } var previous, next string - if int(q.Offset)-int(q.Limit) >= 0 { - t.Offset = q.Offset - q.Limit + if int(q.Offset)-int(q.PageSize) >= 0 { + t.Offset = q.Offset - q.PageSize raw, err := json.Marshal(t) if err != nil { return sharedapi.Cursor[core.AccountsBalances]{}, s.error(err) @@ -160,9 +160,9 @@ func (s *Store) getBalances(ctx context.Context, exec executor, q storage.Balanc previous = base64.RawURLEncoding.EncodeToString(raw) } - if len(accounts) == int(q.Limit+1) { + if len(accounts) == int(q.PageSize+1) { accounts = accounts[:len(accounts)-1] - t.Offset = q.Offset + q.Limit + t.Offset = q.Offset + q.PageSize raw, err := json.Marshal(t) if err != nil { return sharedapi.Cursor[core.AccountsBalances]{}, s.error(err) diff --git a/pkg/storage/sqlstorage/balances_test.go b/pkg/storage/sqlstorage/balances_test.go index ebda70ac9..a15d5714e 100644 --- a/pkg/storage/sqlstorage/balances_test.go +++ b/pkg/storage/sqlstorage/balances_test.go @@ -21,7 +21,7 @@ func testGetBalances(t *testing.T, store *sqlstorage.Store) { t.Run("all accounts", func(t *testing.T) { cursor, err := store.GetBalances(context.Background(), storage.BalancesQuery{ - Limit: 10, + PageSize: 10, }) assert.NoError(t, err) assert.Equal(t, 3, cursor.PageSize) @@ -50,7 +50,7 @@ func testGetBalances(t *testing.T, store *sqlstorage.Store) { t.Run("limit", func(t *testing.T) { cursor, err := store.GetBalances(context.Background(), storage.BalancesQuery{ - Limit: 1, + PageSize: 1, }) assert.NoError(t, err) assert.Equal(t, 1, cursor.PageSize) @@ -69,8 +69,8 @@ func testGetBalances(t *testing.T, store *sqlstorage.Store) { t.Run("limit and offset", func(t *testing.T) { cursor, err := store.GetBalances(context.Background(), storage.BalancesQuery{ - Limit: 1, - Offset: 1, + PageSize: 1, + Offset: 1, }) assert.NoError(t, err) assert.Equal(t, 1, cursor.PageSize) @@ -89,7 +89,7 @@ func testGetBalances(t *testing.T, store *sqlstorage.Store) { t.Run("after", func(t *testing.T) { cursor, err := store.GetBalances(context.Background(), storage.BalancesQuery{ - Limit: 10, + PageSize: 10, AfterAddress: "world", }) assert.NoError(t, err) @@ -114,7 +114,7 @@ func testGetBalances(t *testing.T, store *sqlstorage.Store) { t.Run("after and filter on address", func(t *testing.T) { cursor, err := store.GetBalances(context.Background(), storage.BalancesQuery{ - Limit: 10, + PageSize: 10, AfterAddress: "world", Filters: storage.BalancesQueryFilters{AddressRegexp: "users.+"}, }) @@ -141,7 +141,7 @@ func testGetBalancesAggregated(t *testing.T, store *sqlstorage.Store) { assert.NoError(t, err) q := storage.BalancesQuery{ - Limit: 10, + PageSize: 10, } cursor, err := store.GetBalancesAggregated(context.Background(), q) assert.NoError(t, err) diff --git a/pkg/storage/sqlstorage/migrations_test.go b/pkg/storage/sqlstorage/migrations_test.go index 77b4cb07b..96f3f95d9 100644 --- a/pkg/storage/sqlstorage/migrations_test.go +++ b/pkg/storage/sqlstorage/migrations_test.go @@ -238,7 +238,7 @@ var postMigrate = map[string]func(t *testing.T, store *sqlstorage.Store){ } txs, err := store.GetTransactions(context.Background(), storage.TransactionsQuery{ - Limit: 100, + PageSize: 100, }) if !assert.NoError(t, err) { return diff --git a/pkg/storage/sqlstorage/pagination.go b/pkg/storage/sqlstorage/pagination.go index 6d049210c..554a811af 100644 --- a/pkg/storage/sqlstorage/pagination.go +++ b/pkg/storage/sqlstorage/pagination.go @@ -15,10 +15,11 @@ type TxsPaginationToken struct { StartTime time.Time `json:"start_time,omitempty"` EndTime time.Time `json:"end_time,omitempty"` MetadataFilter map[string]string `json:"metadata,omitempty"` + PageSize uint `json:"page_size,omitempty"` } type AccPaginationToken struct { - Limit uint `json:"limit"` + PageSize uint `json:"page_size"` Offset uint `json:"offset"` AfterAddress string `json:"after,omitempty"` AddressRegexpFilter string `json:"address,omitempty"` @@ -28,7 +29,7 @@ type AccPaginationToken struct { } type BalancesPaginationToken struct { - Limit uint `json:"limit"` + PageSize uint `json:"page_size"` Offset uint `json:"offset"` AfterAddress string `json:"after,omitempty"` AddressRegexpFilter string `json:"address,omitempty"` diff --git a/pkg/storage/sqlstorage/store_bench_test.go b/pkg/storage/sqlstorage/store_bench_test.go index 516c92af5..6d49d9afa 100644 --- a/pkg/storage/sqlstorage/store_bench_test.go +++ b/pkg/storage/sqlstorage/store_bench_test.go @@ -139,7 +139,7 @@ func testBenchmarkGetTransactions(b *testing.B, store *sqlstorage.Store) { b.ResetTimer() for n := 0; n < b.N; n++ { txs, err := store.GetTransactions(context.Background(), storage.TransactionsQuery{ - Limit: 100, + PageSize: 100, }) assert.NoError(b, err) if txs.PageSize != 100 { diff --git a/pkg/storage/sqlstorage/store_test.go b/pkg/storage/sqlstorage/store_test.go index f5ffc1af6..368d81794 100644 --- a/pkg/storage/sqlstorage/store_test.go +++ b/pkg/storage/sqlstorage/store_test.go @@ -249,20 +249,20 @@ func testGetAccounts(t *testing.T, store *sqlstorage.Store) { assert.NoError(t, err) accounts, err := store.GetAccounts(context.Background(), storage.AccountsQuery{ - Limit: 1, + PageSize: 1, }) assert.NoError(t, err) assert.Equal(t, 1, accounts.PageSize) accounts, err = store.GetAccounts(context.Background(), storage.AccountsQuery{ - Limit: 1, + PageSize: 1, AfterAddress: accounts.Data[0].Address, }) assert.NoError(t, err) assert.Equal(t, 1, accounts.PageSize) accounts, err = store.GetAccounts(context.Background(), storage.AccountsQuery{ - Limit: 10, + PageSize: 10, Filters: storage.AccountsQueryFilters{ Address: ".*der.*", }, @@ -272,7 +272,7 @@ func testGetAccounts(t *testing.T, store *sqlstorage.Store) { assert.Equal(t, 10, accounts.PageSize) accounts, err = store.GetAccounts(context.Background(), storage.AccountsQuery{ - Limit: 10, + PageSize: 10, Filters: storage.AccountsQueryFilters{ Metadata: map[string]string{ "foo": "bar", @@ -283,7 +283,7 @@ func testGetAccounts(t *testing.T, store *sqlstorage.Store) { assert.Len(t, accounts.Data, 1) accounts, err = store.GetAccounts(context.Background(), storage.AccountsQuery{ - Limit: 10, + PageSize: 10, Filters: storage.AccountsQueryFilters{ Metadata: map[string]string{ "number": "3", @@ -294,7 +294,7 @@ func testGetAccounts(t *testing.T, store *sqlstorage.Store) { assert.Len(t, accounts.Data, 1) accounts, err = store.GetAccounts(context.Background(), storage.AccountsQuery{ - Limit: 10, + PageSize: 10, Filters: storage.AccountsQueryFilters{ Metadata: map[string]string{ "boolean": "true", @@ -305,7 +305,7 @@ func testGetAccounts(t *testing.T, store *sqlstorage.Store) { assert.Len(t, accounts.Data, 1) accounts, err = store.GetAccounts(context.Background(), storage.AccountsQuery{ - Limit: 10, + PageSize: 10, Filters: storage.AccountsQueryFilters{ Metadata: map[string]string{ "a.super.nested.key": "hello", @@ -362,7 +362,7 @@ func testTransactions(t *testing.T, store *sqlstorage.Store) { t.Run("Get", func(t *testing.T) { cursor, err := store.GetTransactions(context.Background(), storage.TransactionsQuery{ - Limit: 1, + PageSize: 1, }) assert.NoError(t, err) // Should get only the first transaction. @@ -370,7 +370,7 @@ func testTransactions(t *testing.T, store *sqlstorage.Store) { cursor, err = store.GetTransactions(context.Background(), storage.TransactionsQuery{ AfterTxID: cursor.Data[0].ID, - Limit: 1, + PageSize: 1, }) assert.NoError(t, err) // Should get only the second transaction. @@ -381,7 +381,7 @@ func testTransactions(t *testing.T, store *sqlstorage.Store) { Account: "world", Reference: "tx1", }, - Limit: 1, + PageSize: 1, }) assert.NoError(t, err) assert.Equal(t, 1, cursor.PageSize) @@ -392,7 +392,7 @@ func testTransactions(t *testing.T, store *sqlstorage.Store) { Filters: storage.TransactionsQueryFilters{ Source: "central_bank", }, - Limit: 10, + PageSize: 10, }) assert.NoError(t, err) assert.Equal(t, 10, cursor.PageSize) @@ -403,7 +403,7 @@ func testTransactions(t *testing.T, store *sqlstorage.Store) { Filters: storage.TransactionsQueryFilters{ Destination: "users:1", }, - Limit: 10, + PageSize: 10, }) assert.NoError(t, err) assert.Equal(t, 10, cursor.PageSize) @@ -415,7 +415,7 @@ func testTransactions(t *testing.T, store *sqlstorage.Store) { StartTime: now.Add(-2 * time.Hour), EndTime: now.Add(-1 * time.Hour), }, - Limit: 10, + PageSize: 10, }) assert.NoError(t, err) assert.Equal(t, 10, cursor.PageSize) @@ -428,7 +428,7 @@ func testTransactions(t *testing.T, store *sqlstorage.Store) { "priority": "high", }, }, - Limit: 10, + PageSize: 10, }) assert.NoError(t, err) assert.Equal(t, 10, cursor.PageSize) diff --git a/pkg/storage/sqlstorage/transactions.go b/pkg/storage/sqlstorage/transactions.go index e3d569119..7b3c62aa2 100644 --- a/pkg/storage/sqlstorage/transactions.go +++ b/pkg/storage/sqlstorage/transactions.go @@ -74,18 +74,19 @@ func (s *Store) buildTransactionsQuery(p storage.TransactionsQuery) (*sqlbuilder func (s *Store) getTransactions(ctx context.Context, exec executor, q storage.TransactionsQuery) (sharedapi.Cursor[core.Transaction], error) { txs := make([]core.Transaction, 0) - if q.Limit == 0 { + if q.PageSize == 0 { return sharedapi.Cursor[core.Transaction]{Data: txs}, nil } sb, t := s.buildTransactionsQuery(q) - sb.OrderBy("id desc") + sb.OrderBy("id").Desc() if q.AfterTxID > 0 { - sb.Where(sb.L("id", q.AfterTxID)) + sb.Where(sb.LE("id", q.AfterTxID)) } - // We fetch an additional transaction to know if there are more - sb.Limit(int(q.Limit + 1)) + // We fetch additional transactions to know if there are more before and/or after. + sb.Limit(int(q.PageSize + 2)) + t.PageSize = q.PageSize sqlq, args := sb.BuildWithFlavor(s.schema.Flavor()) rows, err := exec.QueryContext(ctx, sqlq, args...) @@ -99,11 +100,7 @@ func (s *Store) getTransactions(ctx context.Context, exec executor, q storage.Tr }(rows) for rows.Next() { - var ( - ref sql.NullString - ts sql.NullString - ) - + var ref, ts sql.NullString tx := core.Transaction{} if err := rows.Scan( &tx.ID, @@ -132,8 +129,11 @@ func (s *Store) getTransactions(ctx context.Context, exec executor, q storage.Tr } var previous, next string - if q.AfterTxID > 0 && len(txs) > 0 { - t.AfterTxID = txs[0].ID + storage.QueryDefaultLimit + 1 + + // Page with transactions before + if q.AfterTxID > 0 && len(txs) > 1 && txs[0].ID == q.AfterTxID { + t.AfterTxID = txs[0].ID + uint64(q.PageSize) + txs = txs[1:] raw, err := json.Marshal(t) if err != nil { return sharedapi.Cursor[core.Transaction]{}, s.error(err) @@ -141,8 +141,9 @@ func (s *Store) getTransactions(ctx context.Context, exec executor, q storage.Tr previous = base64.RawURLEncoding.EncodeToString(raw) } - if len(txs) == int(q.Limit+1) { - txs = txs[:len(txs)-1] + // Page with transactions after + if len(txs) > int(q.PageSize) { + txs = txs[:q.PageSize] t.AfterTxID = txs[len(txs)-1].ID raw, err := json.Marshal(t) if err != nil { @@ -152,7 +153,7 @@ func (s *Store) getTransactions(ctx context.Context, exec executor, q storage.Tr } return sharedapi.Cursor[core.Transaction]{ - PageSize: int(q.Limit), + PageSize: int(q.PageSize), HasMore: next != "", Previous: previous, Next: next, diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go index 93a9e7dc2..b0aaf488c 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -12,7 +12,7 @@ import ( type Code string const ( - QueryDefaultLimit = 15 + QueryDefaultPageSize = 15 ConstraintFailed Code = "CONSTRAINT_FAILED" TooManyClient Code = "TOO_MANY_CLIENT" diff --git a/pkg/storage/transactions.go b/pkg/storage/transactions.go index 025288cfa..56c8329b5 100644 --- a/pkg/storage/transactions.go +++ b/pkg/storage/transactions.go @@ -5,7 +5,7 @@ import ( ) type TransactionsQuery struct { - Limit uint + PageSize uint AfterTxID uint64 Filters TransactionsQueryFilters } @@ -21,15 +21,14 @@ type TransactionsQueryFilters struct { } func NewTransactionsQuery() *TransactionsQuery { - return &TransactionsQuery{ - Limit: QueryDefaultLimit, + PageSize: QueryDefaultPageSize, } } -func (a *TransactionsQuery) WithLimit(limit uint) *TransactionsQuery { - if limit != 0 { - a.Limit = limit +func (a *TransactionsQuery) WithPageSize(pageSize uint) *TransactionsQuery { + if pageSize != 0 { + a.PageSize = pageSize } return a From 53a10148620a575171be80c8ca549fa9cc0e0c48 Mon Sep 17 00:00:00 2001 From: Antoine Gelloz Date: Mon, 11 Jul 2022 12:18:41 +0200 Subject: [PATCH 2/2] fix: pagination (#274) fix: first --- pkg/api/controllers/pagination_test.go | 90 ++++++++++--------- pkg/api/controllers/transaction_controller.go | 5 ++ pkg/storage/sqlstorage/accounts.go | 9 +- pkg/storage/sqlstorage/balances.go | 9 +- 4 files changed, 69 insertions(+), 44 deletions(-) diff --git a/pkg/api/controllers/pagination_test.go b/pkg/api/controllers/pagination_test.go index ad3a8fa80..ffb89c7ca 100644 --- a/pkg/api/controllers/pagination_test.go +++ b/pkg/api/controllers/pagination_test.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "net/http" - "net/http/httptest" "net/url" "testing" @@ -30,7 +29,7 @@ func TestGetPagination(t *testing.T) { t.Run(fmt.Sprintf("%d-pages-%d-additional", txsPages, additionalTxs), func(t *testing.T) { internal.RunTest(t, fx.Invoke(func(lc fx.Lifecycle, api *api.API) { lc.Append(fx.Hook{ - OnStart: getPagination(t, api, txsPages, additionalTxs), + OnStart: testGetPagination(t, api, txsPages, additionalTxs), }) })) }) @@ -38,31 +37,44 @@ func TestGetPagination(t *testing.T) { } } -func getPagination(t *testing.T, api *api.API, txsPages, additionalTxs int) func(ctx context.Context) error { +func testGetPagination(t *testing.T, api *api.API, txsPages, additionalTxs int) func(ctx context.Context) error { return func(ctx context.Context) error { - var rsp *httptest.ResponseRecorder - numTxs := txsPages*pageSize + additionalTxs - for i := 0; i < numTxs; i++ { - rsp = internal.PostTransaction(t, api, core.TransactionData{ - Postings: core.Postings{ - { - Source: "world", - Destination: fmt.Sprintf("accounts:%06d", i), - Amount: 10, - Asset: "USD", + if numTxs > 0 { + txsData := make([]core.TransactionData, numTxs) + for i := 0; i < numTxs; i++ { + txsData[i] = core.TransactionData{ + Postings: core.Postings{ + { + Source: "world", + Destination: fmt.Sprintf("accounts:%06d", i), + Amount: 10, + Asset: "USD", + }, }, - }, - Reference: fmt.Sprintf("ref:%06d", i), - }) + Reference: fmt.Sprintf("ref:%06d", i), + } + } + rsp := internal.PostTransactionBatch(t, api, core.Transactions{Transactions: txsData}) require.Equal(t, http.StatusOK, rsp.Code, rsp.Body.String()) } - t.Run("transactions", func(t *testing.T) { - rsp = internal.CountTransactions(api, url.Values{}) - require.Equal(t, http.StatusOK, rsp.Result().StatusCode) - require.Equal(t, fmt.Sprintf("%d", numTxs), rsp.Header().Get("Count")) + rsp := internal.CountTransactions(api, url.Values{}) + require.Equal(t, http.StatusOK, rsp.Result().StatusCode) + require.Equal(t, fmt.Sprintf("%d", numTxs), rsp.Header().Get("Count")) + + numAcc := 0 + if numTxs > 0 { + numAcc = numTxs + 1 // + world account + } + rsp = internal.CountAccounts(api, url.Values{}) + require.Equal(t, http.StatusOK, rsp.Result().StatusCode) + require.Equal(t, fmt.Sprintf("%d", numAcc), rsp.Header().Get("Count")) + + accPages := numAcc / pageSize + additionalAccs := numAcc % pageSize + t.Run("transactions", func(t *testing.T) { var paginationToken string cursor := &sharedapi.Cursor[core.Transaction]{} @@ -146,17 +158,6 @@ func getPagination(t *testing.T, api *api.API, txsPages, additionalTxs int) func }) t.Run("accounts", func(t *testing.T) { - numAcc := 0 - if numTxs > 0 { - numAcc = numTxs + 1 // + world account - } - rsp = internal.CountAccounts(api, url.Values{}) - require.Equal(t, http.StatusOK, rsp.Result().StatusCode) - require.Equal(t, fmt.Sprintf("%d", numAcc), rsp.Header().Get("Count")) - - accPages := numAcc / pageSize - additionalAccs := numAcc % pageSize - var paginationToken string cursor := &sharedapi.Cursor[core.Account]{} @@ -256,16 +257,18 @@ func getPagination(t *testing.T, api *api.API, txsPages, additionalTxs int) func t.Run("balances", func(t *testing.T) { var paginationToken string - var cursor *sharedapi.Cursor[core.AccountsBalances] + cursor := &sharedapi.Cursor[core.AccountsBalances]{} // MOVING FORWARD - for i := 0; i < txsPages; i++ { + for i := 0; i < accPages; i++ { + values := url.Values{} if paginationToken == "" { values.Set("page_size", fmt.Sprintf("%d", pageSize)) } else { values.Set("pagination_token", paginationToken) } + rsp = internal.GetBalances(api, values) assert.Equal(t, http.StatusOK, rsp.Result().StatusCode) cursor = internal.DecodeCursorResponse[core.AccountsBalances](t, rsp.Body) @@ -278,34 +281,34 @@ func getPagination(t *testing.T, api *api.API, txsPages, additionalTxs int) func assert.True(t, ok) } else { _, ok := cursor.Data[0][fmt.Sprintf( - "accounts:%06d", (txsPages-i)*pageSize+additionalTxs)] + "accounts:%06d", (accPages-i)*pageSize+additionalAccs-1)] assert.True(t, ok) } // Last account balances of the page _, ok := cursor.Data[len(cursor.Data)-1][fmt.Sprintf( - "accounts:%06d", (txsPages-i-1)*pageSize+additionalTxs+1)] + "accounts:%06d", (accPages-i-1)*pageSize+additionalAccs)] assert.True(t, ok) paginationToken = cursor.Next } - if additionalTxs > 0 { + if additionalAccs > 0 { rsp = internal.GetBalances(api, url.Values{ "pagination_token": []string{paginationToken}, }) assert.Equal(t, http.StatusOK, rsp.Result().StatusCode, rsp.Body.String()) cursor = internal.DecodeCursorResponse[core.AccountsBalances](t, rsp.Body) - assert.Len(t, cursor.Data, additionalTxs+1) + assert.Len(t, cursor.Data, additionalAccs) assert.Equal(t, cursor.Next != "", cursor.HasMore) // First account balances of the last page - if txsPages == 0 { + if accPages == 0 { _, ok := cursor.Data[0]["world"] assert.True(t, ok) } else { _, ok := cursor.Data[0][fmt.Sprintf( - "accounts:%06d", additionalTxs)] + "accounts:%06d", additionalAccs-1)] assert.True(t, ok) } @@ -316,7 +319,8 @@ func getPagination(t *testing.T, api *api.API, txsPages, additionalTxs int) func } // MOVING BACKWARD - if txsPages > 0 { + if accPages > 0 { + back := 0 for cursor.Previous != "" { paginationToken = cursor.Previous rsp = internal.GetBalances(api, url.Values{ @@ -326,6 +330,12 @@ func getPagination(t *testing.T, api *api.API, txsPages, additionalTxs int) func cursor = internal.DecodeCursorResponse[core.AccountsBalances](t, rsp.Body) assert.Len(t, cursor.Data, pageSize) assert.Equal(t, cursor.Next != "", cursor.HasMore) + back++ + } + if additionalAccs > 0 { + assert.Equal(t, accPages, back) + } else { + assert.Equal(t, accPages-1, back) } // First account balances of the first page diff --git a/pkg/api/controllers/transaction_controller.go b/pkg/api/controllers/transaction_controller.go index 97098478f..9a3b5d20f 100644 --- a/pkg/api/controllers/transaction_controller.go +++ b/pkg/api/controllers/transaction_controller.go @@ -248,6 +248,11 @@ func (ctl *TransactionController) PostTransactionsBatch(c *gin.Context) { return } + if len(txs.Transactions) == 0 { + ResponseError(c, ledger.NewValidationError("no transaction to insert")) + return + } + res, err := l.(*ledger.Ledger).Commit(c.Request.Context(), txs.Transactions) if err != nil { ResponseError(c, err) diff --git a/pkg/storage/sqlstorage/accounts.go b/pkg/storage/sqlstorage/accounts.go index c7566e2b1..8fc465039 100644 --- a/pkg/storage/sqlstorage/accounts.go +++ b/pkg/storage/sqlstorage/accounts.go @@ -133,8 +133,13 @@ func (s *Store) getAccounts(ctx context.Context, exec executor, q storage.Accoun } var previous, next string - if int(q.Offset)-int(q.PageSize) >= 0 { - t.Offset = q.Offset - q.PageSize + if q.Offset > 0 { + offset := int(q.Offset) - int(q.PageSize) + if offset < 0 { + t.Offset = 0 + } else { + t.Offset = uint(offset) + } raw, err := json.Marshal(t) if err != nil { return sharedapi.Cursor[core.Account]{}, s.error(err) diff --git a/pkg/storage/sqlstorage/balances.go b/pkg/storage/sqlstorage/balances.go index cd5dd8791..682c977ec 100644 --- a/pkg/storage/sqlstorage/balances.go +++ b/pkg/storage/sqlstorage/balances.go @@ -151,8 +151,13 @@ func (s *Store) getBalances(ctx context.Context, exec executor, q storage.Balanc } var previous, next string - if int(q.Offset)-int(q.PageSize) >= 0 { - t.Offset = q.Offset - q.PageSize + if q.Offset > 0 { + offset := int(q.Offset) - int(q.PageSize) + if offset < 0 { + t.Offset = 0 + } else { + t.Offset = uint(offset) + } raw, err := json.Marshal(t) if err != nil { return sharedapi.Cursor[core.AccountsBalances]{}, s.error(err)