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 bool processing; clean date processing #197

Merged
merged 5 commits into from
May 10, 2023
Merged

Fix bool processing; clean date processing #197

merged 5 commits into from
May 10, 2023

Conversation

zephraph
Copy link
Contributor

Changes

  • Bumps zod requirements to at least 3.20
  • Uses coerce to transform dates instead of the custom helper we were using previously
  • Adds a new helper for bools to enable it to parse string types
  • Bumps omicron version and generated output

@zephraph
Copy link
Contributor Author

The IpPoolRange type was failing type check match against its zod inferred type but they're substantively the same. I added some explicit expect errors to ignore them temporarily, but I'll follow up on it later.

@zephraph zephraph merged commit 792f53c into main May 10, 2023
@zephraph zephraph deleted the fix-bool branch May 10, 2023 18:32
@david-crespo
Copy link
Collaborator

The type failure is introduced in Zod 3.21.2. It does not happen with 3.21.1. At least some, maybe all, of these issues are ultimately about this, I think:

colinhacks/zod#2196
colinhacks/zod#2203
colinhacks/zod#2325
colinhacks/zod#2334

@@ -30,6 +25,9 @@ export function generateZodValidators(spec: OpenAPIV3.Document) {
*/
const IntEnum = <T extends readonly number[]>(values: T) =>
z.number().refine((v) => values.includes(v)) as ZodType<T[number]>;

/** Helper to ensure booleans provided as strings end up with the correct value */
const SafeBoolean = z.preprocess(v => v === "false" ? false : v, z.coerce.boolean())
Copy link
Collaborator

Choose a reason for hiding this comment

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

This has the undesirable feature that '0' parses to true. Empty string works ok.

image

I think it would be good to match the API's behavior, which is to error on anything other than 'true' or 'false' or ''.

https://github.com/oxidecomputer/omicron/blob/d997d2e02ef742e0b994090d3289487aac2603e7/nexus/types/src/external_api/params.rs#L151C27-L158

This matches that behavior:

const booleanString = z.enum(['true', 'false', '']).transform((v) => v === 'true')

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.

2 participants