fix: make nullable arrays configurable #612
Conversation
WalkthroughThe pull request introduces updates to the documentation and codebase regarding request validation and nullability in Go structs. It clarifies the handling of nullable fields, particularly for arrays, by introducing a new global variable Changes
Possibly related PRs
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 #612 +/- ##
=======================================
Coverage 92.83% 92.83%
=======================================
Files 22 22
Lines 3923 3923
=======================================
Hits 3642 3642
Misses 236 236
Partials 45 45 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
docs/docs/features/request-validation.md (3)
60-63: LGTM! Consider adding a brief explanation ofhuma.DefaultArrayNullable.The changes clearly explain the new logic for determining nullable fields, particularly for arrays. This aligns well with the PR objectives of making nullable arrays configurable.
To enhance clarity, consider adding a brief explanation of
huma.DefaultArrayNullableand how users can configure it. This could be added as a note or included in the existing explanation.
66-67: LGTM! Consider adding an explanation for the object nullability limitation.The changes clearly state the limitations of object nullability and introduce a panic for unsupported cases. This aligns with the PR objectives of preventing potential issues.
To provide more context, consider adding a brief explanation of why object nullability is not currently supported (e.g., complexity and poor tool support for
anyOf/oneOf). This could help users understand the reasoning behind this limitation.
90-93: LGTM! Valuable explanation of default slice nullability.The added note provides excellent context for why slices are nullable by default in Go, referencing the official Go JSON package documentation. This aligns well with the PR objectives of clarifying nullability behavior.
To make the information even more accessible, consider adding a brief code example demonstrating the behavior of
nilslices when marshaled to JSON. This could help reinforce the concept for readers.🧰 Tools
🪛 Markdownlint
92-92: Expected: fenced; Actual: indented
Code block style(MD046, code-block-style)
schema.go (1)
25-29: Consider the implications of introducing a global variable.The introduction of
DefaultArrayNullableas a global variable allows for configurable default nullability of arrays. While this provides flexibility, it's important to consider the potential drawbacks of using global state:
- It may lead to unexpected behavior if modified during runtime.
- It could make the code harder to reason about, as the behavior now depends on a global state.
- It might complicate testing, as tests may need to reset this global state.
Consider alternatives such as:
- Making this a configuration option passed to relevant functions.
- Using a context-based approach for configuration.
If you decide to keep it as a global variable, ensure it's well-documented and its usage is consistent throughout the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- docs/docs/features/request-validation.md (3 hunks)
- schema.go (2 hunks)
- schema_test.go (2 hunks)
🧰 Additional context used
🪛 Markdownlint
docs/docs/features/request-validation.md
92-92: Expected: fenced; Actual: indented
Code block style(MD046, code-block-style)
🔇 Additional comments (4)
docs/docs/features/request-validation.md (1)
81-81: LGTM! Clear documentation update.The minor update to the comment for struct examples improves clarity without changing the functionality.
schema.go (1)
Line range hint
1-894: Overall assessment: Approved with suggestions for improvementThe changes introduced in this PR provide a flexible way to configure the default nullability of arrays in the schema generation process. This is a valuable feature that can enhance the usability of the library. However, there are a few points to consider for improving the implementation and ensuring its robustness:
- The use of a global variable (
DefaultArrayNullable) introduces potential risks and complexities. Consider alternative approaches such as configuration options or context-based configuration.- Ensure comprehensive test coverage for the new array nullability behavior, including scenarios with
DefaultArrayNullableset to bothtrueandfalse.- Update any relevant documentation to reflect this change in behavior.
- Verify that this change doesn't introduce inconsistencies in schema generation or break any existing assumptions in the codebase or dependent projects.
These suggestions aim to maintain the flexibility introduced by this change while mitigating potential risks and ensuring the continued reliability of the schema generation process.
schema_test.go (2)
725-743: Good addition of test case for nullable arrayThe test case
"field-nullable-array"correctly verifies that an array field withnullable:"true"generates the expected schema where the type includes"null".
745-763: Good addition of test case for non-nullable arrayThe test case
"field-non-nullable-array"appropriately tests that an array field withnullable:"false"generates a schema without the"null"type, ensuring non-nullability.
|
Looks good to me ! I like the idea of keeping things simple by using a global variable in this case 👍 |
This is an alternative approach to #594. @lsdch would love to know what you think.
huma.DefaultArrayNullable.true, as this is more correct based on how Go marshals JSON. Anilslice will encode asnullso we ask the user to opt-in to alternative behavior.nilslices encoding asnull.IMO this prevents another break, moving back to less correct/accurate behavior, and makes everything explicit. I'm not the biggest fan of global variables but it seems reasonable here instead of trying to plumb everything through the various types and interfaces. Thoughts?
Summary by CodeRabbit
Documentation
New Features
Tests