feat(api): add configurable page sizes #628
Conversation
WalkthroughThis pull request introduces a comprehensive pagination configuration system across the Formance Ledger API. The changes span multiple files and include adding new flags for default and maximum page sizes, creating a Changes
Sequence DiagramsequenceDiagram
participant Client
participant Router
participant Controller
participant Service
Client->>Router: Request with pagination params
Router->>Controller: Pass pagination config
Controller->>Service: Apply pagination settings
Service-->>Controller: Return paginated results
Controller-->>Client: Respond with paginated data
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (12)
internal/api/v2/controllers_accounts_list.go (3)
11-13: Encapsulate pagination logic with a clear function docstring
This refactored function signature introduces acommon.PaginationConfigparameter. Consider adding a docstring explaining how thepaginationConfigis utilized and what behavior clients should expect, especially around default behavior vs. configured behavior. Documenting these expectations helps improve code clarity.
21-29: Align error messages and status codes
Upon encounteringledgercontroller.ErrInvalidQueryorledgercontroller.ErrMissingFeature, this block returns a 400 (Bad Request). For future-proofing, ensure more advanced or domain-specific errors are also properly mapped to the correct HTTP status code.
32-33: Add instrumentation/logging for troubleshooting
Rendering the final cursor result is crucial for pagination. Adding minimal logs or traces here could help in diagnosing pagination-related issues in production, especially if the ledger is large or page sizes are deeply customized.internal/api/v2/controllers_logs_list.go (2)
12-14: Adopt consistent naming conventions for real-time clarity
Renaming the function parameter frompaginationConfigto something likecfgorpCfgmight shorten line lengths and mirror naming patterns in other files. The concise naming can improve readability when scanning the code.
16-19: Validate column-based pagination for user correctness
Here,getColumnPaginatedQueryis used with a default order (Desc) and a default column ("id"). Consider verifying whether external users might want ascending summaries or alternative columns. Could we allow them to pass an "order=asc" parameter or a different column name to meet their needs?Do you want me to propose a code snippet that implements ascending sorting if requested by the user?
internal/api/module.go (1)
24-24: Explain new pagination configuration in config documentation
The addition ofPagination common.PaginationConfigis straightforward, but be sure to update the relevant docstrings or configuration references so that everyone knows how to set or override the default and max page sizes when using the module.internal/api/v2/controllers_transactions_list.go (3)
12-14: Document the effect of pagination configuration
Like other similar endpoints, this newly introduced higher-order function pattern benefits from a brief comment describing how thepaginationConfiginfluences transaction listing (e.g., max allowed transactions per page).
16-19: Support custom ordering
Currently, pagination can switch columns based on theorderquery parameter. Consider extending this to accept anascordescparameter for even more user control, consistent with typical RESTful design.
38-39: Enhance logging
Rendering the cursor is a key part of the user experience. Consider minimal success logs to track usage patterns or diagnose user retrieval issues (especially helpful for large queries or complex pagination scenarios).internal/api/router.go (1)
129-132: Default pagination settings.Setting
MaxPageSizeandDefaultPageSizeto the bunpaginate constants in the default options ensures out-of-the-box, robust pagination behavior. Consider reusing or referencing the CLI flags to fully unify default settings if they're configurable outside the code.cmd/serve.go (1)
79-83: Handling maxPageSize
Errors are handled properly upon retrieving the flag. Consider providing warnings or help messages when unrealistic page sizes are set.pkg/testserver/server.go (1)
51-52: Consider adding validation or documentation for new pagination fields.
While addingMaxPageSizeandDefaultPageSizeasuint64allows large values, you might want to ensure they make sense in your application’s context. For example, you may add a docstring or inline validation to confirm thatDefaultPageSizedoes not exceedMaxPageSize.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
cmd/serve.go(5 hunks)internal/api/common/pagination.go(1 hunks)internal/api/module.go(3 hunks)internal/api/router.go(4 hunks)internal/api/v2/common.go(3 hunks)internal/api/v2/controllers_accounts_list.go(1 hunks)internal/api/v2/controllers_accounts_list_test.go(10 hunks)internal/api/v2/controllers_ledgers_list.go(1 hunks)internal/api/v2/controllers_ledgers_list_test.go(1 hunks)internal/api/v2/controllers_logs_list.go(1 hunks)internal/api/v2/controllers_logs_list_test.go(6 hunks)internal/api/v2/controllers_transactions_list.go(1 hunks)internal/api/v2/controllers_transactions_list_test.go(13 hunks)internal/api/v2/controllers_volumes.go(1 hunks)internal/api/v2/controllers_volumes_test.go(6 hunks)internal/api/v2/query.go(0 hunks)internal/api/v2/routes.go(6 hunks)pkg/testserver/server.go(2 hunks)test/e2e/api_ledgers_list_test.go(3 hunks)
💤 Files with no reviewable changes (1)
- internal/api/v2/query.go
✅ Files skipped from review due to trivial changes (1)
- internal/api/common/pagination.go
🔇 Additional comments (72)
internal/api/v2/controllers_accounts_list.go (1)
15-19: Ensure robust query parameter validation
While getOffsetPaginatedQuery presumably handles invalid query parameters, consider integrating additional guards or user-friendly error messages if page sizes or offsets exceed configured limits. This helps keep error-handling logic consistent and comprehensive across all paginated endpoints.
internal/api/v2/controllers_logs_list.go (1)
22-31: Consistency in error handling
Switch statements are used for domain-specific errors. Just ensure any newly introduced pagination-related errors (like out-of-bounds page sizes) are also handled here. This keeps all pagination errors consistent in their handling and user feedback.
internal/api/module.go (1)
45-45: Confirm module wiring for pagination
WithPaginationConfiguration(cfg.Pagination) seamlessly integrates pagination into the router, but consider verifying that all relevant endpoints are indeed picking up the final Pagination values (especially if new endpoints are added later).
internal/api/v2/controllers_transactions_list.go (2)
21-24: Add boundary checks for column pagination
Be mindful of unexpected or malicious inputs in the query parameters. Ensuring the chosen pagination column and order are validated against a known set of valid columns, or at least robustly sanitized, will protect from potential injection or unexpected data ordering.
[security]
27-36: Review ledger handling for concurrency
In l.ListTransactions(), ensure ledger concurrency is properly managed. If ledger updates occur concurrently, verifying read locks or concurrency controls within the ledger is recommended. This endpoint usage is correct, but concurrency checks remain an important operational consideration.
test/e2e/api_ledgers_list_test.go (4)
8-8: Import usage is appropriate
Introducing the "github.com/formancehq/go-libs/v2/pointer" import is consistent with using pointer.For(...). This is a neat way to avoid manually handling addressable variables.
27-28: Page size settings look good
Defining both MaxPageSize and DefaultPageSize as 5 makes sense for the test scenario. This ensures the test environment is consistent and predictable, even if callers request larger page sizes.
42-43: Intentional large request size
Setting PageSize to 100 while expecting only 5 items validates the maximum page size constraint. This is a valuable test to confirm that the server respects the upper boundary.
46-46: Expect a 5-element result
Verifying that ledgers.Data has a length of 5 is a direct and effective way to confirm pagination is capped appropriately.
internal/api/v2/controllers_ledgers_list.go (2)
15-15: Signature change enhances pagination flexibility
Accepting paginationConfig as a new parameter is a solid approach to injecting pagination requirements without global variables or tight coupling.
19-22: Dynamic pagination configuration
Invoking bunpaginate.GetPageSize with WithMaxPageSize(...) and WithDefaultPageSize(...) leverages the newly introduced paginationConfig. This ensures a consistent pagination strategy across your routes.
internal/api/v2/controllers_volumes.go (2)
12-12: Refactored to accept pagination config
Changing the function signature to take in paginationConfig common.PaginationConfig helps maintain a unified approach to pagination, aligning this controller with the rest of the API.
14-64: Well-structured request handling
Wrapping the HTTP handling logic in the returned http.HandlerFunc keeps the code organized and clearly associates pagination settings with the request lifecycle. The approach ensures that each incoming request can be processed with consistent pagination parameters.
internal/api/v2/common.go (3)
5-5: Importing the new pagination config
Bringing in common for PaginationConfig aligns with the broader pagination refactoring across the codebase.
65-76: Offset pagination adapted to new config
Properly retrieving and applying MaxPageSize and DefaultPageSize ensures the offset-based pagination respects the new constraints.
88-99: Column pagination adapted to new config
Similarly, adapting getColumnPaginatedQuery to use paginationConfig unifies pagination logic across different query patterns, minimizing the chance of configuration drift.
internal/api/v2/controllers_ledgers_list_test.go (2)
74-74: Adopting a centralized default page size.
Switching to bunpaginate.QueryDefaultPageSize ensures consistency across the codebase. Verify that all references of the old default page size have been updated accordingly.
82-82: Uniform pagination settings.
Same as above—this maintains a single, central source of truth for default pagination across different test scenarios for ledgers.
internal/api/router.go (4)
5-5: Added import for bunpaginate.
This new import is required to configure and observe pagination constraints (max and default page sizes). Good addition.
75-75: Passing pagination configuration to v2 router.
Integrating the router's paginationConfig ensures each endpoint consistently uses the same page size constraints. Double-check if older endpoints must be migrated to the v2 router or if there's a need to replicate this logic to other routers.
96-97: Enhancing router with paginationConfig.
The new paginationConfig common.PaginationConfig field in routerOptions provides centralized control over pagination. This improves configurability and reduces duplication in page size logic.
120-124: Introducing WithPaginationConfiguration.
This function is an excellent way to inject custom pagination settings when constructing the router. It precisely follows the style of other functional options.
internal/api/v2/routes.go (9)
4-4: Added import for bunpaginate.
This import indicates pagination is now managed via the shared library. Makes for more consistent pagination logic across routes.
39-39: listLedgers now receives paginationConfig.
Great improvement to unify the ledger listing's page limits under a central config. Verify that any direct references to old page size constants are replaced.
65-65: listLogs: paginationConfig introduced.
Bringing logs listing under the same pagination scheme fosters uniform behavior. Ensures consistent user experience across routes.
70-70: listAccounts: standardized pagination.
Accounts route also leverages the shared pagination config. Keep an eye on any new endpoints that might need this same approach.
77-77: listTransactions: consolidated pagination.
Transactions listing logic is more robust when it uses the same global defaults and maximums, minimizing domain-specific differences between endpoints.
89-89: readVolumes now uses paginationConfig.
Volumes retrieval is also unified under the shared config. Helps ensure consistency for all entity listings.
102-102: Added paginationConfig field to routerOptions.
Storing pagination settings in the router ensures each endpoint stubs into the same page size constraints. This design pattern fosters maintainability.
131-135: New WithPaginationConfig function.
Analogous to the rest of the functional options. Clean approach to inject pagination logic.
144-147: Defaulting to QueryDefaultPageSize and MaxPageSize.
Consistently referencing bunpaginate values across the default router options. Makes it straightforward to change in one place if needed.
internal/api/v2/controllers_volumes_test.go (6)
44-44: Switched to bunpaginate.QueryDefaultPageSize.
By using QueryDefaultPageSize, the test is aligned with the new default pagination approach. Ensure all references are standardized.
55-55: Maintaining uniform page size.
Same rationale—this test now matches the centralized page size approach. Keeps consistency across the suite.
67-67: Refactoring to remove old default fallback.
This standardization on bunpaginate.QueryDefaultPageSize clarifies overflow logic and parallels with other tests. Good move.
88-88: Use of the new default in pit-based test.
Ensures that PIT queries and other specialized filters share the same default pagination. Solidifies uniform usage.
102-102: Exists metadata filter test updated.
Even advanced query features (like $exists) now integrate with the new standard page size. Great for consistency.
114-114: Balance filter test also adopting the new default.
All functional areas are aligned to the same pagination default. Overall consistency is improved for complex queries.
internal/api/v2/controllers_logs_list_test.go (6)
46-46: Consistent usage of bunpaginate.QueryDefaultPageSize
The switch to bunpaginate.QueryDefaultPageSize is consistent with the new pagination config.
59-59: Ensuring test coverage for pagination
Make sure there is a test scenario that validates the boundary behavior when page size is near or at the minimum (e.g., 1), especially with date-based filters.
73-73: Date filters remain unaffected
The pagination change doesn't conflict with the existing date filters. This is good.
87-93: Cursor decoding logic
This snippet encodes and decodes the cursor using the updated page size. It looks consistent.
125-125: Validating error handling
The usage of bunpaginate.QueryDefaultPageSize in conjunction with forced errors (e.g., ErrInvalidQuery) is correctly tested.
140-140: Test for unexpected errors
Swapping out the default page size with bunpaginate.QueryDefaultPageSize is fine. Consider verifying that the error handling path remains consistent for large requests.
internal/api/v2/controllers_accounts_list_test.go (10)
44-44: Use of bunpaginate.QueryDefaultPageSize
Aligning with the new pagination approach is correct.
57-57: Metadata filter with default pagination
Good coverage ensuring metadata-based queries also respect the default pagination size.
70-70: Refining the address-based query
The snippet correctly applies bunpaginate.QueryDefaultPageSize.
83-88: Empty cursor scenario
The updated pagination constant still covers this corner case well.
115-115: Handling page size over maximum
Switching from DefaultPageSize to MaxPageSize here is correct. Ensure we keep test coverage for large user inputs.
127-127: Balance filter correctness
All filters and the new pagination setting are integrated well.
140-140: $exists filter scenario
The bunpaginate.QueryDefaultPageSize usage maintains consistent pagination for existence-based queries.
161-161: Invalid query from the core
This test ensures that default sizing is used even when the ledger returns an invalid query error.
175-175: Missing feature scenario
No additional logic needed, but testing is consistent with the new pagination defaults.
189-189: Unexpected error
Page size remains at bunpaginate.QueryDefaultPageSize for normal cases; the error path also remains covered.
cmd/serve.go (6)
5-5: New import for pagination config
Importing internal/api/common is correct for accessing the pagination configuration.
47-48: Adding new CLI flags
Introducing DefaultPageSizeFlag and MaxPageSizeFlag as separate options is a good design choice that promotes flexibility.
84-88: Handling defaultPageSize
Retrieval and error handling for the default page size follows the same pattern as maxPageSize. This is consistent.
118-121: Adding pagination config to API module
Populating the PaginationConfig struct with user-provided values is an excellent approach for flexible pagination control.
125-133: Decorating final router
The changes to the fx.In struct order and the new function to assemble the final router remain consistent. No issues.
160-161: Flag defaults
Using default values of 100 for max-page-size and 15 for default-page-size fosters a balanced pagination experience.
internal/api/v2/controllers_transactions_list_test.go (12)
43-43: New page size reference
Switching to bunpaginate.QueryDefaultPageSize is consistent with the updated config.
56-56: Metadata matching with updated pagination
Ensures filter logic is unaffected by the pagination switch.
70-70: Time-based filter
Good coverage ensuring the new default page size merges well with time-based queries.
84-84: End time filter
No issues with the integration between the filter and updated pagination references.
98-98: Account filter
Still consistent with the new pagination approach.
112-112: Handling references
The usage of the updated default is well-structured.
126-126: Destination-based filter
Implementation remains valid with bunpaginate.QueryDefaultPageSize.
140-140: Source-based filter
No problems noted using the updated reference.
179-179: Page size over maximum
Switching to bunpaginate.MaxPageSize is correct to cap excessive requests.
Line range hint 193-203: Using cursor
The code snippet ensures that partially specified pagination logic gets properly replaced with the default or max values.
215-215: Testing the $exists operator
Ensures the new pagination constant doesn’t break advanced filter usage.
229-229: Effective ordering
Switching pagination references remains consistent with the rest of the PR, and the order is correctly handled.
pkg/testserver/server.go (1)
182-184: Guard against potentially invalid pagination values.
If MaxPageSize is set to a very large number, it could degrade performance. Consider adding an upper limit or a sanity check at the command parsing stage.
| if s.configuration.DefaultPageSize != 0 { | ||
| args = append(args, "--"+cmd.DefaultPageSizeFlag, fmt.Sprint(s.configuration.DefaultPageSize)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add safety check for default vs. max page size.
If DefaultPageSize is greater than MaxPageSize, it might lead to confusing user behavior. Consider enforcing an order or logging a warning when DefaultPageSize > MaxPageSize.
Fixes ENG-1576