fix(v2.2): fix version api field #745
Conversation
WalkthroughThis PR introduces multiple improvements. A new entry is added to the Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant D as Driver
participant DB as Database (bun.DB)
participant S as SystemStore
App->>D: New(db, ledgerStoreFactory, bucketFactory, opts...)
D->>DB: Store DB connection
D->>S: Invoke systemstore.New(d.db)
S-->>D: Return system store instance
D-->>App: Driver initialized
sequenceDiagram
participant Test as Test Suite
participant R as NewRouter
participant Auth as Authenticator
participant SC as SystemController
Test->>R: NewRouter(SC, auth.NewNoAuth(), "develop", debug flag)
R-->>Test: Returns chi.Router with configured routes (_info endpoint)
Possibly related PRs
Poem
✨ Finishing Touches
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:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (22)
internal/storage/ledger/main_test.go (1)
53-56: Good addition of explicit migration step.Adding this migration step for the "_default" bucket ensures the database schema is properly initialized before tests run, which aligns with the PR's objective to ensure atomicity in ledger creation processes. This should help prevent issues with incomplete migrations.
Consider adding a brief comment explaining why this specific migration step is necessary for the test environment.
pkg/client/USAGE.md (1)
21-25: Update to Versioned API Call for GetInfoSwitching to
s.Ledger.V1.GetInfo(ctx)and checking forres.ConfigInfoResponsealigns the usage example with the new API versioning model. This explicit invocation helps clarify which API version is in use. Additionally, note that some static analysis tools flagged hard tab characters in this section; consider replacing these with spaces to adhere to markdown lint standards (MD010).🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
21-21: Hard tabs
Column: 1(MD010, no-hard-tabs)
22-22: Hard tabs
Column: 1(MD010, no-hard-tabs)
23-23: Hard tabs
Column: 1(MD010, no-hard-tabs)
24-24: Hard tabs
Column: 1(MD010, no-hard-tabs)
25-25: Hard tabs
Column: 1(MD010, no-hard-tabs)
docs/api/README.md (1)
22-23: Clarify API Documentation with Versioned HeadingReplacing the old ledger heading with
<h1 id="ledger-api-ledger-v2">ledger.v2</h1>clearly communicates that the documentation now pertains to version 2 of the API. This change helps prevent confusion about which API version is referenced and ensures the docs are aligned with the SDK and operational changes.pkg/client/docs/sdks/v2/README.md (3)
6-7: New Operations Added to Available Operations List
The addition of the two new operations, GetInfo and GetMetrics, in the operations list is clear and consistent with the API’s versioning strategy. Please verify that the linked anchors (i.e.#getinfoand#getmetrics) correctly navigate to the corresponding sections later in this document.
34-38: Clear Documentation for the GetInfo Operation
The new GetInfo section (starting at line 34) succinctly describes the operation and shows an example usage in Go. The explanation “Show server information” is brief but sufficient. Ensure that the response field (ConfigInfoResponse) and parameter descriptions are aligned with the server’s behavior.
85-88: Comprehensive GetMetrics Operation Documentation
The GetMetrics section is well structured, with a brief description ("Read in memory metrics") and an example usage later in the section. Please confirm that the sample code and the response type match the actual API response structure.pkg/client/README.md (6)
72-76: Versioned API Call in SDK Example Usage
In the example usage code block, the method call has been updated from an unversioned call to usings.Ledger.V1.GetInfo(ctx)and the response check now accessesres.ConfigInfoResponse. This aligns with the API’s new versioning scheme. Please ensure that all consumers of the SDK are aware of this change and that the documentation elsewhere is updated accordingly.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
72-72: Hard tabs
Column: 1(MD010, no-hard-tabs)
73-73: Hard tabs
Column: 1(MD010, no-hard-tabs)
74-74: Hard tabs
Column: 1(MD010, no-hard-tabs)
75-75: Hard tabs
Column: 1(MD010, no-hard-tabs)
76-76: Hard tabs
Column: 1(MD010, no-hard-tabs)
112-113: Updated Ledger.V2 Section in Available Operations
The split of available operations into separate sections for Ledger.V1 and Ledger.V2 is a good move for clarity. The two tilde-marked lines (112–113) in the Ledger.V2 section introduce GetInfo and GetMetrics. Verify that these links correctly reference the v2 documentation pages.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
112-112: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
113-113: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
168-182: Retries Example Uses Updated Versioned Call
Within the Retries section’s code block, the example now usess.Ledger.V1.GetInfo(ctx, operations.WithRetries(...))and checksres.ConfigInfoResponse. This update correctly reflects the versioned call and ensures consistency in retry configuration examples.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
168-168: Hard tabs
Column: 1(MD010, no-hard-tabs)
169-169: Hard tabs
Column: 1(MD010, no-hard-tabs)
170-170: Hard tabs
Column: 1(MD010, no-hard-tabs)
171-171: Hard tabs
Column: 1(MD010, no-hard-tabs)
172-172: Hard tabs
Column: 1(MD010, no-hard-tabs)
173-173: Hard tabs
Column: 1(MD010, no-hard-tabs)
174-174: Hard tabs
Column: 1(MD010, no-hard-tabs)
175-175: Hard tabs
Column: 1(MD010, no-hard-tabs)
176-176: Hard tabs
Column: 1(MD010, no-hard-tabs)
177-177: Hard tabs
Column: 1(MD010, no-hard-tabs)
178-178: Hard tabs
Column: 1(MD010, no-hard-tabs)
179-179: Hard tabs
Column: 1(MD010, no-hard-tabs)
180-180: Hard tabs
Column: 1(MD010, no-hard-tabs)
181-181: Hard tabs
Column: 1(MD010, no-hard-tabs)
182-182: Hard tabs
Column: 1(MD010, no-hard-tabs)
319-324: Server Selection Example Updated for Versioned API
In the Server Selection section, the example now callss.Ledger.V1.GetInfo(ctx)and validates the response viares.ConfigInfoResponse. This reinforces the versioning change. Ensure that similar changes are made elsewhere if the default server is switched to a v1 endpoint.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
319-319: Hard tabs
Column: 1(MD010, no-hard-tabs)
320-320: Hard tabs
Column: 1(MD010, no-hard-tabs)
321-321: Hard tabs
Column: 1(MD010, no-hard-tabs)
322-322: Hard tabs
Column: 1(MD010, no-hard-tabs)
323-323: Hard tabs
Column: 1(MD010, no-hard-tabs)
324-324: Hard tabs
Column: 1(MD010, no-hard-tabs)
433-439: Authentication Example Consistency Check
The Authentication section code sample has been updated to uses.Ledger.V1.GetInfo(ctx). The response check againstres.ConfigInfoResponseis consistent with the rest of the examples. Confirm that all error handling and security configurations are in line with the updated API structure.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
433-433: Hard tabs
Column: 1(MD010, no-hard-tabs)
434-434: Hard tabs
Column: 1(MD010, no-hard-tabs)
435-435: Hard tabs
Column: 1(MD010, no-hard-tabs)
436-436: Hard tabs
Column: 1(MD010, no-hard-tabs)
437-437: Hard tabs
Column: 1(MD010, no-hard-tabs)
438-438: Hard tabs
Column: 1(MD010, no-hard-tabs)
439-439: Hard tabs
Column: 1(MD010, no-hard-tabs)
265-274: Error Handling Example Uses Correct Error Types
Within the Error Handling section’s sample code block, the use ofs.Ledger.V1.GetInfo(ctx)(tilde-marked at line 266) and the subsequent error type checks (for bothsdkerrors.ErrorResponseandsdkerrors.SDKError) are correct. This ensures that errors are handled in a robust manner in light of the new versioning.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
265-265: Hard tabs
Column: 1(MD010, no-hard-tabs)
266-266: Hard tabs
Column: 1(MD010, no-hard-tabs)
267-267: Hard tabs
Column: 1(MD010, no-hard-tabs)
269-269: Hard tabs
Column: 1(MD010, no-hard-tabs)
270-270: Hard tabs
Column: 1(MD010, no-hard-tabs)
271-271: Hard tabs
Column: 1(MD010, no-hard-tabs)
272-272: Hard tabs
Column: 1(MD010, no-hard-tabs)
273-273: Hard tabs
Column: 1(MD010, no-hard-tabs)
internal/storage/ledger/resource.go (1)
303-303: Consider removing commented-out debug codeThis commented-out debug statement adds unnecessary noise to the codebase.
-//fmt.Println(finalQuery.Model(&ret).String())If this logging is needed for troubleshooting, consider using a proper logging system with debug levels that can be enabled when needed.
internal/storage/ledger/accounts.go (1)
51-52: Changed metadata merge order - verify expected behaviorThe order of metadata fields concatenation has been reversed from
excluded.metadata || accounts.metadatatoaccounts.metadata || excluded.metadata. In PostgreSQL's JSON concatenation operation (||), fields from the right operand take precedence when there are key conflicts.This change means that new metadata values (from
excluded) will now override existing metadata values (fromaccounts), rather than the existing values taking precedence.Is this the intended behavior for metadata updates? This change ensures that new metadata values override existing ones with the same keys, which is typically the expected behavior for an update operation. However, it differs from the implementation in
UpsertAccounts()on line 107, which uses the same approach as the previous implementation.internal/storage/ledger/accounts_test.go (1)
29-30: Enhanced test timing and debuggingTwo important changes here:
- Setting the initial time to 1 minute in the past
- Wrapping the context with debug support
The time change helps create a more predictable time separation between transactions in the test, potentially addressing timing-related test issues.
Consider adding a comment explaining why the initial time is set to 1 minute in the past to help other developers understand the intent.
internal/storage/ledger/logs_test.go (1)
55-55: Update transaction ID assignmentThe transaction ID is being assigned directly, which doesn't match the pointer-based ID approach used elsewhere in the code.
Consider updating this to use
pointer.For(1)instead of1to maintain consistency with the pointer-based approach.- Transaction: ledger.NewTransaction().WithID(1), + Transaction: ledger.NewTransaction().WithID(pointer.For(1)),internal/controller/ledger/listener_generated_test.go (1)
13-13: Rename the import alias for clarity.Using
internalas an import alias can be confusing, as "internal" usually refers to Go's internal directory concept. Consider a more descriptive alias, such asledgerinternal.- internal "github.com/formancehq/ledger/internal" + ledgerinternal "github.com/formancehq/ledger/internal"internal/storage/driver/driver.go (1)
45-89: Transactional ledger creation workflow
Encapsulating ledger creation and bucket initialization in a single transaction enhances atomicity. Ensure partial failures are rolled back properly, and consider verifying concurrency behaviors when multiple ledgers are created simultaneously.internal/storage/bucket/migrations/24-accounts-metadata-index/up.sql (1)
1-1: Conditional CONCURRENTLY index creation is a good improvementThis change adds a conditional check for the
.Transactionalcontext variable to determine whether to use theCONCURRENTLYoption when creating the index. This is an important improvement as:
- Creating indexes concurrently is generally preferred in production environments as it doesn't block writes
- However, concurrent index creation cannot be run within a transaction
- By making this conditional, you're allowing flexibility based on the migration context
The change aligns with PostgreSQL best practices, where concurrent index creation is preferred for production databases but requires special handling with regard to transactions. This pattern appears to be consistently applied across multiple migration files in this PR.
internal/storage/driver/main_test.go (1)
28-44: Resource cleanup missing in test setupThe test setup correctly initializes resources like the PostgreSQL server and database connection, but there's no explicit cleanup after tests complete. This could lead to resource leaks, especially during continuous integration.
Consider adding cleanup code to properly close the database connection and shutdown the PostgreSQL server after tests complete:
func TestMain(m *testing.M) { WithTestMain(func(t *TestingTForMain) int { srv = pgtesting.CreatePostgresServer(t, docker.NewPool(t, logging.Testing()), pgtesting.WithExtension("pgcrypto")) sqlDB, err := sql.Open("pgx", srv.GetDSN()) require.NoError(t, err) db = bun.NewDB(sqlDB, pgdialect.New(), bun.WithDiscardUnknownColumns()) if os.Getenv("DEBUG") == "true" { queryHook := bundebug.NewQueryHook() queryHook.Debug = true db.AddQueryHook(queryHook) } require.NoError(t, systemstore.Migrate(logging.TestingContext(), db)) - return m.Run() + code := m.Run() + + // Cleanup resources + db.Close() + srv.Close() + + return code }) }internal/api/v1/controllers_config.go (1)
35-35: Function exported to support cross-package usageThe
getInfofunction has been renamed toGetInfo(capitalized), making it exported and accessible from other packages. This supports the PR objective to address the breaking change in the GET /_info endpoint.Since this function is now part of the public API, consider adding a documentation comment:
+// GetInfo returns an HTTP handler function that provides information about the ledger +// configuration, including available ledgers and the server version. func GetInfo(systemController system.Controller, version string) func(w http.ResponseWriter, r *http.Request) {internal/storage/bucket/migrations/18-transactions-fill-inserted-at/up.sql (1)
11-20: Improved trigger handling for Azure Postgres compatibilityThis change addresses two important issues:
- Ensures compatibility with Azure Postgres by avoiding session-level trigger disabling which requires superuser privileges
- Optimizes performance by only triggering the metadata history function when the metadata column is actually updated
The dynamic approach of iterating through ledgers to create specific triggers is well-implemented.
🛑 Comments failed to post (2)
internal/storage/ledger/transactions.go (1)
142-142: 💡 Verification agent
🧩 Analysis chain
Add nil check before dereferencing tx.ID
Similar to the earlier issue, the code is directly dereferencing
tx.IDwithout checking if it's nil. This could lead to a nil pointer dereference panic iftx.IDis nil when this code is executed.Consider adding a nil check or using a helper function to safely handle this case.
🏁 Script executed:
#!/bin/bash # Check if there's any nil check for tx.ID before dereferencing in this file rg -A 3 -B 3 "tx\.ID == nil" internal/storage/ledger/transactions.goLength of output: 315
Action Required: Safeguard the dereference of
tx.IDWhile the file already contains a nil-check for
tx.IDin one branch (used when constructing the query), the call toattribute.Int("id", *tx.ID)does not have its own guard. This direct dereference can lead to a nil pointer dereference panic iftx.IDis nil at that point.
- Add a nil check immediately before the use of
*tx.ID(e.g., wrap the call in anif tx.ID != nil { … }block or use a helper function that safely handles nil values).- Ensure that, in all execution paths,
tx.IDis either safely dereferenced or an alternative behavior is implemented if it’s nil.internal/log.go (1)
107-110:
⚠️ Potential issuePotential nil pointer reference
Whenpreviousis not nil,previous.IDmight still be nil, causing a panic on*previous.ID. Consider checking thatprevious.ID != nilbefore dereferencing.- ret.ID = pointer.For(*previous.ID + 1) + if previous.ID != nil { + ret.ID = pointer.For(*previous.ID + 1) + } else { + ret.ID = pointer.For(1) + }📝 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.if previous != nil { if previous.ID != nil { ret.ID = pointer.For(*previous.ID + 1) } else { ret.ID = pointer.For(1) } } else { ret.ID = pointer.For(1) }
Fixes LX-24