middleware icon indicating copy to clipboard operation
middleware copied to clipboard

feat(zod-openapi): implement Zod schema validation on response

Open masnormen opened this issue 2 years ago • 14 comments

Hi, here is my try to implement a fix for #181.

Because this (might be) a breaking change, and not all people might want to use the feature, I implemented them behind the flag strictStatusCode?: boolean, strictResponse?: boolean so we can enable the feature when initializing OpenAPIHono

  • strictStatusCode: basically we read from RouterConfig about the list of possible status codes, and just return an error whenever the schema doesn't contain the status code.
  • strictResponse basically matches the status code, get that status code's schema, then do safeParseAsync against the data.

Honestly I still don't know the best practice in Hono on how throw and/or modify the response (and if this is a minor or major bump). I hope the tests can make it more clear though. Thank you

masnormen avatar Sep 29 '23 13:09 masnormen

🦋 Changeset detected

Latest commit: edd032651dcb591eea97a3dc8d5c201373049d63

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@hono/zod-openapi Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

changeset-bot[bot] avatar Sep 29 '23 13:09 changeset-bot[bot]

Hi @masnormen

Thank for the PR! Please wait a little while, I'll look at it in detail later. This may sound difficult, but I'll see if we can't also validate the "types".

yusukebe avatar Sep 30 '23 12:09 yusukebe

Hi, thank you, it's fine. Before this I've also tried exploring the typing but it seems TypeScript doesn't really support it...

https://stackoverflow.com/a/64266045

https://github.com/microsoft/TypeScript/issues/241

masnormen avatar Sep 30 '23 13:09 masnormen

Yeah. We often can't do what we want to do with TypeScript.

yusukebe avatar Sep 30 '23 13:09 yusukebe

Hi @masnormen

You are right, type validations seem difficult, so let's not consider it this time.

This PR looks good to me, but I am curious about one point about returning an error. If the validation fails, it returns a 500 response, as shown below, but ideally it would be better to allow the user to customize it.

https://github.com/honojs/middleware/blob/edd032651dcb591eea97a3dc8d5c201373049d63/packages/zod-openapi/src/index.ts#L284-L286

https://github.com/honojs/middleware/blob/edd032651dcb591eea97a3dc8d5c201373049d63/packages/zod-openapi/src/index.ts#L300-L308

However, the API can be complex. What do you think about this?

yusukebe avatar Oct 01 '23 21:10 yusukebe

@yusukebe We probably can make .route() take some kind of hook for that purpose.

But at this point, I think there would be a need to make distinction between a beforeHook for the current implemented hook (for request validation) and a new afterHook/responseHook (for response validation.

Should we make OpenAPIHono take a defaultAfterHook, and make .route() take a fourth argument for afterHook? Or maybe overload the third argument to take an object like { beforeHook: ..., afterHook: ... }? I saw Elysia use this type of distinction

masnormen avatar Oct 02 '23 13:10 masnormen

@masnormen

But at this point, I think there would be a need to make distinction between a beforeHook for the current implemented hook (for request validation) and a new afterHook/responseHook (for response validation.

Yeah. I think so. It's good!

Should we make OpenAPIHono take a defaultAfterHook, and make .route() take a fourth argument for afterHook? Or maybe overload the third argument to take an object like { beforeHook: ..., afterHook: ... }? I saw Elysia use this type of distinction

I think either is fine, but the second - { beforeHook: ..., afterHook: ... } might be better. It prevents longer arguments and is easier to handle when the number of arguments increases.

yusukebe avatar Oct 03 '23 09:10 yusukebe

Come to think of it, it's better if this before/after hook stuff is implemented in the main Hono first. But currently I don't have the resource to look into that

masnormen avatar Oct 04 '23 09:10 masnormen

@masnormen

Come to think of it, it's better if this before/after hook stuff is implemented in the main Hono first.

You are right, maybe we should rethink from the design of Hono's Validator. Or we could make “Response” validation only with Zod OpenAPI. Either way, we'd do it without too much haste.

yusukebe avatar Oct 05 '23 13:10 yusukebe

Is there some deadline to merge that PR ? I would like to using that in my project 😄

mandado avatar Dec 07 '23 20:12 mandado

Any news about this PR? @yusukebe

I would need this as well in my project. In fact, I thought it was already a thing when I started using the zod-openapi middleware because you can define the response schema, so it seemed obvious to me that it would validate it. However, I did not expect that it would not be a strict validation, which poses a security issue.

I will continue to validate all my response schemas myself with zod while waiting for the functionality to be integrated. Thank you very much for your work!

ewauq avatar Aug 29 '24 15:08 ewauq

Ah, it's just an idea.

If we can allow a custom JSON serializer in c.json(), we may validate the JSON object in the Response:

app.get('/api', (c) => {
  return c.json(
    {
      message: 'Hi',
      ok: true,
    },
    jsonSerializerWithResponseValidator({
      schema: z.object({
        message: z.string(),
        ok: z.boolean(),
      }),
    })
  )
})

yusukebe avatar Sep 10 '24 06:09 yusukebe

Anyway, it's not good to parse the Response already created, like this PR. So, the best is to add a hook for c.json() to access the object before creating Response.

yusukebe avatar Sep 10 '24 07:09 yusukebe

Recent zod version 3.24 implements the standard schema spec. I'm wondering how does it impact work in this area ?

https://github.com/standard-schema/standard-schema?tab=readme-ov-file#what-schema-libraries-implement-the-spec

belgattitude avatar Jan 25 '25 08:01 belgattitude