fix(open-api): make insertedAt optional #616
Conversation
WalkthroughThe pull request introduces extensive updates to the Ledger API v2 documentation, enhancing the Changes
Possibly related PRs
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
gfyrag
force-pushed
the
fix/InsertedAtOptionnal
branch
from
5fb49c2 to
da10f66
Compare
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumopenapi.yamlis excluded by!**/*.yamlopenapi/v2.yamlis excluded by!**/*.yamlpkg/client/.speakeasy/gen.lockis excluded by!**/*.lock,!**/*.locktools/generator/go.modis excluded by!**/*.modtools/generator/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (7)
docs/api/README.md(1 hunks)pkg/client/docs/models/components/v2errorsenum.md(1 hunks)pkg/client/docs/models/components/v2transaction.md(1 hunks)pkg/client/models/components/v2errorsenum.go(2 hunks)pkg/client/models/components/v2transaction.go(2 hunks)pkg/testserver/helpers.go(1 hunks)test/e2e/api_balances_aggregated_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/client/docs/models/components/v2errorsenum.md
- pkg/client/models/components/v2errorsenum.go
- pkg/client/models/components/v2transaction.go
- pkg/client/docs/models/components/v2transaction.md
🔇 Additional comments (2)
docs/api/README.md (2)
2638-2638: Documentation accurately reflects InsertedAt optionality
The schema update correctly shows that InsertedAt is now an optional field in the V2Transaction type.
2638-2638: Verify all InsertedAt usages in the codebase
Let's verify that all other usages of InsertedAt have been updated to handle the optional nature of the field.
✅ Verification successful
Let me gather more context about the InsertedAt field's type definition and usage in API responses.
Based on the gathered information, I can now provide the final response:
InsertedAt field is already properly handled as optional
The codebase already handles InsertedAt as an optional field:
- In the model definition (
pkg/client/models/components/v2transaction.go), InsertedAt is properly defined as a pointer (*time.Time) withomitemptyJSON tag - The test cases and helper functions handle the field appropriately
- The OpenAPI/Swagger spec correctly defines it as a date-time string without required constraint
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of InsertedAt to verify they handle nil case
# Search for direct InsertedAt field access
rg -A 2 "InsertedAt\."
# Search for InsertedAt in struct initialization
rg -A 2 "InsertedAt:"
# Search for InsertedAt in type definitions
ast-grep --pattern 'InsertedAt $type'
Length of output: 882
Script:
#!/bin/bash
# Search for InsertedAt in API response types and models
rg -A 3 -B 1 "type.*struct" | rg -A 3 -B 1 "InsertedAt"
# Search for InsertedAt in OpenAPI/Swagger specs
fd -e yaml -e json --exec rg -A 2 "insertedAt:"
# Search for validation or handling of InsertedAt
rg -A 3 "InsertedAt.*\b(nil|optional|required)\b"
Length of output: 756
| @@ -26,7 +26,7 @@ func ConvertSDKTxToCoreTX(tx *components.V2Transaction) ledger.Transaction { | |||
| TransactionData: ledger.TransactionData{ | |||
| Postings: collectionutils.Map(tx.Postings, ConvertSDKPostingToCorePosting), | |||
| Timestamp: time.New(tx.Timestamp), | |||
| InsertedAt: time.New(tx.InsertedAt), | |||
| InsertedAt: time.New(*tx.InsertedAt), | |||
There was a problem hiding this comment.
Add nil check before dereferencing InsertedAt pointer
The code assumes tx.InsertedAt is never nil, which could lead to a panic. Since InsertedAt is now optional, we should handle the nil case gracefully.
Consider adding a nil check:
-InsertedAt: time.New(*tx.InsertedAt),
+InsertedAt: func() time.Time {
+ if tx.InsertedAt == nil {
+ return time.Now() // or another appropriate default
+ }
+ return time.New(*tx.InsertedAt)
+}(),📝 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.
| InsertedAt: time.New(*tx.InsertedAt), | |
| InsertedAt: func() time.Time { | |
| if tx.InsertedAt == nil { | |
| return time.Now() // or another appropriate default | |
| } | |
| return time.New(*tx.InsertedAt) | |
| }(), |
| @@ -99,7 +99,7 @@ var _ = Context("Ledger engine tests", func() { | |||
| }) | |||
| Expect(err).To(Succeed()) | |||
|
|
|||
| firstTransactionsInsertedAt = ret[2].V2BulkElementResultCreateTransaction.Data.InsertedAt | |||
| firstTransactionsInsertedAt = *ret[2].V2BulkElementResultCreateTransaction.Data.InsertedAt | |||
There was a problem hiding this comment.
Add nil check in test for InsertedAt
Since InsertedAt is now optional, the test should verify both cases: when InsertedAt is present and when it's nil.
Add test coverage for nil case and add nil check:
-firstTransactionsInsertedAt = *ret[2].V2BulkElementResultCreateTransaction.Data.InsertedAt
+if ret[2].V2BulkElementResultCreateTransaction.Data.InsertedAt == nil {
+ t.Fatal("expected InsertedAt to be set in test case")
+}
+firstTransactionsInsertedAt = *ret[2].V2BulkElementResultCreateTransaction.Data.InsertedAt📝 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.
| firstTransactionsInsertedAt = *ret[2].V2BulkElementResultCreateTransaction.Data.InsertedAt | |
| if ret[2].V2BulkElementResultCreateTransaction.Data.InsertedAt == nil { | |
| t.Fatal("expected InsertedAt to be set in test case") | |
| } | |
| firstTransactionsInsertedAt = *ret[2].V2BulkElementResultCreateTransaction.Data.InsertedAt |
No description provided.