fix(zod-validator): support for case-insensitive headers target validation#860
Conversation
🦋 Changeset detectedLatest commit: 1eff5c3 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 |
| // create an object that maps lowercase schema keys to lowercase | ||
| const schemaKeys = Object.keys(schema.shape) | ||
| const caseInsensitiveKeymap = Object.fromEntries( | ||
| schemaKeys.map((key) => [key.toLowerCase(), key]) |
There was a problem hiding this comment.
It is a very very slight difference, but toLowerCase() takes longer to execute. Therefore, this PR will cause performance degradation. We would like to avoid that as much as possible. Also, we may need to add the same procedure to other validators if we will add this feature for the Zod Validator.
What do you think about these?
We can merge this PR if we can allow for these pain points.
But if not so, for example, could we encourage documents to use only lowercase? We wrote about it in the Validation section of the website. https://hono.dev/docs/guides/validation
There was a problem hiding this comment.
I think the issue with documentation is it's very easy to miss. For example, I've been using Hono for over 2 years at this point and never knew headers are casted to lower case if you retrieve them through c.req.headers... It's not something that you see in other frameworks a lot, so I think it actually makes sense to solve in a different way
As for a runtime-performant solution I might have an idea. What if restrict the schema keys for headers target to only lowercase characters using TypeScript? Is that something that could work? This way it would just let you know in an IDE that you have a problem with your headers validation
There was a problem hiding this comment.
What if restrict the schema keys for
headerstarget to only lowercase characters using TypeScript? Is that something that could work? This way it would just let you know in an IDE that you have a problem with your headers validation
This is a super interesting idea. We have to find good ways.
|
@yusukebe like i just noticed in #884, people get confused about this. I was not able to find a pure TS approach to this, and tbh it has its performance drawbacks as well. Here's my 5 cents on the performance issue: people using Zod will not notice the difference in performance with this PR as Zod is extremely slow anyways. Additionally, the TS solution I proposed before adds type complexity, which will make the already significant IDE performance issue even worse. Ultimately it's up to you, but I feel like this solution will make things a bit more clear for the general public using this framework. I guess this is the question of tradeoffs between the DX and speed. |
|
Hi @askorupskyy ! Thank you for spending time. As you said, the overhead to convert header values lowercase will be very slight. So, this PR is okay. Let's go with this. |
Issue explained here.
While it does make sense to keep
headerscase-insensitive (aka always casting to lowercase), it's easy to miss when writing your own validation schema. My little implementation makes sure the Zod schema doesn't care for lower/upper casing of the headers and treats them the same.Fixes the issue above, with included test-cases.
Keep in mind that the resulting
schemavalue keeps the casing of the schema, so the casing is preserved.c.req.header()on the other hand would return lower-cased values as intended by hono/core.