Merge pull request #1749 from overlookmotel/fix-many-to-many-self-associations

Fix many-to-many self-associations

Merge pull request #1749 from overlookmotel/fix-many-to-many-self-associations
Fix many-to-many self-associations
Mick Hansen
Commit 35f34b20 authored May 14, 2014 by Mick Hansen
Showing with 73 additions and 35 deletions
lib/associations/has-many.js
lib/associations/helpers.js
test/associations/self.test.js
--- a/lib/associations/has-many.js
+++ b/lib/associations/has-many.js
@@ -49,17 +49,25 @@ module.exports = (function() {
     */
    this.associationAccessor = this.as
-    if (!this.associationAccessor && (typeof this.through === "string" || Object(this.through) === this.through)) {
+    if (!this.associationAccessor) {
-      this.associationAccessor = this.through.tableName || this.through
+      if (typeof this.through === 'string') {
-    }
+        this.associationAccessor = this.through
-    else if (!this.associationAccessor) {
+      } else if (Object(this.through) === this.through) {
-      this.associationAccessor = this.combinedTableName
+        this.associationAccessor = this.through.tableName
+      } else {
+        this.associationAccessor = this.combinedTableName
+      }
    }
    /*
     * If self association, this is the target association - Unless we find a pairing association
     */
    if (this.isSelfAssociation) {
+      // check 'as' is defined for many-to-many self-association
+      if (this.through && this.through !== true && !this.as) {
+        throw new Error('\'as\' must be defined for many-to-many self-associations')
+      }
      this.targetAssociation = this
    }
@@ -69,15 +77,15 @@ module.exports = (function() {
    if (this.through) {
      _.each(this.target.associations, function (association, accessor) {
        if (self.source === association.target) {
-          var paired = false
+          var paired
          // If through is default, we determine pairing by the accesor value (i.e. DAOFactory's using as won't pair, but regular ones will)
-          if (self.through === true && accessor === self.associationAccessor) {
+          if (self.through === true) {
-            paired = true
+            paired = accessor === self.associationAccessor
          }
          // If through is not default, determine pairing by through value (model/string)
-          if (self.through !== true && self.options.through === association.options.through) {
+          else {
-            paired = true
+            paired = self.options.through === association.options.through
          }
          // If paired, set properties identifying both associations as double linked, and allow them to each eachtoerh
          if (paired) {
@@ -90,13 +98,12 @@ module.exports = (function() {
        }
      })
    }
    /*
     * If we are double linked, and through is either default or a string, we create the through model and set it on both associations
     */
-    if (this.doubleLinked) {
+    if (this.doubleLinked && this.through === true) {
-      if (this.through === true) {
+      this.through = this.combinedTableName
-        this.through = this.combinedTableName
-      }
    }
    if (typeof this.through === "string") {
@@ -150,10 +157,12 @@ module.exports = (function() {
    if ((this.isSelfAssociation && Object(this.through) === this.through) || doubleLinked) {
      // We need to remove the keys that 1:M have added
      if (this.isSelfAssociation && doubleLinked) {
-        if (self.through.rawAttributes[this.targetAssociation.identifier]._autoGenerated) {
+        if (self.through.rawAttributes[this.targetAssociation.identifier]
+            && self.through.rawAttributes[this.targetAssociation.identifier]._autoGenerated) {
          delete self.through.rawAttributes[this.targetAssociation.identifier];
        }
-        if (self.through.rawAttributes[this.targetAssociation.foreignIdentifier]._autoGenerated) {
+        if (self.through.rawAttributes[this.targetAssociation.foreignIdentifier]
+            && self.through.rawAttributes[this.targetAssociation.foreignIdentifier]._autoGenerated) {
          delete self.through.rawAttributes[this.targetAssociation.foreignIdentifier];
        }
      }
@@ -161,19 +170,19 @@ module.exports = (function() {
      this.foreignIdentifier = this.targetAssociation.identifier
      this.targetAssociation.foreignIdentifier = this.identifier
+      if (isForeignKeyDeletionAllowedFor.call(this, this.source, this.foreignIdentifier)) {
+        delete this.source.rawAttributes[this.foreignIdentifier]
+      }
      if (this.isSelfAssociation && this.foreignIdentifier === this.identifier) {
        this.foreignIdentifier = Utils._.camelizeIf(
          [Utils.singularize(this.as, this.source.options.language), this.source.primaryKeyAttribute].join("_"),
          !this.source.options.underscored
-        );
+        )
-      }
-      if (isForeignKeyDeletionAllowedFor.call(this, this.source, this.foreignIdentifier)) {
+        if (doubleLinked) {
-        delete this.source.rawAttributes[this.foreignIdentifier]
+          this.targetAssociation.identifier = this.foreignIdentifier
-      }
+        }
-      if (isForeignKeyDeletionAllowedFor.call(this, this.target, this.identifier)) {
-        delete this.targetAssociation.source.rawAttributes[this.identifier]
      }
      // remove any PKs previously defined by sequelize
@@ -185,8 +194,7 @@ module.exports = (function() {
      })
      // define a new model, which connects the models
-      var combinedTableAttributes = {}
+      var sourceKeyType = this.source.rawAttributes[this.source.primaryKeyAttribute].type
-        , sourceKeyType = this.source.rawAttributes[this.source.primaryKeyAttribute].type
        , targetKeyType = this.target.rawAttributes[this.target.primaryKeyAttribute].type
        , sourceAttribute = { type: sourceKeyType }
        , targetAttribute = { type: targetKeyType }
@@ -206,15 +214,11 @@ module.exports = (function() {
      if (primaryKeyDeleted) {
        targetAttribute.primaryKey = sourceAttribute.primaryKey = true
      } else {
        var uniqueKey = [this.through.tableName, this.identifier, this.foreignIdentifier, 'unique'].join('_')
        targetAttribute.unique = sourceAttribute.unique = uniqueKey
      }
-      combinedTableAttributes[this.identifier] = sourceAttribute
-      combinedTableAttributes[this.foreignIdentifier] = targetAttribute
      if (!this.through.rawAttributes[this.identifier]) {
        this.through.rawAttributes[this.identifier] = {
          _autoGenerated: true
@@ -227,8 +231,8 @@ module.exports = (function() {
        }
      }
-      this.through.rawAttributes[this.identifier] = Utils._.merge(this.through.rawAttributes[this.identifier], sourceAttribute);
+      this.through.rawAttributes[this.identifier] = Utils._.extend(this.through.rawAttributes[this.identifier], sourceAttribute);
-      this.through.rawAttributes[this.foreignIdentifier] = Utils._.merge(this.through.rawAttributes[this.foreignIdentifier], targetAttribute);
+      this.through.rawAttributes[this.foreignIdentifier] = Utils._.extend(this.through.rawAttributes[this.foreignIdentifier], targetAttribute);
      this.through.init(this.through.daoFactoryManager)
    } else {

--- a/lib/associations/helpers.js
+++ b/lib/associations/helpers.js
@@ -3,10 +3,10 @@ var Utils = require("./../utils")
 module.exports = {
  addForeignKeyConstraints: function(newAttribute, source, target, options) {
-    // FK constraints are opt-in: users must either rset `foreignKeyConstraints`
+    // FK constraints are opt-in: users must either set `foreignKeyConstraints`
    // on the association, or request an `onDelete` or `onUpdate` behaviour
-    if(options.foreignKeyConstraint || options.onDelete || options.onUpdate) {
+    if (options.foreignKeyConstraint || options.onDelete || options.onUpdate) {
      // Find primary keys: composite keys not supported with this approach
      var primaryKeys = Utils._.filter(Utils._.keys(source.rawAttributes), function(key) {

--- a/test/associations/self.test.js
+++ b/test/associations/self.test.js
@@ -57,7 +57,41 @@ describe(Support.getTestDialectTeaser("Self"), function() {
    });
  });
-  it('can handle n:m associations', function(done) {
+  it('can handle n:m associations', function() {
+    var self = this
+    var Person = this.sequelize.define('Person', { name: DataTypes.STRING })
+    Person.hasMany(Person, { as: 'Parents', through: 'Family' })
+    Person.hasMany(Person, { as: 'Childs', through: 'Family' })
+    var foreignIdentifiers = _.map(_.values(Person.associations), 'foreignIdentifier')
+    var rawAttributes = _.keys(this.sequelize.models.Family.rawAttributes)
+    expect(foreignIdentifiers.length).to.equal(2)
+    expect(rawAttributes.length).to.equal(4)
+    expect(foreignIdentifiers).to.have.members([ 'PersonId', 'ChildId' ])
+    expect(rawAttributes).to.have.members([ 'createdAt', 'updatedAt', 'PersonId', 'ChildId' ])
+    return this.sequelize.sync({ force: true }).then(function() {
+      return self.sequelize.Promise.all([
+        Person.create({ name: 'Mary' }),
+        Person.create({ name: 'John' }),
+        Person.create({ name: 'Chris' })
+      ]).spread(function (mary, john, chris) {
+        return mary.setParents([john]).then(function() {
+          return chris.addParent(john)
+        }).then(function() {
+          return john.getChilds()
+        }).then(function(children) {
+          expect(_.map(children, 'id')).to.have.members([mary.id, chris.id])
+        })
+      })
+    })
+  })
+  it('can handle n:m associations with pre-defined through table', function(done) {
    var Person = this.sequelize.define('Person', { name: DataTypes.STRING });
    var Family = this.sequelize.define('Family', {
      preexisting_child: {