check if the caught error is an instance of HttpError #4190
Conversation
📝 WalkthroughWalkthroughThe pull request focuses on refining error handling within the Changes
Possibly related issues
Possibly related PRs
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## staging #4190 +/- ##
===========================================
- Coverage 12.09% 12.09% -0.01%
===========================================
Files 137 137
Lines 17534 17542 +8
Branches 329 333 +4
===========================================
Hits 2121 2121
- Misses 15413 15421 +8
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/auth-service/models/User.js (2)
1048-1050: Consider optimizing error logging.While the error handling is correct, consider moving the error logging after the HttpError check to avoid logging expected errors.
- logger.error(`🐛🐛 Internal Server Error -- ${error.message}`); if (error instanceof HttpError) { return next(error); } + logger.error(`🐛🐛 Internal Server Error -- ${error.message}`);
1090-1093: Consider optimizing error logging.Similar to the modify method, consider moving the error logging after the HttpError check.
- logger.error(`🐛🐛 Internal Server Error -- ${error.message}`); if (error instanceof HttpError) { return next(error); } + logger.error(`🐛🐛 Internal Server Error -- ${error.message}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/auth-service/models/User.js(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-push-deploy-auth-service
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
src/auth-service/models/User.js (2)
789-792: LGTM! Proper error handling implementation.The error handling logic correctly preserves HttpError instances while wrapping other errors appropriately.
1003-1006: LGTM! Consistent error handling pattern.The error handling implementation maintains consistency with the listStatistics method.
|
Auth-service changes in this PR available for preview here |
Description
Error Handling in Pre-Save Hook:
In the pre-save hook, we were correctly checking for validation errors and creating an instance of HttpError with a specific message related to the field that failed validation. However, when this error is thrown, it is being caught in the static modify function.
Error Propagation:
In the modify function, when an error is caught, we were creating a new HttpError instance with a generic message ("Internal Server Error") and using error.message from the caught error. This results in the original validation error message being lost or replaced by a more generic one.
Final API Response:
The final response shows "Internal Server Error" because that is the message set in the catch block of the modify function, instead of propagating the original validation error.
Changes Made
Testing
Affected Services
Endpoints Ready for Testing
API Documentation Updated?
Summary by CodeRabbit