feat: add filtering on ledgers list #834
Conversation
WalkthroughThis PR refactors resource query and pagination handling across the ledger system. Query and error types are migrated from legacy packages (ledgercontroller/ledger0/storagecommon) to a unified common package. Key modifications include updated API documentation (with a new RequestBody for list ledgers), standardized method signatures in controllers and mocks, overhauled storage resource handlers and pagination utilities, and corresponding updates to tests and drivers. These changes streamline ledger listing, filtering (including substring matching for ledger names), and overall query processing. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant V as V2 Client API
participant D as Driver/Store
participant DB as Database
C->>V: Create ListLedgers request with RequestBody
V->>V: Serialize RequestBody & set Content-Type header
V->>D: Call Ledgers().Paginate(ctx, query)
D->>DB: Execute paginated query using common package types
DB-->>D: Return ledger records
D-->>V: Return paginated resource data
V-->>C: Respond with ledger list and cursor
Assessment against linked issues
Possibly related PRs
Poem
✨ Finishing Touches
🪧 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: 4
🔭 Outside diff range comments (1)
internal/api/v1/controllers_accounts_list_test.go (1)
1-173: 💡 Verification agent❓ Verification inconclusive
Verify test coverage completeness.
The test cases appear comprehensive, covering various query scenarios including metadata filtering, address filtering, balance filtering, cursor handling, and pagination. Consider adding tests for any new filtering capabilities specifically added for ledgers list (like name substring matching mentioned in the AI summary).
🏁 Script executed:
#!/bin/bash # Check if there are ledger name filtering tests in the codebase rg -g "*.go" -A 10 -B 10 "ledger.*name" ".*test.*"Length of output: 101
Ledger List Tests: Enhance Coverage for Name Filtering
The existing test suite robustly verifies metadata, address, balance filters, cursor handling, and pagination. However, after a search for ledger name filtering (e.g. using instances of
query.Match("name", …)) in the test file, no such tests were found. If name substring filtering has been added or is planned as part of the ledger listing capabilities, please add corresponding tests—covering both nominal and erroneous cases—to ensure full coverage.
- Verify that ledger name filtering is implemented.
- Add test cases for name substring matching (both valid and invalid scenarios).
🧹 Nitpick comments (16)
internal/storage/ledger/resource_volumes.go (3)
41-49: Consider extending metadata filter support.
Currently, only$existsis supported formetadata, and$matchwhen referencing a specific key viametadata[...]. If your use case expands, you might consider adding$eq,$ne, or similar operators in the future to enable more granular control.
71-94: Refactor the lateral join for improved readability.
When!query.UsePIT() && !query.UseOOT(), we rely on a lateral join to fetch address metadata. Consider encapsulating this join logic into a helper function, reducing complexity withinBuildDataset.
228-228: No expansions supported yet.
Returning an error inExpand()is acceptable for now, but if expansions become necessary, a consistent approach here will be key.internal/storage/system/resource_ledgers.go (2)
11-13: Consider moving the featuresRegex to the common packageThe
featuresRegexpattern follows the same pattern ascommon.MetadataRegex(as seen in the relevant snippets). For consistency and reusability, consider moving this regex to the common package alongsideMetadataRegex.
15-17: Add a constructor function for the ledgersResourceHandlerThe
ledgersResourceHandlerstruct requires astorefield, but there's no constructor function to initialize it. Consider adding a constructor function to ensure proper initialization.+func newLedgersResourceHandler(store *DefaultStore) ledgersResourceHandler { + return ledgersResourceHandler{ + store: store, + } +}docs/api/README.md (1)
127-131: Request body documentation needs more detailed examples.While you've added the body parameter section, the example shows an empty JSON object (
{}). This doesn't provide users with information about what filtering options are available or how to structure their queries.Consider enhancing the documentation with a more comprehensive example showing the available filtering options:
- {} + { + "name": "ledger1", + "pageSize": 10, + "metadata": { + "environment": "production" + } + }internal/storage/common/errors.go (1)
5-16: Consider enhancing error comparison witherrors.AsYour
Ismethod implementation only checks for direct type equality rather than also considering wrapped errors. This could lead to issues when errors are wrapped (e.g., withfmt.Errorf("something: %w", err)).Consider enhancing the
Ismethod to useerrors.Asfor more robust error comparison:-func (e ErrInvalidQuery) Is(err error) bool { - _, ok := err.(ErrInvalidQuery) - return ok +func (e ErrInvalidQuery) Is(err error) bool { + var target ErrInvalidQuery + return errors.As(err, &target)Don't forget to add the import:
-import "fmt" +import ( + "errors" + "fmt" +)test/e2e/api_ledgers_list_test.go (1)
93-104: Test case has incorrect name.The test case name indicates it's testing filtering for "ledger0", but it's actually testing pattern matching using $like operator for "ledger1%".
- It("filtering on name = ledger0 should return 1 ledger", func() { + It("filtering on name using $like operator should return 11 ledgers", func() {internal/worker/async_block.go (1)
73-73: Improved query construction using structured builderThe implementation has been improved by replacing a direct map assignment with a structured query.Match builder. This provides better type safety and readability for feature filtering.
internal/storage/common/resource.go (4)
18-33: Handle unknown operators gracefully instead of panickingCurrently, an unknown operator triggers
panic("unreachable"). Consider returning an error or a more user-friendly message to avoid abrupt termination in production.
49-56: Validate potential case sensitivity
AcceptOperatorschecks for exact matches inoperators. If operators might be uppercase or mixed case, consider normalizing them.
92-146: Potential performance concern with regex usage in tight loops
regexp.MatchStringis invoked for each filter invalidateFilters. If multiple filters or frequent calls are expected, consider caching compiled regexes or using direct string comparisons to optimize performance.
282-305: Use a stable sorting key for paginationUsing
ORDER BY row_numbercan lead to unstable pagination if rows shift while paging. Ensure there's a unique ordering column or composite ordering to produce consistent pagination results.internal/storage/common/paginator_column.go (1)
20-59: Ensure valid pagination ID handlingIn
Paginate, unknown or invalidPaginationIDcan cause confusion if not validated. It might help to return more user-friendly errors ifPaginationIDis malformed or out of range.pkg/client/docs/sdks/v2/README.md (1)
65-67: Example RequestBody added to ListLedgers exampleThe example shows how to initialize the new RequestBody field, but uses generic placeholder values that don't provide enough context on how this field should be used for filtering ledgers.
Consider enhancing this example with a more realistic filtering case, such as:
RequestBody: map[string]any{ - "key": "<value>", + "name": "ledger001", // Filter by ledger name + "metadata": map[string]string{ // Filter by metadata + "environment": "production", + }, },internal/storage/system/store_test.go (1)
160-162: Simplified debug hook configurationThe debug hook initialization has been refactored to create the hook first and then set its Debug property based on the environment variable. This approach is cleaner and more maintainable than the previous conditional logic.
Consider encapsulating this debug hook configuration into a separate function for better reusability across tests:
- debugHook := bundebug.NewQueryHook() - debugHook.Debug = os.Getenv("DEBUG") == "true" - hooks = append(hooks, debugHook) + hooks = append(hooks, createDebugHook()) + // At file scope + func createDebugHook() bun.QueryHook { + hook := bundebug.NewQueryHook() + hook.Debug = os.Getenv("DEBUG") == "true" + return hook + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumopenapi.yamlis excluded by!**/*.yamlopenapi/v2.yamlis excluded by!**/*.yamlpkg/client/.speakeasy/gen.lockis excluded by!**/*.lock,!**/*.locktools/generator/go.modis excluded by!**/*.modtools/generator/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (90)
docs/api/README.md(1 hunks)internal/api/bulking/mocks_ledger_controller_test.go(10 hunks)internal/api/common/mocks_ledger_controller_test.go(10 hunks)internal/api/common/mocks_system_controller_test.go(2 hunks)internal/api/v1/controllers_accounts_count.go(2 hunks)internal/api/v1/controllers_accounts_count_test.go(7 hunks)internal/api/v1/controllers_accounts_list_test.go(8 hunks)internal/api/v1/controllers_accounts_read.go(2 hunks)internal/api/v1/controllers_accounts_read_test.go(5 hunks)internal/api/v1/controllers_balances_aggregates_test.go(3 hunks)internal/api/v1/controllers_config.go(2 hunks)internal/api/v1/controllers_logs_list_test.go(5 hunks)internal/api/v1/controllers_transactions_count_test.go(9 hunks)internal/api/v1/controllers_transactions_list_test.go(12 hunks)internal/api/v1/controllers_transactions_read_test.go(2 hunks)internal/api/v1/mocks_ledger_controller_test.go(10 hunks)internal/api/v1/mocks_system_controller_test.go(2 hunks)internal/api/v1/utils.go(5 hunks)internal/api/v2/common.go(6 hunks)internal/api/v2/controllers_accounts_count.go(2 hunks)internal/api/v2/controllers_accounts_count_test.go(10 hunks)internal/api/v2/controllers_accounts_list.go(2 hunks)internal/api/v2/controllers_accounts_list_test.go(12 hunks)internal/api/v2/controllers_accounts_read.go(2 hunks)internal/api/v2/controllers_accounts_read_test.go(3 hunks)internal/api/v2/controllers_balances.go(2 hunks)internal/api/v2/controllers_balances_test.go(6 hunks)internal/api/v2/controllers_ledgers_list.go(1 hunks)internal/api/v2/controllers_ledgers_list_test.go(3 hunks)internal/api/v2/controllers_logs_list.go(2 hunks)internal/api/v2/controllers_logs_list_test.go(7 hunks)internal/api/v2/controllers_transactions_count.go(2 hunks)internal/api/v2/controllers_transactions_count_test.go(13 hunks)internal/api/v2/controllers_transactions_list.go(2 hunks)internal/api/v2/controllers_transactions_list_test.go(14 hunks)internal/api/v2/controllers_transactions_read.go(2 hunks)internal/api/v2/controllers_transactions_read_test.go(2 hunks)internal/api/v2/controllers_volumes.go(2 hunks)internal/api/v2/controllers_volumes_test.go(8 hunks)internal/api/v2/mocks_ledger_controller_test.go(10 hunks)internal/api/v2/mocks_system_controller_test.go(2 hunks)internal/controller/ledger/controller.go(2 hunks)internal/controller/ledger/controller_default.go(4 hunks)internal/controller/ledger/controller_default_test.go(10 hunks)internal/controller/ledger/controller_generated_test.go(10 hunks)internal/controller/ledger/controller_with_traces.go(10 hunks)internal/controller/ledger/errors.go(0 hunks)internal/controller/ledger/mocks.go(1 hunks)internal/controller/ledger/mocks_test.go(1 hunks)internal/controller/ledger/stats.go(2 hunks)internal/controller/ledger/stats_test.go(2 hunks)internal/controller/ledger/store.go(4 hunks)internal/controller/ledger/store_generated_test.go(7 hunks)internal/controller/system/controller.go(3 hunks)internal/controller/system/store.go(2 hunks)internal/storage/common/errors.go(1 hunks)internal/storage/common/metadata.go(1 hunks)internal/storage/common/paginator.go(1 hunks)internal/storage/common/paginator_column.go(5 hunks)internal/storage/common/paginator_offset.go(3 hunks)internal/storage/common/resource.go(1 hunks)internal/storage/driver/driver.go(2 hunks)internal/storage/driver/driver_test.go(1 hunks)internal/storage/driver/system_generated_test.go(2 hunks)internal/storage/ledger/accounts_test.go(23 hunks)internal/storage/ledger/adapters.go(2 hunks)internal/storage/ledger/balances_test.go(10 hunks)internal/storage/ledger/logs_test.go(4 hunks)internal/storage/ledger/moves_test.go(2 hunks)internal/storage/ledger/paginator.go(0 hunks)internal/storage/ledger/resource.go(0 hunks)internal/storage/ledger/resource_accounts.go(3 hunks)internal/storage/ledger/resource_aggregated_balances.go(5 hunks)internal/storage/ledger/resource_logs.go(1 hunks)internal/storage/ledger/resource_transactions.go(7 hunks)internal/storage/ledger/resource_volumes.go(10 hunks)internal/storage/ledger/store.go(2 hunks)internal/storage/ledger/transactions.go(0 hunks)internal/storage/ledger/transactions_test.go(13 hunks)internal/storage/ledger/volumes_test.go(30 hunks)internal/storage/system/resource_ledgers.go(1 hunks)internal/storage/system/store.go(2 hunks)internal/storage/system/store_test.go(3 hunks)internal/worker/async_block.go(2 hunks)pkg/client/docs/models/operations/v2listledgersrequest.md(1 hunks)pkg/client/docs/sdks/v2/README.md(1 hunks)pkg/client/models/operations/v2listledgers.go(2 hunks)pkg/client/v2.go(2 hunks)test/e2e/api_ledgers_list_test.go(2 hunks)test/e2e/api_transactions_list_test.go(3 hunks)
💤 Files with no reviewable changes (4)
- internal/storage/ledger/transactions.go
- internal/storage/ledger/resource.go
- internal/storage/ledger/paginator.go
- internal/controller/ledger/errors.go
🧰 Additional context used
🧬 Code Definitions (52)
internal/api/v2/controllers_accounts_list.go (2)
internal/storage/common/errors.go (1)
ErrInvalidQuery(5-7)internal/controller/ledger/errors.go (1)
ErrMissingFeature(93-95)
internal/api/v2/controllers_transactions_list.go (2)
internal/storage/common/errors.go (1)
ErrInvalidQuery(5-7)internal/controller/ledger/errors.go (1)
ErrMissingFeature(93-95)
internal/api/v1/controllers_accounts_count.go (2)
internal/storage/common/errors.go (1)
ErrInvalidQuery(5-7)internal/controller/ledger/errors.go (1)
ErrMissingFeature(93-95)
internal/api/v2/controllers_balances.go (2)
internal/storage/common/errors.go (1)
ErrInvalidQuery(5-7)internal/controller/ledger/errors.go (1)
ErrMissingFeature(93-95)
internal/api/v2/controllers_volumes.go (2)
internal/storage/common/errors.go (1)
ErrInvalidQuery(5-7)internal/controller/ledger/errors.go (1)
ErrMissingFeature(93-95)
internal/api/v2/controllers_balances_test.go (2)
internal/storage/common/resource.go (1)
ResourceQuery(358-364)internal/controller/ledger/store.go (1)
GetAggregatedVolumesOptions(141-143)
internal/api/v2/controllers_transactions_count.go (2)
internal/storage/common/errors.go (1)
ErrInvalidQuery(5-7)internal/controller/ledger/errors.go (1)
ErrMissingFeature(93-95)
internal/api/v1/controllers_transactions_count_test.go (1)
internal/storage/common/resource.go (1)
ResourceQuery(358-364)
internal/storage/ledger/balances_test.go (3)
internal/volumes.go (1)
AggregatedVolumes(157-159)internal/storage/common/resource.go (1)
ResourceQuery(358-364)internal/controller/ledger/store.go (1)
GetAggregatedVolumesOptions(141-143)
internal/api/v1/controllers_accounts_read_test.go (1)
internal/storage/common/resource.go (1)
ResourceQuery(358-364)
internal/storage/driver/driver_test.go (1)
internal/controller/ledger/store.go (1)
NewListLedgersQuery(90-99)
internal/api/v1/controllers_config.go (1)
internal/storage/common/resource.go (1)
ColumnPaginatedQuery(405-414)
internal/api/v2/controllers_transactions_read.go (1)
internal/storage/common/resource.go (1)
ResourceQuery(358-364)
internal/api/v1/controllers_accounts_read.go (1)
internal/storage/common/resource.go (1)
ResourceQuery(358-364)
internal/api/v2/controllers_accounts_count.go (2)
internal/storage/common/errors.go (1)
ErrInvalidQuery(5-7)internal/controller/ledger/errors.go (1)
ErrMissingFeature(93-95)
internal/storage/ledger/adapters.go (2)
internal/storage/common/resource.go (1)
Resource(392-395)internal/controller/ledger/store.go (1)
GetAggregatedVolumesOptions(141-143)
internal/api/v1/controllers_accounts_count_test.go (2)
internal/storage/common/resource.go (1)
ResourceQuery(358-364)internal/storage/common/errors.go (1)
ErrInvalidQuery(5-7)
internal/api/v1/controllers_logs_list_test.go (1)
internal/storage/common/resource.go (2)
ColumnPaginatedQuery(405-414)ResourceQuery(358-364)
internal/api/v2/controllers_transactions_list_test.go (1)
internal/storage/common/resource.go (2)
ColumnPaginatedQuery(405-414)ResourceQuery(358-364)
internal/controller/ledger/controller_default_test.go (1)
internal/storage/common/resource.go (3)
ColumnPaginatedQuery(405-414)OffsetPaginatedQuery(398-404)ResourceQuery(358-364)
internal/api/v2/controllers_ledgers_list_test.go (2)
internal/storage/common/resource.go (1)
ColumnPaginatedQuery(405-414)internal/storage/common/errors.go (1)
ErrInvalidQuery(5-7)
internal/api/v2/controllers_volumes_test.go (2)
internal/storage/common/resource.go (2)
OffsetPaginatedQuery(398-404)ResourceQuery(358-364)internal/controller/ledger/store.go (1)
GetVolumesOptions(145-148)
internal/storage/ledger/logs_test.go (1)
internal/storage/common/resource.go (2)
ColumnPaginatedQuery(405-414)ResourceQuery(358-364)
internal/controller/ledger/stats_test.go (2)
internal/controller/ledger/mocks_test.go (1)
NewMockPaginatedResource(225-229)internal/storage/common/resource.go (3)
ColumnPaginatedQuery(405-414)OffsetPaginatedQuery(398-404)ResourceQuery(358-364)
internal/api/v1/controllers_accounts_list_test.go (1)
internal/storage/common/resource.go (2)
OffsetPaginatedQuery(398-404)ResourceQuery(358-364)
internal/api/v2/controllers_accounts_read_test.go (1)
internal/storage/common/resource.go (1)
ResourceQuery(358-364)
internal/api/v2/controllers_accounts_count_test.go (2)
internal/storage/common/resource.go (1)
ResourceQuery(358-364)internal/storage/common/errors.go (1)
ErrInvalidQuery(5-7)
internal/api/v1/controllers_transactions_read_test.go (1)
internal/storage/common/resource.go (1)
ResourceQuery(358-364)
internal/api/v2/controllers_accounts_list_test.go (2)
internal/storage/common/resource.go (2)
OffsetPaginatedQuery(398-404)ResourceQuery(358-364)internal/storage/common/errors.go (1)
ErrInvalidQuery(5-7)
internal/api/v2/controllers_logs_list_test.go (3)
internal/storage/common/resource.go (2)
ColumnPaginatedQuery(405-414)ResourceQuery(358-364)internal/api/common/errors.go (1)
ErrValidation(15-15)internal/storage/common/errors.go (1)
ErrInvalidQuery(5-7)
internal/api/v2/controllers_logs_list.go (1)
internal/storage/common/errors.go (1)
ErrInvalidQuery(5-7)
internal/api/v1/controllers_balances_aggregates_test.go (2)
internal/storage/common/resource.go (1)
ResourceQuery(358-364)internal/controller/ledger/store.go (1)
GetAggregatedVolumesOptions(141-143)
internal/api/v2/controllers_transactions_count_test.go (2)
internal/storage/common/resource.go (1)
ResourceQuery(358-364)internal/storage/common/errors.go (1)
ErrInvalidQuery(5-7)
internal/storage/system/store_test.go (2)
internal/controller/ledger/store.go (1)
NewListLedgersQuery(90-99)internal/storage/common/resource.go (1)
ColumnPaginatedQuery(405-414)
internal/storage/ledger/transactions_test.go (3)
internal/transaction.go (3)
Transactions(15-17)Transaction(38-50)Transaction(52-59)internal/storage/common/resource.go (2)
ResourceQuery(358-364)ColumnPaginatedQuery(405-414)internal/storage/common/errors.go (1)
ErrInvalidQuery(5-7)
pkg/client/v2.go (1)
pkg/client/internal/utils/requestbody.go (1)
SerializeRequestBody(28-39)
test/e2e/api_transactions_list_test.go (1)
internal/storage/common/resource.go (2)
ColumnPaginatedQuery(405-414)ResourceQuery(358-364)
internal/api/v2/common.go (2)
internal/api/common/pagination.go (1)
PaginationConfig(3-6)internal/storage/common/resource.go (3)
OffsetPaginatedQuery(398-404)ColumnPaginatedQuery(405-414)ResourceQuery(358-364)
internal/storage/ledger/moves_test.go (3)
internal/volumes.go (1)
AggregatedVolumes(157-159)internal/storage/common/resource.go (1)
ResourceQuery(358-364)internal/controller/ledger/store.go (1)
GetAggregatedVolumesOptions(141-143)
internal/controller/system/store.go (1)
internal/storage/common/resource.go (1)
ColumnPaginatedQuery(405-414)
internal/storage/common/paginator_offset.go (2)
internal/storage/common/resource.go (1)
OffsetPaginatedQuery(398-404)internal/storage/common/paginator.go (1)
Paginator(8-11)
test/e2e/api_ledgers_list_test.go (1)
pkg/client/models/operations/v2listledgers.go (1)
V2ListLedgersRequest(9-20)
internal/api/v2/controllers_ledgers_list.go (4)
internal/controller/system/controller.go (1)
Controller(23-34)internal/api/common/pagination.go (1)
PaginationConfig(3-6)internal/storage/common/errors.go (1)
ErrInvalidQuery(5-7)internal/controller/ledger/errors.go (1)
ErrMissingFeature(93-95)
internal/storage/system/resource_ledgers.go (4)
internal/storage/system/store.go (2)
DefaultStore(34-36)New(125-129)internal/storage/common/resource.go (8)
Filter(58-63)PropertyValidator(40-42)AcceptOperators(49-56)RepositoryHandlerBuildContext(65-68)ResourceQuery(358-364)ConvertOperatorToSQL(18-33)JoinCondition(35-38)RepositoryHandler(84-90)internal/storage/common/metadata.go (1)
MetadataRegex(6-6)internal/storage/common/errors.go (1)
NewErrInvalidQuery(18-22)
internal/worker/async_block.go (2)
pkg/features/features.go (1)
FeatureHashLogs(20-20)internal/storage/common/resource.go (1)
ColumnPaginatedQuery(405-414)
internal/storage/ledger/resource_logs.go (1)
internal/storage/common/resource.go (6)
Filter(58-63)RepositoryHandlerBuildContext(65-68)ResourceQuery(358-364)ConvertOperatorToSQL(18-33)JoinCondition(35-38)RepositoryHandler(84-90)
internal/controller/ledger/controller_default.go (1)
internal/storage/common/resource.go (3)
ColumnPaginatedQuery(405-414)ResourceQuery(358-364)OffsetPaginatedQuery(398-404)
internal/storage/ledger/resource_accounts.go (5)
internal/storage/ledger/store.go (1)
Store(24-46)internal/storage/common/resource.go (8)
Filter(58-63)PropertyValidator(40-42)PropertyValidatorFunc(43-43)AcceptOperators(49-56)RepositoryHandlerBuildContext(65-68)ResourceQuery(358-364)JoinCondition(35-38)RepositoryHandler(84-90)internal/controller/ledger/errors.go (1)
NewErrMissingFeature(106-110)internal/storage/common/metadata.go (1)
MetadataRegex(6-6)internal/storage/common/errors.go (1)
NewErrInvalidQuery(18-22)
internal/api/v1/utils.go (1)
internal/storage/common/resource.go (3)
OffsetPaginatedQuery(398-404)ColumnPaginatedQuery(405-414)ResourceQuery(358-364)
internal/storage/ledger/resource_volumes.go (4)
internal/storage/ledger/store.go (2)
Store(24-46)New(161-248)internal/controller/ledger/store.go (2)
Store(31-66)GetVolumesOptions(145-148)internal/storage/common/resource.go (7)
Filter(58-63)PropertyValidator(40-42)PropertyValidatorFunc(43-43)AcceptOperators(49-56)RepositoryHandlerBuildContext(65-68)ResourceQuery(358-364)RepositoryHandler(84-90)internal/storage/common/metadata.go (1)
MetadataRegex(6-6)
internal/storage/ledger/resource_aggregated_balances.go (6)
internal/storage/ledger/store.go (2)
Store(24-46)New(161-248)internal/controller/system/store.go (1)
Store(14-21)internal/storage/common/resource.go (6)
Filter(58-63)PropertyValidator(40-42)RepositoryHandlerBuildContext(65-68)ResourceQuery(358-364)JoinCondition(35-38)RepositoryHandler(84-90)internal/storage/common/metadata.go (1)
MetadataRegex(6-6)internal/controller/ledger/errors.go (1)
NewErrMissingFeature(106-110)internal/storage/common/errors.go (1)
NewErrInvalidQuery(18-22)
internal/controller/ledger/mocks_test.go (3)
internal/controller/ledger/controller.go (1)
Controller(18-78)internal/controller/system/controller.go (1)
Controller(23-34)internal/storage/common/resource.go (5)
RepositoryHandlerBuildContext(65-68)ResourceQuery(358-364)JoinCondition(35-38)Filter(58-63)PaginatedQuery(415-417)
🪛 GitHub Actions: Default
internal/storage/driver/driver_test.go
[error] 103-103: "[{{} {706fd570 map[] } 1 35680cb9-a8f2-45ec-be7d-fb771578e4f7 .221392 +0000 UTC initializing}]" should have 2 item(s), but has 1
🪛 LanguageTool
docs/api/README.md
[uncategorized] ~136-~136: A punctuation mark might be missing here.
Context: ...uired|Description| |---|---|---|---|---| |pageSize|query|integer(int64)|false|The...
(AI_EN_LECTOR_MISSING_PUNCTUATION)
🔇 Additional comments (283)
internal/storage/ledger/adapters.go (2)
8-8: Added import reflects refactoring effort to centralize resource handling.The new import of the common package aligns with the PR objective to create a shared resources package for utilities across the system.
43-43: Return type change standardizes resource interfaces across the codebase.The method signature has been updated to use
common.Resourceinstead ofledgercontroller.Resourcewhile preserving the same generic type parameters. This change is part of the broader effort to standardize pagination and resource handling mentioned in the PR objectives.Ensure that all callers of this method have been updated to work with the new return type, although the interface methods should remain compatible based on the provided code snippets.
internal/storage/ledger/balances_test.go (1)
7-7: Refactoring: Consistent transition fromledgercontroller.ResourceQuerytocommon.ResourceQueryThese changes correctly implement the migration of query types from package-specific implementations to a unified common package. The refactoring is consistent across all test cases in the TestBalancesAggregates function, and the functionality remains unchanged while improving code maintainability.
Also applies to: 218-218, 242-242, 260-260, 279-279, 301-301, 320-320, 336-336, 347-347, 366-366
internal/storage/ledger/resource_aggregated_balances.go (6)
12-14: Store-based architecture looks goodThe refactoring to include the
storefield in the struct is a clean approach that provides access to both database and ledger functionality, improving encapsulation and maintainability.
16-49: Well-structured filter definitions with proper exportsThe conversion to exported method names and use of common package types aligns well with the PR's goal of creating a shared resources package. The filter validations correctly use the store's ledger reference and common metadata regex pattern.
51-133: BuildDataset implementation maintains functionality while improving architectureThe method successfully maintains all the conditional logic for features while updating to use the store for database and ledger access. All SQL queries are properly parameterized to prevent SQL injection.
135-152: Enhanced filter resolution with standardized error handlingThe ResolveFilter method appropriately uses common package utilities for metadata matching and error creation, improving consistency across the codebase.
158-171: Simplified Project method signatureRemoving the explicit store parameter and using the struct's store field results in cleaner code while maintaining the same functionality.
173-173: Proper interface compliance checkThe type assertion ensures the struct correctly implements the common.RepositoryHandler interface, which is a good practice for catching potential interface mismatches at compile time.
internal/storage/ledger/resource_transactions.go (7)
5-5: Good addition of common package importThis import aligns with the PR objective of creating a resources package for sharing utilities across the system package.
11-13: Good implementation of dependency injectionAdding the store field to the handler follows good dependency injection practices, allowing direct access to database and ledger properties without global references.
15-70: Well structured filter definitions using common packageThe method has been properly exported and standardized using the common package types. The validation logic is now more consistent across filters with proper use of
common.PropertyValidatorFuncandcommon.AcceptOperators.
72-124: Clean implementation of BuildDataset with store accessThe method now properly uses the store reference to access database and ledger properties, making the code more maintainable by centralizing store access while preserving the existing query construction logic.
126-156: Good standardization of filter resolutionThe method now uses common utilities like
common.ConvertOperatorToSQLandcommon.MetadataRegex, aligning with the standardization effort while preserving the existing filter logic.
158-160: Clean implementation of Project and Expand methodsBoth methods have been properly updated to use common package types in their signatures while maintaining the existing functionality. The Expand method now correctly uses the store for database access.
Also applies to: 162-193
195-195: Good use of compile-time interface verificationThe type assertion ensures at compile time that
transactionsResourceHandlerimplements thecommon.RepositoryHandler[any]interface, which is a good practice to catch implementation issues early.internal/storage/ledger/resource_volumes.go (14)
7-7: Import usage looks good.
The import of"github.com/formancehq/ledger/internal/storage/common"is aligned with the broader refactoring toward using thecommonpackage.
13-15: Ensurestoreis always properly initialized.
While embeddingstore *StorewithinvolumesResourceHandleris useful, it's important to verify in upstream code thatstoreis never nil to prevent runtime panics.Would you like me to generate a script to search for all constructors or call sites and confirm
storeis properly set?
17-18: Good introduction of Filters() method.
The newFilters()method provides a coherent list ofcommon.Filterobjects. This helps unify filtering logic in a single place, improving readability.
20-24: Confirm supported operators for address filter.
Right now, we rely onvalidateAddressFilter(h.store.ledger, operator, value). Ensure it gracefully handles common operators such as$eq,$in, etc. If additional operators are needed in the future, consider documenting them here.
29-30: Validate the use of$matchfor numeric filters.
common.AcceptOperators("$lt", "$gt", "$lte", "$gte", "$match")is acceptable, but$matchis typically used with strings. Verify we truly need string-based matching on numeric balances.
35-36:first_usagefilter logic looks consistent.
This mirrors the balance filter's operator set. The approach seems sound for filtering timestamps or usage metrics.
65-65:BuildDatasetmethod signature is clear.
Usingquery common.RepositoryHandlerBuildContext[ledgercontroller.GetVolumesOptions]clarifies the context for building queries. No issues found.
69-69: Validate partial address logic.
needAddressSegments := query.UseFilter("address", isPartialAddress)suggests a specialized check. EnsureisPartialAddressis well-documented or tested so future maintainers know the intended matching strategy.
103-103: Feature toggle approach is correct.
Early return ifFeatureMovesHistoryis off is an appropriate guard. The error handling is straightforward.
107-153: Ensure adequate test coverage for PIT and OOT queries.
The summation logic overmovestable is critical. Confirm that your integration tests verify both scenarios where PIT/OOT boundaries are valid or missing.
165-166:ResolveFiltermethod signature is consistent with new interface.
No immediate concerns. Consider adding doc comments or clarifying param names if the codebase supports further detail.
205-206:Projectmethod: well-structured for grouping columns.
The method’s signature and usage aligns well with the refactored approach, enabling grouped or ungrouped data.
214-219: Good approach to intermediate aggregation.
Wrapping the initial dataset intointermediateand then aggregating is clean. Keep an eye on query performance with large data sets.
232-232: Type assertion ensures the handler satisfiescommon.RepositoryHandler.
This is a neat final check guaranteeing the correct interface implementation.internal/storage/system/resource_ledgers.go (6)
19-46: The Filters implementation looks goodThe implementation correctly defines filters for "bucket", "features", "metadata", and "name" with appropriate operators for each. The inclusion of the "$like" operator for the "name" filter is particularly useful for substring searches.
48-52: BuildDataset implementation is straightforwardThe method correctly creates a base query for selecting ledger records. This provides a good foundation for further query modifications by the caller.
54-78: ResolveFilter implementation handles different filter types appropriatelyThe implementation correctly translates filter operators into SQL conditions for different property types. I particularly like the use of PostgreSQL's JSON containment operator for features and metadata filtering.
However, I have a small question about the limit in regex matching:
match := featuresRegex.FindAllStringSubmatch(property, 3)Is there a specific reason for limiting to 3 matches? This same limit is used for both features and metadata regex matching.
80-82: Project method implementation is simple and effectiveThe method returns all columns from the query, which is appropriate for this use case.
84-86: Expand method correctly indicates that no expansion is availableThis is a clear and appropriate implementation if ledger resources don't have meaningful relations to expand.
88-88: Good use of interface validationThe type assertion confirms that
ledgersResourceHandlerimplements thecommon.RepositoryHandler[any]interface, which is a good practice for catching implementation errors at compile time.internal/storage/common/metadata.go (1)
5-7: Well-structured regex definition for metadata extractionThe introduction of the
MetadataRegexvariable provides a centralized, reusable regex pattern for extracting metadata from query parameters. The regex pattern correctly uses capturing groups to extract content inside brackets following "metadata".internal/controller/ledger/mocks.go (1)
1-3: Appropriate mock generation setup for testingThe go:generate directives are well-structured to create mock implementations for the Resource and PaginatedResource interfaces. The use of flags like
-write_source_comment=falsekeeps the generated code clean.internal/api/v2/controllers_transactions_list.go (2)
9-9: Clean import of the new storage common packageAdding the storagecommon import is necessary for the error type migration and follows Go's convention of using aliased imports to avoid name conflicts.
31-31: Error handling correctly migrated to common packageThe change from
ledgercontroller.ErrInvalidQuery{}tostoragecommon.ErrInvalidQuery{}aligns with the refactoring effort to standardize error types across the codebase. Error handling behavior remains consistent.internal/controller/ledger/stats.go (2)
6-6: Appropriate import of common package for resource queriesThe addition of the common package import supports the migration to standardized resource query types.
17-17: Consistently migrated to common.ResourceQuery typeBoth transaction and account count operations now use the standardized
common.ResourceQuery[any]type, maintaining consistent behavior while aligning with the broader refactoring effort.Also applies to: 22-22
internal/api/v2/controllers_accounts_list.go (2)
8-8: Add import for storage common packageThe import for
storagecommonfrom the internal storage package has been correctly added to enable the updated error handling.
25-25: Updated error handling to use storage common error typeThe error check has been properly modified to reference
storagecommon.ErrInvalidQuery{}instead of the previousledgercontroller.ErrInvalidQuery{}. This change is part of the broader refactoring to standardize error handling by moving error types to the common storage package.internal/api/v2/controllers_balances.go (2)
4-4: Add import for storage common packageThe import for
storagecommonhas been properly added to allow referencing the error types in this package.
29-29: Updated error handling to use storage common error typeThe error handling has been correctly modified to use
storagecommon.ErrInvalidQuery{}instead ofledgercontroller.ErrInvalidQuery{}, which aligns with the refactoring effort to standardize error handling across the application.internal/api/v2/controllers_transactions_count.go (2)
5-5: Add import for storage common packageThe import for
storagecommonhas been correctly added to access the error types defined in this package.
25-25: Updated error handling to use storage common error typeThe error handling has been appropriately updated to check for
storagecommon.ErrInvalidQuery{}instead of using the previous error type from the ledger controller package. This change is consistent with the refactoring pattern observed in other files.internal/storage/common/paginator.go (2)
1-7: Well-structured package and importsThe package declaration and imports for this new file are clean and appropriate. The imports include the necessary bunpaginate library for cursor-based pagination and the bun library for database query handling.
8-11: Well-designed generic Paginator interfaceThe
Paginatorinterface is well-designed with generic type parameters that allow it to be used with different resource types and pagination options. The two methods provide a clean separation of concerns:
Paginate: Modifies a query to include pagination constraintsBuildCursor: Constructs a cursor from the query resultsThis interface will help standardize pagination handling across the application, making the code more maintainable and consistent.
internal/api/v2/controllers_accounts_read.go (2)
5-5: LGTM - Switched to the new storage common package.The import change from ledgercontroller to storagecommon is part of the broader refactoring to share utilities with the system package.
30-33: Parameter type updated correctly for ResourceQuery.This change correctly updates the GetAccount method parameter type from the legacy ledgercontroller to the new storagecommon package while maintaining the same functionality.
internal/api/v2/controllers_balances_test.go (2)
5-5: LGTM - Added import for the new storage common package.The import for storagecommon aligns with the changes being made to standardize query handling.
31-31: Successfully migrated test query structures to use common package.The test cases are consistently updated to use the new
storagecommon.ResourceQuerytype instead of the legacyledgercontroller.ResourceQuerytype, maintaining test behavior while aligning with the broader refactoring efforts.Also applies to: 39-43, 48-53, 58-63, 70-74, 82-88
internal/storage/ledger/volumes_test.go (2)
8-8: LGTM - Updated import to use the new storage common package.This import change is part of the refactoring to create a shared resources package.
109-112: Successfully standardized query types across test cases.All instances of OffsetPaginatedQuery and ResourceQuery have been consistently updated to use the common package, ensuring consistent query handling across the codebase.
Also applies to: 138-143, 168-174, 181-181, 188-194, 212-219, 226-232, 240-246, 264-268, 285-289, 296-304, 307-311, 328-335, 353-360, 378-383, 400-405, 423-430, 449-454, 463-468, 477-482, 549-555, 563-571, 578-586, 593-601, 609-618, 644-653, 669-679, 713-724, 734-745, 755-762
internal/api/v2/controllers_accounts_count.go (2)
5-5: LGTM - Added import for the storage common package.The import for storagecommon aligns with the changes being made to standardize query handling.
26-26: Properly updated error type reference.The error check now correctly refers to
storagecommon.ErrInvalidQuery{}instead of the legacy ledgercontroller version, while maintaining the same error handling logic. The retention ofledgercontroller.ErrMissingFeature{}is appropriate as that error is still specific to the controller package.internal/api/v2/controllers_volumes.go (2)
8-8: Added Import of Storage Common PackageAdding the import for
storagecommonis necessary to reference the migration of query error types from ledgercontroller to the common storage package. This change aligns with the PR objective of creating a resources package for sharing utilities.
56-56: Updated Error Type ReferenceThe error checking has been updated to check for
storagecommon.ErrInvalidQuery{}while maintaining the check forledgercontroller.ErrMissingFeature{}. This reflects the migration of the invalid query error type to the common storage package while the missing feature error remains in the ledger controller.internal/api/v2/controllers_transactions_count_test.go (4)
7-7: Added Import of Storage Common PackageAdding the import for
storagecommonis necessary to use the migrated ResourceQuery type and ErrInvalidQuery error from the common storage package.
32-33: Updated ResourceQuery Type ReferenceThe expectQuery field type has been changed to use
storagecommon.ResourceQuery[any]instead of the previous ledgercontroller version. This change is part of the standardization of resource types across the codebase.
43-47: Consistent Use of Migrated ResourceQuery TypeAll instances of ResourceQuery have been updated to use the new storagecommon package version. The test cases maintain the same structure and logic, with only the source package of the type changing. This systematic update ensures consistency throughout the test file.
Also applies to: 52-58, 62-68, 72-78, 82-88, 92-98, 102-108, 112-118, 121-124, 135-140, 147-150
135-135: Updated Error Type ReferenceThe test case now uses
storagecommon.ErrInvalidQuery{}instead of the ledgercontroller version, which aligns with the error type migration in the production code.internal/api/v2/controllers_transactions_read.go (2)
5-5: Added Import of Storage Common PackageThe import for storagecommon has been added while removing the ledgercontroller import (which is no longer shown in the diff). This change supports using the migrated ResourceQuery type.
30-34: Updated ResourceQuery Type ReferenceThe GetTransaction method now uses
storagecommon.ResourceQuery[any]instead of the previous ledgercontroller version. The structure of the query remains the same, maintaining the same functionality while using the standardized type from the common storage package.docs/api/README.md (1)
120-120: Content-Type header addition is necessary for the new request body.The addition of the Content-Type header is consistent with the changes to include a request body in the GET /v2 endpoint.
internal/api/v2/controllers_transactions_read_test.go (2)
5-5: Good addition of new import for standardized resource query.The addition of the storagecommon import aligns with the refactoring effort to standardize query handling across the codebase.
28-31: Appropriate update to ResourceQuery type.The change from the previous implementation to use
storagecommon.ResourceQuery[any]supports the goal of standardizing query handling across the system.internal/api/common/mocks_system_controller_test.go (2)
17-17: Appropriate addition of common package import.The addition of the common package import is necessary to support the updated ListLedgers method signature, which now uses the common.ColumnPaginatedQuery type.
104-104: Method signature updated consistently with interface changes.The ListLedgers method signature has been updated to use
common.ColumnPaginatedQuery[any]instead of the previous type. This change aligns with the interface definition updates in the controller package, ensuring consistency across the codebase.internal/controller/system/store.go (2)
5-5: Appropriate addition of common package import.The addition of the common package import is necessary to support the updated ListLedgers method signature, which now uses the common.ColumnPaginatedQuery type.
16-16: Interface method signature standardization.The ListLedgers method signature has been updated to use
common.ColumnPaginatedQuery[any]instead of the previous ledgercontroller type. This change centralizes pagination and query handling in the common package, promoting code reuse and consistency across the system.internal/storage/driver/driver.go (2)
7-7: Appropriate addition of common package import.The addition of the common package import is necessary to support the updated ListLedgers method signature, which now uses the common.ColumnPaginatedQuery type.
185-186: Implementation updated to use the new pagination pattern.The ListLedgers method has been updated to:
- Use
common.ColumnPaginatedQuery[any]as its parameter type- Call the new
Ledgers().Paginate()method patternThis change is consistent with the PR's objective to create a resources package for shared utilities and implement pagination on the ledgers list.
internal/storage/ledger/logs_test.go (3)
10-10: Appropriate addition of common package import.The addition of the common package import is necessary to support the updated method calls in the tests, which now use common.ColumnPaginatedQuery and common.ResourceQuery types.
133-133: Test updated to use new pagination type.The test has been updated to use
common.ColumnPaginatedQuery[any]instead of the previous type, ensuring consistency with the implementation changes.
223-223: Tests consistently updated to use new pagination and query types.All test cases in the TestLogsList function have been updated to use the common package's query types:
common.ColumnPaginatedQuery[any]for paginationcommon.ResourceQuery[any]for filteringThis ensures the tests remain consistent with the implementation changes and properly validate the new pagination functionality.
Also applies to: 230-231, 238-240, 253-255
internal/api/v1/controllers_config.go (1)
7-7: LGTM: Good refactoring to standardize query typesThe change to import
storagecommonand update the function signature to usestoragecommon.ColumnPaginatedQuery[any]instead ofledgercontroller.ListLedgersQueryaligns well with the refactoring effort to standardize query handling across the codebase. This promotes better code organization and reusability.Also applies to: 41-41
internal/storage/common/errors.go (1)
18-22: LGTM: Well-implemented error constructorThe
NewErrInvalidQueryfunction is well-implemented, providing a clean way to create formatted error messages.internal/api/v2/mocks_system_controller_test.go (1)
17-17: LGTM: Correctly updated mock to match interface changesThe changes properly update the mock to align with the refactored system controller interface, ensuring that tests will continue to function correctly with the new type signatures.
Also applies to: 104-104
internal/api/v2/controllers_accounts_read_test.go (1)
7-7: LGTM: Tests properly updated to reflect type changesThe test cases have been correctly updated to use
storagecommon.ResourceQuery[any]instead of the previous type, maintaining test coverage while aligning with the refactored implementation.Also applies to: 28-28, 41-43, 50-54, 63-67
internal/api/v1/controllers_accounts_read_test.go (5)
7-7: Type migration from ledgercontroller to storagecommon.The import change is part of the broader effort to create a unified resources package for sharing utilities, as mentioned in the PR objectives. This change maintains compatibility with the updated ResourceQuery type location.
29-29: Type update for expectQuery field.The field type has been updated from
ledgercontroller.ResourceQuery[any]tostoragecommon.ResourceQuery[any]to align with the moved ResourceQuery struct definition.
41-44: Updated ResourceQuery initialization.The expectQuery initialization now correctly uses the storagecommon package instead of ledgercontroller while maintaining the same functionality.
50-53: Updated ResourceQuery initialization.The expectQuery initialization now correctly uses the storagecommon package instead of ledgercontroller while maintaining the same functionality.
68-71: Updated ResourceQuery initialization.The expectQuery initialization now correctly uses the storagecommon package instead of ledgercontroller while maintaining the same functionality.
internal/api/v1/controllers_accounts_read.go (2)
5-5: Import update for ResourceQuery type.The storagecommon import replaces the previous ledgercontroller import, aligning with the PR objective to create a common resources package.
26-29: Updated GetAccount function parameter type.The ResourceQuery type has been correctly updated from ledgercontroller to storagecommon while maintaining the same structure and functionality.
test/e2e/api_ledgers_list_test.go (5)
30-31: Increased page size limits.The MaxPageSize and DefaultPageSize have been increased from 5 to 15, allowing for more comprehensive tests of the filtering functionality.
37-46: Enhanced test data generation.The test now creates 20 ledgers instead of 10, with additional metadata (bucket and foo attribute) that vary based on the index. This provides a more robust dataset for testing filtering capabilities.
50-56: Updated expectation for default listing.The test now correctly expects 15 ledgers to be returned (matching the new MaxPageSize) instead of 5.
57-80: Added filtering tests for bucket and metadata.New test cases verify that filtering works correctly for both bucket and metadata fields, which is a key feature added in this PR.
81-92: Added exact name filtering test.New test case to verify filtering by exact ledger name, ensuring that exactly one ledger is returned when querying for "ledger0".
internal/storage/ledger/moves_test.go (2)
8-8: Added import for common package.Added import for the storage/common package to support the updated ResourceQuery type.
174-178: Updated ResourceQuery type in GetOne function call.The ResourceQuery type has been updated from ledgercontroller to common while maintaining the same functionality with GetAggregatedVolumesOptions.
internal/api/v2/controllers_ledgers_list_test.go (3)
6-6: Good addition of the storagecommon import.This import supports the migration from ledgercontroller types to storagecommon types, aligning with the PR's goal of creating a shared resources package.
31-31: Appropriate type update for expectQuery.The change from
ledgercontroller.ListLedgersQuerytostoragecommon.ColumnPaginatedQuery[any]properly refactors the test to use the generalized query structure from the shared package.
73-73: Good update to error type.Changing from
ledgercontroller.ErrInvalidQuery{}tostoragecommon.ErrInvalidQuery{}maintains consistent error handling while using the centralized error type from the common package.internal/api/v2/controllers_ledgers_list.go (4)
5-5: Appropriate import addition.Adding the storagecommon import supports the transition to using common resource types.
18-18: Good refactoring to use the getColumnPaginatedQuery helper.Replacing direct use of bunpaginate.Extract with the getColumnPaginatedQuery helper function improves code consistency and reusability across the codebase. The column specification as "id" and setting order to ascending provides clear pagination behavior.
24-24: Good update to match the controller's expected parameter type.This change correctly updates the ListLedgers call to pass the new query structure.
27-27:❓ Verification inconclusive
Good error handling update.
The updated error check correctly handles both storagecommon.ErrInvalidQuery and ledgercontroller.ErrMissingFeature errors. Consider verifying if ledgercontroller.ErrMissingFeature should also be moved to the common package for complete consistency.
🏁 Script executed:
#!/bin/bash # Check if there are other occurrences of ErrMissingFeature in the codebase rg -g "*.go" "ErrMissingFeature" --countLength of output: 970
Attention: Verify Consistency of Error Handling
Good update on the error handling logic in
internal/api/v2/controllers_ledgers_list.go. The change now correctly checks for bothstoragecommon.ErrInvalidQueryandledgercontroller.ErrMissingFeature.
- I ran a search and found that
ErrMissingFeatureappears in multiple locations (e.g., in files underinternal/storage/ledger/andinternal/controller/ledger/), which suggests it is widely used across the codebase.- As a consequence, please evaluate whether moving
ledgercontroller.ErrMissingFeatureto a common package—similar to howstoragecommon.ErrInvalidQueryis managed—would improve the overall consistency and maintainability of our error handling strategy. If you decide against relocating it, a brief explanation in the documentation would help clarify the design choice.internal/api/v1/controllers_logs_list_test.go (4)
7-7: Good import addition.Adding the storagecommon import properly supports the refactoring to use common resource types.
30-30: Appropriate type update for expectQuery.Changing from a ledgercontroller type to
storagecommon.ColumnPaginatedQuery[any]aligns with the centralization of pagination types in the common package.
39-44: Good type migration in test cases.All test cases have been correctly updated to use the storagecommon types instead of ledgercontroller types, maintaining the same structure and functionality while leveraging the shared pagination utilities.
Also applies to: 50-57, 64-71
76-78: Good cursor type update.The cursor encoding and query type update are consistent with the other changes in the file, properly using the new storagecommon types.
internal/api/v1/controllers_accounts_list_test.go (3)
5-5: Good import addition.Adding the storagecommon import properly supports the transition to common resource types.
29-29: Appropriate type update for expectQuery.Changing from
ledgercontroller.OffsetPaginatedQuery[any]tostoragecommon.OffsetPaginatedQuery[any]correctly aligns with the resource package refactoring.
40-43: Comprehensive update to all test cases.All test cases have been thoroughly updated to use the new storagecommon types for both the main query structure and the nested Options field. This maintains test behavior while leveraging the centralized pagination and filtering utilities.
Also applies to: 50-55, 63-68, 73-77, 100-102, 111-116, 124-126
internal/controller/ledger/controller_default_test.go (10)
6-6: Updated import to use the new common package.This import change is part of the refactoring effort to consolidate resource query and pagination types into a unified common package.
218-219: Updated mock initialization and method calls to use common query types.All occurrences of pagination-related types have been consistently updated to use the standardized types from the common package. This change improves code consistency and maintainability by centralizing query type definitions.
Also applies to: 223-227, 231-235
247-250: Updated account-related mocks and queries to use common types.The mock initialization and method calls for accounts have been migrated to use the standardized query types from the common package, maintaining consistency with the overall refactoring.
Also applies to: 253-255
265-276: Transaction query types successfully migrated to common package.The transaction-related mock initialization and method calls have been updated to use the common package types. This is consistent with the overall refactoring strategy.
288-301: Account retrieval queries updated to use common types.GetAccount and related methods now properly use the common.ResourceQuery type. This standardizes the approach to resource queries across the codebase.
311-319: Standardized transaction counting using common query types.Count and related methods for transactions now use the common.ResourceQuery type, aligning with the refactoring goals.
329-344: ListAccounts method updated to use common OffsetPaginatedQuery.The account listing functionality now uses standardized pagination types from the common package, which will make the code more maintainable.
357-363: Aggregated balances queries migrated to common types.GetAggregatedBalances now uses the common.ResourceQuery type with appropriate generic parameters, maintaining type safety.
373-390: Log retrieval now uses standardized pagination types.ListLogs and related methods have been updated to use common.ColumnPaginatedQuery, ensuring consistency with other resource types.
400-415: Volume queries migrated to use common pagination types.GetVolumesWithBalances now uses common.OffsetPaginatedQuery with appropriate generic parameters, completing the standardization across all resource types.
internal/api/v2/controllers_logs_list.go (2)
8-8: Updated import to use the storage common package.This import change supports the refactoring effort to consolidate resource and error types into the common package.
25-26: Updated error type check for consistency.Changed from using ledgercontroller.ErrInvalidQuery to storagecommon.ErrInvalidQuery, maintaining the same error handling logic while using the consolidated error type.
internal/storage/ledger/accounts_test.go (9)
7-7: Added import for common package.This change supports the refactoring to use consolidated query types from the common package.
74-77: Updated account pagination queries to use common types.Multiple test cases for account listing have been refactored to use common.OffsetPaginatedQuery and common.ResourceQuery, ensuring consistency with the new type system.
Also applies to: 81-96, 103-115, 120-132
137-148: Effective volumes queries updated to use common types.Tests for retrieving accounts with volumes and effective volumes now use the standardized query types from the common package.
Also applies to: 152-164
168-188: Address filtering tests updated with common query types.Tests for filtering accounts by address have been refactored to use common.OffsetPaginatedQuery and common.ResourceQuery.
191-208: Balance filtering tests modernized with common query types.Test cases for filtering accounts by balance now use the consolidated query types from the common package.
Also applies to: 211-230
234-260: Metadata filtering and error handling updated.Tests for metadata filtering and invalid query error handling now use common types, with proper error type checking against common.ErrInvalidQuery.
265-272: Account retrieval tests refactored to use common query types.All test cases for retrieving individual accounts now use common.ResourceQuery, maintaining consistent type usage across the codebase.
Also applies to: 288-352
357-427: Point-in-time account retrieval using common query types.Tests for retrieving accounts at specific points in time have been updated to use the common query types, preserving the same functionality with standardized interfaces.
433-437: Account count tests migrated to common types.The account counting functionality has been updated to use common.ResourceQuery, completing the standardization across all account operations.
Also applies to: 451-454
internal/api/v1/controllers_accounts_count_test.go (4)
5-5: Added import for storage common package.This import change supports the refactoring to use consolidated query types from the common package.
28-28: Updated expectQuery type to use storage common package.Changed the type of the expectQuery field from ledgercontroller.ResourceQuery to storagecommon.ResourceQuery, aligning with the refactoring effort to consolidate query types.
39-40: Test cases updated to use standardized query types.All test cases have been refactored to use storagecommon.ResourceQuery, ensuring consistency with the new type system. This includes nominal cases, metadata filtering, address filtering, and balance filtering.
Also applies to: 48-50, 56-58, 66-67, 75-77, 84-85
93-94: Error case test vectors updated to use common types.Test cases for error handling now use storagecommon.ResourceQuery and appropriate error types, maintaining consistent error handling across the codebase.
Also applies to: 101-102
internal/api/v1/mocks_system_controller_test.go (2)
17-17: Good addition of the import for the common package.The import of the common package is necessary to support the updated method signature. This aligns with the broader effort to standardize resource queries across the codebase.
104-104: Method signature updated to use standardized query type.The
ListLedgersmethod now usescommon.ColumnPaginatedQuery[any]instead of the previous domain-specific query type. This change:
- Standardizes the query interface across controllers
- Enables more consistent handling of pagination and filtering
- Improves code reuse by leveraging the common package
This is in line with the PR objective to create a resources package for sharing utilities.
internal/api/v2/controllers_volumes_test.go (3)
6-6: Good addition of the import for storagecommon.Adding the import for the storage common package is necessary to support the updated test case structure and aligns with the standardization effort.
34-34: Standardized query type for improved code consistency.The test case structure now uses
storagecommon.OffsetPaginatedQueryinstead of the controller-specific type. This change enhances code maintainability by centralizing pagination and query handling.
43-49: Query definitions updated consistently throughout test cases.All test cases have been properly updated to use the standardized
storagecommonquery types while maintaining the original test logic and scenarios. This consistency ensures all pagination and filtering functionality continues to work as expected.Also applies to: 54-61, 66-73, 87-96, 101-108, 113-120
internal/api/v2/controllers_logs_list_test.go (4)
9-9: Good addition of the import for storagecommon.The import statement is correctly added to support the new standardized query types.
33-33: Standardized query type for consistency across the codebase.Changed from controller-specific type to the standardized
storagecommon.ColumnPaginatedQuery[any], which is part of the broader refactoring to improve code sharing and maintainability.
44-51: Test cases updated consistently to use the storage common types.All test scenarios have been properly migrated to use the standardized query and resource types from the common package while preserving the original test coverage and assertions.
Also applies to: 57-65, 71-79, 85-89, 91-95, 123-130, 138-144
133-133: Error type correctly updated to use common package.The error type has been updated from a controller-specific error to
storagecommon.ErrInvalidQuery{}, which is appropriate since error handling is now standardized in the common package.internal/api/v1/controllers_balances_aggregates_test.go (3)
4-4: Good addition of the import for storagecommon.The import statement is correctly added to support the standardized query types.
28-28: Standardized resource query type for improved consistency.The test case structure now uses
storagecommon.ResourceQueryinstead of the controller-specific type, which aligns with the broader refactoring effort to standardize resource querying.
34-38: Test cases properly updated to use the common resource query type.All test scenarios have been correctly migrated to use the standardized query types from the common package while maintaining the original test functionality and assertions.
Also applies to: 45-50
internal/api/v2/controllers_accounts_count_test.go (4)
6-6: Import added for storage common package.The import path for
storagecommonhas been added, which is necessary for the type changes in this file.
29-29: Type reference updated to use common package.The
expectQueryfield in thetestCasestruct now usesstoragecommon.ResourceQuery[any]instead ofledgercontroller.ResourceQuery[any], which aligns with the refactoring effort to move resource query types to a common package.
40-44: Resource query type consistently updated throughout test cases.All instances of
expectQueryin the test cases now usestoragecommon.ResourceQuery[any]for consistency, which is part of the broader refactoring to standardize resource query handling across the codebase.Also applies to: 50-54, 60-64, 70-74, 80-84, 98-101, 109-112, 120-123
97-97: Error type updated to use common package.The error return type has been updated from
ledgercontroller.ErrInvalidQuery{}tostoragecommon.ErrInvalidQuery{}, maintaining consistency with the refactoring approach.internal/storage/ledger/transactions_test.go (7)
10-10: Import added for common storage package.The import path for the common storage package has been added, which is required for the type changes in this file.
56-59: Updated resource query type in GetOne methods.The
GetOnemethod now usescommon.ResourceQuery[any]instead ofledgercontroller.ResourceQuery[any], improving consistency with the new shared resource utilities.Also applies to: 80-83
117-118: Consistently updated query type across various test methods.All instances of transaction querying methods across different test functions have been updated to use
common.ResourceQuery[any], ensuring consistent resource handling throughout the test suite.Also applies to: 197-199, 208-210, 723-726
153-157: Updated transaction metadata test to use common resource types.The transaction metadata test cases now use the common resource query type, maintaining consistency with the broader refactoring effort.
Also applies to: 161-164
455-459: Updated pagination query type for transaction listing.The
Paginatemethod now usescommon.ColumnPaginatedQuery[any]with nestedcommon.ResourceQuery[any]for options, providing a standardized approach to pagination and filtering that aligns with the PR objectives.
741-742: Consistent update of query types in test cases.All test cases for transaction listing now use
common.ColumnPaginatedQuery[any]with nestedcommon.ResourceQuery[any]for query options, ensuring a uniform approach to handling resource queries and pagination.Also applies to: 748-749, 753-756, 762-765, 771-774, 780-783, 789-792, 798-801, 807-810, 816-819, 825-829, 835-838, 844-847
803-803: Error type updated to use common package.The error return type has been updated from
ledgercontroller.ErrInvalidQuery{}tocommon.ErrInvalidQuery{}, maintaining consistency with the refactoring effort.internal/controller/ledger/stats_test.go (3)
4-4: Added import for common storage package.The import path for the common storage package has been added, which is necessary for the type changes in this file.
19-20: Updated mock resource types to use common package.The mock paginated resources now use
common.ColumnPaginatedQuery[any]andcommon.OffsetPaginatedQuery[any]types, ensuring consistency with the broader refactoring effort.
23-23: Updated resource query types in count method expectations.The mock expectations for the
Countmethod now usecommon.ResourceQuery[any], aligning with the standardized approach to resource queries introduced in this PR.Also applies to: 25-25
internal/storage/system/store.go (3)
13-13: Added import for common storage package.The import path for the common storage package has been added, which is essential for the new method signature and resource handler implementation.
21-21: Refactored ledger listing method in Store interface.The
ListLedgersmethod has been replaced with aLedgers()method that returns acommon.PaginatedResource. This is a significant improvement that aligns with the PR objective to add filtering on ledgers list by utilizing the new resources package.
94-102: Implemented new Ledgers method using common pagination utilities.The implementation of the
Ledgers()method now uses the common pagination utilities, including a resource handler and column paginator. This provides a more standardized and reusable approach to handling paginated ledger resources.This change is central to the PR objective of adding filtering capability to the ledgers list, as it leverages the new resources package for consistent pagination and filtering.
internal/api/v2/controllers_accounts_list_test.go (4)
6-6: Appropriate addition of common storage packageThe import statement correctly introduces the storage common package which will be used to replace the ledgercontroller's pagination and query types with their new location.
32-33: Type migration from ledgercontroller to storagecommonThe expectQuery type has been properly updated to use the storage common package's implementation of OffsetPaginatedQuery instead of the previous ledgercontroller version. This aligns with the PR objective of sharing utilities through a common resources package.
43-49: Consistent update of query types in test casesThe test case correctly updates both the OffsetPaginatedQuery and its nested ResourceQuery types to use the storagecommon versions, maintaining the same test logic while using the newly refactored types.
159-160: Error type correctly migratedThe error type has been properly updated from ledgercontroller.ErrInvalidQuery to storagecommon.ErrInvalidQuery, maintaining consistent error handling across the refactor.
test/e2e/api_transactions_list_test.go (3)
12-12: Appropriate import of common packageThe import correctly introduces the internal/storage/common package which contains the refactored pagination and query types.
258-264: Consistent cursor type usage in test assertionsThe test appropriately updates the cursor type to common.ColumnPaginatedQuery and updates the expectations to use common.ResourceQuery types, maintaining the same test assertions while using the refactored types.
301-310: Type consistency maintained in complex query testsThe test correctly updates the advanced query scenario to use the common package types while preserving the existing test logic for verifying complex queries with AND conditions.
internal/worker/async_block.go (2)
8-12: Added necessary imports for refactored query handlingThe additions of the query package and storage common package imports support the refactored query construction and pagination interface.
78-80: Updated pagination interface with improved API designThe pagination function signature has been updated to use common.ColumnPaginatedQuery and now uses the more fluent Ledgers().Paginate() method, which is more consistent with object-oriented design patterns. This change maintains functionality while simplifying the API structure.
internal/controller/system/controller.go (3)
5-5: Added storage common package importThe import correctly introduces the storage common package which contains the refactored pagination and query types.
26-26: Updated interface method signature with common typeThe Controller interface has been properly updated to use common.ColumnPaginatedQuery instead of the previous ledgercontroller type, ensuring consistent type usage across the codebase.
126-126: Implementation correctly aligned with interface changeThe DefaultController implementation has been properly updated to match the interface change, ensuring type consistency throughout the controller hierarchy.
internal/api/v1/utils.go (7)
4-4: Good addition of the storage common import.This import aligns with the overall refactoring of moving pagination and resource query functionality to the unified
storagecommonpackage.
28-29: Clean type transition to storagecommon.The function signature update appropriately changes the return type from
(*ledgercontroller.OffsetPaginatedQuery[v], error)to(*storagecommon.OffsetPaginatedQuery[v], error)while maintaining the same functionality. This is part of the overall refactoring to centralize pagination and resource query handling.
40-43: Correctly updated return type to use storagecommon.The struct initialization now properly uses the
storagecommon.OffsetPaginatedQuery[v]type, maintaining field consistency with the previous implementation.
47-48: Clean type transition for getColumnPaginatedQuery.The function signature has been correctly updated to return
(*storagecommon.ColumnPaginatedQuery[v], error)instead of the ledgercontroller equivalent.
59-64: Proper update to return storagecommon type.The struct initialization now correctly uses
storagecommon.ColumnPaginatedQuery[v]while maintaining all the existing field values.
68-69: Clean type transition for getResourceQuery.The function signature has been appropriately updated to return
(*storagecommon.ResourceQuery[v], error).
76-79: Correct implementation of ResourceQuery return type.The update maintains the same field structure while transitioning to the new
storagecommon.ResourceQuery[v]type.internal/api/v2/common.go (7)
6-6: Good addition of the storage common import.This import supports the refactoring effort to move pagination and resource query types to the unified
storagecommonpackage.
65-66: Clean type transition for getOffsetPaginatedQuery.The function signature has been correctly updated to return
(*storagecommon.OffsetPaginatedQuery[v], error)instead of using the ledgercontroller type.
81-84: Properly updated return type to use storagecommon.The struct initialization now correctly uses
storagecommon.OffsetPaginatedQuery[v], maintaining consistency with the refactoring goals.
88-89: Clean type transition for getColumnPaginatedQuery.The function signature has been appropriately updated to return
(*storagecommon.ColumnPaginatedQuery[v], error).
104-109: Correctly implemented return with storagecommon type.The struct initialization properly uses
storagecommon.ColumnPaginatedQuery[v]while maintaining all existing field assignments.
113-113: Clean type transition for getResourceQuery.The function signature has been appropriately updated to return
(*storagecommon.ResourceQuery[v], error).
134-140: Properly implemented return with new ResourceQuery type.The struct initialization now correctly uses
storagecommon.ResourceQuery[v]while maintaining all the existing field assignments (PIT, OOT, Builder, Expand, and Opts).internal/storage/ledger/store.go (6)
12-12: Good addition of the common package import.This import supports the refactoring effort to centralize pagination and resource handling in the common package.
48-56: Well-executed return type and implementation transition for Volumes.The method signature has been updated to return
common.PaginatedResourceinstead ofledgercontroller.PaginatedResourcewhile maintaining the same type parameters. The implementation now correctly usescommon.NewPaginatedResourceRepositoryandcommon.OffsetPaginatorwith appropriate parameters.
58-62: Clean transition for AggregatedVolumes method.The method now returns
common.Resourceand usescommon.NewResourceRepository, maintaining the same functionality while aligning with the refactoring goals.
64-72: Properly updated Transactions method.The return type has been changed to
common.PaginatedResourceand the implementation now usescommon.NewPaginatedResourceRepositoryandcommon.ColumnPaginatorwith appropriate parameters.
74-84: Well-executed transition for Logs method.The method signature has been updated to return
common.PaginatedResourceand the implementation now correctly usescommon.NewPaginatedResourceRepositoryMapperandcommon.ColumnPaginator.
86-96: Properly updated Accounts method.The return type has been changed to
common.PaginatedResourceand the implementation now usescommon.NewPaginatedResourceRepositoryandcommon.OffsetPaginatorwith appropriate parameters.internal/storage/common/paginator_offset.go (8)
1-1: Good package renaming from ledger to common.This change supports the refactoring effort to centralize pagination and resource handling functionality in a common package that can be used across the codebase.
10-13: Well-executed type and field visibility changes.Renaming from
offsetPaginatortoOffsetPaginatorand making the fields public (DefaultPaginationColumnandDefaultOrder) allows this type to be properly exported and used by other packages, which aligns with the refactoring goals.
16-16: Good method visibility update for Paginate.The method has been renamed from
paginatetoPaginateand its receiver updated toOffsetPaginator, making it accessible from other packages while maintaining its functionality.
18-20: Correctly updated field references.The references to struct fields have been updated to use
DefaultPaginationColumnandDefaultOrderinstead of the previous lowercase versions.
42-42: Good method visibility update for BuildCursor.The method has been renamed from
buildCursortoBuildCursorand its receiver updated toOffsetPaginator, making it accessible from other packages.
44-44: Properly updated variable type declarations.The variable declarations for
previousandnextnow correctly useOffsetPaginatedQuery[OptionsType]instead of the previous ledgercontroller type.
72-74: Correctly updated cursor encoding calls.The calls to
encodeCursornow use the proper type parameters[OptionsType, OffsetPaginatedQuery[OptionsType]]to align with the refactored types.
78-78: Proper interface implementation verification.The interface check statement has been updated to verify that
OffsetPaginatorimplements thePaginatorinterface with the correct type parameters.internal/storage/common/resource.go (2)
1-16: No immediate issues with importsAll imported packages appear relevant. However, ensure that "github.com/formancehq/go-libs/v2/time" addresses needs beyond the standard library's "time" package.
35-48: Interfaces and function types look goodThe
JoinConditionstruct andPropertyValidatorinterface are straightforward. No issues noted.internal/storage/common/paginator_column.go (4)
1-13: Package declaration and importsEverything looks fine. The standard library and third-party imports are relevant.
14-19: Public fields for column paginatorExposing
DefaultPaginationColumnandDefaultOrderis a good design choice for configurability.
63-140: Correctly checks for next/previous cursors
BuildCursorlogic is well structured for constructing next and previous pagination tokens. Ensure thatReverselogic is thoroughly tested for boundary cases.Would you like an automated script to verify the reversing logic with various test data sets?
238-244: Cursor encodingUsing
bunpaginate.EncodeCursoris a neat approach. Just confirm that the encoding does not reveal sensitive details inadvertently.internal/controller/ledger/controller_with_traces.go (10)
7-7: ImportsSwitching to
commonfor queries aligns with the new pagination and resource framework.
209-219: ListTransactions now uses ColumnPaginatedQueryThe updated signature ensures consistent pagination logic. Looks good.
221-231: CountTransactions with ResourceQueryMigrating to
common.ResourceQueryunifies filter handling. No issues noted.
233-243: GetTransactionSignature update to
common.ResourceQuery[any]properly consolidates query logic.
245-255: CountAccountsAdapting from local ResourceQuery to
common.ResourceQueryis consistent with the overall refactor.
257-267: ListAccountsUpdated to use
common.OffsetPaginatedQuery[any]. Ensure all references are properly updated.
269-279: GetAccountGood alignment with the new common types.
281-291: GetAggregatedBalancesSwitching to
common.ResourceQuery[GetAggregatedVolumesOptions]unifies code style.
293-303: ListLogsConsistent usage of
common.ColumnPaginatedQueryensures uniform pagination approach.
341-351: GetVolumesWithBalancesUsing
common.OffsetPaginatedQuery[GetVolumesOptions]standardizes the pagination interface.internal/controller/ledger/store_generated_test.go (7)
5-5: No functional code changes.
This line only updates the mock generation command comment, so there is no impact on functionality.
19-19: Use of 'common' import is appropriate.
Switching the import reference aligns with the new resource abstraction incommon.
49-53: RefactoredAccounts()to return newPaginatedResourcetype.
The method signature now reflects the unified resource-based approach, which improves consistency with other return types.
63-67:AggregatedBalances()refactored to utilizecommon.Resource.
Changing tocommon.Resourceprovides a standardized interface for resource retrieval and manipulations.
238-243:Logs()now returns the newPaginatedResourcetype.
Consistent with the overall refactoring, this ensures logs are handled similarly to other resources.
297-302:Transactions()updated to acommon.PaginatedResource.
This improves consistency and leverages shared pagination logic.
360-365:Volumes()updated for unified pagination.
Switching tocommon.PaginatedResourcestreamlines the code and centralizes pagination concerns.internal/controller/ledger/store.go (5)
6-8: Added external dependencies for pointer, bunpaginate, and common.
These imports are necessary for the new pagination approach and pointer utility methods.
61-65: Refactored store interface methods to return unified resource constructs.
AdoptingPaginatedResourceandResourcefromcommoncentralizes pagination and resource querying.
73-73: Accommodates new resource-based fetch usingGetOne.
This code properly retrieves a single account by address through the new common resource query interface.
90-96: IntroducedNewListLedgersQueryfor column-based pagination.
Returning aColumnPaginatedQuerywith sensible defaults (sorting by ID ascending) consolidates pagination details.
129-129: Fetches account usingAccounts().GetOneunder the new pagination framework.
This leverages the updated resource approach consistently.internal/controller/ledger/controller.go (2)
8-8: Switched import tocommon.
This aligns with the broader refactor centralizing query logic in thecommonpackage.
29-37: Refactored method signatures to usecommon.*Querytypes.
Adoptingcommon.ResourceQuery,common.OffsetPaginatedQuery, andcommon.ColumnPaginatedQueryclarifies pagination and resource retrieval, simplifying maintenance.internal/storage/ledger/resource_logs.go (6)
10-12: Structural improvement with the addition of the store field.The addition of the
storefield to thelogsResourceHandlerstruct provides a cleaner approach to accessing store-related functionality, reducing the need to pass the store as a parameter to each method.
14-24: Method signature standardization and improved type safety.The renamed and exported
Filters()method with its updated return type to[]common.Filterimproves interface consistency across the codebase. The standardized naming and exported fields (Nameinstead ofname) follow proper Go conventions for exported methods.
26-31: Refactored BuildDataset with improved encapsulation.The method now uses the stored reference to access the database and build the query, which is a cleaner approach. The signature change to accept
common.RepositoryHandlerBuildContext[any]aligns with the standardized interfaces.
33-40: Standardized filter resolution with common package utilities.The
ResolveFiltermethod now usescommon.ConvertOperatorToSQLinstead of a local implementation, promoting code reuse and standardization across the codebase.
42-48: Properly exported and standardized expansion method.The
Expandmethod has been properly exported and updated to use standardized interfaces from the common package, maintaining consistent error handling for unsupported expansions.
50-50: Updated interface implementation assertion.The assertion has been updated to reflect the new interface type from the common package, ensuring type safety at compile time.
internal/storage/ledger/resource_accounts.go (6)
12-14: Structure enhancement with store field.Similar to the logs handler, adding the
storefield to theaccountsResourceHandlerstruct eliminates the need to pass store references through parameters, improving encapsulation.
16-51: Improved filter definitions with standardized validators.The exported
Filters()method now returns[]common.Filterwith properly structured validators. This leverages the common package's validator system, including helper functions likecommon.AcceptOperators, which improves code reuse and maintainability.
53-86: Enhanced dataset building with proper store access.The
BuildDatasetmethod now properly utilizes the stored reference to access database functionality. The conditional logic for handling point-in-time (PIT) queries and feature flags is cleaner with direct access toh.store.ledger.HasFeature.
88-135: Standardized filter resolution with improved error handling.The
ResolveFiltermethod now uses standardized components from the common package:
common.ConvertOperatorToSQLfor operator conversionscommon.MetadataRegexfor metadata field matchingcommon.NewErrInvalidQueryfor consistent error creationThis improves consistency across the codebase and reduces duplication.
141-187: Comprehensive expansion implementation with proper feature checks.The
Expandmethod now properly checks feature availability before allowing certain operations usingh.store.ledger.HasFeature. The standardized return type with*common.JoinConditionimproves interface consistency.
189-189: Updated interface assertion for type safety.The assertion has been updated to use the common package interface type, ensuring proper implementation at compile time.
internal/api/v2/mocks_ledger_controller_test.go (2)
19-19: Added common package import.This import is necessary to support the updated method signatures that now use types from the common package instead of package-specific types.
78-85: Standardized query parameter types across controller methods.All controller methods that accept queries have been updated to use standardized types from the common package:
CountAccounts,CountTransactions,GetAccount,GetTransactionnow usecommon.ResourceQuery[any]GetAggregatedBalancesusescommon.ResourceQuery[ledger0.GetAggregatedVolumesOptions]GetVolumesWithBalancesusescommon.OffsetPaginatedQuery[ledger0.GetVolumesOptions]ListAccountsusescommon.OffsetPaginatedQuery[any]ListLogsandListTransactionsusecommon.ColumnPaginatedQuery[any]This standardization improves consistency and reduces duplication by centralizing query handling logic.
Also applies to: 93-100, 168-175, 183-190, 228-235, 243-250, 287-294, 302-309, 317-324
internal/controller/ledger/controller_generated_test.go (2)
18-18: Added common package import for the mock controller.This import supports the updated method signatures that now use query types from the common package.
78-84: Standardized query types in mock controller methods.All mock controller methods have been updated to use standardized query types from the common package, maintaining consistency with the actual controller implementation. This ensures that mocks accurately reflect the real implementation and reduces maintenance overhead when interfaces change.
Also applies to: 93-99, 168-174, 183-189, 228-234, 243-249, 287-293, 302-308, 317-323
internal/api/v1/mocks_ledger_controller_test.go (1)
19-19: Well-structured refactoring of query types.The consistent migration of query types from
ledger0package tocommonpackage across all controller methods improves code organization and maintainability. This aligns well with the PR objective of creating a resources package for sharing utilities.Also applies to: 79-79, 94-94, 169-169, 184-184, 229-229, 244-244, 288-288, 303-303, 318-318
internal/api/common/mocks_ledger_controller_test.go (1)
19-19: Consistent type migration across mock implementations.The query type refactoring is consistently applied here as well, matching the changes in other mock files. This systematic approach ensures that all controller implementations and mocks use the unified type system from the common package.
Also applies to: 79-79, 94-94, 169-169, 184-184, 229-229, 244-244, 288-288, 303-303, 318-318
internal/controller/ledger/controller_default.go (1)
7-7: Well-executed refactoring of controller implementation.The DefaultController implementation has been properly updated to use the common package's query types across all relevant methods. This provides a more cohesive API and better separation of concerns by centralizing query-related types in the common package.
Also applies to: 122-122, 126-126, 130-130, 134-134, 138-138, 142-142, 146-146, 154-154, 158-158, 167-167
internal/controller/ledger/mocks_test.go (1)
1-280: Good addition of comprehensive mock implementations.These newly added mocks provide the necessary test infrastructure for the refactored query types. The mock implementations cover all the key interfaces (PropertyValidator, RepositoryHandler, Resource, and PaginatedResource) and properly utilize Go's generics to maintain type safety during testing.
pkg/client/docs/models/operations/v2listledgersrequest.md (1)
10-10: New required RequestBody field addedThe addition of a required
RequestBodyfield of typemap[string]*any*indicates this API now supports filtering capabilities for ledger listing. However, the description for this field is marked as "N/A", which provides insufficient guidance for API consumers.Consider enhancing the documentation with:
- A description of what this field is used for (filtering ledgers)
- Expected format of keys and values
- Example values demonstrating common filtering scenarios
internal/api/v1/controllers_accounts_count.go (2)
5-5: Package dependency moved from ledgercontroller to storagecommonThe import of the
storagecommonpackage aligns with the PR objective of creating a resources package to share utilities with the system package.
32-32:✅ Verification successful
Error type moved from ledgercontroller to storagecommon
The error handling now checks for
storagecommon.ErrInvalidQuery{}instead ofledgercontroller.ErrInvalidQuery{}, consistent with the package dependency change.
🏁 Script executed:
#!/bin/bash # Verify ErrInvalidQuery is defined in the correct package and has the same structure echo "Checking ErrInvalidQuery definitions..." rg -A 5 "type ErrInvalidQuery" --type goLength of output: 436
Verification passed –
ErrInvalidQuerynow correctly sourced fromstoragecommonThe change to check for
storagecommon.ErrInvalidQuery{}is confirmed. The definition was found ininternal/storage/common/errors.goand matches the expected structure. The remaining error check forledgercontroller.ErrMissingFeature{}appears intentional.
- File:
internal/api/v1/controllers_accounts_count.go, line 32pkg/client/models/operations/v2listledgers.go (2)
18-19: Enhancement of the V2ListLedgersRequest to support filteringThe addition of the
RequestBodyfield of typemap[string]anywith theapplication/jsonmedia type annotation allows for more flexible filtering options when listing ledgers. This aligns with the PR objective to add filtering capabilities to the ledgers list endpoint.
36-41: Well-structured GetRequestBody method implementationThe implementation follows the same pattern as other getter methods in this struct, handling nil receivers gracefully by returning an empty map. This maintains consistency throughout the codebase and provides a safe way to access the request body.
internal/api/v1/controllers_transactions_count_test.go (3)
4-5: Updated import to use the new common package for resource queriesThis change aligns with the PR objective of creating a resources package for sharing utilities, moving from ledgercontroller to storagecommon package.
25-26: Updated ResourceQuery type referenceThe test case struct has been updated to use
storagecommon.ResourceQuery[any]instead of the previous implementation, consistent with the migration of resource handling to the common package.
31-98: Systematic update of all query type referencesAll instances of resource queries in the test cases have been updated to use the new
storagecommon.ResourceQuery[any]type. The implementation is thorough and consistent, ensuring all test cases remain functional with the new type.internal/storage/system/store_test.go (3)
15-15: Added import for the common packageImporting the storage/common package which contains the shared resource utilities as described in the PR objectives.
95-95: Updated ledgers listing approach using paginate patternThe test now uses the new
store.Ledgers().Paginate()method withledgercontroller.NewListLedgersQuery(pageSize)instead of directly callingstore.ListLedgers. This change reflects the implementation of standardized pagination on the ledgers list, as noted in the PR objectives.
101-105: Updated query type and pagination approachThe test now uses
common.ColumnPaginatedQuery[any]and properly handles the cursor-based pagination with the updated API. This is in line with the PR's goal to implement a more structured pagination system for ledgers.internal/api/v2/controllers_transactions_list_test.go (3)
8-8: Updated import for storagecommon packageImporting the storage common package which contains the shared resource utilities, consistent with the PR objectives.
32-33: Updated ColumnPaginatedQuery type referenceThe test case struct has been updated to use
storagecommon.ColumnPaginatedQuery[any]instead of the previous implementation from ledgercontroller. This aligns with the migration to a common resources package.
41-49: Comprehensive update of all ColumnPaginatedQuery referencesAll test cases using ColumnPaginatedQuery and ResourceQuery types have been systematically updated to use the new types from the storagecommon package. This change is thorough and consistent throughout the file, maintaining test functionality while adapting to the new structure.
Also applies to: 54-63, 68-77, 82-91, 96-105, 110-119, 124-133, 138-147, 152-155, 177-185, 191-199, 201-208, 213-222, 227-235
internal/api/v1/controllers_transactions_read_test.go (1)
5-5: Clean refactoring of ResourceQuery typeThe migration from using
ledgercontroller.ResourceQuerytostoragecommon.ResourceQueryis well-executed and aligns with the PR objectives of centralizing shared utilities in a common package. The test functionality remains unchanged while benefiting from the improved code organization.Also applies to: 28-28
internal/api/v1/controllers_transactions_list_test.go (1)
6-6: Consistent migration to unified resource query typesThe systematic replacement of
ledgercontroller.ColumnPaginatedQuery[any]andledgercontroller.ResourceQuery[any]with their counterparts fromstoragecommonis well-executed across all test cases. This refactoring improves code organization by centralizing common types in a dedicated package while maintaining the same test behavior.Also applies to: 30-30, 39-46, 53-61, 68-76, 83-91, 98-106, 113-121, 128-136, 143-151, 156-162, 185-192
pkg/client/v2.go (4)
61-64: Good implementation of request body serializationThe addition of request body serialization enables the filtering functionality on the ledgers list. This is consistent with how other methods in the file handle request bodies.
77-77: Proper integration of serialized bodyUpdating the request creation to include the serialized body reader properly implements the body sending functionality.
83-85: Appropriate setting of Content-Type headerThe conditional setting of the Content-Type header ensures the server can properly interpret the request body format when present, while maintaining compatibility when no body is sent.
61-85:✅ Verification successful
Verify HTTP GET with request body compatibility
The implementation uses an HTTP GET request with a request body. While this is technically allowed by the HTTP specification, some HTTP clients, proxies, or servers might have issues with this approach.
🏁 Script executed:
#!/bin/bash # Checking if similar pattern is used consistently in the codebase echo "Checking for other GET methods with request bodies:" grep -A 2 -B 2 'NewRequestWithContext(ctx, "GET"' --include="*.go" -r . | grep 'bodyReader'Length of output: 744
Action: Confirm GET Request with Body Pattern in pkg/client/v2.go
After running the search, we verified that the only instance of constructing a GET request with a request body exists in
pkg/client/v2.go. Although including a body in a GET request is technically valid per the HTTP specification, it may not be supported by all HTTP clients, proxies, or servers. Since this pattern is isolated and appears intentional, no immediate changes are required. However, please ensure that:
- This design choice is documented.
- Downstream integrations are tested for compatibility with GET requests that include a body.
internal/storage/driver/system_generated_test.go (3)
17-17: New import for common package adds essential query handling typesThe addition of the common package import introduces standardized query types that are now used throughout the codebase for consistent resource handling.
136-142: Method signature simplified for better resource handlingThe
Ledgersmethod replaces the previousListLedgersmethod with a more streamlined approach that returns aPaginatedResource. This change aligns with the PR objective of creating a shared resources package for utilities.
144-148: Updated mock recorder for the new method signatureThe mock recorder for
Ledgershas been correctly updated to match the new method signature, removing parameters and updating the return type check.internal/api/bulking/mocks_ledger_controller_test.go (6)
19-19: New import for common package supports refactored query typesThe addition of the common package import supports the migration of query types from the ledger0 package to the common package, enabling shared utilities across the system.
79-79: Standardized ResourceQuery type usageMethod signatures have been updated to use
common.ResourceQuery[any]instead ofledger0.ResourceQuery[any], aligning with the refactoring goals of centralizing resource handling utilities.Also applies to: 94-94, 169-169, 229-229
184-184: Updated parameterized ResourceQuery for GetAggregatedBalancesThe
GetAggregatedBalancesmethod signature has been updated to use the common package's ResourceQuery while maintaining the specific options type from ledger0.
244-244: Standardized OffsetPaginatedQuery for volume operationsThe
GetVolumesWithBalancesmethod now usescommon.OffsetPaginatedQuerywhile preserving the specialized options type, ensuring consistent pagination behavior across the system.
288-288: Updated ListAccounts to use standardized paginationThe
ListAccountsmethod now uses the common package's OffsetPaginatedQuery type, maintaining a consistent approach to pagination across different resources.
303-303: Standardized ColumnPaginatedQuery for listing operationsThe
ListLogsandListTransactionsmethods have been updated to usecommon.ColumnPaginatedQuery[any], ensuring consistency in pagination handling across the codebase.Also applies to: 318-318
gfyrag
force-pushed
the
feat/ledgers-endpoint-filtering
branch
from
762a196 to
9bc73b3
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #834 +/- ##
==========================================
+ Coverage 82.04% 82.28% +0.23%
==========================================
Files 137 139 +2
Lines 7532 7536 +4
==========================================
+ Hits 6180 6201 +21
+ Misses 1036 1021 -15
+ Partials 316 314 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/storage/system/resource_ledgers.go (2)
58-63:⚠️ Potential issuePotential index out of bounds error in features filtering
The code doesn't check if the regex match contains elements before accessing
match[0][1], which could cause a panic if the regex doesn't match as expected.Fix by adding validation:
match := featuresRegex.FindAllStringSubmatch(property, 3) +if len(match) == 0 || len(match[0]) < 2 { + return "", nil, common.NewErrInvalidQuery("invalid feature property format %s", property) +} return "features @> ?", []any{map[string]any{ match[0][1]: value, }}, nil
64-69:⚠️ Potential issuePotential index out of bounds error in metadata filtering
The code doesn't check if the regex match contains elements before accessing
match[0][1], which could cause a panic if the regex doesn't match as expected.Fix by adding validation:
match := common.MetadataRegex.FindAllStringSubmatch(property, 3) +if len(match) == 0 || len(match[0]) < 2 { + return "", nil, common.NewErrInvalidQuery("invalid metadata property format %s", property) +} return "metadata @> ?", []any{map[string]any{ match[0][1]: value, }}, nil
🧹 Nitpick comments (6)
internal/controller/ledger/store.go (2)
61-65: Method signatures updated to use common package typesThe Store interface methods have been consistently updated to use types from the
commonpackage. This refactoring aligns with the PR objective of creating a resources package for sharing utilities, which should improve code reuse and maintainability.However, consider adding documentation comments to these methods to explain the new return types and their purpose.
3-19: Consider type safety improvementsThe code now uses
anyas a generic type parameter in multiple places (e.g., common.PaginatedResource[ledger.Account, any, common.OffsetPaginatedQuery[any]]). While this provides flexibility, it might reduce type safety.Consider using more specific types where possible to improve compile-time type checking and code clarity.
docs/api/README.md (1)
127-131: Provide Context for the Body Parameter Example
The new code sample showing an empty JSON object (i.e.{}) as the body parameter is useful to indicate that an empty object is acceptable when no filtering criteria are provided. Consider clarifying in the surrounding documentation (or in the example’s caption) that this field can be expanded to include filtering criteria (e.g. filtering by ledger name or metadata) when needed.pkg/client/docs/models/operations/v2listledgersrequest.md (1)
9-10: Document the New 'RequestBody' Field
A new fieldRequestBodyof typemap[string]*any*has been introduced and marked as required. Currently, its description is set to "N/A". For better clarity for API users and client developers, please consider providing a descriptive explanation (for example: "A JSON object containing filtering criteria for ledger listing"). This will align with the new body parameter in the API documentation and improve usability.internal/storage/common/resource.go (1)
184-215:expandmethod
Left-joining expansions via multiple CTEs can be powerful. Be mindful of performance if expansions grow large or chain extensively. Otherwise, this approach is flexible for partial data enrichment.internal/storage/system/store.go (1)
21-21: Updated Store interface with paginated resource methodThe
ListLedgersmethod has been replaced with aLedgersmethod that returns acommon.PaginatedResource. This change follows a more object-oriented approach by returning a specialized resource object that handles pagination internally.Consider adding documentation to clarify how callers should use this new approach, as it represents a significant interface change that will require updates to all code using the previous
ListLedgersmethod.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumopenapi.yamlis excluded by!**/*.yamlpkg/client/.speakeasy/gen.lockis excluded by!**/*.lock,!**/*.locktools/generator/go.modis excluded by!**/*.modtools/generator/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (87)
docs/api/README.md(1 hunks)internal/api/bulking/mocks_ledger_controller_test.go(10 hunks)internal/api/common/mocks_ledger_controller_test.go(10 hunks)internal/api/common/mocks_system_controller_test.go(2 hunks)internal/api/v1/controllers_accounts_count.go(2 hunks)internal/api/v1/controllers_accounts_count_test.go(7 hunks)internal/api/v1/controllers_accounts_list_test.go(8 hunks)internal/api/v1/controllers_accounts_read.go(2 hunks)internal/api/v1/controllers_accounts_read_test.go(5 hunks)internal/api/v1/controllers_balances_aggregates_test.go(3 hunks)internal/api/v1/controllers_config.go(2 hunks)internal/api/v1/controllers_logs_list_test.go(5 hunks)internal/api/v1/controllers_transactions_count_test.go(9 hunks)internal/api/v1/controllers_transactions_list_test.go(12 hunks)internal/api/v1/controllers_transactions_read_test.go(2 hunks)internal/api/v1/mocks_ledger_controller_test.go(10 hunks)internal/api/v1/mocks_system_controller_test.go(2 hunks)internal/api/v1/utils.go(5 hunks)internal/api/v2/common.go(6 hunks)internal/api/v2/controllers_accounts_count.go(2 hunks)internal/api/v2/controllers_accounts_count_test.go(10 hunks)internal/api/v2/controllers_accounts_list.go(2 hunks)internal/api/v2/controllers_accounts_list_test.go(12 hunks)internal/api/v2/controllers_accounts_read.go(2 hunks)internal/api/v2/controllers_accounts_read_test.go(3 hunks)internal/api/v2/controllers_balances.go(2 hunks)internal/api/v2/controllers_balances_test.go(6 hunks)internal/api/v2/controllers_ledgers_list.go(1 hunks)internal/api/v2/controllers_ledgers_list_test.go(3 hunks)internal/api/v2/controllers_logs_list.go(2 hunks)internal/api/v2/controllers_logs_list_test.go(7 hunks)internal/api/v2/controllers_transactions_count.go(2 hunks)internal/api/v2/controllers_transactions_count_test.go(13 hunks)internal/api/v2/controllers_transactions_list.go(2 hunks)internal/api/v2/controllers_transactions_list_test.go(14 hunks)internal/api/v2/controllers_transactions_read.go(2 hunks)internal/api/v2/controllers_transactions_read_test.go(2 hunks)internal/api/v2/controllers_volumes.go(2 hunks)internal/api/v2/controllers_volumes_test.go(8 hunks)internal/api/v2/mocks_ledger_controller_test.go(10 hunks)internal/api/v2/mocks_system_controller_test.go(2 hunks)internal/controller/ledger/controller.go(2 hunks)internal/controller/ledger/controller_default.go(4 hunks)internal/controller/ledger/controller_default_test.go(10 hunks)internal/controller/ledger/controller_generated_test.go(10 hunks)internal/controller/ledger/controller_with_traces.go(10 hunks)internal/controller/ledger/mocks.go(1 hunks)internal/controller/ledger/mocks_test.go(1 hunks)internal/controller/ledger/stats.go(2 hunks)internal/controller/ledger/stats_test.go(2 hunks)internal/controller/ledger/store.go(4 hunks)internal/controller/ledger/store_generated_test.go(7 hunks)internal/controller/system/controller.go(3 hunks)internal/controller/system/store.go(2 hunks)internal/storage/common/errors.go(1 hunks)internal/storage/common/metadata.go(1 hunks)internal/storage/common/paginator.go(1 hunks)internal/storage/common/paginator_column.go(5 hunks)internal/storage/common/paginator_offset.go(3 hunks)internal/storage/common/resource.go(1 hunks)internal/storage/driver/driver.go(2 hunks)internal/storage/driver/driver_test.go(2 hunks)internal/storage/driver/system_generated_test.go(2 hunks)internal/storage/ledger/accounts_test.go(23 hunks)internal/storage/ledger/adapters.go(2 hunks)internal/storage/ledger/balances_test.go(10 hunks)internal/storage/ledger/logs_test.go(4 hunks)internal/storage/ledger/moves_test.go(2 hunks)internal/storage/ledger/resource_accounts.go(3 hunks)internal/storage/ledger/resource_aggregated_balances.go(5 hunks)internal/storage/ledger/resource_logs.go(1 hunks)internal/storage/ledger/resource_transactions.go(7 hunks)internal/storage/ledger/resource_volumes.go(10 hunks)internal/storage/ledger/store.go(2 hunks)internal/storage/ledger/transactions.go(0 hunks)internal/storage/ledger/transactions_test.go(13 hunks)internal/storage/ledger/volumes_test.go(30 hunks)internal/storage/system/resource_ledgers.go(1 hunks)internal/storage/system/store.go(2 hunks)internal/storage/system/store_test.go(3 hunks)internal/worker/async_block.go(2 hunks)pkg/client/docs/models/operations/v2listledgersrequest.md(1 hunks)pkg/client/docs/sdks/v2/README.md(1 hunks)pkg/client/models/operations/v2listledgers.go(2 hunks)pkg/client/v2.go(2 hunks)test/e2e/api_ledgers_list_test.go(2 hunks)test/e2e/api_transactions_list_test.go(3 hunks)
💤 Files with no reviewable changes (1)
- internal/storage/ledger/transactions.go
🚧 Files skipped from review as they are similar to previous changes (74)
- internal/api/v1/controllers_accounts_count.go
- internal/api/v2/controllers_accounts_list.go
- internal/api/v2/controllers_transactions_list.go
- internal/api/v2/controllers_balances.go
- internal/storage/common/paginator.go
- internal/storage/common/metadata.go
- internal/api/v2/controllers_transactions_count.go
- internal/api/v2/controllers_transactions_count_test.go
- internal/storage/ledger/volumes_test.go
- internal/api/v2/controllers_volumes.go
- internal/api/common/mocks_system_controller_test.go
- pkg/client/models/operations/v2listledgers.go
- internal/storage/ledger/adapters.go
- internal/api/v2/controllers_transactions_read_test.go
- internal/controller/ledger/stats.go
- internal/api/v2/controllers_accounts_read.go
- internal/storage/ledger/accounts_test.go
- internal/storage/ledger/balances_test.go
- internal/api/v2/controllers_transactions_read.go
- internal/api/v2/mocks_system_controller_test.go
- internal/api/v1/controllers_accounts_list_test.go
- internal/api/v2/controllers_accounts_count.go
- internal/storage/ledger/moves_test.go
- internal/storage/ledger/logs_test.go
- internal/storage/system/store_test.go
- internal/api/v1/controllers_config.go
- internal/api/v1/controllers_transactions_count_test.go
- internal/api/v1/controllers_balances_aggregates_test.go
- internal/api/v2/controllers_balances_test.go
- internal/api/v2/controllers_accounts_read_test.go
- internal/api/v2/controllers_transactions_list_test.go
- internal/storage/common/errors.go
- internal/storage/driver/driver.go
- internal/api/v1/controllers_accounts_read.go
- internal/controller/ledger/controller_default_test.go
- internal/api/v1/controllers_transactions_list_test.go
- internal/api/v1/mocks_system_controller_test.go
- internal/api/v2/controllers_logs_list.go
- test/e2e/api_ledgers_list_test.go
- internal/api/v2/controllers_ledgers_list_test.go
- internal/controller/ledger/mocks.go
- pkg/client/docs/sdks/v2/README.md
- internal/api/v2/controllers_ledgers_list.go
- internal/api/v2/controllers_accounts_count_test.go
- internal/api/v1/controllers_logs_list_test.go
- internal/api/v2/controllers_volumes_test.go
- internal/api/v1/controllers_transactions_read_test.go
- test/e2e/api_transactions_list_test.go
- internal/api/v2/controllers_logs_list_test.go
- internal/controller/system/store.go
- internal/api/v2/controllers_accounts_list_test.go
- pkg/client/v2.go
- internal/api/v1/controllers_accounts_read_test.go
- internal/storage/driver/driver_test.go
- internal/controller/system/controller.go
- internal/controller/ledger/stats_test.go
- internal/api/v2/common.go
- internal/storage/ledger/transactions_test.go
- internal/api/v1/utils.go
- internal/storage/ledger/store.go
- internal/storage/common/paginator_column.go
- internal/controller/ledger/controller.go
- internal/storage/common/paginator_offset.go
- internal/controller/ledger/controller_with_traces.go
- internal/controller/ledger/store_generated_test.go
- internal/storage/ledger/resource_volumes.go
- internal/api/v2/mocks_ledger_controller_test.go
- internal/storage/ledger/resource_aggregated_balances.go
- internal/controller/ledger/controller_generated_test.go
- internal/api/common/mocks_ledger_controller_test.go
- internal/storage/ledger/resource_transactions.go
- internal/api/bulking/mocks_ledger_controller_test.go
- internal/controller/ledger/controller_default.go
- internal/api/v1/mocks_ledger_controller_test.go
🧰 Additional context used
🧬 Code Definitions (6)
internal/storage/system/store.go (3)
internal/ledger.go (1)
Ledger(18-26)internal/storage/common/resource.go (3)
PaginatedResource(420-423)ColumnPaginatedQuery(405-414)NewPaginatedResourceRepository(307-315)internal/storage/common/paginator_column.go (1)
ColumnPaginator(14-17)
internal/storage/system/resource_ledgers.go (4)
internal/storage/system/store.go (2)
DefaultStore(34-36)New(125-129)internal/storage/common/resource.go (8)
Filter(58-63)PropertyValidator(40-42)AcceptOperators(49-56)RepositoryHandlerBuildContext(65-68)ResourceQuery(358-364)ConvertOperatorToSQL(18-33)JoinCondition(35-38)RepositoryHandler(84-90)internal/storage/common/metadata.go (1)
MetadataRegex(6-6)internal/storage/common/errors.go (1)
NewErrInvalidQuery(18-22)
internal/api/v1/controllers_accounts_count_test.go (2)
internal/storage/common/resource.go (1)
ResourceQuery(358-364)internal/storage/common/errors.go (1)
ErrInvalidQuery(5-7)
internal/storage/ledger/resource_logs.go (1)
internal/storage/common/resource.go (6)
Filter(58-63)RepositoryHandlerBuildContext(65-68)ResourceQuery(358-364)ConvertOperatorToSQL(18-33)JoinCondition(35-38)RepositoryHandler(84-90)
internal/storage/ledger/resource_accounts.go (7)
internal/storage/ledger/store.go (1)
Store(24-46)internal/controller/ledger/store.go (1)
Store(31-66)internal/controller/system/store.go (1)
Store(14-21)internal/storage/common/resource.go (8)
Filter(58-63)PropertyValidator(40-42)PropertyValidatorFunc(43-43)AcceptOperators(49-56)RepositoryHandlerBuildContext(65-68)ResourceQuery(358-364)JoinCondition(35-38)RepositoryHandler(84-90)internal/controller/ledger/errors.go (1)
NewErrMissingFeature(106-110)internal/storage/common/metadata.go (1)
MetadataRegex(6-6)internal/storage/common/errors.go (1)
NewErrInvalidQuery(18-22)
internal/controller/ledger/mocks_test.go (3)
internal/controller/ledger/controller.go (1)
Controller(18-78)internal/controller/system/controller.go (1)
Controller(23-34)internal/storage/common/resource.go (5)
RepositoryHandlerBuildContext(65-68)ResourceQuery(358-364)JoinCondition(35-38)Filter(58-63)PaginatedQuery(415-417)
🪛 LanguageTool
docs/api/README.md
[uncategorized] ~136-~136: A punctuation mark might be missing here.
Context: ...uired|Description| |---|---|---|---|---| |pageSize|query|integer(int64)|false|The...
(AI_EN_LECTOR_MISSING_PUNCTUATION)
🔇 Additional comments (75)
internal/worker/async_block.go (4)
8-8: Import of query package aligns with new query construction approach.This import is necessary for the updated query construction using
query.Matchon line 73. The change is part of the codebase's transition to a more standardized query handling approach.
11-11: Added common storage package import supports the pagination API changes.This import supports the new function signature on line 78 that uses
common.ColumnPaginatedQuery[any], which is part of the refactoring to standardize resource queries across the codebase.
73-73: Improved query construction using the standardized query API.The change from a map-based feature assignment to using
query.Matchprovides a more declarative and type-safe approach for filtering ledgers with the ASYNC feature.
78-79: Updated pagination signature and method call follow the new common API pattern.The function signature now uses
common.ColumnPaginatedQuery[any]instead of the previous ledger-specific query type, and the pagination method has been updated to use the new fluent API style withLedgers().Paginate(). These changes align with the broader refactoring effort to standardize query handling across the codebase.internal/controller/ledger/store.go (3)
73-75: Consistent use of common.ResourceQuery in adapterThe vmStoreAdapter.GetAccount method has been properly updated to use the new common.ResourceQuery type, maintaining consistency with the interface changes.
90-98: Updated NewListLedgersQuery with pagination defaultsThe function now returns a common.ColumnPaginatedQuery type with sensible defaults:
- "id" as the default column
- Ascending order
- Empty Expand slice
- Specified page size
This change supports the new filtering functionality mentioned in the PR objectives. The use of pointer.For to create a pointer to the OrderAsc constant is a clean approach.
129-131: Consistent refactoring in numscriptRewriteAdapterThe numscriptRewriteAdapter.GetAccountsMetadata method has been properly updated to use common.ResourceQuery, maintaining consistency with the rest of the refactoring.
docs/api/README.md (2)
120-121: Clarify Request Content Type
The addition of the headerContent-Type: application/jsonexplicitly indicates that the request body should be in JSON format. This improves consistency with the new body parameter and ensures clients send the proper data type.
139-139: Enhance Description for 'body' Parameter
The table row added for the body parameter now specifies:|body|body|object|true|none|However, this description ("none") does not provide guidance to the developer. Based on past review comments, please update the description to explain that this parameter is used for filtering the ledgers list (for example, detailing that it supports substring matching on ledger names and filtering by metadata).
internal/controller/ledger/mocks_test.go (6)
1-7: Generated code follows best practices.The file is clearly marked as generated by MockGen with the exact command used to generate it. This is good practice as it ensures reproducibility and clarity about the source of the code.
8-18: Appropriate imports for mock functionality.The imports include standard Go packages (context, reflect) and external dependencies needed for the mocks functionality. The code correctly imports the common package from internal storage, which aligns with the PR objectives of creating and utilizing a resources package.
20-56: Well-structured MockPropertyValidator implementation.This mock implementation follows the standard GoMock patterns with proper helper methods and type assertions. The
Validatemethod signature matches what would be expected for a property validator in a filtering system.
58-156: Generic MockRepositoryHandler properly implements all interface methods.The
MockRepositoryHandler[Opts any]implementation correctly uses Go generics to represent a repository handler with flexible option types. All five interface methods (BuildDataset,Expand,Filters,Project, andResolveFilter) are properly implemented with appropriate parameter and return types that match the common package's interfaces.
158-210: MockResource implementation with dual type parameters.The mock correctly implements a generic resource interface with two type parameters (
ResourceTypeandOptionsType), allowing for strong typing in test code. The implementation ofCountandGetOnemethods aligns with resource query patterns.
212-279: Comprehensive MockPaginatedResource implementation with complex generic constraints.The mock uses three type parameters with the third parameter (
PaginationQueryType) constrained to implement thecommon.PaginatedQuery[OptionsType]interface. This advanced use of generics ensures type safety while maintaining the flexibility needed for testing pagination functionality. ThePaginatemethod properly returns a cursor of resources, which is essential for the ledger listing feature mentioned in the PR objectives.internal/storage/ledger/resource_logs.go (8)
6-6: Import of common package is correctly addedThis import is necessary to support the migration from package-specific types to the more standardized common types.
14-24: Method properly exported with standardized return typeThe method has been correctly capitalized to make it exported, and the return type has been updated to use the standardized
common.Filtertype. Field names have also been appropriately capitalized.
26-31: BuildDataset method refactored to use instance storeThe method has been properly updated to use the instance store field rather than accepting it as a parameter, which is consistent with the struct change. The implementation remains functionally equivalent.
33-40: ResolveFilter method properly updatedThe method signature has been updated to use
common.ResourceQuery[any]instead of the package-specific type, and the implementation now usescommon.ConvertOperatorToSQLinstead of the local function. These changes align with the refactoring to use common types.
42-44: Expand method properly migrated to common typesThe method has been correctly updated to use common types, including changing the return type to use
*common.JoinCondition. The implementation remains functionally equivalent.
46-48: Project method signature updated for consistencyThe method signature has been correctly updated to match the common interface, removing the store parameter since it's now accessible via the struct field.
50-50: Interface assertion correctly updatedThe interface assertion has been properly updated to use the
common.RepositoryHandler[any]interface, which is consistent with the other changes in this file.
10-12:✅ Verification successful
Good design change adding store to the struct
Adding the store field to the
logsResourceHandlerstruct is a good design choice, improving encapsulation by maintaining a reference to the store instead of passing it as a parameter to each method.However, I don't see where this struct is initialized. Let's verify that the store field is properly initialized when this struct is created:
🏁 Script executed:
#!/bin/bash # Find where logsResourceHandler is instantiated rg "logsResourceHandler\{" --glob "*.go" -A 3Length of output: 500
Verification Complete:
logsResourceHandlerInitialization ConfirmedThe
logsResourceHandlerstruct is properly instantiated ininternal/storage/ledger/store.gowhere thestorefield is correctly initialized (i.e.,store: store). This confirms that the design change improves encapsulation as intended.internal/api/v1/controllers_accounts_count_test.go (10)
5-5: Switching tostoragecommonimport alias
This import reference is now aligned with the newly introducedstoragecommonpackage for resource queries, ensuring consistency across the codebase.
28-28: UpdatingexpectQuerytostoragecommon.ResourceQuery[any]
This change correctly reflects the migration fromledgercontroller.ResourceQuerytostoragecommon.ResourceQuery, aligning with the new standardized query approach.
39-39: Using an emptyResourceQueryin the nominal scenario
Instantiating an emptystoragecommon.ResourceQuery[any]{}is appropriate for the nominal case test and ensures consistency with the new query type.
48-49: Building a query withquery.Match
Usingquery.Match(...)withinstoragecommon.ResourceQuery[any]follows the new standard design. Good job applying the updated type.
56-57: Address-based match
Here,storagecommon.ResourceQuery[any]is likewise combined withquery.Match("address", ...). This is consistent with the new query framework.
66-66: Handling over-maximum page size scenario
Creating an emptystoragecommon.ResourceQuery[any]{}for the page size test keeps the logic straightforward.
75-76: Usingquery.Ltfor balance filter
The updated callquery.Lt("balance", int64(100))is correctly wrapped in astoragecommon.ResourceQuery[any].
84-85: Switching tostoragecommon.ErrInvalidQuery
Replacingledgercontroller.ErrInvalidQuerywithstoragecommon.ErrInvalidQueryunifies error handling, consistent with the rest of the migration.
93-93: Applying an emptyResourceQueryupon missing feature
Using the newstoragecommon.ResourceQuery[any]{}for this test scenario is in line with the updated code structure.
101-101: Applying an emptyResourceQueryupon unexpected error
This maintains consistency by fully switching tostoragecommon.ResourceQuery[any]{}for backend calls.internal/storage/driver/system_generated_test.go (3)
17-17: Importingcommon
Switching the import reference tocommon "github.com/formancehq/ledger/internal/storage/common"matches the new package layout for shared resource handling.
136-141: Defining theLedgers()mock method
Introducing theLedgers()method returning acommon.PaginatedResource[...]is consistent with the revised data retrieval approach, promoting a unified pagination model.
144-147: Recorder forLedgers()
Providing a corresponding recorder ensures theLedgers()method can be properly mocked and validated in tests.internal/storage/common/resource.go (22)
1-16: Package declaration and imports
The package namecommonand the chosen imports (e.g.,bunpaginate,query,time, etc.) establish a clear foundation for resource handling and pagination. Overall, the imports are well-organized to support these functionalities.
35-47: Validator extenders
DefiningPropertyValidatorandPropertyValidatorFuncfor extensible validation is a good abstraction, enabling more specific or custom checks in a modular way.
49-56:AcceptOperatorsutility
This function ensures filters only permit whitelisted operators, reducing the risk of undetected invalid queries. The approach is concise and secure.
58-63:Filterstruct
StoringName,Aliases,Matchers, andValidatorsprovides a flexible filtering mechanism. This design is clean and encourages future extensibility.
65-82:RepositoryHandlerBuildContextandUseFilter
TheUseFiltermethod elegantly checks for existence and runs custom matchers on filter values. This approach is straightforward and ensures minimal duplication.
84-91:RepositoryHandlerinterface
This interface clearly outlines the methods required to build queries, handle expansions, and project results. It’s a well-defined contract for resource-specific logic.
92-146:ResourceRepositorystruct andvalidateFiltersmethod
Grouping validation logic in one place helps ensure consistent handling of unknown keys and invalid operators. The usage ofquery.Builder.Walkis well-done.
148-182:buildFilteredDatasetmethod
Nicely composes filter validation, dataset creation, and final projection steps. This fosters better separation of concerns by delegating filter resolution to theRepositoryHandler.
217-241:GetOnemethod
The flow to build a filtered dataset, expand fields, and then retrieve a single record is logical. Returningpostgres.ErrNotFoundupon no results standardizes the not-found handling.
243-252:Countmethod
This complementsGetOneby returning the count of matching records. The escalated usage ofpostgres.ResolveErrorfosters consistent error handling across repositories.
254-260:NewResourceRepository
The constructor pattern is straightforward. Instantiating with aRepositoryHandlerensures a well-defined entry point for resource operations.
262-305:PaginatedResourceRepositoryand itsPaginatemethod
The generic approach withPaginatedQuery[OptionsType]supports multiple pagination strategies (offset or column). This design neatly decouples filtering logic from pagination logic.
307-315: Constructor forPaginatedResourceRepository
Wrapping aResourceRepositoryand attaching aPaginatoris a clean composition, preserving the single-responsibility principle.
317-333:PaginatedResourceRepositoryMapperand itsPaginatemethod
Mapping from an original resource type to a core type while paginating addresses transformation needs elegantly. Reusing the existing cursor approach is efficient.
335-345:GetOnemethod in the mapper
By calling the inner repository'sGetOneand then transforming the result with.ToCore(), this pattern remains DRY and ensures consistent data transformations.
347-356:NewPaginatedResourceRepositoryMapper
Providing a straightforward constructor that requires the handler and paginator aligns with the rest of the repository building approach.
358-364:ResourceQuerystruct
This generic query container withPIT,OOT,Builder,Expand, andOptsis a foundational element for flexible resource interactions.
366-372:UsePITandUseOOT
Simple checks for pointer non-nil and zero values help maintain clarity about whether a point-in-time or out-of-time filter applies.
374-390:UnmarshalJSON
The approach to decode into an auxiliary struct, then parse the raw JSON for theBuilderfield usingquery.ParseJSONis robust. It cleanly separates specialized builder parsing from standard field unmarshaling.
392-395:Resourceinterface
DefiningGetOneandCountas fundamental resource operations fosters a consistent standard across multiple resource implementations.
397-418:OffsetPaginatedQuery,ColumnPaginatedQuery, andPaginatedQuery
These strongly typed pagination queries ensure structured parameters for offset- and column-based pagination, increasing clarity and type safety.
420-423:PaginatedResourceinterface
ExtendingResourcewithPaginateis a clean way to provide advanced resource capabilities while retaining the base resource operations.internal/storage/ledger/resource_accounts.go (7)
12-14: Refactored resource handler now holds store referenceThe
accountsResourceHandlerstruct has been redesigned to hold a direct reference to the store instead of receiving it through method parameters. This creates a cleaner API but introduces a tighter coupling between the handler and the store.
16-51: Exported Filter method with standardized common package typesThe method has been renamed from
filterstoFilters(exported) and now returns[]common.Filterinstead of an internal type. The implementation uses the common package's validator types for consistent validation across the codebase.
53-86: BuildDataset method now directly uses store referenceThe method signature has been updated to use
common.RepositoryHandlerBuildContextinstead of an internal type. The implementation now directly usesh.storefor database operations.The feature check at line 66 uses a direct reference to
h.store.ledgerfor more readable code.
88-135: ResolveFilter now uses standardized common package typesThe method now accepts a
common.ResourceQueryparameter and leverages common package utilities likecommon.ConvertOperatorToSQLandcommon.MetadataRegex. Error reporting uses the standardizedcommon.NewErrInvalidQuery.The feature check at line 101 now directly uses
h.store.ledgerto check for theFeatureMovesHistoryfeature.
137-139: Project method updated to use common package typesThe method signature now uses
common.ResourceQueryfor consistency with other resource handlers.
141-187: Expand method now has standardized error handlingThe method signature now uses
common.ResourceQueryand returns acommon.JoinCondition. Feature checks at lines 144 and 148 now useh.store.ledgerdirectly with improved error reporting usingcommon.NewErrInvalidQuery.
189-189: Updated interface compliance checkUpdated to verify implementation of
common.RepositoryHandler[any]instead of an internal type, confirming compliance with the standardized interface.internal/storage/system/store.go (2)
13-13: Imported common package for resource handlersAdded import for the common storage package which provides shared utilities for resource handling.
94-102: Implemented new Ledgers method with paginated resource repositoryThe implementation uses
common.NewPaginatedResourceRepositorywith aledgersResourceHandlerandcommon.ColumnPaginator. This provides a consistent approach to pagination across the codebase.The method configures the paginator with sensible defaults:
- Default pagination column: "id"
- Default order: ascending
internal/storage/system/resource_ledgers.go (8)
11-13: Added regex for features filteringDefined a regex pattern to match properties in the format
features[key]for filtering ledgers by feature values.
15-17: Created ledgersResourceHandler with store referenceThe handler holds a reference to the store to access database operations, following the same pattern as other resource handlers.
19-46: Implemented Filters method for ledger filteringDefined filters for "bucket", "features[...]", "metadata[...]", and "name" with appropriate validators:
- bucket: supports only exact matching ($match)
- features: supports only exact matching ($match)
- metadata: supports only exact matching ($match)
- name: supports both exact matching ($match) and substring matching ($like)
This implementation enables flexible filtering of ledgers.
48-52: Implemented BuildDataset with simple model queryCreates a basic select query for ledgers with all columns selected. The implementation is straightforward as it doesn't require complex joins or filtering.
71-75: Support for metadata and name filteringAdded support for:
- Filtering by metadata existence (metadata -> ? is not null)
- Flexible name filtering with both exact matching and substring matching using the appropriate SQL operator
The
namefilter is particularly useful as it supports $like operator for substring searching.
80-82: Implemented Project method for column selectionSimple implementation that selects all columns (*) from the query. This provides flexibility for future enhancements if specific column projection is needed.
84-86: Expand method returns clear error for unsupported expansionExplicitly returns an error indicating that no expansion is available for ledgers, making it clear to callers that this feature is not supported.
88-88: Added interface compliance checkEnsures that
ledgersResourceHandlerproperly implements thecommon.RepositoryHandler[any]interface, catching any implementation problems at compile time.
gfyrag
force-pushed
the
feat/ledgers-endpoint-filtering
branch
from
9bc73b3 to
2db208d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
internal/storage/system/resource_ledgers.go (2)
65-69:⚠️ Potential issuePotential index out of bounds error in metadata handling
Similar to the features regex handling, the code doesn't check if the
matcharray contains elements or if the inner array has enough elements before accessing them.match := common.MetadataRegex.FindAllStringSubmatch(property, 3) +if len(match) == 0 || len(match[0]) < 2 { + return "", nil, common.NewErrInvalidQuery("invalid metadata property format %s", property) +} return "metadata @> ?", []any{map[string]any{ match[0][1]: value, }}, nilThis issue was previously identified in a past review but appears to still be present in the current code.
59-63:⚠️ Potential issuePotential index out of bounds error in featuresRegex handling
The code accesses
match[0][1]without first checking if the match array has elements or if the inner array has sufficient elements. This could lead to a panic if the regex doesn't match as expected.match := featuresRegex.FindAllStringSubmatch(property, 3) +if len(match) == 0 || len(match[0]) < 2 { + return "", nil, common.NewErrInvalidQuery("invalid feature property format %s", property) +} return "features @> ?", []any{map[string]any{ match[0][1]: value, }}, nilThis issue was previously identified in a past review but appears to still be present in the current code.
🧹 Nitpick comments (14)
docs/api/README.md (2)
127-131: Consider enriching the Body parameter example.
Currently, the example request body is an empty JSON object ({}). Since this endpoint now supports filtering on the ledgers list, it would be very helpful to include a sample payload that shows possible filtering criteria (e.g. filtering by ledger name or metadata fields). This could improve user understanding of what filters are supported.
134-136: Verify Markdown table formatting correctness.
Static analysis flagged a potential missing punctuation mark near the table header lines. Although this might be a false positive, please double-check the Markdown syntax (e.g. ensuring that the header row and separator line are correctly formatted) so that the parameters table renders as expected.🧰 Tools
🪛 LanguageTool
[uncategorized] ~136-~136: A punctuation mark might be missing here.
Context: ...uired|Description| |---|---|---|---|---| |pageSize|query|integer(int64)|false|The...(AI_EN_LECTOR_MISSING_PUNCTUATION)
internal/storage/system/resource_ledgers.go (1)
54-78: Consider refactoring filter resolution for better maintainabilityThe
ResolveFiltermethod handles several different properties with similar patterns. This could be refactored to improve readability and maintainability.Consider using a map or separate methods to handle each property type, which would make the code more maintainable and easier to extend with new filter types in the future.
func (h ledgersResourceHandler) ResolveFilter(opts common.ResourceQuery[any], operator, property string, value any) (string, []any, error) { - switch { - case property == "bucket": - return "bucket = ?", []any{value}, nil - case featuresRegex.Match([]byte(property)): - match := featuresRegex.FindAllStringSubmatch(property, 3) - - return "features @> ?", []any{map[string]any{ - match[0][1]: value, - }}, nil - case common.MetadataRegex.Match([]byte(property)): - match := common.MetadataRegex.FindAllStringSubmatch(property, 3) - - return "metadata @> ?", []any{map[string]any{ - match[0][1]: value, - }}, nil - - case property == "metadata": - return "metadata -> ? is not null", []any{value}, nil - case property == "name": - return "name " + common.ConvertOperatorToSQL(operator) + " ?", []any{value}, nil - default: - return "", nil, common.NewErrInvalidQuery("invalid filter property %s", property) - } + // Handle exact property matches + if property == "bucket" { + return "bucket = ?", []any{value}, nil + } + + if property == "metadata" { + return "metadata -> ? is not null", []any{value}, nil + } + + if property == "name" { + return "name " + common.ConvertOperatorToSQL(operator) + " ?", []any{value}, nil + } + + // Handle regex pattern properties + if featuresRegex.Match([]byte(property)) { + match := featuresRegex.FindAllStringSubmatch(property, 3) + if len(match) == 0 || len(match[0]) < 2 { + return "", nil, common.NewErrInvalidQuery("invalid feature property format %s", property) + } + + return "features @> ?", []any{map[string]any{ + match[0][1]: value, + }}, nil + } + + if common.MetadataRegex.Match([]byte(property)) { + match := common.MetadataRegex.FindAllStringSubmatch(property, 3) + if len(match) == 0 || len(match[0]) < 2 { + return "", nil, common.NewErrInvalidQuery("invalid metadata property format %s", property) + } + + return "metadata @> ?", []any{map[string]any{ + match[0][1]: value, + }}, nil + } + + return "", nil, common.NewErrInvalidQuery("invalid filter property %s", property) }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 71-72: internal/storage/system/resource_ledgers.go#L71-L72
Added lines #L71 - L72 were not covered by testsinternal/api/v2/controllers_balances.go (1)
16-20: Consider migrating GetAggregatedVolumesOptions to the common package.While you've migrated to
storagecommon.ErrInvalidQuery{}for error handling, the function is still usingledgercontroller.GetAggregatedVolumesOptions. For complete refactoring, consider moving this type to the common package as well.internal/storage/ledger/resource_logs.go (2)
14-15: Filters methodConsider adding simple validators for “date” and “id” to prevent invalid data types.
42-45: Expand methodFuture expansions might be needed if logs gain references to related entities.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 42-42: internal/storage/ledger/resource_logs.go#L42
Added line #L42 was not covered by testsinternal/storage/ledger/resource_accounts.go (1)
88-135: ResolveFilter logicConsider returning more specific messages for invalid filter keys.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 133-133: internal/storage/ledger/resource_accounts.go#L133
Added line #L133 was not covered by testsinternal/storage/ledger/resource_volumes.go (4)
13-15:volumesResourceHandlernow holds a*Store.
Having a directstore *Storefield provides each method consistent access to the ledger’s store and ledger instance. Verify that there are no unexpected nil references when this struct is instantiated.
17-63:Filters()method provides comprehensive filtering options.
The returned filters handle address, balance, first usage, and metadata. This is an excellent move towards standardizing filter validation viacommon.Filter.Consider adding or clarifying error messages for invalid values (e.g., parse failures for numeric filters).
165-202:ResolveFilter()effectively translates filter expressions to SQL.
- Good use of
common.ConvertOperatorToSQL(operator). Validate that all accepted operators map correctly to secure SQL fragments (avoid injection).- For the
balance(\[.*])?case, you correctly handle sub-asset identification viabalanceRegex.- The metadata filter logic is consistent with the approach in
Filters().Consider supporting additional operators (
$neq,$exists) for certain fields if your domain requires more advanced queries.
228-230:Expand()returns an error with no expansion available.
No expansions are currently supported, which is intentional. You might add a simple test to ensureExpand()returns the expected “no expansion available” error, improving code coverage.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 228-228: internal/storage/ledger/resource_volumes.go#L228
Added line #L228 was not covered by testsinternal/storage/ledger/resource_aggregated_balances.go (3)
12-14:aggregatedBalancesResourceRepositoryHandlerincludesstore *Store.
Storing a reference toStorecentralizes data access. Check for any initialization paths wherestoremight be nil.
51-133:BuildDataset()method for aggregated balances.
- Good separation between point-in-time queries (PIT) and immediate queries.
- Ensure that partial address matching, metadata, and date constraints combine properly. For large scale data, watch out for potential performance hits from lateral joins.
Consider introducing caching or partial indexing if you notice performance bottlenecks in queries with PIT usage.
154-156:Expand()returns no expansion available.
Add a minimal test for expansions to drive coverage and clarify expected error behavior.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 154-154: internal/storage/ledger/resource_aggregated_balances.go#L154
Added line #L154 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumopenapi.yamlis excluded by!**/*.yamlopenapi/v2.yamlis excluded by!**/*.yamlpkg/client/.speakeasy/gen.lockis excluded by!**/*.lock,!**/*.locktools/generator/go.modis excluded by!**/*.modtools/generator/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (90)
docs/api/README.md(1 hunks)internal/api/bulking/mocks_ledger_controller_test.go(10 hunks)internal/api/common/mocks_ledger_controller_test.go(10 hunks)internal/api/common/mocks_system_controller_test.go(2 hunks)internal/api/v1/controllers_accounts_count.go(2 hunks)internal/api/v1/controllers_accounts_count_test.go(7 hunks)internal/api/v1/controllers_accounts_list_test.go(8 hunks)internal/api/v1/controllers_accounts_read.go(2 hunks)internal/api/v1/controllers_accounts_read_test.go(5 hunks)internal/api/v1/controllers_balances_aggregates_test.go(3 hunks)internal/api/v1/controllers_config.go(2 hunks)internal/api/v1/controllers_logs_list_test.go(5 hunks)internal/api/v1/controllers_transactions_count_test.go(9 hunks)internal/api/v1/controllers_transactions_list_test.go(12 hunks)internal/api/v1/controllers_transactions_read_test.go(2 hunks)internal/api/v1/mocks_ledger_controller_test.go(10 hunks)internal/api/v1/mocks_system_controller_test.go(2 hunks)internal/api/v1/utils.go(5 hunks)internal/api/v2/common.go(6 hunks)internal/api/v2/controllers_accounts_count.go(2 hunks)internal/api/v2/controllers_accounts_count_test.go(10 hunks)internal/api/v2/controllers_accounts_list.go(2 hunks)internal/api/v2/controllers_accounts_list_test.go(12 hunks)internal/api/v2/controllers_accounts_read.go(2 hunks)internal/api/v2/controllers_accounts_read_test.go(3 hunks)internal/api/v2/controllers_balances.go(2 hunks)internal/api/v2/controllers_balances_test.go(6 hunks)internal/api/v2/controllers_ledgers_list.go(1 hunks)internal/api/v2/controllers_ledgers_list_test.go(3 hunks)internal/api/v2/controllers_logs_list.go(2 hunks)internal/api/v2/controllers_logs_list_test.go(7 hunks)internal/api/v2/controllers_transactions_count.go(2 hunks)internal/api/v2/controllers_transactions_count_test.go(13 hunks)internal/api/v2/controllers_transactions_list.go(2 hunks)internal/api/v2/controllers_transactions_list_test.go(14 hunks)internal/api/v2/controllers_transactions_read.go(2 hunks)internal/api/v2/controllers_transactions_read_test.go(2 hunks)internal/api/v2/controllers_volumes.go(2 hunks)internal/api/v2/controllers_volumes_test.go(8 hunks)internal/api/v2/mocks_ledger_controller_test.go(10 hunks)internal/api/v2/mocks_system_controller_test.go(2 hunks)internal/controller/ledger/controller.go(2 hunks)internal/controller/ledger/controller_default.go(4 hunks)internal/controller/ledger/controller_default_test.go(10 hunks)internal/controller/ledger/controller_generated_test.go(10 hunks)internal/controller/ledger/controller_with_traces.go(10 hunks)internal/controller/ledger/errors.go(0 hunks)internal/controller/ledger/mocks.go(1 hunks)internal/controller/ledger/mocks_test.go(1 hunks)internal/controller/ledger/stats.go(2 hunks)internal/controller/ledger/stats_test.go(2 hunks)internal/controller/ledger/store.go(4 hunks)internal/controller/ledger/store_generated_test.go(7 hunks)internal/controller/system/controller.go(3 hunks)internal/controller/system/store.go(2 hunks)internal/storage/common/errors.go(1 hunks)internal/storage/common/metadata.go(1 hunks)internal/storage/common/paginator.go(1 hunks)internal/storage/common/paginator_column.go(5 hunks)internal/storage/common/paginator_offset.go(3 hunks)internal/storage/common/resource.go(1 hunks)internal/storage/driver/driver.go(2 hunks)internal/storage/driver/driver_test.go(2 hunks)internal/storage/driver/system_generated_test.go(2 hunks)internal/storage/ledger/accounts_test.go(23 hunks)internal/storage/ledger/adapters.go(2 hunks)internal/storage/ledger/balances_test.go(10 hunks)internal/storage/ledger/logs_test.go(4 hunks)internal/storage/ledger/moves_test.go(2 hunks)internal/storage/ledger/paginator.go(0 hunks)internal/storage/ledger/resource.go(0 hunks)internal/storage/ledger/resource_accounts.go(3 hunks)internal/storage/ledger/resource_aggregated_balances.go(5 hunks)internal/storage/ledger/resource_logs.go(1 hunks)internal/storage/ledger/resource_transactions.go(7 hunks)internal/storage/ledger/resource_volumes.go(10 hunks)internal/storage/ledger/store.go(2 hunks)internal/storage/ledger/transactions.go(0 hunks)internal/storage/ledger/transactions_test.go(13 hunks)internal/storage/ledger/volumes_test.go(30 hunks)internal/storage/system/resource_ledgers.go(1 hunks)internal/storage/system/store.go(2 hunks)internal/storage/system/store_test.go(3 hunks)internal/worker/async_block.go(2 hunks)pkg/client/docs/models/operations/v2listledgersrequest.md(1 hunks)pkg/client/docs/sdks/v2/README.md(1 hunks)pkg/client/models/operations/v2listledgers.go(2 hunks)pkg/client/v2.go(2 hunks)test/e2e/api_ledgers_list_test.go(2 hunks)test/e2e/api_transactions_list_test.go(3 hunks)
💤 Files with no reviewable changes (4)
- internal/storage/ledger/transactions.go
- internal/storage/ledger/paginator.go
- internal/storage/ledger/resource.go
- internal/controller/ledger/errors.go
🚧 Files skipped from review as they are similar to previous changes (70)
- internal/controller/ledger/mocks.go
- pkg/client/models/operations/v2listledgers.go
- internal/api/v1/controllers_accounts_count.go
- internal/storage/system/store_test.go
- internal/api/v2/controllers_balances_test.go
- internal/storage/common/paginator.go
- internal/api/v2/controllers_transactions_count.go
- internal/storage/common/metadata.go
- internal/api/v2/controllers_transactions_read.go
- internal/api/v2/controllers_accounts_read.go
- internal/api/v2/controllers_transactions_list.go
- internal/controller/ledger/stats.go
- internal/api/v2/controllers_accounts_count.go
- pkg/client/docs/sdks/v2/README.md
- internal/api/v2/controllers_transactions_read_test.go
- internal/api/v2/controllers_accounts_list.go
- pkg/client/docs/models/operations/v2listledgersrequest.md
- internal/api/v1/controllers_transactions_count_test.go
- internal/api/v2/mocks_system_controller_test.go
- internal/storage/ledger/balances_test.go
- internal/controller/system/store.go
- internal/storage/ledger/logs_test.go
- internal/api/v1/controllers_accounts_read_test.go
- internal/api/v1/controllers_transactions_read_test.go
- internal/api/v2/controllers_ledgers_list.go
- internal/api/common/mocks_system_controller_test.go
- internal/worker/async_block.go
- internal/storage/common/errors.go
- internal/api/v1/controllers_accounts_read.go
- internal/api/v2/controllers_accounts_list_test.go
- internal/storage/driver/driver.go
- internal/storage/ledger/volumes_test.go
- internal/api/v2/controllers_logs_list.go
- internal/api/v2/controllers_transactions_count_test.go
- internal/api/v2/controllers_accounts_count_test.go
- internal/storage/ledger/moves_test.go
- test/e2e/api_ledgers_list_test.go
- internal/storage/ledger/transactions_test.go
- internal/api/v2/controllers_volumes_test.go
- internal/api/v1/controllers_transactions_list_test.go
- internal/api/v2/controllers_logs_list_test.go
- internal/storage/ledger/adapters.go
- internal/api/v2/controllers_accounts_read_test.go
- internal/api/v1/controllers_accounts_count_test.go
- internal/api/v1/controllers_accounts_list_test.go
- test/e2e/api_transactions_list_test.go
- internal/controller/system/controller.go
- internal/controller/ledger/stats_test.go
- internal/api/v1/mocks_system_controller_test.go
- internal/api/v2/controllers_ledgers_list_test.go
- internal/controller/ledger/controller_default_test.go
- internal/api/v1/controllers_logs_list_test.go
- internal/api/v2/controllers_transactions_list_test.go
- internal/api/v2/common.go
- internal/api/v1/controllers_balances_aggregates_test.go
- pkg/client/v2.go
- internal/api/v1/utils.go
- internal/api/v1/controllers_config.go
- internal/storage/common/paginator_offset.go
- internal/storage/common/paginator_column.go
- internal/storage/ledger/store.go
- internal/controller/ledger/controller.go
- internal/controller/ledger/controller_with_traces.go
- internal/storage/ledger/accounts_test.go
- internal/controller/ledger/store_generated_test.go
- internal/controller/ledger/controller_generated_test.go
- internal/api/common/mocks_ledger_controller_test.go
- internal/api/v1/mocks_ledger_controller_test.go
- internal/api/bulking/mocks_ledger_controller_test.go
- internal/api/v2/mocks_ledger_controller_test.go
🧰 Additional context used
🧬 Code Definitions (11)
internal/storage/driver/driver_test.go (1)
internal/controller/ledger/store.go (1)
NewListLedgersQuery(90-99)
internal/storage/system/resource_ledgers.go (4)
internal/storage/system/store.go (2)
DefaultStore(34-36)New(125-129)internal/storage/common/resource.go (8)
Filter(58-63)PropertyValidator(40-42)AcceptOperators(49-56)RepositoryHandlerBuildContext(65-68)ResourceQuery(358-364)ConvertOperatorToSQL(18-33)JoinCondition(35-38)RepositoryHandler(84-90)internal/storage/common/metadata.go (1)
MetadataRegex(6-6)internal/storage/common/errors.go (1)
NewErrInvalidQuery(18-22)
internal/storage/system/store.go (2)
internal/storage/common/resource.go (3)
PaginatedResource(420-423)ColumnPaginatedQuery(405-414)NewPaginatedResourceRepository(307-315)internal/storage/common/paginator_column.go (1)
ColumnPaginator(14-17)
internal/storage/common/resource.go (3)
internal/storage/common/errors.go (1)
NewErrInvalidQuery(18-22)internal/storage/ledger/transactions_test.go (1)
Model(640-646)internal/storage/common/paginator.go (1)
Paginator(8-11)
internal/storage/ledger/resource_accounts.go (6)
internal/storage/ledger/store.go (1)
Store(24-46)internal/controller/system/store.go (1)
Store(14-21)internal/storage/common/resource.go (8)
Filter(58-63)PropertyValidator(40-42)PropertyValidatorFunc(43-43)AcceptOperators(49-56)RepositoryHandlerBuildContext(65-68)ResourceQuery(358-364)JoinCondition(35-38)RepositoryHandler(84-90)internal/controller/ledger/errors.go (1)
NewErrMissingFeature(106-110)internal/storage/common/metadata.go (1)
MetadataRegex(6-6)internal/storage/common/errors.go (1)
NewErrInvalidQuery(18-22)
internal/storage/ledger/resource_logs.go (3)
internal/storage/ledger/store.go (2)
Store(24-46)New(161-248)internal/controller/system/store.go (1)
Store(14-21)internal/storage/common/resource.go (6)
Filter(58-63)RepositoryHandlerBuildContext(65-68)ResourceQuery(358-364)ConvertOperatorToSQL(18-33)JoinCondition(35-38)RepositoryHandler(84-90)
internal/storage/ledger/resource_transactions.go (3)
internal/storage/ledger/store.go (1)
Store(24-46)internal/storage/common/resource.go (9)
Filter(58-63)PropertyValidator(40-42)AcceptOperators(49-56)PropertyValidatorFunc(43-43)RepositoryHandlerBuildContext(65-68)ResourceQuery(358-364)ConvertOperatorToSQL(18-33)JoinCondition(35-38)RepositoryHandler(84-90)internal/storage/common/metadata.go (1)
MetadataRegex(6-6)
internal/controller/ledger/controller_default.go (1)
internal/storage/common/resource.go (3)
ColumnPaginatedQuery(405-414)ResourceQuery(358-364)OffsetPaginatedQuery(398-404)
internal/api/v2/controllers_balances.go (2)
internal/storage/common/errors.go (1)
ErrInvalidQuery(5-7)internal/controller/ledger/errors.go (1)
ErrMissingFeature(93-95)
internal/api/v2/controllers_volumes.go (2)
internal/storage/common/errors.go (1)
ErrInvalidQuery(5-7)internal/controller/ledger/errors.go (1)
ErrMissingFeature(93-95)
internal/storage/ledger/resource_volumes.go (5)
internal/storage/ledger/store.go (2)
Store(24-46)New(161-248)internal/storage/common/resource.go (7)
Filter(58-63)PropertyValidator(40-42)PropertyValidatorFunc(43-43)AcceptOperators(49-56)RepositoryHandlerBuildContext(65-68)ResourceQuery(358-364)RepositoryHandler(84-90)internal/storage/common/metadata.go (1)
MetadataRegex(6-6)internal/controller/ledger/errors.go (1)
NewErrMissingFeature(106-110)internal/storage/driver/driver.go (1)
New(279-294)
🪛 GitHub Check: codecov/patch
internal/storage/system/resource_ledgers.go
[warning] 71-72: internal/storage/system/resource_ledgers.go#L71-L72
Added lines #L71 - L72 were not covered by tests
internal/storage/common/resource.go
[warning] 32-32: internal/storage/common/resource.go#L32
Added line #L32 was not covered by tests
[warning] 52-53: internal/storage/common/resource.go#L52-L53
Added lines #L52 - L53 were not covered by tests
[warning] 117-117: internal/storage/common/resource.go#L117
Added line #L117 was not covered by tests
[warning] 129-130: internal/storage/common/resource.go#L129-L130
Added lines #L129 - L130 were not covered by tests
[warning] 160-161: internal/storage/common/resource.go#L160-L161
Added lines #L160 - L161 were not covered by tests
[warning] 172-173: internal/storage/common/resource.go#L172-L173
Added lines #L172 - L173 were not covered by tests
[warning] 195-196: internal/storage/common/resource.go#L195-L196
Added lines #L195 - L196 were not covered by tests
[warning] 221-222: internal/storage/common/resource.go#L221-L222
Added lines #L221 - L222 were not covered by tests
[warning] 226-227: internal/storage/common/resource.go#L226-L227
Added lines #L226 - L227 were not covered by tests
[warning] 234-235: internal/storage/common/resource.go#L234-L235
Added lines #L234 - L235 were not covered by tests
[warning] 247-248: internal/storage/common/resource.go#L247-L248
Added lines #L247 - L248 were not covered by tests
[warning] 278-279: internal/storage/common/resource.go#L278-L279
Added lines #L278 - L279 were not covered by tests
[warning] 289-290: internal/storage/common/resource.go#L289-L290
Added lines #L289 - L290 were not covered by tests
[warning] 294-295: internal/storage/common/resource.go#L294-L295
Added lines #L294 - L295 were not covered by tests
[warning] 301-302: internal/storage/common/resource.go#L301-L302
Added lines #L301 - L302 were not covered by tests
[warning] 329-330: internal/storage/common/resource.go#L329-L330
Added lines #L329 - L330 were not covered by tests
[warning] 338-342: internal/storage/common/resource.go#L338-L342
Added lines #L338 - L342 were not covered by tests
[warning] 344-344: internal/storage/common/resource.go#L344
Added line #L344 was not covered by tests
[warning] 382-383: internal/storage/common/resource.go#L382-L383
Added lines #L382 - L383 were not covered by tests
internal/storage/ledger/resource_accounts.go
[warning] 133-133: internal/storage/ledger/resource_accounts.go#L133
Added line #L133 was not covered by tests
[warning] 145-145: internal/storage/ledger/resource_accounts.go#L145
Added line #L145 was not covered by tests
[warning] 149-150: internal/storage/ledger/resource_accounts.go#L149-L150
Added lines #L149 - L150 were not covered by tests
internal/storage/ledger/resource_logs.go
[warning] 42-42: internal/storage/ledger/resource_logs.go#L42
Added line #L42 was not covered by tests
internal/storage/ledger/resource_aggregated_balances.go
[warning] 154-154: internal/storage/ledger/resource_aggregated_balances.go#L154
Added line #L154 was not covered by tests
internal/api/v2/controllers_balances.go
[warning] 29-29: internal/api/v2/controllers_balances.go#L29
Added line #L29 was not covered by tests
internal/api/v2/controllers_volumes.go
[warning] 56-56: internal/api/v2/controllers_volumes.go#L56
Added line #L56 was not covered by tests
internal/controller/ledger/store.go
[warning] 73-73: internal/controller/ledger/store.go#L73
Added line #L73 was not covered by tests
[warning] 129-129: internal/controller/ledger/store.go#L129
Added line #L129 was not covered by tests
internal/storage/ledger/resource_volumes.go
[warning] 228-228: internal/storage/ledger/resource_volumes.go#L228
Added line #L228 was not covered by tests
🪛 LanguageTool
docs/api/README.md
[uncategorized] ~136-~136: A punctuation mark might be missing here.
Context: ...uired|Description| |---|---|---|---|---| |pageSize|query|integer(int64)|false|The...
(AI_EN_LECTOR_MISSING_PUNCTUATION)
🔇 Additional comments (86)
docs/api/README.md (2)
120-121: Explicit Content-Type header added.
The addition of theContent-Type: application/jsonheader in the HTTP request example clearly signifies that the request body is JSON formatted. This improves clarity and aligns with best practices.
139-139: Update the body parameter description in the parameters table.
The parameters table entry currently reads:|body|body|object|true|none|In light of our new filtering feature, it would be beneficial to update the description to explain that this JSON object contains filtering criteria (for example, ledger name substring filtering or metadata matching). Note that a similar concern was raised in previous reviews.
internal/controller/ledger/mocks_test.go (1)
1-280: Generated mock code looks appropriate and follows best practices.This is a mockgen-generated file that creates mock implementations for various interfaces in the storage/common package. The mocks properly implement
PropertyValidator,RepositoryHandler,Resource, andPaginatedResourceinterfaces with appropriate generics support.The file appears to be generated with the correct parameters and follows Go best practices for testing with gomock. Since this is auto-generated code, no issues need to be addressed.
internal/controller/ledger/store.go (5)
6-8: New imports for the pagination and resource querying refactoring.The introduction of these imports aligns with the PR's goal of creating a resources package for sharing utilities across the system package. The
bunpaginateandpointerpackages are needed for the pagination functionality, while thecommonpackage contains the shared resource types.
61-65: Store interface now uses standardized types from common package.This change is significant as it migrates all resource-related return types to use the standardized types from the
commonpackage. This standardization:
- Ensures consistent pagination behavior across different resources
- Makes it easier to implement filtering on various resources, including ledgers
- Improves code maintainability by centralizing pagination and query logic
The generic type parameters used in each return type provide type safety while maintaining flexibility.
90-98: Refactored NewListLedgersQuery to use common.ColumnPaginatedQuery.The function has been updated to return a
common.ColumnPaginatedQuery[any]type instead of the previous implementation. This change enables:
- Standardized pagination with column-based ordering
- Default sorting by "id" in ascending order
- Support for the expand functionality through the empty expand slice
This implementation allows for filtering on the ledgers list as mentioned in the PR objectives.
129-129:❓ Verification inconclusive
Updated GetAccountsMetadata to use common.ResourceQuery.
Similar to the GetAccount method, this has been updated to use the standardized
common.ResourceQuerytype. This line is also flagged as not covered by tests.
🏁 Script executed:
#!/bin/bash # Check if there are any tests that would cover the numscriptRewriteAdapter.GetAccountsMetadata method rg -n "numscriptRewriteAdapter.*GetAccountsMetadata|mock.*GetAccountsMetadata" --type=go --glob="**/*_test.go"Length of output: 112
Update Verified – Test Coverage Needed
The updated
GetAccountsMetadatanow properly usescommon.ResourceQuery, aligning with theGetAccountimplementation. However, our verification steps did not yield any tests referencing this method. Please add or update unit tests to cover this functionality, ensuring both successful and error cases are handled.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 129-129: internal/controller/ledger/store.go#L129
Added line #L129 was not covered by tests
73-73:❓ Verification inconclusive
Updated GetAccount to use common.ResourceQuery.
The method now uses the standardized
common.ResourceQuerytype, consistent with the overall refactoring. However, this line is not covered by tests according to the static analysis.
🏁 Script executed:
#!/bin/bash # Check if there are any tests covering this specific method usage rg -n "GetOne.*ResourceQuery.*address" --type=go --glob="**/*_test.go"Length of output: 72
Attention: Missing Test Coverage for Updated GetAccount Method
The update to use
common.ResourceQueryis aligned with our refactoring goals. However, our static analysis still doesn’t reveal any tests exercising the updated call toGetOne. Please ensure that tests are added (or manually verify) that this usage scenario is covered.
- The grep using the pattern
"GetOne.*ResourceQuery.*address"returned no hits in test files.- It may be worth expanding the search, e.g., with a pattern like
"GetOne\s*\(.*common\.ResourceQuery\[", to confirm there really isn’t any test coverage.Once additional test coverage is confirmed or added, we can be more confident in the correctness of this change.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 73-73: internal/controller/ledger/store.go#L73
Added line #L73 was not covered by testsinternal/storage/system/resource_ledgers.go (6)
88-88: LGTM! Good use of type assertionUsing a type assertion to verify that
ledgersResourceHandlerimplements thecommon.RepositoryHandlerinterface is a good practice. This helps catch interface implementation errors at compile time.
19-46: LGTM! Well-defined filters with appropriate validationThe
Filtersmethod correctly defines filters for "bucket", "features", "metadata", and "name" with appropriate validators. The use of regular expressions for dynamic property names and the specific operators allowed for each filter type are well thought out.
48-52: LGTM! Simple and effective dataset buildingThe
BuildDatasetmethod efficiently creates a new select query for ledger records. It's straightforward and exactly what's needed for this case.
80-82: LGTM! Simple projection implementationThe
Projectmethod implementation is straightforward and correctly returns all columns.
84-86: LGTM! Clear error message for unsupported operationThe
Expandmethod correctly returns an error indicating that no expansion is available for ledgers. This is clear and appropriate if expansion is not supported for this resource type.
71-72:❓ Verification inconclusive
Add test coverage for metadata property handling
This particular case, which handles filtering by the existence of a metadata key, is not covered by tests according to code coverage analysis.
Please add test cases that verify the behavior of filtering by metadata existence using
property == "metadata".
🏁 Script executed:
#!/bin/bash # Find existing tests involving metadata filtering rg -l "metadata.*\$match" --type go testLength of output: 41
Test Coverage Needed for Filtering on "metadata" Property
It appears that our search for existing tests covering the handling of the metadata property yielded no results. To safeguard the behavior of filtering by metadata existence as implemented around lines 71–72 ofinternal/storage/system/resource_ledgers.go, please add test cases that explicitly verify this behavior.
- Action Item:
- Add unit tests ensuring that when
property == "metadata", the function returns the expected SQL string ("metadata -> ? is not null") along with the correctly passed parameter.Please manually verify that these tests adequately cover the filtering logic as no existing tests have been detected.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 71-72: internal/storage/system/resource_ledgers.go#L71-L72
Added lines #L71 - L72 were not covered by testsinternal/api/v2/controllers_balances.go (1)
4-4: LGTM: Migration to unified error handling.This import change aligns with the PR objective of creating a shared resources package. Good move towards standardization.
internal/storage/driver/driver_test.go (4)
9-9: Import usage looks correct.The newly added
"github.com/formancehq/go-libs/v2/query"import aligns with the code changes below wherequery.Matchis used.
94-95: Bucket filtering logic.Using
q.Options.Builder = query.Match("bucket", bucket)enables filtering at the query level rather than in-memory. Ensure the underlying implementation applies a database-level filter instead of filtering all ledgers in memory, especially if performance is a concern for large datasets.Would you like to confirm the DB-level filtering by searching for related SQL in the repository?
97-97: Consistent usage of the updated listing method.Calling
d.ListLedgers(ctx, q)with the new query object correctly applies your filter and pagination setup.
100-100: Validation of the returned ledger count.Requiring exactly two ledgers confirms that the filtering logic works as expected for this test scenario. This is a good, clear assertion.
internal/storage/system/store.go (4)
13-13: New common package import.This import properly references the shared utilities in
common, in line with the newly introduced pagination and resource abstractions.
21-21: New Ledgers method signature.Exposing
Ledgers()as acommon.PaginatedResourcefosters a consistent, reusable pagination system across the codebase.
94-97: Function signature for Ledgers().Returning
common.PaginatedResourcewith typed parameters is a neat way to standardize resource access for the ledger domain.
98-101: Instantiating a paginated resource repository.Providing default pagination column "id" in ascending order is a clear and straightforward setup for listing ledgers. This design promotes extensibility if future ordering changes are needed.
internal/storage/driver/system_generated_test.go (4)
17-17: Shift to the newcommonpackage.Replacing old imports with the
commonpackage is consistent with the refactoring in this PR.
136-137: New Ledgers mock method signature.Introducing the mock method to return
common.PaginatedResourceensures consistency with the actualStoreinterface.
139-141: Mock method invocation.Dispatching the call and casting to
common.PaginatedResourceis correct, mirroring the standard GoMock pattern.
144-145: Recorder for the new Ledgers method.Defining the recorder method supports testing scenarios by tracking calls to
Ledgers()in the mock.Also applies to: 147-147
internal/storage/common/resource.go (17)
18-33: Avoid panicking on unmapped operators🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 32-32: internal/storage/common/resource.go#L32
Added line #L32 was not covered by tests
35-38: Struct definition looks good
40-42: Interface declaration is straightforward
45-47: PropertyValidatorFunc implementation
49-56: Reusable operator validation🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 52-53: internal/storage/common/resource.go#L52-L53
Added lines #L52 - L53 were not covered by tests
65-83: RepositoryHandlerBuildContext structure
85-90: RepositoryHandler interface
92-146: Filter validation logic🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 117-117: internal/storage/common/resource.go#L117
Added line #L117 was not covered by tests
[warning] 129-130: internal/storage/common/resource.go#L129-L130
Added lines #L129 - L130 were not covered by tests
148-182: Building filtered dataset🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 160-161: internal/storage/common/resource.go#L160-L161
Added lines #L160 - L161 were not covered by tests
[warning] 172-173: internal/storage/common/resource.go#L172-L173
Added lines #L172 - L173 were not covered by tests
184-215: Expand functionality🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 195-196: internal/storage/common/resource.go#L195-L196
Added lines #L195 - L196 were not covered by tests
217-241: Single record retrieval🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 221-222: internal/storage/common/resource.go#L221-L222
Added lines #L221 - L222 were not covered by tests
[warning] 226-227: internal/storage/common/resource.go#L226-L227
Added lines #L226 - L227 were not covered by tests
[warning] 234-235: internal/storage/common/resource.go#L234-L235
Added lines #L234 - L235 were not covered by tests
243-252: Count method🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 247-248: internal/storage/common/resource.go#L247-L248
Added lines #L247 - L248 were not covered by tests
254-260: ResourceRepository constructor
262-314: PaginatedResourceRepository with paginator🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 278-279: internal/storage/common/resource.go#L278-L279
Added lines #L278 - L279 were not covered by tests
[warning] 289-290: internal/storage/common/resource.go#L289-L290
Added lines #L289 - L290 were not covered by tests
[warning] 294-295: internal/storage/common/resource.go#L294-L295
Added lines #L294 - L295 were not covered by tests
[warning] 301-302: internal/storage/common/resource.go#L301-L302
Added lines #L301 - L302 were not covered by tests
317-356: Mapper for paginated repository🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 329-330: internal/storage/common/resource.go#L329-L330
Added lines #L329 - L330 were not covered by tests
[warning] 338-342: internal/storage/common/resource.go#L338-L342
Added lines #L338 - L342 were not covered by tests
[warning] 344-344: internal/storage/common/resource.go#L344
Added line #L344 was not covered by tests
358-390: ResourceQuery struct and JSON unmarshal🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 382-383: internal/storage/common/resource.go#L382-L383
Added lines #L382 - L383 were not covered by tests
392-423: Resource and pagination typesinternal/storage/ledger/resource_logs.go (6)
6-6: Use of common package
10-12: logsResourceHandler with Store field
26-30: BuildDataset method
33-39: ResolveFilter with SQL operator conversion
46-48: Project method
50-50: Implementation of RepositoryHandler interfaceinternal/storage/ledger/resource_accounts.go (7)
6-6: Importing the common package
12-14: accountsResourceHandler with Store
16-51: Filters definition
53-86: BuildDataset for accounts
137-139: Project method
141-187: Expand method🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 145-145: internal/storage/ledger/resource_accounts.go#L145
Added line #L145 was not covered by tests
[warning] 149-150: internal/storage/ledger/resource_accounts.go#L149-L150
Added lines #L149 - L150 were not covered by tests
189-189: Implements RepositoryHandlerinternal/storage/ledger/resource_transactions.go (8)
5-5: Importing the common package
11-13: transactionsResourceHandler with Store
15-70: Filter definitions
72-123: BuildDataset for transactions
126-156: ResolveFilter method
158-160: Project method
162-193: Expand method for effectiveVolumes
195-195: Implements RepositoryHandlerinternal/storage/ledger/resource_volumes.go (4)
7-7: Import fromcommonlooks correct.
This change ensures consistency with the new shared types and filter definitions in thecommonpackage.
65-163:BuildDataset()logic is well-structured but warrants performance checks.
- The function branches between two query approaches (immediate volumes vs. moves history). This is correct but can become costly with large
JOIN LATERALclauses, especially if there are many accounts.- Ensure the indexes on
accounts_volumes,accounts, andmovesare well configured (especially for columns used inJOINandWHEREstatements).I can generate a shell script to confirm adequate indexes on columns like
accounts_address,ledger, andinsertion_dateif you’d like. Please let me know.
204-226:Project()provides grouping logic for hierarchical accounts.
- The intermediate subselect helps aggregate on addresses up to a specified hierarchy level (
Opts.GroupLvl).- Verify large ledger scenarios: repeated grouping on big data sets can be slow.
232-232: Type assertion tocommon.RepositoryHandler
This confirmsvolumesResourceHandlerimplements the required interface. Looks good.internal/storage/ledger/resource_aggregated_balances.go (5)
7-7: Import fromcommonis consistent with the refactor.
The transition to the sharedcommonpackage aligns well with other files.
16-49:Filters()method implements address/metadata filters.
- Reusing
validateAddressFilterandcommon.MetadataRegexensures consistency with the rest of the codebase.- Similar to
volumesResourceHandler, you might clarify or unify any custom error messages for consistency.
135-152:ResolveFilter()logic.
You properly handle “address” and “metadata” filters, returning a structured SQL snippet and arguments. This is consistent with the approach inresource_volumes.go.
158-171:Project()aggregates volumes by asset and builds JSON objects.
- Using
json_build_objectandpublic.aggregate_objectsis a neat approach.- Monitor performance for large result sets if the JSON object grows significantly.
173-173: Assertion tocommon.RepositoryHandler
This confirms the handler’s conformance to the interface. No concerns here.internal/controller/ledger/controller_default.go (12)
7-7: Refactor tocommonpackage import.
Good alignment with the new query types.
122-124:ListTransactionsusingcommon.ColumnPaginatedQuery[any].
This improves consistency with the new pagination queries. No issues noted.
126-128:CountTransactionssignature updated tocommon.ResourceQuery[any].
Straightforward transition. Ensure any mocking or test usage also updates to the new type.
130-132:GetTransactionnow usescommon.ResourceQuery[any].
Consistent with CountTransactions. This is good for unified filtering.
134-136:CountAccountswithcommon.ResourceQuery[any].
No concerns.
138-140:ListAccountswithcommon.OffsetPaginatedQuery[any].
Ensure pagination behaviors remain identical post-migration.
142-144:GetAccountusingcommon.ResourceQuery[any].
Nice unification of query handling.
146-152:GetAggregatedBalancesrefactored to usecommon.ResourceQuery[GetAggregatedVolumesOptions].
This preserves type safety on theOptswhile shifting to common queries.
154-156:ListLogswithcommon.ColumnPaginatedQuery[any].
Fits well with the standard pagination approach.
158-160:GetVolumesWithBalancesupdated tocommon.OffsetPaginatedQuery[GetVolumesOptions].
Matches the rest of the new resource/pagination pattern.
167-169:Importmethod usescommon.ColumnPaginatedQuery[any].
Query construction is consistent. Double-check any prior assumptions about the column or ordering.
270-274:Exportmethod now leveragescommon.ColumnPaginatedQuery[any].
The iteration logic withbunpaginate.Iterateremains intact. This should work seamlessly.
| @@ -25,7 +26,7 @@ | |||
| balances, err := common.LedgerFromContext(r.Context()).GetAggregatedBalances(r.Context(), *rq) | |||
| if err != nil { | |||
| switch { | |||
| case errors.Is(err, ledgercontroller.ErrInvalidQuery{}) || errors.Is(err, ledgercontroller.ErrMissingFeature{}): | |||
| case errors.Is(err, storagecommon.ErrInvalidQuery{}) || errors.Is(err, ledgercontroller.ErrMissingFeature{}): | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add test coverage for the updated error handling logic.
This line has been modified to use the new storagecommon.ErrInvalidQuery{} instead of the legacy error type, but it's not covered by tests according to codecov. Consider adding tests to verify that both error types (storagecommon.ErrInvalidQuery{} and ledgercontroller.ErrMissingFeature{}) are handled correctly.
🏁 Script executed:
#!/bin/bash
# Check if there are any tests for error handling in this controller
rg -A 10 -B 10 "controllers_balances.*test" --type go
# Check if there are any tests for ErrInvalidQuery from storagecommon
rg "storagecommon\.ErrInvalidQuery" --type go --glob "**/*_test.go"Length of output: 758
Action Required: Add tests for error handling in internal/api/v2/controllers_balances.go
The updated switch case now checks for both storagecommon.ErrInvalidQuery{} and ledgercontroller.ErrMissingFeature{}. Although tests exist in other controllers for storagecommon.ErrInvalidQuery, our codecov data still indicates a gap in coverage for this controller. Please add tests that simulate error conditions for both storagecommon.ErrInvalidQuery{} and ledgercontroller.ErrMissingFeature{} to ensure this logic is properly exercised.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 29-29: internal/api/v2/controllers_balances.go#L29
Added line #L29 was not covered by tests
| @@ -5,6 +5,7 @@ | |||
| "github.com/formancehq/go-libs/v2/api" | |||
| "github.com/formancehq/ledger/internal/api/common" | |||
| ledgercontroller "github.com/formancehq/ledger/internal/controller/ledger" | |||
| storagecommon "github.com/formancehq/ledger/internal/storage/common" | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Code refactoring looks good, but needs test coverage.
The change to use storagecommon.ErrInvalidQuery{} instead of ledgercontroller.ErrInvalidQuery{} aligns with the PR objective of creating a shared resources package. This refactoring centralizes error definitions, making them more consistent across the codebase.
Static analysis indicates that the modified error handling logic on line 56 isn't covered by tests. Consider adding a test case that triggers the storagecommon.ErrInvalidQuery{} condition to ensure the error handling works as expected after this change.
Also applies to: 56-56
🏁 Script executed:
#!/bin/bash
# Verify if there are tests covering the new error type
rg -B2 -A2 "storagecommon.ErrInvalidQuery" --type go --glob "*_test.go"Length of output: 2780
Action Required: Add test coverage for volumes controller error handling
The refactoring to use storagecommon.ErrInvalidQuery{} in place of ledgercontroller.ErrInvalidQuery{} aligns well with our goal of centralizing error definitions for consistency. Our search confirms that several controllers (ledgers, transactions, logs, and accounts) already have tests triggering this error condition. However, it appears that the volumes controller (in internal/api/v2/controllers_volumes.go, line 56) does not yet have dedicated tests for this error path.
- Location:
internal/api/v2/controllers_volumes.go(line 56) - Recommendation: Add a test case that simulates an invalid query scenario for the volumes controller to ensure its error handling behaves as expected.
gfyrag
force-pushed
the
feat/ledgers-endpoint-filtering
branch
from
2db208d to
3101065
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
internal/storage/common/resource.go (1)
18-33:⚠️ Potential issueFix potential panic in the
ConvertOperatorToSQLfunction.The function should handle unexpected operators gracefully rather than panicking, which could cause application crashes in production.
func ConvertOperatorToSQL(operator string) string { switch operator { // existing cases... } - panic("unreachable") + // Return a default operator or error handling for unknown operators + return "=" // Default to equality operator as a fallback }internal/storage/system/resource_ledgers.go (2)
58-63:⚠️ Potential issueSame potential index out of bounds error for features.
This reproduces the concern from the previous review regarding regex submatches. When
featuresRegexdoes not match or has insufficient capturing groups,match[0][1]will panic.Here’s an example fix you can apply:
case featuresRegex.Match([]byte(property)): match := featuresRegex.FindAllStringSubmatch(property, 3) + if len(match) == 0 || len(match[0]) < 2 { + return "", nil, common.NewErrInvalidQuery("invalid feature property format %s", property) + } return "features @> ?", []any{map[string]any{ match[0][1]: value, }}, nil
64-69:⚠️ Potential issueSame potential index out of bounds error for metadata.
This also reproduces the concern from the previous review. When
common.MetadataRegexdoes not match or has insufficient capturing groups,match[0][1]will panic.Here’s an example fix you can apply:
case common.MetadataRegex.Match([]byte(property)): match := common.MetadataRegex.FindAllStringSubmatch(property, 3) + if len(match) == 0 || len(match[0]) < 2 { + return "", nil, common.NewErrInvalidQuery("invalid metadata property format %s", property) + } return "metadata @> ?", []any{map[string]any{ match[0][1]: value, }}, nil
🧹 Nitpick comments (6)
docs/api/README.md (2)
127-132: Body Parameter Example Block Clarification:
The new section introducing the body parameter shows an example payload ({}). To further enhance usability, consider adding documentation or an inline comment that explains which filtering criteria (such as ledger names or metadata properties) are supported. This extra detail can help guide users on how to structure their requests.
136-136: Static Analysis – Punctuation Check in Table Headers:
A static analysis hint indicates that a punctuation mark might be missing in the table header section (around line 136). Please review the header formatting of the parameters table to ensure that all punctuation and separators are correct for consistency and readability.🧰 Tools
🪛 LanguageTool
[uncategorized] ~136-~136: A punctuation mark might be missing here.
Context: ...uired|Description| |---|---|---|---|---| |pageSize|query|integer(int64)|false|The...(AI_EN_LECTOR_MISSING_PUNCTUATION)
test/e2e/api_ledgers_list_test.go (3)
57-68: Test description doesn't match the assertion.The test name states "filtering on bucket should return 5 ledgers" but the assertion checks for 10 ledgers. Given the setup (20 ledgers with alternating buckets), 10 ledgers is the correct expectation.
- It("filtering on bucket should return 5 ledgers", func() { + It("filtering on bucket should return 10 ledgers", func() {
69-80: Minor test description inconsistency.The test description says "filtering on metadata[foo] = 0" but the actual filter is checking for "bar0", not "0".
- It("filtering on metadata[foo] = 0 should return 7 ledgers", func() { + It("filtering on metadata[foo] = bar0 should return 7 ledgers", func() {
93-104: Grammar issue in test description.There's a grammatical error in the test description: "should return 11 ledger" should be "should return 11 ledgers" (plural).
- It("filtering on name starting with ledger1 should return 11 ledger", func() { + It("filtering on name starting with ledger1 should return 11 ledgers", func() {The test correctly verifies the substring filtering functionality using the
$likeoperator with a SQL-like pattern. This matches ledger1, ledger10-19 (11 ledgers total).internal/storage/system/resource_ledgers.go (1)
80-86: Consider removing or extending these empty methods.Both
ProjectandExpandsimply pass through or return an unimplemented error. If no projection logic or expansions are expected in the future, you could remove or comment them out to reduce clutter; otherwise, consider implementing them fully or marking them with a TODO for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumopenapi.yamlis excluded by!**/*.yamlpkg/client/.speakeasy/gen.lockis excluded by!**/*.lock,!**/*.locktools/generator/go.modis excluded by!**/*.modtools/generator/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (87)
docs/api/README.md(1 hunks)internal/api/bulking/mocks_ledger_controller_test.go(10 hunks)internal/api/common/mocks_ledger_controller_test.go(10 hunks)internal/api/common/mocks_system_controller_test.go(2 hunks)internal/api/v1/controllers_accounts_count.go(2 hunks)internal/api/v1/controllers_accounts_count_test.go(7 hunks)internal/api/v1/controllers_accounts_list_test.go(8 hunks)internal/api/v1/controllers_accounts_read.go(2 hunks)internal/api/v1/controllers_accounts_read_test.go(5 hunks)internal/api/v1/controllers_balances_aggregates_test.go(3 hunks)internal/api/v1/controllers_config.go(2 hunks)internal/api/v1/controllers_logs_list_test.go(5 hunks)internal/api/v1/controllers_transactions_count_test.go(9 hunks)internal/api/v1/controllers_transactions_list_test.go(12 hunks)internal/api/v1/controllers_transactions_read_test.go(2 hunks)internal/api/v1/mocks_ledger_controller_test.go(10 hunks)internal/api/v1/mocks_system_controller_test.go(2 hunks)internal/api/v1/utils.go(5 hunks)internal/api/v2/common.go(6 hunks)internal/api/v2/controllers_accounts_count.go(2 hunks)internal/api/v2/controllers_accounts_count_test.go(10 hunks)internal/api/v2/controllers_accounts_list.go(2 hunks)internal/api/v2/controllers_accounts_list_test.go(12 hunks)internal/api/v2/controllers_accounts_read.go(2 hunks)internal/api/v2/controllers_accounts_read_test.go(3 hunks)internal/api/v2/controllers_balances.go(2 hunks)internal/api/v2/controllers_balances_test.go(6 hunks)internal/api/v2/controllers_ledgers_list.go(1 hunks)internal/api/v2/controllers_ledgers_list_test.go(3 hunks)internal/api/v2/controllers_logs_list.go(2 hunks)internal/api/v2/controllers_logs_list_test.go(7 hunks)internal/api/v2/controllers_transactions_count.go(2 hunks)internal/api/v2/controllers_transactions_count_test.go(13 hunks)internal/api/v2/controllers_transactions_list.go(2 hunks)internal/api/v2/controllers_transactions_list_test.go(14 hunks)internal/api/v2/controllers_transactions_read.go(2 hunks)internal/api/v2/controllers_transactions_read_test.go(2 hunks)internal/api/v2/controllers_volumes.go(2 hunks)internal/api/v2/controllers_volumes_test.go(8 hunks)internal/api/v2/mocks_ledger_controller_test.go(10 hunks)internal/api/v2/mocks_system_controller_test.go(2 hunks)internal/controller/ledger/controller.go(2 hunks)internal/controller/ledger/controller_default.go(4 hunks)internal/controller/ledger/controller_default_test.go(10 hunks)internal/controller/ledger/controller_generated_test.go(10 hunks)internal/controller/ledger/controller_with_traces.go(10 hunks)internal/controller/ledger/mocks.go(1 hunks)internal/controller/ledger/mocks_test.go(1 hunks)internal/controller/ledger/stats.go(2 hunks)internal/controller/ledger/stats_test.go(2 hunks)internal/controller/ledger/store.go(4 hunks)internal/controller/ledger/store_generated_test.go(7 hunks)internal/controller/system/controller.go(3 hunks)internal/controller/system/store.go(2 hunks)internal/storage/common/errors.go(1 hunks)internal/storage/common/metadata.go(1 hunks)internal/storage/common/paginator.go(1 hunks)internal/storage/common/paginator_column.go(5 hunks)internal/storage/common/paginator_offset.go(3 hunks)internal/storage/common/resource.go(1 hunks)internal/storage/driver/driver.go(2 hunks)internal/storage/driver/driver_test.go(2 hunks)internal/storage/driver/system_generated_test.go(2 hunks)internal/storage/ledger/accounts_test.go(23 hunks)internal/storage/ledger/adapters.go(2 hunks)internal/storage/ledger/balances_test.go(10 hunks)internal/storage/ledger/logs_test.go(4 hunks)internal/storage/ledger/moves_test.go(2 hunks)internal/storage/ledger/resource_accounts.go(3 hunks)internal/storage/ledger/resource_aggregated_balances.go(5 hunks)internal/storage/ledger/resource_logs.go(1 hunks)internal/storage/ledger/resource_transactions.go(7 hunks)internal/storage/ledger/resource_volumes.go(10 hunks)internal/storage/ledger/store.go(2 hunks)internal/storage/ledger/transactions.go(0 hunks)internal/storage/ledger/transactions_test.go(13 hunks)internal/storage/ledger/volumes_test.go(30 hunks)internal/storage/system/resource_ledgers.go(1 hunks)internal/storage/system/store.go(2 hunks)internal/storage/system/store_test.go(3 hunks)internal/worker/async_block.go(2 hunks)pkg/client/docs/models/operations/v2listledgersrequest.md(1 hunks)pkg/client/docs/sdks/v2/README.md(1 hunks)pkg/client/models/operations/v2listledgers.go(2 hunks)pkg/client/v2.go(2 hunks)test/e2e/api_ledgers_list_test.go(2 hunks)test/e2e/api_transactions_list_test.go(3 hunks)
💤 Files with no reviewable changes (1)
- internal/storage/ledger/transactions.go
🚧 Files skipped from review as they are similar to previous changes (75)
- internal/storage/common/metadata.go
- internal/controller/ledger/mocks.go
- internal/storage/ledger/logs_test.go
- internal/api/v2/controllers_transactions_count_test.go
- internal/controller/system/store.go
- internal/api/v2/controllers_accounts_count.go
- pkg/client/docs/models/operations/v2listledgersrequest.md
- internal/api/v1/controllers_transactions_read_test.go
- internal/api/v2/controllers_accounts_list.go
- internal/api/v2/controllers_transactions_read.go
- pkg/client/models/operations/v2listledgers.go
- internal/api/v1/controllers_config.go
- internal/controller/ledger/controller_default_test.go
- internal/controller/ledger/stats.go
- internal/api/v2/controllers_transactions_list.go
- internal/api/v1/controllers_accounts_count.go
- internal/api/v1/mocks_system_controller_test.go
- internal/api/v2/mocks_system_controller_test.go
- internal/storage/common/errors.go
- pkg/client/docs/sdks/v2/README.md
- internal/api/common/mocks_system_controller_test.go
- internal/api/v1/controllers_transactions_count_test.go
- internal/api/v2/controllers_volumes_test.go
- internal/api/v2/controllers_balances.go
- internal/storage/ledger/moves_test.go
- internal/api/v2/controllers_ledgers_list.go
- internal/api/v2/controllers_logs_list_test.go
- internal/api/v2/controllers_ledgers_list_test.go
- internal/storage/system/store_test.go
- internal/storage/ledger/adapters.go
- internal/api/v1/controllers_accounts_count_test.go
- internal/storage/ledger/volumes_test.go
- internal/api/v2/controllers_logs_list.go
- internal/worker/async_block.go
- internal/controller/ledger/stats_test.go
- internal/api/v1/controllers_transactions_list_test.go
- internal/api/v2/controllers_accounts_list_test.go
- internal/api/v1/controllers_logs_list_test.go
- internal/storage/common/paginator.go
- internal/api/v2/controllers_accounts_read.go
- internal/api/v2/controllers_balances_test.go
- internal/api/v1/controllers_accounts_list_test.go
- test/e2e/api_transactions_list_test.go
- internal/api/v1/controllers_balances_aggregates_test.go
- internal/api/v2/controllers_transactions_count.go
- internal/api/v2/controllers_transactions_read_test.go
- pkg/client/v2.go
- internal/storage/ledger/balances_test.go
- internal/api/v2/controllers_accounts_read_test.go
- internal/api/v2/controllers_volumes.go
- internal/controller/system/controller.go
- internal/storage/driver/driver.go
- internal/api/v2/controllers_transactions_list_test.go
- internal/storage/ledger/store.go
- internal/storage/ledger/accounts_test.go
- internal/api/v2/controllers_accounts_count_test.go
- internal/storage/ledger/transactions_test.go
- internal/api/v2/common.go
- internal/storage/driver/driver_test.go
- internal/storage/common/paginator_column.go
- internal/storage/common/paginator_offset.go
- internal/api/v1/controllers_accounts_read_test.go
- internal/controller/ledger/store_generated_test.go
- internal/controller/ledger/controller_with_traces.go
- internal/api/v1/controllers_accounts_read.go
- internal/api/v1/utils.go
- internal/controller/ledger/controller.go
- internal/controller/ledger/store.go
- internal/api/bulking/mocks_ledger_controller_test.go
- internal/storage/ledger/resource_transactions.go
- internal/storage/ledger/resource_volumes.go
- internal/controller/ledger/controller_generated_test.go
- internal/api/v2/mocks_ledger_controller_test.go
- internal/api/common/mocks_ledger_controller_test.go
- internal/api/v1/mocks_ledger_controller_test.go
🧰 Additional context used
🧬 Code Definitions (6)
test/e2e/api_ledgers_list_test.go (1)
pkg/client/models/operations/v2listledgers.go (1)
V2ListLedgersRequest(9-20)
internal/storage/system/store.go (2)
internal/storage/common/resource.go (3)
PaginatedResource(420-423)ColumnPaginatedQuery(405-414)NewPaginatedResourceRepository(307-315)internal/storage/common/paginator_column.go (1)
ColumnPaginator(14-17)
internal/storage/system/resource_ledgers.go (4)
internal/storage/system/store.go (2)
DefaultStore(34-36)New(125-129)internal/storage/common/resource.go (8)
Filter(58-63)PropertyValidator(40-42)AcceptOperators(49-56)RepositoryHandlerBuildContext(65-68)ResourceQuery(358-364)ConvertOperatorToSQL(18-33)JoinCondition(35-38)RepositoryHandler(84-90)internal/storage/common/metadata.go (1)
MetadataRegex(6-6)internal/storage/common/errors.go (1)
NewErrInvalidQuery(18-22)
internal/storage/ledger/resource_aggregated_balances.go (5)
internal/storage/ledger/store.go (2)
Store(24-46)New(161-248)internal/storage/common/resource.go (7)
Filter(58-63)PropertyValidator(40-42)PropertyValidatorFunc(43-43)RepositoryHandlerBuildContext(65-68)ResourceQuery(358-364)JoinCondition(35-38)RepositoryHandler(84-90)internal/storage/common/metadata.go (1)
MetadataRegex(6-6)internal/controller/ledger/errors.go (1)
NewErrMissingFeature(106-110)internal/storage/common/errors.go (1)
NewErrInvalidQuery(18-22)
internal/controller/ledger/controller_default.go (1)
internal/storage/common/resource.go (3)
ColumnPaginatedQuery(405-414)ResourceQuery(358-364)OffsetPaginatedQuery(398-404)
internal/controller/ledger/mocks_test.go (3)
internal/controller/ledger/controller.go (1)
Controller(18-78)internal/controller/system/controller.go (1)
Controller(23-34)internal/storage/common/resource.go (5)
RepositoryHandlerBuildContext(65-68)ResourceQuery(358-364)JoinCondition(35-38)Filter(58-63)PaginatedQuery(415-417)
🪛 LanguageTool
docs/api/README.md
[uncategorized] ~136-~136: A punctuation mark might be missing here.
Context: ...uired|Description| |---|---|---|---|---| |pageSize|query|integer(int64)|false|The...
(AI_EN_LECTOR_MISSING_PUNCTUATION)
🔇 Additional comments (47)
docs/api/README.md (1)
120-121: Content-Type Header Addition:
The HTTP request sample forGET /v2now includes theContent-Type: application/jsonheader, which clearly indicates that the request body should be in JSON format. This addition improves clarity for API consumers.test/e2e/api_ledgers_list_test.go (4)
30-31: Pagination settings updated correctly.The maximum and default page sizes have been increased from 5 to 15, which aligns with the pagination changes mentioned in the PR objectives and matches the constraint described in the
V2ListLedgersRequeststruct from the client models.
37-46: Test data setup enhanced for filter testing.The test now creates 20 ledgers instead of 10, with systematic variations in bucket and metadata values that will help test filtering functionality:
- Bucket values alternate between "bucket0" and "bucket1" (using modulo 2)
- Metadata "foo" values cycle through "bar0", "bar1", "bar2" (using modulo 3)
This setup provides a good foundation for the new filtering test cases.
50-56: Default pagination correctly handled.The test now expects 15 ledgers in the first page (matching the new
DefaultPageSize), rather than the previous expectation of 5 ledgers.
81-92: Good test for exact ledger name matching.This test correctly verifies the functionality to filter ledgers by exact name match.
internal/controller/ledger/controller_default.go (9)
7-7: Import addition for common package - LGTMThe new import for the common package properly supports the migration of paginated query types from legacy packages to a unified location.
122-124: Method signature update maintains compatibilityUpdating the method signature to use
common.ColumnPaginatedQuery[any]instead of a previous type maintains the same functionality while leveraging the centralized common package. The method implementation remains unchanged, indicating compatibility between the types.
126-132: Consistent update of resource query typesBoth
CountTransactionsandGetTransactionmethod signatures have been updated to usecommon.ResourceQuery[any]consistently, maintaining the same generic type parameter and preserving type safety.
134-144: Accounts API methods correctly updatedThe account methods (
CountAccounts,ListAccounts, andGetAccount) have been properly updated to use the corresponding types from the common package while maintaining their respective pagination approaches (resource-based, offset-based, and resource-based).
146-156: Complex query types correctly migratedThe specialized query types for
GetAggregatedBalancesandListLogshave been properly migrated to use the common package while preserving their specific type parameters.
158-160: Volumes API updated with correct generic typeThe
GetVolumesWithBalancesmethod correctly maintains its specific options type (GetVolumesOptions) while using the common package's pagination structure.
167-169: Import method pagination query updatedThe
Importmethod implementation has been updated to use the common package's pagination query type, maintaining functionality consistency with the method signature changes.
270-274: Export method correctly uses common pagination typesThe
Exportmethod's implementation properly uses the common package's pagination query types, maintaining consistent usage throughout the controller.
1-527:✅ Verification successful
Verify API compatibility with callers
The refactoring to use types from the common package looks thorough and consistent throughout the file. However, it's important to verify that all callers of these methods have been updated to use the new types.
🏁 Script executed:
#!/bin/bash # Look for imports of the legacy packages that might need updating echo "Checking for imports of legacy packages that might need updating:" rg -l "import\s+\([^)]*?(ledgercontroller|ledger0|storagecommon)" --type go | grep -v "internal/storage/common" # Look for any usages of the old types that might need updating echo "Checking for potential usages of old query types:" rg "(ResourceQuery|ColumnPaginatedQuery|OffsetPaginatedQuery)" --type go | grep -v "internal/storage/common" | head -n 20Length of output: 3469
API Compatibility Verified: Caller Updates Confirmed
Your changes using the new types from the common package are consistent across the codebase. Our search shows that all callers—including those in e2e tests (e.g.,
test/e2e/api_transactions_list_test.go), system controllers, and related mocks—are using the updated query types (e.g.,ResourceQuery,ColumnPaginatedQuery,OffsetPaginatedQuery). No legacy package references (like those forledgercontroller,ledger0, orstoragecommon) were found outside of the intended locations.internal/storage/system/store.go (3)
13-13: Good addition of the common package import.This import aligns with the refactoring goal of creating a common resources package for sharing utilities.
21-21: Interface method signature change looks good.The change from
ListLedgersmethod toLedgersmethod represents a shift from a query-based approach to a more structured paginated resource approach, which should improve consistency across the codebase.
94-102: Implementation of the Ledgers method follows best practices.The implementation of the
Ledgersmethod effectively uses the common paginated resource repository pattern with appropriate defaults:
- Uses the handler pattern with proper encapsulation of the store
- Sets up column pagination with sensible defaults for the pagination column and order
- Follows the generic pattern established by the common package
This implementation will help standardize ledger listing and pagination.
internal/storage/common/resource.go (2)
262-266: Good implementation of the PaginatedResourceRepository.The struct is well-designed with composition of ResourceRepository and inclusion of a paginator. This design promotes code reuse and follows the single responsibility principle.
358-364: Well-structured ResourceQuery type definition.The ResourceQuery type is well-defined with appropriate fields for point-in-time queries, filtering, and expansion capabilities. Using generics for the Options type allows for flexible, type-safe usage across different resource types.
internal/storage/ledger/resource_logs.go (6)
10-12: Good refactoring of logsResourceHandler struct.Adding the store field to the struct allows for better encapsulation and eliminates the need to pass the store as a parameter to each method, which is a more object-oriented approach.
14-24: Method signature standardization looks good.Renaming from
filters()toFilters()and updating the return type to use the common filter type enhances consistency across the codebase.
26-31: BuildDataset implementation is efficient.The method now uses the encapsulated store field to access the database, which simplifies the method signature and makes the code more maintainable.
33-40: ResolveFilter implementation leverages common utilities.Using
common.ConvertOperatorToSQL(operator)instead of a local function ensures consistency in SQL operator mapping across the application.
42-48: Expand method correctly utilizes common types.The method now returns a common JoinCondition type, aligning with the standardized resource handling approach throughout the system.
50-50: Type assertion confirms interface implementation.The type assertion ensures that the
logsResourceHandlerproperly implements thecommon.RepositoryHandler[any]interface, which is good practice for compile-time verification.internal/storage/ledger/resource_accounts.go (5)
12-14: Good encapsulation pattern for accountsResourceHandler.Adding the store field to the struct follows the same pattern as seen in other resource handlers, ensuring consistency across implementations.
16-51: Well-structured Filters implementation with appropriate validators.The Filters method properly defines the allowed filters for accounts with appropriate validators for each property. Using
common.PropertyValidatorandcommon.AcceptOperatorsensures consistent validation across the system.
88-135: ResolveFilter has comprehensive handling for different filter properties.The method handles various filter types (address, first_usage, balance, metadata) with appropriate query transformations. The use of feature flags to conditionally enable certain queries is a good practice.
141-187: Expand method includes proper feature validation.The method checks for required features before allowing expansion operations, which is good defensive programming. It prevents users from accessing functionality that requires specific features to be enabled.
189-189: Type assertion confirms interface implementation.The type assertion ensures that the
accountsResourceHandlerproperly implements thecommon.RepositoryHandler[any]interface, providing compile-time verification.internal/storage/ledger/resource_aggregated_balances.go (7)
12-14: Good structural improvementAdding the
storefield to the struct eliminates the need to pass it as a parameter to every method, simplifying the interface and improving encapsulation.
16-49: Interface alignment with common packageThe method has been properly renamed to
Filters()with an uppercase first letter to make it exportable, and now returns the standardized[]common.Filtertype. The structure of filters has been updated to use the standardized field names and validators from the common package.
51-133: Improved method signature and consistent store usageThe
BuildDatasetmethod now accepts a standardized context type and consistently usesh.storeinstead of a passed store parameter. This improves code consistency and readability.
135-152: Standardized error handlingThe
ResolveFiltermethod now uses standardized error handling withcommon.NewErrInvalidQueryand correctly references the common package's regex utilities.
154-156: Interface alignment for Expand methodThe
Expandmethod now returns a pointer to the standardizedcommon.JoinConditiontype, maintaining consistency with the common package interfaces.
158-171: Removed redundant parameterThe
Projectmethod no longer needs the store parameter since it can accessh.storedirectly from the struct field, making the method signature cleaner.
173-173: Updated type assertion for interface complianceThe type assertion has been updated to verify compliance with the common package's interface definition.
internal/controller/ledger/mocks_test.go (6)
1-6: Correctly configured mock generationThe mockgen command is properly configured to generate mocks for the specified interfaces. The source file path and destination are correctly set up.
10-18: Appropriate imports for mock functionalityThe imports include all necessary packages for mock functionality, including context, reflect, bunpaginate, common, bun, and gomock.
20-56: Well-structured PropertyValidator mockThe MockPropertyValidator implementation correctly implements the PropertyValidator interface with a Validate method that matches the expected signature.
58-156: Complete and type-safe RepositoryHandler mockThe MockRepositoryHandler implements all required methods of the RepositoryHandler interface with proper generic type parameters. Each method is correctly implemented to record and verify calls during testing.
158-210: Generic implementation of Resource mockThe MockResource is implemented with proper generic type parameters for both the resource type and options type, ensuring type safety in tests.
212-279: Advanced generic PaginatedResource mockThe MockPaginatedResource correctly implements the PaginatedResource interface with three generic type parameters, including a constrained PaginationQueryType that must satisfy the common.PaginatedQuery interface.
internal/storage/driver/system_generated_test.go (3)
17-17: No concerns about the new import.
It aligns with the usage ofcommon.PaginatedResource.
144-148: The mock recorder for 'Ledgers' is consistent with the new method signature.
No issues spotted here.
136-142:❓ Verification inconclusive
Consider verifying whether error handling is needed for the new method.
The newLedgers()method no longer returns an error, which might be correct if the underlying logic never fails. Otherwise, ensure that all error cases are properly handled upstream.Here's a script to help confirm there's no usage that expects an error:
🏁 Script executed:
#!/bin/bash # Searching for usage of the Ledgers() method rg -A 5 'Ledgers('Length of output: 87
Below is a revised verification step. The initial grep query failed because the parenthesis wasn’t escaped, so it didn’t yield any information. To proceed, please run the following updated shell script to search for any usage of the
Ledgers()method (with the parenthesis properly escaped):#!/bin/bash # Searching for usages of the Ledgers() method with escaped parenthesis rg -A 5 'Ledgers\('Once you’ve run this command, please manually verify whether any calling code is expecting an error (which the new
Ledgers()implementation no longer returns). If nothing indicates a dependency on an error return, then the change is likely safe. Otherwise, consider adding appropriate upstream error handling.Action Required: Please execute the above script and manually verify the result.
internal/storage/system/resource_ledgers.go (1)
19-46: Filters implementation looks good with basic operators.The usage of
$matchand$likefor the "name" property is consistent. If you plan to allow partial matching or a wider range of operators for other properties like "bucket," consider adding them to the property validators.
| <h3 id="list-ledgers-parameters">Parameters</h3> | ||
|
|
||
| |Name|In|Type|Required|Description| | ||
| |---|---|---|---|---| | ||
| |pageSize|query|integer(int64)|false|The maximum number of results to return per page.| | ||
| |cursor|query|string|false|Parameter used in pagination requests. Maximum page size is set to 15.| | ||
| |body|body|object|true|none| |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve 'body' Parameter Description in the Table:
The parameters table now includes a row for the body parameter with a type of object and a placeholder description of none. It would be beneficial to provide a concise description such as:
"JSON object containing filtering criteria for ledger listing; supports filtering by ledger name (including substring matching) and metadata."
This update will enhance the documentation's clarity.
| ColumnPaginatedQuery[OptionsType any] struct { | ||
| PageSize uint64 `json:"pageSize"` | ||
| Bottom *big.Int `json:"bottom"` | ||
| Column string `json:"column"` | ||
| PaginationID *big.Int `json:"paginationID"` | ||
| // todo: backport in go-libs | ||
| Order *bunpaginate.Order `json:"order"` | ||
| Options ResourceQuery[OptionsType] `json:"filters"` | ||
| Reverse bool `json:"reverse"` | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding documentation for the TODO comment.
There's a TODO comment in the ColumnPaginatedQuery struct that should be tracked in your issue tracker to ensure it gets addressed in a future update.
🏁 Script executed:
#!/bin/bash
# Search for TODO comments in the file to identify items needing attention
grep -n "todo:" internal/storage/common/resource.goLength of output: 86
Document the TODO Tracking in Your Issue Tracker
The TODO comment at line 410 in internal/storage/common/resource.go ("todo: backport in go-libs") is still present. Please add documentation for this TODO by linking it to a corresponding issue/ticket in your tracker. This will help ensure that the backporting work is properly follow‑up‑tracked for future updates.
| func (r *PaginatedResourceRepository[ResourceType, OptionsType, PaginationQueryType]) Paginate( | ||
| ctx context.Context, | ||
| paginationOptions PaginationQueryType, | ||
| ) (*bunpaginate.Cursor[ResourceType], error) { | ||
|
|
||
| var resourceQuery ResourceQuery[OptionsType] | ||
| switch v := any(paginationOptions).(type) { | ||
| case OffsetPaginatedQuery[OptionsType]: | ||
| resourceQuery = v.Options | ||
| case ColumnPaginatedQuery[OptionsType]: | ||
| resourceQuery = v.Options | ||
| default: | ||
| panic("should not happen") | ||
| } | ||
|
|
||
| finalQuery, err := r.buildFilteredDataset(resourceQuery) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("building filtered dataset: %w", err) | ||
| } | ||
|
|
||
| finalQuery, err = r.paginator.Paginate(finalQuery, paginationOptions) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("paginating request: %w", err) | ||
| } | ||
|
|
||
| finalQuery, err = r.expand(finalQuery, resourceQuery) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("expanding results: %w", err) | ||
| } | ||
| finalQuery = finalQuery.Order("row_number") | ||
|
|
||
| ret := make([]ResourceType, 0) | ||
| err = finalQuery.Model(&ret).Scan(ctx) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("scanning results: %w", err) | ||
| } | ||
|
|
||
| return r.paginator.BuildCursor(ret, paginationOptions) | ||
| } |
There was a problem hiding this comment.
Avoid panic in type switch for pagination options.
In the Paginate method, there's a panic statement that could lead to runtime crashes when encountering unexpected pagination types.
func (r *PaginatedResourceRepository[ResourceType, OptionsType, PaginationQueryType]) Paginate(
ctx context.Context,
paginationOptions PaginationQueryType,
) (*bunpaginate.Cursor[ResourceType], error) {
var resourceQuery ResourceQuery[OptionsType]
switch v := any(paginationOptions).(type) {
case OffsetPaginatedQuery[OptionsType]:
resourceQuery = v.Options
case ColumnPaginatedQuery[OptionsType]:
resourceQuery = v.Options
default:
- panic("should not happen")
+ return nil, fmt.Errorf("unsupported pagination query type: %T", paginationOptions)
}
// ... rest of the function
}📝 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 (r *PaginatedResourceRepository[ResourceType, OptionsType, PaginationQueryType]) Paginate( | |
| ctx context.Context, | |
| paginationOptions PaginationQueryType, | |
| ) (*bunpaginate.Cursor[ResourceType], error) { | |
| var resourceQuery ResourceQuery[OptionsType] | |
| switch v := any(paginationOptions).(type) { | |
| case OffsetPaginatedQuery[OptionsType]: | |
| resourceQuery = v.Options | |
| case ColumnPaginatedQuery[OptionsType]: | |
| resourceQuery = v.Options | |
| default: | |
| panic("should not happen") | |
| } | |
| finalQuery, err := r.buildFilteredDataset(resourceQuery) | |
| if err != nil { | |
| return nil, fmt.Errorf("building filtered dataset: %w", err) | |
| } | |
| finalQuery, err = r.paginator.Paginate(finalQuery, paginationOptions) | |
| if err != nil { | |
| return nil, fmt.Errorf("paginating request: %w", err) | |
| } | |
| finalQuery, err = r.expand(finalQuery, resourceQuery) | |
| if err != nil { | |
| return nil, fmt.Errorf("expanding results: %w", err) | |
| } | |
| finalQuery = finalQuery.Order("row_number") | |
| ret := make([]ResourceType, 0) | |
| err = finalQuery.Model(&ret).Scan(ctx) | |
| if err != nil { | |
| return nil, fmt.Errorf("scanning results: %w", err) | |
| } | |
| return r.paginator.BuildCursor(ret, paginationOptions) | |
| } | |
| func (r *PaginatedResourceRepository[ResourceType, OptionsType, PaginationQueryType]) Paginate( | |
| ctx context.Context, | |
| paginationOptions PaginationQueryType, | |
| ) (*bunpaginate.Cursor[ResourceType], error) { | |
| var resourceQuery ResourceQuery[OptionsType] | |
| switch v := any(paginationOptions).(type) { | |
| case OffsetPaginatedQuery[OptionsType]: | |
| resourceQuery = v.Options | |
| case ColumnPaginatedQuery[OptionsType]: | |
| resourceQuery = v.Options | |
| default: | |
| return nil, fmt.Errorf("unsupported pagination query type: %T", paginationOptions) | |
| } | |
| finalQuery, err := r.buildFilteredDataset(resourceQuery) | |
| if err != nil { | |
| return nil, fmt.Errorf("building filtered dataset: %w", err) | |
| } | |
| finalQuery, err = r.paginator.Paginate(finalQuery, paginationOptions) | |
| if err != nil { | |
| return nil, fmt.Errorf("paginating request: %w", err) | |
| } | |
| finalQuery, err = r.expand(finalQuery, resourceQuery) | |
| if err != nil { | |
| return nil, fmt.Errorf("expanding results: %w", err) | |
| } | |
| finalQuery = finalQuery.Order("row_number") | |
| ret := make([]ResourceType, 0) | |
| err = finalQuery.Model(&ret).Scan(ctx) | |
| if err != nil { | |
| return nil, fmt.Errorf("scanning results: %w", err) | |
| } | |
| return r.paginator.BuildCursor(ret, paginationOptions) | |
| } |
Fixes LX-27