fix: parse schema for complex struct#47
Merged
Merged
Conversation
Summary by CodeRabbit
WalkthroughThe PR refactors schema parsing to comprehensively handle struct-based DTOs with nested field support, JSON tag resolution, required field detection, and proper OpenAPI type mappings. New helper functions enable recursive parsing of nested structs and arrays while supporting validation tags, time formatting, and example metadata. Changes
Sequence DiagramsequenceDiagram
participant User as Caller
participant PS as ParseSchema()
participant Helper as Helper Functions
participant Reflect as Reflect Package
User->>PS: Call ParseSchema(dto struct)
PS->>Reflect: Reflect.TypeOf(dto)
rect rgb(200, 220, 240)
Note over PS,Helper: Pointer dereferencing & struct validation
PS->>Helper: isTimeType()
Helper-->>PS: Check time.Time
end
PS->>Reflect: Iterate struct fields
rect rgb(220, 240, 220)
Note over PS,Helper: Field processing loop
Reflect-->>PS: For each field
PS->>Helper: parseJSONName(field.Tag)
Helper-->>PS: Extract JSON name
PS->>Helper: parseNested(field)
Helper-->>PS: Recursive schema/Items
PS->>PS: Collect Required fields
PS->>PS: Build Properties map
end
PS-->>User: Return SchemaObject with Properties & Required
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
utils.go (1)
35-57: Pointer types are mapped as "object"; dereference before mapping.Current logic reports
*int,*string,*time.Time,*[]Tas"object". This produces incorrect schemas for optional primitives and arrays and breaks time formatting. Fix by unwrapping pointers first, then mapping the underlying kind.Apply this diff to make mapping robust:
func mappingType(val reflect.Type) string { - if val == reflect.TypeOf(time.Time{}) { - return "string" - } - switch val.Kind() { + if val == nil { + return "" + } + // Unwrap pointers (handles *T, **T, *time.Time, *[]T, etc.) + for val.Kind() == reflect.Ptr { + val = val.Elem() + } + if val == reflect.TypeOf(time.Time{}) { + return "string" + } + switch val.Kind() { case reflect.Bool: return "boolean" case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: return "integer" case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: return "integer" case reflect.Float32, reflect.Float64: return "number" case reflect.String: return "string" - case reflect.Pointer, reflect.Struct: + case reflect.Struct: return "object" case reflect.Slice, reflect.Array: return "array" default: return "" } }parse.go (2)
52-58: RequestBody content map key must be a media type, not a schema name.OpenAPI 3.0 uses MIME types (e.g., "application/json") as keys under
content. Using struct names is non‑compliant.- mediaTypes[common.GetStructName(val)] = &MediaTypeObject{ + mediaTypes["application/json"] = &MediaTypeObject{ Schema: &SchemaObject{ Ref: "#/components/schemas/" + common.GetStructName(val), }, }
169-172: Guard against nil Components to avoid panic.
spec.Componentscan be nil; assigningSchemaswill panic.- // spec.Definitions = definitions - spec.Components.Schemas = schemas + // ensure components exists + if spec.Components == nil { + spec.Components = &ComponentObject{} + } + spec.Components.Schemas = schemas spec.Paths = pathObject
🧹 Nitpick comments (4)
type.go (1)
98-103: Expose ItemsObject.Required but parser doesn’t populate it for array-of-structs.You added
ItemsObject.Required []stringandProperties, butparseNesteddoesn’t setItems.Requiredfor arrays of structs. Populate it so item-level required fields are preserved.Follow-up is in
parse.gosuggestions where we setItems.Required = inner.Required. Based on learnings.parse_test.go (1)
61-61: Remove debug print from tests.
fmt.Printfin tests adds noise and can mask issues in CI logs.- fmt.Printf("%+v\n", defintion.Properties) + // no-op: avoid debug prints in testsparse.go (2)
176-260: Solid struct parsing; a few gaps to tighten.
- time.Time format works for non-pointers only; pointer times won’t get
date-time.- Arrays of structs (when not tagged
nested) won’t carry item properties—might be intentional. Fornestedarrays, required fields on the element aren’t propagated toItems.Required.If intentional that only
validate:"nested"triggers deep parsing, ignore the second point. Otherwise, consider auto-nesting for struct fields.To propagate
Requiredfor nested arrays, seeparseNesteddiff below. Also updateisTimeTypeto unwrap pointers (dedicated comment).
264-272: Optional: prefer lowerCamelCase fallback for JSON names.Lowercasing the entire field (
LocationID→locationid) can be surprising. Consider convertingPascalCasetolowerCamelCasewhen nojsontag is present.No diff included to avoid churn; happy to provide a helper if you want to adopt this.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
parse.go(2 hunks)parse_test.go(2 hunks)type.go(1 hunks)utils.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
parse.go (1)
type.go (2)
SchemaObject(81-90)ItemsObject(97-103)
parse_test.go (1)
parse.go (1)
ParseSchema(177-260)
Comment on lines
+144
to
+147
| text, err := json.Marshal(defintion) | ||
| require.Nil(t, err) | ||
| require.Equal(t, `{"type":"object","properties":{"category":{"type":"string","example":"paid-time-off"},"config":{"type":"object","properties":{"accrualPolicy":{"type":"object","properties":{"accrualMethod":{"type":"string","example":"year"},"accrualRates":{"type":"array","items":{"type":"object","properties":{"from":{"type":"integer","example":"0"},"to":{"type":"integer","example":"100"},"value":{"type":"integer","example":"12"}}}}}},"allowedApplyFuture":{"type":"boolean","example":"true"},"annualResetPolicy":{"type":"object","properties":{"date":{"type":"string","example":"2024-01-01"},"type":{"type":"string","example":"calendarDate"}}},"autoApproval":{"type":"object","properties":{"expireDuration":{"type":"integer","example":"72"},"isEnable":{"type":"boolean","example":"true"},"leaveAmount":{"type":"number","example":"3"}}},"carryForwardPolicy":{"type":"object","properties":{"carryForwardRates":{"type":"array","items":{"type":"object","properties":{"from":{"type":"integer","example":"0"},"to":{"type":"integer","example":"100"},"value":{"type":"integer","example":"12"}}}},"expireDuration":{"type":"integer","example":"90"}}},"emailReminder":{"type":"object","properties":{"expireDuration":{"type":"integer","example":"24"},"isEnable":{"type":"boolean","example":"true"}}},"leaveApplicationStart":{"type":"integer","example":"60"},"maxLeaveAmount":{"type":"number","example":"5"},"minLeaveAmount":{"type":"number","example":"0.5"},"newHireProbationPolicy":{"type":"object","properties":{"isEnable":{"type":"boolean","example":"false"},"rules":{"type":"array","items":{"type":"object","properties":{"from":{"type":"integer","example":"0"},"to":{"type":"integer","example":"100"},"value":{"type":"integer","example":"12"}}}}}},"timeUnit":{"type":"string","example":"d"}}},"country":{"type":"string","example":"US"},"locationId":{"type":"string","example":"3fa85f64-5717-4562-b3fc-2c963f66afa6"},"name":{"type":"string","example":"Annual Leave"},"requiredInfo":{"type":"object","properties":{"employeeType":{"type":"string","example":"full-time"},"gender":{"type":"string","example":"male"}}}}}`, string(text)) | ||
| } |
There was a problem hiding this comment.
Avoid brittle JSON string equality; compare structurally.
Map key order in JSON is not guaranteed; exact string compare will be flaky. Compare decoded structures instead (or use JSONEq).
Apply this diff:
- text, err := json.Marshal(defintion)
- require.Nil(t, err)
- require.Equal(t, `{"type":"object","properties":{"category":{"type":"string","example":"paid-time-off"},"config":{"type":"object","properties":{"accrualPolicy":{"type":"object","properties":{"accrualMethod":{"type":"string","example":"year"},"accrualRates":{"type":"array","items":{"type":"object","properties":{"from":{"type":"integer","example":"0"},"to":{"type":"integer","example":"100"},"value":{"type":"integer","example":"12"}}}}}},"allowedApplyFuture":{"type":"boolean","example":"true"},"annualResetPolicy":{"type":"object","properties":{"date":{"type":"string","example":"2024-01-01"},"type":{"type":"string","example":"calendarDate"}}},"autoApproval":{"type":"object","properties":{"expireDuration":{"type":"integer","example":"72"},"isEnable":{"type":"boolean","example":"true"},"leaveAmount":{"type":"number","example":"3"}}},"carryForwardPolicy":{"type":"object","properties":{"carryForwardRates":{"type":"array","items":{"type":"object","properties":{"from":{"type":"integer","example":"0"},"to":{"type":"integer","example":"100"},"value":{"type":"integer","example":"12"}}}},"expireDuration":{"type":"integer","example":"90"}}},"emailReminder":{"type":"object","properties":{"expireDuration":{"type":"integer","example":"24"},"isEnable":{"type":"boolean","example":"true"}}},"leaveApplicationStart":{"type":"integer","example":"60"},"maxLeaveAmount":{"type":"number","example":"5"},"minLeaveAmount":{"type":"number","example":"0.5"},"newHireProbationPolicy":{"type":"object","properties":{"isEnable":{"type":"boolean","example":"false"},"rules":{"type":"array","items":{"type":"object","properties":{"from":{"type":"integer","example":"0"},"to":{"type":"integer","example":"100"},"value":{"type":"integer","example":"12"}}}}}},"timeUnit":{"type":"string","example":"d"}}},"country":{"type":"string","example":"US"},"locationId":{"type":"string","example":"3fa85f64-5717-4562-b3fc-2c963f66afa6"},"name":{"type":"string","example":"Annual Leave"},"requiredInfo":{"type":"object","properties":{"employeeType":{"type":"string","example":"full-time"},"gender":{"type":"string","example":"male"}}}}}`, string(text))
+ text, err := json.Marshal(defintion)
+ require.NoError(t, err)
+ var got, want map[string]any
+ require.NoError(t, json.Unmarshal(text, &got))
+ require.NoError(t, json.Unmarshal([]byte(`{"type":"object","properties":{"category":{"type":"string","example":"paid-time-off"},"config":{"type":"object","properties":{"accrualPolicy":{"type":"object","properties":{"accrualMethod":{"type":"string","example":"year"},"accrualRates":{"type":"array","items":{"type":"object","properties":{"from":{"type":"integer","example":"0"},"to":{"type":"integer","example":"100"},"value":{"type":"integer","example":"12"}}}}}},"allowedApplyFuture":{"type":"boolean","example":"true"},"annualResetPolicy":{"type":"object","properties":{"date":{"type":"string","example":"2024-01-01"},"type":{"type":"string","example":"calendarDate"}}},"autoApproval":{"type":"object","properties":{"expireDuration":{"type":"integer","example":"72"},"isEnable":{"type":"boolean","example":"true"},"leaveAmount":{"type":"number","example":"3"}}},"carryForwardPolicy":{"type":"object","properties":{"carryForwardRates":{"type":"array","items":{"type":"object","properties":{"from":{"type":"integer","example":"0"},"to":{"type":"integer","example":"100"},"value":{"type":"integer","example":"12"}}}},"expireDuration":{"type":"integer","example":"90"}}},"emailReminder":{"type":"object","properties":{"expireDuration":{"type":"integer","example":"24"},"isEnable":{"type":"boolean","example":"true"}}},"leaveApplicationStart":{"type":"integer","example":"60"},"maxLeaveAmount":{"type":"number","example":"5"},"minLeaveAmount":{"type":"number","example":"0.5"},"newHireProbationPolicy":{"type":"object","properties":{"isEnable":{"type":"boolean","example":"false"},"rules":{"type":"array","items":{"type":"object","properties":{"from":{"type":"integer","example":"0"},"to":{"type":"integer","example":"100"},"value":{"type":"integer","example":"12"}}}}}},"timeUnit":{"type":"string","example":"d"}}},"country":{"type":"string","example":"US"},"locationId":{"type":"string","example":"3fa85f64-5717-4562-b3fc-2c963f66afa6"},"name":{"type":"string","example":"Annual Leave"},"requiredInfo":{"type":"object","properties":{"employeeType":{"type":"string","example":"full-time"},"gender":{"type":"string","example":"male"}}}}}`), &want))
+ require.Equal(t, want, got)🤖 Prompt for AI Agents
In parse_test.go around lines 144 to 147, the test currently does a brittle
string equality check of JSON output; replace the exact string compare with a
structural JSON comparison — either use require.JSONEq(t, expectedJSONString,
string(text)) (preferred) or unmarshal both expected JSON and text into
interface{} and use require.Equal(t, expectedObj, actualObj) so key order won’t
break the test.
Comment on lines
+274
to
+276
| func isTimeType(t reflect.Type) bool { | ||
| return t == reflect.TypeOf(time.Time{}) | ||
| } |
There was a problem hiding this comment.
Handle pointer to time.Time.
isTimeType should unwrap pointers so *time.Time gets format: date-time.
-func isTimeType(t reflect.Type) bool {
- return t == reflect.TypeOf(time.Time{})
-}
+func isTimeType(t reflect.Type) bool {
+ if t == nil {
+ return false
+ }
+ for t.Kind() == reflect.Ptr {
+ t = t.Elem()
+ }
+ return t == reflect.TypeOf(time.Time{})
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func isTimeType(t reflect.Type) bool { | |
| return t == reflect.TypeOf(time.Time{}) | |
| } | |
| func isTimeType(t reflect.Type) bool { | |
| if t == nil { | |
| return false | |
| } | |
| for t.Kind() == reflect.Ptr { | |
| t = t.Elem() | |
| } | |
| return t == reflect.TypeOf(time.Time{}) | |
| } |
🤖 Prompt for AI Agents
In parse.go around lines 274 to 276, isTimeType only recognizes time.Time but
not *time.Time; update it to unwrap pointer types before comparison by looping
while t.Kind() == reflect.Ptr { t = t.Elem() } (ensuring t is non-nil) and then
compare against reflect.TypeOf(time.Time{}), so pointer-to-time values are
correctly detected as time types.
Comment on lines
+279
to
+326
| // Handle pointer or slice types | ||
| switch t.Kind() { | ||
| case reflect.Ptr: | ||
| if t.Elem().Kind() == reflect.Struct { | ||
| return ParseSchema(reflect.New(t.Elem()).Interface()) | ||
| } | ||
| case reflect.Slice, reflect.Array: | ||
| elemType := t.Elem() | ||
|
|
||
| switch elemType.Kind() { | ||
| case reflect.Struct: | ||
| // Array of struct | ||
| return &SchemaObject{ | ||
| Type: "array", | ||
| Items: &ItemsObject{ | ||
| Type: "object", | ||
| Properties: ParseSchema(reflect.New(elemType).Interface()).Properties, | ||
| }, | ||
| } | ||
|
|
||
| case reflect.Ptr: | ||
| // Array of pointer to struct | ||
| if elemType.Elem().Kind() == reflect.Struct { | ||
| return &SchemaObject{ | ||
| Type: "array", | ||
| Items: &ItemsObject{ | ||
| Type: "object", | ||
| Properties: ParseSchema(reflect.New(elemType.Elem()).Interface()).Properties, | ||
| }, | ||
| } | ||
| } | ||
| fallthrough | ||
|
|
||
| default: | ||
| // Array of primitive type (string, int, etc.) | ||
| return &SchemaObject{ | ||
| Type: "array", | ||
| Items: &ItemsObject{ | ||
| Type: mappingType(elemType), | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| case reflect.Struct: | ||
| return ParseSchema(reflect.New(t).Interface()) | ||
| } | ||
| return &SchemaObject{Type: mappingType(t)} | ||
| } |
There was a problem hiding this comment.
Handle pointer-to-slice/array and propagate Required for array items.
parseNesteddoesn’t handle*[]T/[]*T; pointer case only covers struct.- Item-level
Requiredis omitted for arrays of structs, despiteItemsObject.Requiredbeing available.
Apply this diff:
func parseNested(v reflect.Value, t reflect.Type) *SchemaObject {
- // Handle pointer or slice types
+ // Handle pointer or slice types
switch t.Kind() {
case reflect.Ptr:
- if t.Elem().Kind() == reflect.Struct {
- return ParseSchema(reflect.New(t.Elem()).Interface())
- }
+ // Deref and re-enter to support *T, *[]T, []*T
+ return parseNested(v, t.Elem())
case reflect.Slice, reflect.Array:
elemType := t.Elem()
switch elemType.Kind() {
case reflect.Struct:
// Array of struct
- return &SchemaObject{
+ inner := ParseSchema(reflect.New(elemType).Interface())
+ return &SchemaObject{
Type: "array",
Items: &ItemsObject{
- Type: "object",
- Properties: ParseSchema(reflect.New(elemType).Interface()).Properties,
+ Type: "object",
+ Properties: inner.Properties,
+ Required: inner.Required,
},
}
case reflect.Ptr:
// Array of pointer to struct
if elemType.Elem().Kind() == reflect.Struct {
- return &SchemaObject{
+ inner := ParseSchema(reflect.New(elemType.Elem()).Interface())
+ return &SchemaObject{
Type: "array",
Items: &ItemsObject{
- Type: "object",
- Properties: ParseSchema(reflect.New(elemType.Elem()).Interface()).Properties,
+ Type: "object",
+ Properties: inner.Properties,
+ Required: inner.Required,
},
}
}
fallthrough
default:
// Array of primitive type (string, int, etc.)
return &SchemaObject{
Type: "array",
Items: &ItemsObject{
- Type: mappingType(elemType),
+ Type: mappingType(elemType),
},
}
}
case reflect.Struct:
return ParseSchema(reflect.New(t).Interface())
}
return &SchemaObject{Type: mappingType(t)}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Handle pointer or slice types | |
| switch t.Kind() { | |
| case reflect.Ptr: | |
| if t.Elem().Kind() == reflect.Struct { | |
| return ParseSchema(reflect.New(t.Elem()).Interface()) | |
| } | |
| case reflect.Slice, reflect.Array: | |
| elemType := t.Elem() | |
| switch elemType.Kind() { | |
| case reflect.Struct: | |
| // Array of struct | |
| return &SchemaObject{ | |
| Type: "array", | |
| Items: &ItemsObject{ | |
| Type: "object", | |
| Properties: ParseSchema(reflect.New(elemType).Interface()).Properties, | |
| }, | |
| } | |
| case reflect.Ptr: | |
| // Array of pointer to struct | |
| if elemType.Elem().Kind() == reflect.Struct { | |
| return &SchemaObject{ | |
| Type: "array", | |
| Items: &ItemsObject{ | |
| Type: "object", | |
| Properties: ParseSchema(reflect.New(elemType.Elem()).Interface()).Properties, | |
| }, | |
| } | |
| } | |
| fallthrough | |
| default: | |
| // Array of primitive type (string, int, etc.) | |
| return &SchemaObject{ | |
| Type: "array", | |
| Items: &ItemsObject{ | |
| Type: mappingType(elemType), | |
| }, | |
| } | |
| } | |
| case reflect.Struct: | |
| return ParseSchema(reflect.New(t).Interface()) | |
| } | |
| return &SchemaObject{Type: mappingType(t)} | |
| } | |
| // Handle pointer or slice types | |
| switch t.Kind() { | |
| case reflect.Ptr: | |
| // Deref and re-enter to support *T, *[]T, []*T | |
| return parseNested(v, t.Elem()) | |
| case reflect.Slice, reflect.Array: | |
| elemType := t.Elem() | |
| switch elemType.Kind() { | |
| case reflect.Struct: | |
| // Array of struct | |
| inner := ParseSchema(reflect.New(elemType).Interface()) | |
| return &SchemaObject{ | |
| Type: "array", | |
| Items: &ItemsObject{ | |
| Type: "object", | |
| Properties: inner.Properties, | |
| Required: inner.Required, | |
| }, | |
| } | |
| case reflect.Ptr: | |
| // Array of pointer to struct | |
| if elemType.Elem().Kind() == reflect.Struct { | |
| inner := ParseSchema(reflect.New(elemType.Elem()).Interface()) | |
| return &SchemaObject{ | |
| Type: "array", | |
| Items: &ItemsObject{ | |
| Type: "object", | |
| Properties: inner.Properties, | |
| Required: inner.Required, | |
| }, | |
| } | |
| } | |
| fallthrough | |
| default: | |
| // Array of primitive type (string, int, etc.) | |
| return &SchemaObject{ | |
| Type: "array", | |
| Items: &ItemsObject{ | |
| Type: mappingType(elemType), | |
| }, | |
| } | |
| } | |
| case reflect.Struct: | |
| return ParseSchema(reflect.New(t).Interface()) | |
| } | |
| return &SchemaObject{Type: mappingType(t)} | |
| } |
🤖 Prompt for AI Agents
In parse.go around lines 279–326, update parseNested to handle
pointer-to-slice/array (e.g., *[]T and *[]*T) and to copy item-level Required
fields when array items are objects: when t.Kind() == reflect.Ptr and
t.Elem().Kind() is Slice or Array, descend into t.Elem() (i.e., treat
pointer-to-slice as slice) and process the element type the same as the
Slice/Array case; in the Slice/Array branch, when building Items for element
kinds Struct or Ptr->Struct, call ParseSchema on the element type, assign the
returned Properties to Items.Properties and also copy its Required slice into
Items.Required; keep the existing fallback for primitive element types using
mappingType. Ensure nil checks before accessing Elem() kinds.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
No description provided.