Skip to content

fix: transactions paging using effective order #972

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 16, 2025
Merged

fix: transactions paging using effective order #972

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
078e0ba
fix: transactions paging using effective order
gfyrag Jun 16, 2025
db9f2e7
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
25 changes: 21 additions & 4 deletions internal/storage/ledger/paginator_column.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"github.com/formancehq/go-libs/v2/time"
ledgercontroller "github.com/formancehq/ledger/internal/controller/ledger"
"github.com/uptrace/bun"
"github.com/uptrace/bun/schema"
"math/big"
"reflect"
"strings"
Expand All @@ -15,6 +16,7 @@ import (
type columnPaginator[ResourceType, OptionsType any] struct {
defaultPaginationColumn string
defaultOrder bunpaginate.Order
Table *schema.Table
}
Comment on lines 17 to 20
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Table should be nullable-safe or mandatory – pick one

You added the Table *schema.Table handle but client code can still instantiate columnPaginator{} without setting it (e.g., in future resources).
Because paginate() dereferences o.Table unconditionally, any forgotten assignment will panic.

Either:

  1. Make the field non-pointer (schema.Table) and always populate it, or
  2. Keep it a pointer but add defensive checks in paginate() (see next comment).
🤖 Prompt for AI Agents 
In internal/storage/ledger/paginator_column.go around lines 17 to 20, the Table
field is a pointer but can be nil, causing a panic when dereferenced in
paginate(). To fix this, either change Table to a non-pointer type and ensure it
is always set when creating columnPaginator instances, or keep it as a pointer
but add nil checks in paginate() to handle the case when Table is not assigned,
preventing runtime panics.


//nolint:unused
Expand All @@ -24,6 +26,7 @@ func (o columnPaginator[ResourceType, OptionsType]) paginate(sb *bun.SelectQuery
if query.Column != "" {
paginationColumn = query.Column
}

originalOrder := o.defaultOrder
if query.Order != nil {
originalOrder = *query.Order
Expand All @@ -43,26 +46,40 @@ func (o columnPaginator[ResourceType, OptionsType]) paginate(sb *bun.SelectQuery
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 {
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)
}
} 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)
}
Comment on lines +49 to 66
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Dereferencing FieldMap without checks is unsafe

o.Table.FieldMap[paginationColumn] will panic when:

  • o.Table is nil.
  • paginationColumn is not present in the table (e.g., user passed a computed column such as effective).

A minimal, crash-free fallback:

- paginationID := convertPaginationIDToSQLType(
-     o.Table.FieldMap[paginationColumn].DiscoveredSQLType,
-     query.PaginationID,
- )
+ var sqlType string
+ if o.Table != nil {
+     if field, ok := o.Table.FieldMap[paginationColumn]; ok {
+         sqlType = field.DiscoveredSQLType
+     }
+ }
+ paginationID := convertPaginationIDToSQLType(sqlType, query.PaginationID)

This still honours type conversion when metadata exists but gracefully degrades to the previous behaviour otherwise.

📝 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.

Suggested change
paginationID := convertPaginationIDToSQLType(
o.Table.FieldMap[paginationColumn].DiscoveredSQLType,
query.PaginationID,
)
if query.Reverse {
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)
}
} 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)
}
// Safely derive the SQL type for pagination, falling back if metadata is missing.
var sqlType string
if o.Table != nil {
if field, ok := o.Table.FieldMap[paginationColumn]; ok {
sqlType = field.DiscoveredSQLType
}
}
paginationID := convertPaginationIDToSQLType(sqlType, query.PaginationID)
if query.Reverse {
switch originalOrder {
case bunpaginate.OrderAsc:
sb = sb.Where(fmt.Sprintf("%s < ?", paginationColumn), paginationID)
case bunpaginate.OrderDesc:
sb = sb.Where(fmt.Sprintf("%s > ?", paginationColumn), paginationID)
}
} else {
switch originalOrder {
case bunpaginate.OrderAsc:
sb = sb.Where(fmt.Sprintf("%s >= ?", paginationColumn), paginationID)
case bunpaginate.OrderDesc:
sb = sb.Where(fmt.Sprintf("%s <= ?", paginationColumn), paginationID)
}
}
🤖 Prompt for AI Agents 
In internal/storage/ledger/paginator_column.go around lines 49 to 66, the code
dereferences o.Table.FieldMap[paginationColumn] without checking if o.Table is
nil or if paginationColumn exists in FieldMap, which can cause a panic. Fix this
by adding checks to ensure o.Table is not nil and paginationColumn is present in
FieldMap before accessing it. If these conditions are not met, fall back to the
previous behavior without type conversion to avoid crashes while still honoring
type conversion when metadata exists.

}
}

