Improved findall warnings (#7047)

* added validation checks for findall

* unittest input validations checking code for findall

* es6 changes function to =>

* trailing whitespace

* remove unnecessary use of bind given es6 => funcs

* update changelog and fix jshint errors

* fix changelog

* add test to make sure findall calls input validation function

* remove only from test

* remove generic invalid option support and only apply for model attributes

changelog.md

@@ -31,6 +31,7 @@
 - [FIXED] No `value` in built-in/custom validation errors [#3899](https://github.com/sequelize/sequelize/pull/3899)
 - [FIXED] No `value` in built-in/custom validation errors [#3899](https://github.com/sequelize/sequelize/pull/3899)
 - [FIXED] Custom error messages used for incorrect eager loading [#7005](https://github.com/sequelize/sequelize/pull/7005)
 - [FIXED] Custom error messages used for incorrect eager loading [#7005](https://github.com/sequelize/sequelize/pull/7005)
 - [FIXED] Enforce unique association aliases [#7025](https://github.com/sequelize/sequelize/pull/7025)
 - [FIXED] Enforce unique association aliases [#7025](https://github.com/sequelize/sequelize/pull/7025)
+- [FIXED] Information warnings when findAll is given incorrect inputs [#7047](https://github.com/sequelize/sequelize/pull/7047)
 
 
 ## BC breaks:
 ## BC breaks:
 - `DATEONLY` now returns string in `YYYY-MM-DD` format rather than `Date` type
 - `DATEONLY` now returns string in `YYYY-MM-DD` format rather than `Date` type

lib/model.js

@@ -1446,6 +1446,8 @@ class Model {
       throw new Error('The argument passed to findAll must be an options object, use findById if you wish to pass a single primary key value');
       throw new Error('The argument passed to findAll must be an options object, use findById if you wish to pass a single primary key value');
     }
     }
 
 
+    this.warnOnInvalidOptions(options, Object.keys(this.rawAttributes));
+
     const tableNames = {};
     const tableNames = {};
     let originalOptions;
     let originalOptions;
 
 
@@ -1457,13 +1459,6 @@ class Model {
     // set rejectOnEmpty option from model config
     // set rejectOnEmpty option from model config
     options.rejectOnEmpty = options.rejectOnEmpty || this.options.rejectOnEmpty;
     options.rejectOnEmpty = options.rejectOnEmpty || this.options.rejectOnEmpty;
 
 
-    // forgot to pass options.where ?
-    if (!options.where) {
-      const commonKeys =  _.intersection(_.keys(options), _.keys(this.rawAttributes));
-      // jshint -W030
-      commonKeys.length && Utils.warn(`Model attributes (${commonKeys.join(',')}) found in finder method options but options.where object is empty. Did you forget to use options.where?`);
-    }
-
     return Promise.try(() => {
     return Promise.try(() => {
       this._injectScope(options);
       this._injectScope(options);
 
 
@@ -1529,6 +1524,26 @@ class Model {
     });
     });
   }
   }
 
 
+  static warnOnInvalidOptions(options, validColumnNames) {
+    if (!_.isPlainObject(options)) {
+      return;
+    }
+
+    // This list will quickly become dated, but failing to maintain this list just means
+    // we won't throw a warning when we should. At least most common cases will forever be covered
+    // so we stop throwing erroneous warnings when we shouldn't.
+    const validQueryKeywords = ['where', 'attributes', 'paranoid', 'include', 'order', 'limit', 'offset',
+      'transaction', 'lock', 'raw', 'logging', 'benchmark', 'having', 'searchPath', 'rejectOnEmpty', 'plain',
+      'scope', 'group', 'through', 'defaults', 'distinct', 'primary', 'exception', 'type', 'hooks', 'force',
+      'name'];
+
+    const unrecognizedOptions = _.difference(Object.keys(options), validQueryKeywords);
+    const unexpectedModelAttributes = _.intersection(unrecognizedOptions, validColumnNames);
+    if (!options.where && unexpectedModelAttributes.length > 0) {
+      Utils.warn(`Model attributes (${unexpectedModelAttributes.join(', ')}) passed into finder method options, but the options.where object is empty. Did you forget to use options.where?`);
+    }
+  }
+
   static _findSeparate(results, options) {
   static _findSeparate(results, options) {
     if (!options.include || options.raw || !results) return Promise.resolve(results);
     if (!options.include || options.raw || !results) return Promise.resolve(results);
 
 

test/unit/model/findall.test.js

 'use strict';
 'use strict';
 
 
-/* jshint -W030 */
-var chai = require('chai')
-  , expect = chai.expect
-  , Support = require(__dirname + '/../support')
-  , current = Support.sequelize
-  , sinon = require('sinon')
-  , DataTypes = require(__dirname + '/../../../lib/data-types');
-
-describe(Support.getTestDialectTeaser('Model'), function() {
-  describe('method findAll', function () {
-    var Model = current.define('model', {
+const chai = require('chai');
+const expect = chai.expect;
+const Support = require(__dirname + '/../support');
+const current = Support.sequelize;
+const sinon = require('sinon');
+const DataTypes = require(__dirname + '/../../../lib/data-types');
+const Utils = require('../../../lib/utils.js');
+
+describe(Support.getTestDialectTeaser('Model'), () => {
+  describe('warnOnInvalidOptions', () => {
+    beforeEach(() => {
+      this.loggerSpy = sinon.spy(Utils, 'warn');
+    });
+
+    afterEach(() => {
+      this.loggerSpy.restore();
+    });
+
+    it('Warns the user if they use a model attribute without a where clause', () => {
+      const User = current.define('User', {firstName: 'string'});
+      User.warnOnInvalidOptions({firstName : 12, order: []}, ['firstName']);
+      const expectedError = 'Model attributes (firstName) passed into finder method options, but the options.where object is empty. Did you forget to use options.where?';
+      expect(this.loggerSpy.calledWith(expectedError)).to.equal(true);
+    });
+
+    it('Does not warn the user if they use a model attribute without a where clause that shares its name with a query option', () => {
+      const User = current.define('User', {order: 'string'});
+      User.warnOnInvalidOptions({order: []}, ['order']);
+      expect(this.loggerSpy.called).to.equal(false);
+    });
+
+    it('Does not warn the user if they use valid query options', () => {
+      const User = current.define('User', {order: 'string'});
+      User.warnOnInvalidOptions({where: {order: 1}, order: []});
+      expect(this.loggerSpy.called).to.equal(false);
+    });
+  });
+
+  describe('method findAll', () => {
+    const Model = current.define('model', {
       name: DataTypes.STRING
       name: DataTypes.STRING
     }, { timestamps: false });
     }, { timestamps: false });
 
 
-    before(function () {
-      this.stub = sinon.stub(current.getQueryInterface(), 'select', function () {
+    before(() => {
+      this.stub = sinon.stub(current.getQueryInterface(), 'select', () => {
         return Model.build({});
         return Model.build({});
       });
       });
+      this.warnOnInvalidOptionsStub = sinon.stub(Model, 'warnOnInvalidOptions');
     });
     });
 
 
-    beforeEach(function () {
+    beforeEach(() => {
       this.stub.reset();
       this.stub.reset();
+      this.warnOnInvalidOptionsStub.reset();
     });
     });
 
 
-    after(function () {
+    after(() => {
       this.stub.restore();
       this.stub.restore();
+      this.warnOnInvalidOptionsStub.restore();
+    });
+
+    describe('handles input validation', () => {
+      it('calls warnOnInvalidOptions', () => {
+        Model.findAll();
+        expect(this.warnOnInvalidOptionsStub.calledOnce).to.equal(true);
+      });
     });
     });
 
 
-    describe('attributes include / exclude', function () {
-      it('allows me to include additional attributes', function () {
+    describe('attributes include / exclude', () => {
+      it('allows me to include additional attributes', () => {
         return Model.findAll({
         return Model.findAll({
           attributes: {
           attributes: {
             include: ['foobar']
             include: ['foobar']
           }
           }
-        }).bind(this).then(function () {
+        }).then(() => {
           expect(this.stub.getCall(0).args[2].attributes).to.deep.equal([
           expect(this.stub.getCall(0).args[2].attributes).to.deep.equal([
             'id',
             'id',
             'name',
             'name',
@@ -43,25 +82,25 @@ describe(Support.getTestDialectTeaser('Model'), function() {
         });
         });
       });
       });
 
 
-      it('allows me to exclude attributes', function () {
+      it('allows me to exclude attributes', () => {
         return Model.findAll({
         return Model.findAll({
           attributes: {
           attributes: {
             exclude: ['name']
             exclude: ['name']
           }
           }
-        }).bind(this).then(function () {
+        }).then(() => {
           expect(this.stub.getCall(0).args[2].attributes).to.deep.equal([
           expect(this.stub.getCall(0).args[2].attributes).to.deep.equal([
             'id'
             'id'
           ]);
           ]);
         });
         });
       });
       });
 
 
-      it('include takes precendence over exclude', function () {
+      it('include takes precendence over exclude', () => {
         return Model.findAll({
         return Model.findAll({
           attributes: {
           attributes: {
             exclude: ['name'],
             exclude: ['name'],
             include: ['name']
             include: ['name']
           }
           }
-        }).bind(this).then(function () {
+        }).then(() => {
           expect(this.stub.getCall(0).args[2].attributes).to.deep.equal([
           expect(this.stub.getCall(0).args[2].attributes).to.deep.equal([
             'id',
             'id',
             'name'
             'name'
@@ -69,9 +108,9 @@ describe(Support.getTestDialectTeaser('Model'), function() {
         });
         });
       });
       });
 
 
-      it('works for models without PK #4607', function () {
-        var Model = current.define('model', {}, { timestamps: false });
-        var Foo = current.define('foo');
+      it('works for models without PK #4607', () => {
+        const Model = current.define('model', {}, { timestamps: false });
+        const Foo = current.define('foo');
         Model.hasOne(Foo);
         Model.hasOne(Foo);
 
 
         Model.removeAttribute('id');
         Model.removeAttribute('id');
@@ -81,7 +120,7 @@ describe(Support.getTestDialectTeaser('Model'), function() {
             include: ['name']
             include: ['name']
           },
           },
           include: [Foo]
           include: [Foo]
-        }).bind(this).then(function () {
+        }).then(() => {
           expect(this.stub.getCall(0).args[2].attributes).to.deep.equal([
           expect(this.stub.getCall(0).args[2].attributes).to.deep.equal([
             'name'
             'name'
           ]);
           ]);