🩹 Fix: improper query/body parsing with embedded structs #2906
Conversation
devhsoj
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
WalkthroughThe recent changes aim to enhance the parsing logic within the Changes
Related issues
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| // Is the field an embedded struct? | ||
| if structFieldKind == reflect.Struct { | ||
| // Loop over embedded struct fields | ||
| for j := 0; j < structField.NumField(); j++ { | ||
| structFieldField := structField.Field(j) | ||
|
|
||
| // Can this embedded field be changed? | ||
| if !structFieldField.CanSet() { | ||
| continue | ||
| } | ||
|
|
||
| // Is the embedded struct field type equal to the input? | ||
| if structFieldField.Kind() == kind { | ||
| return true | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The addition of the check for embedded struct fields in the equalFieldType function is a significant improvement to the query parsing mechanism. This change allows for a more accurate and reliable parsing of queries that include embedded structs, directly addressing the issue highlighted in the related problem report.
However, there are a few considerations and potential improvements to ensure the robustness and maintainability of this solution:
-
Performance Considerations: The nested loop introduced for checking embedded struct fields could potentially impact performance, especially for structs with a large number of fields or deeply nested embedded structs. It might be beneficial to benchmark this change with various struct configurations to understand its impact on performance.
-
Recursion for Deeply Nested Structs: The current implementation handles one level of embedded structs. If there are structs embedded within embedded structs, this implementation might not correctly identify the field type. Consider using a recursive approach to handle arbitrary levels of nested structs.
-
Error Handling and Logging: While the current implementation focuses on enhancing the comparison logic, adding error handling or logging for unexpected scenarios (e.g., when reflection operations fail) could improve the robustness and debuggability of the code.
-
Code Comments and Documentation: Adding detailed comments explaining the logic behind handling embedded structs and the rationale for this approach would enhance the maintainability of the code. Future contributors would benefit from understanding the context and reasoning behind these changes.
Overall, this enhancement is a valuable addition to the system, and with some refinements, it can be further improved to ensure its effectiveness and maintainability.
Consider implementing the suggested improvements to address potential performance concerns, support deeper levels of nested structs, and enhance the maintainability of the code.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2906 +/- ##
==========================================
+ Coverage 82.66% 82.82% +0.16%
==========================================
Files 116 116
Lines 8368 8375 +7
==========================================
+ Hits 6917 6937 +20
+ Misses 1107 1097 -10
+ Partials 344 341 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- ctx_test.go (4 hunks)
Additional comments: 5
ctx_test.go (5)
- 1358-1369: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
Looks good! The test cases in
TestCtx_ParamsInteffectively cover the functionality of theParamsIntmethod, including edge cases and default value behavior.
- 1358-1369: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The test
Test_Ctx_extractIPsFromHeadercorrectly verifies the extraction of IP addresses from theX-Forwarded-Forheader, including handling of various edge cases.
- 1358-1369: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The test
Test_Ctx_extractIPsFromHeader_EnableValidateIpeffectively verifies the extraction of IP addresses with IP validation enabled, ensuring that only valid IPs are considered.
- 1358-1369: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The test
Test_Ctx_GetRespHeaderscorrectly verifies the functionality of theGetRespHeadersmethod, ensuring that all response headers, including those with multiple values, are correctly returned.
- 1358-1369: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The test
Test_Ctx_GetReqHeaderseffectively verifies the functionality of theGetReqHeadersmethod, ensuring that all request headers, including those with multiple values, are correctly returned.
|
Thanks for the feedback, I just pushed updates to the |
devhsoj
changed the title
devhsoj
changed the title
|
think we can take over the customization for now in this pull request @gaby @sixcolors @efectn |
|
@ReneWerner87 My only concern is codecov shows that those new lines are not covered by the updated tests. |
|
since we will be refactoring later anyway, it's not a big deal, otherwise we will increase the coverage even later |
|
Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
Description
This PR fixes improper bodyparser/query parsing when using embedded structs within a schema.
Related to #2859
Type of change
Summary by CodeRabbit
equalFieldTypeto better handle embedded struct fields.Namesfield in the process.