return sb, nil
}

//nolint:unused
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 ledgercontroller.ColumnPaginatedQuery[OptionsType]) (*bunpaginate.Cursor[ResourceType], error) {

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 @@ -65,6 +65,7 @@ func (store *Store) Transactions() ledgercontroller.PaginatedResource[
return newPaginatedResourceRepository(store, store.ledger, &transactionsResourceHandler{}, columnPaginator[ledger.Transaction, any]{
defaultPaginationColumn: "id",
defaultOrder: bunpaginate.OrderDesc,
Table: store.db.Dialect().Tables().ByName("transactions"),
})
}
Comment on lines 65 to 70
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Guard against nil table metadata to avoid panics at runtime

store.db.Dialect().Tables().ByName("transactions") (and "logs") can legally return nil if the schema introspection misses the table name (e.g. because of a non-default schema, quoting, or a typo).
columnPaginator.paginate() immediately dereferences o.Table.FieldMap[...], which would panic in that situation (or when a caller specifies a Column that is not present in the FieldMap).

Consider resolving the table upfront and failing fast (or falling back) instead of letting the paginator crash later:

- Table: store.db.Dialect().Tables().ByName("transactions"),
+ tbl := store.db.Dialect().Tables().ByName("transactions")
+ if tbl == nil {
+     // You may want to log and bubble up a coherent error instead of panicking
+     panic("transactions table metadata not found – paginator needs schema information")
+ }
+ Table: tbl,

Do the same for the "logs" paginator or, even better, extend columnPaginator.paginate() to tolerate a missing Table / column and fall back to the raw big.Int value.
Fail-fast here prevents production panics and makes the error surface early in the boot path.

Also applies to: 76-81

🤖 Prompt for AI Agents 
In internal/storage/ledger/store.go around lines 65 to 70 and 76 to 81, the
calls to store.db.Dialect().Tables().ByName("transactions") and ByName("logs")
can return nil if the table metadata is missing, which causes panics later when
the paginator dereferences the table fields. To fix this, add checks immediately
after retrieving the table metadata to verify it is not nil. If nil, return an
error or handle the fallback gracefully to fail fast during initialization
instead of panicking at runtime. This ensures safer handling of missing table
metadata and prevents crashes.


Expand All @@ -75,6 +76,7 @@ func (store *Store) Logs() ledgercontroller.PaginatedResource[
return newPaginatedResourceRepositoryMapper[ledger.Log, Log, any, ledgercontroller.ColumnPaginatedQuery[any]](store, store.ledger, &logsResourceHandler{}, columnPaginator[Log, any]{
defaultPaginationColumn: "id",
defaultOrder: bunpaginate.OrderDesc,
Table: store.db.Dialect().Tables().ByName("logs"),
})
}

Expand Down
16 changes: 16 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 @@ -219,6 +219,22 @@ var _ = Context("Ledger transactions list API tests", func() {
slices.Reverse(page)
Expect(rsp.Data).To(Equal(page))
})
When("using next page", func() {
JustBeforeEach(func(specContext SpecContext) {
rsp, err = ListTransactions(
ctx,
testServer.GetValue(),
operations.V2ListTransactionsRequest{
Ledger: "default",
Cursor: rsp.Next,
},
)
Expect(err).ToNot(HaveOccurred())
})
It("Should return next elements", func() {
Expect(rsp.Data).To(HaveLen(int(pageSize)))
})
})
})
It("Should be ok", func() {
Expect(rsp.PageSize).To(Equal(pageSize))
Expand Down
Loading