When creating some endpoints using Node.js and express I came across a piece of code that is repeated a few times in the same controller file on different endpoints. This code is inside a conditional that is inside a try…catch block, it serves to validate if a sent parameter exists in the DB and so, if it exists or not, the execution proceeds accordingly. Here is my original code:
const productListing = async (req, res) => {
const { category_id } = req.query;
try {
// if there are query params execute this if statement
if (req.url.includes("?")) {
// the next 3 lines of code repeat a few times throughout the controller
const validCategory = await knex("categories").where("id", category_id)
if (validCategory.length === 0) {
return res.status(404).json({ "message": "Category not found" });
};
const productsByCategory = await knex("products").where({ category_id })
return res.status(200).json(productsByCategory);
};
const allProducts = await knex("products")
return res.status(200).json(allProducts);
} catch (error) {
return res.status(500).json({ "message": "Internal error" });
};
};
In order to improve the code and avoid repetition I tried to create the function below and call it instead of the repetitive code, what I expected to get was similar behavior, but although the function works when no query params are sent, or when the category exists, when the category does not exist, it makes the node crash and does not give the proper return, because in this condition the function should finish the try…catch block but the node tries to set the headers after the function has already done that.
FUNCTION:
const validId = async (res, table, id) => {
const validateId = await knex(table).where({ id })
if (validateId.length === 0) {
return res.status(404).json({ "message": `${table} not found` });
};
return
};
MAIN CODE:
const productListing = async (req, res) => {
const { category_id } = req.query;
try {
if (req.url.includes("?")) {
validId(res, "categories", category_id);
const productsByCategory = await knex("products").where({ category_id })
return res.status(200).json(productsByCategory);
};
const allProducts = await knex("products")
return res.status(200).json(allProducts);
} catch (error) {
return res.status(500).json({ "message": "Internal error" });
};
};
ERROR CODE:
> nodemon ./src/index.js
[nodemon] 2.0.22
[nodemon] to restart at any time, enter `rs`
[nodemon] watching path(s): *.*
[nodemon] watching extensions: js,mjs,json
[nodemon] starting `node ./src/index.js`
node:internal/errors:490
ErrorCaptureStackTrace(err);
^
Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client
at new NodeError (node:internal/errors:399:5)
at ServerResponse.setHeader (node:_http_outgoing:663:11)
at ServerResponse.header (D:CODINGCUBOSBACKENDunidade-5desafio-backend-final-dbe-b2b-t03-ifoodnode_modulesexpresslibresponse.js:794:10)
at ServerResponse.send (D:CODINGCUBOSBACKENDunidade-5desafio-backend-final-dbe-b2b-t03-ifoodnode_modulesexpresslibresponse.js:174:12)
at ServerResponse.json (D:CODINGCUBOSBACKENDunidade-5desafio-backend-final-dbe-b2b-t03-ifoodnode_modulesexpresslibresponse.js:278:15)
at validaId (D:CODINGCUBOSBACKENDunidade-5desafio-backend-final-dbe-b2b-t03-ifoodsrcfuncoesvalidaId.js:6:36)
at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
code: 'ERR_HTTP_HEADERS_SENT'
}
Node.js v18.15.0
[nodemon] app crashed - waiting for file changes before starting...
2
Answers
The
in the
validId
function is actually sending the response to the client but it never breaks execution of the function so then it goes on to the nextand throws because the response has already been sent. You would need some way to determine that the response has already been sent by the
validId
function, if we kept things as they are currently. You could do something leveraging theres.headersSent
property and not change too much of your code.I think things become a bit more clear if you keep the
validId
function fairly limited in its responsibilities and leave the response handling to theproductListing
function.You forgot to
await
validId
. Withoutawait
you can see that function actually gets executed afterproductListing
returns. You can seevalidId
was executed in the error stack trace – after a response was already sent.You also need to know whether or not
validId
already returned a response, so you don’t try to send a second response in the main function, resulting in the same error. You can do that by returning a boolean.