Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

packages/core/src/associations/belongs-to-many.ts (1)

931-952: Do not mutate caller options; clone and dedupe fields before bulkCreate.

Nice job ensuring scope keys are included when fields is provided. To avoid side effects on the caller’s options and to dedupe cleanly, shallow‑clone into bulkCreateOptions and build fields via a Set, then pass that to bulkCreate.

Apply this diff:

-    // Handle fields option to include scope columns
-    if (options.fields && this.scope) {
-      const scopeKeys = Object.keys(this.scope);
-      options.fields = [
-        ...options.fields,
-        ...scopeKeys.filter(key => !options.fields!.includes(key)),
-      ];
-    }
+    // Handle fields option to include scope columns without mutating caller options
+    let bulkCreateOptions = options;
+    if (this.scope && options.fields) {
+      const fieldSet = new Set(options.fields as Array<keyof Attributes<TargetModel>>);
+      for (const key of Object.keys(this.scope)) {
+        fieldSet.add(key as keyof Attributes<TargetModel>);
+      }
+      bulkCreateOptions = { ...options, fields: Array.from(fieldSet) };
+    }
@@
-    // Create the related model instances
-    const newAssociatedObjects = await this.target.bulkCreate(recordsToCreate, options);
+    // Create the related model instances
+    const newAssociatedObjects = await this.target.bulkCreate(recordsToCreate, bulkCreateOptions);

packages/core/src/associations/has-many.ts (1)

887-889: Expose bulk options: extend BulkCreateOptions instead of CreateOptions.

createMultiple delegates to bulkCreate; callers must be able to pass bulk‑specific options (e.g., updateOnDuplicate, individualHooks).

Apply this diff:

-export interface HasManyCreateAssociationsMixinOptions<T extends Model>
-  extends CreateOptions<Attributes<T>> {}
+export interface HasManyCreateAssociationsMixinOptions<T extends Model>
+  extends BulkCreateOptions<Attributes<T>> {}

Add the missing import at the top of the file:

-  Attributes,
-  CreateOptions,
+  Attributes,
+  BulkCreateOptions,
+  CreateOptions,

packages/core/src/associations/has-many.ts (1)

630-644: Avoid mutating options and dedupe fields before bulkCreate.

Clone options when augmenting fields and build it via a Set; then pass the cloned options to bulkCreate.

Apply this diff:

-    // Handle fields option to include scope and foreign key columns
-    if (options.fields) {
-      // Add scope attributes to fields if scope exists
-      if (this.scope) {
-        for (const attribute of Object.keys(this.scope)) {
-          if (!options.fields.includes(attribute)) {
-            options.fields.push(attribute);
-          }
-        }
-      }
-
-      // Add foreign key to fields if not already present
-      if (!options.fields.includes(this.foreignKey)) {
-        options.fields.push(this.foreignKey);
-      }
-    }
+    // Handle fields option without mutating caller options
+    let bulkCreateOptions = options;
+    if (options.fields) {
+      const fieldSet = new Set(options.fields as Array<keyof Attributes<T>>);
+      if (this.scope) {
+        for (const attribute of Object.keys(this.scope)) {
+          fieldSet.add(attribute as keyof Attributes<T>);
+        }
+      }
+      fieldSet.add(this.foreignKey as keyof Attributes<T>);
+      bulkCreateOptions = { ...options, fields: Array.from(fieldSet) };
+    }
@@
-    return this.target.bulkCreate(recordsToCreate, options);
+    return this.target.bulkCreate(recordsToCreate, bulkCreateOptions);

Also applies to: 664-664

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

packages/core/src/associations/has-many.ts (1)

631-645: Consider cloning options before mutating fields to avoid side effects.

The current implementation mutates the caller's options.fields array by pushing to it. While this mirrors the pattern in the single-record create method (lines 593–600), it can cause unexpected behavior if the caller:

• Reuses the same options object for multiple calls
• Passes a frozen or readonly object
• Expects the options object to remain unchanged

Apply this diff to create a shallow copy:

     const sourceKeyValue = sourceInstance.get(this.sourceKey);

     // Handle fields option to include scope and foreign key columns
