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

Throwing errors in preprocess #696

Closed
hampuskraft opened this issue Oct 9, 2021 · 12 comments
Closed

Throwing errors in preprocess #696

hampuskraft opened this issue Oct 9, 2021 · 12 comments
Labels
enhancement New feature or request wontfix This will not be worked on

Comments

@hampuskraft
Copy link

hampuskraft commented Oct 9, 2021

[Deleted for privacy reasons]

@hampuskraft
Copy link
Author

hampuskraft commented Oct 9, 2021

[Deleted for privacy reasons]

@scotttrinh
Copy link
Collaborator

The few times I've played around with preprocess, I actually use zod schemas with the "looser" version of what I expect. For instance, if you're using preprocess to get from a serialized query-parameter style string-heavy object to a parsed object, something like:

// Query parameter schema
const loose = z.object({
  aBoolean: z.string().transform(Boolean),
  aNumber: z.string().transform(Number),
});

// Domain schema
const query = z.object({
  aBoolean: z.boolean(),
  aNumber: z.number(),
});

const fromQueryParams = z.preprocess(
  loose.parse,
  query
);

I have to admit that I haven't used preprocess in a serious manner yet, so there might be others who have come up with something more generic like what you we originally attempting.

@colinhacks
Copy link
Owner

colinhacks commented Oct 12, 2021

Perhaps you should be able to add issues inside preprocess similar to superRefine. I'll look into this. It would save you from having to call coerceString twice.

@JacobWeisenburger JacobWeisenburger added the enhancement New feature or request label Feb 28, 2022
@selimb
Copy link

selimb commented Mar 23, 2022

Perhaps you should be able to add issues inside preprocess similar to superRefine. I'll look into this. It would save you from having to call coerceString twice.

+1 for adding issues in preprocess. @colinhacks Would you be willing to accept a PR on this?

