Skip to content

fix: transactions paging using effective order #971

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 17, 2025
Merged

fix: transactions paging using effective order #971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
75bdeda
fix: transactions paging using effective order
gfyrag Jun 16, 2025
a2a7f3e
fix: missing type registration in cas of server restart
gfyrag Jun 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions internal/storage/common/paginator_column.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
"github.com/formancehq/go-libs/v3/bun/bunpaginate"
"github.com/formancehq/go-libs/v3/time"
"github.com/uptrace/bun"
"github.com/uptrace/bun/schema"
"math/big"
"reflect"
"strings"
Expand All @@ -14,6 +15,7 @@
type ColumnPaginator[ResourceType, OptionsType any] struct {
DefaultPaginationColumn string
DefaultOrder bunpaginate.Order
Table *schema.Table
}
Comment on lines +18 to 19
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.


//nolint:unused
Expand All @@ -23,6 +25,7 @@
if query.Column != "" {
paginationColumn = query.Column
}

originalOrder := o.DefaultOrder
if query.Order != nil {
originalOrder = *query.Order
Expand All @@ -42,26 +45,39 @@
sb = sb.ColumnExpr("row_number() OVER (ORDER BY " + orderExpression + ")")

if query.PaginationID != nil {
paginationID := convertPaginationIDToSQLType(
o.Table.FieldMap[paginationColumn].DiscoveredSQLType,
query.PaginationID,
)
if query.Reverse {
Comment on lines +48 to 52
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

switch originalOrder {
case bunpaginate.OrderAsc:
sb = sb.Where(fmt.Sprintf("%s < ?", paginationColumn), query.PaginationID)
sb = sb.Where(fmt.Sprintf("%s < ?", paginationColumn), paginationID)

Check warning on line 55 in internal/storage/common/paginator_column.go

View check run for this annotation

Codecov / codecov/patch

internal/storage/common/paginator_column.go#L55 

Added line #L55 was not covered by tests
case bunpaginate.OrderDesc:
sb = sb.Where(fmt.Sprintf("%s > ?", paginationColumn), query.PaginationID)
sb = sb.Where(fmt.Sprintf("%s > ?", paginationColumn), paginationID)
}
} else {
switch originalOrder {
case bunpaginate.OrderAsc:
sb = sb.Where(fmt.Sprintf("%s >= ?", paginationColumn), query.PaginationID)
sb = sb.Where(fmt.Sprintf("%s >= ?", paginationColumn), paginationID)
case bunpaginate.OrderDesc:
sb = sb.Where(fmt.Sprintf("%s <= ?", paginationColumn), query.PaginationID)
sb = sb.Where(fmt.Sprintf("%s <= ?", paginationColumn), paginationID)
}
}
}

return sb, nil
}

func convertPaginationIDToSQLType(sqlType string, id *big.Int) any {
switch sqlType {
case "timestamp without time zone", "timestamp":
return libtime.UnixMicro(id.Int64())
default:
return id
}
}

//nolint:unused
func (o ColumnPaginator[ResourceType, OptionsType]) BuildCursor(ret []ResourceType, query ColumnPaginatedQuery[OptionsType]) (*bunpaginate.Cursor[ResourceType], error) {

Expand Down
10 changes: 10 additions & 0 deletions internal/storage/driver/module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package driver

import (
"context"
ledger "github.com/formancehq/ledger/internal"
"github.com/formancehq/ledger/internal/storage/bucket"
ledgerstore "github.com/formancehq/ledger/internal/storage/ledger"
systemstore "github.com/formancehq/ledger/internal/storage/system"
Expand All @@ -21,6 +22,15 @@ func NewFXModule() fx.Option {
fx.Provide(fx.Annotate(func(tracerProvider trace.TracerProvider) bucket.Factory {
return bucket.NewDefaultFactory(bucket.WithTracer(tracerProvider.Tracer("store")))
})),
fx.Invoke(func(db *bun.DB) {
db.Dialect().Tables().Register(
&ledger.Transaction{},
&ledger.Log{},
&ledger.Account{},
&ledger.Move{},
&ledger.Ledger{},
)
}),
fx.Provide(func(params struct {
fx.In

Expand Down
2 changes: 2 additions & 0 deletions internal/storage/ledger/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ func (store *Store) Transactions() common.PaginatedResource[
return common.NewPaginatedResourceRepository(&transactionsResourceHandler{store: store}, common.ColumnPaginator[ledger.Transaction, any]{
DefaultPaginationColumn: "id",
DefaultOrder: bunpaginate.OrderDesc,
Table: store.db.Dialect().Tables().ByName("transactions"),
})
}

Expand All @@ -79,6 +80,7 @@ func (store *Store) Logs() common.PaginatedResource[
}, common.ColumnPaginator[Log, any]{
DefaultPaginationColumn: "id",
DefaultOrder: bunpaginate.OrderDesc,
Table: store.db.Dialect().Tables().ByName("logs"),
})
}

Expand Down
1 change: 1 addition & 0 deletions internal/storage/system/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ func (d *DefaultStore) Ledgers() common.PaginatedResource[
return common.NewPaginatedResourceRepository(&ledgersResourceHandler{store: d}, common.ColumnPaginator[ledger.Ledger, any]{
DefaultPaginationColumn: "id",
DefaultOrder: bunpaginate.OrderAsc,
Table: d.db.Dialect().Tables().ByName("_system.ledgers"),
})
}

Expand Down
15 changes: 15 additions & 0 deletions test/e2e/api_transactions_list_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,21 @@ var _ = Context("Ledger transactions list API tests", func() {
slices.Reverse(page)
Expect(rsp.V2TransactionsCursorResponse.Cursor.Data).To(Equal(page))
})
When("using next page", func() {
JustBeforeEach(func(specContext SpecContext) {
rsp, err = Wait(specContext, DeferClient(testServer)).Ledger.V2.ListTransactions(
ctx,
operations.V2ListTransactionsRequest{
Ledger: "default",
Cursor: rsp.V2TransactionsCursorResponse.Cursor.Next,
},
)
Expect(err).ToNot(HaveOccurred())
})
It("Should return next elements", func() {
Expect(rsp.V2TransactionsCursorResponse.Cursor.Data).To(HaveLen(int(pageSize)))
})
})
})
It("Should be ok", func() {
Expect(rsp.V2TransactionsCursorResponse.Cursor.PageSize).To(Equal(pageSize))
Expand Down
Loading