feat: Add configurable pageSize on paginated requests #245

gfyrag · 2022-06-28T14:10:55Z

Add configurable limit on paginated request

This add the query parameter 'page_size' on both GET /transactions, GET /accounts and GET /balances 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).

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Refactoring / Technical debt

What parts of the code are impacted ?

pkg/api/controllers
pkg/storage

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

codecov · 2022-06-28T14:37:23Z

Codecov Report

Merging #245 (53a1014) into main (a6c1a6d) will increase coverage by 0.32%.
The diff coverage is 84.78%.

@@            Coverage Diff             @@
##             main     #245      +/-   ##
==========================================
+ Coverage   71.19%   71.51%   +0.32%     
==========================================
  Files          78       79       +1     
  Lines        4180     4227      +47     
==========================================
+ Hits         2976     3023      +47     
+ Misses        942      937       -5     
- Partials      262      267       +5

Impacted Files	Coverage Δ
pkg/storage/storage.go	`55.22% <ø> (ø)`
pkg/storage/sqlstorage/accounts.go	`83.73% <66.66%> (-1.98%)`	⬇️
pkg/api/controllers/balance_controller.go	`75.92% <70.00%> (-2.80%)`	⬇️
pkg/storage/sqlstorage/balances.go	`77.67% <70.00%> (-1.96%)`	⬇️
pkg/api/controllers/transaction_controller.go	`92.63% <75.00%> (-1.52%)`	⬇️
pkg/storage/sqlstorage/transactions.go	`85.79% <91.66%> (-0.08%)`	⬇️
pkg/api/controllers/account_controller.go	`88.11% <100.00%> (+0.88%)`	⬆️
pkg/api/controllers/query.go	`100.00% <100.00%> (ø)`
pkg/storage/accounts.go	`100.00% <100.00%> (+11.76%)`	⬆️
pkg/storage/balances.go	`100.00% <100.00%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6c1a6d...53a1014. Read the comment docs.

pkg/api/controllers/query.go

pkg/storage/sqlstorage/pagination.go

pkg/api/controllers/swagger.yaml

antoinegelloz · 2022-07-05T14:20:18Z

We might want to wait for #243 to be merged, and apply the same feature on the new GetBalances route ?

jdupas22 · 2022-07-05T14:16:24Z

pkg/api/controllers/query.go

+		if pageSize > MaxPageSize {
+			pageSize = MaxPageSize
+		}
+	} else {


maybe we can print a log here ?

Why? If we print a log each time the code take a default value, it will give a lot of useless logs.

Also, i don't remind if it complete, i have to check, but all requests should be logged using opentelemetry.

Why? If we log each time a default value is used, it will give a lot of logs ^^

i didn't mean especially logging the default value, but logging that the parameter given was higher than expeced. I think this can be useful to determine how our api (and our parameters) are used by our customers in way we didn't predict ?

Ok, i agree.
Instead of a log, i think an opentelemetry attributes is well suited.

@jdupas22 I checked the instrumentation library used to handle http request (https://pkg.go.dev/go.opentelemetry.io/contrib/instrumentation/github.com/gin-gonic/gin/otelgin) and it register the full path inside an attribute named "http.target".

pkg/storage/sqlstorage/accounts.go

gfyrag · 2022-07-05T20:55:53Z

#243

We might want to wait for #243 to be merged, and apply the same feature on the new GetBalances route ?

Completely agree.

pkg/api/controllers/account_controller_test.go

pkg/storage/sqlstorage/accounts.go

This add the query parameter 'page_size' on both GET /transactions, GET /accounts, and GET /balances 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).

fix: first

* feat(api): Add configurable page_size on paginated request. Co-authored-by: Antoine Gelloz <antoine.gelloz@me.com>

gfyrag added this to the v1.6.0 milestone Jun 28, 2022

gfyrag self-assigned this Jun 28, 2022

gfyrag requested a review from a team as a code owner June 28, 2022 14:10

gfyrag force-pushed the feature/num-567-let-users-get-more-than-15-transactions branch 3 times, most recently from 404181c to 039f00a Compare June 28, 2022 14:32

github-advanced-security bot found potential problems Jun 28, 2022

View reviewed changes

pkg/api/controllers/query.go Fixed Show resolved Hide resolved

altitude mentioned this pull request Jun 30, 2022

Add limit to get transactions endpoint #72

Closed

github-advanced-security bot found potential problems Jul 1, 2022

View reviewed changes

pkg/api/controllers/query.go Fixed Show resolved Hide resolved

altitude changed the title ~~feat: Add configurable limit on paginated request.~~ feat: Add configurable pageSize on paginated requests Jul 1, 2022

altitude reviewed Jul 1, 2022

View reviewed changes

pkg/storage/sqlstorage/pagination.go Outdated Show resolved Hide resolved

altitude reviewed Jul 1, 2022

View reviewed changes

pkg/api/controllers/swagger.yaml Outdated Show resolved Hide resolved

pkg/api/controllers/swagger.yaml Outdated Show resolved Hide resolved

gfyrag force-pushed the feature/num-567-let-users-get-more-than-15-transactions branch 2 times, most recently from 41572ae to d6c66c7 Compare July 5, 2022 10:11

jdupas22 previously approved these changes Jul 5, 2022

View reviewed changes

gfyrag dismissed jdupas22’s stale review via 0d81c79 July 5, 2022 20:54

antoinegelloz suggested changes Jul 6, 2022

View reviewed changes

pkg/api/controllers/account_controller_test.go Outdated Show resolved Hide resolved

pkg/storage/sqlstorage/accounts.go Show resolved Hide resolved

gfyrag force-pushed the feature/num-567-let-users-get-more-than-15-transactions branch from d3ba1fc to 83bc397 Compare July 8, 2022 10:33

gfyrag force-pushed the feature/num-567-let-users-get-more-than-15-transactions branch from 83bc397 to 2bce66b Compare July 8, 2022 13:31

fix: pagination (#274)

53a1014

fix: first

antoinegelloz approved these changes Jul 11, 2022

View reviewed changes

jdupas22 approved these changes Jul 11, 2022

View reviewed changes

altitude approved these changes Jul 12, 2022

View reviewed changes

altitude merged commit 3ca2993 into main Jul 12, 2022

altitude deleted the feature/num-567-let-users-get-more-than-15-transactions branch July 12, 2022 14:39

gfyrag added a commit that referenced this pull request Jul 18, 2022

feat: Add configurable pageSize on paginated requests (#245)

d3b1c6e

* feat(api): Add configurable page_size on paginated request. Co-authored-by: Antoine Gelloz <antoine.gelloz@me.com>

flemzord pushed a commit that referenced this pull request Apr 18, 2023

chore: Ledger: deprecation message for sqlite (#245)

c730c6d

flemzord pushed a commit that referenced this pull request Apr 18, 2023

chore: Ledger: deprecation message for sqlite (#245)

6502b82

flemzord pushed a commit that referenced this pull request Sep 20, 2023

chore: Ledger: deprecation message for sqlite (#245)

38353dd

flemzord pushed a commit that referenced this pull request Sep 25, 2024

chore: Ledger: deprecation message for sqlite (#245)

d5c075a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add configurable pageSize on paginated requests #245

feat: Add configurable pageSize on paginated requests #245

gfyrag commented Jun 28, 2022 •

edited

Loading

codecov bot commented Jun 28, 2022 •

edited

Loading

antoinegelloz commented Jul 5, 2022

jdupas22 Jul 5, 2022

gfyrag Jul 5, 2022 •

edited

Loading

gfyrag Jul 5, 2022

gfyrag Jul 5, 2022

jdupas22 Jul 7, 2022

gfyrag Jul 8, 2022

gfyrag Jul 8, 2022

gfyrag commented Jul 5, 2022

feat: Add configurable pageSize on paginated requests #245

feat: Add configurable pageSize on paginated requests #245

Conversation

gfyrag commented Jun 28, 2022 • edited Loading

Add configurable limit on paginated request

Type of change

What parts of the code are impacted ?

Checklist:

codecov bot commented Jun 28, 2022 • edited Loading

Codecov Report

antoinegelloz commented Jul 5, 2022

jdupas22 Jul 5, 2022

Choose a reason for hiding this comment

gfyrag Jul 5, 2022 • edited Loading

Choose a reason for hiding this comment

gfyrag Jul 5, 2022

Choose a reason for hiding this comment

gfyrag Jul 5, 2022

Choose a reason for hiding this comment

jdupas22 Jul 7, 2022

Choose a reason for hiding this comment

gfyrag Jul 8, 2022

Choose a reason for hiding this comment

gfyrag Jul 8, 2022

Choose a reason for hiding this comment

gfyrag commented Jul 5, 2022

gfyrag commented Jun 28, 2022 •

edited

Loading

codecov bot commented Jun 28, 2022 •

edited

Loading

gfyrag Jul 5, 2022 •

edited

Loading