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

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

masnormen
Copy link

@masnormen masnormen commented Sep 29, 2023

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

@changeset-bot
Copy link

changeset-bot bot commented Sep 29, 2023

🦋 Changeset detected

Latest commit: edd0326

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

@masnormen masnormen changed the title feat(zod-openapi): add response Zod schema validation on response feat(zod-openapi): implement Zod schema validation on response Sep 29, 2023
@yusukebe
Copy link
Member

yusukebe commented Sep 30, 2023

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".

@masnormen
Copy link
Author

masnormen commented Sep 30, 2023

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

microsoft/TypeScript#241

@yusukebe
Copy link
Member

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

@yusukebe
Copy link
Member

yusukebe commented Oct 1, 2023

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.

c.res = c.json(result.error, {
status: 500,
})

c.res = c.json(
{
success: false,
error: 'Response code does not match any of the defined responses.',
},
{
status: 500,
}
)

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

@masnormen
Copy link
Author

masnormen commented Oct 2, 2023

@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

@yusukebe
Copy link
Member

yusukebe commented Oct 3, 2023

@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.

@masnormen
Copy link
Author

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

@yusukebe
Copy link
Member

yusukebe commented Oct 5, 2023

@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.

@mandado
Copy link

mandado commented Dec 7, 2023

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

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.

None yet

3 participants