How can I force this promise to throw specific errors?

Question

The code is a part of a much bigger and complicated code, so I am just gonna put the relevant snippets to my question.

I have this promise.all snippet:

 Promise.all(receivedObjs.arrayCMsIds.map(cmid => 
                    server.writeAttachedCMinfo(invId,cmid)))
                            .then(function (results) { // for promise all
                                return res.json(apiHelp.success(results,"success"));

                            }).catch(function (error) {
                                res.json(apiHelp.error(error, error));
                            });

And this long complicated writeAttachedCMinfo function:

    server.writeAttachedCMinfo = function (invId,cmid) {
    return new Promise(function (resolve, reject) {


        console.log("writeAttachedCMinfo");
        console.log("invoiceId " + invId);
        console.log("cmid "+ cmid);

        var invoiceId = JSON.stringify(invId);
        var cmId = JSON.stringify(cmid);
        var invIdString = invoiceId;
        var cmIdString = cmId;

        invIdString = invIdString.slice(1, -1);
        cmIdString = cmIdString.slice(1, -1);

        var projection = 'gwCode certifiedInvoiceAmount buyerReference supplierReference invoiceNo invoiceSerialNo invoiceFiles creditMemos';
        ubiqInvoice.findById(invIdString, projection).then(function (dbInvoice) {

            var intInvCertifiedAmount = parseInt(dbInvoice.certifiedInvoiceAmount);


            creditMemo.findById(cmIdString).then(function (dbCreditMemo) {
                var intCreditMemoAmount = parseInt(dbCreditMemo.creditMemoAmount);

                if (intInvCertifiedAmount <= intCreditMemoAmount) {

                    console.log('cm bigger than invoice')

                    return new Error ('CMisbiggerThanInvoice');
                }

                if (dbCreditMemo.isAssociated) {

                    return new Error ('CMisAssociated');
                }

                if (dbInvoice.gwCode === "100000000000"
                    || dbInvoice.gwCode === "110000000000"
                    || dbInvoice.gwCode === "111200000000"
                    || dbInvoice.gwCode === "111100000000"
                    || dbInvoice.gwCode === "111110000000"
                ) { 

                    var creditMemoEntry = {

                        id: guid.create().value,
                        batchId: dbCreditMemo.batchId,
                        invoiceId: dbInvoice._id,
                        recordTypeCode: "CM",
                        buyerReference: dbInvoice.buyerReference,
                        supplierReference: dbInvoice.supplierReference,
                        creditMemoNo: dbCreditMemo.creditMemoNo,
                        creditMemoIssuingDate: dbCreditMemo.creditMemoIssuingDate,
                        creditMemoEffectiveDate: dbCreditMemo.creditMemoEffectiveDate,
                        lastModificationDate: dbCreditMemo.lastModificationDate,
                        currencyCode: dbCreditMemo.currencyCode,
                        creditMemoAmount: dbCreditMemo.creditMemoAmount,
                        hashCode: dbCreditMemo.hashCode,
                        description: dbCreditMemo.description,
                        uploadDate: dbCreditMemo.uploadDate,
                        isAssociated: true,
                    }


                    dbInvoice.creditMemos.push(creditMemoEntry);
                    dbInvoice.certifiedInvoiceAmount = dbInvoice.certifiedInvoiceAmount - dbCreditMemo.creditMemoAmount;
                    dbInvoice.save();


                    dbCreditMemo.isAssociated = true;

                    dbCreditMemo.save();

                    resolve(dbInvoice)


                }
                else { return new Error ('wrongggwcode'); }

    })

        });

    }), function (error) {
        console.log("error: " + error);
    }

}

My goal, is to force error throwing in case one of the if conditions aren't met, and I want to pass the error to client in a form of a custom message so i can use it on the client said for displaying various errors , such as 'CMisbiggerThanInvoice'

if (intInvCertifiedAmount <= intCreditMemoAmount) {

                        console.log('cm bigger than invoice')

                        return new Error ('CMisbiggerThanInvoice');
                    }

I am just trying to figure out a way to pass the error from the writeAttachedCMinfo function to the promise.all's .catch(function (error) but it's not working, the promise.all is always returning success even if one of the if conditions aren't met.

