Make Eager Loading Error Messages More User-Friendly (#7005)

* 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

changelog.md

@@ -28,6 +28,7 @@
 - [FIXED] HSTORE and JSON fields being renamed when `options.field` is specified on a matching model attribute
 - [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)
 
 ## BC breaks:
 - `DATEONLY` now returns string in `YYYY-MM-DD` format rather than `Date` type

lib/associations/mixin.js

@@ -209,18 +209,21 @@ const Mixin = {
     return association;
   },
 
-  getAssociation(target, alias) {
-    for (const associationName in this.associations) {
-      if (this.associations.hasOwnProperty(associationName)) {
-        const association = this.associations[associationName];
+  getAssociations(target) {
+    return _.values(this.associations).filter(association => association.target.name === target.name);
+  },
 
-        if (association.target.name === target.name && (alias === undefined ? !association.isAliased : association.as === alias)) {
-          return association;
-        }
-      }
-    }
+  getAssociationForAlias(target, alias) {
+    // Two associations cannot have the same alias, so we can use find instead of filter
+    return this.getAssociations(target).find(association => this.verifyAssociationAlias(association, alias)) || null;
+  },
 
-    return null;
+  verifyAssociationAlias(association, alias) {
+    if (alias) {
+      return association.as === alias;
+    } else {
+      return !association.isAliased;
+    }
   }
 };
 

lib/dialects/abstract/query-generator.js

@@ -651,7 +651,7 @@ const QueryGenerator = {
           association = {as: model.name};
         } else {
           // find applicable association for linking parent to this model
-          association = parent.getAssociation(model, as);
+          association = parent.getAssociationForAlias(model, as);
         }
 
         if (association) {

lib/errors.js

@@ -355,3 +355,17 @@ class EmptyResultError extends BaseError {
   }
 }
 exports.EmptyResultError = EmptyResultError;
+
+/**
+ * Thrown when an include statement is improperly constructed (see message for details)
+ * @extends BaseError
+ * @memberof Errors
+ */
+class EagerLoadingError extends BaseError {
+  constructor(message) {
+    super(message);
+    this.name = 'SequelizeEagerLoadingError';
+    this.message = message;
+  }
+}
+exports.EagerLoadingError = EagerLoadingError;
\ No newline at end of file

lib/model.js

@@ -274,7 +274,7 @@ class Model {
 
         const types = validTypes[type];
         if (!types) {
-          throw new Error('include all \'' + type + '\' is not valid - must be BelongsTo, HasOne, HasMany, One, Has, Many or All');
+          throw new sequelizeErrors.EagerLoadingError('include all \'' + type + '\' is not valid - must be BelongsTo, HasOne, HasMany, One, Has, Many or All');
         }
 
         if (types !== true) {
@@ -472,19 +472,7 @@ class Model {
     }
 
     // check if the current Model is actually associated with the passed Model - or it's a pseudo include
-    const association = include.association || this.getAssociation(include.model, include.as);
-
-    if (!association) {
-      let msg = include.model.name;
-
-      if (include.as) {
-        msg += ' (' + include.as + ')';
-      }
-
-      msg += ' is not associated to ' + this.name + '!';
-
-      throw new Error(msg);
-    }
+    const association = include.association || this._getIncludedAssociation(include.model, include.as);
 
     include.association = association;
     include.as = association.as;
@@ -564,6 +552,33 @@ class Model {
     return include;
   }
 
+  static _getIncludedAssociation(targetModel, targetAlias) {
+    const associations = this.getAssociations(targetModel);
+    let association = null;
+    if (associations.length === 0) {
+      throw new sequelizeErrors.EagerLoadingError(`${targetModel.name} is not associated to ${this.name}!`);
+    } else if (associations.length === 1) {
+      association = this.getAssociationForAlias(targetModel, targetAlias);
+      if (!association) {
+        if (targetAlias) {
+          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. ` +
+            `You must use the 'as' keyword to specify the alias within your include statement.`);
+        }
+      }
+    } else {
+      association = this.getAssociationForAlias(targetModel, targetAlias);
+      if (!association){
+        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;
+  }
+
+
   static _expandIncludeAll(options) {
     const includes = options.include;
     if (!includes) {
@@ -2761,7 +2776,7 @@ class Model {
           if (scopeInclude.as) {
             return item.as === scopeInclude.as;
           } else {
-            const association = scopeInclude.association || this.getAssociation(scopeInclude.model, scopeInclude.as);
+            const association = scopeInclude.association || this.getAssociationForAlias(scopeInclude.model, scopeInclude.as);
             return association ? item.as === association.as : false;
           }
         })) {

test/integration/model/find.test.js

@@ -551,10 +551,11 @@ describe(Support.getTestDialectTeaser('Model'), function() {
             });
           });
 
-          it('throws an error if alias is not associated', function() {
+          it(`throws an error indicating an incorrect alias was entered if an association 
+              and alias exist but the alias doesn't match`, function() {
             var self = this;
             return self.Worker.findOne({ include: [{ model: self.Task, as: 'Work' }] }).catch (function(err) {
-              expect(err.message).to.equal('Task (Work) is not associated to Worker!');
+              expect(err.message).to.equal(`Task is associated to Worker using an alias. You've included an alias (Work), but it does not match the alias defined in your association.`);
             });
           });
 
@@ -712,10 +713,11 @@ describe(Support.getTestDialectTeaser('Model'), function() {
             });
           });
 
-          it('throws an error if alias is not associated', function() {
+          it(`throws an error indicating an incorrect alias was entered if an association 
+              and alias exist but the alias doesn't match`, function() {
             var self = this;
             return self.Worker.findOne({ include: [{ model: self.Task, as: 'Work' }] }).catch (function(err) {
-              expect(err.message).to.equal('Task (Work) is not associated to Worker!');
+              expect(err.message).to.equal(`Task is associated to Worker using an alias. You've included an alias (Work), but it does not match the alias defined in your association.`);
             });
           });
 

test/integration/model/findAll.test.js

@@ -601,14 +601,16 @@ describe(Support.getTestDialectTeaser('Model'), function() {
         it('throws an error if included DaoFactory is not referenced by alias', function() {
           var self = this;
           return self.Worker.findAll({ include: [self.Task] }).catch (function(err) {
-            expect(err.message).to.equal('Task is not associated to Worker!');
+            expect(err.message).to.equal('Task is associated to Worker using an alias. ' +
+            'You must use the \'as\' keyword to specify the alias within your include statement.');
           });
         });
 
         it('throws an error if alias is not associated', function() {
           var self = this;
           return self.Worker.findAll({ include: [{ model: self.Task, as: 'Work' }] }).catch (function(err) {
-            expect(err.message).to.equal('Task (Work) is not associated to Worker!');
+            expect(err.message).to.equal('Task is associated to Worker using an alias. ' +
+            'You\'ve included an alias (Work), but it does not match the alias defined in your association.');
           });
         });
 
@@ -693,14 +695,16 @@ describe(Support.getTestDialectTeaser('Model'), function() {
         it('throws an error if included DaoFactory is not referenced by alias', function() {
           var self = this;
           return self.Worker.findAll({ include: [self.Task] }).catch (function(err) {
-            expect(err.message).to.equal('Task is not associated to Worker!');
+            expect(err.message).to.equal('Task is associated to Worker using an alias. ' +
+            'You must use the \'as\' keyword to specify the alias within your include statement.');
           });
         });
 
         it('throws an error if alias is not associated', function() {
           var self = this;
           return self.Worker.findAll({ include: [{ model: self.Task, as: 'Work' }] }).catch (function(err) {
-            expect(err.message).to.equal('Task (Work) is not associated to Worker!');
+            expect(err.message).to.equal('Task is associated to Worker using an alias. ' +
+            'You\'ve included an alias (Work), but it does not match the alias defined in your association.');
           });
         });
 

test/unit/model/include.test.js

@@ -285,6 +285,23 @@ describe(Support.getTestDialectTeaser('Model'), function() {
       });
     });
 
+    describe('_getIncludedAssociation', function () {
+      it('returns an association when there is a single unaliased association', function () {
+        expect(this.User._getIncludedAssociation(this.Task)).to.equal(this.User.Tasks);
+      });
+
+      it('returns an association when there is a single aliased association', function () {
+        const User = this.sequelize.define('User');
+        const Task = this.sequelize.define('Task');
+        const Tasks = Task.belongsTo(User, {as: 'owner'});
+        expect(Task._getIncludedAssociation(User, 'owner')).to.equal(Tasks);
+      });
+
+      it('returns an association when there are multiple aliased associations', function () {
+        expect(this.Company._getIncludedAssociation(this.User, 'Owner')).to.equal(this.Company.Owner);
+      });
+    });
+
     describe('subQuery', function () {
       it('should be true if theres a duplicating association', function () {
         var options = Sequelize.Model._validateIncludedElements({