feat: make ledger creations atomic #703
Conversation
WalkthroughThis pull request revises multiple components across the codebase. In the Justfile, the pre-commit target has been reordered. The removal of the Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant JC as Pre-commit Target
Dev->>JC: Trigger pre-commit checks
JC->>JC: Run "tidy"
JC->>JC: Run "generate"
JC->>JC: Run "lint"
JC->>JC: Run "export-docs-events"
JC->>JC: Run "openapi"
JC->>JC: Run "generate-client"
sequenceDiagram
participant C as Client
participant D as Driver
participant DB as BunDB
participant Tx as Transaction Context
C->>D: Invoke CreateLedger()
D->>DB: RunInTx() to start transaction
DB->>Tx: Provide transaction context
Tx->>D: Instantiate dynamic systemStore within Tx
D->>Tx: Execute ledger creation within Tx
Tx-->>D: Return result (ledger or error)
D-->>C: Relay outcome to client
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #703 +/- ##
==========================================
+ Coverage 81.74% 81.85% +0.11%
==========================================
Files 135 135
Lines 7290 7291 +1
==========================================
+ Hits 5959 5968 +9
+ Misses 1025 1014 -11
- Partials 306 309 +3 ☔ View full report in Codecov by Sentry. |
![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
🔭 Outside diff range comments (1)
internal/storage/driver/buckets_generated_test.go (1)
1-6: 💡 Verification agent🧩 Analysis chain
Verify mock generation command.
The file is auto-generated. Please ensure to run the mock generation command after merging to keep it in sync with the interface changes.
Run this command to verify mock generation:
🏁 Script executed:
#!/bin/bash # Description: Verify mock generation command output matches the current file. # Get the command from the file header MOCK_CMD=$(head -n 6 internal/storage/driver/buckets_generated_test.go | grep mockgen) # Run the command and compare output $MOCK_CMD | diff - internal/storage/driver/buckets_generated_test.goLength of output: 7260
Action Required: Update Verification Script for Mock Generation
The current verification script extracts the mock generation command from the header as-is—including its comment markers—which causes it to fail (e.g., "Is a directory" error) when executed. This prevents accurately verifying that the auto-generated mocks are in sync with interface changes.
Please address the following:
- Strip Comment Markers: Update the script to remove the leading comment (
//) and any extra whitespace before executing the mockgen command.- Re-run Verification Properly: Either modify the shell script or manually run the corrected mockgen command to ensure that the generated file content matches the expected output after interface changes.
- Manual Check Post-Merge: As the file is auto-generated, verify that running the updated command (or a manual check) keeps the mock file in sync.
🧹 Nitpick comments (3)
internal/storage/bucket/migrations/16-create-transaction-id-index-on-moves/up.sql (1)
1-1: Consistent Templating Syntax Recommendation
The command correctly conditionally includes theconcurrentlykeyword. However, note that the schema variable is referenced as"{{ .Schema }}"(with extra spaces) in this file, while other files use"{{.Schema}}". It is recommended to standardize the templating syntax across all migration files for consistency and to avoid any subtle parsing issues.internal/storage/ledger/main_test.go (1)
54-55: Ensure consistent error handling style.While the error handling is correct, consider using
require.NoError(t, err)directly without the intermediate error variable assignment for consistency with the surrounding code.- err = bucket.GetMigrator(bunDB, "_default").Up(logging.TestingContext()) - require.NoError(t, err) + require.NoError(t, bucket.GetMigrator(bunDB, "_default").Up(logging.TestingContext()))internal/storage/bucket/default_bucket.go (1)
95-97: Helpful documentation for migration order.
Thank you for clarifying that the sequence of operations is critical to avoid deadlocks. If there are known scenarios or references about typical deadlock cases, consider adding them as examples.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumtools/generator/go.modis excluded by!**/*.modtools/generator/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (24)
Justfile(1 hunks)cmd/buckets_upgrade.go(1 hunks)cmd/root.go(1 hunks)internal/controller/ledger/store.go(1 hunks)internal/storage/bucket/bucket.go(1 hunks)internal/storage/bucket/default_bucket.go(5 hunks)internal/storage/bucket/migrations.go(1 hunks)internal/storage/bucket/migrations/12-transaction-sequence-index/up.sql(1 hunks)internal/storage/bucket/migrations/13-accounts-sequence-index/up.sql(1 hunks)internal/storage/bucket/migrations/14-transaction-reference-index/up.sql(1 hunks)internal/storage/bucket/migrations/16-create-transaction-id-index-on-moves/up.sql(1 hunks)internal/storage/bucket/migrations/24-accounts-metadata-index/up.sql(1 hunks)internal/storage/bucket/migrations/25-accounts-volumes-index/up.sql(1 hunks)internal/storage/bucket/migrations/26-accounts-recreate-unique-index/up.sql(1 hunks)internal/storage/driver/buckets_generated_test.go(2 hunks)internal/storage/driver/driver.go(9 hunks)internal/storage/driver/driver_test.go(1 hunks)internal/storage/driver/main_test.go(1 hunks)internal/storage/driver/module.go(2 hunks)internal/storage/ledger/main_test.go(1 hunks)internal/storage/system/migrations.go(1 hunks)internal/storage/system/store.go(3 hunks)test/e2e/app_lifecycle_test.go(2 hunks)test/migrations/upgrade_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (113)
internal/storage/bucket/migrations/13-accounts-sequence-index/up.sql (1)
1-1: Conditional Index Creation Based on Transaction Context
The SQL statement correctly leverages a templated conditional to include theconcurrentlykeyword only when the migration is not running in a transactional context. This enhances flexibility when applying the migration.internal/storage/bucket/migrations/24-accounts-metadata-index/up.sql (1)
1-1: GIN Index with Conditional Concurrent Execution
The modified statement conditionally enablesconcurrentlyfor the GIN index on theaccountstable using JSONB_PATH_OPS. It follows the pattern established in similar migration files. Ensure that the.Transactionalvariable is reliably set in your migration environment for the behavior to be as expected.internal/storage/bucket/migrations/25-accounts-volumes-index/up.sql (1)
1-2: Efficient Use of Covering Index for Accounts Volumes
The statement conditionally applies theconcurrentlykeyword and creates an index on theaccounts_volumestable with anincludeclause for extra columns. This approach can help optimize query performance by creating a covering index. Please verify that the included columns (input,output) align with the anticipated query patterns for optimal performance.internal/storage/system/migrations.go (1)
14-14: LGTM! Good abstraction improvement.Changing from concrete type
*bun.DBto interfacebun.IDBimproves code flexibility and testability.Justfile (1)
6-6: LGTM! Better command ordering.Running
tidybeforegenerateensures all dependencies are properly organized before code generation, preventing potential issues.test/e2e/app_lifecycle_test.go (2)
66-66: Verify if reducing test transactions maintains sufficient coverage.The number of test transactions has been reduced from 80 to 60. While this might improve test execution time, please ensure this still provides adequate coverage for concurrent transaction handling scenarios.
126-126: LGTM! Fixed typo in comment.Corrected "gorouting" to "goroutine" in the comment, improving code readability.
internal/storage/bucket/migrations/12-transaction-sequence-index/up.sql (1)
1-1: LGTM! Smart conditional index creation.The conditional
concurrentlykeyword based on.Transactionalflag is a good optimization. It allows:
- Faster index creation without table locks in non-transactional mode
- Consistent index creation within transactions when needed
cmd/buckets_upgrade.go (1)
57-61: LGTM! Driver initialization simplified.The removal of systemstore dependency and simplified bucket factory initialization aligns with the PR's objective of making ledger operations more atomic by reducing unnecessary database connections.
internal/storage/bucket/migrations/14-transaction-reference-index/up.sql (1)
1-1:✅ Verification successful
Excellent modification for atomic operations!
The conditional CONCURRENTLY clause is crucial for atomic operations. PostgreSQL doesn't allow concurrent index creation within transactions, so this change correctly handles both transactional and non-transactional contexts.
Let's verify if this pattern is consistently applied across other index migrations:
🏁 Script executed:
#!/bin/bash # Check if all index creation migrations follow the same pattern rg -l "create.*index.*concurrently" internal/storage/bucket/migrations/Length of output: 582
Verification: Consistent Use of Conditional Index Creation for Atomic Operations
Excellent modification! The use of the conditional
CONCURRENTLYclause ininternal/storage/bucket/migrations/14-transaction-reference-index/up.sqlcorrectly prevents concurrent index creation within transactions—a crucial adjustment for atomic operations in PostgreSQL.A quick check of the migrations shows that the
concurrentlykeyword appears in several index creation scripts:
internal/storage/bucket/migrations/14-transaction-reference-index/up.sqlinternal/storage/bucket/migrations/13-accounts-sequence-index/up.sqlinternal/storage/bucket/migrations/12-transaction-sequence-index/up.sqlinternal/storage/bucket/migrations/16-create-transaction-id-index-on-moves/up.sqlinternal/storage/bucket/migrations/25-accounts-volumes-index/up.sqlinternal/storage/bucket/migrations/24-accounts-metadata-index/up.sqlinternal/storage/bucket/migrations/26-accounts-recreate-unique-index/up.sqlThis confirms that a consistent approach is followed across our index migrations.
internal/storage/bucket/migrations/26-accounts-recreate-unique-index/up.sql (1)
1-9: Well-documented change supporting atomic operations!The comments excellently explain the rationale behind conditional CONCURRENT index creation. This change correctly handles PostgreSQL's limitation of not allowing concurrent index creation within transaction blocks.
cmd/root.go (1)
45-45: LGTM! Consistent driver initialization.The simplified driver initialization aligns with the changes in buckets_upgrade.go and supports the PR's objective of atomic operations.
test/migrations/upgrade_test.go (1)
68-73: LGTM! Simplified driver initialization.The removal of the systemstore dependency and the updated bucket factory initialization aligns with making ledger creations more atomic by reducing coupling.
internal/storage/driver/main_test.go (2)
1-2: LGTM! Proper integration test setup.The
//go:build ittag correctly identifies this as an integration test file.
34-40: LGTM! Robust test environment setup.The test setup uses proper dependency injection with deferred initialization and PostgreSQL test server creation.
internal/controller/ledger/store.go (1)
141-151: LGTM! Well-designed query enhancement.The addition of bucket filtering capability through
ListLedgersQueryPayloadandWithBucketmethod follows good design practices:
- Clear separation of concerns
- Fluent interface pattern for query building
- Type-safe implementation
internal/storage/ledger/main_test.go (1)
57-61: LGTM! Clean driver initialization.The simplified driver initialization with the updated bucket factory aligns with the atomic ledger creation objective.
internal/storage/driver/driver_test.go (27)
1-2: Build tag usage is noted.
Looks good for marking integration tests specifically.
6-7: Imports rearranged.
No apparent issues with importing "fmt" and "github.com/formancehq/go-libs/v2/collectionutils" here.
17-18: Additional standard library imports.
Importing "math/rand" and "sync" is fine for concurrency and random number generation in tests.
22-22: New test functionTestLedgersCreate.
Overall structure is sensible for creating ledgers in parallel.
26-26: Verifydriver.New(db, ...)usage.
Ensuredbis correctly initialized and not nil before instantiating the driver.
28-29: List of buckets and ledger count.
You defined two buckets and a count of 80 ledgers; this appears fine for concurrency testing.
53-53: Wait group usage confirmed.
Explicitly waiting for all goroutines to finish is necessary for concurrency tests.
55-55: Closing the error channel.
Correct approach to prevent further sends after goroutines are done.
57-59: Iterating over errors channel.
This pattern ensures all errors are exposed; it's a good practice in concurrency tests.
61-63: Checking minimal version.
Verifies driver state after ledger creation, looks appropriate.
65-66: Upgrading buckets.
Ensures any new or changed schema is up-to-date.
69-69: New testTestLedgersList.
Clear separation of functionalities for listing ledgers.
74-74: Reusingdriver.New(...).
Consistent with previous usage; ensure no collisions between tests when running in parallel.
76-76: Generating a short random bucket name.
An 8-character slice is adequate for uniqueness in test contexts.
78-80: Creating a new ledger with configuration.
Straightforward ledger instantiation with a specified bucket.
83-84: Creating ledger successfully and requiring no error.
This pattern of creation and assertion is correct.
86-88: Second ledger instantiation.
Slight repetition, but still clear for coverage.
89-89: Error check on second ledger creation.
Confirms no error encountered.
91-92: Creating the second ledger.
Ensures coverage of repeated creation logic for separate ledger names.
94-94: Constructing the query with a bucket filter.
Correct approach for listing only relevant ledgers.
96-97: CallingListLedgers.
Straightforward test call verifying the driver’s listing method.
99-103: Filtering obtained ledgers by bucket and checking count.
Proper validation of returned data usingcollectionutils.Filter.
111-111: Initializing driver inTestLedgerUpdateMetadata.
Consistent approach for test setup; usage is correct.
114-115: Creating ledger and asserting no error.
Maintains clarity in test logic.
128-128: Initializing driver inTestLedgerDeleteMetadata.
Methodologically similar to other tests; good consistency.
133-134: Creating ledger for delete metadata test.
Ensures consistent ledger setup before testing delete functionality.
136-137: Deleting ledger metadata.
Good coverage of key removal logic.internal/storage/driver/module.go (2)
28-29: Factory creation no longer requiresdb.
Transitioning to a tracer-based approach fosters a more modular factory design.
45-46: Refined driver creation with explicit parameters.
Bundlingdb,bucketFactory, andledgerStoreFactoryin one constructor call simplifies the dependency injection flow.Also applies to: 52-52
internal/storage/system/store.go (4)
35-35: Switcheddbfield tobun.IDB.
Using an interface can improve testability and flexibility.
100-104: Applying bucket filter if provided.
Enhances theListLedgersmethod, enabling targeted queries by bucket.
107-107: Refined pagination withledgercontroller.PaginatedQueryOptions[ledgercontroller.ListLedgersQueryPayload].
Ensures typed payload is used for offset pagination.
128-128: New constructorNew(db bun.IDB).
Aligns with the interface-based approach for theDefaultStore, promoting a more decoupled design.internal/storage/bucket/default_bucket.go (21)
22-22: Use ofbun.IDBimproves flexibility.
By storing a more generic interface, the code is more testable and can accommodate transaction-scoped objects seamlessly.
82-88: Constructor aligns well with the interface approach.
Passingbun.IDBintoNewDefaultensures we can use a transaction or a global DB interchangeably, enhancing testability.
106-106: Consistent reference to thetransactionstable.
This change ensures the correct table is referenced when creating or updating sequences.
112-114: Confirm sync mode for transaction metadata history.
Ensure "SYNC" is the intended feature flag forFeatureTransactionMetadataHistoryusage across the codebase.
116-124: Trigger logic for updating transaction metadata history is sound.
The creation of a trigger after updates totransactionsproperly invokesupdate_transaction_metadata_history().
128-128: Reusing "SYNC" flag.
Double-check that the async vs. sync modes align with performance requirements.
131-139: Insert trigger for transaction metadata history.
This after-insert trigger ensures historical metadata is accurately recorded.
143-143: Feature flag for transaction accounts indexing is ON.
Indexing can improve query performance on large ledgers.
146-154: Transaction addresses trigger.
This before-insert trigger ensures any address-based logic is properly applied prior to row insertion.
158-159: Feature flags for address segments and transaction accounts.
Great for systematic enabling of advanced indexing features.
162-164: Trigger for setting transaction address segments.
This ensures the correct segmentation of addresses is applied before insertion.Also applies to: 169-169
204-204: FeatureIndexAddressSegments enabled.
No issue noted; this is consistent with the prior usage of indexing.
207-209: Accounts address array trigger.
This before-insert trigger sets an address array for each account, likely improving lookups.Also applies to: 214-214
219-219: FeatureMovesHistoryPostCommitEffectiveVolumes set to SYNC.
Ensure the chosen mode (SYNC) matches the performance needs and data consistency standards.
222-224: Setting effective volumes for moves.
Before-insert trigger ensures volumes are properly calculated in real-time.Also applies to: 229-229
234-234: Same moves history feature.
No concerns; consistent with previous triggers.
237-239: Updating effective volumes after insert.
Allows for incremental updates to volumes once moves are committed.Also applies to: 244-244
249-249: Comment clarifies logs sequence creation.
Keeping contiguous log IDs helps maintain clarity in log ordering.
250-256: Implementation of logs sequence.
Similar approach astransactionsto ensure log IDs remain contiguous and consistent.
261-261: FeatureHashLogs set to SYNC.
Good for ensuring immediate hashing of logs but watch for potential overhead if logs are frequent.
264-264: Trigger for setting log hash.
Ensures logs are hashed before insertion, enhancing the tamper-proof aspect.internal/storage/driver/driver.go (35)
12-12: Import forgithub.com/uptrace/bun.
Properly introduced for direct usage of Bun’s DB and transaction features.
30-30: DB field inDriverstruct.
Storing*bun.DBat the driver level centralizes database management.
35-36: Migration-related fields for retries and parallelism.
These additions allow flexible configuration for bucket migrations.
41-42: Initialize local store reference and transaction.
UsingRunInTxprovides a clear transaction boundary for ledger creation.
43-43: Instantiate system store within the transaction.
This approach ensures ledger creation and migrations remain transactional.
45-48: Check if ledger already exists and handle constraint errors.
Nicely ensures friendly error handling withErrLedgerAlreadyExists.
52-52: Error check for system store creation.
Properly returns an error if anything unexpected occurs.
53-53: Local error mapping.
Graceful handling of ledger errors is well-managed.
55-55: Validates if bucket is initialized.
Avoids additional migrations if the bucket was previously set up.
57-58: Check bucket’s up-to-date status.
Promptly surfacesErrBucketOutdatedif needed.
64-64: Reject usage of outdated buckets.
Prevents partial use of stale migrations.
67-68: Trigger ledger addition if bucket is already initialized.
Ensures triggers and sequences are set specifically for this new ledger.
71-71: Run migrations if bucket not yet initialized.
Automatically ensures correct schema setup for new buckets.
76-76: Attach newly created store.
Keeping a reference toretensures correct store usage post-transaction.
79-82: Completes transaction scope.
Rolling back on error, or committing if successful, is cleanly handled.
84-84: Return newly created ledger store.
Final step ensures the store is unified with transaction results.
89-89: Retrieve ledger from system store.
One-liner usage ofsystemstore.New(d.db).GetLedger.
94-94: Construct store with the bucket factory.
Combines ledger info and bucket creation in a clear step.
106-106: System store migration on driver initialization.
Guarantees the system backing tables are updated.
116-116: Create systemStore for detecting rollbacks.
Centralizes logic to handle potential schema downgrades.
118-118: Check for system schema downgrades.
Helps keep database states consistent over multiple deployments.
122-122: Fetch distinct buckets.
Used to iterate over each for potential rollback detection.
130-130: Iterate to detect downgrades.
Log-based debugging clarifies which bucket is being processed.
132-132: Run rollback checks on each bucket’s migrator.
Ensures each ledger remains at a valid migration version.
136-136: Propagate errors for rollback detection.
Prevents ignoring partial schema rollbacks.
157-157: Upgrade a single bucket by name.
CallingMigrateensures the bucket gets the latest migrations.
162-162: Retrieve all buckets before upgrading in parallel.
Prepares for concurrent migrations across all buckets.
170-170: Submit concurrent jobs to the worker pool.
Allows parallel bucket migrations.
174-175: Create bucket instance inside each pond worker.
Ensures each upgrade job uses the correct DB handle.
181-181: Run bucket migration in a goroutine.
Allows the worker to wait for completion or retry on error.
216-216: Instantiate systemStore for minimal version checks.
Keeps system metadata queries consistent with the same DB handle.
224-224: Retrieve distinct buckets for version checks.
Ensure each bucket meets at least the minimal schema version.
230-230: Check each bucket’s minimal version.
Short-circuits if any bucket is lagging behind.
243-243: RefinedNewsignature.
Accepting*bun.DBaligns with the rest of the code’s dependency on Bun.
249-249: Storedbin constructed driver.
Central location for injection into factories.internal/storage/bucket/bucket.go (4)
22-23: Passdb bun.IDBexplicitly toCreateandGetMigrator.
This change allows better control over connection lifecycles, enabling transactional contexts and mocking.
30-31: ImplementCreateusing theNewDefaultconstructor.
Clear delegation of object creation; ensures consistent tracer usage.
34-35: ImplementGetMigratorusing the provideddb bun.IDB.
Centralizes migrator retrieval without storing the DB in the factory struct.
38-39: Factory no longer holds the DB reference.
This design removes hidden state in favor of explicit parameter passing.internal/storage/bucket/migrations.go (2)
15-35: LGTM! Good use of interface type and transactional awareness.The changes improve the code by:
- Using the interface type
bun.IDBinstead of concrete type*bun.DB, making the function more flexible.- Adding transactional awareness through type assertion and template variables.
37-37: LGTM! Consistent interface usage.The change to use
bun.IDBaligns with the interface-based approach used inGetMigrator.internal/storage/driver/buckets_generated_test.go (1)
163-174: LGTM! Mock interfaces updated correctly.The mock interfaces have been correctly updated to match the new method signatures with
bun.IDBparameter.Also applies to: 177-188
gfyrag
force-pushed
the
ledger-atomic-creation
branch
3 times, most recently
from
e0cc582 to
930f823
Compare
gfyrag
force-pushed
the
ledger-atomic-creation
branch
from
930f823 to
8ffabef
Compare
gfyrag
force-pushed
the
ledger-atomic-creation
branch
from
8ffabef to
3dd477a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/storage/driver/driver.go (1)
39-85: Significant improvement: Atomic ledger creation with transactions.The
CreateLedgermethod now wraps all operations in a transaction, ensuring that ledger creation, bucket initialization/verification, and adding the ledger to the bucket are all atomic operations. This prevents partial operations from being committed if something fails.Consider extracting some of the error handling into separate helper functions to improve readability, especially for the repeated error wrapping patterns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumtools/generator/go.modis excluded by!**/*.modtools/generator/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (24)
Justfile(1 hunks)cmd/buckets_upgrade.go(1 hunks)cmd/root.go(1 hunks)internal/controller/ledger/store.go(1 hunks)internal/storage/bucket/bucket.go(1 hunks)internal/storage/bucket/default_bucket.go(5 hunks)internal/storage/bucket/migrations.go(1 hunks)internal/storage/bucket/migrations/12-transaction-sequence-index/up.sql(1 hunks)internal/storage/bucket/migrations/13-accounts-sequence-index/up.sql(1 hunks)internal/storage/bucket/migrations/14-transaction-reference-index/up.sql(1 hunks)internal/storage/bucket/migrations/16-create-transaction-id-index-on-moves/up.sql(1 hunks)internal/storage/bucket/migrations/24-accounts-metadata-index/up.sql(1 hunks)internal/storage/bucket/migrations/25-accounts-volumes-index/up.sql(1 hunks)internal/storage/bucket/migrations/26-accounts-recreate-unique-index/up.sql(1 hunks)internal/storage/driver/buckets_generated_test.go(2 hunks)internal/storage/driver/driver.go(9 hunks)internal/storage/driver/driver_test.go(1 hunks)internal/storage/driver/main_test.go(1 hunks)internal/storage/driver/module.go(2 hunks)internal/storage/ledger/main_test.go(1 hunks)internal/storage/system/migrations.go(1 hunks)internal/storage/system/store.go(3 hunks)test/e2e/app_lifecycle_test.go(2 hunks)test/migrations/upgrade_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (20)
- internal/storage/bucket/migrations/13-accounts-sequence-index/up.sql
- internal/storage/bucket/migrations/24-accounts-metadata-index/up.sql
- internal/storage/bucket/migrations/26-accounts-recreate-unique-index/up.sql
- internal/storage/bucket/migrations/12-transaction-sequence-index/up.sql
- Justfile
- internal/storage/system/migrations.go
- internal/storage/bucket/migrations/16-create-transaction-id-index-on-moves/up.sql
- internal/storage/bucket/migrations/25-accounts-volumes-index/up.sql
- test/migrations/upgrade_test.go
- cmd/buckets_upgrade.go
- internal/storage/bucket/migrations/14-transaction-reference-index/up.sql
- test/e2e/app_lifecycle_test.go
- cmd/root.go
- internal/storage/driver/module.go
- internal/controller/ledger/store.go
- internal/storage/driver/main_test.go
- internal/storage/ledger/main_test.go
- internal/storage/system/store.go
- internal/storage/bucket/migrations.go
- internal/storage/driver/buckets_generated_test.go
🧰 Additional context used
🧠 Learnings (1)
internal/storage/driver/driver_test.go (2)
Learnt from: gfyrag
PR: formancehq/ledger#703
File: internal/storage/driver/driver_test.go:31-34
Timestamp: 2025-02-24T11:31:17.135Z
Learning: In Go, using range with an integer constant (e.g., `for i := range countLedgers`) is valid syntax and compiles correctly.
Learnt from: gfyrag
PR: formancehq/ledger#703
File: internal/storage/driver/driver_test.go:35-51
Timestamp: 2025-02-24T11:31:03.968Z
Learning: Starting from Go 1.22, loop variables in goroutines are handled correctly by default - each iteration gets a fresh variable, eliminating the need for explicit local copies to avoid closure capture issues.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (11)
internal/storage/driver/driver_test.go (3)
22-67: Test refactoring improves concurrency testing.The new
TestLedgersCreatefunction effectively tests concurrent ledger creation by spawning 80 goroutines, each creating a ledger with a randomly selected bucket. The approach using a wait group and error channel to manage concurrency is well-implemented.
34-52: Goroutine implementation is correct with Go 1.22+.Using the loop variable directly in the goroutine is safe in Go 1.22+ as each iteration gets a fresh variable.
69-104: Improved testing of bucket-based filtering.The updated
TestLedgersListfunction effectively tests the listing functionality with bucket filtering, creating a more direct and clear test compared to using mocks.internal/storage/driver/driver.go (3)
28-37: Structural improvement: Direct DB access instead of system store.Changing from storing a
systemStoreto having direct access to the*bun.DBgives more flexibility in how database operations are performed.
87-97: Consistent pattern for system store usage.Creating a system store on demand using
systemstore.New(d.db)establishes a consistent pattern throughout the codebase.
243-258: Updated constructor to support new DB management pattern.The
Newfunction now takesdb *bun.DBas a parameter, which aligns with the architectural changes.internal/storage/bucket/default_bucket.go (3)
20-24: Enhanced flexibility with bun.IDB interface.Changing from concrete
*bun.DBto the interfacebun.IDBimproves testability and flexibility, allowing the code to work with both database connections and transactions.
82-88: Constructor updated for interface usage.The
NewDefaultfunction now acceptsbun.IDBinstead of*bun.DB, which is consistent with the struct field change.
95-98: Helpful comments for preventing deadlocks.The added comments about migration order provide important guidance to prevent deadlocks when modifying triggers and operations.
internal/storage/bucket/bucket.go (2)
21-24: Interface updated to support transaction-based operations.The
Factoryinterface methods now accept adb bun.IDBparameter, enabling the caller to provide either a database connection or a transaction.
26-44: Improved design: Database passed explicitly rather than stored.Removing the
dbfield from theDefaultFactoryand passing it explicitly through method calls is a superior design. This approach:
- Makes dependencies clearer
- Supports transaction-based operations
- Improves testability
- Aligns with the dependency injection pattern
The
NewDefaultFactoryfunction has been correctly updated to remove the now-unnecessarydbparameter.
Fixes LX-3