I'm trying to implement a zotero-like app using express.js and I've got a problem.
I actually don't know exactly what the problem is, but based on the logs I get, I understood that somehow my middlewares don't execute in the order they are set and also the request leaks into another route handler I've set for errors.
this is my route handler for Collections:
router
.route('/')
.post(
controller.addLibraryToBody,
controller.validateBody.create,
controller.createOne,
controller.sendResponse('create'),
controller.debugLog
);
these are the middlewares:
// in the parent Controller class
moveReqKeyToBody(bodyKey: string, ...nestedReqKey: string[]) {
return function (req: IRequest, res: Response, next: NextFunction) {
let iterator: any = req;
nestedReqKey.forEach(key => {
if (iterator[key]) iterator = iterator[key];
else
next(createError(400, `missing item from request: ${nestedReqKey}`));
});
req.body[bodyKey] = iterator;
next();
};
}
preventMaliciousBody(bodyValidationKeys: BodyValidationKeys) {
let { mandatory = [], allowed = [] } = bodyValidationKeys;
return function (req: Request, res: Response, next: NextFunction) {
allowed = allowed.concat(mandatory);
if (
mandatory.every(value => req.body[value]) &&
Object.keys(req.body).every(value => allowed.includes(value))
)
next();
else next(createError(400, 'invalid body'));
};
}
createOne = catchAsync(
async (
req: IRemoveFieldsRequest,
res: Response,
next: NextFunction
): Promise<void> => {
const document = await this.model.create(req.body);
if (req.removeFields) {
req.removeFields.forEach(field => {
document[field] = undefined;
});
}
req[this.modelName] = document;
next();
}
);
sendResponse = (operation: CRUD) => {
return (req: IRequest, res: Response, next: NextFunction) => {
switch (operation) {
case 'create':
res.status(201).json({
status: 'success',
data: req[this.modelName]
});
break;
}
};
};
debugLog(req: IRequest, res: Response, next: NextFunction) {
console.log(
`${Date.now()} - ${req.url} - ParamKeys: ${Object.keys(
req.params
)} - BodyKeys: ${Object.keys(req.body)}`
);
next();
}
// in the CollectionController
addLibraryToBody = this.moveReqKeyToBody('parent', 'library', 'id');
validateBody = {
create: catchAsync(
async (req: IRequest, res: Response, next: NextFunction) => {
if (!req.body.type) req.body.type = collectionTypes.collection;
this.preventMaliciousBody(this.bodyKeys.create)(req, res, next);
if (
req.body.type === collectionTypes.searchingCollection &&
!req.body.searchQuery
)
next(createError(400, 'invalid body'));
else next();
}
)
}
and this is my app.js:
app
.use('/api', apiRouter)
.use(
'*',
function (req: Request, res: Response, next: NextFunction) {
// todo fix this weird bug
if (res.headersSent) {
console.log(req.url);
console.log(req.body);
console.log(req.params);
} else next();
},
Controller.unavailable
)
.use(errorHandler);
this is postman's output on that route:
{
"status": "success"
}
this is the server's cli output: I have morgan middleware running(first line)
POST /api/libraries/6447a4c4dc088d6d43204668/collections 201 6.358 ms - 20
1683371139354 - / - ParamKeys: id - BodyKeys: name,parent,type
/
{
name: 'norma coll',
parent: '6447a4c4dc088d6d43204668',
type: 'Collection'
}
{ '0': '/api/libraries/6447a4c4dc088d6d43204668/collections' }
I tried logging everything, and searched the web but didn't understand what's wrong.
EDIT: I added 2 console.logs to the createOne method:
createOne = catchAsync(
async (
req: IRemoveFieldsRequest,
res: Response,
next: NextFunction
): Promise<void> => {
console.log('started creating...');
const document = await this.model.create(req.body);
if (req.removeFields) {
req.removeFields.forEach(field => {
document[field] = undefined;
});
}
req[this.modelName] = document;
console.log('created');
next();
}
);
and it was printed before and after the morgan's log:
started creating...
POST /api/libraries/6447a4c4dc088d6d43204668/collections 201 23.049 ms - 20
created
1683387954094 - / - ParamKeys: id - BodyKeys: name,parent,type
/
{
name: 'nor coll',
parent: '6447a4c4dc088d6d43204668',
type: 'Collection'
}
{ '0': '/api/libraries/6447a4c4dc088d6d43204668/collections' }
So I guess there's a problem here? maybe it doesn't get executed completely and therefore calls the next middlewares?
It looks like the main control problem is in validateBody.create(). You are executing:
this.preventMaliciousBody(this.bodyKeys.create)(req, res, next);
But, assuming that it's synchronous as you continue executing right after it. Instead, it isn't done until it calls next() internal to its implementation. You can use nested middleware like this by sending your own callback for next instead of the actual next. That way, you can see when it's actually done by when your callback gets called.
validateBody = {
create: catchAsync(
async (req: IRequest, res: Response, next: NextFunction) => {
if (!req.body.type) req.body.type = collectionTypes.collection;
this.preventMaliciousBody(this.bodyKeys.create)(req, res, (err) => {
if (err) {
next(err);
return;
}
if (
req.body.type === collectionTypes.searchingCollection &&
!req.body.searchQuery
) {
next(createError(400, 'invalid body'));
} else {
next();
}
}));
}
)
}
There are also several coding errors in moveReqKeyToBody().
Inside a .forEach(), you can call next(createError(400, ...)); multiple times and then after the .forEach() loop, you then call next(). You need to code it so that next(...) is only ever called once.
Though I can't really tell what this function is supposed to be doing or if this is entirely responsible for the problem you are asking about, you can start by fixing the above two issues like this:
// in the parent Controller class
moveReqKeyToBody(bodyKey: string, ...nestedReqKey: string[]) {
return function (req: IRequest, res: Response, next: NextFunction) {
let iterator: any = req;
for (let key: any of nestedReqKey) {
if (iterator[key]) {
iterator = iterator[key];
} else {
next(createError(400, `missing item from request: ${nestedReqKey}`));
return;
}
}
req.body[bodyKey] = iterator;
next();
};
}
I also notice that sendResponse() doesn't do anything if the switch operation is not create. You should at least have a default arm to that switch that sends a response or triggers an error. If you're always expecting the argument to be create, then you wouldn't even need the switch statement. So, it should be one or the other.