Get formatted query from Pool.query() method

[surfbuds]

Solution to resolve #984

var p= Pool.query(sql,data, function (){});
console.log(p.sql); // p.sql is not formatted

This commit changes the behavior of Pool.query() method so as to return the formatted SQL statement as expected, as in Connection.query().

Preventing side effects

Since Pool.query() method uses Connection.query() internally, Connection.query() needs to know if the SQL statement has already been formatted so as to prevent double formatting and corrupting the query.

A new Query object property isFormatted keeps track of formatting status.

Additional method

The Pool.prototype.format method was added to Pool module. Similar to Connection.prototype.format method and modified to work inside the Pool class.

Testing with error inducing query

SELECT name, rdate FROM contacts WHERE id = 1 OR name = 'John ? J ?'

Using the Pool.query method this query could be passed this way

var columns = ['name', 'rdate'];
var userId = 1;
var userName="John ? J ?";
var sql_query = 'SELECT ?? FROM ?? WHERE id=? OR name=?';

var P = pool.query(search, [columns, 'contacts', userId, userName], function(err, rows, fields) {
        if (err) throw err;
        // do whatever
});

In the absence of the isFormatted property, if we allow the Pool.query() method to format (escape and replace placeholders) a query before it is passed to the Connection.query method, the latter will try to reformat the query a second time by replacing the ? inside 'John ? J ?' with another copy of the same string, resulting in an SQL error.

The addition of the isFormatted property to the Query object is crucial for this fix to work. It is undefined by default and evaluates to false in a boolean operation. Therefore no change was made to the lib/protocol/sequences/Query.js file. The property is set after Connection.createQuery() method returns a Query object.

Note: Connection.createQuery() doesn't return a new Query object if it is passed a Query object, thus conserving the isFormatted property if already set.

[dougwilson]

Move this so it always exists, otherwise we are altering the hidden class and fracturing it.

[dougwilson]

i.e. it should be set within createQuery

[dougwilson]

Thanks for this! I won't be able to merge it until there are tests, though. If I have time, I can add some tests, but if you can first, that would be great :)

[surfbuds]

@dougwilson I will have to rewrite my tests to make them easily replicable. I used real tables and data for my test so will need to make self sufficient queries with no table dependency. Do you have some recommendation for the kind of tests you'd like to see?

[dougwilson]

The minimum is a test for the issue you referenced that fails without this and passes with this. For patterns, there are lots and lots of tests to look at in the tests folder :)

If you were wondering, the reason I cannot put things in without tests is because if they don't go in at the same time, they get forgotten and eventually something regresses the fix.

[surfbuds]

@dougwilson I've included a unit test file: test/unit/pool/test-query-return-formatted.js which passes the test when the fix is applied.

Also I moved the isFormatted property default value setting to its own Class, which is Query. The Query object is returned by the Connection.createQuery() method. The isFormatted property is now always set to false by default, and can be changed by both the Pool.query() and Connection.query() methods when they format the query.

[sidorares]

I'm not sure if I'm missing something but it looks like one could just move formatting to createQuery() (maybe if (!(typeof sql == 'object' && 'typeCast' in sql)) { code as well as its duplicated in pool and connection) - it's simple sync code, no need for "isFormatted" flag

[dougwilson]

Yep, I agree with @sidorares if it's possible :) Also, if you're going to introduce a new pool.format method, it probably needs to be added to the documentation at least.

[surfbuds]

@sidorares and @dougwilson if so shouldn't the call to this.format() be just above return new Query(options, bindToCurrentDomain(cb)); in Connection.createQuery ?

One problem seems to be that Connection.createQuery() is a static method not an instance method like Connection.prototype.format(). It could still call the format method by binding createQuery() to the instance of Connection but it seems like it would require more code refactoring overall, especially if createQuery() is used or will be used in other places.

What do you think?

[sidorares]

I think createQuery() should be instance method as well like format() is, to be able to see changes to instance config

[surfbuds]

@sidorares The problem with making createQuery() an instance method is that it is used by other modules or script files expecting it to be a static method since they don't have a Connection instance available at the time the Query object is created.

• test/unit/query/test-stream-before-queue.js (the test would fail)
• index.js
• lib/Pool.js

Any suggestion?

[dougwilson]

In the end, it's really impossible to make this solution look good without it not being backwards-compatible :)

[dougwilson]

I have to think about if it's acceptable that the context of the queryFormat call is not going to be a Connection object in these cases and if this is going to be an issue.

[surfbuds]

I did find a solution to keep createQuery as a static method of Connection and provide the right context for instance config, which keeps things backward compatible. I'll commit the changes later from another machine.

[dougwilson]

Does it take care of this context issue, though?

[dougwilson]

Otherwise there is no need to add a new commit, really. I've already adjusted your commit to make it an instance method (still under your name). The problem right now is backwards compatibility with this function context.