Merge pull request #1269 from wubzz/bugfix/fix_valuesForUndefined_act…

…ual_query

Fix valuesForUndefined actual query being executed. Fixes #1268

src/dialects/mssql/index.js

@@ -101,7 +101,6 @@ assign(Client_MSSQL.prototype, {
     if (!obj || typeof obj === 'string') obj = {sql: obj}
     // convert ? params into positional bindings (@p1)
     obj.sql = this.positionBindings(obj.sql);
-    obj.bindings = this.prepBindings(obj.bindings) || [];
     return new Promise(function(resolver, rejecter) {
       stream.on('error', rejecter);
       stream.on('end', resolver);
@@ -128,7 +127,6 @@ assign(Client_MSSQL.prototype, {
     if (!obj || typeof obj === 'string') obj = {sql: obj}
     // convert ? params into positional bindings (@p1)
     obj.sql = this.positionBindings(obj.sql);
-    obj.bindings = this.prepBindings(obj.bindings) || [];
     return new Promise(function(resolver, rejecter) {
       var sql = obj.sql
       if (!sql) return resolver()

src/dialects/oracle/index.js

@@ -125,8 +125,6 @@ assign(Client_Oracle.prototype, {
     // convert ? params into positional bindings (:1)
     obj.sql = this.positionBindings(obj.sql);
 
-    obj.bindings = this.prepBindings(obj.bindings) || [];
-
     if (!obj.sql) throw new Error('The query is empty');
 
     return connection.executeAsync(obj.sql, obj.bindings).then(function(response) {

src/interface.js

@@ -5,7 +5,7 @@ import {isArray, map, clone, each} from 'lodash'
 module.exports = function(Target) {
 
   Target.prototype.toQuery = function(tz) {
-    var data = this.toSQL(this._method);
+    var data = this.toSQL(this._method, tz);
     if (!isArray(data)) data = [data];
     return map(data, (statement) => {
       return this._formatQuery(statement.sql, statement.bindings, tz);
@@ -14,9 +14,6 @@ module.exports = function(Target) {
 
   // Format the query as sql, prepping bindings as necessary.
   Target.prototype._formatQuery = function(sql, bindings, tz) {
-    if (this.client && this.client.prepBindings) {
-      bindings = this.client.prepBindings(bindings, tz);
-    }
     return this.client.SqlString.format(sql, bindings, tz);
   };
 

src/query/builder.js

@@ -34,8 +34,8 @@ assign(Builder.prototype, {
   },
 
   // Convert the current query "toSQL"
-  toSQL: function(method) {
-    return this.client.queryCompiler(this).toSQL(method || this._method);
+  toSQL: function(method, tz) {
+    return this.client.queryCompiler(this).toSQL(method || this._method, tz);
   },
 
   // Create a shallow clone of the current query builder.

src/query/compiler.js

@@ -29,7 +29,7 @@ assign(QueryCompiler.prototype, {
   _emptyInsertValue: 'default values',
 
   // Collapse the builder into a single object
-  toSQL: function(method) {
+  toSQL: function(method, tz) {
     method = method || this.method
     var val = this[method]()
     var defaults = {
@@ -44,6 +44,9 @@ assign(QueryCompiler.prototype, {
     if (method === 'select' && this.single.as) {
       defaults.as = this.single.as;
     }
+
+    defaults.bindings = this.client.prepBindings(defaults.bindings || [], tz);
+
     return assign(defaults, val);
   },
 

src/raw.js

@@ -51,7 +51,7 @@ assign(Raw.prototype, {
   },
 
   // Returns the raw sql for the query.
-  toSQL: function() {
+  toSQL: function(method, tz) {
     if (this._cached) return this._cached
     if (Array.isArray(this.bindings)) {
       this._cached = replaceRawArrBindings(this)
@@ -61,7 +61,7 @@ assign(Raw.prototype, {
       this._cached = {
         method: 'raw',
         sql: this.sql,
-        bindings: this.bindings
+        bindings: isUndefined(this.bindings) ? void 0 : [this.bindings]
       }
     }
     if (this._wrappedBefore) {
@@ -74,6 +74,9 @@ assign(Raw.prototype, {
     if(this._timeout) {
       this._cached.timeout = this._timeout;
     }
+    if(this.client && this.client.prepBindings) {
+      this._cached.bindings = this.client.prepBindings(this._cached.bindings || [], tz);
+    }
     return this._cached
   }
 

test/integration/builder/selects.js

@@ -694,8 +694,13 @@ module.exports = function(knex) {
     it('Retains array bindings, #228', function() {
       var raw  = knex.raw('select * from table t where t.id = ANY( ?::int[] )', [[1, 2, 3]]);
       var raw2 = knex.raw('select "stored_procedure"(?, ?, ?)', [1, 2, ['a', 'b', 'c']]);
-      expect(raw.toSQL().bindings).to.eql([[1, 2, 3]]);
-      expect(raw2.toSQL().bindings).to.eql([1, 2, ['a', 'b', 'c']]);
+      var expected1 = [[1, 2, 3]];
+      var expected2 = [1, 2, ['a', 'b', 'c']];
+      expect(raw.toSQL().bindings).to.eql(knex.client.prepBindings(expected1));
+      expect(raw2.toSQL().bindings).to.eql(knex.client.prepBindings(expected2));
+      //Also expect raw's bindings to not have been modified by calling .toSQL() (preserving original bindings)
+      expect(raw.bindings).to.eql(expected1);
+      expect(raw2.bindings).to.eql(expected2);
     });
 
     it('always returns the response object from raw', function() {

test/tape/raw.js

@@ -67,7 +67,7 @@ test('allows for options in raw queries, #605', function(t) {
     sql: "select 'foo', 'bar';",
     options: {rowMode: "array"},
     method: 'raw',
-    bindings: undefined
+    bindings: []
   })
 })
 
@@ -92,3 +92,9 @@ test('raw bindings are optional, #853', function(t) {
   t.deepEqual(sql.bindings, [4])
 
 })
+
+test('raw with no client should still be able to run', function(t) {
+  t.plan(1)
+
+  t.equal(new Raw().set('select * from ?', [raw('table')]).toSQL().sql, 'select * from table')
+});

test/unit/query/builder.js

@@ -28,6 +28,15 @@ var clientsWithNullAsDefault = {
   default:  new Client(useNullAsDefaultConfig)
 }
 
+var valuesForUndefined = {
+  mysql: clients.mysql.valueForUndefined,
+  sqlite3: clients.sqlite3.valueForUndefined,
+  oracle: clients.oracle.valueForUndefined,
+  postgres: clients.postgres.valueForUndefined,
+  mssql: clients.mssql.valueForUndefined,
+  default: clients.default.valueForUndefined
+};
+
 function qb() {
   return clients.default.queryBuilder()
 }
@@ -1372,7 +1381,7 @@ describe("QueryBuilder", function() {
       },
       postgres: {
         sql: 'select * from "users" offset ?',
-        bindings: [5]
+        bindings: ['5']
       },
       oracle: {
         sql: 'select * from (select row_.*, ROWNUM rownum_ from (select * from "users") row_ where rownum <= ?) where rownum_ > ?',
@@ -1930,28 +1939,36 @@ describe("QueryBuilder", function() {
 
   it("normalizes for missing keys in insert", function() {
     var data = [{a: 1}, {b: 2}, {a: 2, c: 3}];
+
+    //This is done because sqlite3 does not support valueForUndefined, and can't manipulate testsql to use 'clientsWithUseNullForUndefined'.
+    //But we still want to make sure that when `useNullAsDefault` is explicitly defined, that the query still works as expected. (Bindings being undefined)
+    //It's reset at the end of the test.
+    var previousValuesForUndefinedSqlite3 = clients.sqlite3.valueForUndefined;
+    clients.sqlite3.valueForUndefined = null;
+
     testsql(qb().insert(data).into('table'), {
       mysql: {
         sql: 'insert into `table` (`a`, `b`, `c`) values (?, ?, ?), (?, ?, ?), (?, ?, ?)',
-        bindings: [1, undefined, undefined, undefined, 2, undefined, 2, undefined, 3]
+        bindings: [1, valuesForUndefined.mysql, valuesForUndefined.mysql, valuesForUndefined.mysql, 2, valuesForUndefined.mysql, 2, valuesForUndefined.mysql, 3]
       },
       sqlite3: {
         sql: 'insert into "table" ("a", "b", "c") select ? as "a", ? as "b", ? as "c" union all select ? as "a", ? as "b", ? as "c" union all select ? as "a", ? as "b", ? as "c"',
         bindings: [1, undefined, undefined, undefined, 2, undefined, 2, undefined, 3]
       },
       oracle: {
         sql: "begin execute immediate 'insert into \"table\" (\"a\", \"b\", \"c\") values (:1, :2, :3)' using ?, ?, ?; execute immediate 'insert into \"table\" (\"a\", \"b\", \"c\") values (:1, :2, :3)' using ?, ?, ?; execute immediate 'insert into \"table\" (\"a\", \"b\", \"c\") values (:1, :2, :3)' using ?, ?, ?;end;",
-        bindings: [1, undefined, undefined, undefined, 2, undefined, 2, undefined, 3]
+        bindings: [1, valuesForUndefined.oracle, valuesForUndefined.oracle, valuesForUndefined.oracle, 2, valuesForUndefined.oracle, 2, valuesForUndefined.oracle, 3]
       },
       mssql: {
         sql: 'insert into [table] ([a], [b], [c]) values (?, ?, ?), (?, ?, ?), (?, ?, ?)',
-        bindings: [1, undefined, undefined, undefined, 2, undefined, 2, undefined, 3]
+        bindings: [1, valuesForUndefined.mssql, valuesForUndefined.mssql, valuesForUndefined.mssql, 2, valuesForUndefined.mssql, 2, valuesForUndefined.mssql, 3]
       },
       default: {
         sql: 'insert into "table" ("a", "b", "c") values (?, ?, ?), (?, ?, ?), (?, ?, ?)',
-        bindings: [1, undefined, undefined, undefined, 2, undefined, 2, undefined, 3]
+        bindings: [1, valuesForUndefined.default, valuesForUndefined.default, valuesForUndefined.default, 2, valuesForUndefined.default, 2, valuesForUndefined.default, 3]
       }
     });
+    clients.sqlite3.valueForUndefined = previousValuesForUndefinedSqlite3;
   });
 
   it("empty insert should be a noop", function() {
@@ -2741,7 +2758,7 @@ describe("QueryBuilder", function() {
       },
       postgres: {
         sql: 'select * from "value" inner join "table" on "table"."array_column"[1] = ?',
-        bindings: [1]
+        bindings: ['1']
       },
       default: {
         sql: 'select * from "value" inner join "table" on "table"."array_column[1]" = ?',
@@ -3282,6 +3299,39 @@ describe("QueryBuilder", function() {
 
     expect(defaultQb.sql).to.equal('select * from "users" where "users"."name" = ? or "users"."name" = ?');
     expect(defaultQb.bindings).to.deep.equal(['Bob', 'Jay']);
+  });
 
+  it('#1268 - valueForUndefined should be in toSQL(QueryCompiler)', function() {
+    testsql(qb().insert([{id: void 0, name: 'test', occupation: void 0}, {id: 1, name: void 0, occupation: 'none'}]).into('users'), {
+      mysql: {
+        sql: 'insert into `users` (`id`, `name`, `occupation`) values (?, ?, ?), (?, ?, ?)',
+        bindings: [valuesForUndefined.mysql, 'test', valuesForUndefined.mysql, 1, valuesForUndefined.mysql, 'none']
+      },
+      oracle: {
+        sql: 'begin execute immediate \'insert into "users" ("id", "name", "occupation") values (:1, :2, :3)\' using ?, ?, ?; execute immediate \'insert into "users" ("id", "name", "occupation") values (:1, :2, :3)\' using ?, ?, ?;end;',
+        bindings: [valuesForUndefined.oracle, 'test', valuesForUndefined.oracle, 1, valuesForUndefined.oracle, 'none']
+      },
+      mssql: {
+        sql: 'insert into [users] ([id], [name], [occupation]) values (?, ?, ?), (?, ?, ?)',
+        bindings: [valuesForUndefined.mssql, 'test', valuesForUndefined.mssql, 1, valuesForUndefined.mssql, 'none']
+      },
+      postgres: {
+        sql: 'insert into "users" ("id", "name", "occupation") values (?, ?, ?), (?, ?, ?)',
+        bindings: [valuesForUndefined.postgres, 'test', valuesForUndefined.postgres, '1', valuesForUndefined.postgres, 'none']
+      }
+    });
+
+    expect(function() {
+      clients.sqlite3.queryBuilder().insert([{id: void 0}]).into('users').toString();
+    })
+    .to
+    .throw(TypeError);
+
+    expect(function() {
+      clientsWithNullAsDefault.sqlite3.queryBuilder().insert([{id: void 0}]).into('users').toString();
+    })
+    .to
+    .not
+    .throw(TypeError);
   });
 });