Exclude embedded fields in response header docs #656
Conversation
WalkthroughThe changes in this pull request modify the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Warning Rate limit exceeded@lucaspopp-wbd has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 47 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. 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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #656 +/- ##
=======================================
Coverage 93.04% 93.04%
=======================================
Files 22 22
Lines 4901 4905 +4
=======================================
+ Hits 4560 4564 +4
Misses 298 298
Partials 43 43 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
lucaspopp-wbd
changed the title
![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
🧹 Outside diff range and nitpick comments (2)
huma.go (2)
233-236: Add test coverage for embedded field exclusion.The new code block that handles embedded field exclusion is not covered by tests. Please add test cases to verify:
- Embedded fields with header tags are correctly ignored
- Non-embedded fields with header tags are still processed
Would you like me to help generate test cases for this functionality?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 235-236: huma.go#L235-L236
Added lines #L235 - L236 were not covered by tests
233-236: Document the embedded field behavior.Consider adding a comment to explain that embedded fields are intentionally ignored when processing headers.
Apply this diff to add documentation:
func findHeaders(t reflect.Type) *findResult[*headerInfo] { return findInType(t, nil, func(sf reflect.StructField, i []int) *headerInfo { + // Embedded (anonymous) fields are intentionally ignored when processing headers if sf.Anonymous { return nil }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 235-236: huma.go#L235-L236
Added lines #L235 - L236 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
huma.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
huma.go
[warning] 235-236: huma.go#L235-L236
Added lines #L235 - L236 were not covered by tests
🔇 Additional comments (1)
huma.go (1)
233-236: LGTM! The change correctly implements the exclusion of embedded fields.
The addition of the embedded field check aligns with the PR objective and prevents embedded fields from being processed as headers.
Let's verify the impact of this change on embedded structs:
✅ Verification successful
Let me search for embedded structs with header tags to verify the impact of ignoring embedded fields.
Let me try a different approach to search for embedded fields and header tags.
Let me try one more search to specifically look for header tags and embedded fields together.
Based on the search results, I can now provide a final response:
The change to ignore embedded fields is safe and does not impact header processing.
The codebase search shows that while there are many structs with header tags, none of them use embedded fields for header processing. The header tags are always explicitly defined on regular struct fields. This means ignoring embedded fields won't affect any existing header processing functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for embedded structs that might be affected by this change
# Look for struct types that are embedded in other structs and have header tags
ast-grep --pattern 'type $_ struct {
$$$
$_
$$$
}'
Length of output: 58427
Script:
#!/bin/bash
# Search for embedded structs with header tags
ast-grep --pattern 'type $_ struct {
$$$
$_ `header:"$_"`
$$$
}'
Length of output: 72
Script:
#!/bin/bash
# Search for struct definitions with embedded fields
rg -U "type.*struct.*{[\s\S]*?\s+\w+\s+`header:.*?`[\s\S]*?}" --type go
# Also search for anonymous (embedded) fields
rg "^\s+\w+(\.\w+)?\s*$" --type go
Length of output: 6222
Script:
#!/bin/bash
# Search for struct definitions with header tags
rg -U "type \w+ struct \{(\n[^\}]*?)*?\s+\w+\s+\`.*?header:.*?\`" --type go
# Search for embedded fields in the same files
rg "type \w+ struct \{[^}]*?\n\s+[A-Z]\w+(\.[A-Z]\w+)?\s*(\n|\})" --type go
Length of output: 5234
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 235-236: huma.go#L235-L236
Added lines #L235 - L236 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
huma_test.go (3)
1938-1940: Add documentation comment for exported structPaginationHeadersConsider adding a documentation comment for the exported struct
PaginationHeadersto adhere to Go conventions and improve code readability.
1943-1946: Add documentation comment for exported structRespConsider adding a documentation comment for the exported struct
Respto enhance code clarity and maintainability.
1973-1981: Enhance testignore-anonymous-header-structsfor more precise validationWhile the current test checks that
PaginationHeadersis not included in the OpenAPI output, consider adding an assertion to verify that theLinkheader is not documented. This makes the test more robust by directly checking the absence of the header.Example:
assert.NotContains(t, openapiBody, `"link"`)This ensures that the
Linkheader field is not present in the OpenAPI documentation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
huma_test.go (1)
1974-1983: LGTM! Well-structured test case for embedded struct exclusion.The test case effectively verifies that embedded structs are excluded from OpenAPI output while preserving the header fields. Consider adding additional test cases to verify:
- Multiple embedded structs
- Nested embedded structs
- Embedded structs with conflicting header fields
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
huma_test.go(2 hunks)
🔇 Additional comments (1)
huma_test.go (1)
1938-1942: LGTM! Clean type definition for testing embedded header fields.
The PaginationHeaders struct is well-defined with appropriate header tag for testing the exclusion of embedded fields from response headers.
Ignore embedded fields when finding response headers (resolves #655)
Not sure if it’s intentional that any field regardless of if tagged with
headeris considered a response header. If that’s not your intent, we could simplify this by returningnilwhenheader == “”Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes