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

fix: implement discriminations correctly in zod #1907

Merged
merged 3 commits into from
Feb 18, 2025

Conversation

Georgegriff
Copy link
Contributor

@Georgegriff Georgegriff commented Feb 18, 2025

Status

READY

Description

Fixes #1906

This PR makes use of Discriminated unions in zod instead of chaning .or the main advantage of this is that zod can provide more accurate error messages as it will use the discriminator property to decide which zod schema to pick for the the rest of the validation of fields.

Before

export const testResponseItem = zod.object({
 "breed": zod.enum(['Labradoodle']),
 "cuteness": zod.number()
}).or(zod.object({
 "breed:" zod.enum(['Dachshund']),
 "length": zod.number()
}))
export const testResponse = zod.array(testResponseItem)

After

export const testResponseItem = zod.discriminatedUnion('breed', [zod.object({
 "breed": zod.enum(['Labradoodle']),
  "cuteness": zod.number()
}),zod.object({
 "breed:" zod.enum(['Dachshund']),
  "length": zod.number()
})])
export const testResponse = zod.array(testResponseItem)

By using '.or' you can end up with confusing messages like "invalid breed Dachshund" when switching a form between the types e.g. imagine having a radio button field with current value of Dachshund with validation errors .e.g. length not a number, and then you switch the "radio" to "Labradoodle" which changes the form field, the validation can get confused because it doesn't know which schema it should validate against because a .or cannot discriminate

Todos

  • Tests
  • Documentation
  • Changelog Entry (unreleased)

) {
// store the discriminator property name in the separator to be used when building the zod schema
// adding discriminator property name is already handled via the deference function
separator = `discriminator__${schema.discriminator.propertyName}`;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

open to better ways of doing this... but with the separator only being a string I was struggling to come up with a way of saving this "metadata"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally don't have a more clever way but maybe @soartec-lab does?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no discomfort with this either 👍

cuteness: {
type: 'integer',
},
// in the real runner breed is added by getApiSchemas in import-open-api.ts, inferred from the discriminator
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its unfortunate that i can't do a full integration style test with this because there is some pre-processing of the schema in the core code which handles the expanding of the breed properties onto the types specifies in the discriminator

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed in CI logs this spec was invalid and the name made me take a look, thought i'd fix it
image

@Georgegriff Georgegriff marked this pull request as ready for review February 18, 2025 16:29
@Georgegriff
Copy link
Contributor Author

Georgegriff commented Feb 18, 2025

@melloware My first pass is ready to be reviewed whenever time allows

@melloware melloware merged commit 1018f0c into orval-labs:master Feb 18, 2025
2 checks passed
@soartec-lab
Copy link
Member

Hi @Georgegriff
We updated the sample application with #1885 and noticed an error. cloud you fix this?

Error:  samples/hono/hono-with-zod/src/petstore.zod.ts: SyntaxError: Expression expected. (17:4)
Error:    15 |   "cuteness": zod.number(),
Error:    16 |   "breed": zod.enum(['Labradoodle'])
Error:  > 17 | }),.strict(),zod.object({
Error:       |    ^
Error:    18 |   "length": zod.number(),
Error:    19 |   "breed": zod.enum(['Dachshund'])
Error:    20 | }),.strict()]).or(zod.object({

@Georgegriff
Copy link
Contributor Author

Yep i'll have a look now

@Georgegriff
Copy link
Contributor Author

Georgegriff commented Feb 21, 2025

the .strict(): I see why this caused the bug, i would have thought .strict() should have been chained onto the object but its been separated for some reason, bizarre.
image

I think i can fix this though, i probably shouldn't be doing flatMap here
image
and need to merge properly.

@Georgegriff
Copy link
Contributor Author

@soartec-lab PR for fix is here: #1923

@Georgegriff Georgegriff deleted the fix/discriminatedUnions branch February 21, 2025 13:01
@soartec-lab
Copy link
Member

@Georgegriff
Thanks for your quickly fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Zod: discriminatedUnion
3 participants