+    let bulkCreateOptions = options;
     if (options.fields) {
+      bulkCreateOptions = {
+        ...options,
+        fields: [...options.fields],
+      };
+
       // Add scope attributes to fields if scope exists
       if (this.scope) {
         for (const attribute of Object.keys(this.scope)) {
-          if (!options.fields.includes(attribute)) {
-            options.fields.push(attribute);
+          if (!bulkCreateOptions.fields.includes(attribute)) {
+            bulkCreateOptions.fields.push(attribute);
           }
         }
       }

       // Add foreign key to fields if not already present
-      if (!options.fields.includes(this.foreignKey)) {
-        options.fields.push(this.foreignKey);
+      if (!bulkCreateOptions.fields.includes(this.foreignKey)) {
+        bulkCreateOptions.fields.push(this.foreignKey);
       }
     }

     const recordsToCreate = valuesArray.map(values => {
       const recordValues = { ...values };

       // Apply scope if it exists
       if (this.scope) {
         for (const attribute of Object.keys(this.scope)) {
           // @ts-expect-error -- TODO: fix the typing of {@link AssociationScope}
           recordValues[attribute] = this.scope[attribute];
         }
       }

       // Set the foreign key
       // @ts-expect-error -- TODO: fix the typing of foreign key assignment
       recordValues[this.foreignKey] = sourceKeyValue;

       return recordValues;
     });

-    return this.target.bulkCreate(recordsToCreate, options);
+    return this.target.bulkCreate(recordsToCreate, bulkCreateOptions);

packages/core/src/associations/belongs-to-many.ts (1)

930-937: Consider cloning options before mutating fields to avoid side effects.

The current implementation mutates the caller's options object by reassigning options.fields (lines 933–936). While this approach creates a new array, it still modifies the original options object, which can cause unexpected behavior if the caller:

• Reuses the same options object for multiple calls
• Passes a frozen or readonly object
• Expects the options object to remain unchanged

Apply this diff to create a shallow copy:

-    // Handle fields option to include scope columns
-    if (options.fields && this.scope) {
+    let bulkCreateOptions = options;
+    if (bulkCreateOptions.fields && this.scope) {
       const scopeKeys = Object.keys(this.scope);
-      options.fields = [
-        ...options.fields,
+      bulkCreateOptions = {
+        ...bulkCreateOptions,
+        fields: [
+        ...bulkCreateOptions.fields,
         ...scopeKeys.filter(key => !options.fields!.includes(key)),
-      ];
+        ],
+      };
     }

     const recordsToCreate = valuesArray.map(values => {
       const recordValues = { ...values };

       // Apply scope if it exists
       if (this.scope) {
         Object.assign(recordValues, this.scope);
       }

       return recordValues;
     });

     // Create the related model instances
-    const newAssociatedObjects = await this.target.bulkCreate(recordsToCreate, options);
+    const newAssociatedObjects = await this.target.bulkCreate(recordsToCreate, bulkCreateOptions);

     // Associate all created instances
     await this.add(sourceInstance, newAssociatedObjects, omit(options, ['fields']));

     return newAssociatedObjects;

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

test-createmultiple-simple.js (3)

17-24: Remove redundant foreign key definition.

The userId field on line 20 is redundant. When you define User.hasMany(Post), Sequelize automatically creates the userId foreign key on the Post model. Explicitly defining it can cause confusion and potential conflicts.

Apply this diff:

 const Post = sequelize.define('Post', {
   title: DataTypes.STRING,
   content: DataTypes.STRING,
-  userId: DataTypes.INTEGER,
 });

---

55-66: Restructure test to avoid dropping tables mid-execution.

The second sync({ force: true }) on line 66 drops all tables created in the first test scenario, making the test structure confusing.

Consider either:

1. Creating separate test functions for each scenario
2. Using a fresh Sequelize instance for each test
3. Using unique table names for each scenario (UserWithScope/PostWithScope vs User/Post)

Also, the explicit userId definition on line 58 is redundant, as noted in the previous comment.

Apply this diff:

 const PostWithScope = sequelize.define('PostWithScope', {
   title: DataTypes.STRING,
   content: DataTypes.STRING,
-  userId: DataTypes.INTEGER,
   status: DataTypes.STRING,
 });

---

4-10: Clean up SQLite database file after test.

The test creates ./test.db but doesn't clean it up. Consider using an in-memory database (:memory:) or explicitly deleting the file after the test completes.

For in-memory database:

   const sequelize = new Sequelize({
     dialect: SqliteDialect,
-    storage: './test.db',
+    storage: ':memory:',
     logging: false,
   });

Or add cleanup in finally block:

+  const fs = require('node:fs');
   } finally {
     await sequelize.close();
+    try {
+      fs.unlinkSync('./test.db');
+    } catch (error) {
+      // Ignore if file doesn't exist
+    }
   }

packages/core/test/integration/associations/belongs-to-many.test.js (1)

2381-2385: Consider removing unused scope definition.

The scope defined on User.belongsToMany(Task) (lines 2381-2385) is not used in this test, since the test only calls task.createUserScopes(), which uses the Task.belongsToMany(User) association. The unused scope definition may cause confusion.

If the scope on the User→Task association is not needed for this test, consider removing it for clarity.

-User.belongsToMany(Task, {
-  through: 'UserTaskScopes',
-  as: 'taskScopes',
-  scope: { status: 'active' },
-});
+User.belongsToMany(Task, {
+  through: 'UserTaskScopes',
+  as: 'taskScopes',
+});

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

packages/core/src/associations/belongs-to-many.ts (1)

914-959: LGTM: Implementation correctly handles bulk creation with scope and association.

The method properly:

• Returns early for empty arrays
• Merges scope keys into options.fields to ensure scoped columns are persisted (addressing previous review concerns)
• Applies scope attributes to each record before bulk creation
• Associates all created instances with the source

The implementation follows the established pattern from the single create method and efficiently uses bulkCreate for the bulk operation.

Optional: Consider using omit for consistency.

For consistency with the single create method (line 909), consider using omit instead of manual property deletion:

-    const addOptions = { ...options };
-    delete addOptions.fields;
-    await this.add(sourceInstance, newAssociatedObjects, addOptions);
+    await this.add(sourceInstance, newAssociatedObjects, omit(options, ['fields']));

This is a minor stylistic preference and not a functional issue.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro