Skip to content
This repository has been archived by the owner on Nov 2, 2018. It is now read-only.

Pagination for /wallet/addresses and /wallet/transactions #2511

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Pagination for /wallet/addresses and /wallet/transactions #2511

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8245194
Pagination for /wallet/addresses and /wallet/transactions
hudaniel Dec 7, 2017
5db8549
start and limit queries
hudaniel Dec 8, 2017
804e2e5
go fmt
hudaniel Feb 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
96 changes: 96 additions & 0 deletions node/api/api.go
View file
Open in desktop
Expand Up @@ -2,7 +2,10 @@ package api

import (
"encoding/json"
"math"
"net/http"
"reflect"
"strconv"
"strings"

"github.com/NebulousLabs/Sia/build"
Expand All @@ -29,6 +32,25 @@ type Error struct {
// be valid or invalid depending on the current state of a module.
}

type PaginationRequest struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need that struct at all. We can just return multiple return values in the methods that use it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw all exported structs and methods need a comment.

HasPaginationQuery bool
PaginationQueryIsValid bool
Start int
Limit int
}

type PaginationResponse struct {
Start int `json:"start"`
Limit int `json:"limit"`
TotalPages int `json:"total_pages"`
}

type PaginationWrapper struct {
Start int
End int
TotalPages int
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are probably enough new pagination types and methods to move the generic ones into a node/api/pagination.go file instead of having them in api.go.
The ones that are specific to a module can still be in the corresponding [module].go file.

// Error implements the error interface for the Error type. It returns only the
// Message field.
func (err Error) Error() string {
Expand Down Expand Up @@ -162,3 +184,77 @@ func WriteJSON(w http.ResponseWriter, obj interface{}) {
func WriteSuccess(w http.ResponseWriter) {
w.WriteHeader(http.StatusNoContent)
}

func GetPaginationDefaultRequest(req *http.Request) PaginationRequest {
return GetPaginationRequest(req, "start", "limit")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about the custom names for start and limit. If we do that, the names of the fields of PaginationResponse should probably also be custom to match the fields in the request.

}

func GetPaginationRequest(req *http.Request, startQueryParam string, limitQueryParam string) PaginationRequest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove the PaginationRequest struct and simply return the startIndex, limit and an error that indicates a parsing problem. That way the method gets significantly shorter since we don't always construct the PaginationRequest object and we can return build.ExtendErr-extended errors that indicate which parameter couldn't be parsed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you still want to be able to differentiate between a "no pagination params specified" error and a "parsing error" you can declare a custom error for which you can check in the calling method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I also suggest a different name for the method? ParsePaginationParams might be better after getting rid of the PaginationRequest struct.

startString := req.FormValue(startQueryParam)
if startString == "" {
return PaginationRequest{
HasPaginationQuery: false,
PaginationQueryIsValid: false,
Start: 0,
Limit: 0,
}
}
startIndex, err := strconv.Atoi(startString)
if err != nil || startIndex < 0 {
return PaginationRequest{
HasPaginationQuery: true,
PaginationQueryIsValid: false,
Start: 0,
Limit: 0,
}
}
limit := DefaultPaginationSize
limitString := req.FormValue(limitQueryParam)
if limitString != "" {
limit, err = strconv.Atoi(limitString)
if err != nil || limit <= 0 {
return PaginationRequest{
HasPaginationQuery: true,
PaginationQueryIsValid: false,
Start: 0,
Limit: 0,
}
}
}
return PaginationRequest{
HasPaginationQuery: true,
PaginationQueryIsValid: true,
Start: startIndex,
Limit: limit,
}
}

// returns the starting index, end index, and total pages of a given slice for the page
func GetPaginationIndicesAndResponse(sliceInterface interface{}, paginationRequest PaginationRequest) (PaginationWrapper, PaginationResponse) {
if paginationRequest.Limit <= 0 {
build.Critical("pagination request limit must be greater than 0")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

build.Critical should be used as a last resort. In production this will only print to the terminal and will continue the execution of the program. In a debug build this panics.
You should rather return an errors.New("pagination request limit must be greater than 0") and write that error to the http response later.

}
slice := reflect.ValueOf(sliceInterface)
if slice.Kind() != reflect.Slice {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be a bit complicated for just getting the length of arbitrary data. Why not just change sliceInterface interface{} to dataLen int ?

build.Critical("attempting to paginate on non-slice object type: ", slice.Kind())
}
startingPageIndex := int(math.Min(float64(paginationRequest.Start), float64(slice.Len())))
endingPageIndex := int(math.Min(float64(paginationRequest.Start+paginationRequest.Limit), float64(slice.Len())))
totalPages := slice.Len() / paginationRequest.Limit
if math.Mod(float64(slice.Len()), float64(paginationRequest.Limit)) != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably don't need math.Mod with all the float casting. Instead you can do if slice.Len() % paginationRequest.Limit != 0

totalPages += 1
}

pagination := PaginationWrapper{
Start: startingPageIndex,
End: endingPageIndex,
TotalPages: totalPages,
}

paginationResponse := PaginationResponse{
Start: startingPageIndex,
Limit: paginationRequest.Limit,
TotalPages: totalPages,
}
return pagination, paginationResponse
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pagination and paginationResponse have exactly the same fields. Why not just do a
return startPageIndex, endingPageIndex, totalPages? That reduces the length of the method by 50%. You might even want to use named return values instead. That way it is just a return` at the end of the function.

}
59 changes: 52 additions & 7 deletions node/api/wallet.go
View file
Open in desktop
Expand Up @@ -45,6 +45,11 @@ type (
Addresses []types.UnlockHash `json:"addresses"`
}

WalletAddressesPaginatedGET struct {
Addresses []types.UnlockHash `json:"addresses"`
Pagination PaginationResponse `json:"pagination"`
}

// WalletInitPOST contains the primary seed that gets generated during a
// POST call to /wallet/init.
WalletInitPOST struct {
Expand Down Expand Up @@ -90,6 +95,13 @@ type (
UnconfirmedTransactions []modules.ProcessedTransaction `json:"unconfirmedtransactions"`
}

WalletTransactionsPaginatedGET struct {
ConfirmedTransactions []modules.ProcessedTransaction `json:"confirmedtransactions"`
UnconfirmedTransactions []modules.ProcessedTransaction `json:"unconfirmedtransactions"`
ConfirmedTransactionsPagination PaginationResponse `json:"confirmed_transactions_pagination"`
UnconfirmedTransactionsPagination PaginationResponse `json:"unconfirmed_transactions_pagination"`
}

// WalletTransactionsGETaddr contains the set of wallet transactions
// relevant to the input address provided in the call to
// /wallet/transaction/:addr
Expand Down Expand Up @@ -179,9 +191,24 @@ func (api *API) walletAddressHandler(w http.ResponseWriter, req *http.Request, _

// walletAddressHandler handles API calls to /wallet/addresses.
func (api *API) walletAddressesHandler(w http.ResponseWriter, req *http.Request, _ httprouter.Params) {
WriteJSON(w, WalletAddressesGET{
Addresses: api.wallet.AllAddresses(),
})
paginationRequest := GetPaginationDefaultRequest(req)
allAddresses := api.wallet.AllAddresses()
if !paginationRequest.HasPaginationQuery || allAddresses == nil {
WriteJSON(w, WalletAddressesGET{
Addresses: allAddresses,
})
} else {
if !paginationRequest.PaginationQueryIsValid {
WriteError(w, Error{"start and limit queries must be nonnegative integers"}, http.StatusBadRequest)
return
}
pagination, paginationResponse := GetPaginationIndicesAndResponse(allAddresses, paginationRequest)
addresses := allAddresses[pagination.Start:pagination.End]
WriteJSON(w, WalletAddressesPaginatedGET{
Addresses: addresses,
Pagination: paginationResponse,
})
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned above I would change the call to GetPaginationRequest so that you get something like the following:

start, limit, err := GetPaginationDefaultRequest(req)
...
if err == errNoPagination {
  // legacy behavior
  return
}
// new behavior

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can create a custom error like errNoPagination by declaring it at the top of the file. You can find an example in modules/transactionpool/transactionpool.go where we declare errNilCS and errNilGateway at the top.
Of course that also means you have to do a return errNoPagination in GetPaginationRequest if no params were specified.

}

// walletBackupHandler handles API calls to /wallet/backup.
Expand Down Expand Up @@ -521,10 +548,28 @@ func (api *API) walletTransactionsHandler(w http.ResponseWriter, req *http.Reque
}
unconfirmedTxns := api.wallet.UnconfirmedTransactions()

WriteJSON(w, WalletTransactionsGET{
ConfirmedTransactions: confirmedTxns,
UnconfirmedTransactions: unconfirmedTxns,
})
confirmedPaginationRequest := GetPaginationRequest(req, "confirmedTransactionsStart", "confirmedTransactionsLimit")
unconfirmedPaginationRequest := GetPaginationRequest(req, "unconfirmedTransactionsStart", "unconfirmedTransactionsLimit")

if !confirmedPaginationRequest.HasPaginationQuery || !unconfirmedPaginationRequest.HasPaginationQuery || confirmedTxns == nil || unconfirmedTxns == nil {
WriteJSON(w, WalletTransactionsGET{
ConfirmedTransactions: confirmedTxns,
UnconfirmedTransactions: unconfirmedTxns,
})
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We try to avoid nested if statements. You can get rid of the else by returning after the call to WriteJSON. In the API we usually don't do anything after calling WriteJSON or WriteError which means you can usually simplify your code by returning right afterwards.

if !confirmedPaginationRequest.PaginationQueryIsValid || !unconfirmedPaginationRequest.PaginationQueryIsValid {
WriteError(w, Error{"start and limit queries must be nonnegative integers"}, http.StatusBadRequest)
return
}
confirmedPagination, confirmedPaginationResponse := GetPaginationIndicesAndResponse(confirmedTxns, confirmedPaginationRequest)
unconfirmedPagination, unconfirmedPaginationResponse := GetPaginationIndicesAndResponse(unconfirmedTxns, unconfirmedPaginationRequest)
WriteJSON(w, WalletTransactionsPaginatedGET{
ConfirmedTransactions: confirmedTxns[confirmedPagination.Start:confirmedPagination.End],
UnconfirmedTransactions: unconfirmedTxns[unconfirmedPagination.Start:unconfirmedPagination.End],
ConfirmedTransactionsPagination: confirmedPaginationResponse,
UnconfirmedTransactionsPagination: unconfirmedPaginationResponse,
})
}
}

// walletTransactionsAddrHandler handles API calls to
Expand Down
3 changes: 3 additions & 0 deletions types/constants.go
View file
Open in desktop
Expand Up @@ -49,6 +49,9 @@ var (
SiafundCount = NewCurrency64(10000)
SiafundPortion = big.NewRat(39, 1000)
TargetWindow BlockHeight

// size of each page for paginated API responses
DefaultPaginationSize = 50
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually try to keep the constants in a consts.go file within the module it is used in. Since it is only 1 declaration you can probably add defaultPaginationSize to a node/api/pagination.go file together with the code or move it into a node/api/consts.go file.

)

// init checks which build constant is in place and initializes the variables
Expand Down