fix: inject foreignKey when using separate:true (#9396)

lib/associations/belongs-to.js

@@ -7,7 +7,6 @@ const Transaction = require('../transaction');
 const Association = require('./base');
 const Association = require('./base');
 const Op = require('../operators');
 const Op = require('../operators');
 
 
-
 /**
 /**
  * One-to-one association
  * One-to-one association
  *
  *
@@ -115,16 +114,17 @@ class BelongsTo extends Association {
   /**
   /**
    * Get the associated instance.
    * Get the associated instance.
    *
    *
-   * @param {Object} [options]
-   * @param {String|Boolean} [options.scope] Apply a scope on the related model, or remove its default scope by passing false.
-   * @param {String} [options.schema] Apply a schema on the related model
+   * @param {Object}         [options]
+   * @param {String|Boolean} [options.scope]  Apply a scope on the related model, or remove its default scope by passing false.
+   * @param {String}         [options.schema] Apply a schema on the related model
+   *
    * @see {@link Model.findOne} for a full explanation of options
    * @see {@link Model.findOne} for a full explanation of options
+   *
    * @return {Promise<Model>}
    * @return {Promise<Model>}
    */
    */
   get(instances, options) {
   get(instances, options) {
-    const association = this;
     const where = {};
     const where = {};
-    let Target = association.target;
+    let Target = this.target;
     let instance;
     let instance;
 
 
     options = Utils.cloneDeep(options);
     options = Utils.cloneDeep(options);
@@ -147,14 +147,14 @@ class BelongsTo extends Association {
     }
     }
 
 
     if (instances) {
     if (instances) {
-      where[association.targetKey] = {
-        [Op.in]: instances.map(instance => instance.get(association.foreignKey))
+      where[this.targetKey] = {
+        [Op.in]: instances.map(instance => instance.get(this.foreignKey))
       };
       };
     } else {
     } else {
-      if (association.targetKeyIsPrimary && !options.where) {
-        return Target.findById(instance.get(association.foreignKey), options);
+      if (this.targetKeyIsPrimary && !options.where) {
+        return Target.findById(instance.get(this.foreignKey), options);
       } else {
       } else {
-        where[association.targetKey] = instance.get(association.foreignKey);
+        where[this.targetKey] = instance.get(this.foreignKey);
         options.limit = null;
         options.limit = null;
       }
       }
     }
     }
@@ -167,11 +167,11 @@ class BelongsTo extends Association {
       return Target.findAll(options).then(results => {
       return Target.findAll(options).then(results => {
         const result = {};
         const result = {};
         for (const instance of instances) {
         for (const instance of instances) {
-          result[instance.get(association.foreignKey, {raw: true})] = null;
+          result[instance.get(this.foreignKey, {raw: true})] = null;
         }
         }
 
 
         for (const instance of results) {
         for (const instance of results) {
-          result[instance.get(association.targetKey, {raw: true})] = instance;
+          result[instance.get(this.targetKey, {raw: true})] = instance;
         }
         }
 
 
         return result;
         return result;
@@ -184,28 +184,26 @@ class BelongsTo extends Association {
   /**
   /**
    * Set the associated model.
    * Set the associated model.
    *
    *
-   * @param {Model|String|Number} [newAssociation] An persisted instance or the primary key of an instance to associate with this. Pass `null` or `undefined` to remove the association.
-   * @param {Object} [options] Options passed to `this.save`
-   * @param {Boolean} [options.save=true] Skip saving this after setting the foreign key if false.
-   * @return {Promise}
+   * @param {Model|String|Number} [newAssociation]    An persisted instance or the primary key of an instance to associate with this. Pass `null` or `undefined` to remove the association.
+   * @param {Object}              [options]           Options passed to `this.save`
+   * @param {Boolean}             [options.save=true] Skip saving this after setting the foreign key if false.
+   *
+   *  @return {Promise}
    */
    */
-  set(sourceInstance, associatedInstance, options) {
-    const association = this;
-
-    options = options || {};
-
+  set(sourceInstance, associatedInstance, options = {}) {
     let value = associatedInstance;
     let value = associatedInstance;
-    if (associatedInstance instanceof association.target) {
-      value = associatedInstance[association.targetKey];
+
+    if (associatedInstance instanceof this.target) {
+      value = associatedInstance[this.targetKey];
     }
     }
 
 
-    sourceInstance.set(association.foreignKey, value);
+    sourceInstance.set(this.foreignKey, value);
 
 
     if (options.save === false) return;
     if (options.save === false) return;
 
 
     options = _.extend({
     options = _.extend({
-      fields: [association.foreignKey],
-      allowNull: [association.foreignKey],
+      fields: [this.foreignKey],
+      allowNull: [this.foreignKey],
       association: true
       association: true
     }, options);
     }, options);
 
 
@@ -218,22 +216,24 @@ class BelongsTo extends Association {
    *
    *
    * @param {Object} [values]
    * @param {Object} [values]
    * @param {Object} [options] Options passed to `target.create` and setAssociation.
    * @param {Object} [options] Options passed to `target.create` and setAssociation.
+   *
    * @see {@link Model#create}  for a full explanation of options
    * @see {@link Model#create}  for a full explanation of options
+   *
    * @return {Promise}
    * @return {Promise}
    */
    */
   create(sourceInstance, values, fieldsOrOptions) {
   create(sourceInstance, values, fieldsOrOptions) {
-    const association = this;
-
     const options = {};
     const options = {};
 
 
+    options.logging = (fieldsOrOptions || {}).logging;
+
     if ((fieldsOrOptions || {}).transaction instanceof Transaction) {
     if ((fieldsOrOptions || {}).transaction instanceof Transaction) {
       options.transaction = fieldsOrOptions.transaction;
       options.transaction = fieldsOrOptions.transaction;
     }
     }
-    options.logging = (fieldsOrOptions || {}).logging;
 
 
-    return association.target.create(values, fieldsOrOptions).then(newAssociatedObject =>
-      sourceInstance[association.accessors.set](newAssociatedObject, options)
-    );
+    return this.target.create(values, fieldsOrOptions)
+      .then(newAssociatedObject =>
+        sourceInstance[this.accessors.set](newAssociatedObject, options)
+      );
   }
   }
 }
 }
 
 

lib/associations/has-one.js

@@ -120,9 +120,9 @@ class HasOne extends Association {
    * @return {Promise<Model>}
    * @return {Promise<Model>}
    */
    */
   get(instances, options) {
   get(instances, options) {
-    const association = this;
     const where = {};
     const where = {};
-    let Target = association.target;
+
+    let Target = this.target;
     let instance;
     let instance;
 
 
     options = Utils.cloneDeep(options);
     options = Utils.cloneDeep(options);
@@ -145,15 +145,15 @@ class HasOne extends Association {
     }
     }
 
 
     if (instances) {
     if (instances) {
-      where[association.foreignKey] = {
-        [Op.in]: instances.map(instance => instance.get(association.sourceKey))
+      where[this.foreignKey] = {
+        [Op.in]: instances.map(instance => instance.get(this.sourceKey))
       };
       };
     } else {
     } else {
-      where[association.foreignKey] = instance.get(association.sourceKey);
+      where[this.foreignKey] = instance.get(this.sourceKey);
     }
     }
 
 
-    if (association.scope) {
-      _.assign(where, association.scope);
+    if (this.scope) {
+      _.assign(where, this.scope);
     }
     }
 
 
     options.where = options.where ?
     options.where = options.where ?
@@ -164,61 +164,61 @@ class HasOne extends Association {
       return Target.findAll(options).then(results => {
       return Target.findAll(options).then(results => {
         const result = {};
         const result = {};
         for (const instance of instances) {
         for (const instance of instances) {
-          result[instance.get(association.sourceKey, {raw: true})] = null;
+          result[instance.get(this.sourceKey, {raw: true})] = null;
         }
         }
 
 
         for (const instance of results) {
         for (const instance of results) {
-          result[instance.get(association.foreignKey, {raw: true})] = instance;
+          result[instance.get(this.foreignKey, {raw: true})] = instance;
         }
         }
 
 
         return result;
         return result;
       });
       });
     }
     }
+
     return Target.findOne(options);
     return Target.findOne(options);
   }
   }
 
 
   /**
   /**
    * Set the associated model.
    * Set the associated model.
    *
    *
-   * @param {Model|String|Number} [newAssociation] An persisted instance or the primary key of a persisted instance to associate with this. Pass `null` or `undefined` to remove the association.
-   * @param {Object} [options] Options passed to getAssociation and `target.save`
+   * @param {Model|String|Number} [associatedInstance] An persisted instance or the primary key of a persisted instance to associate with this. Pass `null` or `undefined` to remove the association.
+   * @param {Object}              [options]            Options passed to getAssociation and `target.save`
+   *
    * @return {Promise}
    * @return {Promise}
    */
    */
   set(sourceInstance, associatedInstance, options) {
   set(sourceInstance, associatedInstance, options) {
-    const association = this;
-
     let alreadyAssociated;
     let alreadyAssociated;
 
 
     options = _.assign({}, options, {
     options = _.assign({}, options, {
       scope: false
       scope: false
     });
     });
 
 
-    return sourceInstance[association.accessors.get](options).then(oldInstance => {
+    return sourceInstance[this.accessors.get](options).then(oldInstance => {
       // TODO Use equals method once #5605 is resolved
       // TODO Use equals method once #5605 is resolved
-      alreadyAssociated = oldInstance && associatedInstance && _.every(association.target.primaryKeyAttributes, attribute =>
+      alreadyAssociated = oldInstance && associatedInstance && _.every(this.target.primaryKeyAttributes, attribute =>
         oldInstance.get(attribute, {raw: true}) === (associatedInstance.get ? associatedInstance.get(attribute, {raw: true}) : associatedInstance)
         oldInstance.get(attribute, {raw: true}) === (associatedInstance.get ? associatedInstance.get(attribute, {raw: true}) : associatedInstance)
       );
       );
 
 
       if (oldInstance && !alreadyAssociated) {
       if (oldInstance && !alreadyAssociated) {
-        oldInstance[association.foreignKey] = null;
+        oldInstance[this.foreignKey] = null;
         return oldInstance.save(_.extend({}, options, {
         return oldInstance.save(_.extend({}, options, {
-          fields: [association.foreignKey],
-          allowNull: [association.foreignKey],
+          fields: [this.foreignKey],
+          allowNull: [this.foreignKey],
           association: true
           association: true
         }));
         }));
       }
       }
     }).then(() => {
     }).then(() => {
       if (associatedInstance && !alreadyAssociated) {
       if (associatedInstance && !alreadyAssociated) {
-        if (!(associatedInstance instanceof association.target)) {
+        if (!(associatedInstance instanceof this.target)) {
           const tmpInstance = {};
           const tmpInstance = {};
-          tmpInstance[association.target.primaryKeyAttribute] = associatedInstance;
-          associatedInstance = association.target.build(tmpInstance, {
+          tmpInstance[this.target.primaryKeyAttribute] = associatedInstance;
+          associatedInstance = this.target.build(tmpInstance, {
             isNewRecord: false
             isNewRecord: false
           });
           });
         }
         }
 
 
-        _.assign(associatedInstance, association.scope);
-        associatedInstance.set(association.foreignKey, sourceInstance.get(association.sourceIdentifier));
+        _.assign(associatedInstance, this.scope);
+        associatedInstance.set(this.foreignKey, sourceInstance.get(this.sourceIdentifier));
 
 
         return associatedInstance.save(options);
         return associatedInstance.save(options);
       }
       }
@@ -232,30 +232,30 @@ class HasOne extends Association {
    *
    *
    * @param {Object} [values]
    * @param {Object} [values]
    * @param {Object} [options] Options passed to `target.create` and setAssociation.
    * @param {Object} [options] Options passed to `target.create` and setAssociation.
+   *
    * @see {@link Model#create} for a full explanation of options
    * @see {@link Model#create} for a full explanation of options
+   *
    * @return {Promise}
    * @return {Promise}
    */
    */
   create(sourceInstance, values, options) {
   create(sourceInstance, values, options) {
-    const association = this;
-
     values = values || {};
     values = values || {};
     options = options || {};
     options = options || {};
 
 
-    if (association.scope) {
-      for (const attribute of Object.keys(association.scope)) {
-        values[attribute] = association.scope[attribute];
+    if (this.scope) {
+      for (const attribute of Object.keys(this.scope)) {
+        values[attribute] = this.scope[attribute];
         if (options.fields) {
         if (options.fields) {
           options.fields.push(attribute);
           options.fields.push(attribute);
         }
         }
       }
       }
     }
     }
 
 
-    values[association.foreignKey] = sourceInstance.get(association.sourceIdentifier);
+    values[this.foreignKey] = sourceInstance.get(this.sourceIdentifier);
     if (options.fields) {
     if (options.fields) {
-      options.fields.push(association.foreignKey);
+      options.fields.push(this.foreignKey);
     }
     }
 
 
-    return association.target.create(values, options);
+    return this.target.create(values, options);
   }
   }
 }
 }
 
 

lib/model.js

@@ -655,16 +655,28 @@ class Model {
       include.separate = true;
       include.separate = true;
     }
     }
 
 
-    if (include.separate === true && !(include.association instanceof HasMany)) {
-      throw new Error('Only HasMany associations support include.separate');
-    }
-
     if (include.separate === true) {
     if (include.separate === true) {
+      if (!(include.association instanceof HasMany)) {
+        throw new Error('Only HasMany associations support include.separate');
+      }
+
       include.duplicating = false;
       include.duplicating = false;
-    }
 
 
-    if (include.separate === true && options.attributes && options.attributes.length && !_.includes(options.attributes, association.source.primaryKeyAttribute)) {
-      options.attributes.push(association.source.primaryKeyAttribute);
+      if (
+        options.attributes
+        && options.attributes.length
+        && !_.includes(_.flattenDepth(options.attributes, 2), association.sourceKey)
+      ) {
+        options.attributes.push(association.sourceKey);
+      }
+
+      if (
+        include.attributes
+        && include.attributes.length
+        && !_.includes(_.flattenDepth(include.attributes, 2), association.foreignKey)
+      ) {
+        include.attributes.push(association.foreignKey);
+      }
     }
     }
 
 
     // Validate child includes
     // Validate child includes
@@ -1769,16 +1781,14 @@ class Model {
 
 
       return include.association.get(results, _.assign(
       return include.association.get(results, _.assign(
         {},
         {},
-        _.omit(options, 'include', 'attributes', 'order', 'where', 'limit', 'offset', 'plain'),
-        _.omit(include, 'parent', 'association', 'as')
+        _.omit(options, ['include', 'attributes', 'originalAttributes', 'order', 'where', 'limit', 'offset', 'plain']),
+        _.omit(include, ['parent', 'association', 'as', 'originalAttributes'])
       )).then(map => {
       )).then(map => {
         for (const result of results) {
         for (const result of results) {
           result.set(
           result.set(
             include.association.as,
             include.association.as,
-            map[result.get(include.association.source.primaryKeyAttribute)],
-            {
-              raw: true
-            }
+            map[result.get(include.association.sourceKey)],
+            { raw: true }
           );
           );
         }
         }
       });
       });

test/integration/include/seperate.test.js → test/integration/include/separate.test.js

@@ -103,6 +103,65 @@ if (current.dialect.supports.groupedLimit) {
         });
         });
       });
       });
 
 
+      it('should work even if include does not specify foreign key attribute with custom sourceKey', function() {
+        const User = this.sequelize.define('User', {
+          name: DataTypes.STRING,
+          userExtraId: {
+            type: DataTypes.INTEGER,
+            unique: true
+          }
+        });
+        const Task = this.sequelize.define('Task', {
+          title: DataTypes.STRING
+        });
+        const sqlSpy = sinon.spy();
+
+        User.Tasks = User.hasMany(Task, {
+          as: 'tasks',
+          foreignKey: 'userId',
+          sourceKey: 'userExtraId'
+        });
+
+        return this.sequelize
+          .sync({force: true})
+          .then(() => {
+            return User.create({
+              id: 1,
+              userExtraId: 222,
+              tasks: [
+                {},
+                {},
+                {}
+              ]
+            }, {
+              include: [User.Tasks]
+            });
+          })
+          .then(() => {
+            return User.findAll({
+              attributes: ['name'],
+              include: [
+                {
+                  attributes: [
+                    'title'
+                  ],
+                  association: User.Tasks,
+                  separate: true
+                }
+              ],
+              order: [
+                ['id', 'ASC']
+              ],
+              logging: sqlSpy
+            });
+          })
+          .then(users => {
+            expect(users[0].get('tasks')).to.be.ok;
+            expect(users[0].get('tasks').length).to.equal(3);
+            expect(sqlSpy).to.have.been.calledTwice;
+          });
+      });
+
       it('should not break a nested include with null values', function() {
       it('should not break a nested include with null values', function() {
         const User = this.sequelize.define('User', {}),
         const User = this.sequelize.define('User', {}),
           Team = this.sequelize.define('Team', {}),
           Team = this.sequelize.define('Team', {}),