Merge pull request #4506 from Americas/foreignkeys

Foreign keys with allowNull: false with new default values

changelog.md

@@ -2,6 +2,8 @@
 - [ADDED] Expose Association constructor as `Sequelize.Association`
 - [FIXED] Calling set with dot.separated key on a JSON/JSONB attribute will not flag the entire object as changed [#4379](https://github.com/sequelize/sequelize/pull/4379)
 - [FIXED] instances returned from `bulkCreate` now has `isNewRecord: false` and should be updateable if using `returning: true` with dialects that support it.
+- [FIXED] Find with Include with a where clause generates wrong SQL [#3940](https://github.com/sequelize/sequelize/issues/3940)
+- [FIXED] ON DELETE constraint should default to CASCADE if foreignKey has allowNull: false] [#2831](https://github.com/sequelize/sequelize/issues/2831)
 
 # 3.9.0
 - [ADDED] beforeRestore/afterRestore hooks [#4371](https://github.com/sequelize/sequelize/issues/4371)

lib/associations/belongs-to.js

@@ -109,11 +109,13 @@ BelongsTo.prototype.injectAttributes = function() {
   var newAttributes = {};
 
   newAttributes[this.foreignKey] = _.defaults(this.foreignKeyAttribute, {
-    type: this.options.keyType || this.target.rawAttributes[this.targetKey].type
+    type: this.options.keyType || this.target.rawAttributes[this.targetKey].type,
+    allowNull : true
   });
 
   if (this.options.constraints !== false) {
-    this.options.onDelete = this.options.onDelete || 'SET NULL';
+    var source = this.source.rawAttributes[this.foreignKey] || newAttributes[this.foreignKey];
+    this.options.onDelete = this.options.onDelete || (source.allowNull ? 'SET NULL' : 'NO ACTION');
     this.options.onUpdate = this.options.onUpdate || 'CASCADE';
   }
 

lib/associations/has-many.js

@@ -201,10 +201,14 @@ util.inherits(HasMany, Association);
 HasMany.prototype.injectAttributes = function() {
   var newAttributes = {};
   var constraintOptions = _.clone(this.options); // Create a new options object for use with addForeignKeyConstraints, to avoid polluting this.options in case it is later used for a n:m
-  newAttributes[this.foreignKey] = _.defaults(this.foreignKeyAttribute, { type: this.options.keyType || this.source.rawAttributes[this.source.primaryKeyAttribute].type });
+  newAttributes[this.foreignKey] = _.defaults(this.foreignKeyAttribute, {
+    type: this.options.keyType || this.source.rawAttributes[this.source.primaryKeyAttribute].type,
+    allowNull : true
+  });
 
   if (this.options.constraints !== false) {
-    constraintOptions.onDelete = constraintOptions.onDelete || 'SET NULL';
+    var target = this.target.rawAttributes[this.foreignKey] || newAttributes[this.foreignKey];
+    constraintOptions.onDelete = constraintOptions.onDelete || (target.allowNull ? 'SET NULL' : 'CASCADE');
     constraintOptions.onUpdate = constraintOptions.onUpdate || 'CASCADE';
   }
   Helpers.addForeignKeyConstraints(newAttributes[this.foreignKey], this.source, this.target, constraintOptions);

lib/associations/has-one.js

@@ -101,15 +101,20 @@ HasOne.prototype.injectAttributes = function() {
   var newAttributes = {}
     , keyType = this.source.rawAttributes[this.source.primaryKeyAttribute].type;
 
-  newAttributes[this.foreignKey] = _.defaults(this.foreignKeyAttribute, { type: this.options.keyType || keyType });
+  newAttributes[this.foreignKey] = _.defaults(this.foreignKeyAttribute, {
+    type: this.options.keyType || keyType,
+    allowNull : true
+  });
   Utils.mergeDefaults(this.target.rawAttributes, newAttributes);
 
   this.identifierField = this.target.rawAttributes[this.foreignKey].field || this.foreignKey;
 
   if (this.options.constraints !== false) {
-    this.options.onDelete = this.options.onDelete || 'SET NULL';
+    var target = this.target.rawAttributes[this.foreignKey] || newAttributes[this.foreignKey];
+    this.options.onDelete = this.options.onDelete || (target.allowNull ? 'SET NULL' : 'CASCADE');
     this.options.onUpdate = this.options.onUpdate || 'CASCADE';
   }
+
   Helpers.addForeignKeyConstraints(this.target.rawAttributes[this.foreignKey], this.source, this.target, this.options);
 
   // Sync attributes and setters/getters to Model prototype

lib/associations/mixin.js

@@ -123,7 +123,7 @@ var singleLinked = function (Type) {
  * @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 `name` property to set the name of the colum. Defaults to the name of source + primary key of source
- * @param {string}          [options.onDelete='SET NULL']
+ * @param {string}          [options.onDelete='SET NULL|CASCADE'] SET NULL if foreignKey allows nulls, CASCADE if otherwise
  * @param {string}          [options.onUpdate='CASCADE']
  * @param {boolean}         [options.constraints=true] Should on update and on delete constraints be enabled on the foreign key.
  */
@@ -141,7 +141,7 @@ Mixin.hasOne = singleLinked(HasOne);
  * @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 association, 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 `name` property to set the name of the column. Defaults to the name of target + primary key of target
  * @param {string}          [options.targetKey] The name of the field to use as the key for the association in the target table. Defaults to the primary key of the target table
- * @param {string}          [options.onDelete='SET NULL']
+ * @param {string}          [options.onDelete='SET NULL|NO ACTION'] SET NULL if foreignKey allows nulls, NO ACTION if otherwise
  * @param {string}          [options.onUpdate='CASCADE']
  * @param {boolean}         [options.constraints=true] Should on update and on delete constraints be enabled on the foreign key.
  */
@@ -197,7 +197,7 @@ Mixin.belongsTo = singleLinked(BelongsTo);
  * @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 `name` property to set the name of the colum. Defaults to the name of source + primary key of source
  * @param {object}              [options.scope] A key/value set that will be used for association create and find defaults on the target. (sqlite not supported for N:M)
- * @param {string}              [options.onDelete='SET NULL|CASCADE'] Cascade if this is a n:m, and set null if it is a 1:m
+ * @param {string}              [options.onDelete='SET NULL|CASCADE'] SET NULL if foreignKey allows nulls, CASCADE if otherwise
  * @param {string}              [options.onUpdate='CASCADE']
  * @param {boolean}             [options.constraints=true] Should on update and on delete constraints be enabled on the foreign key.
  */

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

@@ -463,6 +463,25 @@ describe(Support.getTestDialectTeaser('BelongsTo'), function() {
       });
     });
 
+    it('sets to NO ACTION if allowNull: false', function() {
+      var Task = this.sequelize.define('Task', { title: DataTypes.STRING })
+        , User = this.sequelize.define('User', { username: DataTypes.STRING });
+
+      Task.belongsTo(User, { foreignKey: { allowNull: false }}); // defaults to NO ACTION
+
+      return this.sequelize.sync({ force: true }).then(function() {
+        return User.create({ username: 'foo' }).then(function(user) {
+          return Task.create({ title: 'task', UserId: user.id }).then(function(task) {
+            return expect(user.destroy()).to.eventually.be.rejectedWith(Sequelize.ForeignKeyConstraintError).then(function () {
+              return Task.findAll().then(function(tasks) {
+                expect(tasks).to.have.length(1);
+              });
+            });
+          });
+        });
+      });
+    });
+
     it('should be possible to disable them', function() {
       var Task = this.sequelize.define('Task', { title: Sequelize.STRING })
         , User = this.sequelize.define('User', { username: Sequelize.STRING });

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

@@ -870,6 +870,25 @@ describe(Support.getTestDialectTeaser('HasMany'), function() {
         });
       });
 
+      it('sets to CASCADE if allowNull: false', function() {
+        var Task = this.sequelize.define('Task', { title: DataTypes.STRING })
+          , User = this.sequelize.define('User', { username: DataTypes.STRING });
+
+        User.hasMany(Task, { foreignKey: { allowNull: false }}); // defaults to CASCADE
+
+        return this.sequelize.sync({ force: true }).then(function() {
+          return User.create({ username: 'foo' }).then(function(user) {
+            return Task.create({ title: 'task', UserId: user.id }).then(function() {
+              return user.destroy().then(function() {
+                return Task.findAll();
+              });
+            });
+          }).then(function(tasks) {
+            expect(tasks).to.be.empty;
+          });
+        });
+      });
+
       it('should be possible to remove all constraints', function() {
         var Task = this.sequelize.define('Task', { title: DataTypes.STRING })
           , User = this.sequelize.define('User', { username: DataTypes.STRING });

test/integration/associations/has-one.test.js

@@ -352,6 +352,25 @@ describe(Support.getTestDialectTeaser('HasOne'), function() {
       });
     });
 
+    it('sets to CASCADE if allowNull: false', function() {
+      var Task = this.sequelize.define('Task', { title: Sequelize.STRING })
+        , User = this.sequelize.define('User', { username: Sequelize.STRING });
+
+      User.hasOne(Task, { foreignKey: { allowNull: false }}); // defaults to CASCADE
+
+      return this.sequelize.sync({ force: true }).then(function() {
+        return User.create({ username: 'foo' }).then(function(user) {
+          return Task.create({ title: 'task', UserId: user.id }).then(function() {
+            return user.destroy().then(function() {
+              return Task.findAll();
+            });
+          });
+        }).then(function(tasks) {
+          expect(tasks).to.be.empty;
+        });
+      });
+    });
+
     it('should be possible to disable them', function() {
       var Task = this.sequelize.define('Task', { title: Sequelize.STRING })
         , User = this.sequelize.define('User', { username: Sequelize.STRING });