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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
registry.go (5)
12-19: LGTM, but consider placement of configuration.The
RegistryConfigstruct is well-defined and documented. However, as mentioned in the PR description, this configuration might be better suited within thehuma.Configstructure for better accessibility during schema generation.Consider exploring ways to integrate this configuration with the existing
huma.Configstructure 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 theRegistryinterface is consistent with the newRegistryConfigstruct. 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
configfield tomapRegistryand the implementation of theConfig()method are consistent with theRegistryinterface update. However, returning a pointer to theconfigfield 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
configfield 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
RegistryConfigstruct, 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
Registrymight not be ideal.Consider exploring ways to integrate this configuration with the existing
huma.Configstructure 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.Configstructure for easier access during schema generation.The method lacks null checks, which could lead to a null pointer dereference if
c.Componentsorc.Components.Schemasis 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
Nullablefield for array types:schema_test.go (17)
109-111: Initialization ofnullSlicesCfgis CorrectThe
nullSlicesCfgvariable is properly initialized withNullSlices: true, allowing tests to configure the registry to generate nullable slices and arrays.
118-118: AddingconfigField Enhances Test FlexibilityIntroducing the
config huma.RegistryConfigfield 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 withNullSlicesConfigurationThe
"array-null-cfg"test case correctly verifies that arrays are nullable whenNullSlicesis 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 withNullSlicesConfigurationThe
"slice-null-cfg"test case appropriately tests that slices are nullable whenNullSlicesistrue. The expected schema output correctly includes["array", "null"]in thetypefield.
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 theNullSlicesconfiguration is enabled. The expected JSON schema accurately represents the nullable array type.
1142-1143: Proper Configuration ofNullSlicesfor Each Test CaseSetting
r.Config().NullSlices = c.config.NullSliceswithin the test loop ensures that each test case uses the intendedNullSlicesconfiguration. 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 thearrayandslicefields 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 theNullSlicesconfiguration is effective.
1116-1135: Testing Nullable Array Field within StructsThe expected schema in
"field-array-null-cfg"precisely includes"type": ["array", "null"]for thevaluefield, demonstrating that array fields within structs are correctly treated as nullable when configured.
110fcaa to
1869d0e
Compare
There was a problem hiding this comment.
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
NullSlicesconfiguration, 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
NullSlicesconfiguration 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
NullSlicesis 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
NullSlicesconfiguration 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
NullSlicesconfiguration 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.Configstructure 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
NullSlicesfield to theRegistryConfigstruct 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
NullSlicesis 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
NullSlicesis set to true, the generated schema includes"null"as a possible type for the slice.
| 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.
🛠️ 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
NullSlicesbefore 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.
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
typefield 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
NullSlicesconfiguration option inhuma.RegistryConfigis 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
nullSlicesCfgvariable is correctly defined withNullSlicesset to true. This will be useful for testing the new nullable slices feature.
1142-1143: LGTM: Correct application of NullSlices configuration.The
NullSlicesconfiguration 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
RegistryConfigstruct to manage nullability of slices in OpenAPI schema generation.Configmethods to provide direct access to registry configurations.Bug Fixes
Nullableproperty for array types, allowing dynamic control based on configuration settings.Tests
NullSlicesconfiguration, ensuring accurate schema generation for nullable arrays and slices.