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

refactor: use celebrate error handler #458

Merged
merged 4 commits into from
Oct 20, 2020
Merged

Conversation

mantariksh
Copy link
Contributor

This PR uses the default celebrate error handler to handle validation errors instead of rolling our own generic one. This gives the client more informative error messages when validation fails. Logging was also improved to include the details of the validation error.

Why not use the celebrate error handler as a middleware?
Using isCelebrateError allows us to retain control over the error object before passing it to the celebrate error handler, which allows us to perform customised actions, e.g. logging. Moreover, I thought it was cleaner to use the celebrate error handler within the if (isCelebrateError) block rather than calling next, because that allows our error handler to retain control of the error object rather than calling next and trusting that the celebrate error handler will be called.

Before

image
image

After

image
image

@karrui
Copy link
Contributor

karrui commented Oct 13, 2020

I thought giving explicit Joi errors to the client is not a good idea, since it gives people who are calling our API programatically too much information about the shape of our API? But then again we are open source so....

@mantariksh
Copy link
Contributor Author

there's nothing secret about our APIs, right? even if we weren't open source, a client could always deduce the required parameters from their network traffic. the point here is that as a general principle of API design, we should strive to give the client informative error messages when their requests fail. (if I'm interpreting the issue correctly haha, @liangyuanruo can correct me if I'm wrong?)

@liangyuanruo
Copy link
Contributor

we should strive to give the client informative error messages when their requests fail.

I think the principle is not to return any information under the hood of the API interface. Telling the caller that a missing/incorrect/inconsistent parameter is OK, revealing internal database error messages is not.

@mantariksh mantariksh changed the base branch from refactor/res-json-3 to develop October 15, 2020 03:24
Copy link
Contributor

@karrui karrui left a comment

Choose a reason for hiding this comment

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

🤩 lgtm

@mantariksh mantariksh merged commit d10380d into develop Oct 20, 2020
@tshuli tshuli mentioned this pull request Oct 20, 2020
@mantariksh mantariksh deleted the refactor/celeb-error branch November 10, 2020 01:51
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