Association mixins refactoring

Fix es6 syntax, docs and changelog

changelog.md

@@ -4,6 +4,7 @@
 - [FIXED] Issue with query generation in MSSQL, an identifier was not escaped [#6686] (https://github.com/sequelize/sequelize/pull/6686)
 - [FIXED] Issue with query generation in MSSQL, an identifier was not escaped [#6686] (https://github.com/sequelize/sequelize/pull/6686)
 - [FIXED] GroupedLimit when foreignKey has a field alias
 - [FIXED] GroupedLimit when foreignKey has a field alias
 - [FIXED] groupedLimit.through.where support
 - [FIXED] groupedLimit.through.where support
+- [FIXED] Issue with overrriding custom methods with association mixins (all association methods are now exposed) [#6682](https://github.com/sequelize/sequelize/issues/6682)
 
 
 # 4.0.0-2
 # 4.0.0-2
 - [ADDED] include now supports string as an argument (on top of model/association), string will expand into an association matched literally from Model.associations
 - [ADDED] include now supports string as an argument (on top of model/association), string will expand into an association matched literally from Model.associations

lib/associations/belongs-to.js

@@ -140,14 +140,9 @@ class BelongsTo extends Association {
   }
   }
 
 
   mixin(obj) {
   mixin(obj) {
-    const association = this;
-
-    obj[this.accessors.get] = function(options) {
-      return association.get(this, options);
-    };
+    const methods = ['get', 'set', 'create'];
 
 
-    association.injectSetter(obj);
-    association.injectCreator(obj);
+    Helpers.mixinMethods(this, obj, methods);
   }
   }
 
 
   get(instances, options) {
   get(instances, options) {
@@ -206,57 +201,47 @@ class BelongsTo extends Association {
         return result;
         return result;
       });
       });
     }
     }
+
     return Target.findOne(options);
     return Target.findOne(options);
   }
   }
 
 
-  // Add setAssociation method to the prototype of the model instance
-  injectSetter(instancePrototype) {
+  set(sourceInstance, associatedInstance, options) {
     const association = this;
     const association = this;
 
 
-    instancePrototype[this.accessors.set] = function(associatedInstance, options) {
-      options = options || {};
+    options = options || {};
 
 
-      let value = associatedInstance;
-      if (associatedInstance instanceof association.target) {
-        value = associatedInstance[association.targetKey];
-      }
-
-      this.set(association.foreignKey, value);
-
-      if (options.save === false) return;
+    let value = associatedInstance;
+    if (associatedInstance instanceof association.target) {
+      value = associatedInstance[association.targetKey];
+    }
 
 
-      options = _.extend({
-        fields: [association.foreignKey],
-        allowNull: [association.foreignKey],
-        association: true
-      }, options);
+    sourceInstance.set(association.foreignKey, value);
 
 
+    if (options.save === false) return;
 
 
-      // passes the changed field to save, so only that field get updated.
-      return this.save(options);
-    };
+    options = _.extend({
+      fields: [association.foreignKey],
+      allowNull: [association.foreignKey],
+      association: true
+    }, options);
 
 
-    return this;
+    // passes the changed field to save, so only that field get updated.
+    return sourceInstance.save(options);
   }
   }
 
 
-  // Add createAssociation method to the prototype of the model instance
-  injectCreator(instancePrototype) {
+  create(sourceInstance, values, fieldsOrOptions) {
     const association = this;
     const association = this;
 
 
-    instancePrototype[this.accessors.create] = function(values, fieldsOrOptions) {
-      const options = {};
-
-      if ((fieldsOrOptions || {}).transaction instanceof Transaction) {
-        options.transaction = fieldsOrOptions.transaction;
-      }
-      options.logging = (fieldsOrOptions || {}).logging;
+    const options = {};
 
 
-      return association.target.create(values, fieldsOrOptions).then(newAssociatedObject =>
-        this[association.accessors.set](newAssociatedObject, options)
-      );
-    };
+    if ((fieldsOrOptions || {}).transaction instanceof Transaction) {
+      options.transaction = fieldsOrOptions.transaction;
+    }
+    options.logging = (fieldsOrOptions || {}).logging;
 
 
-    return this;
+    return association.target.create(values, fieldsOrOptions).then(newAssociatedObject =>
+      sourceInstance[association.accessors.set](newAssociatedObject, options)
+    );
   }
   }
 }
 }
 
 

