0
\$\begingroup\$

Background

I have a REST API using MongoDB, Node.js and Express that makes a request to my NoSQL DB and depending on different results, I want to differentiate which error I send the customer.

Problem

The current version of my code has a generic error handler and always sends the same error message to the client:

api.post("/Surveys/", (req, res) => {
        const surveyJSON = req.body;
        const sender = replyFactory(res);

        Survey.findOne({_id: surveyJSON.id})
            .then(doc => {
                if(doc !== null)
                    throw {reason: "ObjectRepeated"};

                //do stuff
                return new Survey(surveyJSON).save();
            })
            .then(() => sender.replySuccess("Object saved with success!")) 
            .catch(error => {
                /*
                 * Here I don't know if:
                 * 1. The object is repeated
                 * 2. There was an error while saving (eg. validation failed)
                 * 3. Server had a hiccup (500)
                 */
                sender.replyBadRequest(error);    
            });
    });

This is a problem, because the client will always get the same error message, no matter what and I need error differentiation!

Research

I found a possible solution, based on the division of logic and error/response handling:

However, I don't understand a few things:

  1. I don't see how, at least in my example, I can separate the logic from the response. The response will depend on the logic after all!
  2. I would like to avoid error sub-classing and hierarchy. First because I don't use bluebird, and I can't subclass the error class the answer suggests, and second because I don't want my code with a billion different error classes with brittle hierarchies that will change in the future.

My idea, that I don't really like either

With this structure, if I want error differentiation, the only thing I can do is to detect an error occurred, build an object with that information and then throw it:

.then(doc => {
    if(doc === null)
        throw {reason: "ObjectNotFound"};

    //do stuff
    return doc.save();
})
.catch(error => {
    if(error.reason === "ObjectNotFound")  
        sendJsonResponse(res, 404, err);
    else if(error.reason === "Something else ")
        sendJsonResponse(/*you get the idea*/);
    else //if we don't know the reasons, its because the server likely crashed
        sendJsonResponse(res, 500, err);
});

I personally don't find this solution particularly attractive because it means I will have a huge if then else chain of statements in my catch block.

Also, as mentioned in the previous post, generic error handlers are usually frowned upon (and for a good reason imo).

Questions

How can I improve this code?

\$\endgroup\$
  • \$\begingroup\$ Not a complete answer, but it seems to me like you should let mongo handle the _id uniqueness constraint, and handle the error when you try to save stuff. Right now, you're vulnerable to race conditions (e.g. you check and don't find a matching doc, but one is added by another request microseconds later, and then you add your - now non-unique - doc in the then). Also... this question is basically "hypothetical code" which would make it off-topic for Code Review. And your first and second examples are exactly opposite; throw if doc found vs throw if doc not found...? \$\endgroup\$ – Flambino Apr 1 '17 at 19:33
  • \$\begingroup\$ This is very not hypothetical. It is a clear and well defined question with code samples. If you cant answer it, don't comment your ignorance. \$\endgroup\$ – Flame_Phoenix Apr 1 '17 at 20:44
  • \$\begingroup\$ Nice attitude. You'll note I didn't vote to close your question. I just pointed out how your code examples are less than complete. Lines like something else and you get the idea deserve to be fleshed out. Please see how to ask. Also, have you considered a switch statement instead of the if...else? That'd be a start. But I'm ignorant, so what do I know? \$\endgroup\$ – Flambino Apr 1 '17 at 21:47
  • \$\begingroup\$ You didn't vote to close my question but you opened the door for someone else to do it. I spent to much time and effort making this post for someone else to nicely "suggest" it should be closed because I removed clutter code from the samples - so yes, I felt offended. As for the switch, I thank you for the proposal, but right now I am trying to avoid the if then else of doom, a switch would make little difference. \$\endgroup\$ – Flame_Phoenix Apr 2 '17 at 6:04
1
\$\begingroup\$

Would it help to rewrite it along the lines of:

