feat(api): Add configurable limit on paginated request.

This add the query parameter 'limit' on both GET /transactions and GET /accounts 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).
formancehq · Jun 28, 2022 · 404181c · 404181c
1 parent aa4e4ba
commit 404181c
Show file tree

Hide file tree

Showing 12 changed files with 325 additions and 31 deletions.
diff --git 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("limit") != "" {
 			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).
+			WithLimit(token.Limit)
 
 	} else {
 		balance := c.Query("balance")
@@ -97,12 +99,19 @@ func (ctl *AccountController) GetAccounts(c *gin.Context) {
 			}
 		}
 
+		limit, err := getLimit(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")).
+			WithLimit(limit)
 	}
 
 	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
@@ -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,97 @@ func TestGetAccounts(t *testing.T) {
 	}))
 }
 
+func TestGetAccountsWithLimit(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.MaxLimit; 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 limit", func(t *testing.T) {
+					rsp := internal.GetTransactions(api, url.Values{
+						"limit": []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.ErrInvalidLimit.Error(),
+					}, err)
+				})
+				t.Run("negative limit", func(t *testing.T) {
+					rsp := internal.GetAccounts(api, url.Values{
+						"limit": []string{"-1"},
+					})
+					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.ErrNegativeLimit.Error(),
+					}, err)
+				})
+				t.Run("limit over maximum", func(t *testing.T) {
+					httpResponse := internal.GetAccounts(api, url.Values{
+						"limit": []string{fmt.Sprintf("%d", 2*controllers.MaxLimit)},
+					})
+					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.MaxLimit)
+					assert.Equal(t, cursor.PageSize, controllers.MaxLimit)
+					assert.NotEmpty(t, cursor.Next)
+					assert.True(t, cursor.HasMore)
+				})
+				t.Run("with limit greater than max count", func(t *testing.T) {
+					httpResponse := internal.GetAccounts(api, url.Values{
+						"limit": []string{fmt.Sprintf("%d", controllers.MaxLimit)},
+						"after": []string{fmt.Sprintf("accounts:%06d", controllers.MaxLimit-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.MaxLimit-100)
+					assert.Equal(t, controllers.MaxLimit-100, cursor.PageSize)
+					assert.Empty(t, cursor.Next)
+					assert.False(t, cursor.HasMore)
+				})
+				t.Run("with limit lower than max count", func(t *testing.T) {
+					httpResponse := internal.GetAccounts(api, url.Values{
+						"limit": []string{fmt.Sprintf("%d", controllers.MaxLimit/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.MaxLimit/10)
+					assert.Equal(t, cursor.PageSize, controllers.MaxLimit/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/pagination_test.go b/pkg/api/controllers/pagination_test.go
@@ -12,7 +12,6 @@ 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"
@@ -41,7 +40,9 @@ 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
+		limit := 10
+
+		numTxs := txsPages*limit + additionalTxs
 		for i := 0; i < numTxs; i++ {
 			rsp = internal.PostTransaction(t, api, core.TransactionData{
 				Postings: core.Postings{
@@ -67,21 +68,27 @@ func getPagination(t *testing.T, api *api.API, txsPages, additionalTxs int) func
 
 			// 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("limit", fmt.Sprintf("%d", limit))
+				} 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, limit)
 				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)*limit+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)*limit+additionalTxs), cursor.Data[len(cursor.Data)-1].ID)
 
 				paginationToken = cursor.Next
 			}
@@ -113,17 +120,17 @@ func getPagination(t *testing.T, api *api.API, txsPages, additionalTxs int) func
 					})
 					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, limit)
 					assert.Equal(t, cursor.Next != "", cursor.HasMore)
 				}
 
 				// First txid of the first page
 				assert.Equal(t,
-					uint64(txsPages*storage.QueryDefaultLimit+additionalTxs-1), cursor.Data[0].ID)
+					uint64(txsPages*limit+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)*limit+additionalTxs), cursor.Data[len(cursor.Data)-1].ID)
 			}
 		})
 
