Throw an error if the name of an association clashes with an attribute. Closes #1272

changelog.md

@@ -4,13 +4,14 @@ Notice: All 1.7.x changes are present in 2.0.x aswell
 - [FEATURE] Hooks can now return promises
 - [FEATURE] There is now basic support for assigning a field name to an attribute `name: {type: DataTypes.STRING, field: 'full_name'}`
 - [FEATURE] It's now possible to add multiple relations to a hasMany association, modelInstance.addRelations([otherInstanceA, otherInstanceB])
+- [FEATURE] `define()` stores models in `sequelize.models` Object e.g. `sequelize.models.MyModel`
+- [BUG] An error is now thrown if an association would create a naming conflict between the association and the foreign key when doing eager loading. Closes [#1272](https://github.com/sequelize/sequelize/issues/1272)
 
 #### Breaking changes
 - Sequelize now returns promises instead of its custom event emitter from most calls. This affects methods that return multiple values (like `findOrCreate` or `findOrInitialize`). If your current callbacks do not accept the 2nd success parameter you might be seeing an array as the first param. Either use `.spread()` for these methods or add another argument to your callback: `.success(instance)` -> `.success(instance, created)`.
 - `.success()`/`.done()` and any other non promise methods are now deprecated (we will keep the codebase around for a few versions though). on('sql') persists for debugging purposes.
 - Model association calls (belongsTo/hasOne/hasMany) are no longer chainable. (this is to support being able to pass association references to include rather than model/as combinations)
 - `QueryInterface` no longer emits global events. This means you can no longer do things like `QueryInterface.on('showAllSchemas', function ... `
-- `define()` stores models in `sequelize.models` Object e.g. `sequelize.models.MyModel`
 
 # v2.0.0-dev11
 ### Caution: This release contains many changes and is highly experimental

lib/associations/belongs-to.js

@@ -55,6 +55,10 @@ module.exports = (function() {
     // Sync attributes and setters/getters to DAO prototype
     this.source.refreshAttributes()
 
+    if (this.source.rawAttributes.hasOwnProperty(this.as)) {
+      throw new Error("Naming collision between attribute '" + this.as + "' and association '" + this.as + "' on model " + this.source.name + ". To remedy this, change either foreignKey or as in your association definition")
+    }
+
     return this
   }
 

lib/associations/has-many.js

@@ -147,7 +147,6 @@ module.exports = (function() {
 
     // is there already a single sided association between the source and the target?
     // or is the association on the model itself?
-
     if ((this.isSelfAssociation && Object(this.through) === this.through) || doubleLinked) {
       // We need to remove the keys that 1:M have added
       if (this.isSelfAssociation && doubleLinked) {
@@ -249,6 +248,10 @@ module.exports = (function() {
     this.target.refreshAttributes()
     this.source.refreshAttributes()
 
+    if (this.source.rawAttributes.hasOwnProperty(this.as)) {
+      throw new Error("Naming collision between attribute '" + this.as + "' and association '" + this.as + "' on model " + this.source.name + ". To remedy this, change either foreignKey or as in your association definition")
+    }
+
     return this
   }
 

lib/associations/has-one.js

@@ -56,6 +56,11 @@ module.exports = (function() {
 
     // Sync attributes and setters/getters to DAO prototype
     this.target.refreshAttributes()
+
+    if (this.source.rawAttributes.hasOwnProperty(this.as)) {
+      throw new Error("Naming collision between attribute '" + this.as + "' and association '" + this.as + "' on model " + this.source.name + ". To remedy this, change either foreignKey or as in your association definition")
+    }
+
     return this
   }
 

test/associations/belongs-to.test.js

@@ -486,5 +486,23 @@ describe(Support.getTestDialectTeaser("BelongsTo"), function() {
         })
       })
     })
+
+    it('should throw an error if foreignKey and as result in a name clash', function () {
+      var Person = this.sequelize.define('person', {})
+        , Car = this.sequelize.define('car', {})
+
+      expect(Car.belongsTo.bind(Car, Person, {foreignKey: 'person'})).to
+        .throw("Naming collision between attribute 'person' and association 'person' on model car. To remedy this, change either foreignKey or as in your association definition")
+    })
+
+    it('should throw an error if an association clashes with the name of an already define attribute', function () {
+       var Person = this.sequelize.define('person', {})
+        , Car = this.sequelize.define('car', {
+            person: Sequelize.INTEGER
+          })
+
+        expect(Car.belongsTo.bind(Car, Person, {as: 'person'})).to
+        .throw("Naming collision between attribute 'person' and association 'person' on model car. To remedy this, change either foreignKey or as in your association definition")
+    })
   })
 })

test/associations/has-many.test.js

@@ -1969,5 +1969,14 @@ describe(Support.getTestDialectTeaser("HasMany"), function() {
         done()
       })
     })
+
+    it('should throw an error if foreignKey and as result in a name clash', function () {
+      var User = this.sequelize.define('user', {
+            user: Sequelize.INTEGER
+          })
+
+      expect(User.hasMany.bind(User, User, { as: 'user' })).to
+        .throw("Naming collision between attribute 'user' and association 'user' on model user. To remedy this, change either foreignKey or as in your association definition")
+    })
   })
 })

test/associations/has-one.test.js

@@ -451,6 +451,16 @@ describe(Support.getTestDialectTeaser("HasOne"), function() {
         })
       })
     })
+
+    it('should throw an error if an association clashes with the name of an already define attribute', function () {
+       var User = this.sequelize.define('user', {
+            attribute: Sequelize.STRING
+           })
+        , Attribute = this.sequelize.define('attribute', {})
+
+        expect(User.hasOne.bind(User, Attribute)).to
+        .throw("Naming collision between attribute 'attribute' and association 'attribute' on model user. To remedy this, change either foreignKey or as in your association definition")
+    })
   })
 
   describe("Counter part", function () {

test/include.test.js

@@ -36,35 +36,6 @@ describe(Support.getTestDialectTeaser("Include"), function () {
       })
     })
 
-    // We don't support naming associations the same as the foreign key, however the system should not crash because of it, the results hould just be wrong as is expected behaviour currently.
-    it('should not throw an error when an empty include is named the same as the foreign key', function (done) {
-      var section = this.sequelize.define('section', { name: DataTypes.STRING });
-      var layout = this.sequelize.define('layout', { name: DataTypes.STRING });
-
-      section.belongsTo(layout, {
-        as: layout.name,
-        foreignKey: layout.name,
-        foreignKeyConstraint: true
-      });
-       
-      this.sequelize.sync({force: true}).done(function(err) {
-        expect(err).to.be.null
-
-        section.create({ name: 'test1' }).success(function() {
-          section.find({
-            where: { name: 'test1' },
-            include: [
-              { model: layout, as: 'layout'}
-            ]
-          }).done(function(err, user) {
-            expect(err).to.be.null
-            expect(user).to.be.ok
-            done()
-          });
-        });
-      });
-    })
-
     it('should support a empty hasOne include', function (done) {
       var Company = this.sequelize.define('Company', {})
         , Person = this.sequelize.define('Person', {})