{{ message }}
fix: avoid wrong primary keys from bulkCreate with updateOnDuplicate/ignoreDuplicates - #18296
Open
spokodev wants to merge 2 commits into
Open
fix: avoid wrong primary keys from bulkCreate with updateOnDuplicate/ignoreDuplicates#18296spokodev wants to merge 2 commits into
spokodev wants to merge 2 commits into
Conversation
Contributor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/core/test/unit/dialects/mysql/query.test.js (1)
65-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required dialect teaser in the nested suite description.
Replace the raw dialect name with
Support.getTestDialectTeaser(...)for this dialect-specific test group.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/test/unit/dialects/mysql/query.test.js` around lines 65 - 66, Update the nested describe block in the dialects test loop to use Support.getTestDialectTeaser(...) with the current dialect value instead of the raw name, while preserving the existing dialect-specific test grouping.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/test/unit/dialects/mysql/query.test.js`:
- Line 65: Update the destructuring pattern in the dialects loop so insertId
appears before Query, satisfying the required key ordering while preserving all
existing bindings and behavior.
- Around line 47-56: Update the test lifecycle around the model setup in the
before hook: store the dedicated Sequelize instance, create and sync it in an
async beforeEach using force: true, and add an async afterEach that awaits
closing that instance. Keep the formatResultsBulkCreate model definition
associated with the isolated instance.
In `@packages/mysql/src/query.js`:
- Around line 114-121: Move the new formatResults guard from the JavaScript
implementations into their TypeScript counterparts, preserving the
auto-increment primary-key check and updateOnDuplicate/ignoreDuplicates
exclusions. Apply this migration in packages/mysql/src/query.js (lines 114-121)
and packages/mariadb/src/query.js (lines 109-116), removing the added logic from
both JavaScript files.
---
Nitpick comments:
In `@packages/core/test/unit/dialects/mysql/query.test.js`:
- Around line 65-66: Update the nested describe block in the dialects test loop
to use Support.getTestDialectTeaser(...) with the current dialect value instead
of the raw name, while preserving the existing dialect-specific test grouping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f04f122e-34c3-4195-ac67-3d0663d6fec5
📒 Files selected for processing (3)
packages/core/test/unit/dialects/mysql/query.test.jspackages/mariadb/src/query.jspackages/mysql/src/query.js
Comment on lines
+47
to
+56
| before(() => { | ||
| model = Support.createSequelizeInstance().define( | ||
| 'formatResultsBulkCreate', | ||
| { | ||
| id: { type: DataTypes.INTEGER, primaryKey: true, autoIncrement: true }, | ||
| name: DataTypes.STRING, | ||
| }, | ||
| { timestamps: false }, | ||
| ); | ||
| }); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use isolated test lifecycle hooks for the dedicated Sequelize instance.
Store the instance, create/sync it in beforeEach with { force: true }, and await its closure in afterEach. The current before hook neither syncs nor closes the instance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/test/unit/dialects/mysql/query.test.js` around lines 47 - 56,
Update the test lifecycle around the model setup in the before hook: store the
dedicated Sequelize instance, create and sync it in an async beforeEach using
force: true, and add an async afterEach that awaits closing that instance. Keep
the formatResultsBulkCreate model definition associated with the isolated
instance.
Source: Coding guidelines
Comment on lines
+114
to
+121
| modelDefinition?.autoIncrementAttributeName === this.model.primaryKeyAttribute && | ||
| // `updateOnDuplicate`/`ignoreDuplicates` break the `insertId` + `affectedRows` | ||
| // arithmetic below: MySQL counts an updated row as 2 affected and an ignored one | ||
| // as 0, so the synthesised id range desynchronises from the inserted rows and | ||
| // returned instances get primary keys belonging to other rows. Fall back to the | ||
| // raw insertId in that case rather than fabricating a wrong range. See #18281. | ||
| !this.options.updateOnDuplicate && | ||
| !this.options.ignoreDuplicates |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Move the new dialect logic into TypeScript source. Both changes add implementation code under packages/**/src/**/*.js, contrary to the repository rule.
packages/mysql/src/query.js#L114-L121: migrate this newformatResultsguard to the TypeScript implementation.packages/mariadb/src/query.js#L109-L116: migrate the matching guard to the TypeScript implementation.
📍 Affects 2 files
packages/mysql/src/query.js#L114-L121(this comment)packages/mariadb/src/query.js#L109-L116
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/mysql/src/query.js` around lines 114 - 121, Move the new
formatResults guard from the JavaScript implementations into their TypeScript
counterparts, preserving the auto-increment primary-key check and
updateOnDuplicate/ignoreDuplicates exclusions. Apply this migration in
packages/mysql/src/query.js (lines 114-121) and packages/mariadb/src/query.js
(lines 109-116), removing the added logic from both JavaScript files.
Source: Coding guidelines
spokodev
force-pushed
the
fix/bulkcreate-upsert-pk
branch
from
July 30, 2026 19:47
aaa8b85 to
8e3fc1a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/test/unit/dialects/mysql/query.test.js`:
- Around line 65-66: Update the nested suite declaration inside the dialects
loop to use Support.getTestDialectTeaser() instead of passing name directly to
describe. Preserve the existing dialect-specific test structure and callback
contents.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 077b8dc9-88d1-4528-8b56-ab9d833c3d67
📒 Files selected for processing (3)
packages/core/test/unit/dialects/mysql/query.test.jspackages/mariadb/src/query.jspackages/mysql/src/query.js
Comment on lines
+65
to
+66
| for (const { id, insertId, name, Query } of dialects) { | ||
| describe(name, () => { |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the dialect teaser for the nested suites. describe(name, ...) bypasses the required dialect-specific description helper.
Proposed fix
- describe(name, () => {
+ describe(Support.getTestDialectTeaser(name), () => {As per coding guidelines, “Use Support.getTestDialectTeaser() for dialect-specific test descriptions.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/test/unit/dialects/mysql/query.test.js` around lines 65 - 66,
Update the nested suite declaration inside the dialects loop to use
Support.getTestDialectTeaser() instead of passing name directly to describe.
Preserve the existing dialect-specific test structure and callback contents.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Checklist
Description of Changes
Addresses #18281.
On MySQL and MariaDB,
bulkCreate()synthesises the primary keys of the returned instances frominsertId+affectedRows(packages/mysql/src/query.js,packages/mariadb/src/query.js). Thatarithmetic assumes exactly one inserted row per submitted record.
updateOnDuplicateandignoreDuplicatesbreak the assumption: MySQL reportsaffectedRowsas 2 perupdated row and 0 per ignored row, and
LAST_INSERT_ID()is only the first generated id, so thesynthesised id range desynchronises from the instance array. Returned instances (which come back with
isNewRecord === false) then carry primary keys belonging to other rows, including rows outsidethe batch. A later
instance.destroy()/instance.update()silently hits the wrong row (the issuedocuments a
destroy()deleting a different record and anupdate()overwriting a pre-existing rowoutside the batch).
Fix: when
updateOnDuplicateorignoreDuplicatesis set,formatResultsno longer fabricatesthe contiguous id range; it returns the raw
insertIdinstead, so the wrong-key path inmodel.js(
if (Array.isArray(results))) is skipped and no instance is given a fabricated key. Returning nosynthesised key is correct here: with the weighted
affectedRowsthe real per-row keys cannot bederived from the result header without a re-
SELECT, and a missing key is safe where a wrong onecorrupts data. The plain
bulkCreatepath (noupdateOnDuplicate/ignoreDuplicates) is unchanged:every row goes through
AUTO_INCREMENT, so the range stays contiguous and index-aligned.Tests (
packages/core/test/unit/dialects/mysql/query.test.js, no DB): using a mockResultSetHeader,both
MySqlQueryandMariaDbQuerystill synthesise the contiguous range for a plain bulkCreate, andreturn the raw
insertId(no fabricated range) underupdateOnDuplicate/ignoreDuplicates. Failingbefore the change, passing after; the full
coreunit suite stays green (2269 passing).Related items from the issue (not in this PR)
The issue also lists two companion items. I kept this PR focused on the data-integrity bug and am
happy to fold either in here or handle as a follow-up, whichever you prefer:
Capability-based carve-out. The block in
packages/core/src/model.jsthat skips replacing atruthy primary key keys off the dialect allowlist
['mysql', 'mariadb']. Sincesupports.returnValuesdefaults tofalse, db2/ibmi/snowflake inherit it and oracle sets itexplicitly, yet they are not in the list. Deriving the predicate from
!this.sequelize.dialect.supports.returnValuescovers all of them and is a no-op for mysql/mariadb(already
returnValues: false):updateOnDuplicate+ primary key footgun. Listing the primary key inupdateOnDuplicatebutnot in
fieldsemits`id`=VALUES(`id`)and reassigns a matched row's primary key. Worth avalidation or documentation note; separate from this fix.
Caveat on scope
The contiguous-range assumption in the plain path is only verified at
auto_increment_increment = 1;the existing comment in
packages/mariadb/src/query.jsnotes it does not hold under Galera or a non-1increment. This PR does not change that path, so it neither fixes nor worsens that case.
List of Breaking Changes
bulkCreate(..., { updateOnDuplicate })/{ ignoreDuplicates }on MySQL/MariaDB previously returnedinstances with (often wrong) primary keys; they now come back without a synthesised primary key. This
removes silent wrong-row writes at the cost of not returning generated keys for these two options:
callers who need the keys should re-query by a unique column.
Summary by CodeRabbit