5

I'm trying to handle a custom error that my async method throws, but the try catch block doesn't work appropriately.

I think the way I'm doing it should work but the error is not caught and the program terminates by displaying it in the terminal.

Here is where it throws the error:

async setupTap(tap) {
  const model = this.connection.model('Tap', TapSchema);

  await model.findOneAndUpdate({ id: tap.id }, tap, (err, result) => {
    let error = null;
    if (!result) {
      throw new Error('Tap doesn\'t exists', 404);
    }
    return result;
  });
}

Then, the error handling code:

async setupTapHandler(request, h) {
  const tapData = {
    id: request.params.id,
    clientId: request.payload.clientId,
    beerId: request.payload.beerId,
    kegId: request.payload.kegId,
  };

  try {
    await this.kegeratorApi.setupTap(tapData);
  } catch (e) {
    if (e.code === 404) return h.response().code(404);
  }

  return h.response().code(204);
}

Can someone help me?

I also looked at other topics:

Correct Try...Catch Syntax Using Async/Await

How to properly implement error handling in async/await case

João Neto
  • 51
  • 1
  • 3

2 Answers2

3

You can only use await to successfully wait on an async operation if you are awaiting a promise. Assuming you are using mongoose, I don't know mongoose really well, but it appears that model.findOneAndUpdate() does not return a promise if you pass it a callback. Instead, it executes and puts the result in the callback.

In addition, doing a throw from a callback like this just throws into the database (the code that called the callback) and won't do you any good at all. To have a throw make a rejected promise, you need to either be throwing from the top level of an async function or be throwing from inside a .then() or .catch() handler or inside a promise executor function. That's where throw makes a promise rejected.

The key here is that you want to use the promise interface to your database, not the callback interface. If you don't pass a callback, then it returns a query which you can use .exec() on to get a promise which you can then use with await.

In addition, you weren't building an error object that would have a .code property set to 404. That isn't a property that is supported by the Error object constructor, so if you want that property, you have to set it manually.

I'd suggest this:

async setupTap(tap) {
    const model = this.connection.model('Tap', TapSchema);

    let result = await model.findOneAndUpdate({ id: tap.id }, tap).exec();
    if (!result) {
        let err = new Error('Tap doesn\'t exists');
        err.code = 404;
        throw err;
    }
    return result;
}

Or, with only one async operation here, there's really not much benefit to using await. You could just do this:

setupTap(tap) {
    const model = this.connection.model('Tap', TapSchema);

    return model.findOneAndUpdate({ id: tap.id }, tap).exec().then(result => {
        if (!result) {
            let err = new Error('Tap doesn\'t exists');
            err.code = 404;
            throw err;
        }
        return result;
    });
}
jfriend00
  • 683,504
  • 96
  • 985
  • 979
1

The function findOneAndUpdate returns a promise so there should be no need for the callback. If a callback is needed and you can't update to a newer version then maybe wrap calls in a promise (under To use a callback api as promise you can do:)

Then you want to set code on error, you can't do that with the constructor.

async setupTap(tap) {
  const model = this.connection.model('Tap', TapSchema);

  const result =   await model.findOneAndUpdate({ id: tap.id }, tap);
  if (!result) {
    const e = new Error('Tap doesn\'t exists');
    e.code = 404;
    throw(e);
  }
  return result;
}

async setupTapHandler(request, h) {
  const tapData = {
    id: request.params.id,
    clientId: request.payload.clientId,
    beerId: request.payload.beerId,
    kegId: request.payload.kegId,
  };

  try {
    await this.kegeratorApi.setupTap(tapData);
  } catch (e) {
    if (e.code === 404) return h.response().code(404);
  }

  return h.response().code(204);
}
HMR
  • 37,593
  • 24
  • 91
  • 160
  • Too much over thinking. If the test is simply whether "result" is empyt/false, and the _only_ result of the is sending a 404, then all the code is overkill. Check my answer and you'll it results in the same thing but much simpler. – Randy Casburn Feb 19 '18 at 06:26
  • Doesn't `findOneAndUpdate()` return a query, not a promise? – jfriend00 Feb 19 '18 at 08:09
  • @jfriend00 Not sure, that's why I added the link to wrapping callback as promise but [this documentation](http://mongodb.github.io/node-mongodb-native/3.0/reference/ecmascriptnext/crud/) doesn't mention `.exec` to start as promise, older versions or other libraries may need it and if the library version is old enough then it's probably only callback. – HMR Feb 19 '18 at 08:16
  • 1
    OK, I guess it depends upon exactly what version of what DB the OP is using. I was looking at mongoose doc here: http://mongoosejs.com/docs/api.html. In either case, the OP has to stop using the callback and use the promise interface for `await` to do its job. – jfriend00 Feb 19 '18 at 08:20