IMO this would make it much more straightforward to build a reusable set of "lenient preprocessors" on top of zod (#804 #1019), while keeping good error messages.

I currently work around it with something like this:

function maybeParseNumber(v: unknown): unknown | number {
  if (typeof v !== "string") {
    return v;
  }
  if (!v) {
    return v; // Otherwise "" is parsed as 0
  }
  const ret = Number(v);
  return isNaN(ret) ? v : ret;
}

// Preprocessors for stringly-typed data.
const zs = {
  number: (augment?: (s: ZodNumber) => ZodNumber) => {
    let schema = z.number({ invalid_type_error: "Not a number", required_error: "Required" });
    schema = augment ? augment(schema) : schema;

    return z.preprocess((v) => maybeParseNumber(v), schema);
  },
}

Notice that:

  • The invalid_type_error needs to be overriden on the inner z.number() schema, otherwise you get the generic Expected number, received string error, which is not ideal for stringly-typed data like query parameters and environment variables.
  • If required_error is not defined, then you get invalid_type_error instead of the default required_error.

Consequently, the only way to allow adding constraints on the inner z.number() schema (like .min()) is the augment callback, e.g.:

zs.number((s) => s.min(5).max(10))

Unfortunately, this means that I can't simply stick a pre-existing number schema into a lenient schema. For instance, I'd like to be able to do:

const age = z.number().lte(5, { message: "this👏is👏too👏big" });

const queryParams = z.object({
  age: zs.number(age)
})

Or even just take a full "strict schema" and automatically loosen it recursively (similar to what znv does), e.g.:

// age is defined as above
const CreatePerson = z.object({
  age: age
});

const CreatePersonLenient = lenient(CreatePerson);
CreatePersonLenient.parse(...);

@morgs32
Copy link
Contributor

morgs32 commented Mar 23, 2022

Perhaps you should be able to add issues inside preprocess similar to superRefine. I'll look into this. It would save you from having to call coerceString twice.

Couldn't the same be said for a transform you want to validate?

@morgs32
Copy link
Contributor

morgs32 commented Mar 29, 2022

Took a stab at the PR for transform(). Wouldn't be hard to do for preprocess? #1056

@selimb
Copy link

selimb commented Mar 30, 2022

@morgs32 I could be wrong, but I doubt adding issues inside transform will be accepted? I'm relatively new to the project, but after reading through #264 and #505 (comment) it looks like .transform by design doesn't want you to throw (or add issues), or in other words .transform must assume it operates on valid data -- for the record I'm not a fan of that decision, but I'm sure the author/maintainers had good reasons.

@morgs32
Copy link
Contributor

morgs32 commented Apr 6, 2022

Well @selimb this would not break from the idea that a transform would ONLY operate on valid data that has been "refined" before it.
The feature here is that as you transform it into new data you can catch new errors that PREVENT transform.
Put another way - the data might be INVALID in that it CANNOT be transformed correctly.

Let's consider this note from the README on .transform():

⚠️ Transform functions must not throw. Make sure to use refinements before the transform to make sure the input can be parsed by the transform.

What if you are doing a transform that might not work? We shouldn't have to look far for a transform that might NOT work.

For example, from the README on async-transforms:

const IdToUser = z
  .string()
  .uuid()
  .transform(async (id) => {
    return await getUserById(id);
  })

If you wrote this - hopefully your PR reviewer says, "what if getUserById doesn't work?" Your answer cannot be, "well .transform must assume it operates on valid data". So I guess we should update the README code snippet:

const IdToUser = z
  .string()
  .uuid()
  .transform(async (id) => {
    return await getUserById(id)
       .catch(e => {
          logger.log('Who would have guessed this person could not be found')
          // zod say you can't throw an error in a transform
          return null
       })
  })
  .refine(v => v, {
     message: 'See that transform above, must be that getUserById did not find our user',
  })

You know what else smells? You don't have access to id in your message. Isn't that a shame?
Here's a playground: https://runkit.com/morgs32/zod-transform

Thoughts?? @colinhacks whaddya think?

@stale
Copy link

stale bot commented Jun 19, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Jun 19, 2022
@stale stale bot closed this as completed Jun 26, 2022
@cyrilchapon
Copy link

@scotttrinh same stuff then inside #330, all of this is actually only OK with parse but not with safeParse

Your — elegant, I admit — example :

// Query parameter schema
const loose = z.object({
  aBoolean: z.string().transform(Boolean),
  aNumber: z.string().transform(Number),
});

// Domain schema
const query = z.object({
  aBoolean: z.boolean(),
  aNumber: z.number(),
});

const fromQueryParams = z.preprocess(
  loose.parse,
  query
);

actually throws on

fromQueryParams.safeParse({ aBoolean: 'hu', aNumber: 'ho' })

Of course

const fromQueryParams = z.preprocess(
  loose.safeParse, // <= here
  query
)

is not an option because preprocess is not intendented to work like this,

and

const fromQueryParams = z.preprocess(
  val => {
    const parsed = loose.safeParse(val)
    // ^ HERE
  },
  query
)

leave me in a doubt of what to do on parsed.error


I think in that last case, ctx inside preprocessors would be a way to go. Any temporary workarounds ?

@cyrilchapon
Copy link

@scotttrinh I think I found a solution, less elegant but working apparently.
Based on your "loose then strict" suggestion; but scattered on each property (to actually make a plain big z.object; .mergeable in the process)

import { z, ZodType } from 'zod'

const stringToNumberSchema = (def: number) => (z.string().default(`${def}`).transform(Number))
const safePreprocessor = <O, Z extends ZodType<O>> (preprocessorSchema: Z) => (val: unknown): O | null => {
  const parsed = preprocessorSchema.safeParse(val)
  if (!parsed.success) {
    return null
  }
  return parsed.data
}

const _paginationSchema = z.object({
  skip: z.preprocess(
    safePreprocessor(stringToNumberSchema(0)),
    z.number().min(0)
  ),
  limit: z.preprocess(
    safePreprocessor(stringToNumberSchema(20)),
    z.number().min(0).max(100)
  )
}).strict()

export type PaginationQuery = z.infer<typeof _paginationSchema>

Can I have your thoughts ?

@adrian-gierakowski
Copy link

Perhaps you should be able to add issues inside preprocess similar to superRefine. I'll look into this. It would save you from having to call coerceString twice.

@colinhacks are you still considering this?

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

No branches or pull requests

8 participants