By Adrian Matei
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 into the details
taking a top down approach.
Before
Our sample candidate is the router method where a personal bookmark was created:
/**
* CREATE bookmark for user
*/
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 mentioned in the beginning of the post)
- 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 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
Code size reduction
Not only that the code is much cleaner now, it takes also less Javascript code (from 3441 to 3012), although the number of Javascript files
has grown by 9 (from 32 to 41). To measure this I used cloc – brew install cloc for MacOS
**Before **
$ cloc $(git ls-files)
69 text files.
69 unique files.
18 files ignored.
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
JSON 5 0 0 7964
JavaScript 32 502 125 3441
YAML 3 8 36 351
Markdown 5 114 0 224
Dockerfile 1 11 18 17
-------------------------------------------------------------------------------
SUM: 46 635 179 11997
-------------------------------------------------------------------------------
** After **
$ cloc $(git ls-files)
68 text files.
68 unique files.
18 files ignored.
github.com/AlDanial/cloc v 1.84 T=0.36 s (159.2 files/s, 37246.4 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
JSON 6 0 0 8603
JavaScript 41 713 186 3007
YAML 3 8 36 351
Markdown 6 127 0 257
Dockerfile 1 11 18 17
-------------------------------------------------------------------------------
SUM: 57 859 240 12235
-------------------------------------------------------------------------------
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-api github repo.
During refactoring I researched quite a few links, and I bookmarked the best of them at www.bookmarks.dev
with the following tags [expressjs] [error-handling] [async-await]
They will be present shortly at the generated public bookmarks at https://github.com/CodepediaOrg/bookmarks.
Cleaner code in an Express REST API with unified error handling was originally published by CodepediaOrg at CodepediaOrg on December 02, 2019.
Source:: codingpedia.org