lib/associations/has-many.js

@@ -233,37 +233,15 @@ class HasMany extends Association {
   }
   }
 
 
   mixin(obj) {
   mixin(obj) {
-    const association = this;
-
-    obj[this.accessors.get] = function(options) {
-      return association.get(this, options);
-    };
-
-    if (this.accessors.count) {
-      obj[this.accessors.count] = function(options) {
-        return association.count(this, options);
-      };
-    }
-
-    obj[this.accessors.hasSingle] = obj[this.accessors.hasAll] = function(instances, options) {
-      return association.has(this, instances, options);
+    const methods = ['get', 'count', 'hasSingle', 'hasAll', 'set', 'add', 'addMultiple', 'remove', 'removeMultiple', 'create'];
+    const aliases = {
+      hasSingle: 'has',
+      hasAll: 'has',
+      addMultiple: 'add',
+      removeMultiple: 'remove'
     };
     };
 
 
-    obj[this.accessors.set] = function(instances, options) {
-      return association.set(this, instances, options);
-    };
-
-    obj[this.accessors.add] = obj[this.accessors.addMultiple] = function(instances, options) {
-      return association.add(this, instances, options);
-    };
-
-    obj[this.accessors.remove] = obj[this.accessors.removeMultiple] = function(instances, options) {
-      return association.remove(this, instances, options);
-    };
-
-    obj[this.accessors.create] = function(values, options) {
-      return association.create(this, values, options);
-    };
+    Helpers.mixinMethods(this, obj, methods, aliases);
   }
   }
 
 
   get(instances, options) {
   get(instances, options) {
@@ -351,7 +329,7 @@ class HasMany extends Association {
     options.raw = true;
     options.raw = true;
     options.plain = true;
     options.plain = true;
 
 
-    return this.get(instance, options).then(result => parseInt(result.count, 10));
+    return association.get(instance, options).then(result => parseInt(result.count, 10));
   }
   }
 
 
   has(sourceInstance, targetInstances, options) {
   has(sourceInstance, targetInstances, options) {
@@ -384,7 +362,7 @@ class HasMany extends Association {
       ]
       ]
     };
     };
 
 
