Fix enums placement #124
Fix enums placement #124
Conversation
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
Auto Pull Request Review from LlamaPReview
1. Overview
1.1 Core Changes
- Primary purpose and scope: The primary purpose of this PR is to fix the placement of enum definitions within JSON schemas generated by the
swaggest/jsonschema-golibrary. It specifically targets scenarios where schemas are nested (e.g., arrays of arrays, or schemas with nested object properties). - Key components modified: The core modification is the introduction of the
searchDeepestSchemafunction and its integration into thereflectEnumfunction. Additionally, a new test caseTestReflector_Reflect_EnumsPlacementis added to validate the fix. - Cross-component impacts: This PR should not have significant cross-component impacts as it is focused on internal logic within the
swaggest/jsonschema-golibrary. However, it may affect any code that relies on the correct generation of JSON schemas with enum definitions. - Business value alignment: Correctly placing enum definitions in JSON schemas is crucial for data validation, API documentation, and code generation tools that rely on accurate JSON schema representations. This PR aligns with the business value of ensuring the library generates semantically correct and useful schemas.
1.2 Technical Architecture
- System design modifications: Not applicable for this PR as it focuses on internal logic and does not involve system design changes.
- Component interaction changes: Not applicable as this PR does not change how components interact with each other.
- Integration points impact: Not applicable as this PR does not affect integration points with other systems or libraries.
- Dependency changes and implications: Not applicable as this PR does not introduce or modify dependencies.
2. Critical Findings
2.1 Must Fix (P0🔴)
- Issue: None identified in this PR.
- Impact: N/A
- Resolution: N/A
2.2 Should Fix (P1🟡)
Issue: Lack of explicit nil checks and documentation in the searchDeepestSchema function.
- Impact: While the current implementation likely works in the intended scenarios, being explicit about handling potential nil values at each step improves code robustness and maintainability.
- Suggested Solution: Add explicit nil checks and potentially early returns for each level of nesting in
searchDeepestSchemato improve readability and prevent potential panic if the schema structure deviates unexpectedly.
Suggested Code Snippet:
func searchDeepestSchema(in *Schema) *Schema {
if in.Items == nil {
return in
}
items := in.Items
if items.SchemaOrBool == nil {
return in // Explicitly handle nil SchemaOrBool
}
schemaOrBool := items.SchemaOrBool
if schemaOrBool.TypeObject == nil {
return in // Explicitly handle nil TypeObject
}
typeObject := schemaOrBool.TypeObject
return searchDeepestSchema(typeObject)
}2.3 Consider (P2🟢)
Area: Documentation and test coverage.
- Improvement Opportunity: Add documentation for the
searchDeepestSchemafunction explaining its purpose and logic. Additionally, consider adding more complex test cases to cover a wider range of nested schema scenarios.
Suggested Code Snippet (Documentation):
// searchDeepestSchema recursively traverses the schema structure to find the deepest
// schema object within the 'Items' hierarchy. This is crucial for correctly placing
// enum definitions in nested schema structures, such as arrays of arrays.
// It navigates down through the Items -> SchemaOrBool -> TypeObject path, returning
// the deepest Schema object found along this path. If any of the intermediate
// elements are nil, it returns the current schema, effectively stopping the recursion.
func searchDeepestSchema(in *Schema) *Schema {
// ... function body ...
}Suggested Test Cases (Complex Scenarios):
- Arrays of objects, where the objects have enum properties.
- Nested arrays of arrays of different types, some with enums.
- Schemas with
allOf,oneOf, oranyOfcompositions that involve enums in nested structures.
3. Technical Analysis
3.1 Code Logic Analysis
📁 reflect.go - searchDeepestSchema function
- Submitted PR Code:
func searchDeepestSchema(in *Schema) *Schema {
if in.Items == nil {
return in
}
if in.Items.SchemaOrBool == nil {
return in
}
if in.Items.SchemaOrBool.TypeObject == nil {
return in
}
return searchDeepestSchema(in.Items.SchemaOrBool.TypeObject)
}-
Analysis:
- The function correctly identifies the deepest schema in the
Itemshierarchy by recursively navigating through theItems -> SchemaOrBool -> TypeObjectpath. - However, it relies on implicit nil checks, which could be made more explicit for improved readability and robustness.
- The function lacks documentation explaining its purpose and logic.
- The function correctly identifies the deepest schema in the
-
LlamaPReview Suggested Improvements:
func searchDeepestSchema(in *Schema) *Schema {
if in.Items == nil {
return in
}
items := in.Items
if items.SchemaOrBool == nil {
return in // Explicitly handle nil SchemaOrBool
}
schemaOrBool := items.SchemaOrBool
if schemaOrBool.TypeObject == nil {
return in // Explicitly handle nil TypeObject
}
typeObject := schemaOrBool.TypeObject
return searchDeepestSchema(typeObject)
}- Improvement rationale:
- Technical Benefits: Improved code readability, robustness, and maintainability.
- Business Value: Improved code quality, reduced maintenance costs, and lower risk of bugs.
📁 reflect.go - reflectEnum function modification
- Submitted PR Code:
func reflectEnum(schema *Schema, fieldTag reflect.StructTag, fieldVal interface{}) {
// ... (rest of the function remains unchanged)
target := searchDeepestSchema(schema)
target.Enum = enum.items
// ... (rest of the function remains unchanged)
}-
Analysis:
- The modification to use
searchDeepestSchemais the core fix and appears to be logically sound. It correctly addresses the issue of enum placement in nested schemas. - No specific code improvement is needed for
reflectEnumitself, but ensure that the documentation forreflectEnum(if any exists) is updated to reflect this change in behavior and the usage ofsearchDeepestSchema.
- The modification to use
-
LlamaPReview Suggested Improvements: N/A
-
Improvement rationale: Maintaining accurate documentation is important when code behavior changes, even if it's a bug fix.
📁 reflect_test.go - New test case TestReflector_Reflect_EnumsPlacement
- Submitted PR Code:
func TestReflector_Reflect_EnumsPlacement(t *testing.T) {
// ... (rest of the test case remains unchanged)
}-
Analysis:
- The new test case is well-structured and effectively verifies the intended behavior for basic nested array scenarios.
- However, it might be beneficial to consider adding more complex test cases to cover a wider range of nested schema scenarios.
-
LlamaPReview Suggested Improvements: Consider adding more complex test cases as suggested in the P2.2 area for potential improvement.
-
Improvement rationale: Increased test coverage leads to more reliable software.
3.2 Key Quality Aspects
- System scalability considerations: Not applicable as this PR does not introduce changes that would significantly impact system scalability.
- Performance bottlenecks and optimizations: Not applicable as this PR focuses on functional changes and does not introduce performance-related optimizations.
- Testing strategy and coverage: The PR introduces a new test case, which is a positive addition to the testing strategy. However, consider enhancing test coverage with more complex scenarios as suggested in the P2.2 area for potential improvement.
- Documentation needs: The PR introduces a new function (
searchDeepestSchema) that lacks documentation. Addressing this as suggested in the P2.1 area for potential improvement would improve the code's maintainability and understandability.
4. Overall Evaluation
- Technical assessment: The PR addresses a significant bug related to enum placement in nested JSON schemas. The changes are well-targeted and the introduction of
searchDeepestSchemais a reasonable approach to solve the problem. The added test case provides initial validation. - Business impact: Correctly placing enum definitions in JSON schemas is crucial for data validation, API documentation, and code generation tools that rely on accurate JSON schema representations. This PR aligns with the business value of ensuring the library generates semantically correct and useful schemas.
- Risk evaluation: The PR appears to be low risk as it addresses a specific bug and does not introduce significant changes to the library's behavior or architecture.
- Notable positive aspects and good practices: The PR includes a well-structured test case to validate the fix, which is a positive practice. It also addresses a significant bug that could impact the library's usability and reliability.
- Implementation quality: The implementation is well-targeted and logically sound. However, there are opportunities for improvement in code clarity and documentation, as highlighted in the P1 and P2 issues.
- Final recommendation: Merge the PR after addressing the P1 issues (explicit nil checks and documentation for
searchDeepestSchema) to ensure the code is robust and maintainable. Consider addressing the P2 issues (documentation and more complex test cases) in subsequent updates to further enhance the library's quality.
💡 LlamaPReview Community
Have feedback on this AI Code review tool? Join our GitHub Discussions to share your thoughts and help shape the future of LlamaPReview.
|
Hey, @vearutop could you check this? I have already added tests to both repos. Now we need to merge this and bump version in another) We really need this fix in our project) And passing changes via two replacement go.mod (to my forks with changes) - it's a little bit difficult and sad) |
|
Hi, thank you for this PR. However, I don't think this should be merged. The point raised in the original discussion is still relevant. The workaround introduced in this PR would be inconsistent and incomplete.
|
|
But my PRs are following specification In upstream repo I need this PR merged to bump version. And after that add code for checking/validating full list of enums has the type of current placement schema type... |
|
About
This is spec https://swagger.io/docs/specification/v3_0/data-models/enums/
Combing with JSON schema spec, you could have only variants: string/number/bool/null. That's why enum declaration MUST be on the same indent with primitive type |
|
The variety is type MyStruct struct {
F1 []string `enum:"a"`
F2 [][][]string `enum:"b"`
F3 map[string]string `enum:"c"`
F4 map[int]map[string][]int `enum:"123"`
// And unlimited more different types of fields that may have sub elements in different "positions"
}Another consistency concern is: why only The current design idea is to apply field tags directly to fields they are defined on, which does not make any assumptions on sub elements. This is consistent and should not lead to surprising behavior. I think field tags are not fit to define behavior of sub elements, mostly because there is no clear definition on what sub element is in different types. |
|
Ok. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #124 +/- ##
==========================================
- Coverage 75.82% 75.70% -0.12%
==========================================
Files 7 7
Lines 1249 1840 +591
==========================================
+ Hits 947 1393 +446
- Misses 237 379 +142
- Partials 65 68 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Looking at the test cases, I think this might be good default behavior, with applicability defined as "deepest non-key scalar type". What do you think if we also bind it to a different field tag, for example |
|
In case of enum (like RFC said) we don't have possibility to have other keywords: only enum with list of possible values and type with type definition. Format is not applied at all in these cases. I don't think we need to change tag because golang code behaviour is not changing at all due to my PR. It simply move enum to "correct/best" place and after that we have "correct" openapi spec. For format and etc - later I could check generated spec and bring PRs if needed. To clarify situation, why we are making these changes: we have "old" project with tones of code and no spec. Using its API is really hard, so we decided to add spec and after that generate clients according to this spec. So, we started to use library and got some problems. Examples here. type ProductFilters struct {
Relations []string `json:"relations,omitempty" query:"relations" enum:"components,teams"`In current version we get: - in: query
name: relations
schema:
enum:
- components
- teams
items:
type: string
type: arrayAnd seems like it valid? In case of JSON schema: we have In case of OpenAPI we have My PR just place yaml part in correct place - in: query
name: relations
schema:
type: array
items:
enum:
- components
- teams
type: stringAnd after that we will get correct generated code with enum/const declaration type APIV1ProductsGetRelationsItem string
const (
APIV1ProductsGetRelationsItemComponents APIV1ProductsGetRelationsItem = "components"
APIV1ProductsGetRelationsItemTeams APIV1ProductsGetRelationsItem = "teams"
) |
Related to swaggest/openapi-go#144
We need to merge this and after that bump version in upstream repo.