1

I'm making a simple database call wrapped in a promise, and trying to catch the error and pass it through to the promise reject(), but the reject isn't being handled or bubbled back up by the calling code. The code just stops executing when the mysql call fails.

The error within the mysql callback is:

REJECTING QUERY { Error: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '?' at line 1

Here is the database query code:

this.query = function(sql, params) {
    return new Promise((resolve, reject) => {
        _pool.query(sql, params, function(err, result) {
            if (err) {
                console.log("REJECTING QUERY", err);
                return reject(err);
            }
            resolve(result);
        });
    });
}

Here is the calling code:

this.createUser = function(data) {
    var query = "INSERT into users SET ?";

    return new Promise((resolve, reject) => {
        Container.DB.query(query, data)
            .then((response) => {
                console.log("Resolved", response);
                return resolve(response);
            },(error) => {
                console.log("REJECTION ERROR", error);
                return reject('An unknown error occurred and has been reported.');
            })
            .catch((err) => {
                console.log("CAUGHT ERROR", err);
            });
    });
}

I get to "REJECTING QUERY" within the database query code, but nothing in the calling code is reached (ie. .then, or the error handler, or the .catch handler). Is it possible to get the mysql error to reach these points in code so I can respond to the user? Am I doing something wrong?

thefourtheye
  • 233,700
  • 52
  • 457
  • 497
Ryan Weiss
  • 1,308
  • 1
  • 14
  • 36
  • 1
    your calling code (this.createUser) is using the [Promise constructor anti-pattern](http://stackoverflow.com/q/23803743/1048572) - but that's not the issue. If the query _pool.query is successful, is the Resolved code called? – Jaromanda X Sep 28 '16 at 08:55
  • Yeah, you know, somehow it's all working now... but thanks for pointing out the antipattern. However, even though it is only just passing the resolve(response) (in which case I can just: return Container.DB.query() from this.createUser()), I'd still like to be able to handle some other things within this call, ie. within createUser, and not it's calling code (ie. the view).. so in that sense, I think it makes sense to wrap the Promise again, doesn't it? – Ryan Weiss Sep 28 '16 at 09:27
  • without specifics, I wont commit to an opinion – Jaromanda X Sep 28 '16 at 09:28
  • Let me elaborate: Layer 1: The View - calls UserApi.createNewUser Layer 2: The UserApi facade: createNewUser method talks to the transport layer and returns the response to the view Layer 3: The transport layer does the communication with the remote server Here's a gist: https://gist.github.com/rw3iss/018b2447b46ce6f401f1fc0d32fed67b In this respect, if I wanted to handle transport-layer debugging or logging, or something else within either the transport layer or the Api layer, it makes sense to intercept the Promise with another? Maybe it's just a matter of... opinion? Opinionate! – Ryan Weiss Sep 28 '16 at 18:06

1 Answers1

1

The anti-pattern mentioned by @JaromandaX is forcing you to unnecessarily jump through flaming hoops to accommodate it... and your getting burned.

But, first, you are rejecting to the outer (redundant) promise from the then before the catch so the catch is by-passed. After an error is thrown in a promise chain, the first thenable with a second argument (onRejected) will consume it: so it won't be propagated beyond that. But, anyway, you need to trap the error on the outer promise which you are rejecting.

this.createUser = function (data) {
  var query = "INSERT into users SET ?";

  return new Promise((resolve, reject) => {  //   the first 'then' rejects to here
    Container.DB.query(query, data)          //   broken promise: anti-pattern
      .then((response) => {
        console.log("Resolved", response);
        return resolve(response);
      }, (error) => {
        console.log("REJECTION ERROR", error);//<--the error is consumed here and will
                                              //   not go any further on this chain
        return reject('An unknown error occurred and has been reported.');
      })
      .catch((err) => {                       //   this will not be called
        console.log("CAUGHT ERROR", err);     //   because it is the 'onRejected'
                                              //   argument of a then
      });
  })
    .catch((err) => {   // this will be called and the error will be consumed
      console.log("CAUGHT ERROR", err);
      return 'An unknown error occurred and has been reported.';
    });
  ;
}

Less worse, you can log and re-throw the error in one catch...

this.createUser = function (data) {
  var query = "INSERT into users SET ?";

  return new Promise((resolve, reject) => {  // this is still redundant
    Container.DB.query(query, data)          // broken promise: anti-pattern
      .then((response) => {                  // on error, skip this because it has no
        console.log("Resolved", response);   // 'onRejected' argument
        return resolve(response);
      })
      .catch((err) => {                      // this will be called and the error
        console.log("CAUGHT ERROR", err);    // will be consumed
        return reject('An unknown error occurred and has been reported.');
      });
  })
    ;
}

Better, eliminate the anti-pattern and propagate the message with a throw instead of a reject on the (redundant) promise wrapper...

this.createUser = function (data) {
  var query = "INSERT into users SET ?";

  return Container.DB.query(query, data)
    .then((response) => {                  // on error, skip this because it has no
      console.log("Resolved", response);   // 'onRejected' argument
      return resolve(response);
    })
    .catch((err) => {                      // this will be called and the error
      console.log("CAUGHT ERROR", err);    // will be consumed so need to re-throw
      throw new Error('An unknown error occurred and has been reported.');
    });
}

Bearing in mind that a catch is just syntactic sugar for then(undefined, reject) and that, once rejected, a promise is no longer pending, calling it's then method will return undefined 'as soon as possible'. So you can chain another then after the catch if you prefer not to throw...

this.createUser = function (data) {
  var query = "INSERT into users SET ?";

  return Container.DB.query(query, data)
    .then((response) => {                  // on error, skip this because it has no
      console.log("Resolved", response);   // 'onRejected' argument
      return resolve(response);
    })
    .catch((err) => {                      // this will be called and the error
      console.log("CAUGHT ERROR", err);    // will be consumed. The returned promise
    })                                     // state will be rejected but not pending
                                           // It's then method returns 'undefined' as 
                                           // soon as possible
    .then(() => 'An unknown error occurred and has been reported.');
}

Taking it one step further, bearing in mind that the value returned by a resolved or rejected promise is the return value of whichever of those is called, you can pass any value you like to the consumer via the return in the catch...

this.createUser = function (data) {
  var query = "INSERT into users SET ?";

  return Container.DB.query(query, data)
    .then((response) => {                  // on error, skip this because it has no
      console.log("Resolved", response);   // 'onRejected' argument
      return resolve(response);
    })
    .catch((err) => {                      // this will be called and the error
      console.log("CAUGHT ERROR", err);    // will be consumed. The returned promise
                                           // state will be rejected but not pending
                                           // but you can still return any value
      return 'An unknown error occurred and has been reported.'
    })
}
Cool Blue
  • 6,438
  • 6
  • 29
  • 68
  • Thanks for the thorough response! I just figured all this out earlier today, and have amended the code appropriately. – Ryan Weiss Sep 29 '16 at 02:39