xddq / schema2typebox

Creating TypeBox code from JSON schemas
MIT License
66 stars 15 forks source link

Fails with `oneOf` #16

Closed jessekrubin closed 1 year ago

jessekrubin commented 1 year ago

get Error: JSON schema had invalid value for 'type' attribute. Got: undefined

sinclairzx81 commented 1 year ago

@xddq Heya, You can emit for the oneOf case using something like the following.

TypeScript Link Here

import { Type, Static, TSchema, TUnion, SchemaOptions } from '@sinclair/typebox'

// -------------------------------------------------------------------------------------------------
// If the schema contains 'oneOf`, it's possible to emit this support type using Type.Unsafe. Just
// be mindful that the TypeCompiler does not support `oneOf`, so the following definition is only
// useful when Ajv is used for validation.
// -------------------------------------------------------------------------------------------------
const OneOf = <T extends TSchema[]>(oneOf: [...T], options: SchemaOptions = {}) => 
  Type.Unsafe<Static<TUnion<T>>>({ ... options, oneOf })

const T = Type.Object({
  x: OneOf([Type.String(), Type.Number(), Type.Boolean()])
})

type T = Static<typeof T>

Similar techniques can be used to emit for { enum: ['A', 'B', 'C'] } schematics also.

TypeScript Link Here

import { Type, Static, SchemaOptions } from '@sinclair/typebox'

const StringEnum = <const T extends string[]>(values: [...T], options: SchemaOptions = {}) => 
  Type.Unsafe<T[number]>({ ... options, enum: values })

const T = Type.Object({
  x: StringEnum(['A', 'B', 'C'])
})

type T = Static<typeof T>

It should be possible to handle many non-mappable schemas by emitting these support types at the top of the generated output. Additionally, there are actually ways to get these working with the TypeCompiler, but you might want to put custom type setup behind a CLI flag. Let me know if you want some info on how to do that.

Hope this helps! S

sinclairzx81 commented 1 year ago

@xddq Actually, just for completeness and reference, the following includes the registry setup to support OneOf and StringEnum validation.

import { Type, Kind, TypeRegistry, Static, TSchema, TUnion, SchemaOptions } from '@sinclair/typebox'

// --------------------------------------------------------------------
// Registry
// --------------------------------------------------------------------
TypeRegistry.Set('StringEnum', (schema: any, value) => schema.enum.includes(value))

TypeRegistry.Set('OneOf', (schema: any, value) => 1 === schema.oneOf.reduce((acc: number, schema: any) => acc + (Value.Check(schema, value) ? 1 : 0), 0))

// --------------------------------------------------------------------
// Support Types
// --------------------------------------------------------------------
const StringEnum = <T extends string[]>(enum_: [...T], options: SchemaOptions = {}) => Type.Unsafe<T[number]>({ ... options, [Kind]: 'StringEnum', enum: enum_ })

const OneOf = <T extends TSchema[]>(oneOf: [...T], options: SchemaOptions = {}) => Type.Unsafe<Static<TUnion<T>>>({ ...options, [Kind]: 'OneOf', oneOf })

// --------------------------------------------------------------------
// Example
// --------------------------------------------------------------------

import { Value } from '@sinclair/typebox/value'

const T = Type.Object({
  x: OneOf([Type.String(), Type.Number(), Type.Boolean()]),
  y: StringEnum(['A', 'B', 'C'])
})

type T = Static<typeof T>

console.log(Value.Check(T, { x: 42, y: 'B' })) // true

Cheers S

jessekrubin commented 1 year ago

@sinclairzx81 With your consistently speedy, thorough and well-written responses to issues I am curious when you find the time to sleep!

xddq commented 1 year ago

@jessekrubin Hey!

Thanks for raising the issue! Would be cool to provide additional information the next time to reduce time spent on issues and PRs. Was my bad, I did now decide to simply create an issue template for the future ones.

I got some questions. I am actually quite bad at json schema so bare with me :p

  1. Why are you using oneOf instead of anyOf? Would an implicit conversion to anyOf do it for you? If not, why is that the case? I need some help understanding the benefits of oneOf.
  2. Could you provide a minimal example schema mimicing your use case?

@sinclairzx81 Hey!

thanks for hopping in and your responses! I am curious.. Your implementation for oneOf seems to be pretty straight forward.

  1. Why is it not part of typebox itself? oneOf is in the json schema spec and seems to be reasonly "simple".
  2. Can the typecompiler be used with your approach?
  3. What do you think about an implicit rewrite to anyOf? I would want to avoid losing the ability to use the typecompiler for the generated code.
jessekrubin commented 1 year ago

Thanks for raising the issue!

