How can I properly promisify these two database methods for nodejs?

Question

I have a database module which handles connection setup and pooling, and a query module that relies on the database module to execute queries. I'm adapting both of them to use Promises for asynchronous calls. So far, I've adapted the query module - now I wish to convert the database module.

Here's the problem: the database module should be usable both directly and implicitly by the query module (which currently relies on callbacks). How can I use promises in both modules' methods without turning this into a maze of twisty little passages?

Here's what I've done so far:

Database Module

getConnection: function(callback) { //this should return a promise
    this.pool.getConnection(function(error, connection){
        callback(error, connection);
    });
},

Query Module this should then on the getConnection promise, execute query, and then reject/resolve for it's caller

    request: function(queryRequest) {
        return new Promise(function(resolve, reject){
            Database.getConnection(function(error, connection){
                if(error) {
                    reject({error: error, queryRequest: queryRequest});
                } else {
                    connection.query(queryRequest.sql, queryRequest.values, function(error, rows, fields){
                        if(error) {
                            reject({error: error, queryRequest: queryRequest});
                        } else {
                            resolve({rows: rows, fields: fields, queryRequest: queryRequest});
                        }
                        connection.release()
                    });
                }
            });
        });
    },

I'm not sure why you're asking us to fix your particular issue in the Q&A format but I'm not a fan of this sort of question. As I said before, as a promise fan and bluebird contributor I wouldn't mind asking your questions in the appropriate support channels but this question would not teach other users anything. — Benjamin Gruenbaum, Jun 29 '15 at 16:07
It will be better if you give the clear summary of previous question here and also highlight what actually you want to do. — Mr_Green, Jun 29 '15 at 16:10
Besides above it has a use case in context of redis where connection has already failed, and I want to have a promisify getConnection as well, I'm working on a solution for this, will edit shortly. both getConnection and request should be treated independently having their own promises. — user2727195, Jun 29 '15 at 16:11
Where is the question here? If this is intended to be a canonical question, you might want to pick something canonical to answer. — ssube, Jun 29 '15 at 16:20
I took a wild guess at your intent here; please review the edits. — Shog9, Jun 29 '15 at 17:00
@Shog9 your edits are awesome... explained well what I'm looking for, thanks — user2727195, Jun 29 '15 at 17:04

score 2 · Answer 1 · edited May 23 '17 at 12:16

2

getConnection should return a promise [for the connection]

Rather not, it should use the disposer pattern instead, and call connection.release() itself:

function withConnection(callback) {
    var pool = this.pool;
    var conn = new Promise(function(resolve, reject) {
        pool.getConnection(function(error, connection){
            if (error)
                reject(error);
            else
                resolve(connection);
        });
    });
    return conn.then(function(connection) {
        var res = conn.then(callback); // safe call
        return res.then(fin); // `finally` shim
        function fin() {
            connection.release();
            return res;
        }
    });
}

function request(queryRequest) {
    return Database.withConnection(function(connection) {
        return new Promise(function(resolve, reject) {
            connection.query(queryRequest.sql, queryRequest.values, function(error, rows, fields) {
                if (error)
                    reject(error);
                else
                    resolve({rows: rows, fields: fields, queryRequest: queryRequest});
            });
        });
    }).catch(function(error) {
        error.queryRequest = queryRequest;
        throw error;
    });
}

edited May 23 '17 at 12:16

Community

1
1

answered Jun 29 '15 at 16:17

Bergi

630,263
148
957
1,375

This would fail because of context in the first chain (and some other stuff) , but I also don't understand why you'd help with this sort of question in the first place. – Benjamin Gruenbaum Jun 29 '15 at 16:19
I like disposer pattern in the `getConnection` – user2727195 Jun 29 '15 at 16:19
@user2727195 are you using a library? – Benjamin Gruenbaum Jun 29 '15 at 16:20
@BenjaminGruenbaum not using any library, native JavaScript promises only. – user2727195 Jun 29 '15 at 16:20
@BenjaminGruenbaum: where exactly does this fail? – Bergi Jun 29 '15 at 16:26
1

@Bergi what is `this.pool` inside the promise constructor executor callback function? (What is `this`?) – Benjamin Gruenbaum Jun 29 '15 at 16:27
@BenjaminGruenbaum `this.pool = mysql.createPool({...})` – user2727195 Jun 29 '15 at 16:29
@BenjaminGruenbaum: Ah, right, I was confused because you said "chain". The promise constructor callback had the wrong `this` context indeed. – Bergi Jun 29 '15 at 16:32
@Bergi for some reason inside `withConnection` the `callback` `return conn.then(function(connection) {` is not executing, I tested with the `console.log` statement there, also ending up with 500 error, no response received – user2727195 Jun 29 '15 at 16:40
@user2727195: Yes, if `pool.getConnection` calls back with an error, the `conn` promise is rejected immediately and the callback is not executed by design. – Bergi Jun 29 '15 at 16:53

user2727195 · Answer 2 · 2015-06-29T16:26:27.937

-2

Database Module

    getConnection: function() {
        return new Promise(function(resolve, reject) {
            this.pool.getConnection(function(error, connection){
                if(error) {
                    reject(error);
                } else {
                    resolve(connection);
                }
            });
        });
    },

Query Module

    execute: function(queryRequest) {
        var self = this;
        this.queryRequest = queryRequest;

        return new Promise(function(resolve, reject) {
            self.resolve = resolve;
            self.reject = reject;
            var promise = Database.getConnection();
            promise.then(self.result.bind(self), self.fault.bind(self));
        });
    },

    result: function(connection) {
        var self = this;
        connection.query(this.queryRequest.sql, this.queryRequest.values, function(error, rows, fields) {
            if (error) {
                self.reject(error);
            } else {
                self.resolve({rows: rows, fields: fields, queryRequest: self.queryRequest});
            }
            connection.release();
        });
    },

    fault: function(info) {
        self.reject(info);
    }

edited Jun 29 '15 at 16:26

answered Jun 29 '15 at 16:16

user2727195

7,122
17
70
118

1

This answer doesn't have any explanation of what is being changed, how, or why it helps. – ssube Jun 29 '15 at 16:21
`this.queryRequest = queryRequest;`, `self.resolve = resolve;` and `self.reject = reject;` seem to be a very bad idea, as it doesn't allow concurrent requests. Also, you seem to be using the [promise construction antipattern](http://stackoverflow.com/q/23803743/1048572) in your `execute` method (buried in `result` and `fault`) – Bergi Jun 29 '15 at 16:22
to handle concurrent requests, there's a new `QueryModule` instantiated for each request – user2727195 Jun 29 '15 at 16:24
@user2727195: But why would you do that? Instead of a `QueryModule` class, where the user could mistakenly call `.execute` multiple times on the same instance, you should just expose a simple `query` function. – Bergi Jun 29 '15 at 16:28

How can I properly promisify these two database methods for nodejs?

2 Answers2