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

Can’t distinguish invalid key from invalid input in new variant schema #235

Closed
lo1tuma opened this issue Oct 31, 2023 · 8 comments
Closed
Assignees
Labels
enhancement New feature or request priority This has priority

Comments

@lo1tuma
Copy link
Contributor

lo1tuma commented Oct 31, 2023

Example Schema:

const mySchema = variant('theKey', [object({ theKey: literal('foo'), data: string() }), object({ theKey: literal('bar'), data: number() })]);

When a non-object input is given I get the following issue:

{
  reason: 'type',
  validation: 'variant',
  origin: 'value',
  message: 'Invalid type',
  input: 'foo',
  issues: undefined,
  abortEarly: undefined,
  abortPipeEarly: undefined,
  skipPipe: undefined
}

When an empty object is given, I get the following issue:

{
  reason: 'type',
  validation: 'variant',
  origin: 'value',
  message: 'Invalid type',
  input: {},
  issues: undefined,
  abortEarly: undefined,
  abortPipeEarly: undefined,
  skipPipe: undefined
}

Apart from the input those two issues are basically the same. It would useful to be able to distinguish those cases. The first issue is about the input type being wrong and the second issue is about missing discriminator key. AFAIR zod handles the latter with a dedicated error reason invalid_union_discriminator and also adds the key to the path. IMHO we could even distinguish a missing key from an invalid key.


Update: Just another use-case came to my mind. In case a non-matching discriminator is given (any value except foo or baz in the above example), it might be useful to have the issues array being set with all the issues collected while trying just to parse the discriminator value. So I could implement a formatter generating a message like this: Invalid discriminator, expected one of "foo", "bar", but got "xyz".

@fabian-hiller
Copy link
Owner

Do you have a concrete idea how you would change the information of the issues in case of an invalid discriminator? I try to keep the error information to a minimum, but still provide all the basic data to be able to generate comprehensive error messages afterwards.

@fabian-hiller fabian-hiller self-assigned this Nov 1, 2023
@fabian-hiller fabian-hiller added enhancement New feature or request priority This has priority labels Nov 1, 2023
@lo1tuma
Copy link
Contributor Author

lo1tuma commented Nov 1, 2023

I’ve just double checked how zod is doing it. They have their own issue code and instead of providing nested issues, they extract all possible options.

So I think the following would be sufficient:

  • the first issue can stay as it is, if the input is not an object, then Invalid type is correct
  • the second issue, I think it is best to introduce a new issue type:
{
  reason: 'invalid_variant_key',
  validation: 'variant',
  origin: 'value',
  message: 'Invalid variant key',
  input: {},
  path: [ { type: 'object', input: {}, key: 'theKey', value: undefined, options: [ 'foo', 'bar'] } ],
  issues: undefined,
  abortEarly: undefined,
  abortPipeEarly: undefined,
  skipPipe: undefined
}

I’m not 100% if it is the best idea to add options to the path item, but I find it also a little bit strange that it already contains input and value.

@fabian-hiller
Copy link
Owner

Thank your for the research. I will think about it and improve it with the next version.

@fabian-hiller
Copy link
Owner

I have now looked at it and I think your idea is good. Can you think of a use case where it makes sense that the discriminatior key is not a literal? Because only with a literal can we extract the possible options.

@lo1tuma
Copy link
Contributor Author

lo1tuma commented Nov 3, 2023

I have now looked at it and I think your idea is good.

Just another thought that came to my mind: with the reflection api from #211 using requirement instead of path.options might be a better fit.

Can you think of a use case where it makes sense that the discriminatior key is not a literal?

Personally I never had the need for something different than literal schemas. Here is a zod issue asking for more types, mainly unions or enums of literals. While I think it is a valid use-case I think it would not be hard to flatten the union of literals manually so instead of:

const myShema = variant('theKey', [
    object({ theKey: literal('foo'), data: string() }),
    object({ theKey: union([literal('bar'), literal('baz')]), data: number() })
]);

we could write:

const numberSchema = object({ data: number() });
const myShema = variant('theKey', [
    object({ theKey: literal('foo'), data: string() }),
    merge([object({ theKey: literal('bar') }), numberSchema]),
    merge([object({ theKey: literal('baz') }), numberSchema]),
]);

@fabian-hiller
Copy link
Owner

Thanks again for your research! I really appreciate it! I will think about it and take #211 into consideration.

@fabian-hiller
Copy link
Owner

This is partially fixed in v0.25.0. I don't think I want to add code to collect the output options at the moment.

@fabian-hiller
Copy link
Owner

I rewrote and improved variant in v0.26.0. I believe that this issue is resolved in the latest version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority This has priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants