Prevent Duplicate Aliases in Associations (#7025)

* rewrite logic for finding associations

* update and add new error messgaes and change existing tests to match

* remove comments

* tighten up code for getting associations and keep support for node4

* create SequelizeEagerLoadingError

* remove spaces

* added some happy path unit tests for _getIncludedAssociation

* updated the change log

* remove only

* associations with duplicate aliases are not allowed

* add semi-colon

* place hasAlias in more sensible locatio

* added hasAlias tests

* added association tests for duplicate aliases

* remove unused require

* update changeload

* small formatting changes

* add missing parameter

* remove some basic setup from each association and add to constructor

* add regression tests for improperly constructed belongsToMany associations

* use single quote strings and small const fix

* eslint fixes

* small refactor

* jshint promise expception added

changelog.md

@@ -30,6 +30,7 @@
 - [FIXED] Soft-delete not returning number of affected rows on mssql [#6930](https://github.com/sequelize/sequelize/pull/6930)
 - [FIXED] No `value` in built-in/custom validation errors [#3899](https://github.com/sequelize/sequelize/pull/3899)
 - [FIXED] Custom error messages used for incorrect eager loading [#7005](https://github.com/sequelize/sequelize/pull/7005)
+- [FIXED] Enforce unique association aliases [#7025](https://github.com/sequelize/sequelize/pull/7025)
 
 ## BC breaks:
 - `DATEONLY` now returns string in `YYYY-MM-DD` format rather than `Date` type

lib/associations/base.js

 'use strict';
+const AssociationError = require('./../errors').AssociationError;
 
-var Association = function() {};
+class Association {
+  constructor(source, target, options) {
+    options = options || {};
+    this.source = source;
+    this.target = target;
+    this.options = options;
+    this.scope = options.scope;
+    this.isSelfAssociation = (this.source === this.target);
+    this.as = options.as;
 
-// Normalize input - may be array or single obj, instance or primary key - convert it to an array of built objects
-Association.prototype.toInstanceArray = function(objs) {
-  if (!Array.isArray(objs)) {
-    objs = [objs];
+    if (source.hasAlias(options.as)) {
+      throw new AssociationError(`You have used the alias ${options.as} in two separate associations. ` +
+      'Aliased associations must have unique aliases.'
+      );
+    }
   }
-  return objs.map(function(obj) {
-    if (!(obj instanceof this.target)) {
-      var tmpInstance = {};
-      tmpInstance[this.target.primaryKeyAttribute] = obj;
-      return this.target.build(tmpInstance, {
-        isNewRecord: false
-      });
+  // Normalize input - may be array or single obj, instance or primary key - convert it to an array of built objects
+  toInstanceArray(objs) {
+    if (!Array.isArray(objs)) {
+      objs = [objs];
     }
-    return obj;
-  }, this);
-};
-
-Association.prototype.inspect = function() {
-  return this.as;
-};
+    return objs.map(function(obj) {
+      if (!(obj instanceof this.target)) {
+        var tmpInstance = {};
+        tmpInstance[this.target.primaryKeyAttribute] = obj;
+        return this.target.build(tmpInstance, {
+          isNewRecord: false
+        });
+      }
+      return obj;
+    }, this);
+  }
+  inspect() {
+    return this.as;
+  }
+}
 
-module.exports = Association;
+module.exports = Association;
\ No newline at end of file

lib/associations/belongs-to-many.js

@@ -7,6 +7,7 @@ const Association = require('./base');
 const BelongsTo = require('./belongs-to');
 const HasMany = require('./has-many');
 const HasOne = require('./has-one');
+const AssociationError = require('../errors').AssociationError;
 
 /**
  * Many-to-many association with a join table.
@@ -41,35 +42,27 @@ const HasOne = require('./has-one');
  */
 class BelongsToMany extends Association {
   constructor(source, target, options) {
-    super();
+    super(source, target, options);
 
-    options = options || {};
-
-    if (options.through === undefined || options.through === true || options.through === null) {
-      throw new Error('belongsToMany must be given a through option, either a string or a model');
+    if (this.options.through === undefined || this.options.through === true || this.options.through === null) {
+      throw new AssociationError('belongsToMany must be given a through option, either a string or a model');
     }
 
-    if (!options.through.model) {
-      options.through = {
+    if (!this.options.through.model) {
+      this.options.through = {
         model: options.through
       };
     }
 
     this.associationType = 'BelongsToMany';
-    this.source = source;
-    this.target = target;
     this.targetAssociation = null;
-    this.options = options;
     this.sequelize = source.sequelize;
-    this.through = _.assign({}, options.through);
-    this.scope = options.scope;
+    this.through = _.assign({}, this.options.through);
     this.isMultiAssociation = true;
-    this.isSelfAssociation = this.source === this.target;
     this.doubleLinked = false;
-    this.as = this.options.as;
 
     if (!this.as && this.isSelfAssociation) {
-      throw new Error('\'as\' must be defined for many-to-many self-associations');
+      throw new AssociationError('\'as\' must be defined for many-to-many self-associations');
     }
 
     if (this.as) {

lib/associations/belongs-to.js

@@ -18,16 +18,10 @@ const Association = require('./base');
  */
 class BelongsTo extends Association {
   constructor(source, target, options) {
-    super();
+    super(source, target, options);
 
     this.associationType = 'BelongsTo';
-    this.source = source;
-    this.target = target;
-    this.options = options;
-    this.scope = options.scope;
     this.isSingleAssociation = true;
-    this.isSelfAssociation = (this.source === this.target);
-    this.as = this.options.as;
     this.foreignKeyAttribute = {};
 
     if (this.as) {
@@ -247,4 +241,4 @@ class BelongsTo extends Association {
 
 module.exports = BelongsTo;
 module.exports.BelongsTo = BelongsTo;
-module.exports.default = BelongsTo;
+module.exports.default = BelongsTo;
\ No newline at end of file

lib/associations/has-many.js

@@ -17,26 +17,19 @@ const Association = require('./base');
  */
 class HasMany extends Association {
   constructor(source, target, options) {
-    super();
+    super(source, target, options);
 
     this.associationType = 'HasMany';
-    this.source = source;
-    this.target = target;
     this.targetAssociation = null;
-    this.options = options || {};
     this.sequelize = source.sequelize;
     this.through = options.through;
-    this.scope = options.scope;
     this.isMultiAssociation = true;
-    this.isSelfAssociation = this.source === this.target;
-    this.as = this.options.as;
     this.foreignKeyAttribute = {};
 
     if (this.options.through) {
       throw new Error('N:M associations are not supported with hasMany. Use belongsToMany instead');
     }
 
-
     /*
     * If self association, this is the target association
     */

lib/associations/has-one.js

@@ -17,17 +17,11 @@ const Association = require('./base');
  * @see {@link Model.hasOne}
  */
 class HasOne extends Association {
-  constructor(srcModel, targetModel, options) {
-    super();
+  constructor(source, target, options) {
+    super(source, target, options);
 
     this.associationType = 'HasOne';
-    this.source = srcModel;
-    this.target = targetModel;
-    this.options = options;
-    this.scope = options.scope;
     this.isSingleAssociation = true;
-    this.isSelfAssociation = (this.source === this.target);
-    this.as = this.options.as;
     this.foreignKeyAttribute = {};
 
     if (this.as) {

lib/errors.js

@@ -368,4 +368,18 @@ class EagerLoadingError extends BaseError {
     this.message = message;
   }
 }
-exports.EagerLoadingError = EagerLoadingError;
\ No newline at end of file
+exports.EagerLoadingError = EagerLoadingError;
+
+/**
+ * Thrown when an association is improperly constructed (see message for details)
+ * @extends BaseError
+ * @memberof Errors
+ */
+class AssociationError extends BaseError {
+  constructor(message) {
+    super(message);
+    this.name = 'SequelizeAssociationError';
+    this.message = message;
+  }
+}
+exports.AssociationError = AssociationError;

lib/model.js

@@ -190,7 +190,7 @@ class Model {
     options.include = options.include.map(include => this._conformInclude(include, self));
   }
 
-  static _transformStringAssociation (include, self) {
+  static _transformStringAssociation(include, self) {
     if (self && typeof include === 'string') {
       if (!self.associations.hasOwnProperty(include)) {
         throw new Error('Association with alias "' + include + '" does not exists');
@@ -561,7 +561,7 @@ class Model {
       association = this.getAssociationForAlias(targetModel, targetAlias);
       if (!association) {
         if (targetAlias) {
-          throw new sequelizeErrors.EagerLoadingError(`${targetModel.name} is associated to ${this.name} using an alias. ` + 
+          throw new sequelizeErrors.EagerLoadingError(`${targetModel.name} is associated to ${this.name} using an alias. ` +
             `You've included an alias (${targetAlias}), but it does not match the alias defined in your association.`);
         } else {
           throw new sequelizeErrors.EagerLoadingError(`${targetModel.name} is associated to ${this.name} using an alias. ` +
@@ -574,7 +574,7 @@ class Model {
         throw new sequelizeErrors.EagerLoadingError(`${targetModel.name} is associated to ${this.name} multiple times. ` +
           `To identify the correct association, you must use the 'as' keyword to specify the alias of the association you want to include.`);
       }
-    } 
+    }
     return association;
   }
 
@@ -2788,6 +2788,10 @@ class Model {
     return this.name;
   }
 
+  static hasAlias(alias) {
+    return this.associations.hasOwnProperty(alias);
+  }
+
   /**
    * Builds a new model instance.
    * @param {Object}  [values={}] an object of key value pairs

test/unit/associations/association.test.js

+'use strict';
+
+/* jshint -W030 */
+const chai = require('chai');
+const expect = chai.expect;
+const Support = require(__dirname + '/../support');
+const current = Support.sequelize;
+const AssociationError = require(__dirname + '/../../../lib/errors').AssociationError;
+
+describe(Support.getTestDialectTeaser('belongsTo'), function() {
+  it('should throw an AssociationError when two associations have the same alias', function() {
+    const User = current.define('User');
+    const Task = current.define('Task');
+
+    User.belongsTo(Task, { as: 'task' });
+    const errorFunction = User.belongsTo.bind(User, Task, { as: 'task' });
+    const errorMessage = 'You have used the alias task in two separate associations. Aliased associations must have unique aliases.';
+    expect(errorFunction).to.throw(AssociationError, errorMessage);
+  });
+});

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

 'use strict';
 
 /* jshint -W030 */
-var chai = require('chai')
-  , sinon = require('sinon')
-  , expect = chai.expect
-  , stub = sinon.stub
-  , Support   = require(__dirname + '/../support')
-  , DataTypes = require(__dirname + '/../../../lib/data-types')
-  , BelongsTo = require(__dirname + '/../../../lib/associations/belongs-to')
-  , HasMany = require(__dirname + '/../../../lib/associations/has-many')
-  , HasOne = require(__dirname + '/../../../lib/associations/has-one')
-  , current   = Support.sequelize
-  , Promise   = current.Promise;
+const chai = require('chai');
+const sinon = require('sinon');
+const expect = chai.expect;
+const stub = sinon.stub;
+const Support = require(__dirname + '/../support');
+const DataTypes = require(__dirname + '/../../../lib/data-types');
+const BelongsTo = require(__dirname + '/../../../lib/associations/belongs-to');
+const HasMany = require(__dirname + '/../../../lib/associations/has-many');
+const HasOne = require(__dirname + '/../../../lib/associations/has-one');
+const current = Support.sequelize;
+/* global -Promise */
+const Promise = current.Promise; 
+const AssociationError = require(__dirname + '/../../../lib/errors').AssociationError;
 
 describe(Support.getTestDialectTeaser('belongsToMany'), function() {
   it('should not inherit scopes from parent to join table', function () {
@@ -91,6 +93,26 @@ describe(Support.getTestDialectTeaser('belongsToMany'), function() {
     });
   });
 
+  describe('proper syntax', function() {
+    it('throws an AssociationError if the through option is undefined, true, or null', function() {
+      const User = current.define('User', {});
+      const Task = current.define('Task', {});
+
+      const errorFunction1 = User.belongsToMany.bind(User, Task, { through: true });
+      const errorFunction2 = User.belongsToMany.bind(User, Task, { through: undefined });
+      const errorFunction3 = User.belongsToMany.bind(User, Task, { through: null });
+      for (const errorFunction of [errorFunction1, errorFunction2, errorFunction3]) {
+        expect(errorFunction).to.throw(AssociationError, 'belongsToMany must be given a through option, either a string or a model');
+      }
+    });
+    it('throws an AssociationError for a self-association defined without an alias', function() {
+      const User = current.define('User', {});
+
+      const errorFunction = User.belongsToMany.bind(User, User, {through: 'jointable'});
+      expect(errorFunction).to.throw(AssociationError, '\'as\' must be defined for many-to-many self-associations');
+    });
+  });
+
   describe('timestamps', function () {
     it('follows the global timestamps true option', function () {
       var User = current.define('User', {})

test/unit/model/helpers.test.js

+'use strict';
+
+/* jshint -W030 */
+const chai = require('chai');
+const expect = chai.expect;
+const Support = require(__dirname + '/../support');
+const current = Support.sequelize;
+
+describe(Support.getTestDialectTeaser('Model'), function() {
+  describe('hasAlias', function () {
+    beforeEach(function() {
+      this.User = current.define('user');
+      this.Task = current.define('task'); 
+    });
+
+    it('returns true if a model has an association with the specified alias', function() {
+      this.Task.belongsTo(this.User, { as: 'owner'});
+      expect(this.Task.hasAlias('owner')).to.equal(true);
+    });
+    
+    it('returns false if a model does not have an association with the specified alias', function() {
+      this.Task.belongsTo(this.User, { as: 'owner'});
+      expect(this.Task.hasAlias('notOwner')).to.equal(false);
+    });
+  });
+});