fix: GET /_info breaking change #666
Conversation
WalkthroughThis pull request introduces a series of modifications across multiple test files and routing configurations in the internal API. The primary changes involve replacing environment variable-based debug mode configuration with a hardcoded "develop" string and modifying the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #666 +/- ##
==========================================
- Coverage 81.71% 81.58% -0.13%
==========================================
Files 131 131
Lines 7059 7061 +2
==========================================
- Hits 5768 5761 -7
- Misses 990 996 +6
- Partials 301 304 +3 ☔ View full report in Codecov by Sentry. |
gfyrag
force-pushed
the
fix/api-breaking-change-info
branch
from
ce088ee to
4d7a0d3
Compare
gfyrag
force-pushed
the
fix/api-breaking-change-info
branch
from
4d7a0d3 to
b1bc475
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: 0
🔭 Outside diff range comments (1)
internal/api/v1/routes.go (1)
Line range hint
13-20: Remove unuseddebugparameter fromNewRoutersignature.The
debugparameter appears to be unused and redundant since the version information is now passed directly toGetInfo. This aligns with the changes in test files where debug mode is replaced with version string.Apply this diff to remove the unused parameter:
func NewRouter( systemController system.Controller, authenticator auth.Authenticator, version string, - debug bool, opts ...RouterOption, ) chi.Router {
🧹 Nitpick comments (5)
internal/api/v1/controllers_config.go (1)
35-35: Add documentation for the exported function.Since
GetInfois now exported, it should have a documentation comment explaining its purpose and behavior.+// GetInfo returns a handler function that provides server configuration information +// including server name, version, and storage details. +// Parameters: +// - systemController: Controller interface for system operations +// - version: Version string to be included in the response +// Returns: An http.HandlerFunc that writes the configuration info as JSON response func GetInfo(systemController system.Controller, version string) func(w http.ResponseWriter, r *http.Request) {internal/api/v2/controllers_accounts_delete_metadata_test.go (1)
74-74: LGTM! Completes the standardization of router initialization.This change maintains the error handling scenarios while aligning with the codebase-wide standardization of router initialization.
Consider documenting this significant change in router initialization, particularly:
- The reason for moving from environment variable to hardcoded version string
- The significance of the "develop" value
- The impact on the GET /_info endpoint mentioned in the PR title
internal/api/router.go (1)
79-79: LGTM! Consider consolidating debug configuration.The change correctly passes the version string to the v2 router. However, since both
versionanddebugparameters are used in different parts of the router, consider consolidating them into a configuration struct for better maintainability.+type RouterConfig struct { + Version string + Debug bool +} -func NewRouter( - systemController system.Controller, - authenticator auth.Authenticator, - version string, - debug bool, - opts ...RouterOption, -) chi.Router { +func NewRouter( + systemController system.Controller, + authenticator auth.Authenticator, + config RouterConfig, + opts ...RouterOption, +) chi.Router {internal/api/v2/controllers_accounts_count_test.go (1)
140-140: Consider using a constant for the version string.While replacing the environment variable with a hardcoded string simplifies the configuration, consider defining a constant for the version string to maintain consistency and make it easier to update across all tests.
+const testVersion = "develop" + func TestAccountsCount(t *testing.T) { // ... - router := NewRouter(systemController, auth.NewNoAuth(), "develop") + router := NewRouter(systemController, auth.NewNoAuth(), testVersion)internal/api/v2/controllers_bulk_test.go (1)
487-487: LGTM! Completes the standardization of router initialization.The change to use "develop" string completes the consistent pattern across all test files.
Consider documenting the significance of the "develop" version string in the router's configuration, particularly how it affects the router's behavior differently from other version strings. This would help future maintainers understand the implications of this value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
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)
✅ Files skipped from review due to trivial changes (1)
- internal/api/v2/controllers_transactions_revert_test.go
🔇 Additional comments (27)
internal/api/v2/controllers_ledgers_update_metadata_test.go (1)
28-28: LGTM: Router initialization updated to use version stringThe change from debug boolean to version string aligns with the router's new signature.
internal/api/v2/controllers_stats_test.go (1)
19-19: LGTM: Consistent router initializationRouter initialization matches the pattern used across other test files.
internal/api/v2/controllers_ledgers_read_test.go (1)
24-24: LGTM: Router initialization standardizedRouter initialization follows the standardized pattern using version string.
internal/api/v2/controllers_ledgers_info_test.go (1)
21-21: LGTM: Router initialization updated for /_info endpoint testRouter initialization follows the standardized pattern. Given this PR addresses a breaking change in the /_info endpoint, let's verify the endpoint compatibility across versions.
Run this script to check for any version-specific handling in the router for the /_info endpoint:
✅ Verification successful
✅ Router initialization verified - no compatibility issues
The router initialization is correct and maintains endpoint compatibility:
- V2 router properly reuses V1's GetInfo handler with version parameter
- Both global "/_info" and "/{ledger}/_info" routes are correctly maintained
- Client-side URL patterns handle version-specific changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check router implementation for /_info endpoint version handling # Search for /_info route definitions echo "Searching for /_info route definitions..." rg -A 5 '_info' --type go # Search for version-specific handling in router echo "Checking for version-specific router handling..." ast-grep --pattern 'func NewRouter($_, $_, $version string) $_ { $$$ }'Length of output: 5122
internal/api/v2/controllers_transactions_read_test.go (1)
39-39: LGTM!The router initialization has been updated to use the new version-based configuration, aligning with the broader refactoring effort.
internal/api/v1/controllers_config.go (1)
35-35: LGTM! Function exported to support v2 router integration.The function rename from
getInfotoGetInfois necessary to make it accessible from the v2 package.internal/api/v2/controllers_ledgers_delete_metadata_test.go (1)
56-56: LGTM!The router initialization has been updated to use the new version-based configuration, maintaining consistency with other test files.
internal/api/v2/controllers_logs_export_test.go (1)
64-64: LGTM!The router initialization has been updated to use the new version-based configuration, maintaining consistency with other test files.
internal/api/v2/controllers_logs_import_test.go (1)
75-75: Verify the impact of hardcoded "develop" mode.The change from environment variable to hardcoded "develop" mode might affect test behavior. Let's verify if this value is used consistently across the codebase.
✅ Verification successful
The hardcoded "develop" mode is correct and consistent.
The change aligns with the v2 API's established pattern, where all v2 test files consistently use
NewRouter(systemController, auth.NewNoAuth(), "develop")without the debug flag parameter.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistency in router initialization across test files # and verify if "develop" is used consistently as the version string # Search for NewRouter calls to ensure consistent usage echo "Checking NewRouter calls:" rg 'NewRouter\([^)]+\)' --type go # Search for any remaining DEBUG environment variable usage echo -e "\nChecking for remaining DEBUG env var usage:" rg 'os\.Getenv\("DEBUG"\)' --type goLength of output: 9183
internal/api/v2/controllers_transactions_delete_metadata_test.go (1)
70-70: LGTM! Consistent with the codebase-wide changes.The modification aligns with the standardization of router initialization across test files.
internal/api/v2/controllers_accounts_add_metadata_test.go (1)
74-74: LGTM! Maintains test coverage while standardizing router initialization.The change is consistent with the codebase-wide standardization and preserves the comprehensive test scenarios.
internal/api/v1/routes.go (1)
30-30: Verify the impact of exportingGetInfo.The change from
getInfotoGetInfomakes the function public. Ensure this change is intentional and verify if any other packages need to access this functionality.Run the following script to check for any external usage of
GetInfo:✅ Verification successful
Export of
GetInfois intentional and necessaryThe function is explicitly used by the v2 API routes to maintain consistency in the info endpoint implementation across API versions. This is a valid architectural decision.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for external usage of GetInfo function # Test: Search for imports of v1 package and usage of GetInfo rg -l "github.com/formancehq/ledger/internal/api/v1" | \ xargs -I {} rg -l "GetInfo" {} || echo "No external usage found"Length of output: 170
Script:
#!/bin/bash # Check the actual usage in v2/routes.go echo "=== Checking v2/routes.go for GetInfo usage ===" rg -A 5 -B 5 "GetInfo" "internal/api/v2/routes.go" echo -e "\n=== Checking GetInfo implementation ===" ast-grep --pattern 'func GetInfo($$$) $$$'Length of output: 7113
internal/api/v2/controllers_transactions_add_metadata_test.go (1)
101-101: LGTM! Test configuration is now deterministic.Replacing environment variable with hardcoded version string makes the test more reliable and predictable.
internal/api/v2/controllers_accounts_read_test.go (1)
95-95: LGTM! Consistent test configuration.The change maintains consistency with other test files by using the hardcoded version string.
internal/api/v2/controllers_balances_test.go (1)
102-102: LGTM! Standardized test configuration.The change aligns with the standardized approach of using a hardcoded version string across test files.
internal/api/v2/controllers_ledgers_list_test.go (1)
97-97: LGTM!The change from environment variable-based debug mode to a hardcoded version string aligns with the broader refactoring effort.
internal/api/v2/controllers_ledgers_create_test.go (1)
102-102: LGTM!The router initialization change is consistent with the broader refactoring pattern across test files.
internal/api/v2/routes.go (3)
6-6: LGTM!The v1 package import is necessary for accessing the GetInfo handler.
23-23: LGTM!The signature change from
debug booltoversion stringaligns with the endpoint requirements.
36-37: Verify the endpoint path consistency.The /_info endpoint is now available in both v1 and v2 routes. Ensure this is the intended behavior and doesn't cause confusion for API consumers.
✅ Verification successful
Endpoint path consistency verified ✓
The /_info endpoint implementation is consistent across v1 and v2, with both versions maintaining the same structure: a root-level endpoint for system information and a ledger-level endpoint for ledger-specific details. The v2 implementation reuses v1's handler, which is a good practice for maintaining consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for /_info endpoint implementations across versions echo "Checking for /_info endpoint implementations:" rg -A 5 '"/_info"' internal/api/Length of output: 3068
internal/api/v2/controllers_transactions_count_test.go (1)
166-166: Same feedback applies regarding version string constant.Consider using a shared constant for the version string across test files.
internal/api/v2/controllers_volumes_test.go (1)
152-152: Same feedback applies regarding version string constant.Consider using a shared constant for the version string across test files.
internal/api/v2/controllers_logs_list_test.go (2)
176-176: Same feedback applies regarding version string constant.Consider using a shared constant for the version string across test files.
176-176: Verify API versioning strategy.Since this PR is fixing a breaking change in GET /_info and modifies how versions are handled in the router, please ensure that:
- The API versioning strategy is documented
- The change maintains backward compatibility with existing clients
- The version string "develop" is appropriate for all test scenarios
✅ Verification successful
API versioning strategy is properly implemented and documented
The verification confirms:
- API versioning uses a clear path-based strategy (/v2 vs /) with separate OpenAPI specs
- Backward compatibility is maintained by keeping v1 endpoints alongside v2
- The version string "develop" is appropriate for tests as it represents the server version, not the API version
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for API versioning documentation and usage # Look for API versioning documentation echo "Checking for API versioning documentation..." rg -l "api.*version|version.*api" -g "*.md" # Look for other version strings in use echo "Checking for other version strings in use..." rg "NewRouter\([^)]+\)" -A 1 -B 1Length of output: 11014
Script:
#!/bin/bash # Check API versioning documentation and implementation # Check CONTRIBUTING.md content around API versioning echo "=== API Versioning Documentation ===" rg -A 5 -B 5 "api.*version|version.*api" CONTRIBUTING.md # Check router implementations echo -e "\n=== Router Implementations ===" fd -e go router.go echo -e "\nV1 Router:" ast-grep --pattern 'func NewRouter($$$)' internal/api/v1/router.go echo -e "\nV2 Router:" ast-grep --pattern 'func NewRouter($$$)' internal/api/v2/router.go # Check version handling in production code echo -e "\n=== Version String Handling ===" rg "version.*=|VERSION.*=" -g "*.go"Length of output: 1118
Script:
#!/bin/bash # Check API versioning implementation and documentation # Check main router implementation echo "=== Main Router Implementation ===" cat internal/api/router.go # Check OpenAPI specs echo -e "\n=== OpenAPI Version Specs ===" fd -e yaml -e json . openapi/ # Look for version string usage echo -e "\n=== Version String Usage ===" rg -A 5 -B 5 '"develop"' internal/api/Length of output: 45301
internal/api/v2/controllers_accounts_list_test.go (1)
220-220: LGTM! Verify consistency of router initialization.The change from environment variable-based debug mode to hardcoded "develop" string aligns with the PR's objective.
Let's verify that this change is consistently applied across all test files:
✅ Verification successful
Router initialization is consistent across v2 API
The change to use hardcoded "develop" without DEBUG environment variable is consistently applied across all v2 API test files, while v1 API maintains its existing pattern for backward compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining os.Getenv("DEBUG") calls in test files # and verify consistent usage of "develop" string in NewRouter calls # Check for any remaining DEBUG environment variable usage rg -l 'os\.Getenv\("DEBUG"\)' 'internal/api/**/test*.go' # Verify consistent usage of "develop" in NewRouter calls rg -l 'NewRouter\([^)]+\)' 'internal/api/**/test*.go' | \ xargs rg 'NewRouter\([^)]+\)'Length of output: 5912
internal/api/v2/controllers_transactions_list_test.go (1)
261-261: LGTM! Router initialization updated consistently.The change to use "develop" string is consistent with the standardization effort across test files.
internal/api/v2/controllers_transactions_create_test.go (1)
418-418: LGTM! Router initialization follows the new pattern.The change maintains consistency with other test files by using the "develop" string.
No description provided.