sequelize icon indicating copy to clipboard operation
sequelize copied to clipboard

fix(core): ensure bulkInsert returns array of inserted IDs

Open youssef2272002salah opened this issue 3 months ago • 5 comments

Pull Request Checklist

  • [x] Have you added new tests to prevent regressions?
  • [ ] If a documentation update is necessary, have you opened a PR to the documentation repository?
  • [x] Did you update the typescript typings accordingly (if applicable)?
  • [x] Does the description below contain a link to an existing issue (Closes #14325)?
  • [x] Does the name of your PR follow our conventions?

Description of Changes

This PR ensures queryInterface.bulkInsert consistently returns an array of inserted IDs across mysql dialect, instead of a single value or undefined.

  • Updated bulkInsert implementation to return an array.

Previously:

await queryInterface.bulkInsert('companies', [
  { name: 'Example Company 1' },
  { name: 'Example Company 2' },
]);
// → 1

now:

await queryInterface.bulkInsert('companies', [
  { name: 'Example Company 1' },
  { name: 'Example Company 2' },
]);
// → [1, 2]

Closes #14325

List of Breaking Changes

  • None (the return type is extended, but still backward-compatible).

Summary by CodeRabbit

  • New Features

    • Bulk inserts now consistently return arrays of id objects for multi-row inserts in MySQL/MariaDB; single-row and empty inserts unchanged.
    • Insert ID types (number, string, BigInt) are preserved and normalized across both model and no-model insert paths.
  • Tests

    • Added integration tests validating bulkInsert return shapes across dialects and single-row behavior.
    • Expanded unit tests for result formatting and related logging.

youssef2272002salah avatar Oct 01 '25 13:10 youssef2272002salah

Walkthrough

Modify MySQL and MariaDB insert result formatting so bulk inserts return consistent arrays of { id }-style objects derived from the driver's insertId and affectedRows; add unit tests for MySQL formatResults and an integration test for QueryInterface.bulkInsert.

Changes

Cohort / File(s) Summary
MySQL insert formatting
packages/mysql/src/query.js
Add convertInsertId and generateInsertResults; replace per-iteration ID construction with range-based generation to return an array of { <pkName>: value } for bulk inserts (with or without model), preserving original id types (Number, String, BigInt).
MariaDB insert formatting
packages/mariadb/src/query.js
Change no-model bulk insert path to coerce startId from data[this.getInsertIdField()] and build an array [{ id: startId }, ...] for affectedRows, returning the array instead of a single insertId.
Unit tests (MySQL)
packages/core/test/unit/dialects/mysql/query.test.js
Initialize query.sql in logWarnings test; add formatResults tests covering model-context bulk, no-model bulk, single-row, zero-rows, string insertId, and BigInt insertId, asserting returned arrays and preserved id types.
Integration tests (QueryInterface.bulkInsert)
packages/core/test/integration/query-interface/bulk-insert.test.ts
New integration suite creating/dropping UsersBulkInsert; tests bulkInsert(..., { returning: true }) expectations across dialects (MySQL/MariaDB -> IDs-only array; others -> full rows) and verifies single-row returns array.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant App
  participant QI as QueryInterface
  participant Conn as Dialect Connection
  participant Q as DialectQuery.formatResults

  App->>QI: bulkInsert(table, rows, { returning: true })
  QI->>Conn: Execute INSERT (bulk)
  Conn-->>QI: ResultSetHeader { insertId, affectedRows, ... }
  QI->>Q: formatResults(ResultSetHeader, options)
  alt model context present
    Q-->>QI: Array of inserted row objects (model-mapped/full rows)
  else no model context
    alt affectedRows > 1
      Q-->>QI: Array of `{ id: startId + n }` for n = 0..affectedRows-1
    else affectedRows == 1
      Q-->>QI: Array with single `{ id: startId }`
    else affectedRows == 0
      Q-->>QI: Empty array
    end
  end
  QI-->>App: Return dialect-specific result

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

Thump — I counted startId in the moonlight glow,
Each hop became a little id arranged in row.
Numbers, strings, BigInts kept tidy and neat,
From one to many, every id found its seat.
Hooray — bulk inserts lined up, all complete! 🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Out of Scope Changes Check ⚠️ Warning This PR extends changes to the MariaDB dialect’s formatResults implementation and alters unrelated unit tests for logWarnings and integration tests for non-MySQL dialects, which exceed the original scope of fixing MySQL bulkInsert return values in issue #14325. These modifications are not directly required to meet the linked issue’s objective and introduce additional surface area beyond the targeted dialect and functionality. Consolidating unrelated changes risks conflating separate concerns and complicates the review and testing of the core MySQL fix. Separate the MariaDB dialect adjustments and unrelated logWarnings or non-MySQL integration test changes into distinct PRs or issues, focusing this PR solely on the MySQL bulkInsert return behavior to align with issue #14325.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly and accurately summarizes the primary change, which is updating bulkInsert in core to return an array of inserted IDs. It follows the conventional “fix(scope): description” style and conveys the main intent clearly without extraneous detail.
Linked Issues Check ✅ Passed The PR modifies MySQL dialect’s formatResults to build and return an array of inserted ID objects for multi-row bulkInsert operations as specified in issue [#14325], and adds corresponding unit and integration tests to verify array returns across scenarios. Tests cover various insertId types and affectedRows values, demonstrating that bulkInsert now aligns with the expected behavior of returning an array of new record IDs. Therefore, the supplied changes satisfy the linked issue’s primary requirement.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • [ ] 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Oct 01 '25 13:10 coderabbitai[bot]

Please adjust the tests and probably add new ones. It could be that we have tests that only check for certain dialects, you'll need to update those expectations as well to include mysql

WikiRik avatar Oct 01 '25 14:10 WikiRik

Hi , it looks like the postgres oldest (Node 18) CI job failed due to a network issue while downloading dependencies (connection reset by peer). All other CI checks have passed . Could someone please retry this job when convenient? Thanks!

youssef2272002salah avatar Oct 02 '25 19:10 youssef2272002salah

Hi @WikiRik I’ve addressed the feedback and updated the tests. CI checks are passed except one falls due to netwok issue. Could you please take another look when you have time? Thanks a lot for your guidance

youssef2272002salah avatar Oct 02 '25 19:10 youssef2272002salah

Hi @WikiRik @sdepold , just checking in — is there anything blocking this PR or anything I should update? Thanks a lot for your time!

youssef2272002salah avatar Nov 21 '25 09:11 youssef2272002salah