-
-
Notifications
You must be signed in to change notification settings - Fork 158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revert nullable schema generation for array slices #594
Conversation
…json.Marshal" This reverts commit 72a101d.
WalkthroughThis pull request introduces several enhancements to the schema configuration capabilities within the Huma framework. A new Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
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 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 #594 +/- ##
==========================================
+ Coverage 92.80% 92.81% +0.01%
==========================================
Files 22 22
Lines 3903 3910 +7
==========================================
+ Hits 3622 3629 +7
Misses 236 236
Partials 45 45 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
registry.go (5)
12-19
: LGTM, but consider placement of configuration.The
RegistryConfig
struct is well-defined and documented. However, as mentioned in the PR description, this configuration might be better suited within thehuma.Config
structure for better accessibility during schema generation.Consider exploring ways to integrate this configuration with the existing
huma.Config
structure in future iterations to improve API consistency and accessibility.
32-32
: LGTM. Consider returning a copy of the config.The addition of the
Config()
method to theRegistry
interface is consistent with the newRegistryConfig
struct. However, returning a pointer might allow unintended modifications to the configuration.Consider returning a copy of the config instead of a pointer to prevent unexpected modifications:
Config() RegistryConfigThis change would ensure that the configuration remains immutable from the caller's perspective.
79-84
: LGTM. Consider ensuring immutability of the config.The addition of the
config
field tomapRegistry
and the implementation of theConfig()
method are consistent with theRegistry
interface update. However, returning a pointer to theconfig
field might allow external modification of the configuration.Consider returning a copy of the config instead of a pointer to ensure immutability:
func (r *mapRegistry) Config() RegistryConfig { return r.config }This change would prevent unintended modifications to the registry's configuration from external code.
188-190
: LGTM. Consider using a named RegistryConfig for clarity.The initialization of the
config
field with default values is correct and consistent with the PR objectives.For improved readability, consider using a named
RegistryConfig
:config: RegistryConfig{ NullSlices: false, },This change would make it clearer that we're initializing a
RegistryConfig
struct, especially if more fields are added in the future.
Line range hint
1-191
: Overall implementation looks good, but reconsider configuration placement.The changes successfully introduce the new configuration option for nullable slices while reverting the previous behavior. The implementation is consistent and well-documented. However, as mentioned in the PR description, the placement of this configuration in the
Registry
might not be ideal.Consider exploring ways to integrate this configuration with the existing
huma.Config
structure in a future iteration. This would improve API consistency and make the configuration more accessible during schema generation without significant alterations to the API.schema.go (1)
765-765
: Consider updating documentation and test cases.With the introduction of configurable slice nullability, it would be beneficial to:
- Update any relevant documentation to reflect this new configuration option.
- Add or modify test cases to cover different configuration settings for slice nullability.
This will ensure that users are aware of the new behavior and that it works correctly under various scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- api.go (1 hunks)
- registry.go (4 hunks)
- schema.go (1 hunks)
- schema_test.go (12 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
api.go
[warning] 194-195: api.go#L194-L195
Added lines #L194 - L195 were not covered by tests
🔇 Additional comments (19)
api.go (1)
194-196
:⚠️ Potential issueConsider refactoring the configuration structure and improve method implementation.
While the
RegistryConfig()
method provides access to the schema registry configuration, there are several points to consider:
As mentioned in the PR description, the placement of this configuration option might not be ideal. Consider moving it to the
huma.Config
structure for easier access during schema generation.The method lacks null checks, which could lead to a null pointer dereference if
c.Components
orc.Components.Schemas
is nil. Consider adding appropriate checks.The method is not documented. Adding a comment explaining its purpose and return value would improve usability.
To address these concerns, consider refactoring the method as follows:
+// RegistryConfig returns the configuration for the schema registry. +// It returns nil if the Components or Schemas are not initialized. func (c *Config) RegistryConfig() *RegistryConfig { + if c.Components == nil || c.Components.Schemas == nil { + return nil + } return c.Components.Schemas.Config() }The lack of test coverage for this new method is concerning. Consider adding unit tests to ensure its functionality. Here's a script to check for existing tests:
If no tests are found, please add appropriate unit tests for this new method.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 194-195: api.go#L194-L195
Added lines #L194 - L195 were not covered by testsschema.go (1)
765-765
: LGTM! Verify impact on existing code.The change to make slice nullability configurable is a good improvement. It allows for more flexibility in schema generation and aligns with the PR objectives.
To ensure this change doesn't unexpectedly affect existing code, let's check for any direct uses of the
Nullable
field for array types:schema_test.go (17)
109-111
: Initialization ofnullSlicesCfg
is CorrectThe
nullSlicesCfg
variable is properly initialized withNullSlices: true
, allowing tests to configure the registry to generate nullable slices and arrays.
118-118
: Addingconfig
Field Enhances Test FlexibilityIntroducing the
config huma.RegistryConfig
field to the test cases enables each test to customize the registry configuration, which is beneficial for testing different scenarios.
1103-1108
: New Test Case for Nullable Arrays withNullSlices
ConfigurationThe
"array-null-cfg"
test case correctly verifies that arrays are nullable whenNullSlices
is set totrue
. The expected JSON schema reflects the nullable array type as["array", "null"]
, ensuring accurate schema generation.
1110-1114
: New Test Case for Nullable Slices withNullSlices
ConfigurationThe
"slice-null-cfg"
test case appropriately tests that slices are nullable whenNullSlices
istrue
. The expected schema output correctly includes["array", "null"]
in thetype
field.
1116-1135
: Test Case for Nullable Array Fields in StructsThe
"field-array-null-cfg"
test case effectively verifies that array fields within structs are correctly marked as nullable when theNullSlices
configuration is enabled. The expected JSON schema accurately represents the nullable array type.
1142-1143
: Proper Configuration ofNullSlices
for Each Test CaseSetting
r.Config().NullSlices = c.config.NullSlices
within the test loop ensures that each test case uses the intendedNullSlices
configuration. This approach maintains test independence and accuracy.
294-294
: Consistency in Array Type DefinitionThe
"type": "array"
in the expected JSON schema for the"field-array"
test case maintains consistency with the schema definitions, aligning with prior expectations.
352-352
: Accurate Typing in Array Enum SchemaIn the
"field-array-enum"
test case, the"type": "array"
is correctly specified, ensuring the array of integers with enumerated values is accurately represented in the schema.
442-442
: Correct Schema Representation for Default Array of StringsThe
"type": "array"
in the expected schema for"field-default-array-string"
is properly defined, accurately reflecting the default value of["foo", "bar"]
.
462-462
: Proper Array Type Specification in Default Array of IntegersThe expected schema for
"field-default-array-int"
includes the correct"type": "array"
, ensuring that the default integer array[1, 2]
is properly represented.
761-761
: Accurate Schema for Recursive Array FieldsIn the
"recursive-embedded-structure"
test case, the"type":"array"
definitions for thearray
andslice
fields are correctly specified, ensuring the schema accurately captures the recursive nature of these fields.Also applies to: 779-779
901-901
: Consistent Array Typing in Custom Length String SliceThe
"type":"array"
in the expected schema for"field-custom-length-string-in-slice"
test case accurately represents the slice ofTypedStringWithCustomLength
, maintaining consistency in array definitions.
1058-1058
: Correct Array Schema with Custom DescriptionThe schema for
"field-custom-array"
correctly specifies the"type":"array"
and includes the custom description, ensuring the array of floats with a fixed length is properly documented.
1081-1081
: Accurate Schema for Pointer to Custom ArrayIn the
"field-ptr-to-custom-array"
test case, the"type":"array"
is correctly defined, and the custom description is preserved, accurately representing the pointer to a custom array type.
1103-1108
: Validating Nullable Array Behavior in Schema GenerationThe expected schema in the
"array-null-cfg"
test case correctly includes"type": ["array", "null"]
, validating that arrays are nullable when the configuration is enabled.
1110-1114
: Ensuring Nullable Slice Behavior with ConfigurationThe
"slice-null-cfg"
test case's expected schema accurately reflects the nullable slice type, confirming that theNullSlices
configuration is effective.
1116-1135
: Testing Nullable Array Field within StructsThe expected schema in
"field-array-null-cfg"
precisely includes"type": ["array", "null"]
for thevalue
field, demonstrating that array fields within structs are correctly treated as nullable when configured.
110fcaa
to
1869d0e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
api_test.go (2)
129-129
: Consider adding documentation for the new configuration optionWhile the new test function effectively demonstrates the usage of the
NullSlices
configuration, it would be beneficial to add some documentation explaining the purpose and implications of this new option. This could be in the form of a comment above the test function or even a package-level comment if this is a significant change to the library's behavior.Consider adding a comment like this before the test function:
// TestRegistryConfig verifies the behavior of the new NullSlices configuration option. // This option allows users to specify whether schemas for arrays or slices should be nullable by default. // Note: The placement of this configuration might change in future versions to be part of huma.Config.This documentation will help other developers understand the purpose of this test and the new configuration option.
130-134
: Address concerns about configuration placementThe implementation of the
NullSlices
configuration option in the schema registry is a step towards addressing the issue mentioned in #571. However, as noted in the PR objectives, there are concerns about its current placement.
- Consider documenting the current limitations of this approach in the code or README.
- Create an issue to track the potential future relocation of this configuration to
huma.Config
.- Evaluate if there's a way to make this configuration accessible during schema generation without significant API changes.
While this implementation provides a solution to the immediate problem, it's important to consider the long-term maintainability and usability of the API. If possible, explore options to refactor the configuration structure in a way that doesn't break existing APIs but allows for more flexibility in the future.
Would you like assistance in drafting a plan for future improvements or creating an issue to track this technical debt?
schema_test.go (3)
1115-1135
: LGTM: Comprehensive test case for nullable array field configuration.This test case thoroughly verifies the behavior of the new nullable array field configuration. It ensures that when
NullSlices
is set to true, the generated schema includes"null"
as a possible type for the array field while preserving other array constraints likeminItems
,maxItems
, anduniqueItems
.Consider adding a test case for a nullable slice field as well, to ensure complete coverage of both array and slice scenarios with field-level constraints.
1142-1143
: LGTM: Integration of new configuration in test function.The changes correctly integrate the new
NullSlices
configuration into the existing test framework. This ensures that the new feature is properly tested alongside existing functionality.Consider adding a comment explaining the purpose of the
NullSlices
configuration for better code readability and maintainability.
Line range hint
109-1143
: Overall implementation of nullable slices configuration looks good.The changes effectively implement and test the new nullable slices configuration as described in the PR objectives. The implementation is consistent, and the test coverage is comprehensive, including cases for arrays, slices, and fields with array types.
While the implementation is good, the PR author's concern about the placement of this configuration option is valid. Consider exploring ways to integrate this configuration into the
huma.Config
structure in a future iteration, which would make it more accessible during schema generation without significant API changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- api.go (1 hunks)
- api_test.go (1 hunks)
- registry.go (4 hunks)
- schema.go (1 hunks)
- schema_test.go (12 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- api.go
- registry.go
- schema.go
🔇 Additional comments (3)
schema_test.go (3)
109-111
: LGTM: New configuration field for nullable slices.The addition of the
NullSlices
field to theRegistryConfig
struct is a good implementation of the new feature described in the PR objectives. This allows users to specify whether schemas for arrays or slices should be nullable by default.
1103-1108
: LGTM: New test case for nullable array configuration.This test case correctly verifies the behavior of the new nullable array configuration. It ensures that when
NullSlices
is set to true, the generated schema includes"null"
as a possible type for the array.
1109-1114
: LGTM: New test case for nullable slice configuration.This test case correctly verifies the behavior of the new nullable slice configuration. It ensures that when
NullSlices
is set to true, the generated schema includes"null"
as a possible type for the slice.
api_test.go
Outdated
func TestRegistryConfig(t *testing.T) { | ||
cfg := huma.DefaultConfig("Test", "3.1") | ||
cfg.RegistryConfig().NullSlices = true | ||
assert.True(t, cfg.Components.Schemas.Config().NullSlices) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Enhance test coverage for the new NullSlices
configuration
The test correctly verifies that the NullSlices
configuration can be set. However, to ensure comprehensive coverage, consider the following improvements:
- Test the default value of
NullSlices
before setting it. - Add a test case that demonstrates how this configuration affects schema generation for array/slice fields.
- Consider testing edge cases or interactions with other configuration options.
Here's a suggested expansion of the test:
func TestRegistryConfig(t *testing.T) {
// Test default value
cfg := huma.DefaultConfig("Test", "3.1")
assert.False(t, cfg.Components.Schemas.Config().NullSlices, "Default value of NullSlices should be false")
// Test setting the value
cfg.RegistryConfig().NullSlices = true
assert.True(t, cfg.Components.Schemas.Config().NullSlices, "NullSlices should be set to true")
// Test effect on schema generation
type TestStruct struct {
SliceField []string `json:"sliceField"`
}
schema := cfg.Components.Schemas.Add(TestStruct{})
sliceFieldSchema := schema.Properties["sliceField"]
assert.True(t, sliceFieldSchema.Nullable, "Slice field should be nullable when NullSlices is true")
// Test interaction with other options (if applicable)
// ...
}
Additionally, given the concerns about the placement of this configuration option mentioned in the PR objectives, it might be worth adding a comment in the test file noting this limitation and potential future changes.
Consider adding a TODO comment about the potential relocation of this configuration option to huma.Config
in the future, as mentioned in the PR objectives.
1869d0e
to
0f04456
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
schema_test.go (2)
1103-1135
: LGTM: Comprehensive test cases for nullable slices.The new test cases thoroughly cover the nullable slices feature for arrays, slices, and struct fields with array types. They correctly expect the
type
field to be["array", "null"]
when using the nullable slices configuration.Consider adding a test case for a nested array or slice to ensure the feature works correctly with more complex data structures.
Line range hint
109-1143
: Summary: Well-implemented nullable slices feature with comprehensive tests.The changes successfully implement the nullable slices feature and provide thorough test coverage. The new
NullSlices
configuration option inhuma.RegistryConfig
is correctly utilized throughout the test cases. The implementation maintains compatibility with existing code while adding support for nullable arrays and slices in schema generation.Consider documenting this new feature in the library's documentation and README, explaining its purpose and usage. This will help users understand and utilize the new nullable slices functionality effectively.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- api.go (1 hunks)
- api_test.go (1 hunks)
- registry.go (4 hunks)
- schema.go (1 hunks)
- schema_test.go (12 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- api.go
- api_test.go
- registry.go
- schema.go
🔇 Additional comments (2)
schema_test.go (2)
109-111
: LGTM: New configuration for nullable slices.The
nullSlicesCfg
variable is correctly defined withNullSlices
set to true. This will be useful for testing the new nullable slices feature.
1142-1143
: LGTM: Correct application of NullSlices configuration.The
NullSlices
configuration is now correctly set in the registry before running each test case. This ensures that the nullable slices feature is properly tested with the provided configuration.
As commented on #612, I like the more straightforward alternative 👍 Closing ! |
1961ef9 reverts #527 which seems to be causing more trouble than good (see #571 and comments on #527).
110fcaa introduces a configuration field in the schema registry, which allows specifying whether schemas for arrays/slices should be nullable by default. This is not ideal since it would rather belong in the huma.Config, but then it would not be accessible at the time of schema generation without major changes in the API. Let me know if you got ideas on how to improve this.
Summary by CodeRabbit
New Features
RegistryConfig
struct to manage nullability of slices in OpenAPI schema generation.Config
methods to provide direct access to registry configurations.Bug Fixes
Nullable
property for array types, allowing dynamic control based on configuration settings.Tests
NullSlices
configuration, ensuring accurate schema generation for nullable arrays and slices.