fix(postgres): prevent duplicate unique constraints during sync({ alt… #18044
Conversation
WalkthroughPropagates existing unique-index metadata through sync and SQL generation so inline UNIQUE clauses are suppressed when an equivalent unique index already exists, preventing duplicate UNIQUE constraints during alter sync operations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Sync as model.sync({ alter: true })
participant Model as Model (core/src/model.js)
participant QI as QueryInterface.changeColumn
participant PG as Postgres QueryGenerator
participant DB as Database
Sync->>Model: run alter flow for changed attributes
Model->>Model: modelDefinition.getIndexes()\nfilter unique indexes → build uniqueKeys
Model->>QI: changeColumn(..., options + uniqueKeys)
QI->>PG: attributesToSQL(context including uniqueKeys)
PG->>PG: normalize field descriptor\ncheck options.uniqueKeys for matching unique index
alt matching unique index exists
PG-->>QI: emit column SQL WITHOUT inline UNIQUE
else no matching unique index
PG-->>QI: emit column SQL WITH inline UNIQUE
end
QI->>DB: execute ALTER TABLE / CREATE INDEX as needed
DB-->>Sync: complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/core/test/integration/query-interface/changeColumn.test.js (3)
273-302: Follow coding guidelines for test structure.The test correctly validates the fix for duplicate unique constraints. However, per coding guidelines, tests should use
beforeEach/afterEachto sync with{ force: true }and close Sequelize instances.Consider restructuring to follow the established pattern:
if (dialect === 'postgres' || dialect === 'postgres-native') { describe('unique constraint duplication fix', () => { + beforeEach(async function () { + this.User = this.sequelize.define('UserUniqueTest', { + email: { + type: DataTypes.STRING, + unique: true, + }, + }); + await this.sequelize.sync({ force: true }); + }); + it('should not create duplicate unique constraints on repeated alter syncs', async function () { - const User = this.sequelize.define('UserUniqueTest', { - email: { - type: DataTypes.STRING, - unique: true, - }, - }); - - await this.sequelize.sync({ force: true }); - await this.sequelize.sync({ alter: true }); await this.sequelize.sync({ alter: true });Based on coding guidelines.
296-296: Consider more robust constraint verification.Filtering constraints by
constraint_name.includes('email')may be fragile if Postgres changes its naming convention or if other constraints include "email" in their names.Consider querying for constraints on the specific column:
- const [results] = await this.sequelize.query(` - SELECT constraint_name - FROM information_schema.table_constraints - WHERE table_name = 'UserUniqueTests' - AND constraint_type = 'UNIQUE'; - `); - - // Filter only email-related constraints - const uniqueConstraints = results.filter(r => r.constraint_name.includes('email')); + const [results] = await this.sequelize.query(` + SELECT tc.constraint_name + FROM information_schema.table_constraints tc + JOIN information_schema.constraint_column_usage ccu + ON tc.constraint_name = ccu.constraint_name + WHERE tc.table_name = 'UserUniqueTests' + AND tc.constraint_type = 'UNIQUE' + AND ccu.column_name = 'email'; + `);This joins with
constraint_column_usageto precisely identify constraints on the
587-587: Remove unrelated formatting change.This blank line addition appears unrelated to the unique constraint fix and should be removed to keep the PR focused.
- }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/abstract-dialect/query-interface.js(2 hunks)packages/core/test/integration/query-interface/changeColumn.test.js(2 hunks)packages/postgres/src/query-generator.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
packages/**/test/**/*.test.js
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Name integration tests with the .test.js suffix
Files:
packages/core/test/integration/query-interface/changeColumn.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/integration/query-interface/changeColumn.test.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/core/src/abstract-dialect/query-interface.jspackages/postgres/src/query-generator.js
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/postgres/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/postgres/src/query-generator.js
🧠 Learnings (6)
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-29T19:49:24.715Z
Learning: Applies to packages/*/src/query-interface.{js,ts} : Add schema operations for new features in query-interface.ts (preferred) or legacy query-interface.js
Applied to files:
packages/core/test/integration/query-interface/changeColumn.test.jspackages/core/src/abstract-dialect/query-interface.jspackages/postgres/src/query-generator.js
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 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/query-interface/changeColumn.test.js
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-29T19:49:24.715Z
Learning: Applies to packages/**/src/**/query-generator.{js,ts} : When modifying query generation, update both the base query generator and all dialect-specific implementations
Applied to files:
packages/core/test/integration/query-interface/changeColumn.test.jspackages/core/src/abstract-dialect/query-interface.jspackages/postgres/src/query-generator.js
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-29T19:49:24.715Z
Learning: Applies to packages/**/test/**/*.test.{js,ts} : In tests, skip unsupported behavior with dialect.supports.featureName checks
Applied to files:
packages/core/test/integration/query-interface/changeColumn.test.js
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 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/query-interface/changeColumn.test.js
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-29T19:49:24.715Z
Learning: Applies to packages/*/src/query-generator.{js,ts} : Implement dialect feature SQL in query-generator.ts (preferred) or legacy query-generator.js
Applied to files:
packages/core/test/integration/query-interface/changeColumn.test.jspackages/core/src/abstract-dialect/query-interface.jspackages/postgres/src/query-generator.js
🧬 Code graph analysis (2)
packages/core/test/integration/query-interface/changeColumn.test.js (1)
packages/core/test/integration/query-interface/createTable.test.js (2)
dialect(9-9)expect(5-5)
packages/core/src/abstract-dialect/query-interface.js (1)
packages/core/src/model-typescript.ts (1)
tableName(164-168)
🪛 ESLint
packages/core/test/integration/query-interface/changeColumn.test.js
[error] 276-276: 'User' is assigned a value but never used. Allowed unused vars must match /^ignore/u.
(no-unused-vars)
⏰ 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 (2)
packages/core/src/abstract-dialect/query-interface.js (1)
220-220: LGTM: Propagates uniqueKeys to query generator.The change correctly passes
model?.uniqueKeysto the query generator options, enabling the generator to avoid duplicate UNIQUE constraints. The optional chaining ensures graceful handling when no model is found.packages/core/test/integration/query-interface/changeColumn.test.js (1)
276-276: ESLint unused variable warning is a false positive.The
Uservariable is used implicitly by thesequelize.sync()operations, which process all registered models including this one. The test verifies the table'UserUniqueTests'created by this model definition.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/abstract-dialect/query-interface.js(2 hunks)packages/core/test/integration/query-interface/changeColumn.test.js(1 hunks)packages/postgres/src/query-generator.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/abstract-dialect/query-interface.js
🧰 Additional context used
📓 Path-based instructions (5)
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/postgres/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/postgres/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/postgres/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/integration/query-interface/changeColumn.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/integration/query-interface/changeColumn.test.js
🧠 Learnings (6)
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-29T19:49:24.715Z
Learning: Applies to packages/**/src/**/query-generator.{js,ts} : When modifying query generation, update both the base query generator and all dialect-specific implementations
Applied to files:
packages/postgres/src/query-generator.js
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-29T19:49:24.715Z
Learning: Applies to packages/*/src/query-generator.{js,ts} : Implement dialect feature SQL in query-generator.ts (preferred) or legacy query-generator.js
Applied to files:
packages/postgres/src/query-generator.jspackages/core/test/integration/query-interface/changeColumn.test.js
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-29T19:49:24.715Z
Learning: Applies to packages/*/src/query-interface.{js,ts} : Add schema operations for new features in query-interface.ts (preferred) or legacy query-interface.js
Applied to files:
packages/postgres/src/query-generator.js
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 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/query-interface/changeColumn.test.js
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-29T19:49:24.715Z
Learning: Applies to packages/**/test/**/*.test.{js,ts} : In tests, skip unsupported behavior with dialect.supports.featureName checks
Applied to files:
packages/core/test/integration/query-interface/changeColumn.test.js
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 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/query-interface/changeColumn.test.js
🧬 Code graph analysis (1)
packages/core/test/integration/query-interface/changeColumn.test.js (2)
packages/core/test/integration/query-interface/createTable.test.js (2)
dialect(9-9)expect(5-5)packages/core/src/index.mjs (2)
DataTypes(91-91)DataTypes(91-91)
⏰ 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 (1)
packages/core/test/integration/query-interface/changeColumn.test.js (1)
273-300: Great regression coverage on duplicate unique constraintsVerifies the repeated
sync({ alter: true })flow end-to-end and asserts only one email constraint remains, which squarely guards the original bug.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/postgres/src/query-generator.js (1)
264-288: Incomplete implementation: Apply to all dialect implementations.This deduplication logic correctly prevents duplicate UNIQUE constraints in PostgreSQL. However, per the coding guidelines and the previous review comment, query generation changes must be applied to all dialect-specific implementations, not just PostgreSQL.
The following dialect query generators still need this same logic:
packages/mysql/src/query-generator.js(around line 208)packages/mssql/src/query-generator.js(around line 512)packages/mariadb/src/query-generator.js(around line 193)packages/ibmi/src/query-generator.js(around line 403)packages/db2/src/query-generator.js(around line 592)packages/snowflake/src/query-generator.js(around line 255)Without this fix in all dialects, users on MySQL, SQL Server, MariaDB, etc. will still encounter duplicate UNIQUE constraints on repeated
sync({ alter: true })calls.Based on coding guidelines and past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/postgres/src/query-generator.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
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/postgres/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/postgres/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/postgres/src/query-generator.js
🧠 Learnings (3)
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-29T19:49:24.715Z
Learning: Applies to packages/**/src/**/query-generator.{js,ts} : When modifying query generation, update both the base query generator and all dialect-specific implementations
Applied to files:
packages/postgres/src/query-generator.js
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-29T19:49:24.715Z
Learning: Applies to packages/*/src/query-generator.{js,ts} : Implement dialect feature SQL in query-generator.ts (preferred) or legacy query-generator.js
Applied to files:
packages/postgres/src/query-generator.js
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-29T19:49:24.715Z
Learning: Applies to packages/*/src/query-interface.{js,ts} : Add schema operations for new features in query-interface.ts (preferred) or legacy query-interface.js
Applied to files:
packages/postgres/src/query-generator.js
⏰ 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). (1)
- GitHub Check: Lint code
|
Hi, it looks like one of the CI runs (MySQL dialect) failed with a connection refused error: SequelizeConnectionRefusedError: connect ECONNREFUSED 127.0.0.1:20057 I’ve reviewed my changes and confirmed they don’t touch any connection or dialect setup code. Could you please re-run the failed job when convenient? |
packages/core/test/integration/query-interface/changeColumn.test.js
Outdated
Show resolved
Hide resolved
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Moved the tests from changeColumn.test.js to model/sync.test.js also used queryInterface instead of raw query simplify unique key comparison using normalizeFieldName helper
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/postgres/src/query-generator.js (1)
264-278: Fix style violations flagged by static analysis.The logic correctly prevents duplicate UNIQUE constraints, but static analysis identifies two style issues:
- Line 267: Arrow function missing curly braces (single-line returns should use concise syntax or explicit braces)
- Line 271: Missing blank line before return statement
Regarding WikiRik's question about checking multiple properties (lines 269): Field descriptors in
uniqueKey.fieldscan be objects with different property names depending on how the index was defined (field,column,attribute, orname). The multiple checks ensure all variants are handled.Apply this diff to address the style issues:
const normalizeFieldName = field => { - if (typeof field === 'string') return field; + if (typeof field === 'string') { + return field; + } + if (field && typeof field === 'object') { return field.field ?? field.column ?? field.attribute ?? field.name; } + return null; };Based on static analysis hints.
packages/core/test/integration/model/sync.test.js (1)
705-719: Add blank line before test for consistency.The test correctly verifies that repeated
sync({ alter: true })doesn't create duplicate UNIQUE constraints and properly usesqueryInterface.showConstraintsas requested in past reviews. However, static analysis flags a missing blank line before the test declaration.Apply this diff to address the style issue:
+ it('should not recreate a UNIQUE constraint if it already exists when { alter: true } is used', async () => {Based on static analysis hints.
packages/core/test/integration/query-interface/changeColumn.test.js (1)
273-278: Remove extra blank line.The SQLite-specific tests for constraint preservation during column changes are appropriately placed in this changeColumn test file. Static analysis flags multiple consecutive blank lines (line 273).
Apply this diff:
- - // sqlite has limited ALTER TABLE capapibilites which requires a workaround involving recreating tables. + // sqlite has limited ALTER TABLE capapibilites which requires a workaround involving recreating tables.Based on static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.yarn/releases/yarn-4.11.0.cjsis excluded by!**/.yarn/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
.github/workflows/ci.yml(10 hunks).yarnrc.yml(1 hunks)dev/db2/latest/docker-compose.yml(1 hunks)package.json(2 hunks)packages/cli/package.json(2 hunks)packages/core/package.json(1 hunks)packages/core/src/abstract-dialect/query-interface.js(4 hunks)packages/core/test/integration/model/sync.test.js(1 hunks)packages/core/test/integration/query-interface/changeColumn.test.js(1 hunks)packages/mariadb/package.json(1 hunks)packages/mssql/package.json(1 hunks)packages/mysql/package.json(1 hunks)packages/postgres/package.json(1 hunks)packages/postgres/src/query-generator.js(1 hunks)packages/utils/package.json(1 hunks)packages/validator-js/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/mysql/package.json
🧰 Additional context used
📓 Path-based instructions (5)
packages/**/test/**/*.test.js
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Name integration tests with the .test.js suffix
Files:
packages/core/test/integration/model/sync.test.jspackages/core/test/integration/query-interface/changeColumn.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/integration/model/sync.test.jspackages/core/test/integration/query-interface/changeColumn.test.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/core/src/abstract-dialect/query-interface.jspackages/postgres/src/query-generator.js
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/postgres/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/postgres/src/query-generator.js
🧠 Learnings (8)
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 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/model/sync.test.jspackages/core/test/integration/query-interface/changeColumn.test.js
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-29T19:49:24.715Z
Learning: Applies to packages/core/test/chai-extensions.d.ts : Use Chai assertions with the project’s custom extensions available in packages/core/test/chai-extensions.d.ts
Applied to files:
packages/utils/package.jsonpackages/mssql/package.jsonpackages/mariadb/package.jsonpackages/postgres/package.jsonpackages/validator-js/package.json
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-29T19:49:24.715Z
Learning: Applies to packages/*/src/query-interface.{js,ts} : Add schema operations for new features in query-interface.ts (preferred) or legacy query-interface.js
Applied to files:
packages/core/src/abstract-dialect/query-interface.jspackages/core/test/integration/query-interface/changeColumn.test.jspackages/postgres/src/query-generator.js
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-29T19:49:24.715Z
Learning: Applies to packages/*/src/query-generator.{js,ts} : Implement dialect feature SQL in query-generator.ts (preferred) or legacy query-generator.js
Applied to files:
packages/core/src/abstract-dialect/query-interface.jspackages/core/test/integration/query-interface/changeColumn.test.jspackages/postgres/src/query-generator.js
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-29T19:49:24.715Z
Learning: Applies to packages/**/src/**/query-generator.{js,ts} : When modifying query generation, update both the base query generator and all dialect-specific implementations
Applied to files:
packages/core/src/abstract-dialect/query-interface.jspackages/core/test/integration/query-interface/changeColumn.test.jspackages/postgres/src/query-generator.js
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-29T19:49:24.715Z
Learning: Applies to packages/**/test/**/*.test.{js,ts} : In tests, skip unsupported behavior with dialect.supports.featureName checks
Applied to files:
packages/core/test/integration/query-interface/changeColumn.test.js
📚 Learning: 2025-11-08T03:05:56.939Z
Learnt from: SippieCup
Repo: sequelize/sequelize PR: 18051
File: packages/core/src/utils/undot.ts:177-177
Timestamp: 2025-11-08T03:05:56.939Z
Learning: In packages/core/src/utils/undot.ts, the defensive check "if (!Array.isArray(obj))" before numeric leaf assignment in setByPathArray is intentional to handle edge cases where table aliases or oddly-named columns produce single-segment numeric keys (e.g., "0").
Applied to files:
packages/postgres/src/query-generator.js
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-29T19:49:24.715Z
Learning: Applies to packages/*/src/dialect.ts : When adding a dialect feature, update packages/{dialect}/src/dialect.ts to declare feature support flags
Applied to files:
packages/postgres/src/query-generator.js
🧬 Code graph analysis (2)
packages/core/src/abstract-dialect/query-interface.js (3)
packages/core/src/model-typescript.ts (1)
uniqueKeys(352-384)packages/core/src/utils/model-utils.ts (1)
extractModelDefinition(35-45)packages/core/src/model-definition.ts (2)
attributeName(414-447)index(706-757)
packages/postgres/src/query-generator.js (1)
packages/core/src/expression-builders/attribute.ts (1)
attribute(58-62)
🪛 Biome (2.1.2)
packages/core/src/abstract-dialect/query-interface.js
[error] 15-16: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
🪛 ESLint
packages/core/test/integration/model/sync.test.js
[error] 705-719: Expected blank line before this statement.
(padding-line-between-statements)
packages/core/src/abstract-dialect/query-interface.js
[error] 77-77: Unexpected console statement.
(no-console)
[error] 211-211: 'model' is not defined.
(no-undef)
[error] 213-213: Unexpected console statement.
(no-console)
packages/core/test/integration/query-interface/changeColumn.test.js
[error] 273-274: More than 1 blank line not allowed.
(no-multiple-empty-lines)
packages/postgres/src/query-generator.js
[error] 267-267: Expected { after 'if' condition.
(curly)
[error] 271-271: Expected blank line before this statement.
(padding-line-between-statements)
⏰ 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 (13)
.yarnrc.yml (1)
3-3: Yarn version alignment is correct.The yarnPath update to 4.11.0 aligns with the root
package.jsonpackageManager specification. This ensures the local toolchain uses the declared version consistently.package.json (1)
90-90: Dependency updates are consistent and safe.Both the mocha patch version (11.7.4 → 11.7.5) and yarn minor version (4.10.3 → 4.11.0) updates are reflected consistently across the repository. These patch/minor version bumps are typically backward-compatible.
Also applies to: 103-103
packages/mariadb/package.json (1)
52-52: Mocha version update is consistent with repository-wide changes.packages/utils/package.json (1)
65-65: Mocha version update is consistent with repository-wide changes.packages/postgres/package.json (1)
55-55: Mocha version update is consistent with repository-wide changes.Note: The PR objectives indicate this package includes query-generator.js changes to gate UNIQUE constraints. While the package.json update here is safe, the functional changes in query-generator.js would require separate review to verify the UNIQUE constraint deduplication logic.
packages/core/package.json (1)
86-86: Mocha version update is consistent with repository-wide changes.However, the AI summary references runtime console.log statements added to query-interface.js (e.g.,
console.log('=== CREATE TABLE TRIGGERED ===', tableName)). These appear to be debug logging left in the code. Please verify whether these should be removed, made conditional, or replaced with proper observability instrumentation before merging.packages/mssql/package.json (1)
51-51: Mocha version update is consistent with repository-wide changes.dev/db2/latest/docker-compose.yml (1)
4-4: DB2 image version update is a safe patch-level bump.The update from DB2 12.1.2.0 to 12.1.3.0 is a patch version bump with no configuration changes. Ensure the image is available in the registry before deployment.
.github/workflows/ci.yml (1)
41-41: LGTM! Routine GitHub Actions version updates.The artifact action version bumps (upload-artifact v4→v5.0.0, download-artifact v5.0.0→v6.0.0) are standard maintenance updates that improve CI infrastructure reliability.
Also applies to: 56-56, 76-76, 149-149, 169-169, 194-194, 221-221, 250-250, 274-274, 336-336
packages/validator-js/package.json (1)
57-57: LGTM! Routine mocha version bump.This devDependency update is part of a PR-wide upgrade to mocha 11.7.5 across multiple packages, consistent with general maintenance.
packages/core/src/abstract-dialect/query-interface.js (1)
211-224: How to obtain the model reference in changeColumn?Since
modelis undefined, consider one of these approaches:Option 1: Add model parameter
- async changeColumn(tableName, attributeName, dataTypeOrOptions, options) { + async changeColumn(tableName, attributeName, dataTypeOrOptions, options, model) {Option 2: Look up model from sequelize.models
const model = Object.values(this.sequelize.models).find( m => { const table = m.table; const targetTable = typeof tableName === 'string' ? tableName : tableName.tableName; const targetSchema = typeof tableName === 'object' ? tableName.schema : null; return table.tableName === targetTable && (targetSchema ? table.schema === targetSchema : true); } );Option 3: Pass model via options
const model = options?.model;Which approach aligns with the existing Sequelize architecture?
packages/core/test/integration/query-interface/changeColumn.test.js (1)
279-556: LGTM! Comprehensive SQLite constraint preservation tests.These tests thoroughly verify that SQLite's table recreation workaround (required for ALTER TABLE operations) correctly preserves indexes, unique constraints, and foreign keys. The test coverage includes edge cases like null handling and ON DELETE/ON UPDATE actions.
packages/cli/package.json (1)
7-16: All new dependencies verified as actively used in CLI codebase.Verification confirms:
- ansis: Used in
packages/cli/src/commands/seed/generate.tsandpackages/cli/src/commands/migration/generate.tsfor terminal styling- cosmiconfig: Used in
packages/cli/src/_internal/config.tsfor configuration file loading- zod: Used in
packages/cli/src/_internal/config.ts(imported aszfrom 'zod/v3') for schema validationThe dependency updates are routine maintenance with properly integrated new tooling for CLI enhancements.
…olumn to model.sync for consistent availability during alters
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/abstract-dialect/query-interface.js(2 hunks)packages/core/src/model.js(1 hunks)packages/core/test/integration/model/sync.test.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
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/core/src/abstract-dialect/query-interface.jspackages/core/src/model.js
packages/**/test/**/*.test.js
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Name integration tests with the .test.js suffix
Files:
packages/core/test/integration/model/sync.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/integration/model/sync.test.js
🧠 Learnings (4)
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-29T19:49:24.715Z
Learning: Applies to packages/*/src/query-interface.{js,ts} : Add schema operations for new features in query-interface.ts (preferred) or legacy query-interface.js
Applied to files:
packages/core/src/abstract-dialect/query-interface.js
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-29T19:49:24.715Z
Learning: Applies to packages/*/src/query-generator.{js,ts} : Implement dialect feature SQL in query-generator.ts (preferred) or legacy query-generator.js
Applied to files:
packages/core/src/abstract-dialect/query-interface.js
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-29T19:49:24.715Z
Learning: Applies to packages/**/src/**/query-generator.{js,ts} : When modifying query generation, update both the base query generator and all dialect-specific implementations
Applied to files:
packages/core/src/abstract-dialect/query-interface.js
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 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/model/sync.test.js
🧬 Code graph analysis (2)
packages/core/src/model.js (1)
packages/core/src/model-typescript.ts (1)
uniqueKeys(352-384)
packages/core/test/integration/model/sync.test.js (1)
packages/core/src/index.mjs (2)
DataTypes(91-91)DataTypes(91-91)
🪛 ESLint
packages/core/src/abstract-dialect/query-interface.js
[error] 76-76: Unexpected console statement.
(no-console)
packages/core/test/integration/model/sync.test.js
[error] 705-723: Expected blank line before this statement.
(padding-line-between-statements)
⏰ 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)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/postgres/src/query-generator.js (1)
278-282: Add defensive check foruniqueKey.fields.While the framework likely guarantees that
fieldsis always an array, adding optional chaining would make this code more robust against unexpected data structures or future refactoring.Apply this diff to add defensive handling:
const hasExistingUniqueKey = options?.uniqueKeys && Object.values(options.uniqueKeys).some(uniqueKey => - uniqueKey.fields.some(field => normalizeFieldName(field) === inlineUniqueField), + uniqueKey.fields?.some(field => normalizeFieldName(field) === inlineUniqueField), );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/postgres/src/query-generator.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
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/postgres/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/postgres/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/postgres/src/query-generator.js
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: sequelize/sequelize PR: 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
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-29T19:49:24.715Z
Learning: Applies to packages/**/src/**/query-generator.{js,ts} : When modifying query generation, update both the base query generator and all dialect-specific implementations
Applied to files:
packages/postgres/src/query-generator.js
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-29T19:49:24.715Z
Learning: Applies to packages/*/src/query-generator.{js,ts} : Implement dialect feature SQL in query-generator.ts (preferred) or legacy query-generator.js
Applied to files:
packages/postgres/src/query-generator.js
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-29T19:49:24.715Z
Learning: Applies to packages/*/src/query-interface.{js,ts} : Add schema operations for new features in query-interface.ts (preferred) or legacy query-interface.js
Applied to files:
packages/postgres/src/query-generator.js
📚 Learning: 2025-11-08T03:05:56.939Z
Learnt from: SippieCup
Repo: sequelize/sequelize PR: 18051
File: packages/core/src/utils/undot.ts:177-177
Timestamp: 2025-11-08T03:05:56.939Z
Learning: In packages/core/src/utils/undot.ts, the defensive check "if (!Array.isArray(obj))" before numeric leaf assignment in setByPathArray is intentional to handle edge cases where table aliases or oddly-named columns produce single-segment numeric keys (e.g., "0").
Applied to files:
packages/postgres/src/query-generator.js
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-29T19:49:24.715Z
Learning: Applies to packages/*/src/dialect.ts : When adding a dialect feature, update packages/{dialect}/src/dialect.ts to declare feature support flags
Applied to files:
packages/postgres/src/query-generator.js
⏰ 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 (2)
packages/postgres/src/query-generator.js (2)
266-276: Clean abstraction for field name normalization.The
normalizeFieldNamehelper elegantly handles both string and object field descriptors, addressing the earlier concern about multiple comparisons. The property precedence order (field ?? column ?? attribute ?? name) is sensible and returningnullfor invalid types provides safe fallback behavior.
264-286: Well-implemented fix for duplicate unique constraints.The logic correctly prevents duplicate UNIQUE constraints by:
- Computing the inline field name with proper fallback (
attribute.field ?? options.key)- Normalizing field descriptors to handle both string and object shapes
- Checking if an existing unique key already covers the field
- Only emitting the inline UNIQUE keyword when necessary
This addresses the root cause described in issue #17978 where repeated
sync({ alter: true })calls created duplicate named constraints in Postgres.Minor note: While coding guidelines prefer new implementations in TypeScript, modifying existing
.jsquery generator code is acceptable until the file is migrated.Based on coding guidelines.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/core/test/integration/model/sync.test.js (3)
707-713: Rename variable for clarity.The variable name
ignoreUseris misleading since the model is actively used for constraint verification rather than being ignored. Consider renaming toUser(consistent with other tests in this file) oruserfor clarity.Apply this diff:
- const ignoreUser = sequelize.define( + const User = sequelize.define( 'UserUniqueSync', { email: { type: DataTypes.STRING, unique: true }, }, { timestamps: false }, );And update the usage on line 719:
- const results = await sequelize.queryInterface.showConstraints(ignoreUser, { + const results = await sequelize.queryInterface.showConstraints(User, {
719-727: Consider verifying constraint name and fields for completeness.While the test correctly verifies that duplicate constraints are not created, similar tests in this file (e.g., lines 167-189) also verify the constraint name and fields for more thorough validation. Additionally, the SQLite handling allows 0 constraints (
lte(1)), which seems overly lenient—a column withunique: trueshould have at least one constraint even on SQLite.Consider this more thorough approach:
const results = await sequelize.queryInterface.showConstraints(ignoreUser, { constraintType: 'UNIQUE', }); - if (sequelize.dialect.name === 'sqlite' || sequelize.dialect.name === 'sqlite3') { - expect(results.length).to.be.lte(1); - } else { - expect(results).to.have.length(1); - } + expect(results).to.have.length(1); + expect(results[0].columnNames).to.deep.eq(['email']);If SQLite truly behaves differently and requires the lenient check, please add a comment explaining why it may have 0 constraints.
729-729: Remove trailing empty line.There's an unnecessary empty line before the closing brace of the test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/test/integration/model/sync.test.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
packages/**/test/**/*.test.js
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Name integration tests with the .test.js suffix
Files:
packages/core/test/integration/model/sync.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/integration/model/sync.test.js
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: sequelize/sequelize PR: 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
📚 Learning: 2025-09-29T19:49:24.715Z
Learnt from: CR
Repo: sequelize/sequelize PR: 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/model/sync.test.js
🧬 Code graph analysis (1)
packages/core/test/integration/model/sync.test.js (1)
packages/core/src/index.mjs (2)
DataTypes(91-91)DataTypes(91-91)
⏰ 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)
|
@WikiRik Can you please review this again it has been open for long, it's my first time contributing and i just wanted to know when can it be merged |
|
I've taken another look at this and it seems that the test passes already on the |
I feel like i will have to write a different test because this because showConstraints in the main branch is also probably unable to detect the duplicate constraint that is probably why it passes, can you please help me in understanding how i can actually verify this one. |
2 participants
Pull Request Checklist
Description of Changes
Summary
Fixes a long-standing issue where Sequelize creates duplicate UNIQUE constraints on repeated
sequelize.sync({ alter: true })calls.Closes #17978
Root Cause
attributeToSQL()emitted the inlineUNIQUEkeyword duringchangeColumn, without checking for existinguniqueKeys.Fix
model.uniqueKeysintoqueryInterface.changeColumnandqueryGenerator.attributeToSQL.UNIQUEif the field already has a unique constraint.Test
Added
test/integration/query-interface/unique-constraint.test.js(Postgres) verifying that repeated syncs do not create duplicate unique constraints.List of Breaking Changes
None
Summary by CodeRabbit
Bug Fixes
Tests