feat: make strict variables parsing #23
Conversation
WalkthroughThis pull request refactors the handling of variable types across multiple components by replacing generic maps ( Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Variables
Note over Caller,Variables: JSON Unmarshalling
Caller->>Variables: UnmarshalJSON(data)
Variables->>Variables: Parse JSON into temp map
Variables->>Variables: Validate value types (string, float64, bool, nil)
Variables-->>Caller: Return parsed Variables or error
Note over Caller,Variables: YAML Unmarshalling
Caller->>Variables: UnmarshalYAML(yamlNode)
Variables->>Variables: Convert YAML to JSON
Variables->>Variables: Call UnmarshalJSON(convertedData)
Variables-->>Caller: Return parsed Variables or error
Possibly related PRs
✨ Finishing Touches
🪧 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
Documentation and Community
|
![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
🧹 Nitpick comments (4)
pkg/variables/variables.go (2)
14-32: Strict validation for variable types in UnmarshalJSONThe implementation correctly limits variable values to primitive types (string, float64, bool, nil) and rejects complex types like arrays and objects. This aligns with the PR objective of making variable parsing more strict.
Consider enhancing the error message to include the actual type that was rejected, not just the key name, to make debugging easier.
- return fmt.Errorf("%w: key %s", errorsx.ErrUnsupportedVariablesValue, key) + return fmt.Errorf("%w: key %s has unsupported type %T", errorsx.ErrUnsupportedVariablesValue, key, value)
34-46: UnmarshalYAML implementation via JSON conversionThe approach of converting YAML to JSON before using UnmarshalJSON ensures consistent validation, but introduces an extra marshalling step. This is a reasonable trade-off for code simplicity and consistency.
Consider adding comments explaining why this approach was chosen over direct YAML validation.
func (v *Variables) UnmarshalYAML(value *yaml.Node) error { + // Convert YAML to JSON to reuse the same type validation logic in UnmarshalJSON var raw any if err := value.Decode(&raw); err != nil { return err }pkg/variables/variables_test.go (2)
10-65: Test cases for UnmarshalJSON cover relevant scenariosThe test cases appropriately verify that:
- Primitive types (boolean, integer, float, string) are accepted
- Complex types (lists, nested objects) are rejected with appropriate error messages
Consider adding a verification of the actual content of the Variables map after successful unmarshalling to ensure the data is preserved correctly.
} else { assert.NoError(t, err) + // Verify the content of the parsed variables + switch tt.name { + case "boolean": + assert.Equal(t, true, v["boolean"]) + case "integer": + assert.Equal(t, float64(3), v["integer"]) + case "float": + assert.Equal(t, 3.21, v["float"]) + case "string": + assert.Equal(t, "hello, world", v["string"]) + } }
124-131: Helper function for YAML node creationThe
yamlNodefunction simplifies test setup, but it panics on unmarshalling errors which could make test failures harder to debug.Consider handling errors more gracefully in this test helper.
func yamlNode(s string) *yaml.Node { var node yaml.Node if err := yaml.Unmarshal([]byte(s), &node); err != nil { - panic(err) + // In a test helper, we can use t.Fatalf, but since we don't have access to t here, + // we'll return a nil node, which will cause the test to fail with a clear error. + return nil } return node.Content[0] }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cmd/root.go(2 hunks)cmd/run.go(2 hunks)internal/errorsx/errorsx.go(1 hunks)main.go(1 hunks)pkg/param/param.go(1 hunks)pkg/request/template.go(2 hunks)pkg/variables/variables.go(1 hunks)pkg/variables/variables_test.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (7)
main.go (1)
cmd/root.go (1)
Execute(74-76)
pkg/request/template.go (2)
pkg/variables/variables.go (1)
Variables(12-12)cmd/root.go (1)
Variables(53-53)
pkg/variables/variables_test.go (3)
pkg/variables/variables.go (1)
Variables(12-12)cmd/root.go (1)
Variables(53-53)internal/assert/assert.go (2)
ErrorContains(49-55)NoError(41-47)
pkg/param/param.go (2)
pkg/variables/variables.go (1)
Variables(12-12)cmd/root.go (1)
Variables(53-53)
cmd/run.go (2)
pkg/variables/variables.go (1)
Variables(12-12)cmd/root.go (1)
Variables(53-53)
cmd/root.go (1)
pkg/variables/variables.go (1)
Variables(12-12)
pkg/variables/variables.go (2)
cmd/root.go (1)
Variables(53-53)internal/errorsx/errorsx.go (1)
ErrUnsupportedVariablesValue(13-13)
🔇 Additional comments (13)
main.go (1)
8-9:❓ Verification inconclusive
Ensure error handling is sufficient for strict variable parsing
The change delegates error handling to the Cobra library instead of explicitly handling it in
main.go. While this simplifies the code, make sure Cobra's error handling behavior is appropriate for your use case, especially considering the focus on stricter variable parsing in this PR.I recommend running the following script to verify how Cobra handles errors and if it properly displays variable parsing errors to the user:
🏁 Script executed:
#!/bin/bash # Verify how Cobra handles errors in the application # Find examples of how Execute() is used in the codebase rg -A 5 "Execute\(\)" --type go # Check for error handling patterns in Cobra initialization rg "cobra\.Command" -A 10 -B 10 --type go | grep -E "Error|err" # Look for Cobra error handling documentation references cat README.md | grep -i "error" || echo "No error handling documentation in README"Length of output: 780
Ensure Cobra's Error Handling Covers Strict Variable Parsing
After verifying the codebase:
- The output confirms that error handling for strict variable parsing is primarily managed in
cmd/root.govia callbacks likeprepareVariables()andprepareHeaders().- In
main.go, delegating error handling using_ = cmd.Execute()is acceptable given that Cobra's internal mechanisms catch and propagate errors.- Please ensure that errors from strict variable parsing are presented clearly to the user. If custom error output is required for better clarity, consider enhancing the error logging or message formatting in the Cobra command callbacks.
internal/errorsx/errorsx.go (1)
13-13: LGTM: New error type for unsupported variablesThe addition of
ErrUnsupportedVariablesValuealigns with the PR objective of implementing stricter variable parsing. This error will be useful for providing clear error messages when users attempt to use unsupported variable types with the newvariables.Variablesstructure.pkg/param/param.go (2)
7-8: LGTM: Clean import additionThe import of the variables package is correctly added with a blank line for separation from the standard library imports, maintaining good code organization.
13-13:✅ Verification successful
LGTM: Updated parameter type to use the new Variables type
Changing the parameter type from
map[string]anytovariables.Variablesis consistent with the PR's objective of implementing stricter variable parsing. Since the function body treats the parameter as a map with string keys, this change should maintain the existing functionality while improving type safety.Let's verify that the
Variablestype is used consistently throughout the codebase:
🏁 Script executed:
#!/bin/bash # Check for consistent usage of variables.Variables type # Find all occurrences of variables.Variables in type declarations echo "Searching for variables.Variables usage in type declarations..." rg "variables\.Variables" --type go # Find all occurrences of map[string]any which might need to be updated echo "Searching for potential map[string]any that might need updating..." rg "map\[string\]any" --type go # Check the definition of variables.Variables echo "Checking the definition of variables.Variables..." rg "type Variables" --type goLength of output: 2209
LGTM: Verified consistent usage of
variables.Variablesacross the codebase.
- The function signatures in both
pkg/param/param.goandpkg/request/template.go, as well as other parts of the project, correctly usevariables.Variables.- The type definition (
type Variables map[string]anyinpkg/variables/variables.go) confirms that this alias encapsulates the expected behavior while enhancing type clarity.- Occasional references to
map[string]anyin tests (e.g., inpkg/param/param_test.goand elsewhere) are acceptable since they directly construct test data, relying on the underlying definition ofvariables.Variables.No further changes are required.
cmd/root.go (2)
16-16: LGTM: Added import for variables packageThe import of the variables package is correctly added, allowing the use of the specialized
variables.Variablestype.
53-53:❓ Verification inconclusive
LGTM: Updated Variables declaration to use the new type
Changing the
Variablesdeclaration from a genericmap[string]any{}tovariables.Variablesaligns with the PR objective of implementing stricter variable parsing. This provides better type safety and makes the code more maintainable.Let's verify that
Variablesis properly initialized:
🏁 Script executed:
#!/bin/bash # Check if Variables is properly initialized # Look for Variables initialization echo "Searching for Variables initialization..." rg "Variables\s*=" --type go # Check if there are any places where Variables needs to be initialized echo "Checking if there are any functions that might need to initialize Variables..." rg "func.*init" -A 15 --type go | grep -A 10 -B 10 "Variables" # Check the definition of variables.Variables constructor if any echo "Checking for any constructor methods for variables.Variables..." rg "func New.*Variables" --type goLength of output: 1398
Verification: Ensure Proper Initialization of
variables.VariablesThe updated declaration in
cmd/root.go(line 53) successfully changesVariablesfrom a generic map to the strictervariables.Variablestype. This aligns with the PR objective of enforcing better type safety. However, our search did not reveal any explicit initialization (e.g. an assignment or a constructor call) for this field. It appears that the code relies on its zero-value (or deferred initialization) behavior. Please manually verify that this implicit initialization is intentional and that any subsequent usage ofVariablescorrectly handles its uninitialized state if it’s a map or similar container.
- File:
cmd/root.go
- Line: 53 – Declaration updated to
variables.Variables- Observations:
- No explicit assignment (i.e.
Variables = ...) was found.- No constructor like
New…Variablesis being used.- The
init()function sets flags for related input variables (e.g.VariablesInput) but does not explicitly initialize theVariablesfield.- Action: Manually verify that the intended zero-value (or deferred initialization) of
variables.Variablesis correct per the design of the PR.pkg/request/template.go (2)
12-12: Added import for the new variables packageThe new import is consistent with the updated function signature in line 32, where generic map types are being replaced with
variables.Variablesfor stricter type validation.
32-32: LGTM: Updated parameter type to use the custom Variables typeThis change replaces
map[string]anywithvariables.Variables, improving type safety by restricting allowed variable types. This is consistent with the PR objective of making variable parsing more strict.pkg/variables/variables.go (1)
12-12: New Variables type definition as map aliasThis type alias forms the foundation for adding custom behavior to variable maps while maintaining compatibility with existing code that expects a map structure.
cmd/run.go (3)
13-13: Added import for variables packageConsistent with the function signature changes in the file.
18-18: Updated variables parameter type in runCommandFunctionChanged from
map[string]anytovariables.Variables, consistent with the PR objective of standardizing on the new type throughout the codebase.
47-47: Updated variables parameter type in runRequestChanged from
map[string]anytovariables.Variables, consistent with the PR objective of standardizing on the new type throughout the codebase.pkg/variables/variables_test.go (1)
67-122: Test cases for UnmarshalYAML mirror JSON testsGood practice to have equivalent test cases for both JSON and YAML unmarshalling, ensuring consistent behavior regardless of input format.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
pkg/variables/variables_test.go (5)
10-51: Consider expanding test coverage for JSON unmarshalling.The test cases for JSON unmarshalling cover basic primitive types and correctly verify that unsupported types like lists and nested objects are rejected. However, you might want to consider a few additional test cases:
- Empty objects (
{})- Null values (
{"key": null})- Multiple fields in a single object
- Very large numbers that might cause overflow
// Additional test cases to consider { name: "empty object", data: []byte(`{}`), wantErr: false, }, { name: "null value", data: []byte(`{"null": null}`), wantErr: false, // or true depending on your requirements }, { name: "multiple fields", data: []byte(`{"a": 1, "b": "hello", "c": true}`), wantErr: false, }
53-65: Consider verifying the actual parsed values.The tests correctly verify error conditions, but they don't check that the actual values were parsed correctly in the success cases. Consider adding assertions to verify that the parsed values match the expected values.
// Example addition to success case testing if !tt.wantErr { assert.NoError(t, err) // Verify the parsed values switch tt.name { case "boolean": assert.Equal(t, true, v["boolean"]) case "integer": assert.Equal(t, float64(3), v["integer"]) // JSON numbers are parsed as float64 // Add cases for other types } }
110-122: Consider verifying the actual parsed values for YAML as well.Similar to the JSON tests, the YAML tests could benefit from assertions that verify the actual parsed values in addition to checking for errors.
// Example addition to success case testing if !tt.wantErr { assert.NoError(t, err) // Verify the parsed values switch tt.name { case "boolean": assert.Equal(t, true, v["boolean"]) case "integer": assert.Equal(t, int(3), v["integer"]) // YAML numbers might be parsed differently // Add cases for other types } }
124-131: Improve error handling in the yamlNode helper function.The
yamlNodefunction silently returns nil if YAML unmarshalling fails, which could make test failures harder to debug. Consider adding a panic or returning the error to make potential issues more visible.-func yamlNode(s string) *yaml.Node { +func yamlNode(s string) *yaml.Node { var node yaml.Node if err := yaml.Unmarshal([]byte(s), &node); err != nil { - return nil + panic(fmt.Sprintf("failed to unmarshal YAML: %v", err)) } return node.Content[0] }
10-131: Document the strict type restrictions for Variables.The tests show that the
Variablestype only supports primitive types (boolean, integer, float, string) and rejects lists and nested objects. This important constraint should be documented in the implementation file and possibly in these tests.Consider adding a comment at the beginning of each test function to explain the purpose and restrictions:
// TestVariables_UnmarshalJSON tests that the Variables type correctly unmarshals // JSON data, accepting only primitive types (boolean, integer, float, string) // and rejecting lists and nested objects. func TestVariables_UnmarshalJSON(t *testing.T) { // ... } // TestVariables_UnmarshalYAML tests that the Variables type correctly unmarshals // YAML data, accepting only primitive types (boolean, integer, float, string) // and rejecting lists and nested objects. func TestVariables_UnmarshalYAML(t *testing.T) { // ... }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/variables/variables.go(1 hunks)pkg/variables/variables_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/variables/variables.go
🧰 Additional context used
🧬 Code Definitions (1)
pkg/variables/variables_test.go (2)
pkg/variables/variables.go (1)
Variables(12-12)internal/assert/assert.go (2)
ErrorContains(49-55)NoError(41-47)
🔇 Additional comments (2)
pkg/variables/variables_test.go (2)
1-8: Good package organization and imports.The package declaration and imports look clean and organized, with appropriate use of the assert package from the internal directory and the yaml.v3 package with an alias.
67-108: Consistent test structure for YAML unmarshalling.The test structure for YAML unmarshalling mirrors the JSON tests, which is good for consistency. The same suggestions for additional test cases apply here as well.
Summary by CodeRabbit