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

Zod Validator #98

Closed
clarknova opened this issue Jun 26, 2023 · 15 comments · Fixed by #164
Closed

Zod Validator #98

clarknova opened this issue Jun 26, 2023 · 15 comments · Fixed by #164

Comments

@clarknova
Copy link

When I use zValidator for validation I encounter a typescript bug.
When I coerce something inside the validator the resulting type is always never.
z.object({quality: z.coerce.number().min(1).max(100)})
So the validation works and the incoming c.req.valid is also correct. It is just the typescript definition that breaks.

@yusukebe
Copy link
Member

@clarknova

Which target do you want to validate? json, form, query, or param?

@clarknova
Copy link
Author

Sorry in my case I wanted to validate query.
I can ofc work around the problem with something like

z.string()
    .transform((e) => +e)
    .pipe(z.number().min(1).max(100)})

Which works as expected. But the coerce Version is so much nicer. Escpially when you throw in stuff like .optional() and .default()

@yusukebe
Copy link
Member

HI @clarknova

Thanks for your response. The behavior is expected because the validation target should be a "string" if it's query, form, or param. Only json allows the types number or boolean. So, if you want to validate a query, you should use transform as you mentioned.

@clarknova
Copy link
Author

Thank you for your reply.
The Problem is that the type is just plain wrong. As I said the code works:
typeof c.valids("query").quality === "number" Evaluates true with the above validation. Only the typescript type evaluates to never. So I think the typing has some issues. I looked into it but crazy typescript generics are not my strong suite.

@yusukebe
Copy link
Member

yusukebe commented Jun 28, 2023

@clarknova

The Problem is that the type is just plain wrong.

The expected behavior that I mentioned is for TypeScript types, not values. If you set the first argument of zValidator as query, it should be treated as string. And if you do not, it will be "never".

@msutkowski
Copy link
Contributor

msutkowski commented Sep 18, 2023

@yusukebe I wanted to bump this issue specifically because of the recent openapi middleware. When using zod as a source of truth of the query schema to generate the api spec, if you think about common query parameters like limit or page, it's really nice to just to have this work. With the implementation of things as they exist currently, it's "broken" as you can't provide an ideal spec without giving up DX.

All of the types for everything are fine until you get to the types for c.req.valid in the handler, which end up being problematic due to the query?: Record<string, string>. So in the case given above where things get transformed, the value extracted from the c.req.valid has the wrong type (the value after being returned from validatorFunc is correct, but the types give back a string). The run time behavior is all correct though, and keeping things as-is would just force casting everywhere.

Is there a solution that you'd recommend for dealing with this, or perhaps I've missed something?

Thanks!

edit: looking through more of the source, I'm not sure why we're defining the return types in ValidationTargets. It makes sense to have the keys, but the current types are just going to wipe anything validators are going to do. Although they're correct for the native hono access (e.g. c.req.query), they're not going to be accurate after passing through a validator that may transform them? 🤔

@yusukebe
Copy link
Member

@msutkowski

I can't understand what you mean well, please give me some snippets as examples.

@msutkowski
Copy link
Contributor

@yusukebe I can make a full repro if you'd like, but this should get the point across. Let me know how I can help more :)

export const exampleApp = new OpenAPIHono();

const exampleRoute = createRoute({
  operationId: "getSomeThings",
  method: "get",
  path: "/things",
  request: {
    query: z
      .object({
        page: z.number().min(1), // 👈 this is important for proper openapi doc generation
        limit: z.number().min(1),
      })
      .partial(),
  },
  responses: {
    200: {
      content: {
        "application/json": {
          schema: z.object({ banana: z.boolean() }),
        },
      },
      description: "woohoo",
    },
  },
});

exampleApp.openapi(
  exampleRoute,
  async (c) => {
    // These come back as`never` due to the type for ValidationTargets. 
    // If you set query: unknown in that type, this works as expected.
    const { page, limit } = c.req.valid("query"); // 👈👈👈👈👈

    return c.jsonT({
      ok: true,
    });
  }
);

As of today, this is the type for ValidationTargets. If you change it to the below, c.req.valid is able to correctly infer the output of the validator function.

export type ValidationTargets = {
  json: any
  form: Record<string, string | File>
  query: Record<string, string | string[]>
  queries: Record<string, string[]> // Deprecated. Will be obsolete in v4.
  param: Record<string, string>
  header: Record<string, string>
  cookie: Record<string, string>
}

to

export type ValidationTargets = {
  json: unknown
  form: unknown
  query: unknown
  queries: Record<string, string[]> // Deprecated. Will be obsolete in v4.
  param: unknown
  header: unknown
  cookie: unknown
}

@yusukebe
Copy link
Member

@msutkowski

Thanks! I can understood well.

But, the input value of query is Record<string, string>, which is no different for OpenAPI. The value used in the handler after a validation will then be of the transformed type. Related: honojs/hono#946

In this OpenAPI case, it is confused because it confuses whether this is a nemeric value or a number type. However, since this Zod OpenAPI is based on Zod, we should think in terms of TypeScript types, i.e. number or string. If we follow it, the value of query will always be a string.

@msutkowski
Copy link
Contributor

@msutkowski

Thanks! I can understood well.

But, the input value of query is Record<string, string>, which is no different for OpenAPI. The value used in the handler after a validation will then be of the transformed type. Related: honojs/hono#946

