fix(mysql): Remove DEFAULT coersion for PK on insert statements for MySQL & MariaDB.
#18030
Conversation
…` records. - converts INSERT INTO `table` (`id`,`name`) VALUES (DEFAULT,?); to INSERT INTO `table` (`name`) values (?); - This ensures that MySQL/MariaDB will always return an ID.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds dialect-specific insertQuery overrides for MySQL and MariaDB to omit null/undefined auto-increment columns, plus unit tests for query generation and integration tests verifying Model.create() returns auto-increment IDs in replication (useMaster) scenarios. Also removes several package CHANGELOG.md contents. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Model as Model.create()
participant QG as QueryGenerator.insertQuery()
participant DB as Database
App->>Model: create({ name: "Test", id: null }, { useMaster: true })
Model->>QG: insertQuery(table, {id: null, name}, modelAttributes)
Note right of QG: NEW — sanitize valueHash\nremove autoIncrement keys with null/undefined
QG->>QG: sanitized = { name: "Test" }
QG->>DB: INSERT INTO table (name) VALUES (?)
DB-->>QG: OK + lastInsertId
QG-->>Model: query result with lastInsertId
Model-->>App: instance with id populated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue with MySQL and MariaDB where auto-incrementing primary keys were not being properly returned when using read/write replicas. The fix removes DEFAULT coercion for auto-increment columns during insert operations by filtering out null/undefined values for these columns before generating the SQL query.
Key changes:
- Overrides the
insertQuerymethod in both MySQL and MariaDB query generators to sanitize value hashes - Adds unit tests to verify that auto-increment columns with null values are omitted from insert queries
- Includes integration tests for replication scenarios to ensure auto-increment PKs are properly populated
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/mysql/src/query-generator.js | Adds insertQuery override to filter out null/undefined auto-increment columns |
| packages/mariadb/src/query-generator.js | Adds identical insertQuery override for MariaDB dialect |
| packages/core/test/unit/dialects/mysql/query-generator.test.js | Adds unit test for auto-increment column filtering behavior |
| packages/core/test/unit/dialects/mariadb/query-generator.test.js | Adds unit test for auto-increment column filtering behavior |
| packages/core/test/integration/dialects/mysql/replication.test.ts | Adds integration test for replication scenario with auto-increment PK |
| packages/core/test/integration/dialects/mariadb/replication.test.ts | Adds integration test for replication scenario with auto-increment PK |
| const typeWithoutDefault = new Set(['BLOB', 'TEXT', 'GEOMETRY', 'JSON']); | ||
|
|
||
| export class MySqlQueryGenerator extends MySqlQueryGeneratorTypeScript { | ||
| insertQuery(table, valueHash, modelAttributes, options) { | ||
| const sanitizedValueHash = | ||
| valueHash && typeof valueHash === 'object' && !Array.isArray(valueHash) | ||
| ? { ...valueHash } | ||
| : valueHash; | ||
|
|
||
| if (sanitizedValueHash && typeof sanitizedValueHash === 'object' && modelAttributes) { |
There was a problem hiding this comment.
The type checking logic is inconsistent between MySQL and MariaDB implementations. MySQL checks typeof valueHash === 'object' twice while MariaDB omits the second check. Consider extracting this logic into a helper function for consistency.
| const typeWithoutDefault = new Set(['BLOB', 'TEXT', 'GEOMETRY', 'JSON']); | |
| export class MySqlQueryGenerator extends MySqlQueryGeneratorTypeScript { | |
| insertQuery(table, valueHash, modelAttributes, options) { | |
| const sanitizedValueHash = | |
| valueHash && typeof valueHash === 'object' && !Array.isArray(valueHash) | |
| ? { ...valueHash } | |
| : valueHash; | |
| if (sanitizedValueHash && typeof sanitizedValueHash === 'object' && modelAttributes) { | |
| function isObjectButNotArray(value) { | |
| return value && typeof value === 'object' && !Array.isArray(value); | |
| } | |
| const typeWithoutDefault = new Set(['BLOB', 'TEXT', 'GEOMETRY', 'JSON']); | |
| insertQuery(table, valueHash, modelAttributes, options) { | |
| const sanitizedValueHash = | |
| isObjectButNotArray(valueHash) | |
| ? { ...valueHash } | |
| : valueHash; | |
| if (isObjectButNotArray(sanitizedValueHash) && modelAttributes) { |
| function getConnectionOptions(): ConnectionOptions<AbstractDialect> { | ||
| return pick( | ||
| CONFIG[dialectName], | ||
| baseSequelize.dialect.getSupportedConnectionOptions(), | ||
| ); | ||
| } | ||
|
|
||
| const sandbox = sinon.createSandbox(); | ||
| const sequelize = createSequelizeInstance({ | ||
| replication: { | ||
| write: getConnectionOptions(), | ||
| read: [getConnectionOptions()], | ||
| }, | ||
| }); | ||
|
|
||
| destroySequelizeAfterTest(sequelize); | ||
|
|
||
| const poolAcquireSpy = sandbox.spy(sequelize.pool, 'acquire'); | ||
|
|
||
| const User = sequelize.define( | ||
| 'User', | ||
| { | ||
| firstName: { | ||
| type: DataTypes.STRING, | ||
| }, | ||
| }, | ||
| { timestamps: false }, | ||
| ); | ||
|
|
||
| await User.sync({ force: true }); | ||
| poolAcquireSpy.resetHistory(); | ||
|
|
||
| return { | ||
| User, | ||
| sequelize, | ||
| sandbox, | ||
| poolAcquireSpy, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
The indentation is inconsistent throughout this block. The code inside the beforeEach2 function should be consistently indented with 2 or 4 spaces.
| function getConnectionOptions(): ConnectionOptions<AbstractDialect> { | |
| return pick( | |
| CONFIG[dialectName], | |
| baseSequelize.dialect.getSupportedConnectionOptions(), | |
| ); | |
| } | |
| const sandbox = sinon.createSandbox(); | |
| const sequelize = createSequelizeInstance({ | |
| replication: { | |
| write: getConnectionOptions(), | |
| read: [getConnectionOptions()], | |
| }, | |
| }); | |
| destroySequelizeAfterTest(sequelize); | |
| const poolAcquireSpy = sandbox.spy(sequelize.pool, 'acquire'); | |
| const User = sequelize.define( | |
| 'User', | |
| { | |
| firstName: { | |
| type: DataTypes.STRING, | |
| }, | |
| }, | |
| { timestamps: false }, | |
| ); | |
| await User.sync({ force: true }); | |
| poolAcquireSpy.resetHistory(); | |
| return { | |
| User, | |
| sequelize, | |
| sandbox, | |
| poolAcquireSpy, | |
| }; | |
| }); | |
| function getConnectionOptions(): ConnectionOptions<AbstractDialect> { | |
| return pick( | |
| CONFIG[dialectName], | |
| baseSequelize.dialect.getSupportedConnectionOptions(), | |
| ); | |
| } | |
| const sandbox = sinon.createSandbox(); | |
| const sequelize = createSequelizeInstance({ | |
| replication: { | |
| write: getConnectionOptions(), | |
| read: [getConnectionOptions()], | |
| }, | |
| }); | |
| destroySequelizeAfterTest(sequelize); | |
| const poolAcquireSpy = sandbox.spy(sequelize.pool, 'acquire'); | |
| const User = sequelize.define( | |
| 'User', | |
| { | |
| firstName: { | |
| type: DataTypes.STRING, | |
| }, | |
| }, | |
| { timestamps: false }, | |
| ); | |
| await User.sync({ force: true }); | |
| poolAcquireSpy.resetHistory(); | |
| return { | |
| User, | |
| sequelize, | |
| sandbox, | |
| poolAcquireSpy, | |
| }; | |
| }); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/mariadb/src/query-generator.js (1)
16-45: Consider aligning the valueHash type check with MySQL implementation.The MariaDB implementation omits the
typeof valueHash === 'object'check on line 18, while the MySQL version includes it (line 23 in packages/mysql/src/query-generator.js). For consistency and explicitness across dialects, consider adding the same check.Apply this diff to align with the MySQL implementation:
insertQuery(table, valueHash, modelAttributes, options) { const sanitizedValueHash = - valueHash && !Array.isArray(valueHash) + valueHash && typeof valueHash === 'object' && !Array.isArray(valueHash) ? { ...valueHash } : valueHash;packages/core/test/integration/dialects/mysql/replication.test.ts (1)
23-62: Consider asserting that the master connection is actually used.The test creates
poolAcquireSpy(line 41) to monitor pool.acquire calls, but it's never used in the test assertion. To strengthen the test and verify thatuseMaster: trueactually routes to the master connection, consider adding an assertion.Add an assertion in the test (after line 73) to verify the spy was called with the expected connection type:
// After line 73, before line 75 expect(deps.poolAcquireSpy.called).to.be.true; // Optionally verify it was called with 'write' pool const acquireCall = deps.poolAcquireSpy.getCall(0); expect(acquireCall.args[0]).to.equal('write');packages/core/test/integration/dialects/mariadb/replication.test.ts (1)
68-76: Consider asserting that the master connection is used.Similar to the MySQL test, the
poolAcquireSpyis created but never asserted. Adding an assertion would verify thatuseMaster: truecorrectly routes to the master connection.Add an assertion after line 73 to verify the spy was called with the write pool:
expect(deps.poolAcquireSpy.called).to.be.true; const acquireCall = deps.poolAcquireSpy.getCall(0); expect(acquireCall.args[0]).to.equal('write');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/core/test/integration/dialects/mariadb/replication.test.ts(1 hunks)packages/core/test/integration/dialects/mysql/replication.test.ts(1 hunks)packages/core/test/unit/dialects/mariadb/query-generator.test.js(1 hunks)packages/core/test/unit/dialects/mysql/query-generator.test.js(1 hunks)packages/mariadb/src/query-generator.js(1 hunks)packages/mysql/src/query-generator.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
packages/*/src/query-generator.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Implement dialect feature SQL in query-generator.ts (preferred) or legacy query-generator.js
Files:
packages/mariadb/src/query-generator.jspackages/mysql/src/query-generator.js
packages/**/src/**/*.js
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Write all new implementations in TypeScript; avoid creating new .js files in src
Files:
packages/mariadb/src/query-generator.jspackages/mysql/src/query-generator.js
packages/**/src/**/query-generator.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
When modifying query generation, update both the base query generator and all dialect-specific implementations
Files:
packages/mariadb/src/query-generator.jspackages/mysql/src/query-generator.js
packages/**/test/**/*.test.js
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Name integration tests with the .test.js suffix
Files:
packages/core/test/unit/dialects/mariadb/query-generator.test.jspackages/core/test/unit/dialects/mysql/query-generator.test.js
packages/**/test/**/*.test.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/**/test/**/*.test.{js,ts}: Use Support.getTestDialectTeaser() for dialect-specific test descriptions
Use beforeEach/afterEach to sync with { force: true } and close Sequelize instances in tests
In tests, skip unsupported behavior with dialect.supports.featureName checks
Files:
packages/core/test/unit/dialects/mariadb/query-generator.test.jspackages/core/test/unit/dialects/mysql/query-generator.test.jspackages/core/test/integration/dialects/mysql/replication.test.tspackages/core/test/integration/dialects/mariadb/replication.test.ts
packages/**/test/**/*.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Name unit tests with the .test.ts suffix
Files:
packages/core/test/integration/dialects/mysql/replication.test.tspackages/core/test/integration/dialects/mariadb/replication.test.ts
🧠 Learnings (2)
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
PR: sequelize/sequelize#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-29T19:49:24.715Z
Learning: Applies to packages/**/test/**/*.test.{js,ts} : Use beforeEach/afterEach to sync with { force: true } and close Sequelize instances in tests
Applied to files:
packages/core/test/integration/dialects/mysql/replication.test.tspackages/core/test/integration/dialects/mariadb/replication.test.ts
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
PR: sequelize/sequelize#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-29T19:49:24.715Z
Learning: Applies to packages/**/test/**/*.test.{js,ts} : Use Support.getTestDialectTeaser() for dialect-specific test descriptions
Applied to files:
packages/core/test/integration/dialects/mariadb/replication.test.ts
🧬 Code graph analysis (2)
packages/core/test/integration/dialects/mysql/replication.test.ts (3)
packages/core/test/support.ts (3)
getTestDialect(164-180)getTestDialectTeaser(182-190)beforeEach2(553-564)packages/core/test/integration/support.ts (2)
setResetMode(184-197)destroySequelizeAfterTest(92-94)packages/core/test/config/config.ts (1)
CONFIG(45-157)
packages/core/test/integration/dialects/mariadb/replication.test.ts (3)
packages/core/test/support.ts (3)
getTestDialect(164-180)getTestDialectTeaser(182-190)beforeEach2(553-564)packages/core/test/integration/support.ts (2)
setResetMode(184-197)destroySequelizeAfterTest(92-94)packages/core/test/config/config.ts (1)
CONFIG(45-157)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Upload install and build artifact (Node 20)
- GitHub Check: Upload install and build artifact (Node 18)
🔇 Additional comments (3)
packages/core/test/unit/dialects/mariadb/query-generator.test.js (1)
332-339: LGTM!The test case correctly verifies that auto-increment columns with null values are omitted from INSERT statements. The test structure and assertions align with existing test patterns in the suite.
packages/core/test/unit/dialects/mysql/query-generator.test.js (1)
327-334: LGTM!The test case properly validates that auto-increment columns with null values are omitted from MySQL INSERT statements. The test mirrors the MariaDB equivalent and maintains consistency across dialect test suites.
packages/core/test/integration/dialects/mysql/replication.test.ts (1)
68-76: LGTM!The test correctly verifies that creating a User with
useMaster: truein a replication setup returns an instance with a populated auto-increment id. This addresses the core issue from #17909 where auto-increment PKs were undefined in replication scenarios.
packages/core/test/integration/dialects/mariadb/replication.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CHANGELOG.md(0 hunks)packages/core/CHANGELOG.md(0 hunks)packages/core/test/integration/dialects/mariadb/replication.test.ts(1 hunks)packages/core/test/integration/dialects/mysql/replication.test.ts(1 hunks)packages/core/test/unit/dialects/mariadb/query-generator.test.js(1 hunks)packages/core/test/unit/dialects/mysql/query-generator.test.js(1 hunks)packages/mariadb/src/query-generator.js(1 hunks)packages/utils/CHANGELOG.md(0 hunks)packages/validator-js/CHANGELOG.md(0 hunks)
💤 Files with no reviewable changes (4)
- packages/validator-js/CHANGELOG.md
- packages/utils/CHANGELOG.md
- CHANGELOG.md
- packages/core/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/mariadb/src/query-generator.js
- packages/core/test/integration/dialects/mysql/replication.test.ts
- packages/core/test/unit/dialects/mariadb/query-generator.test.js
🧰 Additional context used
📓 Path-based instructions (3)
packages/**/test/**/*.test.js
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Name integration tests with the .test.js suffix
Files:
packages/core/test/unit/dialects/mysql/query-generator.test.js
packages/**/test/**/*.test.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/**/test/**/*.test.{js,ts}: Use Support.getTestDialectTeaser() for dialect-specific test descriptions
Use beforeEach/afterEach to sync with { force: true } and close Sequelize instances in tests
In tests, skip unsupported behavior with dialect.supports.featureName checks
Files:
packages/core/test/unit/dialects/mysql/query-generator.test.jspackages/core/test/integration/dialects/mariadb/replication.test.ts
packages/**/test/**/*.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Name unit tests with the .test.ts suffix
Files:
packages/core/test/integration/dialects/mariadb/replication.test.ts
🧠 Learnings (2)
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
PR: sequelize/sequelize#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-29T19:49:24.715Z
Learning: Applies to packages/**/test/**/*.test.{js,ts} : Use beforeEach/afterEach to sync with { force: true } and close Sequelize instances in tests
Applied to files:
packages/core/test/integration/dialects/mariadb/replication.test.ts
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
PR: sequelize/sequelize#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-29T19:49:24.715Z
Learning: Applies to packages/**/test/**/*.test.{js,ts} : Use Support.getTestDialectTeaser() for dialect-specific test descriptions
Applied to files:
packages/core/test/integration/dialects/mariadb/replication.test.ts
🧬 Code graph analysis (1)
packages/core/test/integration/dialects/mariadb/replication.test.ts (2)
packages/core/test/support.ts (3)
getTestDialect(164-180)getTestDialectTeaser(182-190)beforeEach2(553-564)packages/core/test/integration/support.ts (2)
setResetMode(184-197)destroySequelizeAfterTest(92-94)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Upload install and build artifact (Node 18)
- GitHub Check: Upload install and build artifact (Node 20)
🔇 Additional comments (1)
packages/core/test/unit/dialects/mysql/query-generator.test.js (1)
327-338: LGTM! Test correctly validates auto-increment column omission.The test case properly verifies that when an auto-increment column has a null value, it is omitted from the generated INSERT statement. This aligns with the PR objectives to fix auto-increment PK handling.
| it('uses the master connection when requested and populates the auto-increment primary key', async () => { | ||
| const instance = await deps.User.create( | ||
| { firstName: `KozyOps` }, | ||
| // @ts-expect-error -- Mysql dialect supports useMaster | ||
| { useMaster: true }, | ||
| ); | ||
|
|
||
| expect(instance.get('id')).to.be.a('number'); | ||
| }); |
There was a problem hiding this comment.
Test doesn't verify master connection usage despite claiming to.
The test description states "uses the master connection when requested" but the test only verifies that the auto-increment ID is populated. The poolAcquireSpy created on line 38 is never asserted against, so the test doesn't actually confirm that the master connection was used.
Add an assertion to verify master connection usage:
expect(instance.get('id')).to.be.a('number');
+
+ // Verify that the master (write) connection was used
+ expect(deps.poolAcquireSpy.calledOnce).to.be.true;
+ expect(deps.poolAcquireSpy.firstCall.args[0]).to.deep.include({ type: 'write' });
});Alternatively, if the primary goal is only to verify auto-increment ID population, consider updating the test description to match:
- it('uses the master connection when requested and populates the auto-increment primary key', async () => {
+ it('populates the auto-increment primary key when using useMaster', async () => {🤖 Prompt for AI Agents
packages/core/test/integration/dialects/mariadb/replication.test.ts around lines
65 to 73: the test title claims it "uses the master connection when requested"
but never asserts the poolAcquireSpy created earlier, so it only validates
auto-increment ID population; add an assertion after creating the instance to
verify the poolAcquireSpy was invoked for the master connection (or update the
test description to remove the master-connection claim if you only want to
assert ID population).
| it('uses the master connection when requested and populates the auto-increment primary key', async () => { | ||
| const instance = await deps.User.create( | ||
| { firstName: `KozyOps` }, | ||
| // @ts-expect-error -- Mysql dialect supports useMaster |
There was a problem hiding this comment.
Fix comment to reference MariaDB instead of MySQL.
The comment states "Mysql dialect supports useMaster" but this is a MariaDB-specific test file.
Apply this diff:
- // @ts-expect-error -- Mysql dialect supports useMaster
+ // @ts-expect-error -- MariaDB dialect supports useMaster📝 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.
| // @ts-expect-error -- Mysql dialect supports useMaster | |
| // @ts-expect-error -- MariaDB dialect supports useMaster |
🤖 Prompt for AI Agents
In packages/core/test/integration/dialects/mariadb/replication.test.ts around
line 68, the inline comment incorrectly references "Mysql dialect supports
useMaster"; update the comment to reference "MariaDB dialect supports useMaster"
to accurately reflect the test file's dialect and avoid confusion.
1 participant
Pull Request Checklist
Description of Changes
This overrides the default insertQuery method for MariaDB & MySQL to ensure that autoincrementing PKs are always returned when doing an insert, even with working with read/write replicas.
Notes:
I don't think the tests will truly fix regressions as I am unsure about how to create a realistic test environment for this situation. However, The theory around this change is sound, and was tested in a real environment. Creating a more realistic test case may be required.
Original issue was for v6, whereas this fix is for v7, It can be fairly easily backported if someone cares to do so.
List of Breaking Changes
No breaking changes
Closes: #17909
Summary by CodeRabbit
Bug Fixes
Tests
Documentation