fabian-hiller / valibot

The modular and type safe schema library for validating structural data 🤖
https://valibot.dev
MIT License
6.33k stars 204 forks source link

Nested variants #794

Closed ZerNico closed 2 months ago

ZerNico commented 3 months ago

Hey @fabian-hiller, i am currently trying to implement a relatively complex form using Valibot for validation. Here is a simplified example of what I am trying to accomplish

import * as v from 'valibot';

const Schema = v.variant('type', [
  v.variant('subType', [
    v.object({
      type: v.literal('yes'),
      subType: v.literal('yes'),
      test1: v.string(),
    }),
    v.object({
      type: v.literal('yes'),
      subType: v.literal('no'),
      test2: v.string(),
    }),
  ]),

  v.variant('subType', [
    v.object({
      type: v.literal('no'),
      subType: v.literal('yes'),
      test3: v.string(),
    }),
    v.object({
      type: v.literal('no'),
      subType: v.literal('no'),
      test4: v.string(),
    }),
  ]),
]);

const result = v.safeParse(Schema, {
  type: 'no',
  subType: 'yes',
});

if (result.issues) {
  console.log(v.flatten(result.issues));
}

This works fine if the data is valid but if it is not it always goes into the first subType variant and just shows the issues of that. Using union also is not really an option because that would give me issues for all different paths but I only want the ones for the current selection.

Something like

v.multiVariant(['type', 'subType'], [
  //...
]),

would probably work but that seems relatively hard to implement.

Is there currently a way to accomplish something like this?

Thanks in advance! :)

fabian-hiller commented 3 months ago

Thank you for reporting this issue. I think it is solvable, but I need more time to look into it. The main problem is that we currently ignore the discriminator for nested variant schemas. If the nested variant schemas have the same discriminator this doesn't matter, but in your case the type key is basically completely ignored.

To fix this, we need some logic that recursively checks or filters the options of nested variant schemas so that a nested variant schema is only executed if it is valid according to the outer discriminator. I know this sounds a bit complicated.

fabian-hiller commented 3 months ago

I will investigate whether it makes more sense to support multiple discriminators, or whether the outer variant schema should check its discriminator in the inner variant schemas, as written in my previous comment. I think the latter leads to more flexibility and should be preferred. I will try to extend the variant schema to support such cases with the next release.

ZerNico commented 3 months ago

The only advantage for multiple discriminators I see is that it makes the code a bit less messy. Depending on the situation it makes the schema less nested. Apart from that the second variant is probably the nicer solution because it allows a lot more flexibility. I think it also matches better to the general way Valibot works.

fabian-hiller commented 3 months ago

I think I could solve the problem, but I need to prettify the code and write tests that cover every possible success and failure case.

fabian-hiller commented 3 months ago

What default error message would you expect in this case? The problem is that both subdiscriminators are invalid, but if we were to return an issue for both, they would contradict each other and confuse the user.

import * as v from 'valibot';

const Schema = v.variant('type', [
  v.variant('subType1', [
    v.object({
      type: v.literal('A'),
      subType1: v.literal('A'),
      test1: v.string(),
    }),
    v.object({
      type: v.literal('B'),
      subType1: v.literal('B'),
      test2: v.string(),
    }),
  ]),
  v.variant('subType2', [
    v.object({
      type: v.literal('A'),
      subType2: v.literal('A'),
      test3: v.string(),
    }),
    v.object({
      type: v.literal('B'),
      subType2: v.literal('B'),
      test4: v.string(),
    }),
  ]),
]);

const result = v.safeParse(Schema, {
  type: 'A',
});
fabian-hiller commented 3 months ago

I think we basically have two options. We could return only the issue for the first subdiscriminator subType1 or we could return a general issue with a subissue for each invalid subdiscriminator. The first option is easier to implement for us and our users, but the second option is "more" correct.

ZerNico commented 3 months ago

I'd say the second option would be the nicer one. I think it's also closer to how variants with a single discriminator work atm? The first one would probably lead to confusion. Also e.g. in a form the design of the form should usually make sure that always at least one variant is hit.

fabian-hiller commented 3 months ago

The second option would mean that we return one general issue with two subissues, one for each missing/invalid discriminator. This is the same approach we currently use with union when we can't uniquely assign the input to one of the union options.

