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

Fix #26 Validation Error Response #28

Merged
merged 4 commits into from
Aug 8, 2022
Merged

Fix #26 Validation Error Response #28

merged 4 commits into from
Aug 8, 2022

Conversation

sinclairzx81
Copy link
Contributor

@sinclairzx81 sinclairzx81 commented Jul 30, 2022

Checklist

This PR updates the TypeBoxValidatorCompiler to return values of FastifySchemaValidationError on validation fail. This to prevent Fastify from crashing due to throwing an exception within the compiler validate callback. As of this PR, invalid data passed to the route will now result in a correct 400: Bad Request and an appropriate error object is returned on the response with a message hinting at what data is incorrect.

Note

I note that Fastify currently doesn't appear to directly export the FastifyValidationResult and FastifySchemaValidationError types. These types are accessible by importing fastify/types/schema however. I have left a comment here to potentially update the implementation with a TS assertion on the error result (for correctness), however the current error result seems to generate adequate errors with the properties specified.

Submitting for community review

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

I think we should return those types from Fastify

@sinclairzx81
Copy link
Contributor Author

@mcollina Hi, is it possible to have this PR merged through or are there some upstream considerations around potentially exporting the FastifyValidationResult and FastifySchemaValidationError types first? Would be happy to update this PR to include the commented type assertion mentioned in the PR description if the latter is the case.

@mcollina
Copy link
Member

mcollina commented Aug 6, 2022

nothing, this could land and ship. Exporting the other things would be a bonus.

I'm on vacation until Monday and I'm falling behind on releases.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina
Copy link
Member

mcollina commented Aug 7, 2022

Can you rebase? There is a conflict.

@sinclairzx81
Copy link
Contributor Author

@mcollina upstream changes have been merged.

@mcollina mcollina merged commit 9c5d509 into fastify:main Aug 8, 2022
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.

3 participants