refactor: pagination system #978
Conversation
gfyrag
commented
Jun 19, 2025
gfyrag
commented
- chore: move code
- refacto: pagination system
- feat: add sane default for each resource handler
|
""" WalkthroughThis change unifies and generalizes the pagination query types used throughout the codebase. It replaces specific types like Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTP Handler
participant PaginationUtil
participant Controller
participant Store
Client->>HTTP Handler: HTTP request with pagination params/cursor
HTTP Handler->>PaginationUtil: getPaginatedQuery(request, ...)
PaginationUtil-->>HTTP Handler: PaginatedQuery
HTTP Handler->>Controller: ListX(ctx, PaginatedQuery)
Controller->>Store: Paginate(ctx, PaginatedQuery)
Store-->>Controller: Cursor[Resource]
Controller-->>HTTP Handler: Cursor[Resource]
HTTP Handler-->>Client: Paginated response with cursor
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
internal/api/v2/controllers_volumes.go (1)
53-77: Pagination query construction is more explicit but adds complexity.The new
getPaginatedQueryapproach with modifier functions provides better configurability and explicit default ordering. However, this increases the code complexity compared to the previous approach. Consider whether this added complexity is justified by the improved flexibility.The explicit configuration is more maintainable, but you might consider extracting the modifier function to improve readability:
+func configureVolumesQuery(groupBy int, pit, oot *time.Time, useInsertionDate bool) func(*storagecommon.ResourceQuery[ledgercontroller.GetVolumesOptions]) { + return func(rq *storagecommon.ResourceQuery[ledgercontroller.GetVolumesOptions]) { + if groupBy > 0 { + rq.Opts.GroupLvl = groupBy + } + if pit != nil { + rq.PIT = pit + } + if oot != nil { + rq.OOT = oot + } + rq.Opts.UseInsertionDate = useInsertionDate + } +} rq, err := getPaginatedQuery[ledgercontroller.GetVolumesOptions]( r, paginationConfig, "account", bunpaginate.OrderAsc, - func(rq *storagecommon.ResourceQuery[ledgercontroller.GetVolumesOptions]) { - if groupBy > 0 { - rq.Opts.GroupLvl = groupBy - } - if pit != nil { - rq.PIT = pit - } - if oot != nil { - rq.OOT = oot - } - rq.Opts.UseInsertionDate = api.QueryParamBool(r, "insertionDate") - }, + configureVolumesQuery(groupBy, pit, oot, api.QueryParamBool(r, "insertionDate")), )internal/storage/common/cursor.go (1)
30-69: Consider improving type safety in cursor unmarshalingThe current approach uses reflection and type assertions which could be made more type-safe. Additionally, the comment on line 36 suggests this is a temporary solution.
Consider implementing a more robust cursor type detection mechanism that doesn't rely on checking for the presence of specific fields. This could involve:
- Adding a cursor type field to the serialized cursor
- Using a discriminated union pattern
- Implementing separate cursor types with their own unmarshal methods
internal/storage/common/resource.go (1)
310-314: Remove redundant validation.The pagination column validation is performed twice - once during the normalization of
InitialPaginatedQuery(lines 283-286) and again here. Since the field was already validated and retrieved above, this second validation is unnecessary.case ColumnPaginatedQuery[OptionsType]: fieldName, field := r.resourceHandler.Schema().GetFieldByNameOrAlias(v.Column) - if field == nil { - return nil, fmt.Errorf("invalid property '%s' for pagination", v.Column) - } paginator = newColumnPaginator[ResourceType, OptionsType](v, fieldName, field.Type)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.modis excluded by!**/*.mod
📒 Files selected for processing (60)
internal/api/bulking/mocks_ledger_controller_test.go(4 hunks)internal/api/common/mocks_ledger_controller_test.go(4 hunks)internal/api/common/mocks_system_controller_test.go(1 hunks)internal/api/v1/controllers_accounts_list.go(2 hunks)internal/api/v1/controllers_accounts_list_test.go(8 hunks)internal/api/v1/controllers_balances_list.go(2 hunks)internal/api/v1/controllers_config.go(1 hunks)internal/api/v1/controllers_logs_list.go(2 hunks)internal/api/v1/controllers_logs_list_test.go(5 hunks)internal/api/v1/controllers_transactions_list.go(2 hunks)internal/api/v1/controllers_transactions_list_test.go(11 hunks)internal/api/v1/mocks_ledger_controller_test.go(4 hunks)internal/api/v1/mocks_system_controller_test.go(1 hunks)internal/api/v1/utils.go(1 hunks)internal/api/v2/common.go(1 hunks)internal/api/v2/controllers_accounts_list.go(1 hunks)internal/api/v2/controllers_accounts_list_test.go(7 hunks)internal/api/v2/controllers_ledgers_list.go(2 hunks)internal/api/v2/controllers_ledgers_list_test.go(4 hunks)internal/api/v2/controllers_logs_list.go(1 hunks)internal/api/v2/controllers_logs_list_test.go(7 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(2 hunks)internal/api/v2/controllers_volumes_test.go(5 hunks)internal/api/v2/mocks_ledger_controller_test.go(4 hunks)internal/api/v2/mocks_system_controller_test.go(1 hunks)internal/controller/ledger/controller.go(1 hunks)internal/controller/ledger/controller_default.go(4 hunks)internal/controller/ledger/controller_default_test.go(8 hunks)internal/controller/ledger/controller_generated_test.go(4 hunks)internal/controller/ledger/controller_with_traces.go(4 hunks)internal/controller/ledger/mocks_test.go(4 hunks)internal/controller/ledger/stats_test.go(1 hunks)internal/controller/ledger/store.go(1 hunks)internal/controller/ledger/store_generated_test.go(4 hunks)internal/controller/system/controller.go(2 hunks)internal/controller/system/store.go(1 hunks)internal/storage/bucket/migrations_test.go(1 hunks)internal/storage/common/cursor.go(1 hunks)internal/storage/common/pagination.go(1 hunks)internal/storage/common/paginator.go(1 hunks)internal/storage/common/paginator_column.go(6 hunks)internal/storage/common/paginator_offset.go(2 hunks)internal/storage/common/resource.go(6 hunks)internal/storage/common/schema.go(1 hunks)internal/storage/driver/driver.go(1 hunks)internal/storage/driver/driver_test.go(2 hunks)internal/storage/driver/system_generated_test.go(1 hunks)internal/storage/ledger/accounts.go(1 hunks)internal/storage/ledger/accounts_test.go(15 hunks)internal/storage/ledger/logs_test.go(2 hunks)internal/storage/ledger/store.go(2 hunks)internal/storage/ledger/transactions_test.go(13 hunks)internal/storage/ledger/volumes_test.go(29 hunks)internal/storage/system/resource_ledgers.go(1 hunks)internal/storage/system/store.go(2 hunks)internal/storage/system/store_test.go(2 hunks)internal/worker/async_block.go(2 hunks)test/e2e/api_logs_list_test.go(1 hunks)
🔇 Additional comments (92)
internal/storage/ledger/accounts.go (1)
140-140: LGTM - Minor formatting cleanup.Removing the trailing newline is a good housekeeping change.
test/e2e/api_logs_list_test.go (1)
260-260: Good fix - Corrected misleading test description.The test description now accurately reflects that it's testing log listing, not account listing.
internal/api/v2/controllers_logs_list.go (2)
17-17: LGTM - Unified pagination query construction.The change from
getColumnPaginatedQuerytogetPaginatedQuery[any]aligns with the pagination system refactor to use a genericPaginatedQuerytype.
23-23: LGTM - Consistent with new pagination interface.Passing
rqdirectly instead of dereferencing*rqis consistent with the updated pagination interface that works with values rather than pointers.internal/api/v2/controllers_accounts_list.go (2)
18-18: LGTM - Consistent pagination unification.The change from
getOffsetPaginatedQuerytogetPaginatedQuery[any]with explicit ordering parameters ("address", bunpaginate.OrderAsc) maintains the same functionality while using the unified pagination interface.
24-24: LGTM - Updated method signature.Passing
querydirectly aligns with the updatedListAccountsmethod signature that now acceptsPaginatedQueryby value.internal/storage/system/resource_ledgers.go (1)
26-26: LGTM - Added ID field for pagination support.Adding the numeric "id" field to the schema provides a consistent field for ordering and cursor-based pagination, supporting the unified pagination system.
internal/api/v2/controllers_ledgers_list.go (1)
19-19: Pagination refactoring looks good.The changes from
getColumnPaginatedQuerytogetPaginatedQueryand passing the query object by value instead of dereferencing align well with the unified pagination approach described in the PR.Also applies to: 26-26
internal/storage/ledger/volumes_test.go (1)
109-113: Pagination type updates are consistent with the refactoring.All instances of
common.OffsetPaginatedQueryhave been correctly replaced withcommon.InitialPaginatedQuery, maintaining the same test functionality while using the new unified pagination approach.Also applies to: 138-143, 168-174, 181-181, 188-195, 212-219, 226-233, 240-247, 264-268, 285-289, 296-300, 307-311, 328-336, 353-361, 378-383, 400-405, 424-431, 450-455, 464-469, 478-483, 549-556, 563-571, 578-586, 593-601, 609-618, 644-653, 669-679, 714-724, 734-745, 755-763
internal/api/v2/controllers_transactions_list.go (1)
28-28: Pagination refactoring implemented correctly.The migration from
getColumnPaginatedQuerytogetPaginatedQueryand the change from dereferencing the query pointer to passing by value are consistent with the unified pagination approach.Also applies to: 34-34
internal/controller/ledger/stats_test.go (1)
22-23: Mock constructor updates align with pagination refactoring.The removal of the third generic parameter from
NewMockPaginatedResourceconstructors is consistent with the unification of pagination query types described in the PR.internal/storage/system/store_test.go (2)
14-14: Import alias added for consistency.The
storagecommonimport alias helps distinguish between different common packages and aligns with the refactoring.
94-96: Pagination query construction updated correctly.The replacement of
ledgercontroller.NewListLedgersQuerywithstoragecommon.InitialPaginatedQuery[any]and the update to usestoragecommon.ColumnPaginatedQuery[any]are consistent with the unified pagination approach.Also applies to: 102-102
internal/storage/bucket/migrations_test.go (1)
93-103: LGTM! Pagination refactoring implemented correctly.The changes successfully migrate from the old pagination system to the unified approach:
- Replaced
bunpaginate.Iteratewithcommon.Iterate- Updated query type from
ColumnPaginatedQuerytoInitialPaginatedQuery- Simplified function call by passing
store.Logs().PaginatedirectlyThe functionality is preserved while using the new unified pagination API.
internal/storage/driver/driver_test.go (2)
12-12: Import updated correctly for pagination refactoring.The import change from
ledgercontrollertostoragecommonaligns with the unified pagination query types.
105-109: Query construction improved with explicit initialization.The new approach replaces the constructor function with direct struct initialization, making the query building more explicit and transparent. The embedded
ResourceQuerywith the builder filter maintains the same filtering logic.internal/api/v1/controllers_transactions_list.go (3)
4-4: Import added for unified pagination types.The
storagecommonimport is necessary for the new generic pagination query types.
15-18: Pagination query construction unified and improved.The change from
getColumnPaginatedQueryto the genericgetPaginatedQuerywith a configuration callback is cleaner and more flexible. The callback approach allows setting both the expand field and query builder in a single configuration step.
24-24: Method call updated for new pagination API.Passing the query by value instead of dereferencing aligns with the new method signature expectations.
internal/storage/driver/system_generated_test.go (1)
137-141: Generated mock updated correctly for simplified pagination interface.The method signature simplification removes the redundant third generic parameter, and the type assertion is updated accordingly. As generated code, these changes correctly reflect the interface changes.
internal/storage/driver/driver.go (1)
183-185: Method signature updated for unified pagination interface.The change from
common.ColumnPaginatedQuery[any]tocommon.PaginatedQuery[any]aligns with the pagination system refactoring. The method body remains unchanged, maintaining functionality while using the more general pagination interface.internal/storage/ledger/transactions_test.go (2)
455-460: LGTM: Consistent pagination query type updates.The change from
ColumnPaginatedQuerytoInitialPaginatedQueryaligns with the broader pagination system refactoring. The test logic remains intact while adopting the unified pagination query interface.
743-750: LGTM: Test case structure updated consistently.The test case struct field and initialization consistently use the new
InitialPaginatedQuerytype, maintaining the same test behavior while conforming to the unified pagination interface.internal/api/v1/controllers_logs_list.go (2)
4-4: LGTM: Added import for unified pagination types.The import of
storagecommonsupports the new unified pagination query approach.
40-48: LGTM: Improved pagination query construction.The refactoring consolidates pagination query setup into a single
getPaginatedQuerycall with a callback, replacing the previous two-step process. This is more elegant and aligns with the unified pagination system while maintaining the same functionality.internal/api/v1/controllers_balances_list.go (2)
4-4: LGTM: Added import for unified pagination types.The import supports the new unified pagination query approach consistent with other controller files.
16-31: LGTM: Streamlined pagination query construction.The refactoring improves code organization by:
- Building the filter query first
- Using
getPaginatedQuerywith a callback to configure bothExpandandBuilderfields in one step- Passing the query directly without dereferencing
This maintains the same functionality while following the unified pagination pattern.
internal/controller/system/store.go (1)
16-16: LGTM: Interface updated for unified pagination.The change from
ColumnPaginatedQuery[any]toPaginatedQuery[any]aligns the interface with the unified pagination system, providing more flexibility while maintaining compatibility.internal/storage/system/store.go (2)
24-24: LGTM: Simplified interface signature.Removing the generic pagination query type parameter aligns with the unified pagination approach and reduces interface complexity.
98-102: LGTM: Streamlined pagination resource creation.The simplified call to
NewPaginatedResourceRepositorywith explicit pagination column and order parameters is cleaner than the previous struct-based approach, while maintaining the same functionality.internal/api/v1/mocks_system_controller_test.go (1)
104-104: LGTM! Mock signature correctly updated for pagination refactor.The method signature change from
common.ColumnPaginatedQuery[any]tocommon.PaginatedQuery[any]correctly reflects the pagination unification refactor. Since this is a generated mock, it properly mirrors the underlying interface change.internal/api/v2/mocks_system_controller_test.go (1)
104-104: LGTM! Consistent pagination refactor in v2 mock.The signature change to
common.PaginatedQuery[any]is consistent with the pagination unification across the codebase and correctly reflects the underlying interface change.internal/storage/ledger/accounts_test.go (1)
80-281: LGTM! Systematic update to unified pagination query type.All test cases have been correctly updated to use
common.InitialPaginatedQuery[any]instead ofcommon.OffsetPaginatedQuery[any]. This change is semantically appropriate since these tests are all performing initial pagination requests without existing cursors, and aligns with the pagination unification refactor.internal/api/v1/controllers_config.go (1)
38-41: LGTM! Improved iteration pattern with unified pagination utilities.The refactor from direct
bunpaginate.Iteratetostoragecommon.IteratewithInitialPaginatedQueryimproves consistency with the pagination unification. The fixed PageSize of 100 is reasonable for ledger listing, and the new pattern is more generic and maintainable.internal/api/common/mocks_system_controller_test.go (1)
104-104: LGTM! Consistent pagination refactor across all API versions.The mock signature change to
common.PaginatedQuery[any]maintains consistency with the pagination unification refactor across v1, v2, and common API packages.internal/worker/async_block.go (2)
10-10: LGTM: Import alias improves clarity.The change from
commontostoragecommonmakes the import more explicit and avoids potential naming conflicts.
79-88: Excellent refactor: Pagination query construction is more structured.The changes improve the code in several ways:
- The
InitialPaginatedQuerywith embeddedResourceQueryis more explicit about the filtering criteria- The feature flag filter (
FeatureHashLogs="ASYNC") is clearly defined in the query construction- The simplified iterator call removes unnecessary wrapper function
- Using
storagecommon.Iteratemaintains consistency with the new pagination abstractioninternal/api/v2/controllers_logs_list_test.go (2)
26-26: Good: Function renamed for consistency.The rename from
TestGetLogstoTestLogsListaligns with naming conventions used across other test files.
33-153: Excellent: Test cases consistently updated for pagination refactor.All test cases have been properly updated to use the new pagination query types:
- Expected query type changed from specific
ColumnPaginatedQueryto genericPaginatedQuery- All instances of
ColumnPaginatedQuery[any]replaced withInitialPaginatedQuery[any]- The cursor encoding test case correctly embeds
InitialPaginatedQueryinsideColumnPaginatedQuery- Query structure and validation logic remain intact
The changes maintain test coverage while supporting the unified pagination interface.
internal/storage/ledger/logs_test.go (1)
133-262: Consistent update: Pagination query types properly migrated.All pagination calls have been correctly updated from
ColumnPaginatedQuerytoInitialPaginatedQuery:
- Line 133: Concurrency test updated with ascending order specification
- Lines 223, 230, 238, 254: Various pagination scenarios properly migrated
- Line 246: Explicit ascending order added for clarity in date range filtering
The test logic remains intact while supporting the new unified pagination interface.
internal/api/v2/controllers_ledgers_list_test.go (3)
5-5: Good: Added necessary import for order specification.The
pointerpackage import is required for thepointer.For(bunpaginate.Order(bunpaginate.OrderAsc))usage in the mock expectations.
25-25: Consistent: Function renamed to match naming convention.The rename from
TestListLedgerstoTestLedgersListmaintains consistency with other test function names in the codebase.
87-94: Excellent: Mock expectations explicitly specify pagination parameters.The mock setup now explicitly constructs
InitialPaginatedQuery[any]with all required fields:
PageSize: 15- matches the expected page sizeColumn: "id"- specifies the pagination columnOrder: pointer.For(bunpaginate.Order(bunpaginate.OrderAsc))- explicit ascending orderOptions.Expand: []string{}- empty expand slice for resource queriesThis makes the test expectations more explicit and maintainable compared to the previous
ledgercontroller.NewListLedgersQuery(15)approach.internal/api/v1/controllers_accounts_list.go (2)
4-5: Good: Added necessary imports for pagination refactor.The
bunpaginateandstoragecommonimports are required for the new unified pagination query construction.
17-31: Excellent refactor: Simplified pagination query construction.The changes improve the code structure significantly:
Before: Two-step process with separate query construction and builder assignment
After: SinglegetPaginatedQuerycall with clear parametersKey improvements:
"address"column andbunpaginate.OrderAscexplicitly specified- Resource query builder applied through clean callback pattern
- Query passed directly to
ListAccountsmethod (not dereferenced)- Consistent with the unified pagination interface across the codebase
internal/api/v2/controllers_volumes_test.go (4)
28-28: LGTM: Function rename aligns with API conventions.The rename from
TestGetVolumestoTestVolumesListbetter reflects the list operation being tested.
5-5: LGTM: Proper imports and type updates for pagination refactor.The addition of the pointer package import and the change to the unified
PaginatedQueryinterface align with the broader pagination refactor.Also applies to: 35-35
44-52: LGTM: Consistent pagination defaults for volumes.All test cases now use
InitialPaginatedQuerywith appropriate defaults:
- Column: "account" makes sense for volume ordering
- Order: ascending is appropriate for account-based sorting
- Pointer usage is correct for the Order field
Also applies to: 57-66, 71-80, 94-105, 110-119, 124-133
108-108: LGTM: Improved test case naming.The name change from "using exists filter" to "using exists metadata filter" provides better clarity about what's being tested.
internal/api/v1/controllers_logs_list_test.go (3)
30-30: LGTM: Type update aligns with pagination refactor.The change from
ColumnPaginatedQuery[any]to the unifiedPaginatedQuery[any]interface is consistent with the broader refactor.
39-43: LGTM: Appropriate pagination defaults for logs.The use of
InitialPaginatedQuerywith:
- Column: "id" for log ordering
- Order: descending (newest logs first)
- Proper pointer wrapping for Order field
These defaults make sense for log pagination.
Also applies to: 50-57, 64-71
76-86: LGTM: Correct cursor structure for pagination hierarchy.The cursor encoding test correctly uses
ColumnPaginatedQuerywith embeddedInitialPaginatedQuery. This hierarchical structure maintains compatibility with cursor-based pagination while adopting the new pagination types.internal/api/v2/controllers_accounts_list_test.go (3)
5-5: LGTM: Proper imports and type updates for pagination refactor.The pointer package import and the unified
PaginatedQueryinterface change align with the pagination refactor.Also applies to: 33-33
44-52: LGTM: Consistent pagination defaults for accounts.All test cases use
InitialPaginatedQuerywith appropriate defaults:
- Column: "address" is logical for account ordering
- Order: ascending provides intuitive sorting
- Pointer usage for Order field is correct
Also applies to: 59-68, 74-83, 129-137, 143-152, 158-167, 181-189, 197-205, 213-221
89-105: LGTM: Correct cursor structure for offset-based pagination.The cursor encoding test appropriately uses
OffsetPaginatedQuerywith embeddedInitialPaginatedQuery, which is suitable for account pagination (offset-based rather than column-based like logs).internal/api/v1/controllers_accounts_list_test.go (3)
4-4: LGTM: Proper imports and type updates for pagination refactor.The pointer package import and unified
PaginatedQueryinterface change are consistent with the refactor.Also applies to: 30-30
41-45: LGTM: Consistent pagination defaults for v1 accounts API.The defaults match v2 API appropriately:
- Column: "address" for logical account ordering
- Order: ascending for intuitive sorting
- Proper pointer usage for Order field
Also applies to: 53-60, 68-75, 115-119, 128-135, 143-147
80-91: Note: V1 uses column-based pagination.The cursor encoding test uses
ColumnPaginatedQuery(vsOffsetPaginatedQueryin v2). This suggests v1 and v2 APIs use different pagination strategies, which is acceptable for API versioning.internal/api/v2/controllers_transactions_list_test.go (4)
32-32: LGTM: Type update aligns with pagination refactor.The change to unified
PaginatedQuery[any]interface is consistent with the broader refactor.
41-49: LGTM: Appropriate pagination defaults for transactions.The use of
InitialPaginatedQuerywith:
- Column: "id" for transaction ordering
- Order: descending (newest transactions first)
- Proper pointer wrapping for Order field
These defaults make sense for transaction listing.
Also applies to: 54-63, 68-77, 82-91, 96-105, 110-119, 124-133, 138-147, 185-193, 225-234
239-247: LGTM: Flexible column selection for different ordering strategies.The "effective order" test case correctly uses Column: "timestamp" instead of "id", demonstrating the flexibility of the new pagination system to support different ordering strategies.
152-162: LGTM: Correct cursor structure for column-based pagination.The cursor encoding tests appropriately use
ColumnPaginatedQuerywith embeddedInitialPaginatedQuery, which is suitable for transaction pagination.Also applies to: 200-220
internal/storage/common/paginator.go (1)
8-11: Interface simplification looks good.The removal of the
PaginationOptionsgeneric parameter and corresponding method parameter simplifications are well-designed changes that:
- Reduce interface complexity
- Encapsulate pagination state within paginator implementations
- Align with the unified
PaginatedQueryapproach across the codebaseinternal/controller/system/controller.go (2)
27-27: Interface update aligns with pagination unification.The change from
common.ColumnPaginatedQuery[any]tocommon.PaginatedQuery[any]is consistent with the broader pagination query type standardization.
134-137: Implementation correctly matches updated interface.The method signature and delegation to
ctrl.store.ListLedgersproperly reflects the interface change to use the unifiedPaginatedQuerytype.internal/api/v1/controllers_transactions_list_test.go (3)
30-30: Test field type correctly updated.The
expectQueryfield type change fromColumnPaginatedQuerytoPaginatedQueryaligns with the pagination interface unification.
39-46: Test cases properly updated to use InitialPaginatedQuery.All test cases have been consistently updated to use
storagecommon.InitialPaginatedQuery[any]instead of the previous specific pagination query types, maintaining the same test logic and expectations.Also applies to: 53-61, 68-76, 83-91, 98-106, 113-121, 128-136, 143-151
156-172: Cursor encoding test properly handles nested structure.The cursor encoding test correctly uses the nested structure with
InitialPaginatedQueryembedded insideColumnPaginatedQuery, which properly represents the transitional pagination query structure.internal/controller/ledger/controller_default_test.go (2)
234-234: Mock instantiations correctly simplified.The removal of the third generic parameter from
NewMockPaginatedResourcecalls aligns with the simplified pagination query interface. All mock instantiations are consistently updated.Also applies to: 265-265, 285-285, 310-310, 335-335, 355-355, 403-403, 432-432
239-243: Test method calls properly updated to use unified pagination types.All test method calls have been consistently updated to use
common.InitialPaginatedQuery[any]instead of specific pagination query types, maintaining the same test logic and expectations.Also applies to: 247-251, 359-362, 365-368, 407-411, 414-418, 436-439, 442-445
internal/api/v2/controllers_volumes.go (3)
7-7: Import addition is appropriate.The addition of the time package import supports the new explicit query parameter parsing approach.
20-28: Query parameter parsing is now explicit and clear.The manual parsing of
groupByparameter with proper error handling improves code clarity compared to the previous implicit approach.
30-51: Time parameter parsing maintains compatibility.The explicit parsing of
startTimeandendTimeparameters with proper variable naming (pitfor point-in-time,ootfor out-of-time) maintains backward compatibility while improving code readability.internal/storage/ledger/store.go (1)
47-82: LGTM! Good simplification of pagination resource creation.The removal of the third generic parameter and the introduction of default pagination columns and orders make the code cleaner and more maintainable. The chosen defaults are sensible:
- Accounts ordered by "address" (ascending) - logical for alphabetical listing
- Transactions/Logs ordered by "id" (descending) - shows newest entries first
- Volumes ordered by "account" (ascending) - consistent with account ordering
internal/controller/ledger/store.go (1)
59-63: Interface simplification looks good!The removal of the pagination query type parameter from the Store interface methods aligns perfectly with the storage layer implementation. This simplification makes the API cleaner and easier to use while maintaining all necessary functionality through the unified
PaginatedQueryapproach.internal/api/v1/utils.go (1)
26-71: Excellent consolidation of pagination query logic!The new
getPaginatedQueryfunction successfully unifies the previously separate offset and column pagination functions. The implementation properly:
- Uses the generic
storagecommon.Extractfor cursor handling- Applies resource query modifiers before pagination
- Validates page size with appropriate limits
- Maintains backward compatibility with existing behavior
internal/api/v2/common.go (1)
64-109: Well-structured pagination consolidation with improved configurability!The v2 implementation follows the same consolidation pattern as v1 but adds the valuable
PaginationConfigparameter. This allows for endpoint-specific pagination settings, which is a great improvement for API flexibility. The implementation maintains consistency with v1 while adding this enhanced configurability.internal/controller/ledger/store_generated_test.go (1)
49-53: Generated mock correctly updated.The mock implementations have been properly regenerated to match the simplified Store interface signatures. All paginated resource methods now correctly use the two-parameter generic type.
Also applies to: 241-245, 300-304, 363-367
internal/api/bulking/mocks_ledger_controller_test.go (1)
244-244: LGTM: Consistent pagination query unification.The method signatures have been correctly updated to use the unified
common.PaginatedQueryinterface instead of specific pagination query types. This aligns with the broader refactoring to standardize pagination handling across the codebase.Also applies to: 288-288, 303-303, 318-318
internal/storage/common/pagination.go (1)
8-37: Well-designed pagination abstraction.The new pagination query types provide a clean abstraction using the marker interface pattern:
- Layered design:
InitialPaginatedQueryprovides the base, withOffsetPaginatedQueryandColumnPaginatedQueryextending it for specific pagination strategies- Type safety: The marker interface
PaginatedQueryenables type constraints while supporting different pagination implementations- Compile-time verification: The interface compliance checks ensure proper implementation
- Flexibility: Generic
OptionsTypeparameter allows for different filtering options per resource typeThis unified approach should simplify pagination handling across the codebase while maintaining backwards compatibility with existing pagination strategies.
internal/api/common/mocks_ledger_controller_test.go (1)
244-244: Consistent with pagination unification.Method signatures correctly updated to match the unified pagination interface, following the same pattern as other mock files in this refactoring.
Also applies to: 288-288, 303-303, 318-318
internal/api/v2/mocks_ledger_controller_test.go (1)
244-244: Consistent pagination interface updates.Mock signatures properly updated to use the unified
PaginatedQueryinterface, maintaining consistency with the broader refactoring effort.Also applies to: 288-288, 303-303, 318-318
internal/controller/ledger/controller_generated_test.go (1)
243-243: Pagination interface properly unified.Controller mock signatures correctly updated to use
common.PaginatedQuery, completing the consistent interface changes across all controller mocks.Also applies to: 287-287, 302-302, 317-317
internal/controller/ledger/controller.go (1)
32-38: Interface methods correctly unified to use generic pagination typeThe migration from specific pagination query types to the generic
common.PaginatedQuery[any]type is consistent across all paginated methods. This unification simplifies the API surface and improves maintainability.internal/controller/ledger/controller_with_traces.go (1)
214-223: Tracing wrapper methods correctly updatedThe method signatures have been properly updated to match the interface changes, maintaining the delegation pattern to the underlying controller.
Also applies to: 262-271, 298-307, 346-355
internal/storage/common/schema.go (1)
1-254: Well-designed schema system for field typesThe schema implementation provides a clean abstraction for field types with proper:
- Operator support per type
- Value validation
- Indexability and pagination capabilities
- Convenient factory functions
The type system aligns well with the unified pagination approach.
internal/api/v1/mocks_ledger_controller_test.go (1)
244-255: Mock methods correctly updated for unified paginationThe generated mock methods have been properly updated to match the interface changes, using
common.PaginatedQueryinstead of specific pagination query types.Also applies to: 288-299, 303-314, 318-329
internal/storage/common/paginator_column.go (1)
252-259: ```shell
#!/bin/bashVerify which FieldType implementations support pagination
echo "---- TypeString IsPaginated ----"
rg -n "func (t TypeString) IsPaginated" -C3 internal/storage/common/schema.goecho -e "\n---- TypeNumeric IsPaginated ----"
rg -n "func (t TypeNumeric) IsPaginated" -C3 internal/storage/common/schema.goecho -e "\n---- TypeBoolean IsPaginated ----"
rg -n "func (t TypeBoolean) IsPaginated" -C3 internal/storage/common/schema.goecho -e "\n---- TypeMap IsPaginated ----"
rg -n "func (t TypeMap) IsPaginated" -C3 internal/storage/common/schema.go</details> <details> <summary>internal/controller/ledger/mocks_test.go (1)</summary> `212-316`: **LGTM!** The mock changes correctly reflect the new unified pagination query interface. </details> <details> <summary>internal/controller/ledger/controller_default.go (2)</summary> `10-11`: **LGTM!** The import alias change and method signature updates are consistent with the pagination system refactoring. Also applies to: 145-183 --- `317-332`: **Clean iteration implementation.** The migration from `bunpaginate.Iterate` to `storagecommon.Iterate` simplifies the pagination logic by removing the need to pass a pagination function. </details> <details> <summary>internal/storage/common/resource.go (1)</summary> `63-72`: **Well-implemented field lookup method.** The `GetFieldByNameOrAlias` method provides a clean way to find fields by either their name or aliases, which supports the flexible pagination column specification. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| func Iterate[OF any, Options any]( | ||
| ctx context.Context, | ||
| initialQuery InitialPaginatedQuery[Options], | ||
| iterator func(ctx context.Context, q PaginatedQuery[Options]) (*bunpaginate.Cursor[OF], error), | ||
| cb func(cursor *bunpaginate.Cursor[OF]) error, | ||
| ) error { | ||
|
|
||
| var query PaginatedQuery[OF] = initialQuery | ||
| for { | ||
| cursor, err := iterator(ctx, query) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := cb(cursor); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if !cursor.HasMore { | ||
| break | ||
| } | ||
|
|
||
| query, err = unmarshalCursor[OF](cursor.Next) | ||
| if err != nil { | ||
| return fmt.Errorf("paginating next request: %w", err) | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Fix type parameter inconsistencies in Iterate function
There are type parameter mismatches in the Iterate function:
- Line 79:
initialQueryis assigned toquerybut they have different type parameters (OFvsOptions) - Line 94:
unmarshalCursoris called with type parameterOFbut should useOptionsto match the iterator signature
Apply this fix:
func Iterate[OF any, Options any](
ctx context.Context,
initialQuery InitialPaginatedQuery[Options],
iterator func(ctx context.Context, q PaginatedQuery[Options]) (*bunpaginate.Cursor[OF], error),
cb func(cursor *bunpaginate.Cursor[OF]) error,
) error {
- var query PaginatedQuery[OF] = initialQuery
+ var query PaginatedQuery[Options] = initialQuery
for {
cursor, err := iterator(ctx, query)
if err != nil {
return err
}
if err := cb(cursor); err != nil {
return err
}
if !cursor.HasMore {
break
}
- query, err = unmarshalCursor[OF](cursor.Next)
+ query, err = unmarshalCursor[Options](cursor.Next)
if err != nil {
return fmt.Errorf("paginating next request: %w", err)
}
}
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func Iterate[OF any, Options any]( | |
| ctx context.Context, | |
| initialQuery InitialPaginatedQuery[Options], | |
| iterator func(ctx context.Context, q PaginatedQuery[Options]) (*bunpaginate.Cursor[OF], error), | |
| cb func(cursor *bunpaginate.Cursor[OF]) error, | |
| ) error { | |
| var query PaginatedQuery[OF] = initialQuery | |
| for { | |
| cursor, err := iterator(ctx, query) | |
| if err != nil { | |
| return err | |
| } | |
| if err := cb(cursor); err != nil { | |
| return err | |
| } | |
| if !cursor.HasMore { | |
| break | |
| } | |
| query, err = unmarshalCursor[OF](cursor.Next) | |
| if err != nil { | |
| return fmt.Errorf("paginating next request: %w", err) | |
| } | |
| } | |
| return nil | |
| } | |
| func Iterate[OF any, Options any]( | |
| ctx context.Context, | |
| initialQuery InitialPaginatedQuery[Options], | |
| iterator func(ctx context.Context, q PaginatedQuery[Options]) (*bunpaginate.Cursor[OF], error), | |
| cb func(cursor *bunpaginate.Cursor[OF]) error, | |
| ) error { | |
| var query PaginatedQuery[Options] = initialQuery | |
| for { | |
| cursor, err := iterator(ctx, query) | |
| if err != nil { | |
| return err | |
| } | |
| if err := cb(cursor); err != nil { | |
| return err | |
| } | |
| if !cursor.HasMore { | |
| break | |
| } | |
| query, err = unmarshalCursor[Options](cursor.Next) | |
| if err != nil { | |
| return fmt.Errorf("paginating next request: %w", err) | |
| } | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In internal/storage/common/cursor.go lines 72 to 101, the Iterate function has
type parameter mismatches: the variable query should be of type
PaginatedQuery[Options] to match initialQuery, and unmarshalCursor should be
called with type parameter Options instead of OF. Update the declaration of
query to use Options as its type parameter and change the unmarshalCursor call
to unmarshalCursor[Options] to ensure consistent type usage throughout the
function.
| if o.query.Offset > math.MaxInt32 { | ||
| return nil, fmt.Errorf("offset value exceeds maximum allowed value") | ||
| } | ||
| if query.Offset > 0 { | ||
| sb = sb.Offset(int(query.Offset)) | ||
| if o.query.Offset > 0 { | ||
| sb = sb.Offset(int(o.query.Offset)) | ||
| } |
There was a problem hiding this comment.
Fix offset validation and type conversion issues.
The offset validation has two issues:
- The offset is of type
uint64but is being compared againstmath.MaxInt32, which could unnecessarily restrict valid offset values. - The conversion from
uint64tointon line 27 could cause overflow if the offset exceeds the maximum int value.
Consider using math.MaxInt instead of math.MaxInt32 for the comparison, or validate against the maximum value that can be safely converted to int:
- if o.query.Offset > math.MaxInt32 {
- return nil, fmt.Errorf("offset value exceeds maximum allowed value")
- }
+ if o.query.Offset > math.MaxInt {
+ return nil, fmt.Errorf("offset value exceeds maximum allowed value")
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In internal/storage/common/paginator_offset.go around lines 23 to 28, the offset
validation incorrectly compares a uint64 offset against math.MaxInt32 and
converts it to int without ensuring it fits, risking overflow. Update the
validation to compare the offset against math.MaxInt (the maximum int value) or
the maximum safe int conversion limit, and only convert to int after confirming
the offset is within this safe range to prevent overflow errors.
| if o.query.Bottom == nil { | ||
| o.query.Bottom = paginationID | ||
| } |
There was a problem hiding this comment.
Avoid side effects in BuildCursor method.
The BuildCursor method is modifying the query's Bottom field, which is unexpected behavior. Methods that build or return data structures should generally not have side effects on their internal state.
Consider either:
- Making this initialization happen in the constructor or Paginate method
- Working with a copy of the query to avoid modifying the original
- Documenting this behavior if it's intentional
🤖 Prompt for AI Agents
In internal/storage/common/paginator_column.go around lines 82 to 84, the
BuildCursor method is modifying the query's Bottom field, causing unintended
side effects. To fix this, avoid changing the original query inside BuildCursor
by either initializing Bottom earlier in the constructor or Paginate method, or
by working with a copy of the query within BuildCursor so the original remains
unchanged. Alternatively, if this behavior is intentional, add clear
documentation explaining the side effect.
gfyrag
force-pushed
the
refacto/pagination-system
branch
from
d537e72 to
16362d4
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/storage/common/cursor.go (1)
72-101: Fix type parameter inconsistencies in Iterate functionThis duplicates the existing review comment but the issue persists. The type parameter mismatches need to be corrected:
- Line 79:
queryshould bePaginatedQuery[Options]to matchinitialQuery- Line 94:
unmarshalCursorshould useOptionstype parameter, notOFApply this fix:
- var query PaginatedQuery[OF] = initialQuery + var query PaginatedQuery[Options] = initialQuery for { cursor, err := iterator(ctx, query) if err != nil { return err } if err := cb(cursor); err != nil { return err } if !cursor.HasMore { break } - query, err = unmarshalCursor[OF](cursor.Next) + query, err = unmarshalCursor[Options](cursor.Next) if err != nil { return fmt.Errorf("paginating next request: %w", err) } }
🧹 Nitpick comments (1)
internal/storage/common/cursor.go (1)
13-13: Address the TODO comment about backporting to go-libs.Consider creating an issue to track this backport task to avoid it being forgotten.
Do you want me to help draft an issue for tracking this backport task?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.modis excluded by!**/*.mod
📒 Files selected for processing (25)
internal/api/v1/controllers_accounts_list.go(2 hunks)internal/api/v1/controllers_balances_list.go(2 hunks)internal/api/v1/controllers_config.go(1 hunks)internal/api/v1/controllers_logs_list.go(2 hunks)internal/api/v1/controllers_transactions_list.go(2 hunks)internal/api/v1/utils.go(1 hunks)internal/api/v2/common.go(1 hunks)internal/api/v2/controllers_ledgers_list.go(2 hunks)internal/api/v2/controllers_ledgers_list_test.go(4 hunks)internal/controller/ledger/controller_default.go(4 hunks)internal/controller/ledger/store.go(1 hunks)internal/storage/bucket/migrations_test.go(1 hunks)internal/storage/common/cursor.go(1 hunks)internal/storage/common/pagination.go(1 hunks)internal/storage/common/paginator_column.go(6 hunks)internal/storage/common/resource.go(6 hunks)internal/storage/driver/driver_test.go(2 hunks)internal/storage/ledger/accounts_test.go(15 hunks)internal/storage/ledger/logs_test.go(2 hunks)internal/storage/ledger/store.go(2 hunks)internal/storage/ledger/transactions_test.go(13 hunks)internal/storage/ledger/volumes_test.go(29 hunks)internal/storage/system/store.go(2 hunks)internal/storage/system/store_test.go(2 hunks)internal/worker/async_block.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (24)
- internal/api/v1/controllers_balances_list.go
- internal/api/v2/controllers_ledgers_list.go
- internal/storage/system/store_test.go
- internal/storage/ledger/logs_test.go
- internal/api/v1/controllers_config.go
- internal/api/v1/controllers_transactions_list.go
- internal/api/v1/controllers_logs_list.go
- internal/api/v1/controllers_accounts_list.go
- internal/storage/ledger/transactions_test.go
- internal/storage/bucket/migrations_test.go
- internal/storage/ledger/volumes_test.go
- internal/worker/async_block.go
- internal/storage/driver/driver_test.go
- internal/storage/ledger/accounts_test.go
- internal/storage/system/store.go
- internal/api/v2/controllers_ledgers_list_test.go
- internal/storage/ledger/store.go
- internal/api/v1/utils.go
- internal/storage/common/pagination.go
- internal/controller/ledger/store.go
- internal/api/v2/common.go
- internal/storage/common/paginator_column.go
- internal/controller/ledger/controller_default.go
- internal/storage/common/resource.go
🔇 Additional comments (1)
internal/storage/common/cursor.go (1)
103-108: LGTM: Simple and clean cursor encoding.The function correctly handles nil values and delegates to the bunpaginate library.
| func unmarshalCursor[OF any](v string, modifiers ...func(query *InitialPaginatedQuery[OF]) error) (PaginatedQuery[OF], error) { | ||
| res, err := base64.RawURLEncoding.DecodeString(v) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // todo: we should better rely on schema to determine the type of cursor | ||
| type aux struct { | ||
| Offset *uint64 `json:"offset"` | ||
| } | ||
| x := aux{} | ||
| if err := json.Unmarshal(res, &x); err != nil { | ||
| return nil, fmt.Errorf("invalid cursor: %w", err) | ||
| } | ||
|
|
||
| var q PaginatedQuery[OF] | ||
| if x.Offset != nil { // Offset defined, this is an offset cursor | ||
| q = &OffsetPaginatedQuery[OF]{} | ||
| } else { | ||
| q = &ColumnPaginatedQuery[OF]{} | ||
| } | ||
|
|
||
| if err := json.Unmarshal(res, &q); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| var root *InitialPaginatedQuery[OF] | ||
| if x.Offset != nil { // Offset defined, this is an offset cursor | ||
| root = &q.(*OffsetPaginatedQuery[OF]).InitialPaginatedQuery | ||
| } else { | ||
| root = &q.(*ColumnPaginatedQuery[OF]).InitialPaginatedQuery | ||
| } | ||
|
|
||
| for _, modifier := range modifiers { | ||
| if err := modifier(root); err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
|
|
||
| return reflect.ValueOf(q).Elem().Interface().(PaginatedQuery[OF]), nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor cursor type determination to avoid reflection and improve maintainability.
The current implementation has several concerning aspects:
- Hacky type determination: Using the presence of an
offsetfield to determine cursor type (acknowledged in TODO on line 36) - Complex reflection usage: Line 69 uses reflection to extract the interface value, which is error-prone and hard to maintain
- Duplicate type checking logic: Lines 46-50 and 57-61 repeat similar logic for type determination
Consider these improvements:
func unmarshalCursor[OF any](v string, modifiers ...func(query *InitialPaginatedQuery[OF]) error) (PaginatedQuery[OF], error) {
res, err := base64.RawURLEncoding.DecodeString(v)
if err != nil {
return nil, err
}
- // todo: we should better rely on schema to determine the type of cursor
- type aux struct {
- Offset *uint64 `json:"offset"`
- }
- x := aux{}
- if err := json.Unmarshal(res, &x); err != nil {
- return nil, fmt.Errorf("invalid cursor: %w", err)
- }
-
- var q PaginatedQuery[OF]
- if x.Offset != nil { // Offset defined, this is an offset cursor
- q = &OffsetPaginatedQuery[OF]{}
- } else {
- q = &ColumnPaginatedQuery[OF]{}
- }
-
- if err := json.Unmarshal(res, &q); err != nil {
- return nil, err
- }
-
- var root *InitialPaginatedQuery[OF]
- if x.Offset != nil { // Offset defined, this is an offset cursor
- root = &q.(*OffsetPaginatedQuery[OF]).InitialPaginatedQuery
- } else {
- root = &q.(*ColumnPaginatedQuery[OF]).InitialPaginatedQuery
- }
-
- for _, modifier := range modifiers {
- if err := modifier(root); err != nil {
- return nil, err
- }
- }
-
- return reflect.ValueOf(q).Elem().Interface().(PaginatedQuery[OF]), nil
+ // Add a type discriminator field to cursors for cleaner type determination
+ type cursorEnvelope struct {
+ Type string `json:"type"`
+ Data json.RawMessage `json:"data"`
+ }
+
+ var envelope cursorEnvelope
+ if err := json.Unmarshal(res, &envelope); err != nil {
+ // Fallback to legacy detection for backward compatibility
+ return unmarshalLegacyCursor[OF](res, modifiers...)
+ }
+
+ switch envelope.Type {
+ case "offset":
+ var q OffsetPaginatedQuery[OF]
+ if err := json.Unmarshal(envelope.Data, &q); err != nil {
+ return nil, err
+ }
+ for _, modifier := range modifiers {
+ if err := modifier(&q.InitialPaginatedQuery); err != nil {
+ return nil, err
+ }
+ }
+ return &q, nil
+ case "column":
+ var q ColumnPaginatedQuery[OF]
+ if err := json.Unmarshal(envelope.Data, &q); err != nil {
+ return nil, err
+ }
+ for _, modifier := range modifiers {
+ if err := modifier(&q.InitialPaginatedQuery); err != nil {
+ return nil, err
+ }
+ }
+ return &q, nil
+ default:
+ return nil, fmt.Errorf("unknown cursor type: %s", envelope.Type)
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In internal/storage/common/cursor.go between lines 30 and 70, the code uses the
presence of the offset field to determine the cursor type, repeats this logic
twice, and relies on reflection to extract the interface value, which is fragile
and hard to maintain. Refactor by defining a clear interface or factory method
that encapsulates cursor type determination based on the unmarshaled data
schema, eliminate duplicate type checks by consolidating the logic into one
place, and remove reflection by directly returning the concrete PaginatedQuery
implementation without using reflect.ValueOf or type assertions that require
reflection.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #978 +/- ##
==========================================
- Coverage 82.98% 82.92% -0.07%
==========================================
Files 142 145 +3
Lines 8089 8199 +110
==========================================
+ Hits 6713 6799 +86
- Misses 1057 1075 +18
- Partials 319 325 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e/v1_api_accounts_list_test.go (1)
53-53: Add error checking for SetString operationWhile the number string is valid and unlikely to fail, it's good practice to check for errors when using
SetString.-bigInt, _ = big.NewInt(0).SetString("999999999999999999999999999999999999999999999999999999999999999999999999999999999999999", 10) +bigInt, ok := big.NewInt(0).SetString("999999999999999999999999999999999999999999999999999999999999999999999999999999999999999", 10) +Expect(ok).To(BeTrue())
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
openapi.yamlis excluded by!**/*.yamlopenapi/v1.yamlis excluded by!**/*.yamlpkg/client/.speakeasy/gen.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (6)
internal/api/v2/controllers_ledgers_list.go(1 hunks)internal/controller/ledger/mocks_test.go(4 hunks)pkg/client/v1.go(1 hunks)test/e2e/v1_api_accounts_list_test.go(1 hunks)test/e2e/v1_api_logs_list_test.go(1 hunks)test/e2e/v1_api_transactions_list_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/api/v2/controllers_ledgers_list.go
- internal/controller/ledger/mocks_test.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Tests
- GitHub Check: Dirty
🔇 Additional comments (7)
pkg/client/v1.go (1)
614-614: ```shell
#!/bin/bashExtract CountAccounts method body to inspect HTTP method and status code checks
echo "=== CountAccounts snippet (lines 450–650) ==="
sed -n '450,650p' pkg/client/v1.goExtract CountTransactions method body to inspect HTTP method and status code checks
echo -e "\n=== CountTransactions snippet (lines 2100–2330) ==="
sed -n '2100,2330p' pkg/client/v1.go</details> <details> <summary>test/e2e/v1_api_logs_list_test.go (1)</summary> `1-328`: **Well-structured end-to-end tests for logs listing** The test suite provides comprehensive coverage for the logs listing API, including: - Basic listing and content validation - Pagination with forward and backward cursor navigation - Proper assertions for log types and data structure The tests are well-organized using nested contexts and descriptive test names. </details> <details> <summary>test/e2e/v1_api_accounts_list_test.go (1)</summary> `26-305`: **Comprehensive test coverage for accounts listing API** The test suite provides excellent coverage for the accounts listing API, including: - Account counting and listing - Filtering by address patterns and metadata - Empty ledger scenarios - Pagination with cursor navigation The tests follow the same structure as the logs tests, maintaining consistency across the test suite. </details> <details> <summary>test/e2e/v1_api_transactions_list_test.go (4)</summary> `1-21`: **LGTM!** The imports and build constraints are well-structured for an integration test. --- `23-43`: **LGTM!** The test setup properly initializes the database, test server, and creates the default ledger before each test. --- `91-94`: **LGTM!** The transaction prepending logic correctly builds the transactions slice in reverse chronological order (newest first), which aligns with typical API pagination expectations. --- `99-202`: **LGTM!** The pagination test cases are comprehensive and well-structured: - Tests different page sizes (5 and 10) - Tests cursor-based navigation (next/previous) - Tests filtering by reference and other parameters - Properly validates page sizes, data content, and cursor behavior The test logic correctly validates both forward and backward pagination scenarios. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| if offset%2 == 0 { | ||
| offset += 1 | ||
| } else { | ||
| offset -= 1 | ||
| } |
There was a problem hiding this comment.
Fix the timestamp alternation logic.
The current logic uses modulo on a time.Duration type, which doesn't achieve the intended alternating pattern described in the comment. The modulo operation on Duration (nanoseconds) doesn't provide meaningful alternation for testing purposes.
Apply this diff to fix the alternating timestamp logic:
- // 1 transaction of 2 is backdated to test pagination using effective date
- if offset%2 == 0 {
- offset += 1
- } else {
- offset -= 1
- }
+ // 1 transaction of 2 is backdated to test pagination using effective date
+ if i%2 == 0 {
+ offset += time.Nanosecond
+ } else {
+ offset -= time.Nanosecond
+ }🤖 Prompt for AI Agents
In test/e2e/v1_api_transactions_list_test.go around lines 57 to 61, the current
code incorrectly uses modulo on a time.Duration type to alternate timestamps,
which does not produce the intended alternating pattern. Replace the modulo
operation on the Duration with a modulo on an integer counter or index variable
that tracks iterations, and use that to alternate the timestamp by adding or
subtracting a fixed duration accordingly.
2d27b42 to
858712a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/e2e/v1_api_transactions_list_test.go (1)
57-61: Fix the timestamp alternation logic.The current logic uses modulo on a
time.Durationtype, which doesn't achieve the intended alternating pattern described in the comment. The modulo operation on Duration (nanoseconds) doesn't provide meaningful alternation for testing purposes.
🧹 Nitpick comments (1)
test/e2e/v1_api_transactions_list_test.go (1)
91-94: Consider minor optimization for transaction collection.The current prepending pattern has O(n²) complexity as it copies the entire slice on each iteration. While acceptable for test purposes with 20 transactions, consider using a simple append and reversing the slice afterward for better performance in larger test scenarios.
- transactions = append([]components.Transaction{ - response.TransactionsResponse.Data[0], - }, transactions...) + transactions = append(transactions, response.TransactionsResponse.Data[0])Then reverse the slice after the loop if needed for test ordering expectations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
openapi.yamlis excluded by!**/*.yamlopenapi/v1.yamlis excluded by!**/*.yamlpkg/client/.speakeasy/gen.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (7)
internal/api/v2/controllers_ledgers_list.go(1 hunks)internal/controller/ledger/mocks_test.go(4 hunks)internal/storage/common/paginator_offset.go(2 hunks)pkg/client/v1.go(1 hunks)test/e2e/v1_api_accounts_list_test.go(1 hunks)test/e2e/v1_api_logs_list_test.go(1 hunks)test/e2e/v1_api_transactions_list_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/api/v2/controllers_ledgers_list.go
- test/e2e/v1_api_logs_list_test.go
- test/e2e/v1_api_accounts_list_test.go
- pkg/client/v1.go
- internal/storage/common/paginator_offset.go
- internal/controller/ledger/mocks_test.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Tests
- GitHub Check: Dirty
🔇 Additional comments (4)
test/e2e/v1_api_transactions_list_test.go (4)
1-21: LGTM! Standard e2e test setup.The package declaration, build tags, and imports are well-organized and appropriate for an end-to-end integration test.
23-43: Well-structured test setup.The test context setup properly initializes the database, test server, and creates the required ledger. Good use of Ginkgo patterns and proper resource management.
99-130: Excellent pagination test coverage.This test effectively validates that pagination cursors work correctly when page sizes change between requests, which is an important edge case to verify.
131-202: Comprehensive pagination and filtering test suite.This test section provides excellent coverage of:
- Pagination with various filters (source, reference, time range)
- Cursor navigation (forward and backward)
- Pagination metadata validation
- Edge cases like end-of-results scenarios
The test logic correctly handles the transaction ordering created by the prepending pattern in the setup.
There was a problem hiding this comment.
Bug: API Change Breaks Count Endpoint
The count accounts endpoint's HTTP status code was changed from 200 (OK) to 204 (No Content). This is a breaking API change for existing clients that expect a 200 OK response. While 204 might be technically correct as the count is returned in headers, count operations typically return 200. This change is reflected in the OpenAPI specification and client-side response handling.
openapi/v1.yaml#L92-L93
Lines 92 to 93 in 858712a
pkg/client/v1.go#L613-L614
Lines 613 to 614 in 858712a
Was this report helpful? Give feedback by reacting with 👍 or 👎