The "problem" with this approach is that users usually do not write code to handle the subissues. Also, generating a useful general issue for two missing/invalid discriminators is complicated. For union, we could just say Invalid type: Expected (string | number) but received true, but with variant we could have two or more missing/invalid discriminators with different received values. Therefore, it is hard to define an appropriate expected and received property for the general issue.

For now, I think we should just return one issue for the first missing/invalid discriminator. This is much easier to implement, probably more useful for the user, and it probably doesn't matter too much since the user should avoid such cases in the first place. For my provided schema, this would return the error message Invalid type: Expected ("A" | "B") but received undefined for the first subdiscriminator subType1.

ZerNico commented 2 months ago

I think that seems reasonable for now just showing the first invalid discriminator. For my use case that would work out fine.

nabn commented 2 months ago

I believe i bumped into a related issue, so noting here fwiw

import * as v from 'valibot';

const LayoutConfig = v.variant('layout', [
  v.object({ layout: v.literal('text-only') }),
  v.object({
    layout: v.picklist(['hero-image', 'side-image']),
    image: v.pipe(v.string(), v.url()),
  }),
  v.object({
    layout: v.literal('carousel'),
    itemType: v.picklist(['deals']),
    deals: v.pipe(
      v.string(),
      v.transform((deals) => deals.split(',')),
      v.array(v.string()),
    ),
  }),
]);

const SplashScreenConfig = v.object({
  screen: v.literal('splashScreen'),
  backgroundColor: v.optional(v.string()),
});

const FeedConfig = v.object({
  screen: v.literal('feed'),
  position: v.pipe(v.union([v.string(), v.number()]), v.transform(Number)),
});

const DealsConfig = v.intersect([
  v.object({
    screen: v.literal('deals'),
    position: v.pipe(v.union([v.string(), v.number()]), v.transform(Number)),
  }),
  LayoutConfig,
]);

const CmsItem = v.intersect([
  v.object({
    id: v.string(),
    title: v.string(),
    url: v.optional(v.pipe(v.string(), v.url())),
    cardDescription: v.string(),
  }),
  v.variant('screen', [SplashScreenConfig, FeedConfig, DealsConfig]),
]);

v.parse(CmsItem, {
  id: '123',
  title: 'Welcome',
  screen: 'deals',
  url: 'https://google.com',
  cardDescription: 'Sample card description',
  layout: 'carousel',
  itemType: 'deals',
  deals: 'abc,def,ghi',
});

and getting

Type 'IntersectSchema<[ObjectSchema<{ readonly screen: LiteralSchema<"deals", undefined>; readonly position: SchemaWithPipe<[UnionSchema<[StringSchema<undefined>, NumberSchema<undefined>], undefined>, TransformAction<...>]>; }, undefined>, VariantSchema<...>], undefined>' is not assignable to type 'VariantOption<"screen">'.
  Property 'key' is missing in type 'IntersectSchema<[ObjectSchema<{ readonly screen: LiteralSchema<"deals", undefined>; readonly position: SchemaWithPipe<[UnionSchema<[StringSchema<undefined>, NumberSchema<undefined>], undefined>, TransformAction<...>]>; }, undefined>, VariantSchema<...>], undefined>' but required in type 'VariantOptionSchema<"screen">'.(2322)
fabian-hiller commented 2 months ago

No, your problem is not related to this issue. intersect is not currently supported as an option of variant. You can try to use union instead of variant:

const CmsItem = v.intersect([
  v.object({
    id: v.string(),
    title: v.string(),
    url: v.optional(v.pipe(v.string(), v.url())),
    cardDescription: v.string(),
  }),
- v.variant('screen', [SplashScreenConfig, FeedConfig, DealsConfig]),
+ v.union([SplashScreenConfig, FeedConfig, DealsConfig]),
]);
fabian-hiller commented 2 months ago

@ZerNico I am pretty close to the final implementation. I will explain you the new algorithm of variant probably tomorrow, so you can give me feedback before I release this change.

fabian-hiller commented 2 months ago

@ZerNico I described how the new algorithm works in #809. Next I will investigate if I want to support multiple discriminator keys in an array as the first argument of variant.

fabian-hiller commented 2 months ago

v0.41.0 is available