Improving handling of Express endpoint

Question

I have an API gateway in a larger set of microservices, and I'm handling one of my routes. The endpoint ends up making an RPC call to another service, and then I just log the response and send it back to the user. I haven't really completed the error handling section, because was thinking that I could add some middleware later on to handle my errors.

routes.post("/submit", async (request, result) => {
  logger.info("Received call to /gapf/submit endpoint");
  const gapf = request.body;

  // convert relevant request strings into integers
  gapf.facultyId = parseInt(gapf.facultyId, 10);
  gapf.created = parseInt(gapf.created, 10);
  gapf.lastModified = parseInt(gapf.lastModified, 10);
  if (gapf.documents !== undefined) {
    gapf.documents = gapf.documents.map(doc => {
      return {
        name: doc.name,
        link: doc.link,
        attachedDate: parseInt(doc.attachedDate, 10)
      };
    });
  }

  try {
    logger.info("Calling to Ticket Service with gapf body: %j", gapf);
    const response = await tridentClient.SubmitGAPF(gapf);
    logger.info("Received response from Ticket Service: %j", response);
    logger.info("Exiting /gapf/submit endpoint");
    result.json(response);
  } catch (err) {
    logger.error(
      "Received error from submitting to Ticket Service: ",
      err.message
    );
    // TODO: should add middleware later on to handle errors
    result.status(500);
    result.json({});
  }
});

Right now the error.message seems to be some binary data instead of JSON, so that would also need to be fixed later on.

Answer 1

Apart from the first issue below, your code is mostly clean!

• What happens if one of the parseInt calls throws an exception? In that case, I'd send a HTTP 500 error as well. This is especially relevant in your scenario where the data comes from an unknown client.
  Essentially, the solution boils down to wrapping all the code in a try-catch or employing middleware.

  routes.post("/submit", async (request, result) => {
    try {
      /* all the code */
    }
    catch (err) {
      /* log & send a generic HTTP 500 error message as in your code */
      /* Beware that you might not intend to send a HTTP 500 message if
         the code in the try already sent something! */
    }
  });
  
  // <=>
  routes.post("/submit", wrapSafe(yourCodeExtractedAsAFunction));
  
  // <=>
  routes.post("/submit", someMiddleWare(yourCodeAsAFunction));
  

• Catching all exceptions (and not rethrowing ones you cannot handle) is fine in your case since your function is top-level enough for that.
  Exactly as you say, if you add middleware, let the exception bubble up and let the middleware take care of it.

• Omit the return in an arrow function if it is unnecessary and serves no clarity. So this block:

  gapf.documents = gapf.documents.map(doc => {
    return {
      name: doc.name,
      link: doc.link,
      attachedDate: parseInt(doc.attachedDate, 10)
    };
  });
  

  Can be updated like this:

  // You need enclosing parentheses, so that the parser can
  // differentiate between a block statement and an object literal.
  gapf.documents = gapf.documents.map(doc => ({
    name: doc.name,
    link: doc.link,
    attachedDate: parseInt(doc.attachedDate, 10)
  }));
  

• // convert relevant request strings into integers: Maybe you could describe what constitutes a "relevant request string". Is there a list of them?