Ack! Sorry! I have been in your shoes many a time! I was fried yesterday when I made the bug.

I was trying to auto-generate a type-box definition for this schema: https://proj.org/en/9.2/schemas/v0.6/projjson.schema.json

jessekrubin commented 1 year ago

@xddq I think that there are technically key-differences with one-of vs any-of in that a piece of data should really only validate successfully against one as opposed to multiple(?) but I am not sure.

xddq commented 1 year ago

@jessekrubin no worries. Thanks for the example! Yeah, the swagger/openapi page actually give a good rundown with examples for oneOf which made me somewhat understand its use. src

Will probably wait for update from sinclair for next steps

sinclairzx81 commented 1 year ago

@xddq Hi

Why is it not part of typebox itself? oneOf is in the json schema spec and seems to be reasonly "simple".

The oneOf and anyOf schema representations are mostly equivalent. The primary difference is that oneOf validation will fail if more than one sub schema matches a given value. Because of this, oneOf sub-schemas generally require a discriminator field (literal property) or distinct types to prevent multiple sub schema matches. It's this specific semantic detail that prevents TypeBox from implementing it (because unions in TypeScript do not mandate distinct types). For example

type T  = string | string                            // This type is fine (assert for string)
const T = Type.AnyOf([Type.String(), Type.String()]) // This schema is fine (assert for string)
const T = Type.OneOf([Type.String(), Type.String()]) // This schema is illogical (will always fail)

For TypeBox to handle certain compositions (Enum, KeyOf, Conditional Types, TemplateLiterals, Indexers, etc), it also needs to be able to pivot around a singular schema representation describing union types. The anyOf representation best captures TS union semantics (it's essentially 1-1) so it uses that. The inclusion of an additional union representation would require TB to reconcile multiple union type compositions while factoring the distinct type semantics of oneOf which are simply not present in TypeScript. It's really for these reasons TB doesn't support oneOf.

Can the typecompiler be used with your approach?

Yes, the following will make the OneOf schema available to the TypeCompiler, Errors and Value.Check modules.

TypeRegistry.Set('OneOf', (schema: any, value) => {
  return 1 === schema.oneOf.reduce((acc: number, schema: any) => acc + (Value.Check(schema, value) ? 1 : 0), 0)
})

Note: The TypeCompiler cannot generate optimized code for custom types (currently). Instead, the compiled output will call out to a __custom(...) function which is passed into the evaluation scope. If generating standalone validators, it's possible to implement the __custom function explicitly. I wouldn't expect this to be issue for schema > TB transforms tho.

What do you think about an implicit rewrite to anyOf? I would want to avoid losing the ability to use the typecompiler for the generated code.

Yeah, you could implicitly rewrite oneOf to just be TUnion (which may be the easiest approach to get this working), however it's important to be mindful of the differences between the two representations (and you might want to put something in the documentation about it). In practice though, any logical oneOf schema is a perfectly valid anyOf (where any discriminator used in the oneOf case to prevent sub-schema match would work exactly the same in the anyOf case, nothing is lost), so it's probably ok to just map oneOf to TUnion.

Hope this helps!

jessekrubin commented 1 year ago

@sinclairzx81 again, fantastic detailed response!

I think I noticed you have a oneOf thingy in typebox's example/experimental dir. Do you think its a problem that can be cracked?

PS: I just cleaned-up/published/made-public this thing I have been using for a while (https://github.com/jessekrubin/geobox) and was curious to know what you all think (if you have a spare minute to take a look).

xddq commented 1 year ago

@jessekrubin mhh, looks interesting but I think that it lacks documentation. I did not really understand the use case and usage by going through the readme.

Closing this for now since a PR was created here. Will probably be released as 1.2.0.

jessekrubin commented 1 year ago

@xddq thanks for the feed back! Totally agree. I didn't really add much of any documentation (yet!).

Use case (if you are curious) is custom geo (right now geojson) schemas where you can set the schema for the properties field of geojson features. It pairs quite nicely with fastify's fast-json-stringify. Was planning on adding a bunch of other geo related schemas to the package.

I just put it (the package) out there quickly to show you and @sinclairzx81 for some feedback.

PS: I work in the world of geo-sciences.

jessekrubin commented 1 year ago

@xddq did you ever look at forking https://github.com/StefanTerdell/json-schema-to-zod?

xddq commented 1 year ago

@jessekrubin I will keep it in the back of my head if I get to do stuff with geo-json. Did not to do much with it yet.

I did not look at forking json-schema-to-zod, no. However after going over it, it looks like I should be using json-schema-ref-parser for $ref resolution :]

If there is anything, please jump to the discussions or create a new issue.