What started out as a simple code duplication removal, turned out into a major refactoring with complete rewriting of error handling, moving of business logic/db access into separate service files (about this in another blog post) and rewriting of all integration tests to use async/await. In this blog post I will focus on the custom error handling and how it made the code much cleaner for the REST API supporting www.bookmarks.dev. The API uses ExpressJS, currently in version 4.
Refactoring
To make my point I will show you an example of before and after code. In the after part where I drill down into the details taking a top down approach.
Before
Our sample candidate is the router method where a personal bookmark is created:
personalBookmarksRouter.post('/', keycloak.protect(), async (request, response) => {
let userId = request.kauth.grant.access_token.content.sub;
if ( userId !== request.params.userId ) {
return response
.status(HttpStatus.UNAUTHORIZED)
.send(new MyError('Unauthorized', ['the userId does not match the subject in the access token']));
}
const bookmark = bookmarkHelper.buildBookmarkFromRequest(request);
if ( bookmark.userId !== userId ) {
return response
.status(HttpStatus.BAD_REQUEST)
.send(new MyError('The userId of the bookmark does not match the userId parameter', ['The userId of the bookmark does not match the userId parameter']));
}
const missingRequiredAttributes = !bookmark.name || !bookmark.location || !bookmark.tags || bookmark.tags.length === 0;
if ( missingRequiredAttributes ) {
return response
.status(HttpStatus.BAD_REQUEST)
.send(new MyError('Missing required attributes', ['Missing required attributes']));
}
if ( bookmark.tags.length > constants.MAX_NUMBER_OF_TAGS ) {
return response
.status(HttpStatus.BAD_REQUEST)
.send(new MyError('Too many tags have been submitted', ['Too many tags have been submitted']));
}
let blockedTags = '';
for ( let i = 0; i < bookmark.tags.length; i++ ) {
const tag = bookmark.tags[i];
if ( tag.startsWith('awesome') ) {
blockedTags = blockedTags.concat(' ' + tag);
}
}
if ( blockedTags ) {
return response
.status(HttpStatus.BAD_REQUEST)
.send(new MyError('The following tags are blocked:' + blockedTags, ['The following tags are blocked:' + blockedTags]));
}
if ( bookmark.description ) {
const descriptionIsTooLong = bookmark.description.length > constants.MAX_NUMBER_OF_CHARS_FOR_DESCRIPTION;
if ( descriptionIsTooLong ) {
return response
.status(HttpStatus.BAD_REQUEST)
.send(new MyError('The description is too long. Only ' + constants.MAX_NUMBER_OF_CHARS_FOR_DESCRIPTION + ' allowed',
['The description is too long. Only ' + constants.MAX_NUMBER_OF_CHARS_FOR_DESCRIPTION + ' allowed']));
}
const descriptionHasTooManyLines = bookmark.description.split('\n').length > constants.MAX_NUMBER_OF_LINES_FOR_DESCRIPTION;
if ( descriptionHasTooManyLines ) {
return response
.status(HttpStatus.BAD_REQUEST)
.send(new MyError('The description hast too many lines. Only ' + constants.MAX_NUMBER_OF_LINES_FOR_DESCRIPTION + ' allowed',
['The description hast too many lines. Only ' + constants.MAX_NUMBER_OF_LINES_FOR_DESCRIPTION + ' allowed']));
}
}
if ( bookmark.shared ) {
const existingBookmark = await Bookmark.findOne({
shared: true,
location: bookmark.location
}).lean().exec();
if ( existingBookmark ) {
return response
.status(HttpStatus.CONFLICT)
.send(new MyError('A public bookmark with this location is already present',
['A public bookmark with this location is already present']));
}
}
try {
let newBookmark = await bookmark.save();
response
.set('Location', `${config.basicApiUrl}private/${request.params.userId}/bookmarks/${newBookmark.id}`)
.status(HttpStatus.CREATED)
.send({response: 'Bookmark created for userId ' + request.params.userId});
} catch (err) {
const duplicateKeyinMongoDb = err.name === 'MongoError' && err.code === 11000;
if ( duplicateKeyinMongoDb ) {
return response
.status(HttpStatus.CONFLICT)
.send(new MyError('Duplicate key', [err.message]));
}
response
.status(HttpStatus.INTERNAL_SERVER_ERROR)
.send(err);
}
});
There are several issues with this code. To name a few:
- for one it is way too long for a single method
- the userId validation in the beginning is a pattern used in all methods protected with Keycloak (btw. this was the trigger of the refactoring)
- if one validation exception occurs the code breaks and sends the response to the caller, potentially missing validation exceptions
- try/catch block around the database access, which is diligently used in the whole code base; I mean it's good that is there, but maybe you can get rid of it
Now let's see the refactoring result.
After
personalBookmarksRouter.post('/', keycloak.protect(), async (request, response) => {
UserIdValidator.validateUserId(request);
const bookmark = bookmarkHelper.buildBookmarkFromRequest(request);
let newBookmark = await PersonalBookmarksService.createBookmark(request.params.userId, bookmark);
response
.set('Location', `${config.basicApiUrl}private/${request.params.userId}/bookmarks/${newBookmark.id}`)
.status(HttpStatus.CREATED)
.send({response: 'Bookmark created for userId ' + request.params.userId});
});
Notice that the method is much shorter.
Remove try/catch around code
The try/catch block has been eliminated, but there is a hack. When an error is thrown it does not go to the error middleware, because express does not support promises currently and you will get an UnhandledPromiseRejectionWarning
. The initial solution was to wrap the async function with a wrapper that would have a catch
call, which would forward the error to the next middleware:
let wrapAsync = function (fn) {
return function(req, res, next) {
// Make sure to `.catch()` any errors and pass them along to the `next()`
// middleware in the chain, in this case the error handler.
fn(req, res, next).catch(next);
};
}
This meant calling the function like the following:
personalBookmarksRouter.post('/', keycloak.protect(), AsyncWrapper.wrapAsync(async (request, response) => {
UserIdValidator.validateUserId(request);
const bookmark = bookmarkHelper.buildBookmarkFromRequest(request);
let newBookmark = await PersonalBookmarksService.createBookmark(request.params.userId, bookmark);
response
.set('Location', `${config.basicApiUrl}private/${request.params.userId}/bookmarks/${newBookmark.id}`)
.status(HttpStatus.CREATED)
.send({response: 'Bookmark created for userId ' + request.params.userId});
}));
Other options are mentioned on javascript - Handling errors in express async middleware - Stack Overflow
But later I found the express-async-errors script, which you only
need to require before start using it:
const express = require('express');
require('express-async-errors');
Then you are good to go - no wrapper needed.
UserId validation
The userId validation was moved to its own file:
let validateUserId = function (request) {
const userId = request.kauth.grant.access_token.content.sub;
if (userId !== request.params.userId) {
throw new UseridTokenValidationError('the userId does not match the subject in the access token');
}
}
Now instead of returning a response, a custom UserIdValidationError
exception is thrown:
class UserIdValidationError extends Error {
constructor(message) {
super(message);
this.name = 'UserIdValidationError'
}
}
The exception is handled by an error-handling middleware in the app.js
file:
app.use(function handleUserIdValidationError(error, req, res, next) {
if (error instanceof UseridValidationError) {
res.status(HttpStatus.UNAUTHORIZED);
return res.send({
httpStatus: HttpStatus.UNAUTHORIZED,
message: error.message
});
}
next(error);
});
Error-handling middleware functions are similar as other middleware functions, except they have four arguments instead of three:
(err, req, res, next)
.
Service method
The service method PersonalBookmarksService.createBookmark
, takes now the task of input validation and saving the data to the database:
let createBookmark = async function (userId, bookmark) {
BookmarkInputValidator.validateBookmarkInput(userId, bookmark);
await BookmarkInputValidator.verifyPublicBookmarkExistenceOnCreation(bookmark);
let newBookmark = await bookmark.save();
return newBookmark;
}
The service is "Express"-free, so I could easily change the API Framework but keep the business logic part intact
Input validation handling
Let's focus now on the input validation handling - BookmarkInputValidator.validateBookmarkInput(userId, bookmark)
:
function validateBookmarkInput(userId, bookmark) {
let validationErrorMessages = [];
if (bookmark.userId !== userId) {
validationErrorMessages.push("The userId of the bookmark does not match the userId parameter");
}
if (!bookmark.userId) {
validationErrorMessages.push('Missing required attribute - userId');
}
if (!bookmark.name) {
validationErrorMessages.push('Missing required attribute - name');
}
if (!bookmark.location) {
validationErrorMessages.push('Missing required attribute - location');
}
if (!bookmark.tags || bookmark.tags.length === 0) {
validationErrorMessages.push('Missing required attribute - tags');
} else if (bookmark.tags.length > constants.MAX_NUMBER_OF_TAGS) {
validationErrorMessages.push('Too many tags have been submitted - max allowed 8');
}
let blockedTags = '';
for (let i = 0; i < bookmark.tags.length; i++) {
const tag = bookmark.tags[i];
if (tag.startsWith('awesome')) {
blockedTags = blockedTags.concat(' ' + tag);
}
}
if (blockedTags) {
validationErrorMessages.push('The following tags are blocked:' + blockedTags);
}
if (bookmark.description) {
const descriptionIsTooLong = bookmark.description.length > constants.MAX_NUMBER_OF_CHARS_FOR_DESCRIPTION;
if (descriptionIsTooLong) {
validationErrorMessages.push('The description is too long. Only ' + constants.MAX_NUMBER_OF_CHARS_FOR_DESCRIPTION + ' allowed');
}
const descriptionHasTooManyLines = bookmark.description.split('\n').length > constants.MAX_NUMBER_OF_LINES_FOR_DESCRIPTION;
if (descriptionHasTooManyLines) {
validationErrorMessages.push('The description hast too many lines. Only ' + constants.MAX_NUMBER_OF_LINES_FOR_DESCRIPTION + ' allowed');
}
}
if(validationErrorMessages.length > 0){
throw new ValidationError('The bookmark you submitted is not valid', validationErrorMessages);
}
}
Notice how the validation failures are now collected, instead of breaking the process flow when one occurs. At the end, if present,
they are all packed and thrown via a custom exception:
class ValidationError extends Error {
constructor(message, validatinErrors) {
super(message);
this.validationErrors = validatinErrors;
this.name = 'ValidationError'
}
}
which is treated by a specialised error-handling middleware down the line:
app.use(function handleValidationError(error, request, response, next) {
if (error instanceof ValidationError) {
return response
.status(HttpStatus.BAD_REQUEST)
.json({
httpStatus: HttpStatus.BAD_REQUEST,
message: error.message,
validationErrors: error.validationErrors
});
}
next(error);
});
Error handling middleware
Please find below the complete error handling middleware:
app.use(function handleNotFoundError(error, req, res, next) {
if (error instanceof NotFoundError) {
return res.status(HttpStatus.NOT_FOUND).send({
httpStatus: HttpStatus.NOT_FOUND,
message: error.message,
error: {}
});
}
next(error);
});
app.use(function handlePublicBookmarkExistingError(error, req, res, next) {
if (error instanceof PublicBookmarkExistingError) {
return res.status(HttpStatus.CONFLICT).send({
httpStatus: HttpStatus.CONFLICT,
message: error.message,
error: {}
});
}
next(error);
});
app.use(function handleUserIdValidationError(error, req, res, next) {
if (error instanceof UseridTokenValidationError) {
res.status(HttpStatus.UNAUTHORIZED);
return res.send({
httpStatus: HttpStatus.UNAUTHORIZED,
message: error.message
});
}
next(error);
});
app.use(function handleValidationError(error, request, response, next) {
if (error instanceof ValidationError) {
return response
.status(HttpStatus.BAD_REQUEST)
.json({
httpStatus: HttpStatus.BAD_REQUEST,
message: error.message,
validationErrors: error.validationErrors
});
}
next(error);
});
app.use(function handleDatabaseError(error, request, response, next) {
if (error instanceof MongoError) {
if (error.code === 11000) {
return response
.status(HttpStatus.CONFLICT)
.json({
httpStatus: HttpStatus.CONFLICT,
type: 'MongoError',
message: error.message
});
} else {
return response.status(503).json({
httpStatus: HttpStatus.SERVICE_UNAVAILABLE,
type: 'MongoError',
message: error.message
});
}
}
next(error);
});
// production error handler
// no stacktraces leaked to user
app.use(function (error, req, res, next) {
if (res.headersSent) {
return next(error)
} else {
res.status(error.status || HttpStatus.INTERNAL_SERVER_ERROR);
res.send({
message: error.message,
error: {}
});
}
});
Instead of having one
app.use(function (error, req, res, next)
middleware with nested if/else statements verifying the error type, I preferred to check for one exception type or else forward it to the next middleware. I think this improved the readability, and it's a pattern I used in some of the routers too
Conclusion
It's not much left to say, I think that Express offers a decent way to handle exceptions. I hope you've learned something from this post and if you have any improvement please leave a comment, or better make a pull request at bookmarks.dev github repo.
During refactoring I researched quite a few links, and I bookmarked the best of them at www.bookmarks.dev with the tagged with expressjs, error-handling, async-await
They will be present shortly at the generated public bookmarks - https://github.com/BookmarksDev/bookmarks
Top comments (0)