@@ -141,12 +148,18 @@ func getPagination(t *testing.T, api *api.API, txsPages, additionalTxs int) func
 
 			// MOVING FORWARD
 			for i := 0; i < txsPages; i++ {
-				rsp = internal.GetAccounts(api, url.Values{
-					"pagination_token": []string{paginationToken},
-				})
+
+				values := url.Values{}
+				if paginationToken == "" {
+					values.Set("limit", fmt.Sprintf("%d", limit))
+				} 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, limit)
 				assert.Equal(t, cursor.Next != "", cursor.HasMore)
 
 				// First account of the page
@@ -155,13 +168,13 @@ 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", (txsPages-i)*limit+additionalTxs),
 						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", (txsPages-i-1)*limit+additionalTxs+1),
 					cursor.Data[len(cursor.Data)-1].Address)
 
 				paginationToken = cursor.Next
@@ -193,15 +206,20 @@ func getPagination(t *testing.T, api *api.API, txsPages, additionalTxs int) func
 			}
 
 			// MOVING BACKWARD
-			if txsPages > 0 {
-				for i := 0; i < txsPages; i++ {
+			pageToRead := txsPages - 1
+			if additionalTxs > 0 {
+				pageToRead++
+			}
+			if pageToRead > 0 { // Only if we have MORE than one page in the result set
+				for i := 0; i < pageToRead; i++ {
 					paginationToken = cursor.Previous
+					require.NotEmpty(t, paginationToken)
 					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, limit)
 					assert.Equal(t, cursor.Next != "", cursor.HasMore)
 				}
 
@@ -211,7 +229,7 @@ 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)*limit+additionalTxs+1),
 					cursor.Data[len(cursor.Data)-1].Address)
 			}
 		})

diff --git a/pkg/api/controllers/query.go b/pkg/api/controllers/query.go
@@ -0,0 +1,45 @@
+package controllers
+
+import (
+	"strconv"
+
+	"github.com/gin-gonic/gin"
+	"github.com/numary/ledger/pkg/ledger"
+	"github.com/numary/ledger/pkg/storage"
+)
+
+const (
+	MaxLimit     = 1000
+	DefaultLimit = storage.QueryDefaultLimit
+)
+
+var (
+	ErrInvalidLimit  = ledger.NewValidationError("invalid query value 'limit'")
+	ErrNegativeLimit = ledger.NewValidationError("cannot pass negative 'limit'")
+)
+
+func getLimit(c *gin.Context) (uint, error) {
+	var (
+		// Use int instead of uint because if the client pass a negative limit
+		// the uint will transparently convert the value to a valid uint
+		// and the error will not be catched
+		limit int64
+		err   error
+	)
+	if limitParam := c.Query("limit"); limitParam != "" {
+		limit, err = strconv.ParseInt(limitParam, 10, 64)
+		if err != nil {
+			return 0, ErrInvalidLimit
+		}
+		if limit < 0 {
+			return 0, ErrNegativeLimit
+		}
+
+		if limit > MaxLimit {
+			limit = MaxLimit
+		}
+	} else {
+		limit = DefaultLimit
+	}
+	return uint(limit), nil
+}
diff --git a/pkg/api/controllers/swagger.yaml b/pkg/api/controllers/swagger.yaml
@@ -69,6 +69,15 @@ paths:
           schema:
             type: string
             example: ledger001
+        - name: limit
+          in: query
+          description: 'Number of results to return. The api will return at most N items.'
+          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.
@@ -423,6 +432,15 @@ paths:
           schema:
             type: string
             example: ledger001
+        - name: limit
+          in: query
+          description: 'Number of results to return. The api will return at most N items.'
+          example: 100
+          schema:
+            type: integer
+            maximum: 1000
+            minimum: 1
+            default: 15
         - name: after
           in: query
           description: Pagination cursor, will return transactions after given txid