express-validator and promise chains

Question

Is this the correct way of chaining promises when it comes to express-validator? I am using express node and this is the endpoint for a get request

Something about it seems a bit messy, but I can't think of a cleaner way to do it.

function exitChain() {
  return Promise.reject();
}

function getUserByID(req, res) {

  const id = req.query.id;
  req.checkQuery('id', 'ID is not a number').isInt();

  const q = 'select * from users where user_id=$1';
  const p = [id];

  req.getValidationResult()
    .then(function(vRes) {
      if (!vRes.isEmpty()) {
        exitChain();
        return res.status(400).send('Validation Fail: ' + util.inspect(vRes.array()));
      }
    })
    .then(function() {
      pg.connect(postgresConfig)
        .then(function(client) {
          return client.query(q, p).execute();
        })
        .then(function(result) {
          result.client.done();
          return res.status(200).send(res.json(result.rows));
        })
        .catch(function(err) {
          console.log(err);
          return res.status(400).send('Error: ' + err);
        });
    })
    .catch(function(err) {
      console.log(err);
      return res.status(400).send('Error: ' + err);
    });
}

Answer 1

Normally, I'd start of modelling promise chains following the "happy path". Following that, you will almost always find your logic (therefore your promise chain) in a straight line. Any fail condition in the chain should be broken by returning a rejected promise, which then skips to the nearest fail callback (which might redeem it) or catch if nothing caught it midway.

For rejections, it's also advised to reject with an error object because 1) it makes sense to, 2) error-handling mechanisms will almost always expect an error object not a string, and 3) works well with async-await, which uses the try-catch mechanism, where catch expects an error object. Lastly, you can key off of the error object's data to know what status code to return. This versus having multiple ends within the chain.

Avoid hoisting variables to outer scopes unless needed. I'm referring to q and p. It's only used in the callback that does return client.query(q, p).execute();. It makes sense to put it in the callback because it's only used there, and there's no unnecessary operation if ever that callback is never reached.

Also, exitChain() does not work the way you think it works.

function getUserByID(req, res) {
  const id = req.query.id
  req.checkQuery('id', 'ID is not a number').isInt()

  req.getValidationResult().then(vRes => {

    // Do next async operation or fail depending on condition
    return vRes.isEmpty() ? pg.connect(postgresConfig) : Promise.reject(new Error('...'))

  }).then(client => {

    return client.query('select * from users where user_id=$1', [id]).execute()

  }).then(result => {

    result.client.done();
    return res.status(200).send(res.json(result.rows))

  }).catch(error => {

    // If anything fails, it all ends up here. The 400 could be replaced
    // depending on the error object data.
    res.status(400).send('Error: ' + error.message)

  })
}