-    return this.get(sourceInstance, options).then(associatedObjects => associatedObjects.length === targetInstances.length);
+    return association.get(sourceInstance, options).then(associatedObjects => associatedObjects.length === targetInstances.length);
   }
   }
 
 
   set(sourceInstance, targetInstances, options) {
   set(sourceInstance, targetInstances, options) {

lib/associations/has-one.js

@@ -138,14 +138,9 @@ class HasOne extends Association {
   }
   }
 
 
   mixin(obj) {
   mixin(obj) {
-    const association = this;
-
-    obj[this.accessors.get] = function(options) {
-      return association.get(this, options);
-    };
+    const methods = ['get', 'set', 'create'];
 
 
-    association.injectSetter(obj);
-    association.injectCreator(obj);
+    Helpers.mixinMethods(this, obj, methods);
   }
   }
 
 
   get(instances, options) {
   get(instances, options) {
@@ -206,71 +201,70 @@ class HasOne extends Association {
     return Target.findOne(options);
     return Target.findOne(options);
   }
   }
 
 
-  injectSetter(instancePrototype) {
+  set(sourceInstance, associatedInstance, options) {
     const association = this;
     const association = this;
 
 
-    instancePrototype[this.accessors.set] = function(associatedInstance, options) {
-      let alreadyAssociated;
+    let alreadyAssociated;
 
 
-      options = _.assign({}, options, {
-        scope: false
-      });
-      return this[association.accessors.get](options).then(oldInstance => {
-        // TODO Use equals method once #5605 is resolved
-        alreadyAssociated = oldInstance && associatedInstance && _.every(association.target.primaryKeyAttributes, attribute =>
-          oldInstance.get(attribute, {raw: true}) === (associatedInstance.get ? associatedInstance.get(attribute, {raw: true}) : associatedInstance)
-        );
-
-        if (oldInstance && !alreadyAssociated) {
-          oldInstance[association.foreignKey] = null;
-          return oldInstance.save(_.extend({}, options, {
-            fields: [association.foreignKey],
-            allowNull: [association.foreignKey],
-            association: true
-          }));
-        }
-      }).then(() => {
-        if (associatedInstance && !alreadyAssociated) {
-          if (!(associatedInstance instanceof association.target)) {
-            const tmpInstance = {};
-            tmpInstance[association.target.primaryKeyAttribute] = associatedInstance;
-            associatedInstance = association.target.build(tmpInstance, {
-              isNewRecord: false
-            });
-          }
-
-          _.assign(associatedInstance, association.scope);
-          associatedInstance.set(association.foreignKey, this.get(association.sourceIdentifier));
-
-          return associatedInstance.save(options);
+    options = _.assign({}, options, {
+      scope: false
+    });
+
+    return sourceInstance[association.accessors.get](options).then(oldInstance => {
+      // TODO Use equals method once #5605 is resolved
+      alreadyAssociated = oldInstance && associatedInstance && _.every(association.target.primaryKeyAttributes, attribute =>
+        oldInstance.get(attribute, {raw: true}) === (associatedInstance.get ? associatedInstance.get(attribute, {raw: true}) : associatedInstance)
+      );
+
+      if (oldInstance && !alreadyAssociated) {
+        oldInstance[association.foreignKey] = null;
+        return oldInstance.save(_.extend({}, options, {
+          fields: [association.foreignKey],
+          allowNull: [association.foreignKey],
+          association: true
+        }));
+      }
+    }).then(() => {
+      if (associatedInstance && !alreadyAssociated) {
+        if (!(associatedInstance instanceof association.target)) {
+          const tmpInstance = {};
+          tmpInstance[association.target.primaryKeyAttribute] = associatedInstance;
+          associatedInstance = association.target.build(tmpInstance, {
+            isNewRecord: false
+          });
         }
         }
-        return null;
-      });
-    };
 
 
-    return this;
+        _.assign(associatedInstance, association.scope);
+        associatedInstance.set(association.foreignKey, sourceInstance.get(association.sourceIdentifier));
+
+        return associatedInstance.save(options);
+      }
+
+      return null;
+    });
   }
   }
 
 
-  injectCreator(instancePrototype) {
+  create(sourceInstance, values, options) {
     const association = this;
     const association = this;
 
 
-    instancePrototype[this.accessors.create] = function(values, options) {
-      values = values || {};
-      options = options || {};
+    values = values || {};
+    options = options || {};
 
 
-      if (association.scope) {
-        for (const attribute of Object.keys(association.scope)) {
-          values[attribute] = association.scope[attribute];
-          if (options.fields) options.fields.push(attribute);
+    if (association.scope) {
+      for (const attribute of Object.keys(association.scope)) {
+        values[attribute] = association.scope[attribute];
+        if (options.fields) {
+          options.fields.push(attribute);
         }
         }
       }
       }
+    }
 
 
-      values[association.foreignKey] = this.get(association.sourceIdentifier);
-      if (options.fields) options.fields.push(association.foreignKey);
-      return association.target.create(values, options);
-    };
+    values[association.foreignKey] = sourceInstance.get(association.sourceIdentifier);
+    if (options.fields) {
+      options.fields.push(association.foreignKey);
+    }
 
 
-    return this;
+    return association.target.create(values, options);
   }
   }
 }
 }
 
 

