Don't try to select the primary key for models without primary key Cl…

…oses #4607
sequelize · Oct 11, 2015 · 5274b00 · 5274b00
1 parent f6b7855
commit 5274b00
Show file tree

Hide file tree

Showing 4 changed files with 52 additions and 17 deletions.
diff --git a/changelog.md b/changelog.md
@@ -1,6 +1,7 @@
 # Next
 - [FIXED] Partial rollback of datatype validations by hiding it behind the `typeValidation` flag.
 - [FIXED] Don't try to select the primary key for models without primary key [#4607](https://github.com/sequelize/sequelize/issues/4607)
+- [FIXED] Apply `attributes` when including a scoped model. [#4625](https://github.com/sequelize/sequelize/issues/4625)
 
 # 3.11.0
 - [INTERNALS] Updated dependencies [#4594](https://github.com/sequelize/sequelize/pull/4594)

diff --git a/lib/model.js b/lib/model.js
@@ -241,6 +241,10 @@ var findAutoIncrementField = function() {
 };
 
 function conformOptions(options, self) {
+  if (self) {
+    self.$expandAttributes(options);
+  }
+
   if (!options.include) {
     return;
   }
@@ -520,7 +524,7 @@ validateIncludedElement = function(include, tableNames, options) {
   tableNames[include.model.getTableName()] = true;
 
   if (include.attributes && !options.raw) {
-    include.model.$handleAttributes(include);
+    include.model.$expandAttributes(include);
 
     // Need to make sure virtuals are mapped before setting originalAttributes
     include = Utils.mapFinderOptions(include, include.model);
@@ -542,14 +546,11 @@ validateIncludedElement = function(include, tableNames, options) {
     }
   } else {
     include = Utils.mapFinderOptions(include, include.model);
-
-    if (!include.attributes) {
-      include.attributes = Object.keys(include.model.tableAttributes);
-    }
   }
 
   // pseudo include just needed the attribute logic, return
   if (include._pseudo) {
+    include.attributes = Object.keys(include.model.tableAttributes);
     return Utils.mapFinderOptions(include, include.model);
   }
 
@@ -607,6 +608,11 @@ validateIncludedElement = function(include, tableNames, options) {
 
   Model.$injectScope(model.$scope, include);
 
+  // This check should happen after injecting the scope, since the scope may contain a .attributes
+  if (!include.attributes) {
+    include.attributes = Object.keys(include.model.tableAttributes);
+  }
+
   include = Utils.mapFinderOptions(include, include.model);
 
   if (include.required === undefined) {
@@ -689,14 +695,14 @@ Model.prototype.init = function(modelManager) {
   this.$scope = this.options.defaultScope || {};
 
   if (_.isPlainObject(this.$scope)) {
-    conformOptions(this.$scope);
+    conformOptions(this.$scope, this);
   }
 
   _.each(this.options.scopes, function (scope) {
     if (_.isPlainObject(scope)) {
-      conformOptions(scope);
+      conformOptions(scope, this);
     }
-  });
+  }, this);
 
   // Instance prototype
   this.Instance = function() {
@@ -1096,7 +1102,7 @@ Model.prototype.addScope = function (name, scope, options) {
     throw new Error('The scope ' + name + ' already exists. Pass { override: true } as options to silence this error');
   }
 
-  conformOptions(scope);
+  conformOptions(scope, this);
 
   if (name === 'defaultScope') {
     this.options.defaultScope = this.$scope = scope;
@@ -1190,7 +1196,7 @@ Model.prototype.scope = function(option) {
 
         if (_.isFunction(scope)) {
           scope = scope();
-          conformOptions(scope);
+          conformOptions(scope, self);
         }
       }
     }
@@ -1346,8 +1352,6 @@ Model.prototype.findAll = function(options) {
       return this.runHooks('beforeFindAfterExpandIncludeAll', options);
     }
   }).then(function() {
-    this.$handleAttributes(options);
-
     if (options.include) {
       options.hasJoin = true;
 
@@ -1360,6 +1364,10 @@ Model.prototype.findAll = function(options) {
       }
     }
 
+    if (!options.attributes) {
+      options.attributes = Object.keys(this.tableAttributes);
+    }
+
     // whereCollection is used for non-primary key updates
     this.options.whereCollection = options.where || null;
 
@@ -2579,10 +2587,10 @@ Model.prototype.$getDefaultTimestamp = function(attr) {
   return undefined;
 };
 
-Model.prototype.$handleAttributes = function (options) {
-  var attributes = Array.isArray(options.attributes) ? options.attributes : Object.keys(this.tableAttributes);
-
+Model.prototype.$expandAttributes = function (options) {
   if (_.isPlainObject(options.attributes)) {
+    var attributes = Object.keys(this.tableAttributes);
+
     if (options.attributes.exclude) {
       attributes = attributes.filter(function (elem) {
         return options.attributes.exclude.indexOf(elem) === -1;
@@ -2591,9 +2599,9 @@ Model.prototype.$handleAttributes = function (options) {
     if (options.attributes.include) {
       attributes = attributes.concat(options.attributes.include);
     }
-  }
 
-  options.attributes = attributes;
+    options.attributes = attributes;
+  }
 };
 
 // Inject current scope into options. Includes should have been conformed (conformOptions) before calling this

diff --git a/test/unit/model/include.test.js b/test/unit/model/include.test.js
@@ -157,6 +157,9 @@ describe(Support.getTestDialectTeaser('Model'), function() {
               where: { that: false },
               limit: 12
             },
+            attr: {
+              attributes: ['baz']
+            },
             foobar: {
               where: {
                 bar: 42
@@ -189,6 +192,15 @@ describe(Support.getTestDialectTeaser('Model'), function() {
         expect(options.include[0]).to.have.property('limit').which.equals(12);
       });
 
+      it('adds the attributes from a scoped model', function () {
+        var options = Sequelize.Model.$validateIncludedElements({
+          model: this.User,
+          include: [{ model: this.Project.scope('attr') }]
+        });
+
+        expect(options.include[0]).to.have.property('attributes').which.deep.equals(['baz']);
+      });
+
       it('merges where with the where from a scoped model', function () {
         var options = Sequelize.Model.$validateIncludedElements({
           model: this.User,

diff --git a/test/unit/model/scope.test.js b/test/unit/model/scope.test.js
@@ -217,6 +217,20 @@ describe(Support.getTestDialectTeaser('Model'), function() {
         include: [{ model: Project }]
       });
     });
+
+    it('works with exclude and include attributes', function () {
+      Company.addScope('newIncludeScope', {
+        attributes: {
+          include: ['foobar'],
+          exclude: ['createdAt']
+        }
+      });
+
+      expect(Company.scope('newIncludeScope').$scope).to.deep.equal({
+        attributes: ['id', 'updatedAt', 'foobar']
+      });
+    });
+
   });
 
   describe('$injectScope', function () {