fix: issue with default field values that can override payload field values #597
Conversation
WalkthroughThe pull request introduces significant changes to the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (3)
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 and nitpick comments (1)
huma.go (1)
1297-1297: Correct the misspelling of 'overridden'The variable name
atLeastOneFieldIsOverridenand related comments contain a misspelling of the word 'overridden'. It should be spelled with double 'd'.Apply the following diffs to fix the misspellings:
Update the variable declaration and assignment:
- atLeastOneFieldIsOverriden := false + atLeastOneFieldIsOverridden := false- atLeastOneFieldIsOverriden = true + atLeastOneFieldIsOverridden = trueCorrect the comments:
- // - THEN the input required field value is overriden with a different value + // - THEN the input required field value is overridden with a different value- // Restore wrongly overriden input values if any + // Restore wrongly overridden input values if anyAlso applies to: 1303-1303, 1305-1305, 1308-1308
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- huma.go (1 hunks)
- huma_test.go (1 hunks)
🧰 Additional context used
🪛 GitHub Check: Build & Test (1.22)
huma.go
[failure] 1303-1303:
overridenis a misspelling ofoverridden(misspell)
[failure] 1308-1308:
overridenis a misspelling ofoverridden(misspell)huma_test.go
[failure] 630-630:
len: use assert.Len (testifylint)
🔇 Additional comments (3)
huma.go (1)
1296-1311: Logic for handling default values is correctThe implemented logic correctly prevents overriding valid input values with default values when the input explicitly specifies a zero value. The restoration mechanism ensures that any input values that were incorrectly overridden are properly restored.
🧰 Tools
🪛 GitHub Check: Build & Test (1.22)
[failure] 1303-1303:
overridenis a misspelling ofoverridden(misspell)
[failure] 1308-1308:
overridenis a misspelling ofoverridden(misspell)huma_test.go (2)
633-634: LGTM!The assertions correctly verify the properties of the second item in the
Itemsslice.
640-640: LGTM!The test payload is appropriately constructed with the expected
itemsdata.
huma_test.go
Outdated
| @@ -627,14 +627,17 @@ | |||
| assert.Equal(t, 5, input.Body.Count) | |||
| assert.Equal(t, []string{"foo", "bar"}, input.Body.Tags) | |||
| assert.Equal(t, []int{1, 2, 3}, input.Body.Numbers) | |||
| assert.Equal(t, len(input.Body.Items), 2) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use assert.Len for slice length assertion
On line 630, it's more expressive to use assert.Len when checking the length of a slice. This provides better error messages on failure.
Apply this diff:
- assert.Equal(t, len(input.Body.Items), 2)
+ assert.Len(t, input.Body.Items, 2)📝 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.
| assert.Equal(t, len(input.Body.Items), 2) | |
| assert.Len(t, input.Body.Items, 2) |
🧰 Tools
🪛 GitHub Check: Build & Test (1.22)
[failure] 630-630:
len: use assert.Len (testifylint)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #597 +/- ##
=======================================
Coverage 92.80% 92.80%
=======================================
Files 22 22
Lines 3903 3907 +4
=======================================
+ Hits 3622 3626 +4
Misses 236 236
Partials 45 45 ☔ View full report in Codecov by Sentry. |
| } | ||
| }) | ||
| // Restore wrongly overridden input values if any | ||
| if atLeastOneFieldIsOverridden { | ||
| _ = api.Unmarshal(ctx.Header("Content-Type"), body, f.Addr().Interface()) |
There was a problem hiding this comment.
This seems kind of inefficient and I'm wondering if there are any edge cases where it would inadvertently overwrite something... 🤔
There was a problem hiding this comment.
Here I propose a different way to fix this.
Summary by CodeRabbit
New Features
Bug Fixes
Tests