lib/associations/helpers.js

@@ -44,3 +44,30 @@ function addForeignKeyConstraints(newAttribute, source, target, options, key) {
   }
   }
 }
 }
 exports.addForeignKeyConstraints = addForeignKeyConstraints;
 exports.addForeignKeyConstraints = addForeignKeyConstraints;
+
+/**
+ * Mixin (inject) association methods to model prototype
+ *
+ * @param {Object} Association instance
+ * @param {Object} Model prototype
+ * @param {Array} Method names to inject
+ * @param {Object} Mapping between model and association method names
+ */
+function mixinMethods(association, obj, methods, aliases) {
+  aliases = aliases || {};
+
+  for (const method of methods) {
+    // don't override custom methods
+    if (!obj[association.accessors[method]]) {
+      const realMethod = aliases[method] || method;
+
+      obj[association.accessors[method]] = function() {
+        const instance = this;
+        const args = [instance].concat(Array.from(arguments));
+
+        return association[realMethod].apply(association, args);
+      };
+    }
+  }
+}
+exports.mixinMethods = mixinMethods;

lib/associations/mixin.js

@@ -201,11 +201,10 @@ const Mixin = {
 
 
     // the id is in the foreign table or in a connecting table
     // the id is in the foreign table or in a connecting table
     const association = new BelongsToMany(sourceModel, targetModel, options);
     const association = new BelongsToMany(sourceModel, targetModel, options);
-    sourceModel.associations[association.associationAccessor] = association.injectAttributes();
+    sourceModel.associations[association.associationAccessor] = association;
 
 
-    association.injectGetter(sourceModel.prototype);
-    association.injectSetter(sourceModel.prototype);
-    association.injectCreator(sourceModel.prototype);
+    association.injectAttributes();
+    association.mixin(sourceModel.prototype);
 
 
     return association;
     return association;
   },
   },
@@ -241,15 +240,10 @@ function singleLinked(Type) {
 
 
     // the id is in the foreign table
     // the id is in the foreign table
     const association = new Type(source, target, _.extend(options, source.options));
     const association = new Type(source, target, _.extend(options, source.options));
-    source.associations[association.associationAccessor] = association.injectAttributes();
+    source.associations[association.associationAccessor] = association;
 
 
-    if (association.mixin) {
-      association.mixin(source.prototype);
-    } else {
-      association.injectGetter(source.prototype);
-      association.injectSetter(source.prototype);
-      association.injectCreator(source.prototype);
-    }
+    association.injectAttributes();
+    association.mixin(source.prototype);
 
 
     return association;
     return association;
   };
   };

test/unit/associations/belongs-to-many.test.js

@@ -58,6 +58,39 @@ describe(Support.getTestDialectTeaser('belongsToMany'), function() {
     expect(AB.options.validate).to.deep.equal({});
     expect(AB.options.validate).to.deep.equal({});
   });
   });
 
 
