vocdoni / interoperability

Private repository to manage the interoperability between protocol, API, SDK, UIs and documentation
1 stars 0 forks source link

feature: improve and unify pagination and filtered pagination #188

Open elboletaire opened 1 month ago

elboletaire commented 1 month ago

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:

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

Important ones:

Not so important, but should also be unified for coherence:

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:

p4u commented 1 month ago

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 commented 1 month ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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

p4u commented 2 weeks ago

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 commented 2 weeks ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

ph01