[OAS/config-schema] Implement discriminated union#246216
[OAS/config-schema] Implement discriminated union#246216jloleysens wants to merge 9 commits intoelastic:mainfrom
Conversation
jloleysens
added
Team:Core
|
🤖 Jobs for this PR can be triggered through checkboxes. 🚧
ℹ️ To trigger the CI, please tick the checkbox below 👇
|
nickofthyme
left a comment
nickofthyme
left a comment
There was a problem hiding this comment.
Sorry for the duplicate work here but I took a quick stab at this last Thursday. I was under the impression there was no one else working on it.
That said, I have a few concerns with this approach.
There was a problem hiding this comment.
FYI: I also put up a PoC for a discriminated union type here #246095
| function discriminatedOneOf<T extends Array<ObjectType<any>>>( | ||
| discriminator: string, | ||
| types: T, | ||
| options?: DiscriminatedUnionTypeOptions<UnionOfObjectTypes<T>> | ||
| ): Type<UnionOfObjectTypes<T>> { | ||
| return new DiscriminatedUnionType(discriminator, types, options); | ||
| } |
There was a problem hiding this comment.
This typing does not enforce that the schema objects contain the discriminator.
Enforcing this makes the types harder as I could not find a variadic type solution, so I ended up with this.
function oneOfKind<
Discriminator extends string,
A extends PropsWithDiscriminator<Discriminator, Props>,
B extends PropsWithDiscriminator<Discriminator, Props>
>(
discriminator: Discriminator,
types: [ObjectType<A>, ObjectType<B>],
options?: UnionTypeOptions<ObjectResultUnionType<A | B>>
): Type<ObjectResultUnionType<A | B>>;There was a problem hiding this comment.
Yeah, I agree that's a nice improvement!
| private readonly discriminator?: string; | ||
|
|
||
| constructor(discriminator: string, types: RTS, options?: DiscriminatedUnionTypeOptions<T>) { | ||
| let schema = internals.alternatives(types.map((type) => type.getSchema())).match('one'); |
There was a problem hiding this comment.
I don't see how this is discriminated based on the discriminator key, this is just a stricter union.
This approach requires exactly one of the provided branches to match the value. If zero or more than one match, the validation fails.
Compared to using alternatives.conditional, which evaluates conditions in the switch array, picks the first matching branch based on the discriminator (or otherwise if provided). Only that branch is applied all others are ignored.
See my solution here.
There was a problem hiding this comment.
This approach requires exactly one of the provided branches to match the value. If zero or more than one match, the validation fails.
True, this is an exclusive or hidden under a constructor that will ask for a discriminator key - I believe this is essentially what a discriminated union is.
There was a problem hiding this comment.
Yeah but it's not splitting the various schemas by the discriminator. The only discrimination you are doing is within the error handling.
Thus why I think this internal schema is no different than the current oneOf except stricter in that it would not allow multiple matches, much like zod.xor.
There was a problem hiding this comment.
Also from a performance perspective, the internals.alternatives(...).match('one') will not short-circuit when a match is found because it needs to validate exclusivity.
But with alternatives.conditional this is not the case as the exclusivity is implied by the exhaustive nature of the discriminator, or more simply it only ever checks one of the schemas.
There was a problem hiding this comment.
I agree, from an internal implementation perspective it could match this behaviour more optimally, but viewed as a black box they'd work the same, but good point regarding performance!
jloleysens
changed the title
|
@nickofthyme Thanks for leaving your comments and working on this issue! I think it's possible that we may need something of both of our approaches. My main concerns are:
Happy to continue iterating on your PR to resolve correctness nit, @nreese 's use case, OAS introspection |
|
Superseded by #246095 |
3 participants
Summary
Related #181994
TODO