+  it('should not override custom methods with association mixin', function(){
+    const methods = {
+      getTasks: 'get',
+      countTasks: 'count',
+      hasTask: 'has',
+      hasTasks: 'has',
+      setTasks: 'set',
+      addTask: 'add',
+      addTasks: 'add',
+      removeTask: 'remove',
+      removeTasks: 'remove',
+      createTask: 'create',
+    };
+    const User = current.define('User');
+    const Task = current.define('Task');
+
+    current.Utils._.each(methods, (alias, method) => {
+      User.prototype[method] = function () {
+        const realMethod = this.constructor.associations.task[alias];
+        expect(realMethod).to.be.a('function');
+        return realMethod;
+      };
+    });
+
+    User.belongsToMany(Task, { through: 'UserTasks', as: 'task' });
+
+    const user = User.build();
+
+    current.Utils._.each(methods, (alias, method) => {
+      expect(user[method]()).to.be.a('function');
+    });
+  });
+
   describe('timestamps', function () {
   describe('timestamps', function () {
     it('follows the global timestamps true option', function () {
     it('follows the global timestamps true option', function () {
       var User = current.define('User', {})
       var User = current.define('User', {})

test/unit/associations/belongs-to.test.js

+'use strict';
+
+/* jshint -W030 */
+var chai = require('chai')
+  , expect = chai.expect
+  , Support   = require(__dirname + '/../support')
+  , current   = Support.sequelize;
+
+describe(Support.getTestDialectTeaser('belongsTo'), function() {
+  it('should not override custom methods with association mixin', function(){
+    const methods = {
+      getTask : 'get',
+      setTask: 'set',
+      createTask: 'create'
+    };
+    const User = current.define('User');
+    const Task = current.define('Task');
+
+    current.Utils._.each(methods, (alias, method) => {
+      User.prototype[method] = function () {
+        const realMethod = this.constructor.associations.task[alias];
+        expect(realMethod).to.be.a('function');
+        return realMethod;
+      };
+    });
+
+    User.belongsTo(Task, { as: 'task' });
+
+    const user = User.build();
+
+    current.Utils._.each(methods, (alias, method) => {
+      expect(user[method]()).to.be.a('function');
+    });
+  });
+});

test/unit/associations/has-many.test.js

@@ -84,6 +84,37 @@ describe(Support.getTestDialectTeaser('hasMany'), function() {
       expect(obj[association.accessors.hasAll]).to.be.an('function');
       expect(obj[association.accessors.hasAll]).to.be.an('function');
       expect(obj[association.accessors.count]).to.be.an('function');
       expect(obj[association.accessors.count]).to.be.an('function');
     });
     });
+
+    it('should not override custom methods', function(){
+      const methods = {
+        getTasks: 'get',
+        countTasks: 'count',
+        hasTask: 'has',
+        hasTasks: 'has',
+        setTasks: 'set',
+        addTask: 'add',
+        addTasks: 'add',
+        removeTask: 'remove',
+        removeTasks: 'remove',
+        createTask: 'create',
+      };
+
+      current.Utils._.each(methods, (alias, method) => {
+        User.prototype[method] = function () {
+          const realMethod = this.constructor.associations.task[alias];
+          expect(realMethod).to.be.a('function');
+          return realMethod;
+        };
+      });
+
+      User.hasMany(Task, { as: 'task' });
+
+      const user = User.build();
+
+      current.Utils._.each(methods, (alias, method) => {
+        expect(user[method]()).to.be.a('function');
+      });
+    });
   });
   });
 
 
   describe('get', function () {
   describe('get', function () {

test/unit/associations/has-one.test.js

@@ -18,4 +18,30 @@ describe(Support.getTestDialectTeaser('hasOne'), function() {
     User.hasOne(Task, {as : 'Shabda'});
     User.hasOne(Task, {as : 'Shabda'});
     expect(Task.attributes.ShabdaId).not.to.be.empty;
     expect(Task.attributes.ShabdaId).not.to.be.empty;
   });
   });
+
+  it('should not override custom methods with association mixin', function(){
+    const methods = {
+      getTask : 'get',
+      setTask: 'set',
+      createTask: 'create'
+    };
+    const User = current.define('User');
+    const Task = current.define('Task');
+
+    current.Utils._.each(methods, (alias, method) => {
+      User.prototype[method] = function () {
+        const realMethod = this.constructor.associations.task[alias];
+        expect(realMethod).to.be.a('function');
+        return realMethod;
+      };
+    });
+
+    User.hasOne(Task, { as: 'task' });
+
+    const user = User.build();
+
+    current.Utils._.each(methods, (alias, method) => {
+      expect(user[method]()).to.be.a('function');
+    });
+  });
 });
 });