fix: configure sequences after import #935
Conversation
WalkthroughThe changes update the ledger state management logic in the controller, ensuring that state transitions and database sequence resets occur atomically and conditionally within a transaction. End-to-end tests are extended to verify correct sequence restoration after ledger import, and test assertions and comments are refined for accuracy regarding advisory locks and concurrency. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ControllerFacade
participant Database
Client->>ControllerFacade: handleState(fn)
ControllerFacade->>Database: Begin Transaction
ControllerFacade->>Database: Update ledger state to 'InUse' if 'Initializing'
alt State was 'Initializing'
ControllerFacade->>Database: Reset transaction_id and log_id sequences
end
ControllerFacade->>ControllerFacade: fn(ctrl)
ControllerFacade->>Database: Commit Transaction
ControllerFacade-->>Client: Return result or error
Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)internal/controller/system/state_tracker.go (1)⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (6)
✨ 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
|
df3eee0 to
b930d05
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
test/e2e/app_lifecycle_test.go (1)
430-437: Predicate tightened to the actual blocking queryChanging the filter to
LIKE '%select setval%'targets the sequence-update statement that is indeed blocked by the table lock – good catch.
Consider making the pattern case-insensitive (ILIKE) to avoid future breakages due to capitalisation.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/controller/system/state_tracker.go(1 hunks)test/e2e/api_ledgers_import_test.go(2 hunks)test/e2e/app_lifecycle_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
test/e2e/api_ledgers_import_test.go (4)
pkg/testserver/api.go (1)
CreateTransaction(37-45)pkg/client/models/operations/v2createtransaction.go (1)
V2CreateTransactionRequest(9-23)pkg/client/models/components/v2posttransaction.go (1)
V2PostTransaction(29-35)pkg/client/models/components/v2posting.go (1)
V2Posting(10-15)
internal/controller/system/state_tracker.go (1)
internal/ledger.go (2)
StateInUse(15-15)StateInitializing(14-14)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (1)
test/e2e/app_lifecycle_test.go (1)
119-121: Expectation update looks correctAdjusting the expected advisory-lock count from
countTransactions+1tocountTransactionsreflects the removal of the extra lock for sequence hashing. Test intention stays intact.
| rowsAffected, err := ret.RowsAffected() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if rowsAffected > 0 { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return a dedicated error when the ledger is already in use
The current branch silently proceeds when rowsAffected == 0, which is ambiguous: the ledger could already be in-use or the row is missing. Propagate an explicit error (e.g. ErrLedgerAlreadyInUse) so callers can distinguish both cases.
🤖 Prompt for AI Agents
In internal/controller/system/state_tracker.go around lines 46 to 51, the code
currently proceeds silently when rowsAffected is 0, which is ambiguous. Modify
the code to return a dedicated error, such as ErrLedgerAlreadyInUse, when
rowsAffected equals 0, so that callers can clearly distinguish between the
ledger being already in use and the row being missing.
There was a problem hiding this comment.
Don't worry, it's the purpose.
If the ledger was already updated in a parallel transaction, we don't need to update sequences.
There was a problem hiding this comment.
@gfyrag I understand now - thanks for the clarification! The behavior is intentional as part of your concurrency design. If a parallel transaction already updated the ledger state from StateInitializing to StateInUse, the sequences would have already been configured by that transaction, so there's no need to update them again or raise an error.
✏️ Learnings added
Learnt from: gfyrag
PR: formancehq/ledger#935
File: internal/controller/system/state_tracker.go:50-55
Timestamp: 2025-05-20T13:07:54.477Z
Learning: In the ledger codebase's `handleState` method, when updating ledger state from `StateInitializing` to `StateInUse`, it's intentional to proceed silently when `rowsAffected == 0`. This indicates another parallel transaction has already updated the ledger state and configured the sequences, so no error needs to be returned and no sequence updating is required.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| _, err := tx.NewRaw( | ||
| fmt.Sprintf(` | ||
| select setval( | ||
| '"%s"."transaction_id_%d"', | ||
| ( | ||
| select max(id) from "%s".transactions where ledger = '%s' | ||
| )::bigint | ||
| ) | ||
| `, c.ledger.Bucket, c.ledger.ID, c.ledger.Bucket, c.ledger.Name), | ||
| ).Exec(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to update transactions sequence value: %w", err) | ||
| } | ||
|
|
||
| _, err = tx.NewRaw( | ||
| fmt.Sprintf(` | ||
| select setval( | ||
| '"%s"."log_id_%d"', | ||
| ( | ||
| select max(id) from "%s".logs where ledger = '%s' | ||
| )::bigint | ||
| ) | ||
| `, c.ledger.Bucket, c.ledger.ID, c.ledger.Bucket, c.ledger.Name), | ||
| ).Exec(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to update logs sequence value: %w", err) | ||
| } |
There was a problem hiding this comment.
Robustness & SQL-injection hardening of sequence reset queries
setval(.., (select max(id)…))fails withNULL valuewhen the table is empty. Wrap the sub-query withCOALESCE(max(id),0).- Building SQL via
fmt.SprintfinterpolatesBucket,Nameetc. directly into the query. Although these values come from our own schema, using placeholders avoids surprises and quoting headaches. - Both
transaction_id_*andlog_id_*queries share the same structure – consider extracting a helper to DRY.
Example safer statement:
- _, err := tx.NewRaw(fmt.Sprintf(`
- SELECT setval('"%s"."transaction_id_%d"',
- COALESCE((SELECT max(id) FROM "%s".transactions WHERE ledger = $1),0)::bigint)
- `, c.ledger.Bucket, c.ledger.ID, c.ledger.Bucket), c.ledger.Name).Exec(ctx)
+ _, err := tx.NewRaw(`
+ SELECT setval(format('"%I"."transaction_id_%s"', ?, ?)::regclass,
+ COALESCE((SELECT max(id) FROM format('"%I".transactions', ?)::regclass
+ WHERE ledger = $1),0)::bigint)
+ `, c.ledger.Bucket, c.ledger.ID, c.ledger.Bucket, c.ledger.Bucket, c.ledger.Name).Exec(ctx)(Exact syntax may vary, but illustrates parameterisation & COALESCE.)
Failing to apply COALESCE will break brand-new ledgers and the SQL-injection risk is small but real.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In internal/controller/system/state_tracker.go around lines 52 to 78, the SQL
queries resetting sequences use fmt.Sprintf with direct string interpolation,
risking SQL injection and failing when tables are empty due to NULL max(id). Fix
by replacing max(id) with COALESCE(max(id), 0) to handle empty tables, use
parameterized queries or placeholders instead of fmt.Sprintf to safely inject
variables like Bucket and Name, and refactor the repeated sequence reset logic
into a helper function to avoid duplication.
There was a problem hiding this comment.
SQL injections can't occurs as the values of the parameters are know and secured.
Also, the coalesce call is not needed, even for brand new ledgers.
There was a problem hiding this comment.
@gfyrag Thank you for the clarification. I understand that in this context the values are controlled internally, mitigating the SQL injection risk. I also appreciate your insight about brand new ledgers not requiring the COALESCE handling - that's valuable context about how ledger initialization works in this system.
✏️ Learnings added
Learnt from: gfyrag
PR: formancehq/ledger#935
File: internal/controller/system/state_tracker.go:0-0
Timestamp: 2025-05-20T13:48:07.434Z
Learning: In the Formance ledger codebase, sequence reset queries with `select setval` don't require COALESCE around max(id) even for brand new ledgers, as the system handles this case properly.
Learnt from: gfyrag
PR: formancehq/ledger#935
File: internal/controller/system/state_tracker.go:0-0
Timestamp: 2025-05-20T13:48:07.434Z
Learning: The parameters used in sequence reset queries in the Formance ledger system (like Bucket, ID, and Name) are internally controlled values that are known to be secure, reducing SQL injection concerns when using string interpolation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| By("Checking sequence restoration", func() { | ||
| tx, err := CreateTransaction(ctx, testServer.GetValue(), operations.V2CreateTransactionRequest{ | ||
| Ledger: createLedgerRequest.Ledger, | ||
| V2PostTransaction: components.V2PostTransaction{ | ||
| Postings: []components.V2Posting{{ | ||
| Source: "world", | ||
| Destination: "dst", | ||
| Asset: "USD", | ||
| Amount: big.NewInt(100), | ||
| }}, | ||
| }, | ||
| }) | ||
| Expect(err).To(BeNil()) | ||
| Expect(tx.ID.Uint64()).To(Equal(transactionsFromOriginalLedger.Data[0].ID.Uint64() + 1)) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential flakiness: assumes transaction list is ordered
The assertion uses transactionsFromOriginalLedger.Data[0].ID, implicitly relying on the list being ordered descending by ID.
If the API ever changes its default ordering, the test will start failing even though the sequence logic is fine.
Safer alternative:
- Expect(tx.ID.Uint64()).To(Equal(transactionsFromOriginalLedger.Data[0].ID.Uint64() + 1))
+ maxID := uint64(0)
+ for _, t := range transactionsFromOriginalLedger.Data {
+ if id := t.ID.Uint64(); id > maxID {
+ maxID = id
+ }
+ }
+ Expect(tx.ID.Uint64()).To(Equal(maxID + 1))🤖 Prompt for AI Agents
In test/e2e/api_ledgers_import_test.go around lines 338 to 352, the test assumes
the transaction list is ordered by ID descending when accessing
transactionsFromOriginalLedger.Data[0].ID, which can cause flakiness if the API
changes ordering. To fix this, explicitly sort or find the transaction with the
highest ID before asserting the next transaction ID, ensuring the test does not
rely on implicit ordering.
There was a problem hiding this comment.
The default behavior of the api will never changes.
There was a problem hiding this comment.
@gfyrag I understand. If the API's default ordering is guaranteed to remain stable, then the current implementation is indeed reliable. Thanks for the clarification.
✏️ Learnings added
Learnt from: gfyrag
PR: formancehq/ledger#935
File: test/e2e/api_ledgers_import_test.go:338-352
Timestamp: 2025-05-20T13:08:10.000Z
Learning: The transactions list API for the ledger service has a stable ordering by ID that will never change, making it safe to rely on transactionsFromOriginalLedger.Data[0].ID in tests.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
b930d05 to
0c56c9c
Compare
| if rowsAffected > 0 { | ||
| _, err := tx.NewRaw( | ||
| fmt.Sprintf(` | ||
| select setval( |
There was a problem hiding this comment.
bucket and name are open for sql injection -- as we're not only manipulating values but also field names, it's not super obvious, but we should make sure ledger name and bucket are sanitized before this (I assume they are already, but I need to ask ;) )
No description provided.