feature: improve and unify pagination and filtered pagination

[elboletaire]

Describe the feature
Improve and unify all endpoints with pagination.

Motivation
Current pagination implementation has some issues and limitations.

Proposal
To have, preferably via query params, a way to filter all paginated results by indexed fields.

Current filtering implementation has a specific POST endpoint for filtering results... that's one of the things that could be improved by just using GET params on its original endpoint.

For example, list elections filtered should not exist and, instead, list elections should implement the filtering via query params.

Taking that as an example:

• /elections should be the only URL to get the elections filtered and paginated
• Any indexed field should be a possible GET param to filter, i.e. ?finalResults=true&manuallyEnded=true
• limit and page may be reserved words for pagination purposes. order too, but that's a bit more complicated since you need to specify the direction (asc/desc).
• the results should return, among the current processes key, other keys with information like the total results with the applied filter

Example request with pagination and filtering:

https://api-dev.vocdoni.net/v3/elections?page=1&limit=20&finalResults=true

note the v3 in the url... I'm aware these breaking changes may require defining a new version.

Expected response:

{
  "elections": [
    // a maximum of 20 elections with finalResults
    {
      // election body
    },
   "total": 140 // the total number of filtered results
}

Other information like the page or the limit could also be added to this response but, considering is info given by the requester... I don't see it as a requirement. The important one here is total.

Affected routes

Elections

• GET /elections now accepts following query params
  • page: number;
  • limit: number;
  • organizationId: string;
  • electionId: string;
  • withResults: boolean;
  • finalResults: boolean;
  • manuallyEnded: boolean;
  • status: Exclude<AllElectionStatus, ElectionStatus.ONGOING | ElectionStatus.UPCOMING>;

and this new endpoint replaces all the following, marked deprecated:

• GET /elections/page/:page (docs)
• GET /accounts/{organizationID}/elections/page/{page} (docs)
• GET /accounts/{organizationID}/elections/status/{status}/page/{page} (docs)
• POST /elections/filter/page/{page} (docs)

Organizations

• GET /chain/organizations now accepts following query params
  • page: number;
  • limit: number;
  • organizationId: string;

and this new endpoint replaces all the following, marked deprecated:

• GET /chain/organizations/page/:page (docs)
• POST /chain/organizations/filter/page/{page} (docs)

Accounts

• GET /accounts now accepts following query params
  • page: number;
  • limit: number;

and this new endpoint replaces all the following, marked deprecated:

• GET /accounts/page/:page (docs)

Votes

• GET /votes now accepts following query params
  • page: number;
  • limit: number;
  • electionId: string;

and this new endpoint replaces all the following, marked deprecated:

• GET /elections/:electionID/votes/page/:page (docs)
• GET /elections/:electionID/votes/count (docs) since /votes now reports totalItems

Transactions

• GET /chain/transactions now accepts following query params
  • page: number;
  • limit: number;
  • height: number;
  • type: string;

and this new endpoint replaces all the following, marked deprecated:

• GET /chain/transactions/page/:page (docs)
• GET /chain/blocks/:height/transactions/page/:page (docs)

Fees

• GET /chain/fees now accepts following query params
  • page: number;
  • limit: number;
  • reference: string;
  • type: string;
  • accountId: string;

and this new endpoint replaces all the following, marked deprecated:

• GET /chain/fees/page/:page (docs)
• GET /chain/fees/reference/:reference/page/:page (docs)
• GET /chain/fees/type/:type/page/:page (docs)
• GET /accounts/:accountID/fees/page/:page (docs)
• GET /accounts/:accountID/transfers/page/:page (docs)

Transfers

• GET /chain/transfers now accepts following query params
  • page: number;
  • limit: number;
  • accountId: string;
  • accountIdFrom: string;
  • accountIdTo: string;

and this new endpoint replaces the following, marked deprecated:

• GET /accounts/:accountID/transfers/page/:page (docs)

WARNING: exceptionally among all the changes introduced, this endpoint broke backwards compatibility (since it wasn't being used anyway), the json returned changed, now it's a plain list of transfers instead of "received" and "sent".

"Count" endpoints are now redudant

since now all paginated endpoints return a totalItems, the following endpoints become redundant and are marked as deprecated:

• GET /accounts/{organizationId}/elections/count
• GET /accounts/{accountId}/transfers/count
• GET /accounts/count
• GET /chain/organizations/count
• GET /chain/transactions/count
• GET /elections/{electionId}/votes/count

Note: I've added every single call I've seen requiring a :page param (which now should be GET params). I've also set some as "maybe to be merged with" because seems like we have different calls for filtering for different values, and all these could be unified in the same call by simply specifying the indexed field in the URL as a GET param.

Task vocdoni/vocdoni-node#1318 is related since after having that, those new stored metadata fields could/should be filterable via pagination.

Steps or subtasks

Trying to disclose all the major tasks, since maybe they could be done in different steps:

• Return total field in currently filtered endpoint responses.
• Change from POST to GET the filtered endpoints, passing all filters and pagination params via GET params.
• Unify all endpoints. Since the difference between /elections and /elections/filtered now will be based on "if it does or does not receive params", all features should be unified into the main endpoint.

Out of scope for this issue:

• Add new indexed fields to search by, like also via title and description -> support searching elections via title and description #195

[p4u]

It is important to note that @elboletaire is proposing a /v3/ API. So the /v2/ must be kept for backwards compatibility.

In this v3 version, we are using query parameters, which were not part of the v2. So instead of /v2/whatever/page/2
we would do /v3/whatever?page=2

Tbh this is a big change to the API design, we should evaluate if it is worth it (cost of change VS what we are winning).

Another approach would be to keep using the same basis as v2, keeping always backwards compatibility, but allow new query parameters, and keep compatibility with current URL structure

GET /v2/elections/page/1?finalResults=true&titleSearchTerm=vocdoni&limit=20

{
 "elections": [...],
 "remainingPages": 9
}

What do you think @mvdan @altergui @marcvelmer @elboletaire

[elboletaire]

Yes I think that may be a better (and not so breaking) approach.

But still, I'd suggest to try to add some of the functionalities to some of the basic calls though, like with GET /elections, we could still add the full specification I propose to that method, maintaining its current functionality (if no query params are received, fallbacks to its current functionality).

So I'd add the functionality as you just described @p4u (changed total param due to a comment below):

GET /v2/elections/page/1?finalResults=true&titleSearchTerm=vocdoni&limit=20

{
 "elections": [...],
 "total": 120
}

But would also add the new functionality to:

GET /v2/elections?page=1&finalResults=true&titleSearchTerm=vocdoni&limit=20

{
 "elections": [...],
 "total": 120
}

And, as said above, fallback to the previous functionality (non-breaking) when no get params are received.

Following this approach, the current filtered POST method could also be updated by just adding the new fields to the body:

POST /elections/filter/page/:page

// Body
{
    "organizationId": "hexString",
    "electionId": "hexString",
    "withResults": false,
    "status": "READY",
    "limit": 20
}
// Response
{
 "elections": [...],
 "total": 120
}

Also, to avoid making this issue a burden, I'd try to split the tasks between the more interesting and less interesting ones, in terms of which ones we'll be using first. @selankon maybe you can help me here pin-point which ones we need first.

[elboletaire]

No more opinions? Does that mean yall ok with my last message? If so, I'd try to update the issue reflecting everything we've said.

Can we please prioritize the total property in all paginated responses?

Edit: reamainingPages doesn't sound like a good name. Either totalPages telling us the total number of pages, or total with the total number of records, but having a remainingPages is useless.

[altergui]

i'm going to draft a PR with these ideas, to see them in code, and help orient this discussion

[p4u]

After talking to @altergui we decided to go for the GET /v2/elections?page=1&finalResults=true&titleSearchTerm=vocdoni&limit=20 while keeping backwards compatibility with the current endpoint. So no broken client.

[mvdan]

SGTM, happy to review. I don't oppose new APIs or a v2 per se, but I'm also not super pumped about starting from scratch unless we really have to.

[marcvelmer]

Sorry for reopening this but how about something like:

 "pagination": {
   "total_records": 22,
   "current_page": 1,
   "total_pages": 3,
   "next_page": 2,
   "prev_page": null
 }

I think this is much more powerful for integrating with different UIs and shouldn't be a problem for calculating those data in the backend.

[altergui]

Sorry for reopening this but how about something like:

 "pagination": {
   "total_records": 22,
   "current_page": 1,
   "total_pages": 3,
   "next_page": 2,
   "prev_page": null
 }

I think this is much more powerful for integrating with different UIs and shouldn't be a problem for calculating those data in the backend.

LGTM and i agree it would be easy,
just a small nit: in your example the starting page is 1 and currently in the api we consider 0 to be the first page (so specifying param page=0 or specifying no param is equivalent, also for backwards compatibility)
do we really want to switch to 1?
or keep everything in the api "0 indexed" and let the UI do +1 to everything?

[marcvelmer]

Sorry for reopening this but how about something like:

 "pagination": {
   "total_records": 22,
   "current_page": 1,
   "total_pages": 3,
   "next_page": 2,
   "prev_page": null
 }

I think this is much more powerful for integrating with different UIs and shouldn't be a problem for calculating those data in the backend.

LGTM and i agree it would be easy, just a small nit: in your example the starting page is 1 and currently in the api we consider 0 to be the first page (so specifying param page=0 or specifying no param is equivalent, also for backwards compatibility) do we really want to switch to 1? or keep everything in the api "0 indexed" and let the UI do +1 to everything?

I would keep the first page as 0, sure

[elboletaire]

For backwards compatibility it should remain as 0.

But please, for future (new) implementations (version 3 maybe, to not have different pages across endpoints), consider using 1... I think this is the very first time I've seen a pagination starting with page 0 tbh.

[marcvelmer]

[altergui]

so far, here's my progress in vocdoni/vocdoni-node#1341

    * api: add `pagination` field to endpoints:
      * GET /elections
      * GET /accounts
      * GET /chain/organizations
    
    * api: refactor filtered endpoints to unify pagination logic (and add `pagination` field):
      * GET /accounts/{organizationID}/elections/status/{status}/page/{page}
      * GET /accounts/{organizationID}/elections/page/{page}
      * GET /elections/page/{page}
      * POST /elections/filter/page/{page}
      * GET /chain/organizations/page/{page}
      * POST /chain/organizations/filter/page/{page}
      * GET /accounts/page/{page}
    also, marked all of these endpoints as deprecated on swagger docs

i.e. i implemented all of this

• Return total field in currently filtered endpoint responses.
• Change from POST to GET the filtered endpoints, passing all filters and pagination params via GET params.
• Unify all endpoints. Since the difference between /elections and /elections/filtered now will be based on "if it does or does not receive params", all features should be unified into the main endpoint.
• return a pagination struct like @marcvelmer suggested instead of simply total

but not yet for ALL endpoints mentioned, only the "Important ones" from the original post.

i also didn't yet "Add new indexed fields to search by, like also via title and description from vocdoni/vocdoni-node#1318"

[altergui]

btw yesterday we agreed with @elboletaire to limit the scope of a first MVP: just refactor all the current "URL params" and "POST params", allowing them to be passed via "Query params".
and focus on extending this refactor (unifying behaviour) to all endpoints that currently accept page as URL param (listed by OP)

then on a second step (another PR), add NEW filters like finalResults=true&manuallyEnded=true and whatnot.

the only exception would be param limit, which is currently not supported by any endpoint, but would be part of the MVP since it's so fundamental to pagination (actually i already implemented it in the PR)

[altergui]

wishlist of paginated endpoints after this MVP

• GET /chain/blocks (see /chain/blocks : list of blocks. Currently it could be solved making successive calls to /chain/blocks/{height}, but will be better for the client to simply retrieve a list of blocks, as dvote did #33)
• GET /chain/validators

[altergui]

wishlist of new filters (all of these names are tentative, exact param names TBD)

• /elections

  • startDateBefore
  • startDateAfter
  • endDateBefore
  • endDateAfter

• /accounts

  • accountId (substr)

[altergui]

i missed two current endpoints in my merged PR:

• /accounts/{accountId}/fees/page/{page}
• /accounts/{accountId}/transfers/page/{page}

the first one i'll simply mark is as deprecated in favor of /chain/fees?accountId=

the second one will turn into a new endpoint:

• /chain/transfers
  • page
  • limit
  • fromAccountId
  • toAccountId

[altergui]

i missed two current endpoints in my merged PR:

* /accounts/{accountId}/fees/page/{page}

* /accounts/{accountId}/transfers/page/{page}

the first one i'll simply mark is as deprecated in favor of /chain/fees?accountId=

the second one will turn into a new endpoint:

* /chain/transfers
  
  * page
  * limit
  * fromAccountId
  * toAccountId

done in vocdoni/vocdoni-node#1359

[altergui]

/accounts

* accountId (substr)

done in vocdoni/vocdoni-node@03b3863

[altergui]

/elections

* startDateBefore

* startDateAfter

* endDateBefore

* endDateAfter

this last wishlist item is being developed in vocdoni/vocdoni-node#1361

but the original issue is now, finally, finished and merged in main, so i'm closing this.

since the thread of comments is long, i edited the original post with the final result of all the changes, for simpler reference.