Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

api: refactor pagination #1341

Merged
merged 3 commits into from
Jul 26, 2024
Merged

api: refactor pagination #1341

merged 3 commits into from
Jul 26, 2024

Conversation

altergui
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented Jun 18, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9562662039

Details

  • 8 of 24 (33.33%) changed or added relevant lines in 2 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.01%) to 61.294%
Changes Missing Coverage Covered Lines Changed/Added Lines %
httprouter/message.go 0 3 0.0%
api/elections.go 8 21 38.1%
Files with Coverage Reduction New Missed Lines %
vochain/indexer/indexer.go 2 68.58%
Totals Coverage Status
Change from base Build 9546446432: -0.01%
Covered Lines: 15920
Relevant Lines: 25973
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 18, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9564087996

Details

  • 34 of 41 (82.93%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 61.43%
Changes Missing Coverage Covered Lines Changed/Added Lines %
api/elections.go 20 27 74.07%
Totals Coverage Status
Change from base Build 9546446432: 0.1%
Covered Lines: 15957
Relevant Lines: 25976
💛 - Coveralls

@altergui altergui force-pushed the feat/api-refactor-pagination branch 3 times, most recently from 3f62e25 to 879602e Compare June 19, 2024 09:09
@coveralls
Copy link

coveralls commented Jun 19, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9579304909

Details

  • 55 of 72 (76.39%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 61.401%
Changes Missing Coverage Covered Lines Changed/Added Lines %
vochain/indexer/process.go 10 14 71.43%
api/accounts.go 1 6 16.67%
api/elections.go 23 31 74.19%
Totals Coverage Status
Change from base Build 9578771142: 0.1%
Covered Lines: 15955
Relevant Lines: 25985
💛 - Coveralls

@altergui altergui force-pushed the feat/api-refactor-pagination branch from 879602e to ca5f160 Compare June 19, 2024 10:03
@coveralls
Copy link

coveralls commented Jun 19, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9580078879

Details

  • 55 of 72 (76.39%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 61.416%
Changes Missing Coverage Covered Lines Changed/Added Lines %
vochain/indexer/process.go 10 14 71.43%
api/accounts.go 1 6 16.67%
api/elections.go 23 31 74.19%
Totals Coverage Status
Change from base Build 9578771142: 0.1%
Covered Lines: 15959
Relevant Lines: 25985
💛 - Coveralls

@altergui altergui force-pushed the feat/api-refactor-pagination branch from ca5f160 to 4881fa1 Compare June 19, 2024 12:15
@altergui altergui requested a review from mvdan June 19, 2024 12:21
@altergui
Copy link
Contributor Author

this PR is far from finished but since the road ahead is just repeating what i did for /elections to the rest of the endpoints, i'd really appreciate some early feedback on the approach taken so far, particularly from @mvdan on the indexer part

@coveralls
Copy link

coveralls commented Jun 19, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9581805081

Details

  • 79 of 100 (79.0%) changed or added relevant lines in 7 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.2%) to 61.439%
Changes Missing Coverage Covered Lines Changed/Added Lines %
vochain/indexer/process.go 10 14 71.43%
api/elections.go 26 33 78.79%
api/accounts.go 21 31 67.74%
Files with Coverage Reduction New Missed Lines %
api/elections.go 1 58.63%
Totals Coverage Status
Change from base Build 9578771142: 0.2%
Covered Lines: 15976
Relevant Lines: 26003
💛 - Coveralls

@altergui altergui force-pushed the feat/api-refactor-pagination branch 5 times, most recently from 5ef60c1 to 7c1ccc6 Compare June 20, 2024 22:26
@coveralls
Copy link

coveralls commented Jun 20, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9605211222

Details

  • 152 of 187 (81.28%) changed or added relevant lines in 9 files are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.3%) to 61.644%
Changes Missing Coverage Covered Lines Changed/Added Lines %
api/helpers.go 16 18 88.89%
vochain/indexer/indexer.go 11 13 84.62%
vochain/indexer/process.go 11 14 78.57%
api/accounts.go 33 39 84.62%
api/elections.go 51 73 69.86%
Files with Coverage Reduction New Missed Lines %
api/elections.go 1 62.74%
vochain/indexer/indexer.go 2 68.04%
Totals Coverage Status
Change from base Build 9595822141: 0.3%
Covered Lines: 16107
Relevant Lines: 26129
💛 - Coveralls

@altergui
Copy link
Contributor Author

altergui commented Jul 2, 2024 •
edited
Loading

so far:

these endpoints are deprecated

  • GET /accounts/{organizationID}/elections/status/{status}/page/{page}
  • GET /accounts/{organizationID}/elections/page/{page}
  • GET /elections/page/{page}
  • POST /elections/filter/page/{page}

in favor of:

  • GET /elections?page={page}&organizationID={organizationID}&status={status}&electionID={electionID}&withResults={withResults}

(i added, for completeness, a new endpoint POST /elections/filter that accepts those same params on the request body, since it was trivial to implement, but not sure if it's worth to add this possibility as well)

and these endpoints are deprecated

  • GET /chain/organizations/page/{page}
  • POST /chain/organizations/filter/page/{page}

in favor of:

  • GET /chain/organizations?page={page}&organizationID={organizationID}

and as a bonus, this endpoint is also deprecated

  • GET /accounts/page/{page}

in favor of:

  • GET /accounts?page={page}

@altergui altergui force-pushed the feat/api-refactor-pagination branch 2 times, most recently from 0420689 to 5be4415 Compare July 2, 2024 19:34
@altergui altergui marked this pull request as ready for review July 2, 2024 19:42
@altergui altergui force-pushed the feat/api-refactor-pagination branch 2 times, most recently from 3bb7202 to fc15db1 Compare July 2, 2024 19:52
@altergui altergui requested review from p4u and selankon July 2, 2024 19:52
@coveralls
Copy link

coveralls commented Jul 2, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9767055171

Details

  • 223 of 376 (59.31%) changed or added relevant lines in 10 files are covered.
  • 13 unchanged lines in 5 files lost coverage.
  • Overall coverage increased (+0.4%) to 61.665%
Changes Missing Coverage Covered Lines Changed/Added Lines %
vochain/indexer/indexer.go 11 13 84.62%
vochain/indexer/process.go 23 28 82.14%
api/helpers.go 30 39 76.92%
api/accounts.go 42 68 61.76%
api/elections.go 76 118 64.41%
api/chain.go 9 78 11.54%
Files with Coverage Reduction New Missed Lines %
api/chain.go 2 36.35%
api/elections.go 2 64.31%
vochain/state/account.go 2 67.08%
vochain/transaction/election_tx.go 2 62.13%
api/accounts.go 5 61.33%
Totals Coverage Status
Change from base Build 9698468599: 0.4%
Covered Lines: 16102
Relevant Lines: 26112
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 2, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9767120622

Details

  • 223 of 376 (59.31%) changed or added relevant lines in 10 files are covered.
  • 13 unchanged lines in 5 files lost coverage.
  • Overall coverage increased (+0.4%) to 61.665%
Changes Missing Coverage Covered Lines Changed/Added Lines %
vochain/indexer/indexer.go 11 13 84.62%
vochain/indexer/process.go 23 28 82.14%
api/helpers.go 30 39 76.92%
api/accounts.go 42 68 61.76%
api/elections.go 76 118 64.41%
api/chain.go 9 78 11.54%
Files with Coverage Reduction New Missed Lines %
api/chain.go 2 36.35%
api/elections.go 2 64.31%
vochain/state/account.go 2 67.08%
vochain/transaction/election_tx.go 2 62.13%
api/accounts.go 5 61.33%
Totals Coverage Status
Change from base Build 9698468599: 0.4%
Covered Lines: 16102
Relevant Lines: 26112
💛 - Coveralls

@altergui altergui force-pushed the feat/api-refactor-pagination branch from fc15db1 to d3fae84 Compare July 8, 2024 12:46
@coveralls
Copy link

coveralls commented Jul 8, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 10110722018

Details

  • 484 of 934 (51.82%) changed or added relevant lines in 18 files are covered.
  • 13 unchanged lines in 5 files lost coverage.
  • Overall coverage increased (+0.8%) to 61.994%
Changes Missing Coverage Covered Lines Changed/Added Lines %
api/censuses.go 25 27 92.59%
vochain/indexer/vote.go 15 20 75.0%
vochain/indexer/transaction.go 20 27 74.07%
vochain/indexer/db/transactions.sql.go 24 32 75.0%
vochain/indexer/db/token_fees.sql.go 0 11 0.0%
vochain/indexer/db/db.go 4 16 25.0%
vochain/indexer/process.go 46 58 79.31%
api/helpers.go 83 103 80.58%
vochain/indexer/indexer.go 15 47 31.91%
api/accounts.go 67 110 60.91%
Files with Coverage Reduction New Missed Lines %
vochain/indexer/transaction.go 1 64.38%
api/helpers.go 2 72.95%
vochain/indexer/process.go 2 78.43%
api/chain.go 3 35.78%
api/accounts.go 5 63.59%
Totals Coverage Status
Change from base Build 10005893941: 0.8%
Covered Lines: 16253
Relevant Lines: 26217
💛 - Coveralls

@altergui altergui force-pushed the feat/api-refactor-pagination branch 9 times, most recently from cdc086b to 88d4bef Compare July 19, 2024 15:00
@altergui altergui force-pushed the feat/api-refactor-pagination branch 8 times, most recently from 1814fea to a46a92c Compare July 24, 2024 11:56
@altergui altergui force-pushed the feat/api-refactor-pagination branch 4 times, most recently from a3a3867 to c0b932e Compare July 26, 2024 10:40
@altergui altergui force-pushed the feat/api-refactor-pagination branch 2 times, most recently from 64c39dc to be916e8 Compare July 26, 2024 10:50
@altergui
api: implement pagination and query params
b510ac8 
* add new endpoints, all of them include a `pagination` field in reply, and accept QueryParams:
  * GET /elections
    * page
    * limit
    * status
    * organizationId
    * electionId
    * withResults
    * finalResults
    * manuallyEnded

  * GET /accounts
    * page
    * limit

  * GET /chain/transactions
    * page
    * limit
    * height
    * type

  * GET /chain/organizations
    * page
    * limit
    * organizationId

  * GET /chain/fees
    * page
    * limit
    * reference
    * type
    * accountId

  * GET /votes
    * page
    * limit
    * electionId

* mark all of these endpoints as deprecated on swagger docs:
  * GET /accounts/page/{page}
  * 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 /elections/{electionId}/votes/page/{page}
all of these were anyway refactored in a backwards-compatible manner,
to unify pagination logic (adding `pagination` field)

* api: return ErrPageNotFound on all paginated endpoints, when page is negative or higher than last_page
* api: new param `limit` in all paginated endpoints
  (defaults to DefaultItemsPerPage if ommitted, and can't surpass hardcoded MaxItemsPerPage)
* api: all paginated endpoints return an empty list in case of no results (instead of 404)

api internal code refactorings:
* api: unify hardcoded structs into a new types:
  * AccountsList
  * ElectionsList
  * OrganizationsList
  * FeesList
  * VotesList
  * TransactionsList
  * CountResult

* api: deduplicate several code snippets, with marshalAndSend and parse* helpers
* api: rename const MaxPageSize -> MaxItemsPerPage
* api: add Param* consts, and fix case of all URLParams 'ID' -> 'Id'
* api: add *Params types (PaginationParams, ElectionParams, etc)

* api: fix strings in errors returned to client, replacing "ID" -> "Id"
* api: fix swagger docs, replace many occurences of "ID" -> "Id"
* api: fix swagger docs, lots of small inaccuracies

* test: add TestAPIAccountsList and TestAPIElectionsList

indexer changes:
* rename GetListAccounts -> AccountList
* rename GetEnvelopes -> VoteList
* replace GetTokenFees* methods with a single TokenFeesList
* methods AccountList, ProcessList, EntityList, VoteList, TokenFeesList now:
  * return a TotalCount
  * reordered and renamed args (from, max) -> (limit, offset)
@altergui altergui force-pushed the feat/api-refactor-pagination branch from be916e8 to b510ac8 Compare July 26, 2024 11:47
@altergui altergui merged commit b510ac8 into main Jul 26, 2024
16 checks passed
@altergui altergui deleted the feat/api-refactor-pagination branch July 26, 2024 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@p4u p4u p4u approved these changes

@mvdan mvdan Awaiting requested review from mvdan

@selankon selankon Awaiting requested review from selankon

Assignees

@altergui altergui

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@altergui @coveralls @p4u