I have tried reject('CMisbiggerThanInvoice'), reject(new Error('CMisbiggerThanInvoice')...all the same.

how can i really force the promise function to return an error?

Avoid the [`Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it)! — Bergi, May 23 '18 at 21:43
Calling `reject` should have worked. Are you getting the expected `results` when there is no error? Also please fix the code indentation so that we can easily parse the nesting. — Bergi, May 23 '18 at 21:45
Hi Bergi, yes I am getting expected results when there's no error - but nothing when there is an error, a reject(new Error('fail')) for example didn't work, or is there's another way to call rejects? — , May 23 '18 at 21:58
How about those two saves - are they synchronous or asynchronous? — Roamer-1888, May 23 '18 at 22:56
Also, do you realise that because there's only one `invId`, there's only one `dbInvoice` and it needn't be retrieved over and over from the database on each call of `server.writeAttachedCMinfo()`. In fact it probably *shouldn't* be retrieved over and over, otherwise each `dbInvoice.save()` is in danger of overwriting earlier saves within the same overall transaction. It seems safer to accumulate all creditMemos and progressively decrement the `certifiedInvoiceAmount` then perform a single `dbInvoice.save(). — Roamer-1888, May 24 '18 at 00:03
You are absolutely right, i was about to isolate dbInvoice query. — , May 24 '18 at 05:39

trincot · Answer 1 · 2018-05-24T07:37:36.130

In the context of a promise you should actually throw the error:

throw new Error('wrongggwcode');

If this executes in a promise constructor callback or then callback, it can be caught via the catch method (or second argument of then) and the argument of the callback you pass to it will be the error (object).

Calling reject from within a then callback will obviously not work, since you don't have access to reject there, but it will work in a promise constructor callback.

Simple example:

new Promise((resolve, reject) => {
    setTimeout( () => reject(new Error('this is an error')) );
}).then( (value) => console.log('resolved with ' + value) )
.catch( (error) => console.log('error message: ', error.message) );

Nesting

When you have nested promises within then callbacks, make sure you always return the value returned by the inner promise as the return value of the outer then callback.

So in your case do this:

return creditMemo.findById( ....
//^^^^

For the same reason you need to do:

return ubiqInvoice.findById( ....
//^^^^

It would lead to far for this question/answer, but it is best practice to avoid nesting promise then calls all together. Instead of calling then on a nested promise, just return the promise without the then call, and apply that then call one level higher, so that you have a "flat" chain of then calls. This is just a matter of best practice, although it should also work like you have done it provided you always return the inner promises.

Position of the error handler

The error handler is placed at a wrong position; in fact you are using the comma operator. In short, you have this in your code:

new Promise(function (resolve, reject) {
    // ... //
}), function (error) {
    console.log("error: " + error);
}

The function after the comma is never executed as it is not an argument to a method call. It just sits behind a comma operator.

What you want is a catch method call on the new promise, and cascade the error in there so that Promise.all will also receive the rejection:

return new Promise(function (resolve, reject) {
    // ... //
}).catch(function (error) {
    console.log("error: " + error);
    throw error; // cascade it
});

Hi trincto, thank you for your post. I did try changing return new Error to 'throw new Error('CMisAssociated');' but I got an unhandled rejection error when i tested against the related if condition — , May 23 '18 at 21:41
Make sure you always *return* nested promises. See last paragraph I added to my answer. — trincot, May 23 '18 at 21:44
and the 'then' callback still threw the 'return res.json(apiHelp.success(results,"success"));' , no error was caught. — , May 23 '18 at 21:44
Apply this everywhere in your code. I spotted a second occurrence. See addition to my answer. Please check all occurrences of this problem. — trincot, May 23 '18 at 21:46
still getting unhandled rejection error when testing against CMisbiggerThanInvoice, here the more complete code in codeshare: https://codeshare.io/a3vYn1 — , May 23 '18 at 22:05
I cannot access that website (bad gateway). But there is more than a few things wrong with your code. One error handler is misplaced. See the addition to my answer, and make sure all your error handlers are positioned right. — trincot, May 24 '18 at 07:29

Roamer-1888 · Accepted Answer · 2018-05-24T13:39:51.133

In server.writeAttachedCMinfo() the main things are :

to get rid of the new Promise() wrapper - it's not necessary - (Promise constructor anti-pattern).
to throw errors, rather than return them.

Also, because there's only one invId, there's effectively only one dbInvoice and it needn't be retrieved over and over from the database on each call of server.writeAttachedCMinfo(). In fact it shouldn't be retrieved over and over, otherwise each dbInvoice.save() may well overwrite earlier saves within the same overall transaction. It will be safer to accumulate all creditMemos and progressively decrement the certifiedInvoiceAmount in a single dbInvoice object, and evetually perform a single `dbInvoice.save().

server.writeAttachedCMinfo = function(dbInvoice, cmid) {
    return creditMemo.findById(JSON.stringify(cmid).slice(1, -1))
    .then(dbCreditMemo => {
        if(parseInt(dbInvoice.certifiedInvoiceAmount) <= parseInt(dbCreditMemo.creditMemoAmount)) {
            throw new Error('CMisbiggerThanInvoice');
         // ^^^^^
        }
        /* all sorts of synchronous stuff */
        /* all sorts of synchronous stuff */
        /* all sorts of synchronous stuff */
        return dbCreditMemo; // deliver dbCreditMemo via returned Promise
    });
}

Now, in the caller :

ubiqInvoice.findById() can be called once.
perform checks on dbInvoice, and throw on failure.
pass dbInvoice, rather than invId to server.writeAttachedCMinfo() at each call.
all saving can be done here, not in server.writeAttachedCMinfo().

As a consequence, the caller subsumes some of the code that was in server.writeAttachedCMinfo() :

ubiqInvoice.findById(JSON.stringify(invId).slice(1, -1), 'gwCode certifiedInvoiceAmount buyerReference supplierReference invoiceNo invoiceSerialNo invoiceFiles creditMemos')
.then(dbInvoice => {
    if(dbCreditMemo.isAssociated) {
        throw new Error('CMisAssociated');
     // ^^^^^
    }
    if(dbInvoice.gwCode === '100000000000'
        || dbInvoice.gwCode === '110000000000'
        || dbInvoice.gwCode === '111200000000'
        || dbInvoice.gwCode === '111100000000'
        || dbInvoice.gwCode === '111110000000'
    ) {
        return Promise.all(receivedObjs.arrayCMsIds.map(cmid => {
            return server.writeAttachedCMinfo(dbInvoice, cmid)
            .catch(error => {
                console.log(error);
                return null;
            });
        }))
        .then(dbCreditMemos => {
            return Promise.all(dbCreditMemos.map(memo => {
                return memo ? memo.save() : null;
            }))
            .then(() => dbInvoice.save())
            .then(() => {
                res.json(apiHelp.success(dbInvoice, 'success'));
            });
        });
    } else {
        throw new Error('wrongggwcode');
     // ^^^^^
    }
})
.catch(error => {
    console.log('error: ' + error);
    res.json(apiHelp.error(error, error));
});

The whole area around catching/handling errors arising from server.writeAttachedCMinfo() requires more thought. On the one hand, you might want successful sub-transactions to be saved and errors to be swallowed (as coded above) while on the other hand, you might want any single failure to cause nothing to be saved.

Another consideration is whether server.writeAttachedCMinfo() calls should be made sequentially, which would control the priority by which the sub-transactions access the credit balance. As it stands, with parallel requests, it's a bit of a free-for-all.

That addresses a bit more than the question asked for but hopefully it will be useful.

wow...i learned this Promise pattern from a senior dev - the one i am using, I am reading more about this anti-matter thing now. So the new Promise wrapper is a common antipattern? — , May 24 '18 at 13:51
Sam, a Promise wrapper is necessary when manually promisifying a non-promise (eg nodeback) method. When promises are already available, as here, then you can generally do everything you want without a Promise wrapper. So when you see `new Promise(...), `it's not always anti-pattern, but often so. — Roamer-1888, May 24 '18 at 14:43
So I assume it's bad here since the method such as the -.findById method is already a Promise? — , May 24 '18 at 14:50
Yes, at least we assume so. Strictly we *know* only that the method is "thenable" ... which is a pretty good indication that it returns a promise. — Roamer-1888, May 24 '18 at 14:59

How can I force this promise to throw specific errors?

2 Answers2

Nesting

Position of the error handler