xddq / schema2typebox

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

Feature/implement not #21

Closed xddq closed 1 year ago

xddq commented 1 year ago

Summary

implements not from JSON schema by extending the Typebox Registry similar to what is done for "oneOf". using Type.Not().

Would be happy with quick feedback on this if something seems of.

sinclairzx81 commented 1 year ago

@xddq Hi! This is an interesting one :D

The implementation you have looks good, and the default inference of unknown is probably the best option here, but you may be able to use TypeBox's Type.Not(N, T) for this case. The output schematic wouldn't be 1-1 with the input, but would still validate the same.

// input
const N = { not: { type: 'string' } } // ExtendedNot

// typebox
const N = Type.Not(Type.String(), Type.Unknown()) // infer as unknown

// output
const N = { allOf: [{ not: { type: 'string' } }, {}] // TypeBox.Not

The second argument is a little strange, but it represents the allowed type (unknown or any in this case). The Not usage above basically says, "for all possible types in the unknown (infinite) set, disallow strings". There's a bit more of a write up around the type documented on the 0.26.0 release notes here, but at a high level, the Type.Not is TB's attempt at negated types (not supported by TS, but maybe one day!)

Open to thoughts (or follow up questions around Type.Not), The ExtendedNot would be good to retain the input schematic though.

Your PR has actually got me wondering if I should implement an overload for Type.Not() to just accept just the disallowed type (and automatically just apply unknown for the allowed type). Might push an update for this today.

Keep me posted! S

xddq commented 1 year ago

@sinclairzx81 Hey!

Yeah, interesting indeed. What a bummer that I did not check for Type.Not in typebox :D

Generating different json schemas under the hood

While I think this is a valid point and I would prefer it to not generate a structurally different (yet semantically correct) JSON schema representation I won't put work into this because I already noticed that it creates a different representation for Type.Enum (and probably for others as well..?). Also I am not aware of the use cases for having the input schema and the generated schema have the same structure when it validates correctly. I prefer leaning on typebox to handle this for me.

Type.Not

Anyway, updated the code to use Type.Not with Type.Unknown as default argument for now. I will stick to this until I get some real world json schemas provided and then am able to easily/efficiently go for a deeper dive to implementing Type.Not(), when required.

It works alright using something like


    {
      "type": "object",
      "properties": { "x": { "not": { "type": "number" } } },
      "required": ["x"]
    }

generating


      Type.Object({
        x: Type.Not(Type.Number(), Type.Unknown()),
      });

Overloading Type.Not

While it sounds good at first, I think when manually creating the Typebox validations/schemas, the user should know or think about how its data will look like. This is somewhat denied when defaulting to Type.Unknown() and (in my opinion) just makes the users code/validation worse.

Outcome

If you say this is alright and approve I will merge this first implementation and wait for feedback by users. Will probably be released as 1.4.0.

Feel free to hit "squash and merge" :p

sinclairzx81 commented 1 year ago

@xddq Hey thanks for the response and insights!

While I think this is a valid point and I would prefer it to not generate a "structurally different (yet semantically correct) JSON schema representation"

You know, you raise a VERY valid point here, and I agree! Originally the type was written (rather quickly) to reconcile a negated type back to a TS type for a project I'd been working on, but I do think the TB representation is just wrong (at a minimum expressing way more than it should). Having given this more thought after the previous response, I do think there is a much better way to handle this type (outlined below) that solves both inference and representation (win win)

Note: I probably can't change the representation on 0.28.0 (may need to defer to 0.29.0). For now, the current version of TNot<T, TUnknown> should be ok to go with (with the view TB will fix this up the representation in subsequent revisions).

The changes required in future to update in schema2typebox will be to remove the Type.Unknown() from the second parameter.

Type.Not 2.0

So, below is the TB change for TNot (which is really inline with your previous ExtendedNot type), where rather than Type.Not(N, T) representing the allOf, users can construct that themselves using explicit intersection. The inference still works in this case as TS intersections will narrow for unknown for more refined types....for example.

type T = unknown & number // type T = number

Here is the updated TNot type to support this (inline with ExtendedNot), plus inference example using intersect.

TypeScript Link Here

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

// -----------------------------------------------------------
// Not Version 2.0
// -----------------------------------------------------------

export interface TNot<T extends TSchema> extends TSchema, SchemaOptions {
  [Kind]: 'Not'
  static: unknown
  not: T
}
export function Not<T extends TSchema>(not: T, options: SchemaOptions = {}): TNot<T> {
  return { ...options, [Kind]: 'Not', not } as any
}

const T = Type.Intersect([ // all numbers except 1, 2, 3
  Not(Type.Literal(1)),
  Not(Type.Literal(2)),
  Not(Type.Literal(3)),
  Type.Number()
])

type T = Static<typeof T>  // type T = number (because (unknown & unknown & unknown & number))

I'm pretty keen to roll this through as soon as I can, but yeah, would be breaking change (and currently waiting on couple of libraries downstream to take on the 0.28.0 revision before working on 0.29.0). Happy to go with the Type.Not<N, T> for now, but will keep you posted as soon as I'm able to push this change through.

I'll leave the squash/merge honors to you! :) Great work!! S

xddq commented 1 year ago

Alright, yeah it felt somewhat of, thanks!

Also thanks for keeping me posted when 0.29. hits and the required change!