Is it safe to not resolve or reject a promise

Question

Imagine a web application with routes that need to check whether the user is allowed to access a given resource before proceeding. The "is authenticated" check relies on a database call.

In each route, I may have:

authorizeOwnership(req, res)
.then(function() {
    // do stuff
    res.send(200, "Yay");
});

I want the authorizeOwnership() function to handle 403 (access denied) and 500 (e.g. database query error) responses so that each route doesn't need to do so explicitly.

I have a function that can query the database to check for ownership.

function confirmOwnership(resourceId, userId) {
    // SequelizeJS... returns a bluebird promise
    return Resource.find({
        where: {id: resourceId, userId: userId}
    })
    .then(function(resource) {
        if(!resource) {
            return null; // no match for this resource id + user id
        } else {
            return resource;
        }
    });
}

This is then used in authorizeOwnership:

function authorizeOwnership(req, res) {
    var rid      = parseInt(req.params.rid, 10),
        userId   = parseInt(req.authInfo.userid, 10);

    return new Promise(function(resolve, reject) {
        confirmOwnership(rid, userId)
        .then(function(resource) {
            if(resource === null) {
                res.send(403, "Forbidden");
                // Note: we don't resolve; outer handler will not be called
            } else {
                resolve(resource);
            }
        })
        .catch(function(err) {
            console.log(err);
            res.send(500, "Server error");
            // Note: we don't resolve; outer handler will not be called
        });
    });
}

In this scenario, I deliberately don't call reject() or resolve() in some of the code paths. If I do, then my "outer" route logic (the code that's calling authorizeOwnership()) gets more complicated because it has to handle errors (either with a .catch() or with a null check in .then()).

Two things make me a bit nervous though:

• Is it OK for the promise returned by authorizeOwnership() to never be resolved in error scenarios? Will it cause a delay or a memory leak?

• Is it logically sound to resolve confirmOwnership() with null to say "no matching resource found" and then treat that as an error in authorizeOwnership()? My first attempt rejected the promise in confirmOwnership() when there was no matching resource, but this made things more complicated because it is difficult to distinguish between this (the 403 case) and an actual error (the 500 case).

Answer 1

Is it OK for the promise returned by authorizeOwnership() to never be resolved in error scenarios? Will it cause a delay or a memory leak?

Yes, it is safe to not resolve a Bluebird promise (and to be fair, any other implementation I've checked -pretty pictures here). It has no global state on its own.

The question of whether or not it's good practice is different. It's like a synchronous break in a sense. Personally I'm not a fan.

Is it logically sound to resolve confirmOwnership() with null to say "no matching resource found" and then treat that as an error in authorizeOwnership()?

This depends on your API. It's again an opinion. It works, but I probably would not return null but indicate failure if the case is exceptional. You can distinguish the rejection using an error object. You can reject with an AuthorizationError object you create for example. Note Bluebird also supports typed catches.

Something like:

// probably shouldn't send the response to authorizeOwnership but use it externally
// to be fair, should probably not take req either, but rid and userid
var authorizeOwnership = Promise.method(function(req) {
    var rid      = Number(req.params.rid),
        userId   = Number(req.authInfo.userid;
        return confirmOwnership(rid, userId); // return the promise
    });
});

function ServerError(code,reason){
    this.name = "ServerError";
    this.message = reason;
    this.code = code;
    Error.captureStackTrace(this); // capture stack
}
var confirmOwnership = Promise.method(function(resourceId, userId) {
    // SequelizeJS... returns a bluebird promise
    return Resource.find({
        where: {id: resourceId, userId: userId}
    })
    .then(function(resource) {
        if(!resource) {
            throw new ServerError(403,"User not owner"); // promises are throw safe
        }
        return resource;
    });
});

Next, in your server, you can do something like:

app.post("/foo",function(req,res){
     authorizeOwnership(req).then(function(){
          res.send(200, "Owner Yay!");
     }).catch(ServerError,function(e){
            if(e.code === 403) return res.send(403,e.message);
            return res.send(500,"Internal Server Error");
     });
});

Note: you're also using the deferred anti pattern in your code. There is no need to do new Promise(function(...){ in your code, you can simply return the promise.