-
Notifications
You must be signed in to change notification settings - Fork 939
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
refactor: Added input validation using zod #790
Conversation
@Dhruwang is attempting to deploy a commit to the formbricks Team on Vercel. A member of the Team first needs to authorize it. |
Thank you for following the naming conventions for pull request titles! 🙏 |
📦 Next.js Bundle Analysis for @formbricks/webThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dhruwang Looks pretty good 😊💪 Great job! Just left a few small comments
try { | ||
schema.parse(value); | ||
} catch (error: any) { | ||
throw new ValidationError("Validation failed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we provide more information in the error?
I think it makes sense to also print the zod result of the validation to the console so we can debug if an error occurs. e.g. in apps/web/app/api/v1/client/responses/[responseId]/route.ts
we do:
const inputValidation = ZResponseInput.safeParse(responseInput);
if (!inputValidation.success) {
return responses.badRequestResponse(
"Fields are missing or incorrectly formatted",
transformErrorToDetails(inputValidation.error),
true
);
}
Let's maybe also do a safeParse here and if !inputValidation.success
console.error the error message and throw the validation error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that makes sense and it will also keep our validation logic consistent !
@Dhruwang Please also fix merge conflicts 😊 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dhruwang thanks a lot for the changes 😊 Looks great! 💪
* added input validation using zod * changed console.log to console.error * fix formatting issues --------- Co-authored-by: Matthias Nannt <mail@matthiasnannt.com>
* added input validation using zod * changed console.log to console.error * fix formatting issues --------- Co-authored-by: Matthias Nannt <mail@matthiasnannt.com>
* added input validation using zod * changed console.log to console.error * fix formatting issues --------- Co-authored-by: Matthias Nannt <mail@matthiasnannt.com>
What does this PR do?
Adds input validation to services
Fixes 1175
Type of change
How should this be tested?
Check services
Checklist
pnpm build
console.logs
git pull origin main