function notFoundErrorHandler(error) {
  if(error.reason === "ObjectNotFound")  
      return {code: 404, error:err);
}
function someOtherErrorHandler(error) {
  if(error.reason === something else")  
      return {code: 499, error:err);
}
function serverErrorHandler(error) {
  // No if because this is a catch all
  return {code: 500, error:err);
}

let handlers = [notFoundErrorHandler,
                someOtherErrorHandler,
                serverErrorHandler     ];

Survey.findOne({_id: surveyJSON.id})
  ...
  .catch(error => {
    respondToError(error, handlers);
  } );

function respondToError(error, handlers) {
  for (let handler of handlers) {
    let {code, error} = handler(error);
    if (code) {
       sendJsonResponse(res, code, error);
       return;
    }
  }
  console.error("Shouldn't get here");
}
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Although this solution suffers the same issues that solutions with generic error handlers have, it gave inspiration to create some code that I will post here later on. Well thought, thanks ! \$\endgroup\$ – Flame_Phoenix Apr 2 '17 at 7:16
  • \$\begingroup\$ Have a look at my answer, you may enjoy a thing or two there :P \$\endgroup\$ – Flame_Phoenix Apr 2 '17 at 8:50
0
\$\begingroup\$

Objectives

When I started this thread, I had two objectives in mind:

  1. Having error differentiation
  2. Avoid an if then else of doom in a generic catcher

I have now come up with two radically distinct solutions, which I now post here, for future reference.

Solution 1: Generic error handler with Errors Object

This solution is based on the solution from @Marc Rohloff, however, instead of having an array of functions and looping through each one, I have an object with all the errors.

This approach is better because it is faster, and removes the need for the if validation, meaning you actually do less logic:

const errorHandlers = {
    ObjectRepeated: function(error){
        return { code: 400, error };
    },
    SomethingElse: function(error){
        return { code: 499, error };
    }
};

Survey.findOne({
        _id: "bananasId"
    })
    .then(doc => {

        //we dont want to add this object if we already have it
        if (doc !== null)
            throw { reason: "ObjectRepeated", error:"Object could not be inserted because it already exists."};

        //saving empty object for demonstration purposes
        return new Survey({}).save();
    })
    .then(() => console.log("Object saved with success!"))
    .catch(error => {
        respondToError(error);
    });

    const respondToError = error => {
        const errorObj = errorHandlers[error.reason](error);

        if (errorObj !== undefined)
            console.log(`Failed with ${errorObj.code} and reason ${error.reason}: ${JSON.stringify(errorObj)}`);
        else 
            //send default error Obj, server 500
            console.log(`Generic fail message ${JSON.stringify(error)}`);
    };

This solution achieves:

  1. Partial error differentiation (I will explain why)
  2. Avoids an if then else of doom.

This solution only has partial error differentiation. The reason for this is because you can only differentiate errors that you specifically build, via the throw {reaon: "reasonHere", error: "errorHere"} mechanism.

In this example, you would be able to know if the document already exists, but if there is an error saving the said document (lets say, a validation one) then it would be treated as "Generic" error and thrown as a 500.

To achieve full error differentiation with this, you would have to use the nested Promise anti pattern like the following:

.then(doc => {

    //we dont want to add this object if we already have it
    if (doc !== null)
        throw { reason: "ObjectRepeated", error:"Object could not be inserted because it already exists." };

    //saving empty object for demonstration purposes
    return new Survey({}).save()
        .then(() => {console.log("great success!");})
        .catch(error => {throw {reason: "SomethingElse", error}});
})

It would work... But I see it as a best practice to avoid anti-patterns.

Solution 2: Using ECMA6 Generators via co.

This solution uses Generators via the library co. Meant to replace Promises in near future with a syntax similar to async/await this new feature allows you to write asynchronous code that reads like synchronous (well, almost).

To use it, you first need to install co, or something similar like ogen. I pretty much prefer co, so that is what i will be using here instead.

const requestHandler = function*() {

    const survey = yield Survey.findOne({
        _id: "bananasId"
    });

    if (survey !== null) {
        console.log("use HTTP PUT instead!");
        return;
    }

    try {
        //saving empty object for demonstration purposes
        yield(new Survey({}).save());
        console.log("Saved Successfully !");
        return;
    }
    catch (error) {
        console.log(`Failed to save with error:  ${error}`);
        return;
    }

};

co(requestHandler)
    .then(() => {
        console.log("finished!");
    })
    .catch(console.log);

The generator function requestHandler will yield all Promises to the library, which will resolve them and either return or throw accordingly.

Using this strategy, you effectively code like you were coding synchronous code (except for the use of yield).

I personally prefer this strategy because:

  1. Your code is easy to read and it looks synchronous (while still have the advantages of asynchronous code).
  2. You do not have to build and throw error objects every where, you can simply send the message immediately.
  3. And, you can BREAK the code flow via return. This is not possible in a promise chain, as in those you have to force a throw (many times a meaningless one) and catch it to stop executing.

The generator function will only be executed once passed into the library co, which then returns a Promise, stating if the execution was successful or not.

This solution achieves:

  1. error differentiation
  2. avoids if then else hell and generalized catchers (although you will use try/catch in your code, and you still have access to a generalized catcher if you need one).

Using generators is, in my opinion, more flexible and makes for easier to read code. Not all cases are cases for generator usage (like mpj suggests in the video) but in this specific case, I believe it to be the best option.

Conclusion

Solution 1: good classical approach to the problem, but has issues inherent to promise chaining. You can overcome some of them by nesting promises, but that is an anti pattern and defeats their purpose.

Solution 2: more versatile, but requires a library and knowledge on how generators work. Furthermore, different libraries will have different behaviors, so you should be aware of that.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ I would be tempted to change solution 1 with something like: const errorObj = (errorHandlers[error.reason] || errorHandlers.default)(error); \$\endgroup\$ – Marc Rohloff Apr 2 '17 at 19:19
  • \$\begingroup\$ You say that solution (2) provides error differentiation but I don't see any code to do that at all. How would you do it differently than in solution 1? \$\endgroup\$ – Marc Rohloff Apr 2 '17 at 19:20
  • \$\begingroup\$ Regarding solution 1, I rather avoid using falsiness or truthiness. As for solution 2, it allows for error differentiation because with the try/catch you can simply send the error message you want, and you can effectively break the execution flow, while with promises alone that is not possible without referring to the nested anti pattern. \$\endgroup\$ – Flame_Phoenix Apr 3 '17 at 9:26
  • \$\begingroup\$ Also, to note that now with Node.js > 7.6 there is official support for async/await which I highly recommend ! \$\endgroup\$ – Flame_Phoenix Apr 3 '17 at 9:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for?Browse other questions tagged or ask your own question.