fix: add json schema transformation logic#1715
fix: add json schema transformation logic#1715404Wolf wants to merge 8 commits intoopenai:masterfrom
Conversation
404Wolf
changed the title
src/lib/transform-json-schema.ts
Outdated
| if (Array.isArray(anyOf)) { | ||
| jsonSchema['anyOf'] = anyOf.map((variant) => _transformJSONSchema(variant as JSONSchema)); | ||
| } else if (Array.isArray(oneOf)) { | ||
| jsonSchema['anyOf'] = oneOf.map((variant) => _transformJSONSchema(variant as JSONSchema)); | ||
| } else if (Array.isArray(allOf)) { | ||
| jsonSchema['allOf'] = allOf.map((entry) => _transformJSONSchema(entry as JSONSchema)); | ||
| } else { | ||
| if (type === undefined) { | ||
| throw new Error('JSON schema must have a type defined if anyOf/oneOf/allOf are not used'); | ||
| } | ||
|
|
||
| switch (type) { | ||
| case 'object': { | ||
| const properties = pop(jsonSchema, 'properties') || {}; | ||
| jsonSchema['properties'] = Object.fromEntries( | ||
| Object.entries(properties).map(([key, propSchema]) => [ | ||
| key, | ||
| _transformJSONSchema(propSchema as JSONSchema), | ||
| ]), | ||
| ); | ||
| break; | ||
| } | ||
| case 'array': { | ||
| const items = pop(jsonSchema, 'items'); | ||
| if (items !== undefined) { | ||
| jsonSchema['items'] = _transformJSONSchema(items as JSONSchema); | ||
| } | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I can't remember why we originally considered these mutually exclusive, but I think we shouldn't, json schema is wild and all of these properties could be set at the same time.
There was a problem hiding this comment.
This is crazy, yes it seems to be true huh. There is also "not"
src/lib/transform-json-schema.ts
Outdated
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I think you're missing a case for additionalProperties as well, which is boolean | Schema
src/lib/transform-json-schema.ts
Outdated
There was a problem hiding this comment.
This technically makes it part of the public API, but I don't think we should make it public until we've decided on the "unsupported properties to description" behaviour, could you move this to src/lib/internal or src/internal?
| const shouldHaveType = [anyOf, oneOf, allOf, not].some(Array.isArray); | ||
| if (!shouldHaveType && type === undefined) { | ||
| throw new Error('JSON schema must have a type defined if anyOf/oneOf/allOf are not used'); | ||
| } |
There was a problem hiding this comment.
q: whats the value provided in validating this?
There was a problem hiding this comment.
It was for consistency with what we do in anthropic, but yes we don't have to do this to get rid of all the anyOfs
| } | ||
|
|
||
| switch (type) { | ||
| case 'object': { |
There was a problem hiding this comment.
nit: we shouldn't branch on the type here, properties could in theory be set when type is not for example, we can just ignore the type entirely imo
| $refStrategy: 'extract-to-root', | ||
| nullableStrategy: 'property', | ||
| }); | ||
| return transformJSONSchema( |
There was a problem hiding this comment.
q: is this necessary for v3? maybe it would make sense for consistency but I think we'll already do the oneOf -> anyOf transform in the vendored to-json-schema library we use (I could be wrong)
| }) as JSONSchema, | ||
| ) as Record<string, unknown>; | ||
| return transformJSONSchema( | ||
| toStrictJsonSchema( |
There was a problem hiding this comment.
oh wait we already have toStrictJsonSchema() 🤦 we should use that instead of introducing the new transformJSONSchema() function
There was a problem hiding this comment.
Huh yeah you're right.
It looks like it also checks that the root is an object, which is slightly different behavior between v3 and v4, and if we change it users will now get vanilla Errors rather than 400s OpenAIErrors. I think that that's probably fine though
2 participants
Changes being requested
Fixes #1709
Additional context & links