fix: transactions paging using effective order #971
Conversation
WalkthroughThe changes introduce a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Store
participant Paginator
participant DB
Client->>API: Request paginated data (with/without cursor)
API->>Store: Fetch data with pagination options
Store->>Paginator: Initialize with Table metadata
Paginator->>Paginator: Convert PaginationID to SQL type
Paginator->>DB: Query with converted PaginationID
DB-->>Paginator: Return paginated results
Paginator-->>Store: Return results
Store-->>API: Return results
API-->>Client: Respond with data and next cursor
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/storage/ledger/store.go (1)
81-84: Same sanity check needed for the"logs"tableDuplicate of the previous comment, applied to the logs paginator.
🧹 Nitpick comments (2)
internal/storage/common/paginator_column.go (1)
72-79: Extend SQL-type handling inconvertPaginationIDToSQLTypeOnly plain
timestamp/timestamp without time zoneare recognised. Common aliases such as
timestamptz,timestamp with time zone, or dialect-specific names will silently fall through,
sending a*big.Intto the driver and breaking the query.Consider normalising/
strings.HasPrefixchecks or a map-based lookup:-case "timestamp without time zone", "timestamp": +case "timestamp without time zone", "timestamp", "timestamp with time zone", "timestamptz":test/e2e/api_transactions_list_test.go (1)
223-237: Strengthen assertion – validate content, not only lengthThe new test only checks that the second page contains
pageSizeelements.
As we already pre-computed the expected ordering (pageslice in the parent context), we can verify
the actual IDs to protect against subtle off-by-one/ordering regressions.-It("Should return next elements", func() { - Expect(rsp.V2TransactionsCursorResponse.Cursor.Data).To(HaveLen(int(pageSize))) -}) +It("Should return the expected next page", func() { + Expect(rsp.V2TransactionsCursorResponse.Cursor.Data). + To(Equal(page[:pageSize]), "cursor did not return the expected transactions") +})
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/storage/common/paginator_column.go(4 hunks)internal/storage/ledger/store.go(2 hunks)internal/storage/system/store.go(1 hunks)test/e2e/api_transactions_list_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Tests
- GitHub Check: Dirty
🔇 Additional comments (2)
internal/storage/ledger/store.go (1)
68-71: Verify that Dialect().Tables().ByName("transactions") returns a non-nil table
ByNamereturnsnilwhen the table is unknown; this would bubble into the paginator and trip the
nil-checks suggested above. Please add a sanity check (or panic with a descriptive message) during
store initialisation to fail fast rather than at request time.internal/storage/system/store.go (1)
105-106: Ensure_system.ledgerstable metadata is registeredAs with the ledger store, confirm
Dialect().Tables().ByName("_system.ledgers")is notnilto
avoid late panics when paginating ledgers.
| Table *schema.Table | ||
| } |
There was a problem hiding this comment.
Table field can be nil – potential runtime panic
o.Table is dereferenced later (o.Table.FieldMap[...]) without any nil-check.
If any caller forgets (or fails) to set Table, the first paginated request will panic.
Add a defensive check early in Paginate, or at least fail fast with a clear error:
+if o.Table == nil {
+ return nil, fmt.Errorf("column paginator: Table metadata is nil – caller must set it")
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In internal/storage/common/paginator_column.go around lines 18 to 19, the Table
field can be nil but is dereferenced later without a nil-check, risking a
runtime panic. Add a defensive nil-check for o.Table early in the Paginate
method and return a clear error if it is nil to prevent panics and fail fast.
| paginationID := convertPaginationIDToSQLType( | ||
| o.Table.FieldMap[paginationColumn].DiscoveredSQLType, | ||
| query.PaginationID, | ||
| ) | ||
| if query.Reverse { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unsafe access to FieldMap – verify column exists
o.Table.FieldMap[paginationColumn] will return nil for an unknown column and the subsequent
.DiscoveredSQLType dereference will panic.
Guard the lookup and surface a proper error instead of crashing:
-field := o.Table.FieldMap[paginationColumn]
-if field == nil {
- return nil, fmt.Errorf("unknown pagination column %q for table %s", paginationColumn, o.Table.Name)
-}
-paginationID := convertPaginationIDToSQLType(field.DiscoveredSQLType, query.PaginationID)
+field := o.Table.FieldMap[paginationColumn]
+if field == nil {
+ return nil, fmt.Errorf("unknown pagination column %q for table %s", paginationColumn, o.Table.Name)
+}
+paginationID := convertPaginationIDToSQLType(field.DiscoveredSQLType, query.PaginationID)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In internal/storage/common/paginator_column.go around lines 48 to 52, the code
directly accesses o.Table.FieldMap[paginationColumn] without checking if the key
exists, which can cause a panic if the column is unknown. Add a check to verify
that the paginationColumn key exists in FieldMap before accessing
DiscoveredSQLType. If the key is missing, return or propagate a clear error
indicating the unknown column instead of allowing a nil dereference.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #971 +/- ##
==========================================
- Coverage 82.83% 82.83% -0.01%
==========================================
Files 142 142
Lines 8028 8050 +22
==========================================
+ Hits 6650 6668 +18
- Misses 1054 1056 +2
- Partials 324 326 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/storage/driver/module.go (1)
5-5: Avoid ambiguous import aliasUsing the short alias
ledgerfor the rootinternalpackage makes call-sites inside this file (ledger.Transaction,ledger.Log, …) visually indistinguishable from the domain term ledger that is already pervasive in the codebase (e.g.ledgerstore). Consider a more explicit alias such asledgermodelorledgercoreto improve readability.-import ( - ... - ledger "github.com/formancehq/ledger/internal" +import ( + ... + ledgermodel "github.com/formancehq/ledger/internal"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/storage/driver/module.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/storage/driver/module.go (3)
internal/storage/ledger/logs.go (1)
Log(20-26)internal/account.go (1)
Account(14-24)internal/moves.go (1)
Move(12-24)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Tests
- GitHub Check: Dirty
🔇 Additional comments (1)
internal/storage/driver/module.go (1)
25-33: Verify registration happens early enough
db.Dialect().Tables().Register(...)is performed in anfx.Invoke.
Invokehooks run after all constructors (fx.Provide) have been executed.
If any constructor executed earlier instantiates a paginator or another component that looks up table metadata, it will observe an un-registered schema and fail at runtime.Please double-check the call sequence or move the registration into a small constructor that depends only on
*bun.DB, e.g.fx.Provide(func(db *bun.DB) *bun.DB { db.Dialect().Tables().Register(&ledger.Transaction{}, …) return db })This guarantees the registration is complete before any other
Providethat consumes*bun.DB.
gfyrag
force-pushed
the
fix/transactions-paging-using-effective-order
branch
from
d32834f to
a2a7f3e
Compare
No description provided.