sinclairzx81 / typebox

Json Schema Type Builder with Static Type Resolution for TypeScript
Other
4.78k stars 152 forks source link

Issue with Intersect since 0.26.x #368

Closed isnifer closed 1 year ago

isnifer commented 1 year ago

@sinclairzx81 Found an issue after upgrade to 0.26.8 on my schema:

The value of 'merged_0d336815-953f-439f-a6e9-330d90e31152#/properties/view' does not match schema definition.

Started investigating and realised that something has been broken with Intersection since 0.26.x. I have created a repro that does not throw the same error message, but shows the difference between 0.25.x and 0.26.x: https://github.com/isnifer/typebox-issue-repro

Basically my schema looks like:

const ThrowsOnTwentySixType = Type.Intersect([
  Type.Object({
    view: Type.Literal("a"),
  }),
  Type.Object({
    view: Type.Union([
      Type.Literal("b"),
      Type.Literal("c"),
    ]),
  }),
]);
sinclairzx81 commented 1 year ago

@isnifer Hi!

Thanks for reporting. This is actually a known breaking change in 0.26.0. Unfortunately, I haven't been able to retain backwards compatibility for this on 0.26.0 in a way that's tenable to the future of the library. The reason for the change is quite complicated (which ill try and outline below). But see the solution for aligning your structure to 0.26.0 below.

Problem

The heart of the problem stems from "how one can interpret an intersection of overlapping properties of varying types" (which would be the view property in this case you've presented). TypeBox 0.24.0 & 0.25.0 would interpret overlapping properties of this kind as a indirect form of union. This interpretation proved incorrect (both from a TypeScript and JSON Schema standpoint) and was at odds with the idealized allOf schema representation which TypeBox needed to move to (but was pending a lot of dependent infrastructure to make work)

To try and show the issue, consider the following where there are two possible interpretations (A and B),

// This is your type in TS form
type T = { view: 'a' } & { view: 'b' | 'c' }  

// This is the 0.25.0 interpretation (taking a union of overlapping properties)
type A = { view: 'a' | ('b' | 'c') } 

// This is the 0.26.0 interpretation
type B = { view: never }

So, TypeBox 0.25.0 would take the A interpretation but has since changed to B in 0.26.0. This may seem surprising at first, however the issue with the union becomes more clear when you reduce the logical expression into standalone string literal values.

type T = 'a' & ('b' | 'c')

// TypeScript will infer `A` as `never` because the expression ('a' AND `('b' OR 'c'))` is illogical.
type A = 'a' & ('b' | 'c') //  type A = never

// Attempting to swap & for | breaks the intended expression
type B = 'a' | ('b' | 'c')   // type A = 'a' | 'b' | 'c'  (wrong)
         //  ^
         //  swapping & for | changes the semantics of this expression

So, this is a known breaking change in 0.26.0. There were attempts to create a fallback for users via the new Type.Composite function (which was a verbatim 0.25.0 implementation of Intersect), however users have reported inference performance problems as TypeScript needs to work really hard to produce the correct union output type for this representation. This was reverted early on 0.26.2 (with the 0.26.2 change meaning no backwards compatibility was possible).

The changes for both of these are documented at the links below.

Intersect Representation Change https://github.com/sinclairzx81/typebox/blob/master/changelog/0.26.0.md

Composite Revert https://github.com/sinclairzx81/typebox/blob/master/changelog/0.26.2.md

Solution

The following will work on 0.26.0. Here we extract the View into it's own type containing all possible literal values. This is then assigned to the view property for each intersected object.

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

const View = Type.Union([
    Type.Literal('a'),
    Type.Literal('b'),
    Type.Literal('c')
])

const ThrowsOnTwentySixType = Type.Intersect([
  Type.Object({
    view: View,
  }),
  Type.Object({
    view: View
  }),
]);

type T = Static<typeof ThrowsOnTwentySixType>

Apologies for the breakage here. It's actually been a long outstanding issue in TypeBox to get intersections working correctly using allOf. However what's implemented on 0.26.0 is the complete intersect implementation which will be supported into the future, and towards an official 1.0 release later in the year.

Hope this helps! S

sinclairzx81 commented 1 year ago

@isnifer Hey, just a quick follow up on this.

If you need to retain the your current schematics, I've quickly dropped a legacy intersect implementation in the repository for you to use. This will work the same as 0.25.0, but you will need to copy and paste the legacy implementation into your project.

The legacy intersect implementation can be found at the following link:

https://github.com/sinclairzx81/typebox/blob/master/example/legacy/intersect.ts

Example

import { IntersectLegacy } from './legacy'
import { Type } from '@sinclair/typebox'

const ThrowsOnTwentySixType = IntersectLegacy([
  Type.Object({
    view: Type.Literal("a"),
  }),
  Type.Object({
    view: Type.Union([
      Type.Literal("b"),
      Type.Literal("c"),
    ]),
  }),
]);

Value.Check(ThrowsOnTwentySixType, { view: 'a' }) // true
Value.Check(ThrowsOnTwentySixType, { view: 'b' }) // true
Value.Check(ThrowsOnTwentySixType, { view: 'c' }) // true
Value.Check(ThrowsOnTwentySixType, { view: 'd' }) // false

Will close off this issue for now as the 0.26.0 intersect is a documented breaking change, but if you have any questions (either on the 0.26.0 compatible implementation, or the legacy implementation), please feel free to reply on this thread.

Again, hope this helps! S

isnifer commented 1 year ago

@sinclairzx81 I really appreciate your well-described response and I'll try to change my schema to follow typebox changes. Thank you