chore: add openapi tag #670
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces significant changes across multiple files, primarily focusing on API versioning and router configuration. The main modifications include adding a Changes
Sequence DiagramsequenceDiagram
participant Client
participant Router
participant V2Controller
participant V1Controller
Client->>Router: Request with version
Router->>V2Controller: Route to V2 endpoint
alt If V2 not available
Router->>V1Controller: Fallback to V1 endpoint
end
V2Controller-->>Router: Process request
Router-->>Client: Return response
Possibly related PRs
Suggested Reviewers
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
internal/storage/bucket/migrations/11-make-stateless/up.sql (1)
Line range hint
464-469: Consider adding a migration verification step.The hash computation logic is critical for data integrity. Consider adding a verification step in the migration to ensure all existing hashes are correctly recomputed.
Apply this diff to add hash verification:
new.hash = ( select public.digest( case when previousHash is null then marshalledAsJSON::bytea else '"' || encode(previousHash::bytea, 'base64')::bytea || E'"\n' || convert_to(marshalledAsJSON, 'UTF-8')::bytea end || E'\n', 'sha256'::text ) ); + -- Verify hash computation + if TG_OP = 'UPDATE' and new.hash != old.hash then + raise notice 'Hash changed for log % in ledger %: old=%, new=%', + new.id, new.ledger, encode(old.hash, 'hex'), encode(new.hash, 'hex'); + end if;
🧹 Nitpick comments (12)
internal/storage/ledger/legacy/adapters.go (5)
18-39: Handle potential type assertion issues inGetOne()for accounts.
Currently, the code (line 25) usesvalue.(string)with no error handling. If the query builder or user input is malformed, this could result in a panic. Consider adding a type check or returning an error for robustness.- address = value.(string) + s, ok := value.(string) + if !ok { + return nil, fmt.Errorf("expected string address, got %T", value) + } + address = s
72-98: Clarify panics for unimplemented log methods.
MethodsGetOneandCountpanic with"never used". If these methods might be called in future, consider returning a descriptive error instead. Otherwise, the usage is acceptable for stubbing.
99-152: Similar type assertion concern in transactions'GetOne().
Like accounts'GetOne(), line 106 usesvalue.(int)without checking. Handling it gracefully prevents unexpected panics. Other than that,CountandPaginatemethods look structurally sound.
153-188: Simplify the big.Int addition inaggregatedBalancesPaginatedResourceAdapter.
Line 175 repeatedly creates new big.Int and adds a zero. UsingSet(balance)or a single new call is more explicit:- Input: new(big.Int).Add(new(big.Int), balance), + tmp := new(big.Int).Set(balance) + Input: tmp,
189-217: Panic placeholders in aggregated volumes.
Similar to logs,GetOneandCountpanic on usage. This is acceptable if guaranteed never to be called, but consider returning a not-implemented error if there's a possibility of future invocation.internal/api/v2/controllers_ledgers_read_test.go (1)
25-25: Consider using a constant for the version string.The hardcoded "develop" string should be defined as a constant to improve maintainability and ensure consistency across test files.
+const testVersion = "develop" + func TestLedgersRead(t *testing.T) { // ... - router := NewRouter(systemController, auth.NewNoAuth(), "develop", os.Getenv("DEBUG") == "true") + router := NewRouter(systemController, auth.NewNoAuth(), testVersion, os.Getenv("DEBUG") == "true")test/e2e/app_multiple_instance_test.go (1)
53-55: Improve version assertion and use constant.The test correctly verifies the version but could be improved:
- Use a constant for the expected version string
- Consider adding more assertions about the info response
+const expectedVersion = "develop" + func TestLedgerApplication(t *testing.T) { // ... info, err := server.Client().Ledger.V2.GetInfo(ctx) Expect(err).NotTo(HaveOccurred()) - Expect(info.V2ConfigInfoResponse.Version).To(Equal("develop")) + Expect(info.V2ConfigInfoResponse.Version).To(Equal(expectedVersion)) + // Add more assertions about the info responseinternal/api/v2/controllers_accounts_add_metadata_test.go (1)
75-75: Consider extracting version as a constant.The hardcoded "develop" version string is used consistently across test files. Consider extracting it to a package-level constant for better maintainability.
+const ( + TestAPIVersion = "develop" +) + -router := NewRouter(systemController, auth.NewNoAuth(), "develop", os.Getenv("DEBUG") == "true") +router := NewRouter(systemController, auth.NewNoAuth(), TestAPIVersion, os.Getenv("DEBUG") == "true")internal/api/v2/controllers_transactions_revert_test.go (1)
84-84: Consider adding version-specific test cases.Since we're adding version support to the router, consider adding test cases that verify the router's behavior with different API versions.
type testCase struct { name string queryParams url.Values + version string returnTx ledger.Transaction returnErr error expectForce bool expectStatusCode int expectErrorCode string } testCases := []testCase{ { name: "nominal", + version: "develop", returnTx: ledger.NewTransaction()..., }, + { + name: "with_v2_1", + version: "v2.1", + returnTx: ledger.NewTransaction()..., + },internal/api/v1/routes.go (1)
32-32: LGTM! Consider adding documentation for the exported GetInfo function.The change to export the GetInfo function and pass the version parameter aligns well with the OpenAPI tag requirements. This enables proper versioning information in the API responses.
Add godoc comments to document the exported GetInfo function's purpose and parameters.
internal/api/v2/controllers_ledgers_list_test.go (1)
98-98: Consider using a test-specific version string.Using
"develop"as the version string in tests might not be the most appropriate choice. Consider using a more explicit test version like"test-version"or extracting it as a constant.-router := NewRouter(systemController, auth.NewNoAuth(), "develop", os.Getenv("DEBUG") == "true") +const testVersion = "test-version" +router := NewRouter(systemController, auth.NewNoAuth(), testVersion, os.Getenv("DEBUG") == "true")internal/storage/bucket/migrations/11-make-stateless/up.sql (1)
Line range hint
546-567: Review the transaction reference uniqueness implementation.The constraint trigger
enforce_reference_uniquenessuses an advisory lock to prevent duplicate references. While this works, consider these aspects:
- The advisory lock could impact concurrency
- The uniqueness check is performed after insert
Consider alternatives:
- Use a unique index instead of a trigger for better performance
- Add a comment explaining that this is a temporary solution until the next migration
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
.github/workflows/main.ymlis excluded by!**/*.ymlopenapi/v2.yamlis excluded by!**/*.yamlpkg/client/.speakeasy/gen.lockis excluded by!**/*.lock,!**/*.lockpkg/client/.speakeasy/gen.yamlis excluded by!**/*.yaml
📒 Files selected for processing (48)
internal/api/router.go(1 hunks)internal/api/v1/controllers_config.go(1 hunks)internal/api/v1/routes.go(1 hunks)internal/api/v2/controllers_accounts_add_metadata_test.go(1 hunks)internal/api/v2/controllers_accounts_count_test.go(1 hunks)internal/api/v2/controllers_accounts_delete_metadata_test.go(1 hunks)internal/api/v2/controllers_accounts_list_test.go(1 hunks)internal/api/v2/controllers_accounts_read_test.go(1 hunks)internal/api/v2/controllers_balances_test.go(1 hunks)internal/api/v2/controllers_bulk_test.go(1 hunks)internal/api/v2/controllers_ledgers_create_test.go(1 hunks)internal/api/v2/controllers_ledgers_delete_metadata_test.go(1 hunks)internal/api/v2/controllers_ledgers_info_test.go(1 hunks)internal/api/v2/controllers_ledgers_list_test.go(1 hunks)internal/api/v2/controllers_ledgers_read_test.go(1 hunks)internal/api/v2/controllers_ledgers_update_metadata_test.go(1 hunks)internal/api/v2/controllers_logs_export_test.go(1 hunks)internal/api/v2/controllers_logs_import_test.go(1 hunks)internal/api/v2/controllers_logs_list_test.go(1 hunks)internal/api/v2/controllers_stats_test.go(1 hunks)internal/api/v2/controllers_transactions_add_metadata_test.go(1 hunks)internal/api/v2/controllers_transactions_count_test.go(1 hunks)internal/api/v2/controllers_transactions_create_test.go(1 hunks)internal/api/v2/controllers_transactions_delete_metadata_test.go(1 hunks)internal/api/v2/controllers_transactions_list_test.go(1 hunks)internal/api/v2/controllers_transactions_read_test.go(1 hunks)internal/api/v2/controllers_transactions_revert_test.go(1 hunks)internal/api/v2/controllers_volumes_test.go(1 hunks)internal/api/v2/routes.go(3 hunks)internal/controller/ledger/log_process.go(1 hunks)internal/storage/bucket/migrations/11-make-stateless/up.sql(1 hunks)internal/storage/bucket/migrations/23-logs-fill-memento/up.sql(1 hunks)internal/storage/ledger/legacy/adapters.go(1 hunks)internal/storage/ledger/logs.go(1 hunks)internal/storage/ledger/logs_test.go(2 hunks)internal/storage/ledger/transactions_test.go(1 hunks)pkg/client/README.md(10 hunks)pkg/client/USAGE.md(1 hunks)pkg/client/docs/sdks/ledger/README.md(0 hunks)pkg/client/docs/sdks/v2/README.md(2 hunks)pkg/client/formance.go(1 hunks)pkg/client/ledger.go(0 hunks)pkg/client/v2.go(1 hunks)pkg/testserver/api.go(1 hunks)test/e2e/api_transactions_create_test.go(2 hunks)test/e2e/app_lifecycle_test.go(1 hunks)test/e2e/app_multiple_instance_test.go(1 hunks)test/performance/benchmark_test.go(1 hunks)
💤 Files with no reviewable changes (2)
- pkg/client/ledger.go
- pkg/client/docs/sdks/ledger/README.md
✅ Files skipped from review due to trivial changes (4)
- internal/controller/ledger/log_process.go
- internal/storage/ledger/transactions_test.go
- pkg/client/formance.go
- internal/api/v2/controllers_transactions_create_test.go
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
pkg/client/USAGE.md
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)
pkg/client/README.md
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-112: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
113-113: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
168-168: Hard tabs
Column: 1
(MD010, no-hard-tabs)
182-182: Hard tabs
Column: 1
(MD010, no-hard-tabs)
221-221: Hard tabs
Column: 1
(MD010, no-hard-tabs)
222-222: Hard tabs
Column: 1
(MD010, no-hard-tabs)
223-223: Hard tabs
Column: 1
(MD010, no-hard-tabs)
224-224: Hard tabs
Column: 1
(MD010, no-hard-tabs)
225-225: 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)
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)
354-354: Hard tabs
Column: 1
(MD010, no-hard-tabs)
355-355: Hard tabs
Column: 1
(MD010, no-hard-tabs)
356-356: Hard tabs
Column: 1
(MD010, no-hard-tabs)
357-357: Hard tabs
Column: 1
(MD010, no-hard-tabs)
358-358: Hard tabs
Column: 1
(MD010, no-hard-tabs)
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)
pkg/client/docs/sdks/v2/README.md
44-44: Hard tabs
Column: 1
(MD010, no-hard-tabs)
45-45: Hard tabs
Column: 1
(MD010, no-hard-tabs)
46-46: Hard tabs
Column: 1
(MD010, no-hard-tabs)
47-47: Hard tabs
Column: 1
(MD010, no-hard-tabs)
80-80: Tables should be surrounded by blank lines
null
(MD058, blanks-around-tables)
95-95: Hard tabs
Column: 1
(MD010, no-hard-tabs)
96-96: Hard tabs
Column: 1
(MD010, no-hard-tabs)
97-97: Hard tabs
Column: 1
(MD010, no-hard-tabs)
98-98: Hard tabs
Column: 1
(MD010, no-hard-tabs)
131-131: Tables should be surrounded by blank lines
null
(MD058, blanks-around-tables)
🔇 Additional comments (42)
internal/storage/ledger/legacy/adapters.go (4)
41-53: Straightforward counting logic.
This code correctly delegates toCountAccounts. No immediate issues found.
55-68: Pagination logic for accounts looks correct.
The method neatly passes query options toGetAccountsWithVolumes. Implementation aligns with standard pagination best practices.
70-71: Interface conformance check is valid.
Declaringvar _ ledgercontroller.PaginatedResource...confirmsaccountsPaginatedResourceAdapterimplements the required interface.
225-227: Conditional fallback logic inDefaultStoreAdapteris well-structured.
It ensures seamless fallback whenisFullUpToDateis not set, returning the relevant legacy adapters. This code is clean and clear.Also applies to: 232-234, 239-241, 246-248, 253-255
internal/api/v2/controllers_ledgers_update_metadata_test.go (1)
29-29: Consistent addition of the "develop" parameter toNewRouter.
The change aligns with the updated router signature. No issues found.internal/api/v2/controllers_stats_test.go (1)
20-20: Router instantiation updated with version argument.
Including"develop"follows the newNewRoutersignature. Looks good.internal/api/v2/controllers_ledgers_info_test.go (1)
22-22: Use consistent version constant.Same feedback as controllers_ledgers_read_test.go - the hardcoded version string should be replaced with a constant.
internal/api/v2/controllers_transactions_read_test.go (1)
40-40: Use consistent version constant.Same feedback as controllers_ledgers_read_test.go - the hardcoded version string should be replaced with a constant.
internal/api/v1/controllers_config.go (1)
35-35: LGTM! Function export aligns with API versioning requirements.The change from
getInfotoGetInfocorrectly exposes this endpoint for external package usage, which aligns with the API versioning requirements mentioned in the PR objectives.internal/api/v2/controllers_ledgers_delete_metadata_test.go (1)
57-57: LGTM! Router initialization updated with version parameter.The addition of the version parameter aligns with the PR objectives for API versioning and compatibility layer handling.
internal/api/v2/controllers_logs_export_test.go (1)
65-65: LGTM! Router initialization updated with version parameter.The addition of the version parameter aligns with the PR objectives for API versioning and compatibility layer handling.
internal/api/v2/controllers_logs_import_test.go (1)
76-76: LGTM! Router initialization updated with version parameter.The addition of the version parameter aligns with the PR objectives for API versioning and compatibility layer handling.
internal/api/v2/controllers_transactions_delete_metadata_test.go (1)
71-71: Verify version parameter usage across the codebase.The addition of "develop" as a version parameter appears to be part of a broader API versioning strategy. Let's verify its consistent usage and implementation.
✅ Verification successful
Version parameter usage is properly implemented
The version parameter is consistently implemented across the codebase:
- Test files appropriately use "develop" for testing purposes
- Production code correctly uses a Version variable for proper versioning
- The implementation is uniform across both v1 and v2 API versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the NewRouter function signature and version parameter usage # Check NewRouter function definition echo "Checking NewRouter function definition:" rg -A 5 "func NewRouter\(" # Check all NewRouter calls to ensure consistent version parameter echo "Checking NewRouter calls:" rg "NewRouter\(.*\)" --type goLength of output: 7570
Script:
#!/bin/bash # Check router initialization in serve.go echo "Checking serve.go content:" cat cmd/serve.goLength of output: 7590
internal/api/v2/controllers_accounts_delete_metadata_test.go (1)
75-75: LGTM!The router initialization is consistent with other test files, maintaining the same version parameter usage.
internal/api/v2/controllers_transactions_add_metadata_test.go (1)
102-102: LGTM! Router initialization updated for version support.The addition of the version parameter ensures consistency with the router's new versioning capabilities.
internal/api/v2/controllers_accounts_read_test.go (1)
96-96: LGTM! Router initialization aligned with versioning requirements.The addition of the version parameter maintains consistency with the router's versioning support.
internal/api/v2/controllers_balances_test.go (1)
103-103: LGTM! Router initialization updated consistently.The addition of the version parameter aligns with the router's versioning support across test files.
internal/storage/ledger/logs.go (1)
25-25: Verify backward compatibility of the Memento field type change.Changing the field type from
RawMessageto[]bytecould affect existing data. While both types can store binary data, the serialization behavior might differ.✅ Verification successful
The Memento field type change is backward compatible
The change from
RawMessageto[]byteis safe because:
- The
Mementointerface is type-agnostic (returnsany)- Both types are compatible with PostgreSQL's
byteacolumn- Existing data is properly migrated with UTF-8 encoding
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing usage of the Memento field rg -A 5 'Memento.*RawMessage' .Length of output: 33
Script:
#!/bin/bash # Search for Memento field usage echo "=== Searching for Memento field usage ===" rg -p "Memento" . echo -e "\n=== Looking for database migrations ===" fd -e sql .Length of output: 4511
Script:
#!/bin/bash # Check the memento migration file cat internal/storage/bucket/migrations/23-logs-fill-memento/up.sqlLength of output: 810
internal/api/router.go (1)
Line range hint
67-75: LGTM! Enhanced router configuration with versioning and bulk handling support.The changes improve the router's capabilities by:
- Adding version information
- Supporting bulk operations with multiple content types
- Maintaining consistent configuration across v1 and v2 routes
internal/api/v2/controllers_ledgers_create_test.go (1)
103-103: Same version string concern as in controllers_ledgers_list_test.go.Consider applying the same suggestion to use a test-specific version string here.
test/performance/benchmark_test.go (1)
120-120: LGTM! Consistent use of V2 API endpoint.The change correctly updates the metrics endpoint to use the V2 API, aligning with the versioning strategy.
Let's verify that all metrics-related calls use the V2 endpoint consistently:
✅ Verification successful
Confirmed: GetMetrics consistently uses V2 API endpoint
All GetMetrics calls in the codebase correctly use the V2 endpoint, with the implementation properly located in the V2 package.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining unversioned GetMetrics calls # Test: Search for GetMetrics calls. Expect: Only V2 endpoint usage. rg -A 2 'GetMetrics\(' --type goLength of output: 495
internal/api/v2/controllers_accounts_count_test.go (1)
141-141: LGTM! Router initialization updated with version parameter.The test correctly initializes the router with the "develop" version parameter.
internal/api/v2/routes.go (3)
6-6: LGTM! Added v1 package import.The import is required for handling the /_info endpoint using v1.GetInfo.
25-25: LGTM! Added version parameter to NewRouter.The version parameter enables version-aware routing, which is essential for API versioning.
41-42: LGTM! Added /_info endpoint using v1.GetInfo.This change fixes the breaking change in the GET /_info endpoint (issue #666) by properly handling version information.
internal/api/v2/controllers_transactions_count_test.go (1)
167-167: LGTM! Router initialization updated with version parameter.The test correctly initializes the router with the "develop" version parameter.
internal/api/v2/controllers_volumes_test.go (1)
153-153: LGTM! Verify version handling.The addition of the version parameter aligns with the broader API versioning changes.
Run the following script to verify consistent version handling across router initializations:
✅ Verification successful
Version handling is consistent across the codebase
All router initializations in test files consistently use "develop" as the version parameter, and the main router properly handles API versioning for both v1 and v2 endpoints.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent version parameter usage across router initializations # Test: Search for NewRouter calls. Expect: All calls should include the version parameter rg -A 1 'NewRouter\(' | grep -v 'func NewRouter'Length of output: 9487
internal/api/v2/controllers_logs_list_test.go (1)
177-177: LGTM!The addition of the version parameter is consistent with the API versioning changes.
internal/storage/ledger/logs_test.go (2)
26-26: LGTM! Function names are now more consistent.The renaming of test functions to include the "Logs" prefix improves naming consistency.
Also applies to: 171-171
142-168: Great addition of special character handling tests!The new test cases for escaped quotes and UTF-8 characters in metadata are crucial for ensuring robust handling of special characters in transactions.
Let's verify the special character handling in the codebase:
✅ Verification successful
Special character handling is properly implemented
The test cases effectively verify that the system correctly handles both escaped quotes and UTF-8 characters in metadata through JSON encoding, which is properly supported by the underlying PostgreSQL JSON storage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential special character handling issues # Test: Search for metadata handling code. Expect: Proper escaping/encoding # Look for metadata handling in transaction-related code ast-grep --pattern 'WithMetadata($$$)' # Check for any explicit character encoding/escaping rg -A 3 'encoding|escape|utf'Length of output: 31026
internal/api/v2/controllers_accounts_list_test.go (1)
221-221: LGTM!The addition of the version parameter is consistent with the API versioning changes.
pkg/testserver/api.go (1)
30-30: LGTM! API version update is consistent.The change to use
V2.GetInfoaligns with the broader effort to transition to V2 API endpoints.internal/api/v2/controllers_transactions_list_test.go (1)
262-262: LGTM! Router configuration update is consistent.The addition of the "develop" environment parameter aligns with the router configuration changes and matches the environment being verified in the app lifecycle tests.
test/e2e/app_lifecycle_test.go (1)
52-52: LGTM! API version update and test verification are consistent.The change to use
V2.GetInfoaligns with the V2 API transition and correctly verifies the "develop" environment configuration.internal/api/v2/controllers_bulk_test.go (1)
488-488: LGTM! Router configuration update is consistent.The addition of the "develop" environment parameter aligns with the router configuration changes seen across other test files.
test/e2e/api_transactions_create_test.go (1)
85-88: LGTM! Enhanced test coverage for metadata handling.The test now includes validation for special characters in metadata, specifically:
- Quoted values: Testing proper escaping of quotes
- UTF-8 characters: Testing proper handling of non-ASCII characters
pkg/client/v2.go (2)
30-219: LGTM! Well-implemented GetInfo method.The implementation includes:
- Proper OAuth2 scope handling
- Comprehensive error handling
- Configurable timeout and retry logic
- Content-type validation
221-398: LGTM! Well-implemented GetMetrics method.The implementation follows the same robust pattern as GetInfo with:
- Proper error handling
- Retry logic for specific status codes
- Response validation
internal/storage/bucket/migrations/23-logs-fill-memento/up.sql (1)
23-23: LGTM! Improved character encoding support.Changed encoding from LATIN1 to UTF-8 to properly support international characters in log data. This change aligns with the enhanced UTF-8 character support in the API tests.
pkg/client/README.md (1)
72-76: LGTM! Documentation updates are consistent with the API versioning changes.The changes correctly reflect the shift to versioned API endpoints and error handling. The examples are clear and well-documented.
Also applies to: 168-182, 221-225, 266-269, 319-323, 354-358, 433-437
🧰 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)
internal/storage/bucket/migrations/11-make-stateless/up.sql (1)
464-464: Verify the impact of changing hash encoding from LATIN1 to UTF-8.The change in encoding from LATIN1 to UTF-8 for hash computation could affect data integrity if there are existing hashes in the database. This change might cause hash mismatches for existing records.
Run the following script to check for potential hash mismatches:
pkg/client/docs/sdks/v2/README.md (1)
6-7: LGTM! Documentation for new V2 endpoints is clear and consistent.The new operations
GetInfoandGetMetricsare well-documented with clear examples, parameters, and error handling sections. The documentation follows the established pattern.Also applies to: 34-84, 85-135
| res, err := s.Ledger.V1.GetInfo(ctx) | ||
| if err != nil { | ||
| log.Fatal(err) | ||
| } | ||
| if res.V2ConfigInfoResponse != nil { | ||
| if res.ConfigInfoResponse != nil { | ||
| // handle response | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update example to use V2 API.
The example should be updated to use the V2 API since GetInfo has been moved to V2:
- res, err := s.Ledger.V1.GetInfo(ctx)
+ res, err := s.Ledger.V2.GetInfo(ctx)
if err != nil {
log.Fatal(err)
}
- if res.ConfigInfoResponse != nil {
+ if res.V2ConfigInfoResponse != nil {
// handle response
}📝 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.
| res, err := s.Ledger.V1.GetInfo(ctx) | |
| if err != nil { | |
| log.Fatal(err) | |
| } | |
| if res.V2ConfigInfoResponse != nil { | |
| if res.ConfigInfoResponse != nil { | |
| // handle response | |
| } | |
| res, err := s.Ledger.V2.GetInfo(ctx) | |
| if err != nil { | |
| log.Fatal(err) | |
| } | |
| if res.V2ConfigInfoResponse != nil { | |
| // handle response | |
| } |
🧰 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)
26-26: Hard tabs
Column: 1
(MD010, no-hard-tabs)
27-27: Hard tabs
Column: 1
(MD010, no-hard-tabs)
No description provided.