In this OpenAPI case, it is confused because it confuses whether this is a nemeric value or a number type. However, since this Zod OpenAPI is based on Zod, we should think in terms of TypeScript types, i.e. number or string. If we follow it, the value of query will always be a string.

I don't exactly follow there. When you're in the handler, you've already parsed the param/query/headers/etc otherwise it would've thrown an error. zod is going to start out as an any so it's irrelevant if the input value of c.req.query is a string - from the validator perspective its any or unknown until it's parsed. I'd expect this to just work in the handler because you're running the validator function and assigning it to validatedData. At that point, the return type of c.req.valid('query') should be the parsed input schema type (e.g. limit: number)

What I'm ultimately saying is that there is currently no way to use zod with openapi to generate an expected openapi spec, unless i'm totally missing something. It's extremely common to define page/pageSize/limit/etc as numbers. E.g. the stripe api.

For more context, what I'm trying to do is switch from using this hono-based wrapper for ts-rest I made and just the openapi middleware, but I don't see how it's possible today.

As a reference, you wouldn't be able to create the stripe spec...

{
            "description": "A limit on the number of objects to be returned. Limit can range between 1 and 100, and the default is 10.",
            "in": "query",
            "name": "limit",
            "required": false,
            "schema": {
              "type": "integer"
            },
            "style": "form"
          },

Maybe I'm not understanding something? 🤔

@yusukebe
Copy link
Member

@msutkowski

In that case, please use coerce:

const QuerySchema = z.object({
  page: z.coerce.number().openapi({
    description: 'the page number',
  }),
})

@msutkowski
Copy link
Contributor

msutkowski commented Sep 19, 2023

@msutkowski

In that case, please use coerce:

const QuerySchema = z.object({
  page: z.coerce.number().openapi({
    description: 'the page number',
  }),
})

I should've mentioned that I've tried every way of converting from a string to a number and this still won't work. Here is a quick playground: https://www.typescriptlang.org/play?#code/JYWwDg9gTgLgBAbzgeTAUwHYEEAKBJACQgwgBo4AvcgYyjQEMY0AlCAVybgF84AzKCCDgAiAAIALYhAD0FCABMAtBHQZ6YYMIDcAKFCRYiOOOrc+AocMklp1ADbBMMbTtfViAZ3gCOaOAF44WgYmVl8ACgQdOBi4MEZxAC4RaTQAD3pwOzRhUmjYkDQYSXlk4QBzItz8mLoARzY0L2So2La4BrQoAE9kigA6CAAjACs0ahhImvbpaTgHEGAYPv6vKGAMcvCASjz22IWllfcu6jR+jDYQIa6d-sWMcIBGbfv6NPCAVl3ptq4fv57WJ0DyQDAeJotX6xABMAAY4VD9jF3BgmGikci2sJ1GAHNRGMBiNIRh5iMJMViYh5qOI0CB6CthmMJpE4BAANYrIYQCDZeiPbbcAFUmJcIH7cXQmLyJq0YBgGBEjBlDxsahnDweapYqWAnT-XQ6VFeOC4gJwDBoADuKFUuEIUh2rlxg1U6mA4R8THI9A83QwpnC1CF-gAfIhpib4AA3egOeSMNDyACKjR6Fuo-Xq-TjCfCAHJOj0C9sjW06DA2FAMEF+qTiAAVKZYznJGBQRrTf4G7ZuTzweyONEWkwAHhg3XQEF4ZrAYDD4WE0mEZZ0szaAD0APyuIA

I'll open a PR into the middleware for further discussion :). Edit: I was going to do this, but the monorepo for the middleware just didn't run out of the box for me and didn't have extra to look into why this morning. Hopefully the playground is enough.

@yusukebe
Copy link
Member

@msutkowski

You don't have to do that. Use this code with the fix in #164.

const ParamsSchema = z.object({
  id: z
    .string()
    .transform((val, ctx) => {
      const parsed = parseInt(val)
      if (isNaN(parsed)) {
        ctx.addIssue({
          code: z.ZodIssueCode.custom,
          message: 'Not a number',
        })
        return z.NEVER
      }
      return parsed
    })
    .openapi({
      param: {
        name: 'id',
        in: 'path',
      },
      example: 123,
      type: 'integer',
    }),
})

@msutkowski
Copy link
Contributor

thanks @yusukebe! This works. The only thing worth noting here is that in some cases (like mine), this is still going to be problematic. As an example, this is what I'm doing:

  1. We have N open api schemas for various microservices
  2. We generate zod schemas from the various openapi schemas
  3. We define a strict public API contract and normalize the routing structure in a CF worker
  4. We selectively pick/omit parts of those schemas to define the query/params/body/etc on the gateway

There's more to it, but that's the gist... lots of code generation and trying to get types and schemas to play nicely. All of that to say, it's just easier to deal with things if you can use z.number() or z.coerce() directly instead of the transform :)

Thank you for all of this work!!! 🥇

@colinmcdonald22
Copy link

colinmcdonald22 commented Sep 30, 2023

Hi! Just wanted to chime in: we're using the OpenAPI middleware, and running into this issue.

Defining types as a z.string() and transform'ing does solve it for us, but having to include the transform snippet + override the OpenAPI type + losing the lack of .min/.max/etc. results in significantly more boilerplate code than necessary.

I'm not sure if I'd ever expect z.number() to work directly, as the underlying type is of course a string in query params. However, it seems like z.coerce.number() is reasonable + would let us clean up a lot of redundant transforming

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 a pull request may close this issue.

4 participants