fix(query-generator): ignore undefined keys in query (#9548)

lib/dialects/abstract/query-generator.js

@@ -14,6 +14,7 @@ const Association = require('../../associations/base');
 const BelongsTo = require('../../associations/belongs-to');
 const BelongsToMany = require('../../associations/belongs-to-many');
 const HasMany = require('../../associations/has-many');
+const QueryTypes = require('../../query-types');
 const Op = require('../../operators');
 
 const QuoteHelper = require('./query-generator/helpers/quote');
@@ -2062,7 +2063,15 @@ class QueryGenerator {
 
   whereItemQuery(key, value, options) {
     options = options || {};
-    if (key && typeof key === 'string' && key.indexOf('.') !== -1 && options.model) {
+    if (value === undefined) {
+      // protection from stuff like User.delete({where: {id: undefined}})
+      if ([QueryTypes.BULKDELETE, QueryTypes.BULKUPDATE].includes(options.type)) {
+        throw new Error(`WHERE parameter "${key}" of ${options.type} query has value of undefined`);
+      }
+      // for other query types, ignore all where parameters with undefined value
+      return;
+    }
+    if (typeof key === 'string' && key.includes('.') && options.model) {
       const keyParts = key.split('.');
       if (options.model.rawAttributes[keyParts[0]] && options.model.rawAttributes[keyParts[0]].type instanceof DataTypes.JSON) {
         const tmp = {};
@@ -2450,7 +2459,8 @@ class QueryGenerator {
     } else if (_.isPlainObject(smth)) {
       return this.whereItemsQuery(smth, {
         model: factory,
-        prefix: prepend && tableName
+        prefix: prepend && tableName,
+        type: options.type
       });
     } else if (typeof smth === 'number') {
       let primaryKeys = factory ? Object.keys(factory.primaryKeys) : [];

lib/dialects/mssql/query-generator.js

@@ -505,11 +505,11 @@ class MSSQLQueryGenerator extends AbstractQueryGenerator {
     return `TRUNCATE TABLE ${this.quoteTable(tableName)}`;
   }
 
-  deleteQuery(tableName, where, options = {}) {
+  deleteQuery(tableName, where, options = {}, model) {
     const table = this.quoteTable(tableName);
     const query = 'DELETE<%= limit %> FROM <%= table %><%= where %>; SELECT @@ROWCOUNT AS AFFECTEDROWS;';
 
-    where = this.getWhereConditions(where);
+    where = this.getWhereConditions(where, null, model, options);
 
     let limit = '';
 

test/integration/model.test.js

@@ -936,6 +936,22 @@ describe(Support.getTestDialectTeaser('Model'), () => {
       });
     });
 
+    it('throws an error if where has a key with undefined value', function() {
+      const self = this,
+        data = [{ username: 'Peter', secretValue: '42' },
+          { username: 'Paul', secretValue: '42' },
+          { username: 'Bob', secretValue: '43' }];
+
+      return this.User.bulkCreate(data).then(() => {
+        return self.User.update({username: 'Bill'}, {where: {secretValue: '42', username: undefined}}).then(() => {
+          throw new Error('Update should throw an error if where has a key with undefined value');
+        }, err => {
+          expect(err).to.be.an.instanceof(Error);
+          expect(err.message).to.equal('WHERE parameter "username" of BULKUPDATE query has value of undefined');
+        });
+      });
+    });
+
     it('updates only values that match the allowed fields', function() {
       const self = this,
         data = [{ username: 'Peter', secretValue: '42' }];
@@ -1300,6 +1316,19 @@ describe(Support.getTestDialectTeaser('Model'), () => {
       });
     });
 
+    it('throws an error if where has a key with undefined value', function() {
+      const User = this.sequelize.define('User', { username: DataTypes.STRING });
+
+      return this.sequelize.sync({ force: true }).then(() => {
+        return User.destroy({where: {username: undefined}});
+      }).then(() => {
+        throw new Error('Destroy should throw an error if where has a key with undefined value');
+      }, err => {
+        expect(err).to.be.an.instanceof(Error);
+        expect(err.message).to.equal('WHERE parameter "username" of BULKDELETE query has value of undefined');
+      });
+    });
+
     if (current.dialect.supports.transactions) {
       it('supports transactions', function() {
         return Support.prepareTransactionTest(this.sequelize).bind({}).then(sequelize => {

test/integration/model/findAll.test.js

@@ -1455,6 +1455,12 @@ describe(Support.getTestDialectTeaser('Model'), () => {
           });
         });
       });
+
+      it('should ignore undefined in where parameters', function() {
+        return this.User.findAll({where: {username: undefined}}).then(users => {
+          expect(users.length).to.equal(2);
+        });
+      });
     });
   });
 

test/support.js

@@ -219,9 +219,12 @@ const Support = {
 
     if (!expectation) {
       if (expectations['default'] !== undefined) {
-        expectation = expectations['default']
+        expectation = expectations['default'];
+        if (typeof expectation === 'string') {
+          expectation = expectation
             .replace(/\[/g, Support.sequelize.dialect.TICK_CHAR_LEFT)
             .replace(/\]/g, Support.sequelize.dialect.TICK_CHAR_RIGHT);
+        }
       } else {
         throw new Error('Undefined expectation for "' + Support.sequelize.dialect.name + '"!');
       }

test/unit/sql/delete.test.js

 'use strict';
 
 const Support   = require(__dirname + '/../support'),
+  QueryTypes = require('../../../lib/query-types'),
   util = require('util'),
+  _ = require('lodash'),
   expectsql = Support.expectsql,
   current   = Support.sequelize,
   Sequelize = Support.Sequelize,
@@ -22,7 +24,8 @@ suite(Support.getTestDialectTeaser('SQL'), () => {
         where: {},
         truncate: true,
         cascade: true,
-        limit: 10
+        limit: 10,
+        type: QueryTypes.BULKDELETE
       };
 
       test(util.inspect(options, {depth: 2}), () => {
@@ -47,7 +50,8 @@ suite(Support.getTestDialectTeaser('SQL'), () => {
         truncate: true,
         cascade: true,
         restartIdentity: true,
-        limit: 10
+        limit: 10,
+        type: QueryTypes.BULKDELETE
       };
 
       test(util.inspect(options, {depth: 2}), () => {
@@ -69,7 +73,8 @@ suite(Support.getTestDialectTeaser('SQL'), () => {
       const options = {
         table: User.getTableName(),
         where: {name: 'foo' },
-        limit: null
+        limit: null,
+        type: QueryTypes.BULKDELETE
       };
 
       test(util.inspect(options, {depth: 2}), () => {
@@ -93,7 +98,8 @@ suite(Support.getTestDialectTeaser('SQL'), () => {
       const options = {
         table: User.getTableName(),
         where: {name: "foo';DROP TABLE mySchema.myTable;"},
-        limit: 10
+        limit: 10,
+        type: QueryTypes.BULKDELETE
       };
 
       test(util.inspect(options, {depth: 2}), () => {
@@ -117,7 +123,8 @@ suite(Support.getTestDialectTeaser('SQL'), () => {
       const options = {
         table: User.getTableName(),
         where: {name: "foo';DROP TABLE mySchema.myTable;"},
-        limit: 10
+        limit: 10,
+        type: QueryTypes.BULKDELETE
       };
 
       test(util.inspect(options, {depth: 2}), () => {
@@ -158,7 +165,8 @@ suite(Support.getTestDialectTeaser('SQL'), () => {
 
       const options = {
         table: 'test_user',
-        where: { 'test_user_id': 100 }
+        where: { 'test_user_id': 100 },
+        type: QueryTypes.BULKDELETE
       };
 
       test(util.inspect(options, {depth: 2}), () => {
@@ -177,5 +185,27 @@ suite(Support.getTestDialectTeaser('SQL'), () => {
         );
       });
     });
+
+    suite('delete with undefined parameter in where', () => {
+      const options = {
+        table: User.getTableName(),
+        type: QueryTypes.BULKDELETE,
+        where: {name: undefined },
+        limit: null
+      };
+
+      test(util.inspect(options, {depth: 2}), () => {
+        const sqlOrError = _.attempt(
+          sql.deleteQuery.bind(sql),
+          options.table,
+          options.where,
+          options,
+          User
+        );
+        return expectsql(sqlOrError, {
+          default: new Error('WHERE parameter "name" of BULKDELETE query has value of undefined')
+        });
+      });
+    });
   });
 });

test/unit/sql/where.test.js

@@ -2,7 +2,9 @@
 
 const Support = require(__dirname + '/../support'),
   DataTypes = require(__dirname + '/../../../lib/data-types'),
+  QueryTypes = require(__dirname + '/../../../lib/query-types'),
   util = require('util'),
+  _ = require('lodash'),
   expectsql = Support.expectsql,
   current = Support.sequelize,
   sql = current.dialect.QueryGenerator,
@@ -19,7 +21,8 @@ suite(Support.getTestDialectTeaser('SQL'), () => {
       }
 
       test(util.inspect(params, {depth: 10})+(options && ', '+util.inspect(options) || ''), () => {
-        return expectsql(sql.whereQuery(params, options), expectation);
+        const sqlOrError = _.attempt(sql.whereQuery.bind(sql), params, options);
+        return expectsql(sqlOrError, expectation);
       });
     };
 
@@ -29,9 +32,24 @@ suite(Support.getTestDialectTeaser('SQL'), () => {
     testsql([], {
       default: ''
     });
+    testsql({id: undefined}, {
+      default: ''
+    });
     testsql({id: 1}, {
       default: 'WHERE [id] = 1'
     });
+    testsql({id: 1, user: undefined}, {
+      default: 'WHERE [id] = 1'
+    });
+    testsql({id: 1, user: undefined}, {type: QueryTypes.SELECT}, {
+      default: 'WHERE [id] = 1'
+    });
+    testsql({id: 1, user: undefined}, {type: QueryTypes.BULKDELETE}, {
+      default: new Error('WHERE parameter "user" of BULKDELETE query has value of undefined')
+    });
+    testsql({id: 1, user: undefined}, {type: QueryTypes.BULKUPDATE}, {
+      default: new Error('WHERE parameter "user" of BULKUPDATE query has value of undefined')
+    });
     testsql({id: 1}, {prefix: 'User'}, {
       default: 'WHERE [User].[id] = 1'
     });