Accept prebuilt Query object in connection.query

[grncdr]

This allows support code to construct a Query instance to be used with
a connection made available in the future.

My use case is making this ConnectionPool.query method from any-db work directly with mysql Query instances rather than having to wrap them in an adapter (how it works now). By allowing a connection to execute an existing Query instance, we can return the user a query result object synchronously, meaning the user never needs to know about the internals of the connection pool.

[grncdr]

I would like to get feedback on whether you feel this is exposing too much internal stuff before adding the documentation/tests, but if you're ok with this in principle, I can do that this weekend.

[dresende]

I don't see any problem exposing a way of prebuilding a query. I just don't know what QuerySequence exposes, I have to check it out.

[dresende]

Could you give an example on how you would use the Query sequence?

[grncdr]

Sure, as an example, the ConnectionPool.query method linked above would be rewritten to something like this:

ConnectionPool.prototype.query = function (statement, params, callback) {
    var q = this._adapter.createQuery(statement, params, callback)
    var self = this
    this.acquire(function (err, conn) {
        if (err) return callback ? callback(err) : q.emit('error', err)
        conn.query(q)
        var release = self.release.bind(self, conn)
        q.on('end', release)
        q.once('error', function (err) {
            release()
            if (this.listeners('error').length == 0) {
                this.emit('error', err)
            }
        })
    })

    return q
} 

The important part is that I can synchronously return the EventEmitter for the query before I have a connection available.

The createQuery function used above would be something like this (defined in a mysql specific module):

var mysql = require('mysql')

exports.createQuery = function (statement, params, callback) {
    var config = typeof statement == 'object' ? statement : {sql: statement}
    if (typeof params == 'function') {
        callback = params
    } else if (params) {
        config.values = params
    }
    config.sql = queryFormat(config.sql, config.values, config.timezone)
    return new mysql.Query(config, callback)
}

Since this is similar to the logic that already exists inside Connection.prototype.query it might be cleaner & more reliable to to factor that logic out into an exportable function, rather than expose the Query constructor directly.

The one part that I'm not sure how to best address is formatting the SQL string. For my particular use case I'm defining queryFormat myself anyways, but the default behaviour is to use the format method of a connection (which won't work if no connection is available). Should Connection.prototype.query format the sql string of a pre-constructed query?

[dresende]

Wait, what ConnectionPool? One of your own? There's a ConnectionPool pending in the pull requests that will probably be merged. Don't you want to merge your code with that? ConnectionPool could access Query directly without exposing it.

[grncdr]

Wait, what ConnectionPool? One of your own?

Yes, this one from any-db, a driver-agnostic database abstraction layer.

There's a ConnectionPool pending in the pull requests that will probably be merged. Don't you want to merge your code with that?

No, this has nothing to do with the other pull request (FWIW I don't think connection pooling belongs in the mysql driver at all, but that's neither here nor there).

My motivation is to get rid of the wrapper layer that any-db currently has to use around mysql connections and query objects. The mysql driver's Connection objects have an interface that is essentially 100% compatible with any-db, but I need to be able to pre-construct queries to maintain the semantics of the connection pool without wrapping query objects.

[grncdr]

This is a nicer API that will give more consistent behaviour, but I don't particularly like that query formatting has to be destructive. What do you think?

[dresende]

Looks good. What do you think @felixge ?

[felixge]

@dresende had a quick look over the current patch and it LGTM.

[dresende]

Ok, I'm going to pull it and assure tests pass on my machine. I don't know if this should be in the docs, maybe it's something more hardcore.