Revert "Fix enums placement" #126
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit 5662f8c.
Lines Of Code
|
Go API Changes# summary Inferred base version: v0.3.75 Suggested version: v0.3.76 |
Unit Test Coveragetotal: (statements) 79.9% Coverage of changed lines
Coverage diff with base branch
|
Benchmark ResultBenchmark diff with base branchBenchmark result |
![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: This PR reverts a previous change (PR #124) that modified how
enumvalues are placed in the generated JSON schema. The revert aims to fix a critical bug introduced by #124 where enum values for numeric types (e.g.,[]int) were incorrectly serialized as strings (e.g.,["1", "2", "3"]). - Key components modified: The primary changes are in
reflect.go(specifically thereflectEnumfunction) and the corresponding test filereflect_test.go. The logic for determining the target schema for enum placement is reverted, and a test case validating the previous behavior is removed. - Cross-component impacts: This change directly affects the structure of the generated JSON schema, particularly for fields involving arrays or maps with associated enums. It impacts any downstream tooling or clients relying on the schema structure, potentially breaking compatibility with tools expecting enums on nested elements (as per the reverted logic) but fixing issues for tools expecting correct primitive type serialization.
- Business value alignment: The PR prioritizes correcting the serialization of primitive enum types (e.g., integers) over the structural placement of enums within nested types (like array items). This fixes a critical data type mismatch bug but regresses on potentially more spec-compliant placement for container types, representing a trade-off.
2. Critical Findings
2.1 Must Fix (P0🔴)
Issue: Reintroduces JSON Schema Specification Non-Compliance for Container Types
- Analysis Confidence: High
- Impact: The reverted code applies enums directly to the schema of container types (like arrays or maps) instead of their items or values. This violates the JSON Schema specification, which requires enums to constrain the values within the container (e.g.,
items.enumfor arrays). This can lead to incorrect validation and client generation issues for tools strictly adhering to the spec for nested structures. For example, an array of strings[]stringwithenum:"a,b"will incorrectly generate{"type": "array", "enum": ["a","b"]}instead of the correct{"type": "array", "items": {"type": "string", "enum": ["a","b"]}}. - Resolution: The logic needs to correctly identify container types and apply the enum constraint to the appropriate nested schema (
itemsfor arrays,additionalPropertiesorpatternPropertiesfor maps) while ensuring the enum values themselves have the correct primitive type (addressing the original bug of #124).
Issue: Removal of Test Coverage for Nested Enum Placement
- Analysis Confidence: High
- Impact: The deletion of
TestReflector_Reflect_EnumsPlacementremoves validation for how enums are handled in complex scenarios, including nested arrays ([][][]string), maps (map[string]string), and combinations thereof. This significantly increases the risk of future regressions related to enum placement in container types. - Resolution: Restore the test case (
TestReflector_Reflect_EnumsPlacement). Update its assertions to expect the correct behavior: enums placed within the nesteditemsoradditionalPropertiesschema, AND enum values having the correct primitive type (e.g.,[1, 2]forintenums, not["1", "2"]).
2.2 Should Fix (P1🟡)
Issue: Underlying Type Mismatch Issue Not Addressed
- Analysis Confidence: High
- Impact: The original problem leading to the revert (numeric enums becoming strings) indicates an issue in how enum values derived from struct tags (strings) are processed and typed. This revert avoids the symptom but doesn't fix the root cause: the lack of type-aware serialization for enum values based on the field's Go type. This underlying issue might surface again in other contexts.
- Suggested Solution: Modify the
enum.loadFromFieldlogic (or where enum values are finalized) to inspect the actual Go type of the field being processed. Convert the string-based enum values from the tag into the correct target type (e.g.,int,float,bool) before assigning them toschema.Enumortarget.Enum.
Issue: Lack of Documentation on Current Enum Limitations
- Analysis Confidence: Medium
- Impact: With this revert, the library's behavior regarding enum placement for container types is now inconsistent with the JSON Schema specification. Users relying on struct tags for enums on arrays/maps might encounter unexpected schema structures or validation issues without clear guidance.
- Suggested Solution: Add documentation (e.g., in the README or code comments near
reflectEnum) explicitly stating the current limitation: enums defined via struct tags on array/map fields are applied to the container itself, not the items/values, and explain the reasoning (temporary state due to type serialization issues) or link to the relevant issue/PR (#126, #124).
2.3 Consider (P2🟢)
Area: Configurable Enum Placement Strategy
- Analysis Confidence: Medium
- Improvement Opportunity: To provide more flexibility and potentially bridge the gap between different interpretations or needs (direct field enum vs. item enum), consider introducing a way to explicitly control enum placement via struct tags. For example,
enum:"a,b"could remain the default (current reverted behavior), whileenumItems:"a,b"or a modifier likejsonschema:"enum=items"could enforce placement on theitemsschema for arrays. This would allow users to opt-in to spec-compliant placement where needed.
2.4 Summary of Action Items
- P0: Fix enum placement for container types (arrays/maps) to comply with JSON Schema spec (apply to
items/additionalProperties). - P0: Restore and update the deleted test case
TestReflector_Reflect_EnumsPlacementto cover nested types with correct type expectations. - P1: Implement type-aware processing of enum values derived from struct tags to fix the root cause of the string serialization bug.
- P1: Document the current (reverted) behavior and its limitations regarding enum placement on container types.
3. Technical Analysis
3.1 Code Logic Analysis
📁 reflect.go - reflectEnum
- Submitted PR Code:
func reflectEnum(schema *Schema, fieldTag reflect.StructTag, fieldVal interface{}) {
enum := enum{}
enum.loadFromField(fieldTag, fieldVal)
if len(enum.items) > 0 {
// highlight-start
// Reverted: Removed searchDeepestSchema call, applying enum directly to the top-level schema
schema.Enum = enum.items
// highlight-end
if len(enum.names) > 0 {
if schema.ExtraProperties == nil {
schema.ExtraProperties = make(map[string]interface{}, 1)
}
schema.ExtraProperties[XEnumNames] = enum.names
}
}
}
// highlight-start
// Reverted: Removed the helper function that found the deepest nested schema
// func searchDeepestSchema(in *Schema) *Schema { ... }
// highlight-end
// enum can be use for sending enum data that need validate.
type enum struct {
items []interface{}
names []string
}- Analysis:
- Current logic reverts to applying the
enumkeyword directly to the schema associated with the Go field, regardless of whether it's a primitive, array, or map. - This fixes the immediate bug where
searchDeepestSchemacombined with type-unaware enum values caused incorrect serialization (e.g.,[]intenums becoming["1", "2"]). - However, for container types (arrays/maps), applying
enumto the container itself violates JSON Schema specification, which expects the constraint on the items/values within the container. - Edge cases involving complex nested types (arrays of arrays, maps of arrays) are no longer handled correctly regarding enum placement.
- Current logic reverts to applying the
- LlamaPReview Suggested Improvements:
import (
"strconv" // Example: needed for string to number conversion
"reflect"
)
func reflectEnum(schema *Schema, fieldTag reflect.StructTag, fieldVal interface{}, fieldType reflect.Type) { // Added fieldType parameter
enum := enum{}
enum.loadFromField(fieldTag, fieldVal)
if len(enum.items) == 0 {
return
}
// Determine the actual target schema for the enum based on type
targetSchema := schema
valueType := fieldType // Type of the value the enum applies to
// Check if the field is an array or slice
if fieldType.Kind() == reflect.Slice || fieldType.Kind() == reflect.Array {
// Ensure Items schema exists
if schema.Items == nil {
schema.Items = &SchemaOrBool{}
}
if schema.Items.SchemaOrBool == nil {
// Initialize based on element type if possible, otherwise default
// This part might need more sophisticated type reflection from Reflector state
schema.Items.SchemaOrBool = &SchemaOrBool{TypeObject: &Schema{}}
}
if schema.Items.SchemaOrBool.TypeObject != nil {
targetSchema = schema.Items.SchemaOrBool.TypeObject
valueType = fieldType.Elem() // Enum applies to the element type
}
// Check if the field is a map (simplified: assumes string values for now)
// TODO: Handle map keys and more complex map value types if necessary
} else if fieldType.Kind() == reflect.Map {
if schema.AdditionalProperties == nil {
schema.AdditionalProperties = &SchemaOrBool{}
}
if schema.AdditionalProperties.SchemaOrBool == nil {
schema.AdditionalProperties.SchemaOrBool = &SchemaOrBool{TypeObject: &Schema{}}
}
if schema.AdditionalProperties.SchemaOrBool.TypeObject != nil {
targetSchema = schema.AdditionalProperties.SchemaOrBool.TypeObject
valueType = fieldType.Elem() // Enum applies to the value type
}
}
// Convert enum values to the correct type based on the valueType
typedEnumItems, err := convertEnumValues(enum.items, valueType)
if err != nil {
// Handle or log error: could not convert enum values to target type
// Maybe default to string or skip enum? For now, assign raw items.
targetSchema.Enum = enum.items // Fallback or log error
} else {
targetSchema.Enum = typedEnumItems // Assign type-corrected enum values
}
// Apply XEnumNames to the schema where the enum is defined
if len(enum.names) > 0 {
if targetSchema.ExtraProperties == nil {
targetSchema.ExtraProperties = make(map[string]interface{}, 1)
}
targetSchema.ExtraProperties[XEnumNames] = enum.names
}
}
// Helper function to convert enum values (interface{}) based on target Go type
func convertEnumValues(items []interface{}, targetType reflect.Type) ([]interface{}, error) {
if len(items) == 0 {
return items, nil
}
convertedItems := make([]interface{}, len(items))
kind := targetType.Kind()
for i, item := range items {
itemStr, ok := item.(string) // Assume items from tag are initially strings
if !ok {
// If item is not string, attempt to use as is or handle error
// This might happen if loadFromField changes
convertedItems[i] = item // Keep original if not string
continue
}
switch kind {
case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
val, err := strconv.ParseInt(itemStr, 10, 64)
if err != nil {
return nil, fmt.Errorf("enum value %q is not a valid integer: %w", itemStr, err)
}
// Convert to specific int size if necessary, or keep as int64/interface{}
convertedItems[i] = val
case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64:
val, err := strconv.ParseUint(itemStr, 10, 64)
if err != nil {
return nil, fmt.Errorf("enum value %q is not a valid unsigned integer: %w", itemStr, err)
}
convertedItems[i] = val
case reflect.Float32, reflect.Float64:
val, err := strconv.ParseFloat(itemStr, 64)
if err != nil {
return nil, fmt.Errorf("enum value %q is not a valid float: %w", itemStr, err)
}
convertedItems[i] = val
case reflect.Bool:
val, err := strconv.ParseBool(itemStr)
if err != nil {
return nil, fmt.Errorf("enum value %q is not a valid boolean: %w", itemStr, err)
}
convertedItems[i] = val
case reflect.String:
convertedItems[i] = itemStr // Already a string
default:
// For other types (structs, interfaces, etc.), enums might not be applicable directly
// Or might require custom handling. Defaulting to string representation.
convertedItems[i] = itemStr
}
}
return convertedItems, nil
}
// Note: reflectEnum would need access to the reflect.Type of the field.
// This might require passing it down from the caller in Reflector.Reflect.- Improvement rationale:
- Technical benefits: This approach addresses both problems: it places the enum constraint correctly within
itemsoradditionalPropertiesfor container types (restoring spec compliance) and attempts to convert the enum values to the correct primitive type based on the Go field's type (fixing the original bug). It uses a new helperconvertEnumValuesfor type conversion. - Business value: Generates schemas that are both spec-compliant for structure and correct in terms of data types, leading to more reliable validation and client generation.
- Risk assessment: Requires careful handling of type reflection and conversion edge cases. The
reflectEnumfunction needs access to thereflect.Typeof the field, which might require minor refactoring in the calling code (Reflector.Reflect). Error handling during conversion needs consideration (e.g., what to do if an enum tag value cannot be parsed as the target type).
- Technical benefits: This approach addresses both problems: it places the enum constraint correctly within
📁 reflect_test.go - TestReflector_Reflect_EnumsPlacement
- Submitted PR Code:
--- a/reflect_test.go
+++ b/reflect_test.go
@@ -2648,41 +2648,10 @@
require.NoError(t, err)
assertjson.EqMarshal(t, `{
"properties":{
- "a":{"description":"Hello world!","type":"string","format":"base64"},
+ "a":{"description":"Hello world!","type":"string","format":"base64"}, // This context seems unrelated to the deleted test below
"b":{"description":"I am a RawMessage."},
"c":{"description":"I am a RawMessage pointer."}
},
"type":"object"
}`, s)
}
-
-func TestReflector_Reflect_EnumsPlacement(t *testing.T) {
- r := jsonschema.Reflector{}
-
- type Check struct {
- A int `json:"a" enum:"1"`
- B []string `json:"b" enum:"check-string"`
- C []withValNamedEnum `json:"c"`
- D [][][]string `json:"d" enum:"d"`
- F map[string]string `json:"f" enum:"f"`
- G map[int]map[string][]int `json:"g" enum:"1"`
- }
-
- got, err := r.Reflect(Check{})
- require.NoError(t, err)
-
- assertjson.EqMarshal(t, `{
- "definitions": {
- "JsonschemaGoTestWithValNamedEnum": {"enum": [""],"type": "string","x-enum-names": ["n:"]}
- },
- "properties":{
- "a":{"enum":["1"],"type":"integer"},
- "b":{"items":{"enum":["check-string"],"type":"string"},"type":["array","null"]},
- "c":{"items":{"$ref":"#/definitions/JsonschemaGoTestWithValNamedEnum"},"type":["array","null"]},
- "d":{"items":{"items":{"items":{"enum":["d"],"type":"string"},"type":"array"},"type":"array"},"type":["array","null"]},
- "f":{"additionalProperties":{"type":"string"},"enum":["f"],"type":["object","null"]},
- "g":{"additionalProperties":{"additionalProperties":{"items":{"type":"integer"},"type":"array"},"type":"object"},"enum":["1"],"type":["object","null"]}
- },
- "type":"object"
- }`, got)
-}- Analysis:
- The removal of
TestReflector_Reflect_EnumsPlacementeliminates tests specifically designed to validate enum handling for various types, including primitives (int), simple arrays ([]string), arrays of custom types, deeply nested arrays ([][][]string), and maps (map[string]string,map[int]map[string][]int). - This leaves a gap in verifying the correctness of enum placement and typing, especially for the complex cases that the original PR #124 attempted to address.
- The removal of
- LlamaPReview Suggested Improvements:
import (
"testing"
"github.com/stretchr/testify/require"
"github.com/swaggest/assertjson"
"github.com/swaggest/jsonschema-go"
)
// Restore the test case
func TestReflector_Reflect_EnumsPlacement(t *testing.T) {
r := jsonschema.Reflector{}
// Keep the original struct definition
type Check struct {
A int `json:"a" enum:"1"`
B []string `json:"b" enum:"check-string"`
// C []withValNamedEnum `json:"c"` // Assuming withValNamedEnum handles its own enum correctly
D [][][]string `json:"d" enum:"d"`
F map[string]string `json:"f" enum:"f"`
G map[int]map[string][]int `json:"g" enum:"1"` // Enum on map keys is complex, focus on values if applicable
}
got, err := r.Reflect(Check{})
require.NoError(t, err)
// Update assertions to expect CORRECT placement and types
// This assumes the suggested fix in reflectEnum is applied
assertjson.EqMarshal(t, `{
"definitions": {
// "JsonschemaGoTestWithValNamedEnum": {"enum": [""],"type": "string","x-enum-names": ["n:"]} // Keep if relevant
},
"properties":{
"a":{"enum":[1],"type":"integer"}, // Expect number 1, not string "1"
"b":{"items":{"enum":["check-string"],"type":"string"},"type":["array","null"]}, // Correct: enum in items
// "c":{"items":{"$ref":"#/definitions/JsonschemaGoTestWithValNamedEnum"},"type":["array","null"]}, // Keep if relevant
"d":{"items":{"items":{"items":{"enum":["d"],"type":"string"},"type":"array"},"type":"array"},"type":["array","null"]}, // Correct: enum in deepest items
"f":{"additionalProperties":{"enum":["f"],"type":"string"},"type":["object","null"]}, // Correct: enum in additionalProperties (for map values)
"g":{"additionalProperties":{"additionalProperties":{"items":{"enum":[1],"type":"integer"},"type":"array"},"type":"object"},"type":["object","null"]} // Correct: enum in deepest items, and number 1
},
"type":"object"
}`, got)
}
// Dummy type for compilation if needed for the test struct
// type withValNamedEnum string
// func (w withValNamedEnum) Enum() []interface{} { return []interface{}{""} }
// func (w withValNamedEnum) EnumNames() []string { return []string{"n:"} }- Improvement rationale:
- Technical benefits: Restores crucial test coverage for various enum scenarios. The updated assertions validate both the correct placement (inside
itemsoradditionalProperties) and the correct typing of enum values (e.g., numbers forintfields). - Business value: Ensures that the library correctly handles a wider range of common enum use cases, preventing regressions and improving reliability for users employing enums with complex data structures.
- Technical benefits: Restores crucial test coverage for various enum scenarios. The updated assertions validate both the correct placement (inside
3.2 Key Quality Aspects
- Testing strategy and coverage: The removal of
TestReflector_Reflect_EnumsPlacementrepresents a significant regression in test coverage for enum functionality, particularly concerning nested types. Restoring and updating this test is critical. The existing CI checks (lint, other tests) passed, but they didn't cover the specific scenarios removed. - Documentation needs: The change in behavior (reverting to potentially non-spec-compliant placement) necessitates updated documentation to manage user expectations and explain the current limitations or trade-offs.
4. Overall Evaluation
- Technical assessment: This PR successfully fixes a critical bug related to the incorrect serialization of primitive enum types (e.g.,
intenums becoming strings). However, it achieves this by reverting to a previous state that is non-compliant with the JSON Schema specification regarding enum placement for container types (arrays/maps). The underlying issue of type-aware enum value processing remains unaddressed. - Business impact: Fixes a significant data type corruption bug, which is valuable. However, the reintroduction of spec non-compliance for nested enums might negatively impact users relying on strict validation or code generation based on the schema structure for arrays/maps. It represents a trade-off between two different kinds of correctness.
- Risk evaluation: High risk of causing issues for users who depend on spec-compliant enum placement within container types. Medium risk that the underlying type-mismatch bug will reappear in other contexts if not addressed directly. Low risk in the sense that it fixes a definite bug reported by the author.
- Notable positive aspects and good practices: Addresses a clear and impactful bug (#124 regression). The revert is straightforward.
- Implementation quality: The revert itself is simple and correct. However, it leaves the codebase in a state with known specification deviations and reduced test coverage for the affected area.
- Final recommendation: Request Changes 🟡. While fixing the primitive type serialization bug is important, reverting to a spec-non-compliant state for container enums and removing test coverage is not ideal. The recommended path is to:
- Re-introduce the logic for placing enums within
items/additionalProperties(similar to the reverted #124). - Crucially, add type-aware conversion logic (as suggested in P1/Code Logic Analysis) to ensure enum values match the target Go type, fixing the original bug of #124.
- Restore and update the deleted test case to validate both correct placement and correct typing.
- Re-introduce the logic for placing enums within
💡 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reverts #124
It turned out this implementation does not work well with actual types (e.g. you get
["1", "2", "3"]for a enum of []int).