From b44221da017b8a1353b87da371d879139f777a0d Mon Sep 17 00:00:00 2001 From: Jon Takaki Date: Wed, 11 Feb 2015 16:06:19 +0100 Subject: [PATCH 1/5] Enable returning formatted sql in Pool.query() Address issue #984 var p=Pool.query( sql, vdata, function() {}); console.log(p.sql); // unformatted This commit changes the behavior and returns the formatted sql statement immediately. Also prevents side effect of Connection.query() re-formatting the SQL statement so as not to corrupt the SQL statement. A new connection config variable, isFormatted, keeps track of query formatting. The Pool.prototype.format method added. Similar to Connection.prototype.format, except adapted to use config.connectionConfig. The format() method uses mysql.format() so as not to require SqlString module. --- lib/Connection.js | 12 ++++++++++-- lib/Pool.js | 23 ++++++++++++++++++++++- 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/lib/Connection.js b/lib/Connection.js index 600cbdd8a..de30b3e2a 100644 --- a/lib/Connection.js +++ b/lib/Connection.js @@ -179,8 +179,16 @@ Connection.prototype.query = function(sql, values, cb) { if (!(typeof sql == 'object' && 'typeCast' in sql)) { query.typeCast = this.config.typeCast; } - - query.sql = this.format(query.sql, query.values); + //JM: modified + //query.sql = this.format(query.sql, query.values); + if (!query.isFormatted) { + query.sql = this.format(query.sql, query.values); + console.log("format() method called in Connection"); + } else { + console.log("format() method skipped in Connection"); + } + //JM: new line + query.isFormatted = true; return this._protocol._enqueue(query); }; diff --git a/lib/Pool.js b/lib/Pool.js index 5c589eb00..90882f2b6 100644 --- a/lib/Pool.js +++ b/lib/Pool.js @@ -171,6 +171,9 @@ Pool.prototype.end = function (cb) { Pool.prototype.query = function (sql, values, cb) { var query = Connection.createQuery(sql, values, cb); + //JM:(issue #984) added line + query.isFormatted = false; + if (!(typeof sql === 'object' && 'typeCast' in sql)) { query.typeCast = this.config.connectionConfig.typeCast; } @@ -194,7 +197,16 @@ Pool.prototype.query = function (sql, values, cb) { conn.query(query); }); - + // JM: added + if (!query.isFormatted) { + query.isFormatted = true; + console.log("going to format in Pool.query"); + query.sql = this.format(query.sql, query.values); + console.log("format() method called"); + } else { + console.log("format() method skipped"); + } + // JM:end return query; }; @@ -255,6 +267,15 @@ Pool.prototype.escapeId = function escapeId(value) { return mysql.escapeId(value, false); }; +//JM method:this.config.connectionConfig. +Pool.prototype.format = function(sql, values) { + console.log(this.config.connectionConfig.queryFormat); + if (typeof this.config.connectionConfig.queryFormat == "function") { + return this.config.connectionConfig.queryFormat.call(this, sql, values, this.config.connectionConfig.timezone); + } + return mysql.format(sql, values, this.config.connectionConfig.stringifyObjects, this.config.connectionConfig.timezone); +}; + function spliceConnection(array, connection) { var index; if ((index = array.indexOf(connection)) !== -1) { From f341946485ce5ab5aad7b132898d28f9d7a79b55 Mon Sep 17 00:00:00 2001 From: Jon Takaki Date: Wed, 11 Feb 2015 16:50:48 +0100 Subject: [PATCH 2/5] Enable returning formatted query in Pool.query() ### Addresses issue #984 ```javascript var p= Pool.query(sql,data, function (){}); console.log(p.sql); // un-formatted sql statement ``` 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 connection config variable `isFormatted` keeps track of formatting status. #### Additional method The `Pool.prototype.format` method added to Pool module. Similar to `Connection.prototype.format` method and modified to work inside Pool class. --- lib/Connection.js | 6 ------ lib/Pool.js | 10 +--------- 2 files changed, 1 insertion(+), 15 deletions(-) diff --git a/lib/Connection.js b/lib/Connection.js index de30b3e2a..54dd8d47f 100644 --- a/lib/Connection.js +++ b/lib/Connection.js @@ -179,15 +179,9 @@ Connection.prototype.query = function(sql, values, cb) { if (!(typeof sql == 'object' && 'typeCast' in sql)) { query.typeCast = this.config.typeCast; } - //JM: modified - //query.sql = this.format(query.sql, query.values); if (!query.isFormatted) { query.sql = this.format(query.sql, query.values); - console.log("format() method called in Connection"); - } else { - console.log("format() method skipped in Connection"); } - //JM: new line query.isFormatted = true; return this._protocol._enqueue(query); diff --git a/lib/Pool.js b/lib/Pool.js index 90882f2b6..a0c901279 100644 --- a/lib/Pool.js +++ b/lib/Pool.js @@ -171,7 +171,6 @@ Pool.prototype.end = function (cb) { Pool.prototype.query = function (sql, values, cb) { var query = Connection.createQuery(sql, values, cb); - //JM:(issue #984) added line query.isFormatted = false; if (!(typeof sql === 'object' && 'typeCast' in sql)) { @@ -197,16 +196,11 @@ Pool.prototype.query = function (sql, values, cb) { conn.query(query); }); - // JM: added + if (!query.isFormatted) { query.isFormatted = true; - console.log("going to format in Pool.query"); query.sql = this.format(query.sql, query.values); - console.log("format() method called"); - } else { - console.log("format() method skipped"); } - // JM:end return query; }; @@ -267,9 +261,7 @@ Pool.prototype.escapeId = function escapeId(value) { return mysql.escapeId(value, false); }; -//JM method:this.config.connectionConfig. Pool.prototype.format = function(sql, values) { - console.log(this.config.connectionConfig.queryFormat); if (typeof this.config.connectionConfig.queryFormat == "function") { return this.config.connectionConfig.queryFormat.call(this, sql, values, this.config.connectionConfig.timezone); } From 249b0a36bc11cdc611f90fc6b2f6d7ca58080e17 Mon Sep 17 00:00:00 2001 From: Jon Takaki Date: Thu, 12 Feb 2015 01:23:56 +0100 Subject: [PATCH 3/5] Logical statement redundant assignment operation moved inside a conditional --- lib/Connection.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Connection.js b/lib/Connection.js index 54dd8d47f..09b115429 100644 --- a/lib/Connection.js +++ b/lib/Connection.js @@ -181,8 +181,8 @@ Connection.prototype.query = function(sql, values, cb) { } if (!query.isFormatted) { query.sql = this.format(query.sql, query.values); + query.isFormatted = true; } - query.isFormatted = true; return this._protocol._enqueue(query); }; From 6bf19871f113ebd4dccff4e96df7714aae5b5e5e Mon Sep 17 00:00:00 2001 From: Jon Takaki Date: Mon, 16 Feb 2015 01:22:24 +0100 Subject: [PATCH 4/5] Unit test for fix to issue #984 Use 2 Pool instances to test if formatted SQL query is returned from the pool.query() method. pool1 uses default transformation and pool2 applies a custom queryFormat. --- test/unit/pool/test-query-return-formatted.js | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 test/unit/pool/test-query-return-formatted.js diff --git a/test/unit/pool/test-query-return-formatted.js b/test/unit/pool/test-query-return-formatted.js new file mode 100644 index 000000000..b126ba633 --- /dev/null +++ b/test/unit/pool/test-query-return-formatted.js @@ -0,0 +1,60 @@ +var assert = require('assert'); +var common = require('../../common'); +var pool1 = common.createPool({port: common.fakeServerPort}); +var pool2 = common.createPool({port: common.fakeServerPort, queryFormat: queryFormat}); + + +function queryFormat(query, values, tz) { + if (!values) { + return query; + } + + var escape = this.escape.bind(this); + + return query.replace(/\:(\w+)/g, function (txt, key) { + if (values.hasOwnProperty(key)) { + return escape(values[key]); + } + return txt; + }); +} + +var server = common.createFakeServer(); + +server.listen(common.fakeServerPort, function (err) { + assert.ifError(err); + + + + // When the fix is applied, this query demonstrates that connection.query(), + // which gets invoked internally by pool.query(), will not re-format the query + // a second time. Re-formatting the query a second time in this case would cause + // an error when the question marks inside the string 'John ? J ?' are interpreted + // as placeholders for values. + + assert.equal(pool1.query('SELECT ?? FROM ?? WHERE id=? OR name=?', [['name', 'rdate'], 'dummytable', 1, "John ? J ?"]).sql, 'SELECT `name`, `rdate` FROM `dummytable` WHERE id=1 OR name=\'John ? J ?\''); + + + + // This one will will fail before the fix is applied + + assert.equal(pool2.query('SELECT :a1, :a2', { a1: 1, a2: 'two' }).sql, 'SELECT 1, \'two\''); + + + + // These last 2 will work before and after the fix is applied as there is no transformation performed + + assert.equal(pool2.query('SELECT :a1', []).sql, 'SELECT :a1'); + assert.equal(pool2.query('SELECT :a1').sql, 'SELECT :a1'); + + + + pool1.end(function (err) { + assert.ifError(err); + pool2.end(function (err) { + assert.ifError(err); + server.destroy(); + }); + }); + +}); From 3cd3833af4ffc73191e8fb33c1073a93f0613ac7 Mon Sep 17 00:00:00 2001 From: Jon Takaki Date: Mon, 16 Feb 2015 02:01:36 +0100 Subject: [PATCH 5/5] Moved isFormatted property Moved default Query property isFormatted from Pool module to Query module. When a Query object is first instantiated, isFormatted is false by default. --- lib/Pool.js | 2 -- lib/protocol/sequences/Query.js | 1 + 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/Pool.js b/lib/Pool.js index a0c901279..f024f819d 100644 --- a/lib/Pool.js +++ b/lib/Pool.js @@ -171,8 +171,6 @@ Pool.prototype.end = function (cb) { Pool.prototype.query = function (sql, values, cb) { var query = Connection.createQuery(sql, values, cb); - query.isFormatted = false; - if (!(typeof sql === 'object' && 'typeCast' in sql)) { query.typeCast = this.config.connectionConfig.typeCast; } diff --git a/lib/protocol/sequences/Query.js b/lib/protocol/sequences/Query.js index 0be2914f2..0e70c1c10 100644 --- a/lib/protocol/sequences/Query.js +++ b/lib/protocol/sequences/Query.js @@ -11,6 +11,7 @@ Util.inherits(Query, Sequence); function Query(options, callback) { Sequence.call(this, options, callback); + this.isFormatted = false; this.sql = options.sql; this.values = options.values; this.typeCast = (options.typeCast === undefined)