chore: upgrade speakeasy version #969
Conversation
|
Warning Rate limit exceeded@gfyrag has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 5 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (20)
WalkthroughThis update introduces a significant internal refactor of the Go SDK's configuration and hooks handling, moving configuration logic to a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SDK (Formance)
participant Config (SDKConfiguration)
participant Hooks
participant Ledger/V1/V2 Client
participant HTTP
User->>SDK (Formance): New()
SDK (Formance)->>Config: Initialize SDKConfiguration
SDK (Formance)->>Hooks: Initialize hooks
SDK (Formance)->>Ledger/V1/V2 Client: Instantiate with Config and Hooks
User->>Ledger/V1/V2 Client: API Call
Ledger/V1/V2 Client->>Hooks: BeforeRequest(HookContext)
Ledger/V1/V2 Client->>HTTP: Send Request
HTTP-->>Ledger/V1/V2 Client: Response/Error
alt Success
Ledger/V1/V2 Client->>Hooks: AfterSuccess(HookContext)
else Error
Ledger/V1/V2 Client->>Hooks: AfterError(HookContext)
end
Ledger/V1/V2 Client-->>User: Return response/error
Possibly related PRs
Suggested reviewers
Poem
✨ 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
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #969 +/- ##
==========================================
+ Coverage 82.82% 82.83% +0.01%
==========================================
Files 142 142
Lines 8028 8028
==========================================
+ Hits 6649 6650 +1
Misses 1054 1054
+ Partials 325 324 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (2)
pkg/client/docs/models/operations/v2reverttransactionresponse.md (1)
1-9: 🛠️ Refactor suggestionDoc heading & filename still reference the removed type
The model
V2RevertTransactionResponseno longer exists, yet the markdown title and the path keep the old name. This is likely to confuse users who search the docs.-# V2RevertTransactionResponse +# V2CreateTransactionResponse <!-- or rename file accordingly -->Consider renaming both the heading and the file itself (
v2createtransactionresponse.md) to reflect the new type.pkg/client/models/operations/v2reverttransaction.go (1)
70-88: 🛠️ Refactor suggestionStruct name / field mismatch is confusing – align naming to avoid cognitive overhead
V2RevertTransactionResponsenow embedsV2CreateTransactionResponseboth as the field name and the getter.
Readers will wonder why a “revert” operation returns a “create” model.Options:
- Rename the struct itself to
V2RevertTransactionCreateResponse(or similar) if that truly reflects the API contract.- Keep the struct name but rename the field to something neutral like
TransactionResponse.-type V2RevertTransactionResponse struct { +type V2RevertTransactionResponse struct { HTTPMeta components.HTTPMetadata `json:"-"` - // OK - V2CreateTransactionResponse *components.V2CreateTransactionResponse + // OK – reverted transaction details + TransactionResponse *components.V2CreateTransactionResponse }The accompanying getter should be updated accordingly.
This small rename prevents future misunderstandings and accidental misuse.
♻️ Duplicate comments (2)
pkg/client/v1.go (2)
20-31: Repeat ofSDKConfigurationby-value copySame concern as in
ledger.go: keep a pointer rather than copying.
59-67: Duplicate OAuth2ScopesAll generated operations repeat
"ledger:read"twice. Please deduplicate globally in the code-generator template.
🧹 Nitpick comments (14)
pkg/client/.gitignore (1)
1-3: Consider consolidating ignore patterns in the repo-root.gitignoreThe patterns are fine, but you already ignore
.DS_Storeand.speakeasy/*at the repository root. Duplicating rules in a nested.gitignorecan make maintenance harder when the same pattern must be updated in multiple places.
If there’s no need for package-local exceptions, move these three lines to the root file and drop this per-package entry.pkg/client/internal/utils/json.go (1)
189-194:Tag.Lookupis the right call – maybe DRY it upSwitching from
Tag.GettoTag.Lookupcorrectly distinguishes “tag absent” from “tag present with empty value”, fixing subtle default-value edge-cases.You now have identical
defaultTag, ok := tag.Lookup("default")blocks in two places; extracting a tiny helper (getDefault(tag) (string, bool)) would avoid duplication.Also applies to: 260-264
pkg/client/internal/hooks/hooks.go (1)
28-30: Consider narrowing theSDKfield’s type to a dedicated interface instead ofanyUsing the empty type alias
anyforfeits compile-time guarantees and invites misuse from outside packages.
If the SDK root already exposes a stable surface (e.g. a small set of methods such asDoRequest,Config(), etc.), export a minimal interface and store that here:- SDK any + SDK sdkroot.Interface // minimal behaviour contractThis keeps flexibility while still protecting consumers from accidental type assertions.
If total flexibility is really required, add a short doc-comment explaining why an unconstrained type is intentional.pkg/client/internal/utils/requestbody.go (2)
172-183: Slice/array handling duplicates field names with[]suffix – verify server compatibilityAppending
"[]"tofieldNamewhen iterating over slices assumes the server expects the “Rails-style” array notation. Many back-ends simply accept repeated field names without the suffix.
Consider making this behaviour configurable or falling back to the plain name for better interoperability:- if err := encodeMultipartFormDataFile(writer, tag.Name+"[]", arrayVal.Type(), arrayVal); err != nil { + field := tag.Name + if tag.ArraySuffix { // hypothetical flag + field += "[]" + } + if err := encodeMultipartFormDataFile(writer, field, arrayVal.Type(), arrayVal); err != nil { writer.Close() return "", err }
261-270: Content-type detection ok, but consider falling back tohttp.DetectContentTypefor streams without extensions
mime.TypeByExtensionreturns an empty string for unknown or missing extensions, at which point we default toapplication/octet-stream.
Usinghttp.DetectContentTypeon the first 512 bytes can yield a more accurate MIME guess and improve downstream behaviour:- contentType := mime.TypeByExtension(filepath.Ext(fileName)) - if contentType == "" { - contentType = "application/octet-stream" - } + contentType := mime.TypeByExtension(filepath.Ext(fileName)) + if contentType == "" { + // Fallback: sniff the first 512 bytes + if seeker, ok := reader.(io.ReadSeeker); ok { + head := make([]byte, 512) + n, _ := seeker.Read(head) + _, _ = seeker.Seek(0, io.SeekStart) + contentType = http.DetectContentType(head[:n]) + } + if contentType == "" { + contentType = "application/octet-stream" + } + }This remains backwards-compatible while improving accuracy.
pkg/client/formance.go (2)
50-56: Consider embeddingconfig.SDKConfigurationor exposing an accessor
formance.sdkConfigurationis now an unexported, value-typed field.
Down-stream code that previously accessed or mutated configuration directly will no longer compile, and because it is stored by value, any consumer retaining a reference toSDKConfigurationwill get a copy, not a live view.Options:
-type Formance struct { - SDKVersion string - Ledger *Ledger - sdkConfiguration config.SDKConfiguration - hooks *hooks.Hooks -} +type Formance struct { + SDKVersion string + Ledger *Ledger + config.SDKConfiguration // anonymous embed keeps previous API working + hooks *hooks.Hooks +}or alternatively expose an accessor (
Config() *config.SDKConfiguration).
Helps backward compatibility and avoids accidental stale copies.
127-134: Avoid hard-coded, duplicated version literalsThe SDK version appears twice (
"0.8.0"), once forSDKVersionand again inside the User-Agent string. Duplicating literals tends to drift.- SDKVersion: "0.8.0", - sdkConfiguration: config.SDKConfiguration{ - UserAgent: "speakeasy-sdk/go 0.8.0 2.629.1 v2 github.com/formancehq/ledger/pkg/client", + SDKVersion: sdkVersion, + sdkConfiguration: config.SDKConfiguration{ + UserAgent: fmt.Sprintf("speakeasy-sdk/go %s 2.629.1 v2 github.com/formancehq/ledger/pkg/client", sdkVersion),with
const sdkVersion = "0.8.0"Reduces maintenance overhead.
pkg/client/ledger.go (2)
65-72: Duplicate scope strings
OAuth2Scopesis initialised with[]string{"ledger:read", "ledger:read"}– the second entry is redundant.- OAuth2Scopes: []string{"ledger:read", "ledger:read"}, + OAuth2Scopes: []string{"ledger:read"},Small, but worth fixing before it leaks into analytics or auth-caching.
130-156: Nil-hook protection?All hook invocations now use
s.hooks. If a caller builds aLedgermanually and passesnil, these paths will panic.
Either:
- Document that
hooksmust be non-nil, or- Short-circuit when
s.hooks == nil.Option 2 keeps the public surface more robust.
if s.hooks != nil { req, err = s.hooks.BeforeRequest(... ) }pkg/client/v1.go (2)
125-151: Shadowingreqinside retry lambda
req, err = s.hooks.BeforeRequest(...)re-assigns the captured variable.
While legal, it mutates the outerreqfor subsequent retries, which is fine but unnecessary.
Consider using a localr := reqinside the lambda to avoid accidental coupling.
2553-2560: Circular references & GC-pressurePassing
rootSDK *Formanceinto every sub-client creates cycles (Formance → Ledger/V1 → rootSDK).
Go’s GC breaks cycles, but avoidable retention of large graphs (e.g., HTTP client pools) may increase memory.If
rootSDKis only needed for hooks, consider passing the minimal interface instead.pkg/client/internal/utils/oauth2_sessions.go (1)
54-60: Document the expiration buffer logic.The method adds a 60-second buffer when checking expiration, but this magic number isn't documented. This buffer helps prevent using tokens that are about to expire during request processing.
Add a comment explaining the buffer:
// Returns true if the session is expired or within one minute of expiring. func (s oAuth2Session) isExpired() bool { if s.expiresAt == nil { return false } + // Add 60-second buffer to prevent using tokens that might expire during request processing return time.Now().Unix()+60 >= *s.expiresAt }pkg/client/internal/utils/oauth2_credentials.go (2)
120-131: Replace MD5 with a more modern hash function.While this hash is used for cache keys rather than security, MD5 is deprecated and shouldn't be used in new code. Consider using SHA-256 or xxhash for better performance and to avoid security scanner warnings.
+import ( + "crypto/sha256" + // ... other imports +) func (c oAuth2ClientCredentials) Hash() string { if c.hash != nil { return *c.hash } - md5Checksum := md5.Sum([]byte(c.clientID + ":" + c.clientSecret)) - md5ChecksumStr := hex.EncodeToString(md5Checksum[:]) + sha256Checksum := sha256.Sum256([]byte(c.clientID + ":" + c.clientSecret)) + sha256ChecksumStr := hex.EncodeToString(sha256Checksum[:]) - c.hash = &md5ChecksumStr + c.hash = &sha256ChecksumStr return *c.hash }
67-70: Update misleading "Unused" comments.The comments state these methods are "Unused", but they are required to implement the
OAuth2AuthorizationGrantorinterface. These comments are misleading.-// Unused, however required to implement OAuth2AuthorizationGrantor. +// Returns an empty string as pre-authorized tokens don't need credential hashing. func (c OAuth2AccessTokenCredentials) Hash() string { return "" } -// Unused, however required to implement OAuth2AuthorizationGrantor. +// Returns an empty string as pre-authorized tokens don't require a token endpoint. func (c OAuth2AccessTokenCredentials) TokenEndpoint() string { return "" }Also applies to: 82-85
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
flake.lockis excluded by!**/*.lock,!**/*.lockpkg/client/.devcontainer/devcontainer.jsonis excluded by!**/*.jsonpkg/client/.speakeasy/gen.lockis excluded by!**/*.lock,!**/*.lockpkg/client/.speakeasy/gen.yamlis excluded by!**/*.yamlpkg/client/.speakeasy/workflow.lockis excluded by!**/*.lock,!**/*.lockpkg/client/.speakeasy/workflow.yamlis excluded by!**/*.yamlpkg/client/go.modis excluded by!**/*.mod
📒 Files selected for processing (20)
flake.nix(1 hunks)pkg/client/.gitignore(1 hunks)pkg/client/docs/models/components/v2reverttransactionresponse.md(0 hunks)pkg/client/docs/models/operations/v2reverttransactionresponse.md(1 hunks)pkg/client/docs/sdks/v1/README.md(3 hunks)pkg/client/docs/sdks/v2/README.md(9 hunks)pkg/client/formance.go(5 hunks)pkg/client/internal/config/sdkconfiguration.go(1 hunks)pkg/client/internal/hooks/hooks.go(3 hunks)pkg/client/internal/utils/json.go(2 hunks)pkg/client/internal/utils/oauth2_credentials.go(1 hunks)pkg/client/internal/utils/oauth2_sessions.go(1 hunks)pkg/client/internal/utils/requestbody.go(3 hunks)pkg/client/ledger.go(10 hunks)pkg/client/models/components/v2bulkelementresult.go(1 hunks)pkg/client/models/components/v2reverttransactionresponse.go(0 hunks)pkg/client/models/operations/v2reverttransaction.go(2 hunks)pkg/client/v1.go(82 hunks)pkg/client/v2.go(103 hunks)test/e2e/api_transactions_revert_test.go(3 hunks)
💤 Files with no reviewable changes (2)
- pkg/client/docs/models/components/v2reverttransactionresponse.md
- pkg/client/models/components/v2reverttransactionresponse.go
🧰 Additional context used
🧬 Code Graph Analysis (5)
pkg/client/models/operations/v2reverttransaction.go (1)
pkg/client/models/operations/v2createtransaction.go (1)
V2CreateTransactionResponse(62-66)
pkg/client/internal/hooks/hooks.go (1)
pkg/client/internal/config/sdkconfiguration.go (1)
SDKConfiguration(16-25)
pkg/client/internal/utils/oauth2_sessions.go (3)
pkg/client/internal/utils/oauth2_credentials.go (2)
OAuth2AuthorizationGrantor(27-40)HTTPClient(20-22)pkg/client/internal/config/sdkconfiguration.go (1)
HTTPClient(12-14)pkg/client/internal/hooks/hooks.go (2)
HTTPClient(23-25)New(82-101)
pkg/client/v1.go (4)
pkg/client/formance.go (1)
Formance(50-56)pkg/client/internal/config/sdkconfiguration.go (1)
SDKConfiguration(16-25)pkg/client/internal/hooks/hooks.go (5)
Hooks(70-75)BeforeRequestContext(37-39)HookContext(27-35)AfterErrorContext(45-47)AfterSuccessContext(41-43)pkg/client/internal/utils/utils.go (1)
MatchStatusCodes(71-92)
pkg/client/internal/utils/oauth2_credentials.go (3)
pkg/client/internal/config/sdkconfiguration.go (1)
HTTPClient(12-14)pkg/client/internal/hooks/hooks.go (1)
HTTPClient(23-25)pkg/client/formance.go (1)
HTTPClient(25-27)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Tests
- GitHub Check: Dirty
🔇 Additional comments (13)
flake.nix (1)
36-36:speakeasyVersionbump LGTM – no issues spotted
Version string is updated consistently with the download URL below; no further action required.pkg/client/models/components/v2bulkelementresult.go (1)
215-223: Parameter rename eliminates shadowing – good cleanup
Renaming the parameter fromerrortoerrorTavoids shadowing the predeclarederrortype and makes the code clearer. ✅test/e2e/api_transactions_revert_test.go (1)
151-161: Test updates align with new response structAll references now use
V2CreateTransactionResponse, matching the SDK changes. Looks good and should compile cleanly.Also applies to: 202-202, 236-236
pkg/client/docs/sdks/v1/README.md (1)
463-466: Documentation only – no functional impact detected.pkg/client/formance.go (1)
151-156:✅ Verification successful
Verify
SDKInitreturn order matches assignmentThe assignment expects
SDKInitto return(string, HTTPClient):serverURL, sdk.sdkConfiguration.Client = sdk.hooks.SDKInit(currentServerURL, sdk.sdkConfiguration.Client)A silent swap (e.g.
(HTTPClient, string)) will compile but break runtime behaviour (client holds URL, serverURL holds nil-interface).
🏁 Script executed:
#!/bin/bash # Show the signature of SDKInit to ensure it returns (string, client) rg -n "func .*SDKInit" -A 1 -B 0Length of output: 637
No changes needed:
SDKInitreturn signature matches assignment
Verified thatSDKInitis declared asfunc (h *Hooks) SDKInit(baseURL string, client HTTPClient) (string, HTTPClient)which aligns with the assignment in
pkg/client/formance.go.pkg/client/v2.go (4)
20-32: LGTM!The refactoring to separate hooks from SDK configuration and add a reference to the root SDK improves the architecture by following the separation of concerns principle.
126-134: Good defensive programming!The additional checks for
http.NoBodyandreq.GetBody != nilprevent potential runtime panics when dealing with requests that don't have a body or don't support body retrieval.
59-67: Good enhancement to HookContext!The addition of
SDKandSDKConfigurationfields to the HookContext provides hooks with more context for advanced use cases like logging, debugging, or conditional behavior based on SDK configuration. The implementation is consistent across all methods.
4160-4165:✅ Verification successful
Verify the response type change is intentional.
The response type has been changed from
V2RevertTransactionResponsetoV2CreateTransactionResponse. This makes sense if the revert operation creates a compensating transaction, but please ensure this aligns with the API specification.Run the following script to verify the removal of V2RevertTransactionResponse and check for any remaining references:
🏁 Script executed:
#!/bin/bash # Description: Verify that V2RevertTransactionResponse has been completely removed and check API operation files # Test 1: Search for any remaining references to V2RevertTransactionResponse echo "Searching for V2RevertTransactionResponse references..." rg "V2RevertTransactionResponse" -A 2 # Test 2: Check the operations file for RevertTransaction to see the expected response type echo -e "\nChecking RevertTransaction operation definition..." fd -e go "v2reverttransaction.go" --exec cat {} # Test 3: Look for any test files that might need updating echo -e "\nChecking test files for revert transaction tests..." fd -e go -e "_test.go" --exec rg -l "RevertTransaction|V2RevertTransaction" {}Length of output: 7073
Response type change confirmed.
The
RevertTransactionoperation now returns anoperations.V2RevertTransactionResponsethat wraps acomponents.V2CreateTransactionResponse. This update is applied consistently across the codebase:
- pkg/client/models/operations/v2reverttransaction.go:
V2RevertTransactionResponsestruct embeds*components.V2CreateTransactionResponse- pkg/client/v2.go: Unmarshalling into
components.V2CreateTransactionResponseand assigning tores.V2CreateTransactionResponse- Operation definition (v2reverttransaction.go) and README docs updated to reference the new wrapper type
- Tests (e.g., test/e2e/api_transactions_revert_test.go) still use
operations.V2RevertTransactionResponseand will operate against the embedded create-response payloadAll references to a standalone revert-specific response have been replaced or wrapped. This aligns with the API behavior of creating a compensating transaction on revert.
pkg/client/ledger.go (1)
120-128: Retry body-copy guard LGTMThe additional
req.Body != http.NoBody && req.GetBody != nilcheck prevents panics onGetBodyaccess – nice defensive upgrade.pkg/client/v1.go (1)
115-123: Retry body-copy guard acknowledgedSame positive remark as in
ledger.go.pkg/client/docs/sdks/v2/README.md (2)
1294-1294: LGTM! Response type updated correctly.The change from
V2RevertTransactionResponsetoV2CreateTransactionResponsecorrectly reflects the removal of theV2RevertTransactionResponsemodel as mentioned in the PR summary.
437-441:❌ Incorrect review comment
Confirm the bulk operation example change aligns with API updates.
The bulk operation example changed from
RevertTransactiontoDeleteMetadata. Please verify this is the intended replacement example following the removal of theV2RevertTransactionResponsemodel.
🏁 Script executed:
#!/bin/bash # Description: Verify if there are other bulk operation types available that might be more appropriate # Search for V2BulkElement types in the codebase ast-grep --pattern 'type V2BulkElement$$$' # Search for CreateV2BulkElement functions rg 'CreateV2BulkElement' --type goLength of output: 4300
🏁 Script executed:
#!/bin/bash # Search for any occurrence of V2RevertTransactionResponse rg -n 'RevertTransactionResponse' # Search for a type declaration matching V2RevertTransactionResponse rg -n 'type V2RevertTransactionResponse' # List files in the components models directory related to revert transaction ls pkg/client/models/components | grep -i revertLength of output: 2320
RevertTransaction is still supported; DeleteMetadata is simply a different example
The
V2RevertTransactionResponsetype and theRevertTransactionbulk operation are still present in the v2 client (seepkg/client/models/operations/v2reverttransaction.goandpkg/client/v2.go). The README change didn’t remove revert support—it merely swaps in theDeleteMetadataexample. No API removal has occurred, and the new snippet correctly illustrates theDELETE_METADATAbulk operation.Likely an incorrect or invalid review comment.
| "x86_64-linux" = "632559a6bdc765ef42b81b8404fd0a3e5023919a0bb70ff7e40a8cc259545afd"; | ||
| "aarch64-linux" = "c74c502df3a05a2d69e6b282886df23354a319d0510d2c1a21fcc124b7ad00efGOROOT"; | ||
| "x86_64-darwin" = "8814be1fdd4eaf6dcc7fb251ede5693e1d3d4c8e03986f8d37bfd59e049698b9"; | ||
| "aarch64-darwin" = "12c20fa485de4725c9730cb2e8936cab4b0961d0a956e9f4c45534371f2a6148"; |
There was a problem hiding this comment.
Malformed SHA256 for aarch64-linux will cause Nix build failures
c74c502df3a05a2d69e6b282886df23354a319d0510d2c1a21fcc124b7ad00efGOROOT is 71 chars and contains non-hex text (GOROOT). Nix will reject this derivation.
- "aarch64-linux" = "c74c502df3a05a2d69e6b282886df23354a319d0510d2c1a21fcc124b7ad00efGOROOT";
+ # TODO: replace with the correct 64-char hex digest for v1.563.0
+ "aarch64-linux" = "<correct_sha256_here>";Re-download the artifact and compute the hash:
nix hash file speakeasy_linux_arm64.zip🤖 Prompt for AI Agents
In flake.nix around lines 44 to 47, the SHA256 hash for "aarch64-linux" is
malformed because it contains non-hex characters and is too long, causing Nix
build failures. Re-download the corresponding artifact for the aarch64-linux
platform and compute the correct SHA256 hash using the command `nix hash file
<filename>`. Replace the incorrect hash string with the newly computed valid
SHA256 hash.
| func (c *SDKConfiguration) GetServerDetails() (string, map[string]string) { | ||
| if c.ServerURL != "" { | ||
| return c.ServerURL, nil | ||
| } | ||
|
|
||
| return c.ServerList[c.ServerIndex], nil | ||
| } |
There was a problem hiding this comment.
Potential panic when ServerList is empty or ServerIndex is out of bounds
GetServerDetails directly indexes ServerList[c.ServerIndex] without validating that
a) ServerList is non-nil and non-empty, and
b) ServerIndex is within range.
A malformed configuration would bring the entire SDK down at runtime.
-func (c *SDKConfiguration) GetServerDetails() (string, map[string]string) {
- if c.ServerURL != "" {
- return c.ServerURL, nil
- }
-
- return c.ServerList[c.ServerIndex], nil
+func (c *SDKConfiguration) GetServerDetails() (string, map[string]string) {
+ if c == nil {
+ return "", nil
+ }
+ if c.ServerURL != "" {
+ return c.ServerURL, nil
+ }
+ if c.ServerIndex < 0 || c.ServerIndex >= len(c.ServerList) {
+ return "", nil // or return first entry / explicit error
+ }
+ return c.ServerList[c.ServerIndex], nil
}Failing fast (returning an explicit error) would be even safer.
📝 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.
| func (c *SDKConfiguration) GetServerDetails() (string, map[string]string) { | |
| if c.ServerURL != "" { | |
| return c.ServerURL, nil | |
| } | |
| return c.ServerList[c.ServerIndex], nil | |
| } | |
| func (c *SDKConfiguration) GetServerDetails() (string, map[string]string) { | |
| if c == nil { | |
| return "", nil | |
| } | |
| if c.ServerURL != "" { | |
| return c.ServerURL, nil | |
| } | |
| if c.ServerIndex < 0 || c.ServerIndex >= len(c.ServerList) { | |
| return "", nil // or return first entry / explicit error | |
| } | |
| return c.ServerList[c.ServerIndex], nil | |
| } |
🤖 Prompt for AI Agents
In pkg/client/internal/config/sdkconfiguration.go around lines 27 to 33, the
method GetServerDetails risks panicking by directly indexing ServerList with
ServerIndex without checking if ServerList is non-nil and non-empty or if
ServerIndex is within bounds. Modify the method to validate that ServerList is
not nil or empty and that ServerIndex is a valid index before accessing it. If
these conditions are not met, return an explicit error or a safe default to fail
fast and avoid runtime panics.
| rootSDK *Formance | ||
| sdkConfiguration config.SDKConfiguration | ||
| hooks *hooks.Hooks | ||
| } | ||
|
|
||
| func newLedger(sdkConfig sdkConfiguration) *Ledger { | ||
| func newLedger(rootSDK *Formance, sdkConfig config.SDKConfiguration, hooks *hooks.Hooks) *Ledger { | ||
| return &Ledger{ | ||
| rootSDK: rootSDK, | ||
| sdkConfiguration: sdkConfig, | ||
| V1: newV1(sdkConfig), | ||
| V2: newV2(sdkConfig), | ||
| hooks: hooks, | ||
| V1: newV1(rootSDK, sdkConfig, hooks), | ||
| V2: newV2(rootSDK, sdkConfig, hooks), | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Value-copying SDKConfiguration risks silent divergence
sdkConfiguration is stored by value in Ledger, V1, V2, etc.
Because the struct contains mutable pointer fields (e.g. *retry.Config, *time.Duration, custom HTTPClient), two copies may silently diverge if either party mutates a field at runtime (think per–request timeout overrides).
Prefer keeping a single shared pointer:
- sdkConfiguration config.SDKConfiguration
+ sdkConfiguration *config.SDKConfigurationand update call-sites accordingly.
This avoids accidental “copy-on-write” bugs and lowers the per-client copy cost.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/client/ledger.go around lines 24 to 36, the sdkConfiguration field is
currently stored by value in Ledger and its subcomponents, which risks silent
divergence due to mutable pointer fields inside the struct. To fix this, change
sdkConfiguration to be a pointer type throughout Ledger, V1, and V2 structs and
constructors, and update all call-sites to pass and use a pointer to
SDKConfiguration instead of a value copy. This ensures a single shared
configuration instance is used, preventing accidental copy-on-write bugs and
reducing memory overhead.
| @@ -748,7 +750,7 @@ func main() { | |||
|
|
|||
| res, err := s.Ledger.V2.DeleteAccountMetadata(ctx, operations.V2DeleteAccountMetadataRequest{ | |||
| Ledger: "ledger001", | |||
| Address: "96609 Cummings Canyon", | |||
| Address: "6753 S Washington Street", | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the address format change is intentional.
The address parameter changed from "users:001" to "6753 S Washington Street". This appears to be changing from an account identifier format to a physical street address, which may not be the intended format for the API's address parameter.
🏁 Script executed:
#!/bin/bash
# Description: Check other examples of address parameter usage in the documentation to verify the expected format
# Search for other DeleteAccountMetadata examples or address parameter usage
rg -A 5 -B 5 'Address.*"users:' pkg/client/docs/Length of output: 5443
Ensure Address uses the correct account identifier format
The Address field in all other V1 and V2 client examples is treated as an account identifier (e.g. "users:001"), not a postal address. Replacing it with "6753 S Washington Street" is inconsistent and likely incorrect. Please clarify and update:
- pkg/client/docs/sdks/v2/README.md line 753:
• If the API still expects an account ID, revert to"users:001".
• If you’ve intentionally changed Address to accept postal addresses, update all examples and the API docs to reflect this new format.
🤖 Prompt for AI Agents
In pkg/client/docs/sdks/v2/README.md at line 753, the Address parameter was
changed from an account identifier format like "users:001" to a physical street
address "6753 S Washington Street". Verify the intended format for the Address
parameter in the API. If the API expects an account identifier, revert the
Address value back to the original format "users:001". If the change to a postal
address is intentional, update all related examples and API documentation
consistently to reflect this new format.
| for accessToken, session := range s.sessions { | ||
| if session == nil { | ||
| continue | ||
| } | ||
|
|
||
| if session.isExpired() { | ||
| go s.delete(accessToken) | ||
|
|
||
| continue | ||
| } | ||
|
|
||
| if session.containsScopes(scopes) { | ||
| return session, nil | ||
| } | ||
| } | ||
|
|
||
| return s.create(ctx, httpClient, scopes) | ||
| } |
There was a problem hiding this comment.
Fix race condition in session iteration.
The getSessionContainingScopes method reads from the sessions map without holding the mutex, which can cause a race condition if another goroutine modifies the map concurrently.
Apply this diff to fix the race condition:
func (s oauth2Sessions) getSessionContainingScopes(ctx context.Context, httpClient HTTPClient, scopes []string) (*oAuth2Session, error) {
+ s.sessionsMutex.Lock()
+ // Create a list of sessions to check to avoid holding the lock during the entire iteration
+ var sessionsToCheck []*oAuth2Session
+ var accessTokensToDelete []string
+
for accessToken, session := range s.sessions {
if session == nil {
continue
}
if session.isExpired() {
- go s.delete(accessToken)
-
+ accessTokensToDelete = append(accessTokensToDelete, accessToken)
continue
}
if session.containsScopes(scopes) {
- return session, nil
+ s.sessionsMutex.Unlock()
+ return session, nil
}
}
+ s.sessionsMutex.Unlock()
+
+ // Delete expired sessions outside the critical section
+ for _, accessToken := range accessTokensToDelete {
+ s.delete(accessToken)
+ }
return s.create(ctx, httpClient, scopes)
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/client/internal/utils/oauth2_sessions.go around lines 131 to 148, the
method iterates over the sessions map without locking the mutex, causing a race
condition. To fix this, acquire a read lock on the mutex before iterating over
the sessions map and release it after the iteration completes. This ensures safe
concurrent access to the map while checking sessions.
| func (e oAuth2TokenEndpoint) deleteCredentialsSession(authorizationGrantor OAuth2AuthorizationGrantor) { | ||
| if authorizationGrantor == nil { | ||
| return | ||
| } | ||
|
|
||
| e.credentialSessionsMutex.Lock() | ||
| e.credentialSessions[authorizationGrantor.Hash()] = newOAuth2Sessions(authorizationGrantor) | ||
| e.credentialSessionsMutex.Unlock() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Method name doesn't match its behavior.
The deleteCredentialsSession method doesn't actually delete the credentials session - it replaces it with a new empty session collection. This is misleading.
Consider either:
- Renaming the method to
resetCredentialsSessionorclearCredentialsSessionto better reflect its behavior - Actually deleting the entry from the map:
func (e oAuth2TokenEndpoint) deleteCredentialsSession(authorizationGrantor OAuth2AuthorizationGrantor) {
if authorizationGrantor == nil {
return
}
e.credentialSessionsMutex.Lock()
- e.credentialSessions[authorizationGrantor.Hash()] = newOAuth2Sessions(authorizationGrantor)
+ delete(e.credentialSessions, authorizationGrantor.Hash())
e.credentialSessionsMutex.Unlock()
}📝 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.
| func (e oAuth2TokenEndpoint) deleteCredentialsSession(authorizationGrantor OAuth2AuthorizationGrantor) { | |
| if authorizationGrantor == nil { | |
| return | |
| } | |
| e.credentialSessionsMutex.Lock() | |
| e.credentialSessions[authorizationGrantor.Hash()] = newOAuth2Sessions(authorizationGrantor) | |
| e.credentialSessionsMutex.Unlock() | |
| } | |
| func (e oAuth2TokenEndpoint) deleteCredentialsSession(authorizationGrantor OAuth2AuthorizationGrantor) { | |
| if authorizationGrantor == nil { | |
| return | |
| } | |
| e.credentialSessionsMutex.Lock() | |
| delete(e.credentialSessions, authorizationGrantor.Hash()) | |
| e.credentialSessionsMutex.Unlock() | |
| } |
🤖 Prompt for AI Agents
In pkg/client/internal/utils/oauth2_sessions.go around lines 180 to 188, the
method named deleteCredentialsSession does not delete the session but replaces
it with a new empty session collection, which is misleading. To fix this, either
rename the method to resetCredentialsSession or clearCredentialsSession to
reflect its behavior accurately, or modify the method to actually delete the
entry from the credentialSessions map using the delete keyword with the
authorizationGrantor's hash as the key.
| if response.TokenType != "Bearer" { | ||
| return nil, fmt.Errorf("unexpected token type: %s", response.TokenType) | ||
| } |
There was a problem hiding this comment.
Make token type check case-insensitive.
According to RFC 6749, the token_type value is case-insensitive. The current implementation might reject valid responses with different casing.
-if response.TokenType != "Bearer" {
+if strings.ToLower(response.TokenType) != "bearer" {
return nil, fmt.Errorf("unexpected token type: %s", response.TokenType)
}🤖 Prompt for AI Agents
In pkg/client/internal/utils/oauth2_credentials.go around lines 173 to 175, the
token type check is currently case-sensitive, which can incorrectly reject valid
token types with different casing. Modify the condition to compare the token
type in a case-insensitive manner by converting both values to the same case
(e.g., lowercase) before comparison.
| if httpResponse.StatusCode < 200 || httpResponse.StatusCode >= 300 { | ||
| httpResponseBody, _ := io.ReadAll(httpResponse.Body) | ||
| return nil, fmt.Errorf("unexpected status code: %d: %s", httpResponse.StatusCode, httpResponseBody) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error response handling.
The current implementation might include sensitive information or excessively long response bodies in error messages.
if httpResponse.StatusCode < 200 || httpResponse.StatusCode >= 300 {
httpResponseBody, _ := io.ReadAll(httpResponse.Body)
- return nil, fmt.Errorf("unexpected status code: %d: %s", httpResponse.StatusCode, httpResponseBody)
+ // Limit response body in error message to prevent logging sensitive data or excessive content
+ responseSnippet := string(httpResponseBody)
+ if len(responseSnippet) > 200 {
+ responseSnippet = responseSnippet[:200] + "..."
+ }
+ return nil, fmt.Errorf("token request failed with status %d: %s", httpResponse.StatusCode, responseSnippet)
}📝 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 httpResponse.StatusCode < 200 || httpResponse.StatusCode >= 300 { | |
| httpResponseBody, _ := io.ReadAll(httpResponse.Body) | |
| return nil, fmt.Errorf("unexpected status code: %d: %s", httpResponse.StatusCode, httpResponseBody) | |
| } | |
| if httpResponse.StatusCode < 200 || httpResponse.StatusCode >= 300 { | |
| httpResponseBody, _ := io.ReadAll(httpResponse.Body) | |
| // Limit response body in error message to prevent logging sensitive data or excessive content | |
| responseSnippet := string(httpResponseBody) | |
| if len(responseSnippet) > 200 { | |
| responseSnippet = responseSnippet[:200] + "..." | |
| } | |
| return nil, fmt.Errorf("token request failed with status %d: %s", httpResponse.StatusCode, responseSnippet) | |
| } |
🤖 Prompt for AI Agents
In pkg/client/internal/utils/oauth2_credentials.go around lines 162 to 165, the
error handling returns the entire HTTP response body in the error message, which
may expose sensitive data or be too large. Modify the code to limit the size of
the response body included in the error message, for example by truncating it to
a reasonable length, and sanitize or omit any sensitive information before
including it in the error string.
| // HTTPClient provides an interface for supplying the SDK with a custom HTTP client | ||
| type HTTPClient interface { | ||
| Do(req *http.Request) (*http.Response, error) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid duplicating the HTTPClient interface.
The HTTPClient interface is already defined in pkg/client/internal/config/sdkconfiguration.go and pkg/client/internal/hooks/hooks.go. Consider importing it from a common location instead of redefining it here.
🏁 Script executed:
#!/bin/bash
# Description: Check if there's a common location for the HTTPClient interface or if refactoring is needed
# Find all HTTPClient interface definitions
ast-grep --pattern 'type HTTPClient interface {
Do(req *http.Request) (*http.Response, error)
}'
# Check import relationships
rg -l "HTTPClient interface" --type go | xargs -I {} sh -c 'echo "=== {} ==="; head -20 {}'Length of output: 2828
Consolidate HTTPClient interface definitions
The HTTPClient interface is currently defined in four places:
pkg/client/formance.go(package client)pkg/client/internal/config/sdkconfiguration.go(package config)pkg/client/internal/hooks/hooks.go(package hooks)pkg/client/internal/utils/oauth2_credentials.go(package utils)
To avoid distinct, incompatible types with identical signatures, remove the local definition in pkg/client/internal/utils/oauth2_credentials.go and import the shared interface from the config package (github.com/formancehq/ledger/pkg/client/internal/config). For example:
-import (
- // other imports
-)
-
-// HTTPClient provides an interface for supplying the SDK with a custom HTTP client
-type HTTPClient interface {
- Do(req *http.Request) (*http.Response, error)
-}
+import (
+ "github.com/formancehq/ledger/pkg/client/internal/config"
+ // other imports
+)
+
+// HTTPClient aliases the shared interface from config
+type HTTPClient = config.HTTPClientAdditionally, update your Speakeasy generator template so that all generated code consistently references the same HTTPClient from the config package.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/client/internal/utils/oauth2_credentials.go around lines 19 to 22, remove
the local definition of the HTTPClient interface to avoid duplication. Instead,
import the HTTPClient interface from the common config package at
github.com/formancehq/ledger/pkg/client/internal/config. Update the import
statements accordingly and replace all local references to HTTPClient with the
imported one. Also, ensure the Speakeasy generator template is updated to
consistently use the HTTPClient from the config package across all generated
code.
gfyrag
force-pushed
the
chore/upgrade-speakeasy
branch
from
23e465b to
50d18b6
Compare
No description provided.