fix: properly parse includes outside the Configuration plugin #2063
Conversation
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
WalkthroughThe changes in this pull request primarily focus on enhancing the configuration handling for the RPC client. The Changes
Assessment against linked issues
Warning Rate limit exceeded@rustatian has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 56 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. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (2)
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 (
|
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
internal/rpc/includes.go (2)
8-13: Add documentation for version compatibility.Consider adding comments to document:
- The significance of version numbers
- Breaking changes between versions
- Why both versions are maintained
const ( + // versionKey is the key used to identify configuration version in yaml files versionKey string = "version" + // includeKey is the key used to specify additional configuration files to include includeKey string = "include" + // defaultConfigVersion represents the current configuration version defaultConfigVersion string = "3" + // prevConfigVersion represents the last major version for backwards compatibility prevConfigVersion string = "2.7" )
1-62: Consider architectural improvements.To enhance the maintainability and reliability of the configuration system:
Consider implementing a proper logging strategy:
- Add structured logging for configuration operations
- Log important events like file loading and value overrides
- Include correlation IDs for tracking configuration loading
Improve error handling:
- Create specific error types for configuration issues
- Add context to errors for better debugging
- Consider implementing recovery mechanisms
Add comprehensive testing:
- Test cases for circular dependencies
- Test cases for max depth scenarios
- Test cases for various file paths and environment variables
- Test cases for version mismatches
Would you like me to provide example implementations for any of these suggestions?
internal/rpc/client_test.go (1)
55-65: Enhance test robustness and coverageWhile the test validates basic functionality, consider these improvements:
- Use a random available port instead of hardcoding 6010
- Add assertions to verify the included configuration values
- Ensure proper cleanup in case of test failure
Consider this enhanced version:
func TestNewClient_WithIncludes(t *testing.T) { - l, err := net.Listen("tcp", "127.0.0.1:6010") + // Find an available port + l, err := net.Listen("tcp", "127.0.0.1:0") assert.NoError(t, err) + addr := l.Addr().(*net.TCPAddr) + port := addr.Port - defer func() { assert.NoError(t, l.Close()) }() + // Ensure cleanup even if test fails + defer func() { + if err := l.Close(); err != nil { + t.Errorf("failed to close listener: %v", err) + } + }() c, err := rpc.NewClient("test/include1/.rr.yaml", nil) + if err != nil { + t.Fatalf("failed to create client: %v", err) + } + defer func() { + if err := c.Close(); err != nil { + t.Errorf("failed to close client: %v", err) + } + }() assert.NotNil(t, c) assert.NoError(t, err) + + // Verify the included configuration values + // TODO: Add assertions to check if the values from .rr-include.yaml were properly loaded }Additionally, consider adding these test cases:
- Test with nested includes (include file that includes another file)
- Test with circular includes
- Test with non-existent include file
- Test with malformed include file
Would you like me to help implement these additional test cases?
internal/rpc/client.go (1)
48-61: Consider architectural improvements for better maintainability.While the changes address the immediate issue, consider these architectural improvements:
Error Handling:
- Create custom error types for version and include-related errors
- This would help upstream code distinguish between different error cases
Version Management:
- Consider using semantic versioning for better version management
- Add version compatibility matrix in documentation
Configuration Management:
- Consider adding validation for the entire configuration structure
- Add logging for debugging configuration issues
Example implementation of custom errors:
type VersionError struct { Version string Reason string } func (e *VersionError) Error() string { return fmt.Sprintf("invalid version %q: %s", e.Version, e.Reason) } type IncludeError struct { Path string Reason string } func (e *IncludeError) Error() string { return fmt.Sprintf("failed to include %q: %s", e.Path, e.Reason) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
internal/rpc/client.go(1 hunks)internal/rpc/client_test.go(1 hunks)internal/rpc/includes.go(1 hunks)internal/rpc/test/include1/.rr-include.yaml(1 hunks)internal/rpc/test/include1/.rr.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- internal/rpc/test/include1/.rr-include.yaml
- internal/rpc/test/include1/.rr.yaml
🔇 Additional comments (1)
internal/rpc/client.go (1)
57-60: Verify include handling implementation.
The error handling for includes looks good, but we need to verify if this implementation properly addresses the original issue (#2017) where RPC plugin configuration fails when included from another file.
Let's verify the include handling implementation:
✅ Verification successful
Let's gather more information about the include handling implementation and test cases.
Let's check the included configuration file and the original issue to complete our verification.
Let's examine the includes.go file to understand the complete implementation.
Based on the implementation details we've gathered, I can now provide the final response:
Include handling implementation properly addresses the issue.
The implementation in includes.go correctly handles configuration includes by:
- Loading and validating included files
- Ensuring version consistency between root and included configs
- Properly merging configurations by overriding root values with included ones using
v.Set(key, val)
The test case TestNewClient_WithIncludes confirms this works with RPC configuration, and the included test configuration .rr-include.yaml demonstrates the correct structure for RPC settings.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the handleInclude implementation and its usage
# Search for handleInclude implementation
echo "Searching for handleInclude implementation:"
ast-grep --pattern 'func handleInclude($version string, $v *viper.Viper) error {
$$$
}'
# Search for test cases involving includes
echo -e "\nSearching for include-related test cases:"
rg -A 5 'TestNewClient.*Include'
# Search for example YAML configurations with includes
echo -e "\nSearching for YAML configurations with includes:"
rg -t yaml '(include|imports?):'
Length of output: 853
Script:
#!/bin/bash
# Check the implementation details and test configurations
# Get the content of the test file to understand test coverage
echo "Content of client_test.go with include tests:"
cat internal/rpc/client_test.go
# Check the content of include test configuration
echo -e "\nContent of include test configuration:"
cat internal/rpc/test/include1/.rr.yaml
# Search for handleInclude implementation in client.go
echo -e "\nContent of client.go around handleInclude:"
rg -B 5 -A 15 "func handleInclude"
Length of output: 3984
Script:
#!/bin/bash
# Check the included configuration and the original issue
# Get the content of the included configuration
echo "Content of included configuration:"
cat internal/rpc/test/include1/.rr-include.yaml
# Check the original issue
echo -e "\nChecking issue #2017:"
gh issue view 2017
# Check if there are any other files related to includes
echo -e "\nSearching for other include-related files:"
fd -t f "include" internal/rpc/
Length of output: 1537
Script:
#!/bin/bash
# Check the complete implementation of includes handling
# Get the content of includes.go
echo "Content of includes.go:"
cat internal/rpc/includes.go
# Search for any merge-related code in client.go
echo -e "\nSearching for config merging in client.go:"
rg -B 5 -A 10 "MergeConfig|MergeConfigMap" internal/rpc/client.go
# Check if there are any other files that might handle config merging
echo -e "\nSearching for config merging in other files:"
rg -l "MergeConfig|MergeConfigMap" internal/rpc/
Length of output: 1740
| func getConfiguration(path string) (map[string]any, string, error) { | ||
| v := viper.New() | ||
| v.SetConfigFile(path) | ||
| err := v.ReadInConfig() | ||
| if err != nil { | ||
| return nil, "", err | ||
| } | ||
|
|
||
| // get configuration version | ||
| ver := v.Get(versionKey) | ||
| if ver == nil { | ||
| return nil, "", errors.Str("rr configuration file should contain a version e.g: version: 2.7") | ||
| } | ||
|
|
||
| if _, ok := ver.(string); !ok { | ||
| return nil, "", errors.Errorf("type of version should be string, actual: %T", ver) | ||
| } | ||
|
|
||
| // automatically inject ENV variables using ${ENV} pattern | ||
| expandEnvViper(v) | ||
|
|
||
| return v.AllSettings(), ver.(string), nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and improve error handling.
Several improvements could make this function more robust:
- The path parameter should be validated before use
- Version validation could be more comprehensive
- The type assertion in the return statement could panic
Consider applying these improvements:
func getConfiguration(path string) (map[string]any, string, error) {
+ if path == "" {
+ return nil, "", errors.Str("configuration path cannot be empty")
+ }
+
v := viper.New()
v.SetConfigFile(path)
err := v.ReadInConfig()
if err != nil {
return nil, "", err
}
// get configuration version
ver := v.Get(versionKey)
if ver == nil {
return nil, "", errors.Str("rr configuration file should contain a version e.g: version: 2.7")
}
if _, ok := ver.(string); !ok {
return nil, "", errors.Errorf("type of version should be string, actual: %T", ver)
}
+ version := ver.(string)
+ if version != defaultConfigVersion && version != prevConfigVersion {
+ return nil, "", errors.Errorf("unsupported configuration version: %s", version)
+ }
// automatically inject ENV variables using ${ENV} pattern
expandEnvViper(v)
- return v.AllSettings(), ver.(string), nil
+ return v.AllSettings(), version, nil
}Committable suggestion skipped: line range outside the PR's diff.
internal/rpc/includes.go
Outdated
| func handleInclude(rootVersion string, v *viper.Viper) error { | ||
| ifiles := v.GetStringSlice(includeKey) | ||
| if ifiles == nil { | ||
| return nil | ||
| } | ||
|
|
||
| for _, file := range ifiles { | ||
| config, version, err := getConfiguration(file) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if version != rootVersion { | ||
| return errors.Str("version in included file must be the same as in root") | ||
| } | ||
|
|
||
| // overriding configuration | ||
| for key, val := range config { | ||
| v.Set(key, val) | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Add safety checks for configuration includes.
The current implementation has several potential issues:
- No cycle detection for recursive includes
- No depth limit for nested includes
- No validation of included file paths
- Blind overriding of configuration values might cause issues
Consider implementing these safety measures:
+const maxIncludeDepth = 10
+
+func handleInclude(rootVersion string, v *viper.Viper) error {
+ return handleIncludeWithDepth(rootVersion, v, 0, make(map[string]struct{}))
+}
+
+func handleIncludeWithDepth(rootVersion string, v *viper.Viper, depth int, visited map[string]struct{}) error {
+ if depth >= maxIncludeDepth {
+ return errors.Str("maximum include depth exceeded")
+ }
+
ifiles := v.GetStringSlice(includeKey)
if ifiles == nil {
return nil
}
for _, file := range ifiles {
+ // Check for cycles
+ if _, exists := visited[file]; exists {
+ return errors.Errorf("circular include detected: %s", file)
+ }
+ visited[file] = struct{}{}
+
config, version, err := getConfiguration(file)
if err != nil {
return err
}
if version != rootVersion {
return errors.Str("version in included file must be the same as in root")
}
// overriding configuration
for key, val := range config {
+ // Skip special keys
+ if key == versionKey || key == includeKey {
+ continue
+ }
+ // Log when overriding existing values
+ if v.IsSet(key) {
+ // TODO: Use proper logger
+ fmt.Printf("warning: overriding existing config key: %s\n", key)
+ }
v.Set(key, val)
}
+
+ // Process nested includes
+ if err := handleIncludeWithDepth(rootVersion, v, depth+1, visited); err != nil {
+ return err
+ }
}
return nil
}📝 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 handleInclude(rootVersion string, v *viper.Viper) error { | |
| ifiles := v.GetStringSlice(includeKey) | |
| if ifiles == nil { | |
| return nil | |
| } | |
| for _, file := range ifiles { | |
| config, version, err := getConfiguration(file) | |
| if err != nil { | |
| return err | |
| } | |
| if version != rootVersion { | |
| return errors.Str("version in included file must be the same as in root") | |
| } | |
| // overriding configuration | |
| for key, val := range config { | |
| v.Set(key, val) | |
| } | |
| } | |
| return nil | |
| } | |
| const maxIncludeDepth = 10 | |
| func handleInclude(rootVersion string, v *viper.Viper) error { | |
| return handleIncludeWithDepth(rootVersion, v, 0, make(map[string]struct{})) | |
| } | |
| func handleIncludeWithDepth(rootVersion string, v *viper.Viper, depth int, visited map[string]struct{}) error { | |
| if depth >= maxIncludeDepth { | |
| return errors.Str("maximum include depth exceeded") | |
| } | |
| ifiles := v.GetStringSlice(includeKey) | |
| if ifiles == nil { | |
| return nil | |
| } | |
| for _, file := range ifiles { | |
| // Check for cycles | |
| if _, exists := visited[file]; exists { | |
| return errors.Errorf("circular include detected: %s", file) | |
| } | |
| visited[file] = struct{}{} | |
| config, version, err := getConfiguration(file) | |
| if err != nil { | |
| return err | |
| } | |
| if version != rootVersion { | |
| return errors.Str("version in included file must be the same as in root") | |
| } | |
| // overriding configuration | |
| for key, val := range config { | |
| // Skip special keys | |
| if key == versionKey || key == includeKey { | |
| continue | |
| } | |
| // Log when overriding existing values | |
| if v.IsSet(key) { | |
| // TODO: Use proper logger | |
| fmt.Printf("warning: overriding existing config key: %s\n", key) | |
| } | |
| v.Set(key, val) | |
| } | |
| // Process nested includes | |
| if err := handleIncludeWithDepth(rootVersion, v, depth+1, visited); err != nil { | |
| return err | |
| } | |
| } | |
| return nil | |
| } |
| c, err := rpc.NewClient("test/include1/.rr.yaml", nil) | ||
|
|
||
| assert.NotNil(t, c) | ||
| assert.NoError(t, err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add explicit validation of RPC configuration parsing
To better align with the PR objectives (fixing RPC plugin configuration includes), consider adding explicit validation of the parsed RPC configuration values.
Add assertions to verify that the RPC configuration was correctly parsed from the included file:
c, err := rpc.NewClient("test/include1/.rr.yaml", nil)
assert.NotNil(t, c)
assert.NoError(t, err)
+
+ // Verify that the RPC configuration was correctly parsed from the included file
+ cfg := c.Config() // Assuming there's a method to get the parsed config
+ assert.Equal(t, "tcp://127.0.0.1:6010", cfg.Listen, "RPC listen address should be loaded from included file")Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| defer func() { assert.NoError(t, l.Close()) }() | ||
|
|
||
| c, err := rpc.NewClient("test/include1/.rr.yaml", nil) |
There was a problem hiding this comment.
💡 Codebase verification
Missing required test configuration files
The test references a configuration file at test/include1/.rr.yaml, but this path doesn't exist in the repository. The only RPC-related configuration file found is at internal/rpc/test/config_rpc_conn_err.yaml, and the test directory structure is different from what's expected in the test.
internal/rpc/client_test.go: Update the test to use the correct path to the configuration file, likely should beinternal/rpc/test/instead oftest/include1/
🔗 Analysis chain
Verify the referenced configuration files
Let's ensure the required configuration files exist and contain the expected structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the existence and content of configuration files
echo "Checking for configuration files..."
fd -t f ".rr.yaml|.rr-include.yaml" test/include1/
echo -e "\nChecking file contents..."
for f in $(fd -t f ".rr.yaml|.rr-include.yaml" test/include1/); do
echo -e "\nContents of $f:"
cat "$f"
done
Length of output: 458
Script:
#!/bin/bash
# Let's first check if the test directory exists and locate any .rr.yaml files
echo "Checking for test directory and configuration files..."
fd -t d "test"
echo -e "\nSearching for .rr.yaml files in the entire repository..."
fd -t f ".rr.yaml"
echo -e "\nLooking for references to test/include1 in the codebase..."
rg "test/include1" -l
Length of output: 542
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Reason for This PR
closes: #2017
ping: @Kaspiman
Description of Changes
License Acceptance
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
New Features
Bug Fixes
Tests
NewClientmethod to validate configuration file handling.Documentation