5

So I have an Express app that uses middleware to parse JSON POST requests and then populate a req.body object. Then I have a promise chain that validates the data against a schema using Joi, and then stores it in a database.

What I would like to do is check if an error was thrown after one of these processes, handle it appropriately by sending a status code, then COMPLETELY ABORT the promise chain. I feel like there should be some EXTREMELY CLEAN AND SIMPLE way to do this, (perhaps some sort of break statement?) but I can't find it anywhere. Here is my code. I left comments showing where I hope to abort the promise chain.

const joi = require("joi");

const createUserSchema = joi.object().keys({
    username: joi.string().alphanum().min(4).max(30).required(),
    password: joi.string().alphanum().min(2).max(30).required(),
});

//Here begins my promise chain 
app.post("/createUser", (req, res) => {
    //validate javascript object against the createUserSchema before storing in database
    createUserSchema.validate(req.body)
        .catch(validationError => {
           res.sendStatus(400);

           //CLEANLY ABORT the promise chain here

           })
        .then(validatedUser => {
            //accepts a hash of inputs and stores it in a database 
            return createUser({
                    username: validatedUser.username,
                    password: validatedUser.password
                })
        .catch(error => {
            res.sendStatus(500);

            //CLEANLY ABORT the promise chain here

        })
        //Only now, if both promises are resolved do I send status 200
        .then(() => {
            res.sendStatus(200); 
            }                 
        )

});
Joshua Avery
  • 314
  • 2
  • 17

4 Answers4

5

You can't abort a promise chain in the middle. It's going to either call a .then() or a .catch() later in the chain (assuming there are both and assuming your promises resolve or reject).

Usually, the way you handle this is you put one .catch() at the end of the chain and it examines the type of error and takes appropriate action. You don't handle the error earlier in the chain. You let the last .catch() handle things.

Here's what I would suggest:

// helper function
function err(status, msg) {
    let obj = new Error(msg);
    obj.status = status;
    return obj;
}

//Here begins my promise chain 
app.post("/createUser", (req, res) => {
    //validate javascript object against the createUserSchema before storing in database
    createUserSchema.validate(req.body).catch(validationError => {
        throw err("validateError", 400)
    }).then(validatedUser => {
            //accepts a hash of inputs and stores it in a database 
            return createUser({
                    username: validatedUser.username,
                    password: validatedUser.password
            }).catch(err => {
                throw err("createUserError", 500);
            });
    }).then(() => {
        // success
        res.sendStatus(200); 
    }).catch(error => {
        console.log(error);
        if (error && error.status) {
            res.sendStatus(error.status);
        } else {
            // no specific error status specified
            res.sendStatus(500);
        }
    });
});

This has several advantages:

  1. Any error propagates to the last .catch() at the end of the chain where it is logged and an appropriate status is sent in just one place in the code.
  2. Success is handled in just one place where that status is sent.
  3. This is infinitely extensible to more links in the chain. If you have more operations that can have errors, they can "abort" the rest of the chain (except the last .catch() by just rejecting with an appropriate error object).
  4. This is somewhat analogous to the design practice of not having lots of return value statements all over your function, but rather accumulating the result and then returning it at the end which some people consider a good practice for a complicated function.
  5. When debugging you can set breakpoints in one .then() and one .catch() to see the final resolution of the promise chain since the whole chain goes through either the last .then() or the last .catch().
jfriend00
  • 683,504
  • 96
  • 985
  • 979
  • 1
    How would I figure out exactly what type of error was thrown with the `.catch` statement? – Joshua Avery Aug 21 '18 at 04:58
  • 2
    @JoshuaAvery - Did you study my code suggestion? It handles the error, sets up how it wants the error to be dispositioned and then rethrows the error so it will jump to the last `.catch()` with the status and log message already set. That is one technique. Another is to throw a separate type of error for each place and then use a switch statement in the `.catch()` to check for each type of error. I prefer this mechanism because I think it scales better and keeps all the code for one operation together. – jfriend00 Aug 21 '18 at 05:00
  • 1
    Does returning the createUser promise with the `.catch` at the end expose that `.catch` to an error thrown further up the chain? In other words won't that returned `.catch` catch errors thrown above it? – Joshua Avery Aug 21 '18 at 05:07
  • 1
    I apologize if I misunderstand the concepts but node is relatively new to me. – Joshua Avery Aug 21 '18 at 05:18
  • 1
    Never mind I understand now! The then statement will not execute and return the `.catch` because the thrown error will skip all the way down to the final `.catch` statement. Thank you very much! – Joshua Avery Aug 21 '18 at 05:32
  • @JoshuaAvery - Yep, you got it figured out. – jfriend00 Aug 21 '18 at 06:31
  • [Using `.then(…, …)` instead of `.then(…).catch(…)`](https://stackoverflow.com/q/24662289/1048572) in the end of the chain might be even cleaner – Bergi Aug 21 '18 at 08:07
  • @Bergi - I'm in the habit of using `.then(...).catch(...)` just to make sure any unexpected exception in the `.then()` handler gets caught. Probably not required here, but that's my coding habit. – jfriend00 Aug 21 '18 at 20:15
1

.catch returns a resolved Promise by default. You want a rejected Promsise. So, you should return a rejected promise from inside the .catch, so that future .thens won't execute:

     .catch(validationError => {
       res.sendStatus(400);
       return Promise.reject();
     })

But note that this will result in a console warning:

Uncaught (in promise) ...

So it would be nice to add another .catch to the end, to suppress the error (as well as catch any other errors that come along):

const resolveAfterMs = ms => new Promise(res => setTimeout(() => {
  console.log('resolving');
  res();
}), ms);

console.log('start');
resolveAfterMs(500)
  .then(() => {
    console.log('throwing');
    throw new Error();
  })
  .catch(() => {
    console.log('handling error');
    return Promise.reject();
  })
  .then(() => {
    console.log('This .then should never execute');
  })
  .catch(() => void 0);

If you want to avoid all future .thens and future .catches, I suppose you could return a Promise that never resolves, though that doesn't really sound like a sign of a well-designed codebase:

const resolveAfterMs = ms => new Promise(res => setTimeout(() => {
  console.log('resolving');
  res();
}), ms);

console.log('start');
resolveAfterMs(500)
  .then(() => {
    console.log('throwing');
    throw new Error();
  })
  .catch(() => {
    console.log('handling error');
    return new Promise(() => void 0);
  })
  .then(() => {
    console.log('This .then should never execute');
  })
  .catch(() => {
    console.log('final catch');
  });
CertainPerformance
  • 356,069
  • 52
  • 309
  • 320
  • 2
    Yes but then the other `.catch` will end up catching this error as well which is a problem. Will the other `.catch` end up catching the `promise.reject`? – Joshua Avery Aug 21 '18 at 04:44
  • 1
    Sorry so clarification, I want the `.catch` to completely abort the chain, bypassing all of the other `.catch`'s. – Joshua Avery Aug 21 '18 at 04:51
  • 1
    @JoshuaAvery See edit. i suppose you can return a `Promise` that never resolves, ensuring that the chain never progresses, though I don't know if there's a more elegant solution – CertainPerformance Aug 21 '18 at 04:52
  • 1
    @CertainPerformance Then is there another solution would I be able to use? Some way I can handle multiple possible rejections or errors and respond to them differently? – Joshua Avery Aug 21 '18 at 04:54
1

A cleaner solution for what you are trying to accomplish might be to use express-validation, which is a simple wrapper around joi that provides you with express middleware for validation of the body, params, query, headers and cookies of an express request based on your Joi schema.

That way, you could simply handle any Joi validation errors thrown by the middleware within your "generic" express error handler, with something like:

const ev = require('express-validation');
app.use(function (err, req, res, next) {
  // specific for validation errors
  if (err instanceof ev.ValidationError) 
     return res.status(err.status).json(err);
  ...
  ...
  ... 
}

If you don't want to use the express-validation package, you could write your own simple middleware that does more or less the same thing, as described here (see example here).

Yoni Rabinovitch
  • 5,171
  • 1
  • 23
  • 34
0

One strategy is to separate your error handling in subpromises which have their individual error handling. If you throw an error from them, you'll bypass the main promise chain.

Something like:

return Promise.resolve().then(() => {
  return createUserSchema.validate(req.body)
    .catch(validationError => {
       res.sendStatus(400);
       throw 'abort';
    });
}).then(validatedUser => {
   // if an error was thrown before, this code won't be executed
   // accepts a hash of inputs and stores it in a database 
   return createUser({
      username: validatedUser.username,
      password: validatedUser.password
   }).catch(error => {
      // if an error was previously thrown from `createUserSchema.validate`
      // this code won't execute

      res.sendStatus(500);
      throw 'abort';
   });
}).then(() => {
   // can put in even more code here
}).then(() => {
  // it was not aborted
  res.sendStatus(200); 
}).catch(() => {
  // it was aborted
});

You can skip the Promise.resolve().then() wrapping, but it's included for illustrative purposes of the general pattern of subdividing each task and its error handling.

Steve
  • 10,435
  • 15
  • 21