fix(go/plugins/googlegenai): fixed nullable in array type in JSON schema#5162
fix(go/plugins/googlegenai): fixed nullable in array type in JSON schema#5162
Conversation
There was a problem hiding this comment.
Code Review
This pull request enhances the Google GenAI plugin's schema conversion logic to support JSON Schema 2020-12 features, specifically handling "type" arrays (e.g., ["string", "null"]) and improving "anyOf" subschema resolution. It also introduces a comprehensive test suite for these conversion scenarios. The review feedback identifies opportunities to make the "anyOf" logic more robust by recursively validating subschemas and to extend "type" field support to include []string for better compatibility with Go-defined schemas.
| for _, subSchema := range anyOfList { | ||
| if subSchemaType, hasType := subSchema["type"]; hasType { | ||
| if typeStr, isString := subSchemaType.(string); isString && typeStr != "null" { | ||
| if title, ok := genkitSchema["title"]; ok { | ||
| subSchema["title"] = title | ||
| } | ||
| if description, ok := genkitSchema["description"]; ok { | ||
| subSchema["description"] = description | ||
| } | ||
| // Found a schema like: {"type": "string"} | ||
| return toGeminiSchema(originalSchema, subSchema) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The current logic for finding a valid sub-schema in anyOf only checks if the type field is a string. This will skip sub-schemas that use the newly supported array type format (e.g., {"type": ["string", "null"]}). A more robust approach is to recursively call toGeminiSchema and check if the resulting schema has a concrete type. This also simplifies the logic by leveraging the existing type parsing and normalization.
for _, subSchema := range anyOfList {
if v, ok := genkitSchema["title"]; ok {
subSchema["title"] = v
}
if v, ok := genkitSchema["description"]; ok {
subSchema["description"] = v
}
res, err := toGeminiSchema(originalSchema, subSchema)
if err == nil && res != nil && res.Type != 0 {
return res, nil
}
}| case []any: | ||
| for _, t := range tv { | ||
| s, isString := t.(string) | ||
| if !isString { | ||
| return nil, fmt.Errorf("schema 'type' array contains non-string element of type %T", t) | ||
| } | ||
| if s == "null" { | ||
| schema.Nullable = genai.Ptr(true) | ||
| continue | ||
| } | ||
| if typeStr != "" { | ||
| return nil, fmt.Errorf("schema 'type' array contains multiple non-null types: %v", tv) | ||
| } | ||
| typeStr = s | ||
| } |
There was a problem hiding this comment.
To be consistent with the anyOf handling and support schemas constructed directly in Go code, the type field should also accept []string in addition to []any.
case []any, []string:
var list []any
if l, ok := tv.([]any); ok {
list = l
} else {
for _, s := range tv.([]string) {
list = append(list, s)
}
}
for _, t := range list {
s, isString := t.(string)
if !isString {
return nil, fmt.Errorf("schema 'type' array contains non-string element of type %T", t)
}
if s == "null" {
schema.Nullable = genai.Ptr(true)
continue
}
if typeStr != "" {
return nil, fmt.Errorf("schema 'type' array contains multiple non-null types: %v", tv)
}
typeStr = s
}
Fixes #5070.
Checklist (if applicable):