refactor: catch body-parser JSON syntax errors and improve global error logs #113
refactor: catch body-parser JSON syntax errors and improve global error logs #113KGFCH2 wants to merge 1 commit into
Conversation
|
@KGFCH2 is attempting to deploy a commit to the Kunal Verma's projects Team on Vercel. A member of the Team first needs to authorize it. |
🚀 PR Received SuccessfullyHello @KGFCH2, Thank you for taking the initiative to contribute to this project. Please ensure that your PR follows all project guidelines properly before requesting review.
|
📝 WalkthroughWalkthroughThe error handler middleware is enhanced to log internal server errors and handle malformed JSON request bodies uniformly. Internal errors are logged to the server console with request details, and parsing syntax errors now return a structured JSON 400 response instead of raw HTML error output. ChangesError Handler Enhancements
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/middlewares/errorHandler.js (1)
16-21: Harden malformed JSON detection inserver/middlewares/errorHandler.jsThe current condition (
err instanceof SyntaxError && err.status === 400 && 'body' in err) matches theexpress.json()/body-parser invalid-JSON error fields, but addingerr.type === 'entity.parse.failed'would prevent accidentally treating unrelatedSyntaxErrors withstatus/bodyas malformed JSON.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/middlewares/errorHandler.js` around lines 16 - 21, Tighten the malformed JSON check in server/middlewares/errorHandler.js by adding a check for err.type === 'entity.parse.failed' to the existing condition that currently reads (err instanceof SyntaxError && err.status === 400 && 'body' in err); update the conditional inside the error handling block (the code that sets statusCode = 400, message = "Invalid JSON syntax in request body.", and errorResponse.message) to only run when err.type equals 'entity.parse.failed' as well, ensuring unrelated SyntaxError instances are not misclassified as malformed JSON.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/middlewares/errorHandler.js`:
- Around line 11-14: The current logging block using if (!err.statusCode ||
err.statusCode === 500) wrongly treats errors without statusCode as 500; move
the console.error call so it runs only after you determine the effective
response status for the error (i.e., after the error-type checks that map
SyntaxError, Mongoose, JWT, etc. to 400/401/409), and log only when the resolved
status is 500; update references to err, req.method and req.originalUrl (and the
errorHandler function) so the message still includes the original request
context and the full error object for genuine internal server errors.
---
Nitpick comments:
In `@server/middlewares/errorHandler.js`:
- Around line 16-21: Tighten the malformed JSON check in
server/middlewares/errorHandler.js by adding a check for err.type ===
'entity.parse.failed' to the existing condition that currently reads (err
instanceof SyntaxError && err.status === 400 && 'body' in err); update the
conditional inside the error handling block (the code that sets statusCode =
400, message = "Invalid JSON syntax in request body.", and
errorResponse.message) to only run when err.type equals 'entity.parse.failed' as
well, ensuring unrelated SyntaxError instances are not misclassified as
malformed JSON.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24c0072c-66f9-4525-9195-12e44166f1cd
📒 Files selected for processing (1)
server/middlewares/errorHandler.js
| // Log 500 internal errors to the server console for debuggability | ||
| if (!err.statusCode || err.statusCode === 500) { | ||
| console.error(`[Error] Internal Server Error on ${req.method} ${req.originalUrl}:`, err); | ||
| } |
There was a problem hiding this comment.
Logging logic incorrectly flags client errors as internal server errors.
The condition !err.statusCode will match errors that don't have a statusCode property, including the newly handled SyntaxError (which has err.status), Mongoose errors, and JWT errors. These will be logged as "Internal Server Error" even though they ultimately return 400/401/409 status codes.
Move the logging after the error-type determination so only genuine 500-level errors are logged.
🔧 Proposed fix
let errorResponse = {
success: false,
message: message
};
- // Log 500 internal errors to the server console for debuggability
- if (!err.statusCode || err.statusCode === 500) {
- console.error(`[Error] Internal Server Error on ${req.method} ${req.originalUrl}:`, err);
- }
-
// Handle express malformed JSON parsing error
if (err instanceof SyntaxError && err.status === 400 && 'body' in err) {
statusCode = 400;
message = "Invalid JSON syntax in request body.";
errorResponse.message = message;
}
// Handle ApiError
else if (err instanceof ApiError) {
statusCode = err.statusCode;
message = err.message;
errorResponse.message = message;
}
// Handle Mongoose ValidationError
else if (err.name === "ValidationError") {
statusCode = 400;
const messages = Object.values(err.errors).map(val => val.message);
message = `Validation Error: ${messages.join(", ")}`;
errorResponse.message = message;
}
// Handle Mongoose CastError (invalid ObjectId)
else if (err.name === "CastError") {
statusCode = 400;
message = `Invalid ${err.path}: ${err.value}`;
errorResponse.message = message;
}
// Handle Mongoose duplicate key error
else if (err.code === 11000) {
statusCode = 409;
const field = Object.keys(err.keyValue)[0];
message = `${field} already exists.`;
errorResponse.message = message;
}
// Handle JWT errors
else if (err.name === "JsonWebTokenError") {
statusCode = 401;
message = "Invalid token.";
errorResponse.message = message;
}
else if (err.name === "TokenExpiredError") {
statusCode = 401;
message = "Token expired.";
errorResponse.message = message;
}
// Handle other errors
else if (err.message) {
message = err.message;
errorResponse.message = message;
}
+ // Log 500 internal errors to the server console for debuggability
+ if (statusCode === 500) {
+ console.error(`[Error] Internal Server Error on ${req.method} ${req.originalUrl}:`, err);
+ }
+
// Include stack trace in development mode🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/middlewares/errorHandler.js` around lines 11 - 14, The current logging
block using if (!err.statusCode || err.statusCode === 500) wrongly treats errors
without statusCode as 500; move the console.error call so it runs only after you
determine the effective response status for the error (i.e., after the
error-type checks that map SyntaxError, Mongoose, JWT, etc. to 400/401/409), and
log only when the resolved status is 500; update references to err, req.method
and req.originalUrl (and the errorHandler function) so the message still
includes the original request context and the full error object for genuine
internal server errors.
1 participant
Related Issue
Closes #112
Summary
Catches bad request body payload parsing formats gracefully.
Changes Made
errorHandler.jsto handleSyntaxErrorwith status400.Summary by CodeRabbit