-
Notifications
You must be signed in to change notification settings - Fork 187
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
feat(zod-openapi): implement Zod schema validation on response #184
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: edd0326 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
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". |
Hi, thank you, it's fine. Before this I've also tried exploring the typing but it seems TypeScript doesn't really support it... |
Yeah. We often can't do what we want to do with TypeScript. |
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. middleware/packages/zod-openapi/src/index.ts Lines 284 to 286 in edd0326
middleware/packages/zod-openapi/src/index.ts Lines 300 to 308 in edd0326
However, the API can be complex. What do you think about this? |
@yusukebe We probably can make But at this point, I think there would be a need to make distinction between a Should we make OpenAPIHono take a |
Yeah. I think so. It's good!
I think either is fine, but the second - |
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 |
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. |
Is there some deadline to merge that PR ? I would like to using that in my project 😄 |
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 I will continue to validate all my response schemas myself with |
Ah, it's just an idea. If we can allow a custom JSON serializer in app.get('/api', (c) => {
return c.json(
{
message: 'Hi',
ok: true,
},
jsonSerializerWithResponseValidator({
schema: z.object({
message: z.string(),
ok: z.boolean(),
}),
})
)
}) |
Anyway, it's not good to parse the |
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 initializingOpenAPIHono
strictStatusCode
: basically we read fromRouterConfig
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 dosafeParseAsync
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