Task 5267, Make all association to prefer as their foreign key name (sequelize#5957)

* sync behaviour of as for all association type

* changelog and docs for sequelize#5267

* fixed N:M failing test

Author: sushantdhiman

changelog.md

@@ -17,6 +17,7 @@
 - [CHANGED] `instance.equals` now only checks primary keys, instead of all attributes.
 - [REWRITE] Rewrite model and instance to a single class - instance instanceof Model [#5924](https://github.com/sequelize/sequelize/issues/5924)
 - [REMOVED] Counter cache plugin
+- [FIXED] All associations now prefer aliases to construct foreign key [#5267](https://github.com/sequelize/sequelize/issues/5267)
 
 ## BC breaks:
 - `hookValidate` removed in favor of `validate` with `hooks: true | false`. `validate` returns a promise which is rejected if validation fails
@@ -28,10 +29,10 @@
 - Removed support for `pool:false`, if you still want to use single connection set `pool.max` to `1`
 - Removed default `REPEATABLE_READ` transaction isolation, use config option to explicitly set it
 - Removed MariaDB dialect - this was just a thin wrapper around MySQL, so using `dialect: 'mysql'` instead should work with no further changes
-- `hasOne` now prefer `as` option to generate foreign key name, otherwise it defaults to source model name
 - `instance.equals` now provides reference equality (do two instances refer to the same row, i.e. are their primary key(s) equal). Use `instance.get()` to get and compare all values.
 - Instances (database rows) are now instances of the model, instead of being a separate class. This means you can replace User.build() with new User() and sequelize.define with User extends Sequelize.Model. See #5924
 - The counter cache plugin, and consequently the `counterCache` option for associations has been removed. The plugin is seeking a new maintainer - You can find the code [here](https://github.com/sequelize/sequelize/blob/aace1250dfa8cd81a4edfd2086c9058b513f6ee0/lib/plugins/counter-cache.js)
+- All associations type will prefer `as` when constructing the `foreignKey` name. You can override this by `foreignKey` option.
 
 # 3.23.2
 - [FIXED] Type validation now works with non-strings due to updated validator@5.0.0 [#5861](https://github.com/sequelize/sequelize/pull/5861)

docs/docs/associations.md

@@ -189,7 +189,7 @@ var Project = sequelize.define('project', {/* ... */})
 Project.hasMany(User, {as: 'Workers'})
 ```
 
-This will add the attribute `projectId` or `project_id` to User. Instances of Project will get the accessors getWorkers and setWorkers.  We could just leave it the way it is and let it be a one-way association.
+This will add the attribute `WorkersId` or `Workers_id` to User. Instances of Project will get the accessors getWorkers and setWorkers.  We could just leave it the way it is and let it be a one-way association.
 But we want more! Let's define it the other way around by creating a many to many association in the next section:
 
 ## Belongs-To-Many associations

lib/associations/belongs-to-many.js

@@ -112,7 +112,7 @@ var BelongsToMany = function(source, target, options) {
     this.foreignKeyAttribute = {};
     this.foreignKey = this.options.foreignKey || Utils.camelizeIf(
       [
-        Utils.underscoredIf(this.source.options.name.singular, this.source.options.underscored),
+        Utils.underscoredIf(this.options.as || this.source.options.name.singular, this.source.options.underscored),
         this.source.primaryKeyAttribute
       ].join('_'),
       !this.source.options.underscored

lib/associations/belongs-to.js

@@ -47,7 +47,7 @@ var BelongsTo = function(source, target, options) {
   if (!this.foreignKey) {
     this.foreignKey = Utils.camelizeIf(
       [
-        Utils.underscoredIf(this.as, this.source.options.underscored),
+        Utils.underscoredIf(this.options.as || this.as, this.source.options.underscored),
         this.target.primaryKeyAttribute
       ].join('_'),
       !this.source.options.underscored

lib/associations/has-many.js

@@ -71,7 +71,7 @@ var HasMany = function(source, target, options) {
   if (!this.foreignKey) {
     this.foreignKey = Utils.camelizeIf(
       [
-        Utils.underscoredIf(this.source.options.name.singular, this.source.options.underscored),
+        Utils.underscoredIf(this.options.as || this.source.options.name.singular, this.source.options.underscored),
         this.source.primaryKeyAttribute
       ].join('_'),
       !this.source.options.underscored

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

@@ -470,7 +470,14 @@ describe(Support.getTestDialectTeaser('BelongsToMany'), function() {
         }
       });
 
-      return expect(this.user.countActiveTasks({})).to.eventually.equal(1);
+      var self = this;
+      return this.UserTask.sync({ force: true })
+      .then(function() {
+        return self.user.setActiveTasks(self.tasks);
+      })
+      .then(function() {
+        return expect(self.user.countActiveTasks({})).to.eventually.equal(1);
+      });
     });
 
     it('should count scoped through associations', function () {
@@ -486,18 +493,23 @@ describe(Support.getTestDialectTeaser('BelongsToMany'), function() {
         }
       });
 
-      return Promise.join(
-        this.Task.create().then(function (task) {
-          return user.addTask(task, {
-            started: true
-          });
-        }),
-        this.Task.create().then(function (task) {
-          return user.addTask(task, {
-            started: true
-          });
-        })
-      ).then(function () {
+      return this.UserTask.sync({ force: true})
+      .bind(this)
+      .then(function() {
+        return Promise.join(
+          this.Task.create().then(function (task) {
+            return user.addStartedTask(task, {
+              started: true
+            });
+          }),
+          this.Task.create().then(function (task) {
+            return user.addStartedTask(task, {
+              started: true
+            });
+          })
+        );
+      })
+      .then(function () {
         return expect(user.countStartedTasks({})).to.eventually.equal(2);
       });
     });
@@ -1825,8 +1837,8 @@ describe(Support.getTestDialectTeaser('BelongsToMany'), function() {
       Group.belongsToMany(User, { as: 'MyUsers', through: 'group_user'});
 
       expect(Group.associations.MyUsers.through.model === User.associations.MyGroups.through.model);
-      expect(Group.associations.MyUsers.through.model.rawAttributes.UserId).to.exist;
-      expect(Group.associations.MyUsers.through.model.rawAttributes.GroupId).to.exist;
+      expect(Group.associations.MyUsers.through.model.rawAttributes.MyGroupsId).to.exist;
+      expect(Group.associations.MyUsers.through.model.rawAttributes.MyUsersId).to.exist;
     });
 
     it('correctly identifies its counterpart when through is a model', function() {
@@ -1839,8 +1851,8 @@ describe(Support.getTestDialectTeaser('BelongsToMany'), function() {
 
       expect(Group.associations.MyUsers.through.model === User.associations.MyGroups.through.model);
 
-      expect(Group.associations.MyUsers.through.model.rawAttributes.UserId).to.exist;
-      expect(Group.associations.MyUsers.through.model.rawAttributes.GroupId).to.exist;
+      expect(Group.associations.MyUsers.through.model.rawAttributes.MyGroupsId).to.exist;
+      expect(Group.associations.MyUsers.through.model.rawAttributes.MyUsersId).to.exist;
     });
   });
 
@@ -2146,7 +2158,7 @@ describe(Support.getTestDialectTeaser('BelongsToMany'), function() {
 
       Children = Person.belongsToMany(Person, { as: 'Children', through: PersonChildren});
 
-      expect(Children.foreignKey).to.equal('PersonId');
+      expect(Children.foreignKey).to.equal('ChildrenId');
       expect(Children.otherKey).to.equal('ChildId');
       expect(PersonChildren.rawAttributes[Children.foreignKey]).to.be.ok;
       expect(PersonChildren.rawAttributes[Children.otherKey]).to.be.ok;
@@ -2156,7 +2168,7 @@ describe(Support.getTestDialectTeaser('BelongsToMany'), function() {
       PersonChildren = this.sequelize.define('PersonChildren', {}, {underscored: true});
       Children = Person.belongsToMany(Person, { as: 'Children', through: PersonChildren});
 
-      expect(Children.foreignKey).to.equal('person_id');
+      expect(Children.foreignKey).to.equal('children_id');
       expect(Children.otherKey).to.equal('child_id');
       expect(PersonChildren.rawAttributes[Children.foreignKey]).to.be.ok;
       expect(PersonChildren.rawAttributes[Children.otherKey]).to.be.ok;

test/integration/associations/scope.test.js

@@ -261,8 +261,8 @@ describe(Support.getTestDialectTeaser('associations'), function() {
             this.PostTag = this.sequelize.define('post_tag');
 
             this.Tag.belongsToMany(this.Post, {through: this.PostTag});
-            this.Post.belongsToMany(this.Tag, {as: 'categories', through: this.PostTag, scope: { type: 'category' }});
-            this.Post.belongsToMany(this.Tag, {as: 'tags', through: this.PostTag, scope: { type: 'tag' }});
+            this.Post.belongsToMany(this.Tag, {as: 'categories', through: this.PostTag, foreignKey: 'PostId', scope: { type: 'category' }});
+            this.Post.belongsToMany(this.Tag, {as: 'tags', through: this.PostTag, foreignKey: 'PostId', scope: { type: 'tag' }});
           });
 
           it('should create, find and include associations with scope values', function() {

test/integration/include/findAll.test.js

@@ -405,8 +405,8 @@ describe(Support.getTestDialectTeaser('Include'), function() {
             ]).spread(function(user, products) {
               return Promise.all([
                 GroupMember.bulkCreate([
-                  {UserId: user.id, GroupId: groups[0].id, RankId: ranks[0].id},
-                  {UserId: user.id, GroupId: groups[1].id, RankId: ranks[1].id}
+                  {MembershipsId: user.id, GroupId: groups[0].id, RankId: ranks[0].id},
+                  {MembershipsId: user.id, GroupId: groups[1].id, RankId: ranks[1].id}
                 ]),
                 user.setProducts([
                   products[(i * 2) + 0],

test/integration/include/schema.test.js

@@ -276,8 +276,8 @@ describe(Support.getTestDialectTeaser('Includes with schemas'), function() {
                 ]).spread(function(user, products) {
                   return Promise.all([
                     GroupMember.bulkCreate([
-                      {AccUserId: user.id, GroupId: groups[0].id, RankId: ranks[0].id},
-                      {AccUserId: user.id, GroupId: groups[1].id, RankId: ranks[1].id}
+                      {MembershipsId: user.id, GroupId: groups[0].id, RankId: ranks[0].id},
+                      {MembershipsId: user.id, GroupId: groups[1].id, RankId: ranks[1].id}
                     ]),
                     user.setProducts([
                       products[(i * 2) + 0],

test/integration/model/scope/associations.test.js

@@ -96,7 +96,7 @@ describe(Support.getTestDialectTeaser('Model'), function() {
         this.ScopeMe.hasOne(this.Profile, { foreignKey: 'userId' });
 
         this.ScopeMe.belongsTo(this.Company);
-        this.UserAssociation = this.Company.hasMany(this.ScopeMe, { as: 'users' });
+        this.UserAssociation = this.Company.hasMany(this.ScopeMe, { as: 'users', foreignKey: 'companyId'});
 
         return this.sequelize.sync({force: true}).bind(this).then(function() {
           return Promise.all([

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

@@ -512,10 +512,10 @@ describe(Support.getTestDialectTeaser('belongsToMany'), function() {
       Group.belongsToMany(User, { as: 'MyUsers', through: 'group_user', onUpdate: 'SET NULL', onDelete: 'RESTRICT' });
 
       expect(Group.associations.MyUsers.through.model === User.associations.MyGroups.through.model);
-      expect(Group.associations.MyUsers.through.model.rawAttributes.UserId.onUpdate).to.equal('RESTRICT');
-      expect(Group.associations.MyUsers.through.model.rawAttributes.UserId.onDelete).to.equal('SET NULL');
-      expect(Group.associations.MyUsers.through.model.rawAttributes.GroupId.onUpdate).to.equal('SET NULL');
-      expect(Group.associations.MyUsers.through.model.rawAttributes.GroupId.onDelete).to.equal('RESTRICT');
+      expect(Group.associations.MyUsers.through.model.rawAttributes.MyGroupsId.onUpdate).to.equal('RESTRICT');
+      expect(Group.associations.MyUsers.through.model.rawAttributes.MyGroupsId.onDelete).to.equal('SET NULL');
+      expect(Group.associations.MyUsers.through.model.rawAttributes.MyUsersId.onUpdate).to.equal('SET NULL');
+      expect(Group.associations.MyUsers.through.model.rawAttributes.MyUsersId.onDelete).to.equal('RESTRICT');
     });
 
     it('work properly when through is a model', function() {
@@ -527,10 +527,25 @@ describe(Support.getTestDialectTeaser('belongsToMany'), function() {
       Group.belongsToMany(User, { as: 'MyUsers', through: UserGroup, onUpdate: 'SET NULL', onDelete: 'RESTRICT' });
 
       expect(Group.associations.MyUsers.through.model === User.associations.MyGroups.through.model);
-      expect(Group.associations.MyUsers.through.model.rawAttributes.UserId.onUpdate).to.equal('RESTRICT');
-      expect(Group.associations.MyUsers.through.model.rawAttributes.UserId.onDelete).to.equal('SET NULL');
-      expect(Group.associations.MyUsers.through.model.rawAttributes.GroupId.onUpdate).to.equal('SET NULL');
-      expect(Group.associations.MyUsers.through.model.rawAttributes.GroupId.onDelete).to.equal('RESTRICT');
+      expect(Group.associations.MyUsers.through.model.rawAttributes.MyGroupsId.onUpdate).to.equal('RESTRICT');
+      expect(Group.associations.MyUsers.through.model.rawAttributes.MyGroupsId.onDelete).to.equal('SET NULL');
+      expect(Group.associations.MyUsers.through.model.rawAttributes.MyUsersId.onUpdate).to.equal('SET NULL');
+      expect(Group.associations.MyUsers.through.model.rawAttributes.MyUsersId.onDelete).to.equal('RESTRICT');
     });
   });
+
+  it('properly use the `as` key to generate foreign key name', function(){
+    var City = current.define('City', { cityname: DataTypes.STRING })
+      , State = current.define('State', { statename: DataTypes.STRING })
+      , CityStateMap = current.define('CityStateMap', { rating: DataTypes.STRING });
+
+    State.belongsToMany(City, { through: CityStateMap });
+    expect(CityStateMap.attributes.StateId).not.to.be.empty;
+    expect(CityStateMap.attributes.CityId).not.to.be.empty;
+
+    State.belongsToMany(City, { through: CityStateMap, as: 'Test'});
+    expect(CityStateMap.attributes.TestId).not.to.be.empty;
+    expect(CityStateMap.attributes.CityId).not.to.be.empty;
+
+  });
 });

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

@@ -0,0 +1,21 @@
+'use strict';
+
+/* jshint -W030 */
+var chai = require('chai')
+  , expect = chai.expect
+  , Support   = require(__dirname + '/../support')
+  , DataTypes = require(__dirname + '/../../../lib/data-types')
+  , current   = Support.sequelize;
+
+describe(Support.getTestDialectTeaser('hasOne'), function() {
+  it('properly use the `as` key to generate foreign key name', function(){
+    var User = current.define('User', { username: DataTypes.STRING })
+      , Task = current.define('Task', { title: DataTypes.STRING });
+
+    User.belongsTo(Task);
+    expect(User.attributes.TaskId).not.to.be.empty;
+
+    User.belongsTo(Task, {as : 'Naam'});
+    expect(User.attributes.NaamId).not.to.be.empty;
+  });
+});

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

@@ -162,4 +162,15 @@ describe(Support.getTestDialectTeaser('hasMany'), function() {
       });
     });
   });
+
+  it('properly use the `as` key to generate foreign key name', function(){
+    var City = current.define('City', { cityname: DataTypes.STRING })
+      , State = current.define('State', { statename: DataTypes.STRING });
+
+    State.hasMany(City);
+    expect(City.attributes.StateId).not.to.be.empty;
+
+    State.hasMany(City, {as : 'Capital'});
+    expect(City.attributes.CapitalId).not.to.be.empty;
+  });
 });