Skip to content

fix(entry)!: expose pagination for transaction entries - #915

Open
SIDDHANTCOOKIE wants to merge 2 commits into
GaloyMoney:mainfrom
SIDDHANTCOOKIE:codex/transaction-entry-pagination
Open

SIDDHANTCOOKIE wants to merge 2 commits into
GaloyMoney:mainfrom
SIDDHANTCOOKIE:codex/transaction-entry-pagination

Conversation

@SIDDHANTCOOKIE

@SIDDHANTCOOKIE SIDDHANTCOOKIE commented Oct 23, 2026 •

Copy link
Copy Markdown

Entries::list_for_transaction_id silently returned only the first 100 entries while presenting a complete Vec<Entry>. A transaction with 102 persisted entries therefore returned only 100 through this API.

Expose pagination in the public method, consistent with other entry-listing methods: accept PaginatedQueryArgs<EntryByCreatedAtCursor> and ListDirection, and return PaginatedQueryRet<Entry, EntryByCreatedAtCursor>. Callers decide whether to fetch subsequent pages. This follows the maintainer's requested API direction.

Fixes #914.

Breaking API change

Callers now provide query arguments and direction, then handle Page::Last or follow Page::HasNext.next. Entries follow the cursor's (created_at, id) ordering. Callers needing transaction sequence order should sort collected entries by entry.values().sequence.

No database migration is needed. Existing transaction-batch callers explicitly expect one page; the sequence-sensitive caller sorts by sequence.

Validation

  • PostgreSQL 18 regression covers 2, 100, 102, and 250 entries, page sizes 17 and 100, ascending and descending traversal, continuation cursors, and missing transactions. It compares the complete ordered entry set to persisted entries to detect skips and duplicates.
  • All 15 existing transaction_batch integration tests pass.
  • Workspace formatting and git diff --check pass.

Commands run with SQLX_OFFLINE=true and PG_CON pointing to a local test database:

cargo test --locked -p cala-ledger --test transaction_entry_pagination --test transaction_batch -- --test-threads=2

Note

Medium Risk
Breaking public API change; callers must paginate and may see different default ordering than the old implicit sequence sort, though behavior is now correct for large transactions.

Overview
Breaking fix: Entries::list_for_transaction_id no longer returns a full Vec<Entry> that implicitly stopped at the first repository page (~100 rows). It now matches other entry list APIs: callers pass PaginatedQueryArgs<EntryByCreatedAtCursor> and ListDirection, receive PaginatedQueryRet, and follow Page::HasNext for additional pages. Results are ordered by (created_at, id); sequence order is left to callers via entry.values().sequence.

Docs on the method describe pagination and ordering. transaction_batch integration tests use helpers::expect_single_page and sort by sequence where assertions need posting order.

A new transaction_entry_pagination test exercises counts above the old ceiling (102, 250), multiple page sizes, ascending/descending traversal, cursor continuity, and empty results for unknown transaction IDs.

Reviewed by Cursor Bugbot for commit a9a013e. Bugbot is set up for automated code reviews on this repo. Configure here.

@SIDDHANTCOOKIE

Copy link
Copy Markdown
Author

@bodymindarts could you review this fix when you have a chance?

I reproduced #914 against PostgreSQL: a transaction with 102 persisted entries returns only 100 from list_for_transaction_id. This follows the continuation cursors and preserves the sequence ordering. The new regression passes for 2, 100, 102, and 250 entries, and all 15 existing transaction-batch tests pass locally.

@bodymindarts

Copy link
Copy Markdown
Member

Instead of collecting internally we should probably expose the cursor in the public API.

@SIDDHANTCOOKIE SIDDHANTCOOKIE changed the title fix(entry): return all entries when a transaction spans multiple pages fix(entry)!: expose pagination for transaction entries Oct 24, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Entries::list_for_transaction_id silently truncates transactions above 100 entries

2 participants