feat: make set association method delete old associated entity for …

…non-null FKs (#15840)
sequelize · Apr 28, 2023 · 67d66f1 · 67d66f1
1 parent 0a572b9
commit 67d66f1
Show file tree

Hide file tree

Showing 9 changed files with 683 additions and 243 deletions.
diff --git a/packages/core/src/associations/belongs-to-many.ts b/packages/core/src/associations/belongs-to-many.ts
@@ -483,14 +483,14 @@ Add your own primary key to the through model, on different attributes than the
     let model = this.target;
     if (options?.scope != null) {
       if (!options.scope) {
-        model = model.unscoped();
+        model = model.withoutScope();
       } else if (options.scope !== true) { // 'true' means default scope. Which is the same as not doing anything.
-        model = model.scope(options.scope);
+        model = model.withScope(options.scope);
       }
     }
 
     if (options?.schema) {
-      model = model.schema(options.schema, options.schemaDelimiter);
+      model = model.withSchema({ schema: options.schema, schemaDelimiter: options.schemaDelimiter });
     }
 
     return model.findAll(findOptions);

diff --git a/packages/core/src/associations/belongs-to.ts b/packages/core/src/associations/belongs-to.ts
@@ -292,14 +292,14 @@ export class BelongsTo<
     let Target = this.target;
     if (options.scope != null) {
       if (!options.scope) {
-        Target = Target.unscoped();
+        Target = Target.withoutScope();
       } else if (options.scope !== true) { // 'true' means default scope. Which is the same as not doing anything.
-        Target = Target.scope(options.scope);
+        Target = Target.withScope(options.scope);
       }
     }
 
     if (options.schema != null) {
-      Target = Target.schema(options.schema, options.schemaDelimiter);
+      Target = Target.withSchema({ schema: options.schema, schemaDelimiter: options.schemaDelimiter });
     }
 
     let isManyMode = true;

diff --git a/packages/core/src/associations/has-many.ts b/packages/core/src/associations/has-many.ts
@@ -1,3 +1,4 @@
+import isObject from 'lodash/isObject';
 import upperFirst from 'lodash/upperFirst';
 import type { WhereOptions } from '../dialects/abstract/where-sql-builder-types.js';
 import { AssociationError } from '../errors/index.js';
@@ -8,6 +9,7 @@ import type {
   Attributes,
   CreateOptions,
   CreationAttributes,
+  DestroyOptions,
   Filterable,
   FindOptions,
   InstanceUpdateOptions,
@@ -237,14 +239,14 @@ export class HasMany<
     let Model = this.target;
     if (options.scope != null) {
       if (!options.scope) {
-        Model = Model.unscoped();
+        Model = Model.withoutScope();
       } else if (options.scope !== true) { // 'true' means default scope. Which is the same as not doing anything.
-        Model = Model.scope(options.scope);
+        Model = Model.withScope(options.scope);
       }
     }
 
     if (options.schema != null) {
-      Model = Model.schema(options.schema, options.schemaDelimiter);
+      Model = Model.withSchema({ schema: options.schema, schemaDelimiter: options.schemaDelimiter });
     }
 
     const results = await Model.findAll(findOptions);
@@ -315,6 +317,7 @@ export class HasMany<
     const where = {
       [Op.or]: targetInstances.map(instance => {
         if (instance instanceof this.target) {
+
           // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion -- needed for TS < 5.0
           return (instance as T).where();
         }
@@ -377,8 +380,10 @@ export class HasMany<
     });
 
     if (obsoleteAssociations.length > 0) {
-      // TODO: if foreign key cannot be null, delete instead (maybe behind flag) - https://github.com/sequelize/sequelize/issues/14048
-      promises.push(this.remove(sourceInstance, obsoleteAssociations, options));
+      promises.push(this.remove(sourceInstance, obsoleteAssociations, {
+        ...options,
+        destroy: options?.destroyPrevious,
+      }));
     }
 
     if (unassociatedObjects.length > 0) {
@@ -395,7 +400,7 @@ export class HasMany<
         }),
       };
 
-      promises.push(this.target.unscoped().update(
+      promises.push(this.target.withoutScope().update(
         update,
         {
           ...options,
@@ -439,7 +444,7 @@ export class HasMany<
       }),
     };
 
-    await this.target.unscoped().update(update, { ...options, where });
+    await this.target.withoutScope().update(update, { ...options, where });
   }
 
   /**
@@ -466,11 +471,6 @@ export class HasMany<
       return;
     }
 
-    // TODO: if foreign key cannot be null, delete instead (maybe behind flag) - https://github.com/sequelize/sequelize/issues/14048
-    const update = {
-      [this.foreignKey]: null,
-    } as UpdateValues<T>;
-
     const where: WhereOptions = {
       [this.foreignKey]: sourceInstance.get(this.sourceKey),
       // @ts-expect-error -- TODO: what if the target has no primary key?
@@ -493,7 +493,23 @@ export class HasMany<
       }),
     };
 
-    await this.target.unscoped().update(update, { ...options, where });
+    const foreignKeyIsNullable = this.target.modelDefinition.attributes.get(this.foreignKey)?.allowNull ?? true;
+
+    if (options.destroy || !foreignKeyIsNullable) {
+      await this.target.withoutScope().destroy({
+        ...(isObject(options.destroy) ? options.destroy : undefined),
+        logging: options.logging,
+        benchmark: options.benchmark,
+        transaction: options.transaction,
+        where,
+      });
+    } else {
+      const update = {
+        [this.foreignKey]: null,
+      } as UpdateValues<T>;
+
+      await this.target.withoutScope().update(update, { ...options, where });
+    }
   }
 
   /**
@@ -606,7 +622,16 @@ export type HasManyGetAssociationsMixin<T extends Model> = (options?: HasManyGet
  * @see HasManySetAssociationsMixin
  */
 export interface HasManySetAssociationsMixinOptions<T extends Model>
-  extends FindOptions<Attributes<T>>, InstanceUpdateOptions<Attributes<T>> {}
+  extends FindOptions<Attributes<T>>, InstanceUpdateOptions<Attributes<T>> {
+
+  /**
+   * Delete the previous associated model. Default to false.
+   *
+   * Only applies if the foreign key is nullable. If the foreign key is not nullable,
+   * the previous associated model is always deleted.
+   */
+  destroyPrevious?: boolean | Omit<DestroyOptions<Attributes<T>>, 'where' | 'transaction' | 'logging' | 'benchmark'> | undefined;
+}
 
 /**
  * The setAssociations mixin applied to models with hasMany.
@@ -623,7 +648,7 @@ export interface HasManySetAssociationsMixinOptions<T extends Model>
  * @see Model.hasMany
  */
 export type HasManySetAssociationsMixin<T extends Model, TModelPrimaryKey> = (
-  newAssociations?: Array<T | TModelPrimaryKey>,
+  newAssociations?: Array<T | TModelPrimaryKey> | null,
   options?: HasManySetAssociationsMixinOptions<T>,
 ) => Promise<void>;
 
@@ -744,7 +769,16 @@ export type HasManyRemoveAssociationMixin<T extends Model, TModelPrimaryKey> = (
  * @see HasManyRemoveAssociationsMixin
  */
 export interface HasManyRemoveAssociationsMixinOptions<T extends Model>
-  extends InstanceUpdateOptions<Attributes<T>> {}
+  extends Omit<InstanceUpdateOptions<Attributes<T>>, 'where'> {
+
+  /**
+   * Delete the associated model. Default to false.
+   *
+   * Only applies if the foreign key is nullable. If the foreign key is not nullable,
+   * the associated model is always deleted.
+   */
+  destroy?: boolean | Omit<DestroyOptions<Attributes<T>>, 'where' | 'transaction' | 'logging' | 'benchmark'> | undefined;
+}
 
 /**
  * The removeAssociations mixin applied to models with hasMany.

diff --git a/packages/core/src/associations/has-one.ts b/packages/core/src/associations/has-one.ts
@@ -1,3 +1,4 @@
+import isObject from 'lodash/isObject';
 import upperFirst from 'lodash/upperFirst';
 import { AssociationError } from '../errors/index.js';
 import { Model } from '../model';
@@ -7,8 +8,9 @@ import type {
   CreateOptions,
   CreationAttributes,
   FindOptions,
+  InstanceDestroyOptions,
+  InstanceUpdateOptions,
   ModelStatic,
-  SaveOptions,
 } from '../model';
 import { Op } from '../operators';
 import { isSameInitialModel } from '../utils/model-utils.js';
@@ -177,14 +179,14 @@ If having two associations does not make sense (for instance a "spouse" associat
     let Target = this.target;
     if (options.scope != null) {
       if (!options.scope) {
-        Target = Target.unscoped();
+        Target = Target.withoutScope();
       } else if (options.scope !== true) { // 'true' means default scope. Which is the same as not doing anything.
-        Target = Target.scope(options.scope);
+        Target = Target.withScope(options.scope);
       }
     }
 
     if (options.schema != null) {
-      Target = Target.schema(options.schema, options.schemaDelimiter);
+      Target = Target.withSchema({ schema: options.schema, schemaDelimiter: options.schemaDelimiter });
     }
 
     let isManyMode = true;
@@ -248,7 +250,9 @@ If having two associations does not make sense (for instance a "spouse" associat
     // @ts-expect-error -- .save isn't listed in the options because it's not supported, but we'll still warn users if they use it.
     if (options.save === false) {
       throw new Error(`The "save: false" option cannot be honoured in ${this.source.name}#${this.accessors.set}
-because, as this is a hasOne association, the foreign key we need to update is located on the model ${this.target.name}.`);
+because, as this is a hasOne association, the foreign key we need to update is located on the model ${this.target.name}.
+
+This option is only available in BelongsTo associations.`);
     }
 
     // calls the 'get' mixin
@@ -268,14 +272,23 @@ because, as this is a hasOne association, the foreign key we need to update is l
     }
 
     if (oldInstance) {
-      // TODO: if foreign key cannot be null, delete instead (maybe behind flag) - https://github.com/sequelize/sequelize/issues/14048
-      oldInstance.set(this.foreignKey, null);
-
-      await oldInstance.save({
-        ...options,
-        fields: [this.foreignKey],
-        association: true,
-      });
+      const foreignKeyIsNullable = this.target.modelDefinition.attributes.get(this.foreignKey)?.allowNull ?? true;
+
+      if (options.destroyPrevious || !foreignKeyIsNullable) {
+        await oldInstance.destroy({
+          ...(isObject(options.destroyPrevious) ? options.destroyPrevious : undefined),
+          logging: options.logging,
+          benchmark: options.benchmark,
+          transaction: options.transaction,
+        });
+      } else {
+        await oldInstance.update({
+          [this.foreignKey]: null,
+        }, {
+          ...options,
+          association: true,
+        });
+      }
     }
 
     if (associatedInstanceOrPk) {
@@ -410,7 +423,15 @@ export type HasOneGetAssociationMixin<
  * @see HasOneSetAssociationMixin
  */
 export interface HasOneSetAssociationMixinOptions<T extends Model>
-  extends HasOneGetAssociationMixinOptions<T>, SaveOptions<Attributes<T>> {
+  extends HasOneGetAssociationMixinOptions<T>, InstanceUpdateOptions<Attributes<T>> {
+
+  /**
+   * Delete the previous associated model. Default to false.
+   *
+   * Only applies if the foreign key is nullable. If the foreign key is not nullable,
+   * the previous associated model is always deleted.
+   */
+  destroyPrevious?: boolean | Omit<InstanceDestroyOptions, 'transaction' | 'logging' | 'benchmark'>;
 }
 
 /**

diff --git a/packages/core/src/model.d.ts b/packages/core/src/model.d.ts
@@ -40,12 +40,12 @@ export interface Logging {
   /**
    * A function that gets executed while running the query to log the sql.
    */
-  logging?: boolean | ((sql: string, timing?: number) => void);
+  logging?: boolean | ((sql: string, timing?: number) => void) | undefined;
 
   /**
    * Pass query execution time in milliseconds as second argument to logging function (options.logging).
    */
-  benchmark?: boolean;
+  benchmark?: boolean | undefined;
 }
 
 export interface Poolable {
@@ -157,7 +157,7 @@ export interface SchemaOptions {
   /**
    * The character(s) that separates the schema name from the table name
    */
-  schemaDelimiter?: string;
+  schemaDelimiter?: string | undefined;
 }
 
 /**