Merge pull request #5106 from Verdier/master

Do not inject include twice, expand and validate include in aggregate

Merge pull request #5106 from Verdier/master
Do not inject include twice, expand and validate include in aggregate
Mick Hansen
Commit aee7fec4 authored Jan 05, 2016 by Mick Hansen
Showing with 172 additions and 26 deletions
changelog.md
lib/model.js
test/integration/model/count.test.js
test/integration/model/scope/aggregate.test.js
test/integration/model/scope/count.test.js
test/unit/model/scope.test.js
--- a/changelog.md
+++ b/changelog.md
@@ -3,6 +3,8 @@
 - [ADDED] hasOne scope support [#5113](https://github.com/sequelize/sequelize/pull/5113)
 - [FIXED] attributes from multiple scopes does not merge  [#4856](https://github.com/sequelize/sequelize/issues/4856)
 - [FIXED] Support Unicode strings in mssql [#3752](https://github.com/sequelize/sequelize/issues/3752)
+- [FIXED] Do not inject include twice in `options.include` [#5106](https://github.com/sequelize/sequelize/pull/5106)
+- [FIXED] Expand and validate include in `aggregate` [#5106](https://github.com/sequelize/sequelize/pull/5106)
 # 3.15.1
 - [FIXED] calling Model.update() modifies passed values  [#4520](https://github.com/sequelize/sequelize/issues/4520)

--- a/lib/model.js
+++ b/lib/model.js
@@ -601,7 +601,7 @@ validateIncludedElement = function(include, tableNames, options) {
    model = include.association.target.name === include.model.name ? include.association.target : include.association.source;
  }
-  Model.$injectScope(model.$scope, include);
+  model.$injectScope(include);
  // This check should happen after injecting the scope, since the scope may contain a .attributes
  if (!include.attributes) {
@@ -1328,7 +1328,7 @@ Model.prototype.findAll = function(options) {
  return Promise.bind(this).then(function() {
    conformOptions(options, this);
-    Model.$injectScope(this.$scope, options);
+    this.$injectScope(options);
    if (options.hooks) {
      return this.runHooks('beforeFind', options);
@@ -1511,7 +1511,12 @@ Model.prototype.find = Model.prototype.findOne;
 Model.prototype.aggregate = function(attribute, aggregateFunction, options) {
  options = Utils._.extend({ attributes: [] }, options || {});
  conformOptions(options, this);
-  Model.$injectScope(this.$scope, options);
+  this.$injectScope(options);
+  if (options.include) {
+    expandIncludeAll.call(this, options);
+    validateIncludedElements.call(this, options);
+  }
  var attrOptions = this.rawAttributes[attribute]
    , field = attrOptions && attrOptions.field || attribute
@@ -1559,7 +1564,7 @@ Model.prototype.aggregate = function(attribute, aggregateFunction, options) {
 Model.prototype.count = function(options) {
  options = Utils._.clone(options || {});
  conformOptions(options, this);
-  Model.$injectScope(this.$scope, options);
+  this.$injectScope(options);
  var col = '*';
@@ -2242,7 +2247,7 @@ Model.prototype.destroy = function(options) {
  }, options || {});
  options.type = QueryTypes.BULKDELETE;
-  Model.$injectScope(this.$scope, options);
+  this.$injectScope(options);
  Utils.mapOptionFieldNames(options, this);
@@ -2402,7 +2407,7 @@ Model.prototype.update = function(values, options) {
  options.type = QueryTypes.BULKUPDATE;
-  Model.$injectScope(this.$scope, options);
+  this.$injectScope(options);
  // Clone values so it doesn't get modified for caller scope
  values = _.clone(values);
@@ -2606,9 +2611,10 @@ Model.prototype.$expandAttributes = function (options) {
  }
 };
-// Inject current scope into options. Includes should have been conformed (conformOptions) before calling this
+// Inject $scope into options. Includes should have been conformed (conformOptions) before calling this
-Model.$injectScope = function (scope, options) {
+Model.prototype.$injectScope = function (options) {
-  scope = optClone(scope);
+  var self = this;
+  var scope = optClone(this.$scope);
  var filteredScope = _.omit(scope, 'include'); // Includes need special treatment
@@ -2619,15 +2625,18 @@ Model.$injectScope = function (scope, options) {
    options.include = options.include || [];
    // Reverse so we consider the latest include first.
-    // This is used if several scopes specify the same include - the last scope should take precendence
+    // This is used if several scopes specify the same include - the last scope should take precedence
    scope.include.reverse().forEach(function (scopeInclude) {
-      if (scopeInclude.all || !_.any(options.include, function matchesModelAndAlias(item) {
+      if (scopeInclude.all || !options.include.some(function matchesModelAndAlias(item) {
-        var sameModel = item.model && item.model.name === scopeInclude.model.name;
+        var isSameModel = item.model && item.model.name === scopeInclude.model.name;
+        if (!isSameModel || !item.as) return isSameModel;
-        if (sameModel && item.as) {
+        if (scopeInclude.as) {
          return item.as === scopeInclude.as;
+        } else {
+          var association = scopeInclude.association || self.getAssociation(scopeInclude.model, scopeInclude.as);
+          return association ? item.as === association.as : false;
        }
-        return sameModel;
      })) {
        options.include.push(scopeInclude);
      }

--- a/test/integration/model/count.test.js
+++ b/test/integration/model/count.test.js
+'use strict';
+/* jshint -W030 */
+/* jshint -W110 */
+var chai = require('chai')
+  , expect = chai.expect
+  , Support = require(__dirname + '/../support')
+  , DataTypes = require(__dirname + '/../../../lib/data-types');
+describe(Support.getTestDialectTeaser('Model'), function() {
+  beforeEach(function() {
+    this.User = this.sequelize.define('User', {
+      username: DataTypes.STRING
+    });
+    this.Project = this.sequelize.define('Project', {
+      name: DataTypes.STRING
+    });
+    this.User.hasMany(this.Project);
+    this.Project.belongsTo(this.User);
+    return this.sequelize.sync({force: true});
+  });
+  describe('count', function() {
+    beforeEach(function () {
+      var self = this;
+      return this.User.bulkCreate([
+        {username: 'boo'},
+        {username: 'boo2'}
+      ]).then(function () {
+        return self.User.findOne();
+      }).then(function (user) {
+        return user.createProject({
+          name: 'project1'
+        });
+      });
+    });
+    it('should count rows', function () {
+      return expect(this.User.count()).to.eventually.equal(2);
+    });
+    it('should support include', function () {
+      return expect(this.User.count({
+        include: [{
+          model: this.Project,
+          where: {
+            name: 'project1'
+          }
+        }]
+      })).to.eventually.equal(1);
+    });
+  });
+});
--- a/test/integration/model/scope/aggregate.test.js
+++ b/test/integration/model/scope/aggregate.test.js
@@ -5,12 +5,16 @@
 var chai = require('chai')
  , Sequelize = require('../../../../index')
  , expect = chai.expect
-  , Support = require(__dirname + '/../../support');
+  , Support = require(__dirname + '/../../support')
+  , Promise = require(__dirname + '/../../../../lib/promise');
 describe(Support.getTestDialectTeaser('Model'), function() {
  describe('scope', function () {
    describe('aggregate', function () {
      beforeEach(function () {
+        this.Child = this.sequelize.define('Child', {
+          priority: Sequelize.INTEGER
+        });
        this.ScopeMe = this.sequelize.define('ScopeMe', {
          username: Sequelize.STRING,
          email: Sequelize.STRING,
@@ -34,9 +38,19 @@ describe(Support.getTestDialectTeaser('Model'), function() {
            },
            withOrder: {
              order: 'username'
+            },
+            withInclude: {
+              include: [{
+                model: this.Child,
+                where: {
+                  priority: 1
+                }
+              }]
            }
          }
        });
+        this.Child.belongsTo(this.ScopeMe);
+        this.ScopeMe.hasMany(this.Child);
        return this.sequelize.sync({force: true}).then(function() {
          var records = [
@@ -46,7 +60,18 @@ describe(Support.getTestDialectTeaser('Model'), function() {
            {username: 'fred', email: 'fred@foobar.com', access_level: 3, other_value: 7}
          ];
          return this.ScopeMe.bulkCreate(records);
-        }.bind(this));
+        }.bind(this)).then(function () {
+          return this.ScopeMe.findAll();
+        }.bind(this)).then(function (records) {
+          return Promise.all([
+            records[0].createChild({
+              priority: 1
+            }),
+            records[1].createChild({
+              priority: 2
+            })
+          ]);
+        });
      });
      it('should apply defaultScope', function () {
@@ -68,6 +93,18 @@ describe(Support.getTestDialectTeaser('Model'), function() {
      it('should be able to merge scopes with where', function () {
        return expect(this.ScopeMe.scope('lowAccess').aggregate( '*', 'count', { where: { username: 'dan'}})).to.eventually.equal(1);
      });
+      it('should be able to use where on include', function () {
+        return expect(this.ScopeMe.scope('withInclude').aggregate( 'ScopeMe.id', 'count', {
+          plain: true,
+          dataType: new Sequelize.INTEGER(),
+          includeIgnoreAttributes: false,
+          limit: null,
+          offset: null,
+          order: null,
+          attributes: []
+        })).to.eventually.equal(1);
+      });
    });
  });
 });
--- a/test/integration/model/scope/count.test.js
+++ b/test/integration/model/scope/count.test.js
@@ -5,12 +5,16 @@
 var chai = require('chai')
  , Sequelize = require('../../../../index')
  , expect = chai.expect
+  , Promise = require(__dirname + '/../../../../lib/promise')
  , Support = require(__dirname + '/../../support');
 describe(Support.getTestDialectTeaser('Model'), function() {
  describe('scope', function () {
    describe('count', function () {
      beforeEach(function () {
+        this.Child = this.sequelize.define('Child', {
+          priority: Sequelize.INTEGER
+        });
        this.ScopeMe = this.sequelize.define('ScopeMe', {
          username: Sequelize.STRING,
          email: Sequelize.STRING,
@@ -23,7 +27,7 @@ describe(Support.getTestDialectTeaser('Model'), function() {
                gte: 5
              }
            },
-            attributes: ['username', 'email', 'access_level']
+            attributes: ['id', 'username', 'email', 'access_level']
          },
          scopes: {
            lowAccess: {
@@ -35,11 +39,21 @@ describe(Support.getTestDialectTeaser('Model'), function() {
            },
            withOrder: {
              order: 'username'
+            },
+            withInclude: {
+              include: [{
+                model: this.Child,
+                where: {
+                  priority: 1
+                }
+              }]
            }
          }
        });
+        this.Child.belongsTo(this.ScopeMe);
+        this.ScopeMe.hasMany(this.Child);
-        return this.sequelize.sync({force: true}).then(function() {
+        return this.sequelize.sync({force: true}).then(function () {
          var records = [
            {username: 'tony', email: 'tony@sequelizejs.com', access_level: 3, other_value: 7},
            {username: 'tobi', email: 'tobi@fakeemail.com', access_level: 10, other_value: 11},
@@ -47,7 +61,18 @@ describe(Support.getTestDialectTeaser('Model'), function() {
            {username: 'fred', email: 'fred@foobar.com', access_level: 3, other_value: 7}
          ];
          return this.ScopeMe.bulkCreate(records);
-        }.bind(this));
+        }.bind(this)).then(function () {
+          return this.ScopeMe.findAll();
+        }.bind(this)).then(function (records) {
+          return Promise.all([
+            records[0].createChild({
+              priority: 1
+            }),
+            records[1].createChild({
+              priority: 2
+            })
+          ]);
+        });
      });
      it('should apply defaultScope', function () {
@@ -73,6 +98,10 @@ describe(Support.getTestDialectTeaser('Model'), function() {
      it('should ignore the order option if it is found within the scope', function () {
        return expect(this.ScopeMe.scope('withOrder').count()).to.eventually.equal(4);
      });
+      it('should be able to use where on include', function () {
+        return expect(this.ScopeMe.scope('withInclude').count()).to.eventually.equal(1);
+      });
    });
  });
 });
--- a/test/unit/model/scope.test.js
+++ b/test/unit/model/scope.test.js
@@ -253,7 +253,9 @@ describe(Support.getTestDialectTeaser('Model'), function() {
        limit: 9
      };
-      current.Model.$injectScope(scope, options);
+      current.Model.prototype.$injectScope.call({
+        $scope: scope
+      }, options);
      expect(options).to.deep.equal({
        where: {
@@ -275,7 +277,9 @@ describe(Support.getTestDialectTeaser('Model'), function() {
      var options = {};
-      current.Model.$injectScope(scope, options);
+      current.Model.prototype.$injectScope.call({
+        $scope: scope
+      }, options);
      expect(options.include).to.have.length(1);
      expect(options.include[0]).to.deep.equal({ model: Project, where: { something: true }});
@@ -290,7 +294,9 @@ describe(Support.getTestDialectTeaser('Model'), function() {
        include: [{ model: Project, where: { something: true }}]
      };
-      current.Model.$injectScope(scope, options);
+      current.Model.prototype.$injectScope.call({
+        $scope: scope
+      }, options);
      expect(options.include).to.have.length(1);
      expect(options.include[0]).to.deep.equal({ model: Project, where: { something: true }});
@@ -305,7 +311,9 @@ describe(Support.getTestDialectTeaser('Model'), function() {
        include: [{model: User, as: 'otherUser'}]
      };
-      current.Model.$injectScope(scope, options);
+      current.Model.prototype.$injectScope.call({
+        $scope: scope
+      }, options);
      expect(options.include).to.have.length(2);
      expect(options.include[0]).to.deep.equal({model: User, as: 'otherUser'});
@@ -325,7 +333,9 @@ describe(Support.getTestDialectTeaser('Model'), function() {
        ]
      };
-      current.Model.$injectScope(scope, options);
+      current.Model.prototype.$injectScope.call({
+        $scope: scope
+      }, options);
      expect(options.include).to.have.length(2);
      expect(options.include[0]).to.deep.equal({ model: User, where: { something: true }});
@@ -346,7 +356,9 @@ describe(Support.getTestDialectTeaser('Model'), function() {
          ]
        };
-        current.Model.$injectScope(scope, options);
+        current.Model.prototype.$injectScope.call({
+          $scope: scope
+        }, options);
        expect(options.include).to.have.length(2);
        expect(options.include[0]).to.deep.equal({ model: User, where: { something: true }});
@@ -367,7 +379,9 @@ describe(Support.getTestDialectTeaser('Model'), function() {
          ]
        };
-        current.Model.$injectScope(scope, options);
+        current.Model.prototype.$injectScope.call({
+          $scope: scope
+        }, options);
        expect(options.include).to.have.length(2);
        expect(options.include[0]).to.deep.equal({ all: true });