-
Notifications
You must be signed in to change notification settings - Fork 127
fix(error-handling-middleware): handle str error as ApiError #287
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(error-handling-middleware): handle str error as ApiError #287
Conversation
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.
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
lib/response.js:607
- Consider ensuring that 'errorToSend' is always an instance of Error before passing it to 'app.catchErrors' to maintain consistent error handling.
this.app.catchErrors(errorToSend, this, statusCode, errorDetail);
index.js:360
- [nitpick] The variable name 'wasStringError' may be ambiguous; consider renaming it to something more descriptive like 'isResponseErrorFromString' to improve clarity.
const wasStringError = e instanceof ResponseError && e.originalMessage !== undefined;
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.
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (3)
lib/response.js:595
- The logic for assigning message, statusCode, and errorDetail is non-intuitive. Please add inline comments to clarify how the conditions distinguish between a string error and an error object.
error(code, e, detail) {
lib/response.js:603
- Only converting errors with a string message to ApiError may lead to unexpected handling of non-string errors. Verify that this conditional behavior is intentional and covers all error cases.
const errorToSend = typeof message === 'string' ? new ApiError(message, statusCode, errorDetail) : message;
index.js:355
- ApiError instances are logged in a different branch from other Error instances. Please confirm that this distinction in logging levels is intentional for proper error categorization.
const isApiError = e instanceof ApiError;
Description
Before, when using
res.error(message)
, the message would be sent as string and not as an Error object to the error handling middleware.This PR adds a new error type called
ApiError
and this error will be sent to the error handling middleware.Issue
Fixes #286