Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fails with oneOf #16

Closed
jessekrubin opened this issue Jun 22, 2023 · 13 comments
Closed

Fails with oneOf #16

jessekrubin opened this issue Jun 22, 2023 · 13 comments

Comments

@jessekrubin
Copy link

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

@sinclairzx81
Copy link
Collaborator

@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
Copy link
Collaborator

@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
Copy link
Author

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

@xddq
Copy link
Owner

xddq commented Jun 23, 2023

@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
Copy link
Author

jessekrubin commented Jun 23, 2023

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
Copy link
Author

@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
Copy link
Owner

xddq commented Jun 23, 2023

@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
Copy link
Collaborator

@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
Copy link
Author

@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 xddq mentioned this issue Jun 25, 2023
@xddq
Copy link
Owner

xddq commented Jun 25, 2023

@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.

@xddq xddq closed this as completed Jun 25, 2023
@jessekrubin
Copy link
Author

@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
Copy link
Author

jessekrubin commented Jun 26, 2023

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

@xddq
Copy link
Owner

xddq commented Jun 26, 2023

@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.

xddq added a commit that referenced this issue Jun 27, 2023
## Summary
Implements oneOf support for schema2typebox. More info
[here](#16)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants