Merge pull request #2222 from janmeier/master

Fix object syntax for hasmany and belongsto foreign keys

Merge pull request #2222 from janmeier/master
Fix object syntax for hasmany and belongsto foreign keys
Mick Hansen
Commit f2723c7f authored Sep 05, 2014 by Mick Hansen
Showing with 194 additions and 42 deletions
changelog.md
lib/associations/belongs-to.js
lib/associations/has-many.js
lib/associations/has-one.js
lib/associations/mixin.js
lib/utils.js
test/associations/belongs-to.test.js
test/associations/has-many.test.js
test/associations/has-one.test.js
test/dao-factory/findAll.test.js
test/include/findAll.test.js
test/include/findAndCountAll.test.js
--- a/changelog.md
+++ b/changelog.md
+# Next
+- [BUG] Fixed an issue with foreign key object syntax for hasOne and belongsTo
+- [FEATURE] Added `field` and `name` to the object form of foreign key definitions
+#### Backwards compatability changes
+- The `fieldName` property, used in associations with a foreign key object `(A.hasMany(B, { foreignKey: { ... }})`, has been renamed to `name` to avoid confusion with `field`. 
 # v2.0.0-dev13
 We are working our way to the first 2.0.0 release candidate.

--- a/lib/associations/belongs-to.js
+++ b/lib/associations/belongs-to.js
@@ -16,7 +16,7 @@ module.exports = (function() {
    if (Utils._.isObject(this.options.foreignKey)) {
      this.foreignKeyAttribute = this.options.foreignKey;
-      this.foreignKey = this.foreignKeyAttribute.fieldName;
+      this.foreignKey = this.foreignKeyAttribute.name || this.foreignKeyAttribute.fieldName;
    } else {
      this.foreignKeyAttribute = {};
      this.foreignKey = this.options.foreignKey;
@@ -42,7 +42,14 @@ module.exports = (function() {
      );
    }
-    this.identifier = this.options.foreignKey;
+    this.identifier = this.foreignKey || Utils._.camelizeIf(
+      [
+        Utils._.underscoredIf(this.options.name.singular, this.target.options.underscored),
+        this.target.primaryKeyAttribute
+      ].join('_'),
+      !this.target.options.underscored
+    );
    this.targetIdentifier = this.target.primaryKeyAttribute;
    this.associationAccessor = this.as;
    this.options.useHooks = options.useHooks;
@@ -67,7 +74,7 @@ module.exports = (function() {
      this.options.onUpdate = this.options.onUpdate || 'CASCADE';
    }
    Helpers.addForeignKeyConstraints(newAttributes[this.identifier], this.target, this.source, this.options);
-    Utils._.defaults(this.source.rawAttributes, newAttributes);
+    Utils.mergeDefaults(this.source.rawAttributes, newAttributes);
    // Sync attributes and setters/getters to DAO prototype
    this.source.refreshAttributes();

--- a/lib/associations/has-many.js
+++ b/lib/associations/has-many.js
@@ -30,7 +30,7 @@ module.exports = (function() {
    if (Utils._.isObject(this.options.foreignKey)) {
      this.foreignKeyAttribute = this.options.foreignKey;
-      this.foreignKey = this.foreignKeyAttribute.fieldName;
+      this.foreignKey = this.foreignKeyAttribute.name || this.foreignKeyAttribute.fieldName;
    } else {
      this.foreignKeyAttribute = {};
      this.foreignKey = this.options.foreignKey;
@@ -273,7 +273,7 @@ module.exports = (function() {
        constraintOptions.onUpdate = constraintOptions.onUpdate || 'CASCADE';
      }
      Helpers.addForeignKeyConstraints(newAttributes[this.identifier], this.source, this.target, constraintOptions);
-      Utils._.defaults(this.target.rawAttributes, newAttributes);
+      Utils.mergeDefaults(this.target.rawAttributes, newAttributes);
    }
    // Sync attributes and setters/getters to DAO prototype

--- a/lib/associations/has-one.js
+++ b/lib/associations/has-one.js
@@ -16,7 +16,7 @@ module.exports = (function() {
    if (Utils._.isObject(this.options.foreignKey)) {
      this.foreignKeyAttribute = this.options.foreignKey;
-      this.foreignKey = this.foreignKeyAttribute.fieldName;
+      this.foreignKey = this.foreignKeyAttribute.name || this.foreignKeyAttribute.fieldName;
    } else {
      this.foreignKeyAttribute = {};
      this.foreignKey = this.options.foreignKey;
@@ -42,7 +42,14 @@ module.exports = (function() {
      );
    }
-    this.identifier = this.options.foreignKey;
+    this.identifier = this.foreignKey || Utils._.camelizeIf(
+      [
+        Utils._.underscoredIf(this.source.options.name.singular, this.source.options.underscored),
+        this.source.primaryKeyAttribute
+      ].join('_'),
+      !this.source.options.underscored
+    );
    this.sourceIdentifier = this.source.primaryKeyAttribute;
    this.associationAccessor = this.as;
    this.options.useHooks = options.useHooks;
@@ -63,7 +70,7 @@ module.exports = (function() {
      , keyType = this.source.rawAttributes[this.sourceIdentifier].type;
    newAttributes[this.identifier] = Utils._.defaults(this.foreignKeyAttribute, { type: this.options.keyType || keyType });
-    Utils._.defaults(this.target.rawAttributes, newAttributes);
+    Utils.mergeDefaults(this.target.rawAttributes, newAttributes);
    if (this.options.constraints !== false) {
      this.options.onDelete = this.options.onDelete || 'SET NULL';

--- a/lib/associations/mixin.js
+++ b/lib/associations/mixin.js
@@ -48,7 +48,7 @@ var Utils = require('./../utils')
 * ```js
 * User.hasMany(Picture, {
 *   foreignKey: {
- *     fieldName: 'uid'
+ *     name: 'uid'
 *     allowNull: false
 *   }
 * })
@@ -135,7 +135,7 @@ var singleLinked = function (Type) {
 * @param {object}          [options]
 * @param {boolean}         [options.hooks=false] Set to true to run before-/afterDestroy hooks when an associated model is deleted because of a cascade. For example if `User.hasOne(Profile, {onDelete: 'cascade', hooks:true})`, the before-/afterDestroy hooks for profile will be called when a user is deleted. Otherwise the profile will be deleted without invoking any hooks
 * @param {string}          [options.as] The alias of this model, in singular form. See also the `name` option passed to `sequelize.define`. If you create multiple associations between the same tables, you should provide an alias to be able to distinguish between them. If you provide an alias when creating the assocition, you should provide the same alias when eager loading and when getting assocated models. Defaults to the singularized name of target
- * @param {string|object}   [options.foreignKey] The name of the foreign key in the target table or an object representing the type definition for the foreign column (see `Sequelize.define` for syntax). When using an object, you can add a `fieldName` property to set the name of the colum. Defaults to the name of source + primary key of source
+ * @param {string|object}   [options.foreignKey] The name of the foreign key in the target table or an object representing the type definition for the foreign column (see `Sequelize.define` for syntax). When using an object, you can add a `name` property to set the name of the colum. Defaults to the name of source + primary key of source
 * @param {string}          [options.onDelete='SET&nbsp;NULL']
 * @param {string}          [options.onUpdate='CASCADE']
 * @param {boolean}         [options.constraints=true] Should on update and on delete constraints be enabled on the foreign key.
@@ -159,7 +159,7 @@ Mixin.hasOne = singleLinked(HasOne);
 * @param {object}          [options]
 * @param {boolean}         [options.hooks=false] Set to true to run before-/afterDestroy hooks when an associated model is deleted because of a cascade. For example if `User.hasOne(Profile, {onDelete: 'cascade', hooks:true})`, the before-/afterDestroy hooks for profile will be called when a user is deleted. Otherwise the profile will be deleted without invoking any hooks
 * @param {string}          [options.as] The alias of this model, in singular form. See also the `name` option passed to `sequelize.define`. If you create multiple associations between the same tables, you should provide an alias to be able to distinguish between them. If you provide an alias when creating the assocition, you should provide the same alias when eager loading and when getting assocated models. Defaults to the singularized name of target
- * @param {string|object}   [options.foreignKey] The name of the foreign key in the source table or an object representing the type definition for the foreign column (see `Sequelize.define` for syntax). When using an object, you can add a `fieldName` property to set the name of the colum. Defaults to the name of target + primary key of target
+ * @param {string|object}   [options.foreignKey] The name of the foreign key in the source table or an object representing the type definition for the foreign column (see `Sequelize.define` for syntax). When using an object, you can add a `name` property to set the name of the colum. Defaults to the name of target + primary key of target
 * @param {string}          [options.onDelete='SET&nbsp;NULL']
 * @param {string}          [options.onUpdate='CASCADE']
 * @param {boolean}         [options.constraints=true] Should on update and on delete constraints be enabled on the foreign key.
@@ -229,7 +229,7 @@ Mixin.belongsTo = singleLinked(BelongsTo);
 * @param {boolean}           [options.hooks=false] Set to true to run before-/afterDestroy hooks when an associated model is deleted because of a cascade. For example if `User.hasOne(Profile, {onDelete: 'cascade', hooks:true})`, the before-/afterDestroy hooks for profile will be called when a user is deleted. Otherwise the profile will be deleted without invoking any hooks
 * @param {Model|string}      [options.through] The name of the table that is used to join source and target in n:m associations. Can also be a sequelize model if you want to define the junction table yourself and add extra attributes to it.
 * @param {string|object}     [options.as] The alias of this model. If you provide a string, it should be plural, and will be singularized using node.inflection. If you want to control the singular version yourself, provide an object with `plural` and `singular` keys. See also the `name` option passed to `sequelize.define`. If you create multiple associations between the same tables, you should provide an alias to be able to distinguish between them. If you provide an alias when creating the assocition, you should provide the same alias when eager loading and when getting assocated models. Defaults to the pluralized name of target
- * @param {string|object}     [options.foreignKey] The name of the foreign key in the target table / join table or an object representing the type definition for the foreign column (see `Sequelize.define` for syntax). When using an object, you can add a `fieldName` property to set the name of the colum. Defaults to the name of source + primary key of source
+ * @param {string|object}     [options.foreignKey] The name of the foreign key in the target table / join table or an object representing the type definition for the foreign column (see `Sequelize.define` for syntax). When using an object, you can add a `name` property to set the name of the colum. Defaults to the name of source + primary key of source
 * @param {string}            [options.onDelete='SET&nbsp;NULL|CASCADE'] Cascade if this is a n:m, and set null if it is a 1:m
 * @param {string}            [options.onUpdate='CASCADE']
 * @param {boolean}           [options.constraints=true] Should on update and on delete constraints be enabled on the foreign key.

--- a/lib/utils.js
+++ b/lib/utils.js
@@ -57,6 +57,15 @@ var Utils = module.exports = {
    return _;
  })(),
+  // Same concept as _.merge, but don't overwrite properties that have already been assigned
+  mergeDefaults: function (a, b) {
+    return this._.merge(a, b, function (objectValue, sourceValue) {
+      // If it's an object, let _ handle it this time, we will be called again for each property
+      if (!this._.isPlainObject(objectValue) && objectValue !== undefined) {
+        return objectValue;
+      }
+    }, this);
+  },
  lowercaseFirst: function (s) {
    return s[0].toLowerCase() + s.slice(1);
  },

--- a/test/associations/belongs-to.test.js
+++ b/test/associations/belongs-to.test.js
@@ -510,7 +510,26 @@ describe(Support.getTestDialectTeaser("BelongsTo"), function() {
      })
    })
-    it('allows the user to provide an attribute definition as foreignKey', function () {
+    describe('allows the user to provide an attribute definition object as foreignKey', function () {
+      it('works with a column that hasnt been defined before', function () {
+        var Task = this.sequelize.define('task', {})
+        , User = this.sequelize.define('user', {
+          });
+        Task.belongsTo(User, {
+          foreignKey: {
+            allowNull: false,
+            name: 'uid'
+          }
+        });
+        expect(Task.rawAttributes.uid).to.be.defined
+        expect(Task.rawAttributes.uid.allowNull).to.be.false;
+        expect(Task.rawAttributes.uid.references).to.equal(User.getTableName())
+        expect(Task.rawAttributes.uid.referencesKey).to.equal('id')
+      });
+      it('works when taking a column directly from the object', function () {
        var User = this.sequelize.define('user', {
            uid: {
              type: Sequelize.INTEGER,
@@ -530,7 +549,24 @@ describe(Support.getTestDialectTeaser("BelongsTo"), function() {
        expect(Profile.rawAttributes.user_id.references).to.equal(User.getTableName())
        expect(Profile.rawAttributes.user_id.referencesKey).to.equal('uid')
        expect(Profile.rawAttributes.user_id.allowNull).to.be.false
+      });
+      it('works when merging with an existing definition', function () {
+        var Task = this.sequelize.define('task', {
+            projectId: {
+              defaultValue: 42,
+              type: Sequelize.INTEGER
+            }
          })
+        , Project = this.sequelize.define('project', {});
+        Task.belongsTo(Project, { foreignKey: { allowNull: true }});
+        expect(Task.rawAttributes.projectId).to.be.defined
+        expect(Task.rawAttributes.projectId.defaultValue).to.equal(42);
+        expect(Task.rawAttributes.projectId.allowNull).to.be.ok;
+      })
+    });
    it('should throw an error if foreignKey and as result in a name clash', function () {
      var Person = this.sequelize.define('person', {})

--- a/test/associations/has-many.test.js
+++ b/test/associations/has-many.test.js
@@ -504,11 +504,7 @@ describe(Support.getTestDialectTeaser("HasMany"), function() {
          'title': DataTypes.STRING
        }), Label   = this.sequelize.define('Label', {
-          'text': DataTypes.STRING,
+          'text': DataTypes.STRING
-          'ArticleId': {
-            type: DataTypes.INTEGER,
-            allowNull: false
-          }
        });
        Article.hasMany(Label);
@@ -1337,7 +1333,7 @@ describe(Support.getTestDialectTeaser("HasMany"), function() {
        }).then(function (user) {
          return user.setTasks(null).on('sql', spy).on('sql', _.after(2, function (sql) {
            expect(sql).to.have.string("DELETE FROM");
-            expect(sql).to.have.string("IN (1,2)");
+            expect(sql).to.match(/IN \(1,2\)|IN \(2,1\)/);
          }));
        }).then(function () {
          expect(spy.calledTwice).to.be.ok; // Once for SELECT, once for DELETE
@@ -2210,23 +2206,22 @@ describe(Support.getTestDialectTeaser("HasMany"), function() {
      });
    });
-    it('allows the user to provide an attribute definition as foreignKey', function () {
+    describe('allows the user to provide an attribute definition object as foreignKey', function () {
+      it('works with a column that hasnt been defined before', function () {
        var Task = this.sequelize.define('task', {})
-        , User = this.sequelize.define('user', {
+        , User = this.sequelize.define('user', {});
-            uid: {
-              type: Sequelize.INTEGER,
-              primaryKey: true
-            }
-          });
        User.hasMany(Task, {
          foreignKey: {
-          fieldName: 'user_id',
+            name: 'uid',
            allowNull: false
          }
        });
-      expect(Task.rawAttributes.user_id.allowNull).to.be.false;
+        expect(Task.rawAttributes.uid).to.be.defined;
+        expect(Task.rawAttributes.uid.allowNull).to.be.false;
+        expect(Task.rawAttributes.uid.references).to.equal(User.getTableName());
+        expect(Task.rawAttributes.uid.referencesKey).to.equal('id');
        Task.hasMany(User, {
          foreignKey: {
@@ -2234,18 +2229,56 @@ describe(Support.getTestDialectTeaser("HasMany"), function() {
          }
        });
+        expect(Task.rawAttributes.uid).not.to.be.defined;
+        expect(Task.associations.tasksusers.through.rawAttributes.taskId).to.be.defined;
        expect(Task.associations.tasksusers.through.rawAttributes.taskId.allowNull).to.be.false;
+        expect(Task.associations.tasksusers.through.rawAttributes.taskId.references).to.equal(Task.getTableName());
+        expect(Task.associations.tasksusers.through.rawAttributes.taskId.referencesKey).to.equal('id');
+        expect(Task.associations.tasksusers.through.rawAttributes.uid).to.be.defined;
+        expect(Task.associations.tasksusers.through.rawAttributes.uid.allowNull).to.be.false;
+        expect(Task.associations.tasksusers.through.rawAttributes.uid.references).to.equal(User.getTableName());
+        expect(Task.associations.tasksusers.through.rawAttributes.uid.referencesKey).to.equal('id');
+      });
+      it('works when taking a column directly from the object', function () {
        var Project = this.sequelize.define('project', {
            user_id: {
-          type: Sequelize.INTEGER
+              type: Sequelize.INTEGER,
+              defaultValue: 42
+            }
+          })
+        , User = this.sequelize.define('user', {
+            uid: {
+              type: Sequelize.INTEGER,
+              primaryKey: true
            }
          });
        User.hasMany(Project, { foreignKey: Project.rawAttributes.user_id});
+        expect(Project.rawAttributes.user_id).to.be.defined;
        expect(Project.rawAttributes.user_id.references).to.equal(User.getTableName());
        expect(Project.rawAttributes.user_id.referencesKey).to.equal('uid');
+        expect(Project.rawAttributes.user_id.defaultValue).to.equal(42);
+      });
+      it('works when merging with an existing definition', function () {
+        var Task = this.sequelize.define('task', {
+            userId: {
+              defaultValue: 42,
+              type: Sequelize.INTEGER
+            }
+          })
+        , User = this.sequelize.define('user', {});
+        User.hasMany(Task, { foreignKey: { allowNull: true }});
+        expect(Task.rawAttributes.userId).to.be.defined;
+        expect(Task.rawAttributes.userId.defaultValue).to.equal(42);
+        expect(Task.rawAttributes.userId.allowNull).to.be.ok;
+      });
    });
    it('should throw an error if foreignKey and as result in a name clash', function () {

--- a/test/associations/has-one.test.js
+++ b/test/associations/has-one.test.js
@@ -471,7 +471,39 @@ describe(Support.getTestDialectTeaser("HasOne"), function() {
      })
    })
-    it('allows the user to provide an attribute definition as foreignKey', function () {
+    describe('allows the user to provide an attribute definition object as foreignKey', function () {
+      it('works with a column that hasnt been defined before', function () {
+        var User = this.sequelize.define('user', {})
+      , Profile = this.sequelize.define('project', {})
+        User.hasOne(Profile, {
+          foreignKey: {
+            allowNull: false,
+            name: 'uid'
+          }
+        });
+        expect(Profile.rawAttributes.uid).to.be.defined
+        expect(Profile.rawAttributes.uid.references).to.equal(User.getTableName())
+        expect(Profile.rawAttributes.uid.referencesKey).to.equal('id')
+        expect(Profile.rawAttributes.uid.allowNull).to.be.false;
+        // Let's clear it
+        Profile = this.sequelize.define('project', {})
+        User.hasOne(Profile, {
+          foreignKey: {
+            allowNull: false,
+            name: 'uid',
+          }
+        });
+        expect(Profile.rawAttributes.uid).to.be.defined
+        expect(Profile.rawAttributes.uid.references).to.equal(User.getTableName())
+        expect(Profile.rawAttributes.uid.referencesKey).to.equal('id')
+        expect(Profile.rawAttributes.uid.allowNull).to.be.false;
+      });
+      it('works when taking a column directly from the object', function () {
        var User = this.sequelize.define('user', {
          uid: {
            type: Sequelize.INTEGER,
@@ -487,11 +519,36 @@ describe(Support.getTestDialectTeaser("HasOne"), function() {
        User.hasOne(Profile, { foreignKey: Profile.rawAttributes.user_id})
+        console.log(Profile.rawAttributes.user_id);
        expect(Profile.rawAttributes.user_id).to.be.defined
        expect(Profile.rawAttributes.user_id.references).to.equal(User.getTableName())
        expect(Profile.rawAttributes.user_id.referencesKey).to.equal('uid')
-      expect(Profile.rawAttributes.user_id.allowNull).to.be.false
+        expect(Profile.rawAttributes.user_id.allowNull).to.be.false;
+      });
+      it('works when merging with an existing definition', function () {
+        var User = this.sequelize.define('user', {
+            uid: {
+              type: Sequelize.INTEGER,
+              primaryKey: true
+            }
          })
+        , Project = this.sequelize.define('project', {
+            userUid: {
+              type: Sequelize.INTEGER,
+              defaultValue: 42
+            }
+          });
+        User.hasOne(Project, { foreignKey: { allowNull: false }});
+        expect(Project.rawAttributes.userUid).to.be.defined;
+        expect(Project.rawAttributes.userUid.allowNull).to.be.false;
+        expect(Project.rawAttributes.userUid.references).to.equal(User.getTableName())
+        expect(Project.rawAttributes.userUid.referencesKey).to.equal('uid')
+        expect(Project.rawAttributes.userUid.defaultValue).to.equal(42);
+      });
+    });
    it('should throw an error if an association clashes with the name of an already define attribute', function () {
       var User = this.sequelize.define('user', {

--- a/test/dao-factory/findAll.test.js
+++ b/test/dao-factory/findAll.test.js
@@ -284,8 +284,7 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
        User.belongsTo(Binary, { foreignKey: 'binary' })
-        User.sync({ force: true }).success(function() {
+        this.sequelize.sync({ force: true }).success(function() {
-          Binary.sync({ force: true }).success(function() {
          User.bulkCreate([
            {username: 'boo5', aBool: false},
            {username: 'boo6', aBool: true}
@@ -321,7 +320,6 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
          })
        })
      })
-      })
      it('should be able to return a record with primaryKey being null for new inserts', function(done) {
        var Session = this.sequelize.define('Session', {

--- a/test/include/findAll.test.js
+++ b/test/include/findAll.test.js
@@ -933,8 +933,6 @@ describe(Support.getTestDialectTeaser("Include"), function () {
      })
    })
    it("should be possible to define a belongsTo include as required with child hasMany not required", function(done) {
      var S = this.sequelize
        , Address = S.define('Address', { 'active': DataTypes.BOOLEAN })

--- a/test/include/findAndCountAll.test.js
+++ b/test/include/findAndCountAll.test.js
@@ -29,13 +29,13 @@ describe(Support.getTestDialectTeaser("Include"), function () {
      User.hasMany( SomeConnection, { foreignKey: 'u' })
      SomeConnection.belongsTo( User, { foreignKey: 'u' })
-      SomeConnection.belongsTo( A, { foreignKey: 'fk' })
+      SomeConnection.belongsTo( A, { foreignKey: 'fk', constraints: false })
-      SomeConnection.belongsTo( B, { foreignKey: 'fk' })
+      SomeConnection.belongsTo( B, { foreignKey: 'fk', constraints: false })
-      SomeConnection.belongsTo( C, { foreignKey: 'fk' })
+      SomeConnection.belongsTo( C, { foreignKey: 'fk', constraints: false })
-      A.hasMany( SomeConnection, { foreignKey: 'fk' })
+      A.hasMany( SomeConnection, { foreignKey: 'fk', constraints: false })
-      B.hasMany( SomeConnection, { foreignKey: 'fk' })
+      B.hasMany( SomeConnection, { foreignKey: 'fk', constraints: false })
-      C.hasMany( SomeConnection, { foreignKey: 'fk' })
+      C.hasMany( SomeConnection, { foreignKey: 'fk', constraints: false })
      // Sync them
      S.sync({ force: true }).done( function ( err ) { expect( err ).not.to.be.ok;