fix: set defaults during validation to prevent override zero values #628
Conversation
WalkthroughThe changes in this pull request focus on enhancing the validation logic across multiple test files and the core Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (11)
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: 6
🧹 Outside diff range and nitpick comments (6)
adapters/humago/humago_test.go (2)
Line range hint
132-139: Optimize request body handling and improve error handling
- The code performs double JSON unmarshaling which could impact performance in the benchmark.
- Error handling uses panic which is not ideal for a production environment.
Consider combining the validation and unmarshaling steps:
- var tmp any - if err := json.Unmarshal(data, &tmp); err != nil { - panic(err) - } - - huma.ValidateAndSetDefaults(registry, schema, pb, huma.ModeWriteToServer, tmp, res) - if len(res.Errors) > 0 { - panic(res.Errors) - } - - var input GreetingInput - if err := json.Unmarshal(data, &input); err != nil { - panic(err) - } + var input GreetingInput + if err := json.Unmarshal(data, &input); err != nil { + res.Errors = append(res.Errors, &huma.ValidationError{ + Path: "", + Code: "json", + Message: "invalid JSON format", + }) + return + } + + huma.ValidateAndSetDefaults(registry, schema, pb, huma.ModeWriteToServer, input, res) + if len(res.Errors) > 0 { + w.WriteHeader(http.StatusBadRequest) + json.NewEncoder(w).Encode(res.Errors) + return + }
Line range hint
112-135: Consider benchmark implications of validation changesWhile the changes to use
ValidateAndSetDefaultsare correct, note that this might affect benchmark results as it performs additional operations compared to the previousValidatecalls. Consider adding comments in the benchmark results to highlight this difference.adapters/humachi/humachi_test.go (1)
Line range hint
213-217: Consider consistent error handling approach.While the validation error handling is appropriate, it differs from the error handling approach used for other operations (direct panic vs. panic with errors). Consider standardizing the error handling approach across all validations.
Apply this diff to make the error handling consistent:
huma.ValidateAndSetDefaults(registry, schema, pb, huma.ModeWriteToServer, tmp, res) if len(res.Errors) > 0 { - panic(res.Errors) + panic(err) }schema_test.go (1)
Line range hint
1187-1457: Consider adding test cases for zero values.Given that the PR's objective is specifically about preventing override of zero values during validation, it would be beneficial to add test cases that verify this behavior.
Consider adding test cases like:
func TestZeroValueDefaults(t *testing.T) { type TestStruct struct { IntValue int `json:"int_value" default:"42"` FloatValue float64 `json:"float_value" default:"3.14"` BoolValue bool `json:"bool_value" default:"true"` } cases := []struct { name string input map[string]any expected TestStruct }{ { name: "explicit zeros should not be overridden", input: map[string]any{ "int_value": 0, "float_value": 0.0, "bool_value": false, }, expected: TestStruct{ IntValue: 0, FloatValue: 0.0, BoolValue: false, }, }, } // ... test implementation }validate.go (1)
354-354: Consider adding unit tests forValidateAndSetDefaultsTo ensure the new functionality of setting default values during validation works as expected, consider adding unit tests for
ValidateAndSetDefaults.Would you like assistance in creating test cases for this function?
huma.go (1)
1322-1324: Remove unnecessary commented-out codeThe
ifblock at line 1322 is empty due to the commented-out line. To enhance code readability, consider removing the entireifblock.Apply this diff to remove the unused code:
- if item.IsZero() { - // item.Set(reflect.Indirect(reflect.ValueOf(def))) - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- adapters/humabunrouter/humabunrouter_test.go (2 hunks)
- adapters/humachi/humachi_test.go (2 hunks)
- adapters/humaecho/humaecho_test.go (2 hunks)
- adapters/humago/humago_test.go (2 hunks)
- huma.go (3 hunks)
- huma_test.go (1 hunks)
- schema_test.go (3 hunks)
- validate.go (13 hunks)
- validate_test.go (5 hunks)
🧰 Additional context used
🪛 golangci-lint
validate.go
338-338:
ommitedis a misspelling ofomitted(misspell)
🔇 Additional comments (21)
adapters/humago/humago_test.go (1)
112-112: LGTM: Path parameter validation updated correctlyThe change to
ValidateAndSetDefaultsfor the path parameter is appropriate and aligns with the PR's objective.adapters/humaecho/humaecho_test.go (4)
115-115: LGTM! The change aligns with common header validation practices.Using
ValidateAndSetDefaultsfor headers is appropriate as they commonly have default values in OpenAPI specifications.
135-138: LGTM! Proper validation of request body with defaults.The implementation correctly validates the raw unmarshaled data before converting to the typed struct, allowing proper handling of default values. This aligns well with the PR objective of preventing override of zero values.
Line range hint
1-238: Consider consistent validation approach across benchmarks.The file contains three benchmark functions (
BenchmarkHumaEcho,BenchmarkRawEcho, andBenchmarkRawEchoFast), but onlyBenchmarkRawEchois updated to useValidateAndSetDefaults. Consider updating the validation approach consistently across all benchmark functions to ensure comparable results.#!/bin/bash # Description: Check for consistency in validation approach across benchmark files # Search for benchmark functions using Validate rg -U 'func Benchmark.*\n[\s\S]*?huma\.Validate\(' # Search for benchmark functions using ValidateAndSetDefaults rg -U 'func Benchmark.*\n[\s\S]*?huma\.ValidateAndSetDefaults\('
112-112: Verify the necessity of setting defaults for path parameters.Since path parameters are required by nature in the Echo framework and cannot be optional, using
ValidateAndSetDefaultshere might be unnecessary. Consider whetherValidatewould be more appropriate for path parameters.adapters/humabunrouter/humabunrouter_test.go (3)
193-193: LGTM: Proper validation for path parameter.The change to
ValidateAndSetDefaultsis appropriate for the path parameter validation.
216-219: LGTM: Proper validation sequence for request body.The validation is correctly performed on the raw JSON data before type-specific unmarshaling, allowing proper default value handling and validation.
196-196: Verify default Content-Type handling.The change to
ValidateAndSetDefaultsis appropriate. Since Content-Type is an optional header, verify that the default value handling works as expected when the header is missing.✅ Verification successful
Based on the gathered information, I can now provide the final response:
Content-Type validation is properly implemented with default handling
The validation using
ValidateAndSetDefaultsfor Content-Type is correct and includes proper default value handling. The code validates the Content-Type header inModeReadFromServermode, which is appropriate for server-side validation, and the framework includes built-in Content-Type handling with defaults (e.g., "application/json" for JSON format). No issues were found with the implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are any default Content-Type definitions in the schema # or configuration that would be applied during validation # Look for Content-Type related schema definitions or defaults rg -i "content.?type.*default" --type goLength of output: 294
Script:
#!/bin/bash # Let's examine the actual implementation and usage context # Check the schema definition and validation logic for Content-Type rg -A 5 -B 5 "contentType.*default" --type go # Look for Content-Type related test cases ast-grep --pattern 'func Test$_($_, $_) { $$$ "Content-Type" $$$ }' # Check for any Content-Type related constants or defaults rg -A 2 "const.*ContentType" --type goLength of output: 739
Script:
#!/bin/bash # Let's check the specific validation logic in the humabunrouter package # Look for Content-Type header handling in the test file rg -A 5 -B 5 "Header.*Content-Type" adapters/humabunrouter/humabunrouter_test.go # Check the schema validation implementation ast-grep --pattern 'ValidateAndSetDefaults($$$)' # Look for default Content-Type handling in the main implementation rg -A 5 -B 5 "Content-Type" adapters/humabunrouter/humabunrouter.goLength of output: 3844
Script:
#!/bin/bash # Let's check the schema validation implementation details # Look for string schema definition rg -A 10 "strSchema.*:=" adapters/humabunrouter/humabunrouter_test.go # Check the validation modes and their handling rg -A 5 "ModeReadFromServer" --type go # Look for any default value handling in the schema validation rg -A 5 "Default.*string" --type goLength of output: 12526
adapters/humachi/humachi_test.go (1)
190-199: Add test cases for zero value handling.The changes to use
ValidateAndSetDefaultsalign with the PR objective of preventing zero value overrides. However, the current benchmark test doesn't explicitly verify this behavior.Consider adding test cases that verify:
- Zero values are preserved during validation
- Default values are correctly set when parameters are omitted
schema_test.go (3)
1187-1187: LGTM: Consistent with PR objectives.The change from
ValidatetoValidateAndSetDefaultsaligns with the PR's goal of properly handling default values during validation.
1415-1423: LGTM: Benchmark updated consistently.The benchmark has been correctly updated to use
ValidateAndSetDefaults, maintaining test coverage for the new validation behavior.
1449-1457: LGTM: Error handling benchmark updated.The error handling benchmark has been properly updated to use
ValidateAndSetDefaults, ensuring that error cases are still properly tested.huma_test.go (1)
645-679: LGTM! Well-structured test case for validating default value behavior.The test case effectively verifies that default values do not override explicitly provided values, particularly for boolean fields where
falseis a valid value. The test coverage is comprehensive, including:
- Primitive types (string, int)
- Slice types (strings, ints)
- Struct fields within slices (verified, id)
validate.go (2)
700-702:⚠️ Potential issueProperly check for missing keys in map before setting defaults
In
handleMapAny, the conditionm[k] == nilmay not correctly identify keys that are absent from the map. To ensure default values are set for omitted fields, use the_, ok := m[k]syntax to check for the key's existence.Apply this diff:
-if m[k] == nil && v.Default != nil { +if _, ok := m[k]; !ok && v.Default != nil { m[k] = v.Default }Likely invalid or redundant comment.
601-602:⚠️ Potential issueProperly check for missing keys in map before setting defaults
When setting default values for missing properties in a map, using
m[k] == nildoes not distinguish between an absent key and a key with anilvalue. To accurately identify missing keys, use the_, ok := m[k]syntax.Apply this diff to fix the issue:
-if m[k] == nil && v.Default != nil { +if _, ok := m[k]; !ok && v.Default != nil { m[k] = v.Default }Likely invalid or redundant comment.
validate_test.go (5)
1391-1391: Verify tests handle defaults correctly after changing toValidateAndSetDefaultsBy replacing
huma.Validatewithhuma.ValidateAndSetDefaultsat line 1391, default values are now being set during validation. Ensure that all test cases in theTestValidatefunction correctly account for default values being applied, and update any assertions if necessary.
1426-1426: Ensure custom error formatting works with defaultsIn
TestValidateCustomFormatter, changing tohuma.ValidateAndSetDefaultsmay affect how default values influence error messages. Verify that the custom error formatter still produces the expected output when defaults are set during validation.
1449-1449: Check schema transformations with defaults appliedFor
TestValidateSchemaTransformerDeleteField, replacinghuma.Validatewithhuma.ValidateAndSetDefaultsmeans defaults are now set during validation. Confirm that the schema transformation correctly handles default values and that deleted fields do not introduce unexpected behavior.
1518-1518: Assess performance impact of setting defaults in benchmarksIn
BenchmarkValidate, the change tohuma.ValidateAndSetDefaultscould impact performance benchmarks due to the additional overhead of setting defaults. Evaluate whether this affects benchmark results significantly and consider if setting defaults is necessary for benchmarking purposes.
1637-1637: Validate discriminator handling with defaultsIn
Test_validateWithDiscriminator, the switch tohuma.ValidateAndSetDefaultsmay affect how discriminators process default values. Ensure that the tests accurately reflect the behavior when defaults are applied and that the discriminator logic remains correct.huma.go (1)
1171-1171: Correct replacement ofValidatewithValidateAndSetDefaultsYou've correctly replaced
ValidatewithValidateAndSetDefaultsto ensure that default values are set during parameter validation.
|
|
||
| ct := r.Header.Get("Content-Type") | ||
| huma.Validate(registry, strSchema, pb, huma.ModeReadFromServer, ct, res) | ||
| huma.ValidateAndSetDefaults(registry, strSchema, pb, huma.ModeReadFromServer, ct, res) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding Content-Type format validation
While the change to ValidateAndSetDefaults is correct, Content-Type headers typically follow a specific format (e.g., "type/subtype"). Consider adding format validation to ensure valid media types.
- huma.ValidateAndSetDefaults(registry, strSchema, pb, huma.ModeReadFromServer, ct, res)
+ mediaTypeSchema := registry.Schema(reflect.TypeOf(""), false, "format:media-type")
+ huma.ValidateAndSetDefaults(registry, mediaTypeSchema, pb, huma.ModeReadFromServer, ct, res)Committable suggestion was skipped due to low confidence.
| num, err := strconv.Atoi(r.URL.Query().Get("num")) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| huma.Validate(registry, numSchema, pb, huma.ModeReadFromServer, num, res) | ||
| huma.ValidateAndSetDefaults(registry, numSchema, pb, huma.ModeReadFromServer, num, res) |
There was a problem hiding this comment.
Improve error handling for query parameter conversion
The current implementation panics on conversion failure. Consider handling the error gracefully and using the validation result to report the error.
- num, err := strconv.Atoi(r.URL.Query().Get("num"))
- if err != nil {
- panic(err)
- }
- huma.ValidateAndSetDefaults(registry, numSchema, pb, huma.ModeReadFromServer, num, res)
+ numStr := r.URL.Query().Get("num")
+ num := 0
+ if numStr != "" {
+ var err error
+ num, err = strconv.Atoi(numStr)
+ if err != nil {
+ res.Errors = append(res.Errors, &huma.ValidationError{
+ Path: "num",
+ Code: "type",
+ Message: "invalid number format",
+ })
+ return
+ }
+ }
+ huma.ValidateAndSetDefaults(registry, numSchema, pb, huma.ModeReadFromServer, num, res)📝 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.
| num, err := strconv.Atoi(r.URL.Query().Get("num")) | |
| if err != nil { | |
| panic(err) | |
| } | |
| huma.Validate(registry, numSchema, pb, huma.ModeReadFromServer, num, res) | |
| huma.ValidateAndSetDefaults(registry, numSchema, pb, huma.ModeReadFromServer, num, res) | |
| numStr := r.URL.Query().Get("num") | |
| num := 0 | |
| if numStr != "" { | |
| var err error | |
| num, err = strconv.Atoi(numStr) | |
| if err != nil { | |
| res.Errors = append(res.Errors, &huma.ValidationError{ | |
| Path: "num", | |
| Code: "type", | |
| Message: "invalid number format", | |
| }) | |
| return | |
| } | |
| } | |
| huma.ValidateAndSetDefaults(registry, numSchema, pb, huma.ModeReadFromServer, num, res) |
| num, err := strconv.Atoi(c.QueryParam("num")) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| huma.Validate(registry, numSchema, pb, huma.ModeReadFromServer, num, res) | ||
| huma.ValidateAndSetDefaults(registry, numSchema, pb, huma.ModeReadFromServer, num, res) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider validating before type conversion.
The current implementation converts the query parameter to integer before validation. This might interfere with default value handling, especially for zero values. Consider validating the raw string value first, then performing the conversion.
- num, err := strconv.Atoi(c.QueryParam("num"))
- if err != nil {
- panic(err)
- }
- huma.ValidateAndSetDefaults(registry, numSchema, pb, huma.ModeReadFromServer, num, res)
+ numStr := c.QueryParam("num")
+ huma.ValidateAndSetDefaults(registry, strSchema, pb, huma.ModeReadFromServer, numStr, res)
+ if len(res.Errors) > 0 {
+ panic(res.Errors)
+ }
+ num, err := strconv.Atoi(numStr)
+ if err != nil {
+ panic(err)
+ }Committable suggestion was skipped due to low confidence.
| num, err := strconv.Atoi(r.URL.Query().Get("num")) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| huma.Validate(registry, numSchema, pb, huma.ModeReadFromServer, num, res) | ||
| huma.ValidateAndSetDefaults(registry, numSchema, pb, huma.ModeReadFromServer, num, res) |
There was a problem hiding this comment.
Consider validating before type conversion.
The current implementation returns an error for invalid numbers before ValidateAndSetDefaults can apply any default values. Consider moving the validation before the conversion to allow proper default handling for missing or invalid query parameters.
- num, err := strconv.Atoi(r.URL.Query().Get("num"))
- if err != nil {
- return err
- }
- huma.ValidateAndSetDefaults(registry, numSchema, pb, huma.ModeReadFromServer, num, res)
+ numStr := r.URL.Query().Get("num")
+ var num int
+ if numStr != "" {
+ var err error
+ num, err = strconv.Atoi(numStr)
+ if err != nil {
+ return err
+ }
+ }
+ huma.ValidateAndSetDefaults(registry, numSchema, pb, huma.ModeReadFromServer, num, res)📝 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.
| num, err := strconv.Atoi(r.URL.Query().Get("num")) | |
| if err != nil { | |
| return err | |
| } | |
| huma.Validate(registry, numSchema, pb, huma.ModeReadFromServer, num, res) | |
| huma.ValidateAndSetDefaults(registry, numSchema, pb, huma.ModeReadFromServer, num, res) | |
| numStr := r.URL.Query().Get("num") | |
| var num int | |
| if numStr != "" { | |
| var err error | |
| num, err = strconv.Atoi(numStr) | |
| if err != nil { | |
| return err | |
| } | |
| } | |
| huma.ValidateAndSetDefaults(registry, numSchema, pb, huma.ModeReadFromServer, num, res) |
| ValidateAndSetDefaults(oapi.Components.Schemas, inSchema, pb, ModeWriteToServer, parsed, res) | ||
|
|
||
| // Validate changes the original parsed input setting default values when needed | ||
| // so we need to marshal the parsed input back to a byte buffer to get the | ||
| // default values set by the validator. | ||
| parsedBuff := new(strings.Builder) | ||
| err := DefaultJSONFormat.Marshal(parsedBuff, parsed) | ||
| if err != nil { | ||
| WriteErr(api, ctx, http.StatusBadRequest, "could not set default value", err) | ||
| return | ||
| } | ||
| body = []byte(parsedBuff.String()) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing default value handling to avoid re-marshaling
Re-marshaling the parsed input back into body introduces additional overhead. You might consider applying default values directly to the input struct to improve performance.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- validate.go (13 hunks)
🔇 Additional comments (3)
validate.go (3)
354-354: LGTM: Function rename accurately reflects the expanded functionality.The function name change from
ValidatetoValidateAndSetDefaultsbetter describes its dual responsibility of validation and default value assignment.
268-268: LGTM: Consistent function name updates across all call sites.All call sites have been properly updated to use the new
ValidateAndSetDefaultsfunction name.Also applies to: 286-286, 334-334, 374-374, 380-380, 573-573, 648-648, 671-671, 747-747, 778-778, 842-842
354-354: Verify test coverage for default value handling.Please ensure that test cases cover various scenarios for default value handling:
- Setting defaults for nil values
- Not overriding existing zero values
- Handling nested objects with defaults
✅ Verification successful
Test coverage for default value handling is sufficient
The codebase shows comprehensive test coverage for default value handling through:
- Validation tests with nil values and zero values
- Tests for optional fields with omitempty tags
- Tests for nullable fields and pointer types
- Tests across different validation modes (ReadFromServer, WriteToServer)
- Integration tests across multiple adapters that exercise the ValidateAndSetDefaults function
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for test coverage of default value handling # Look for test cases related to default values rg -l "Default.*nil|v\.Default|default.*value" --type go --glob "*_test.go" # Look for specific test function names related to defaults rg -l "Test.*Default|Default.*Test" --type go --glob "*_test.go"Length of output: 571
Script:
#!/bin/bash # Let's examine the actual test cases for default value handling in the most relevant test files # Check registry_test.go for default value test cases echo "=== Registry Test Cases ===" rg "func.*Test.*Default|test.*default" registry_test.go -B 1 -A 10 # Check huma_test.go for default value test cases echo -e "\n=== Huma Test Cases ===" rg "func.*Test.*Default|test.*default" huma_test.go -B 1 -A 10 # Look for actual default value assertions in test files echo -e "\n=== Default Value Assertions ===" rg "Default.*=|SetDefault|v\.Default" --type go --glob "*_test.go" -B 1 -A 3Length of output: 8936
Script:
#!/bin/bash # Let's specifically look for test cases in validate_test.go since it's the most relevant echo "=== Validate Test Cases ===" rg "func Test.*" validate_test.go -B 1 -A 15 # Look for test cases with nil or zero values echo -e "\n=== Nil/Zero Value Test Cases ===" rg "nil|Zero" validate_test.go -B 2 -A 5 # Look for struct definitions with default values in test files echo -e "\n=== Test Structs with Default Values ===" ast-grep --pattern 'type $_ struct { $$$ $_ $_ `$$$default:"$_"$$$` $$$ }'Length of output: 5378
| if m[k] == nil && v.Default != nil { | ||
| m[k] = v.Default | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting duplicate default value logic.
The default value setting logic is duplicated between handleMapString and handleMapAny. Consider extracting this into a helper function to follow the DRY principle.
+func setDefaultIfNil(m map[any]any, k any, defaultValue any) {
+ if m[k] == nil && defaultValue != nil {
+ m[k] = defaultValue
+ }
+}
func handleMapString(r Registry, s *Schema, path *PathBuffer, mode ValidateMode, m map[string]any, res *ValidateResult) {
// ...
- if m[k] == nil && v.Default != nil {
- m[k] = v.Default
- }
+ setDefaultIfNil(unsafeMapCast(m), k, v.Default)
// ...
}
func handleMapAny(r Registry, s *Schema, path *PathBuffer, mode ValidateMode, m map[any]any, res *ValidateResult) {
// ...
- if m[k] == nil && v.Default != nil {
- m[k] = v.Default
- }
+ setDefaultIfNil(m, k, v.Default)
// ...
}
+// unsafeMapCast performs an unsafe cast from map[string]any to map[any]any
+// This is safe because string keys are compatible with any keys
+func unsafeMapCast(m map[string]any) map[any]any {
+ return *(*map[any]any)(unsafe.Pointer(&m))
+}Also applies to: 699-701
|
Superceded by #633 and released in v2.25.0 |
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
These changes enhance the robustness and reliability of input handling within the application.