feat: Add the ability to name an enum to allow for sharing it between multiple tables#18223
Open
papandreou wants to merge 19 commits into
Open
feat: Add the ability to name an enum to allow for sharing it between multiple tables#18223papandreou wants to merge 19 commits into
papandreou wants to merge 19 commits into
Conversation
…js packages/postgres/src/query-generator.js
Contributor
📝 WalkthroughWalkthroughAdds Postgres enum naming support: DataTypes.ENUM accepts optional ChangesPostgres ENUM Custom Naming Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/core/test/unit/dialects/postgres/enum.test.js (1)
218-251: ⚡ Quick winClose
createSequelizeInstance()instances inafterEach.These tests create per-test Sequelize instances but never close them. This can leak pools and make the suite flaky.
As per coding guidelines, "Use beforeEach/afterEach to sync with { force: true } and close Sequelize instances in tests".
Also applies to: 384-533
🤖 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/postgres/enum.test.js` around lines 218 - 251, The tests call createSequelizeInstance() per it-block (variables named sq) but never close those Sequelize instances, leaking connection pools; update the addColumnQuery describe block (and the other affected blocks around lines 384-533) to create the Sequelize instance in a beforeEach or assign each test's sq to an outer-scoped variable and ensure an afterEach awaits sq.close() (or sq.close().then/await) to properly shut down the connection pool; reference createSequelizeInstance(), the local variable sq, and the addColumnQuery describe block when making the change.
🤖 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/src/abstract-dialect/data-types.ts`:
- Around line 2324-2330: The code copies options.name and options.schema into
opts without type checks, so add runtime validation in the
constructor/initializer that sets opts (the block handling
options?.name/options?.schema) to ensure both, if provided, are strings; if not,
throw a clear TypeError (e.g., "ENUM name must be a string" / "ENUM schema must
be a string"). Update the checks around options.name and options.schema to use
typeof === 'string' (or undefined) before assignment and throw with the
identifying symbols options and opts so callers get an immediate, actionable
error at construction time.
In `@packages/postgres/src/query-generator.js`:
- Around line 179-183: The enum-cast path in changeColumnQuery currently only
detects enums by the string pattern "ENUM(...)" so custom-named/schemed enums
that are emitted as qualified identifiers skip adding the required USING cast;
update changeColumnQuery to accept/inspect DataType objects (not just
preprocessed strings) or at minimum detect qualified enum identifiers (e.g.
quoted/qualified names containing dots or schema qualifiers) and treat them as
enums when deciding to append "USING ...::type"; modify the logic in
changeColumnQuery (and callers that pass DataTypes.ENUM) to check for
DataTypes.ENUM instances or a qualified-name pattern and ensure the generated
ALTER COLUMN ... TYPE ... includes the appropriate USING clause casting
old_value::new_enum_type.
In `@packages/postgres/src/query-interface.js`:
- Around line 313-345: The shared-enum check is comparing raw options.schema
values (e.g., undefined vs 'public') and can misclassify enums; update the
isSharedWithOtherModel logic to compare resolved/effective schemas instead of
raw options.schema by normalizing both the current enum's schema and each
otherModel enum's schema to the same effective value (e.g., resolve undefined to
the model/table schema or the DB default like 'public') before comparing names
and schemas; ensure you also use the same normalization for the current table's
schema when generating fullEnumName via this.queryGenerator.pgEnumName and
reference symbols enumType, otherType, DataTypes.ENUM, isSharedWithOtherModel,
this.sequelize.models, options?.schema, attribute.attributeName, and
this.queryGenerator.pgEnumName when making the changes.
---
Nitpick comments:
In `@packages/core/test/unit/dialects/postgres/enum.test.js`:
- Around line 218-251: The tests call createSequelizeInstance() per it-block
(variables named sq) but never close those Sequelize instances, leaking
connection pools; update the addColumnQuery describe block (and the other
affected blocks around lines 384-533) to create the Sequelize instance in a
beforeEach or assign each test's sq to an outer-scoped variable and ensure an
afterEach awaits sq.close() (or sq.close().then/await) to properly shut down the
connection pool; reference createSequelizeInstance(), the local variable sq, and
the addColumnQuery describe block when making the change.
🪄 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
Run ID: 172ce5a5-4c44-4573-b2ed-5a9c5c7731ad
📒 Files selected for processing (6)
packages/core/src/abstract-dialect/data-types.tspackages/core/test/unit/dialects/postgres/enum.test.jspackages/postgres/src/_internal/data-types-overrides.tspackages/postgres/src/query-generator.d.tspackages/postgres/src/query-generator.jspackages/postgres/src/query-interface.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/postgres/src/query-generator.js (1)
151-199:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the named-enum
USINGclause separate fromdefinition.At Line 182,
definitioncan still include trailing modifiers likeUNIQUE, so this path can emit invalid SQL such asUSING ("status"::"shared_status" UNIQUE). It also prevents the laterUNIQUEcleanup from matching. Capture the raw enum type fromENUM_NAMED(...)and append theUSINGclause separately fromdefinition.Suggested fix
for (const attributeName in attributes) { let definition = this.dataTypeMapping(tableName, attributeName, attributes[attributeName]); let attrSql = ''; + let usingClause = ''; @@ } else if (attributes[attributeName].startsWith('ENUM_NAMED(')) { - // Custom-named ENUM: the type is managed externally; the type name is already in - // `definition` (ENUM_NAMED wrapper stripped by dataTypeMapping). Just add USING cast. - definition += ` USING (${this.quoteIdentifier(attributeName)}::${definition.trim()})`; + const namedEnumType = attributes[attributeName].match(/^ENUM_NAMED\((.+?)\)/)?.[1]; + usingClause = ` USING (${this.quoteIdentifier(attributeName)}::${namedEnumType})`; } @@ } else { - attrSql += query(`${this.quoteIdentifier(attributeName)} TYPE ${definition}`); + attrSql += query(`${this.quoteIdentifier(attributeName)} TYPE ${definition}${usingClause}`); }🤖 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/postgres/src/query-generator.js` around lines 151 - 199, The ENUM_NAMED branch currently appends a USING cast directly to definition which can leave trailing modifiers (like UNIQUE) in the type and break later cleanup; instead, extract the raw enum type name from attributes[attributeName] (the ENUM_NAMED(...) payload) or from the stripped definition, store it in a separate usingClause (e.g. usingClause = ` USING (${this.quoteIdentifier(attributeName)}::${enumType})`), do not mutate definition, and then append usingClause to attrSql or to the final TYPE/ALTER statement after UNIQUE/REFERENCES handling so that definition remains clean for later regexes; reference the ENUM_NAMED branch, attributes[attributeName], definition, pgEnumName and quoteIdentifier to implement this change.
♻️ Duplicate comments (1)
packages/postgres/src/query-interface.js (1)
350-383:⚠️ Potential issue | 🟠 Major
dropTablestill resolves shared named enums against the wrong schema whenoptions.schemais used.
resolveEnumSchemaignoresoptions?.schema, sodropTable('users', { schema: 'tenant1' })can still compare and drop the enum as if it belonged tomodel.table.schema/ the default schema instead oftenant1. That keeps the earlier shared-enum misclassification bug reachable here.🤖 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/postgres/src/query-interface.js` around lines 350 - 383, The resolveEnumSchema helper used when dropping enums ignores the dropTable call's options.schema and so misclassifies shared enums; update resolveEnumSchema in query-interface.js to consider the effective table/schema option (e.g., options?.schema) before falling back to ownerModel.table.schema or defaultSchema so it returns et.options.schema ?? options?.schema ?? ownerModel.table.schema ?? defaultSchema (or otherwise prefer the provided options schema when present), and ensure callers that compute currentEnumSchema and the call to this.queryGenerator.pgEnumName use that corrected value.
🧹 Nitpick comments (1)
packages/core/test/unit/data-types/misc-data-types.test.ts (1)
107-111: 💤 Low valueLGTM! Good symmetry with the name validation test.
The test correctly validates that an empty
schemaoption throws aTypeError. Both validation tests (name and schema) are well-structured and provide clear error messages.Optional: Consider additional validation test cases
While the current tests correctly validate empty strings, you might consider adding tests for other edge cases to ensure comprehensive validation:
nullandundefinedvalues (to confirm whether they're allowed as "use default behavior")- Wrong types (e.g., numbers, objects) to verify type checking
However, if these cases are already covered elsewhere or if the implementation only validates empty strings specifically, the current tests are sufficient.
🤖 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/data-types/misc-data-types.test.ts` around lines 107 - 111, Add additional unit tests around DataTypes.ENUM validation to cover edge cases beyond empty strings: create tests that assert a TypeError (or expected behavior) when schema is null, undefined, a number, and an object, and likewise add similar cases for the "name" option; locate the existing test case titled 'throws if the "schema" option is an empty string' and add new it(...) blocks that call DataTypes.ENUM({ values: ['a','b'], schema: <value> }) and assert the appropriate exception or acceptance for each input type.
🤖 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/postgres/src/query-interface.js`:
- Around line 27-35: The guard in changeColumn only pre-creates enums when
enumType.options.name exists, but attributeToSQL can emit schema-qualified enums
when enumType.options.schema is set without a custom name; update the
changeColumn logic that currently calls
this.sequelize.queryRaw(this.queryGenerator.pgEnum(...)) to also pre-create
schema-only enums by checking for enumType.options.schema (in addition to
options.name) and invoking queryGenerator.pgEnum with enumSchema set so the
schema-qualified enum is created before running ALTER COLUMN; locate
changeColumn, attributeToSQL, and the call sites that reference
enumType.options.name/enumType.options.schema to add the extra conditional
branch and ensure options.raw remains true when creating the enum.
---
Outside diff comments:
In `@packages/postgres/src/query-generator.js`:
- Around line 151-199: The ENUM_NAMED branch currently appends a USING cast
directly to definition which can leave trailing modifiers (like UNIQUE) in the
type and break later cleanup; instead, extract the raw enum type name from
attributes[attributeName] (the ENUM_NAMED(...) payload) or from the stripped
definition, store it in a separate usingClause (e.g. usingClause = ` USING
(${this.quoteIdentifier(attributeName)}::${enumType})`), do not mutate
definition, and then append usingClause to attrSql or to the final TYPE/ALTER
statement after UNIQUE/REFERENCES handling so that definition remains clean for
later regexes; reference the ENUM_NAMED branch, attributes[attributeName],
definition, pgEnumName and quoteIdentifier to implement this change.
---
Duplicate comments:
In `@packages/postgres/src/query-interface.js`:
- Around line 350-383: The resolveEnumSchema helper used when dropping enums
ignores the dropTable call's options.schema and so misclassifies shared enums;
update resolveEnumSchema in query-interface.js to consider the effective
table/schema option (e.g., options?.schema) before falling back to
ownerModel.table.schema or defaultSchema so it returns et.options.schema ??
options?.schema ?? ownerModel.table.schema ?? defaultSchema (or otherwise prefer
the provided options schema when present), and ensure callers that compute
currentEnumSchema and the call to this.queryGenerator.pgEnumName use that
corrected value.
---
Nitpick comments:
In `@packages/core/test/unit/data-types/misc-data-types.test.ts`:
- Around line 107-111: Add additional unit tests around DataTypes.ENUM
validation to cover edge cases beyond empty strings: create tests that assert a
TypeError (or expected behavior) when schema is null, undefined, a number, and
an object, and likewise add similar cases for the "name" option; locate the
existing test case titled 'throws if the "schema" option is an empty string' and
add new it(...) blocks that call DataTypes.ENUM({ values: ['a','b'], schema:
<value> }) and assert the appropriate exception or acceptance for each input
type.
🪄 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
Run ID: 03590140-b2d3-493a-9bc1-5ef34b2f67ee
📒 Files selected for processing (5)
packages/core/src/abstract-dialect/data-types.tspackages/core/test/unit/data-types/misc-data-types.test.tspackages/core/test/unit/dialects/postgres/enum.test.jspackages/postgres/src/query-generator.jspackages/postgres/src/query-interface.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/abstract-dialect/data-types.ts
- packages/core/test/unit/dialects/postgres/enum.test.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/test/unit/dialects/postgres/enum.test.js (1)
435-443: ⚡ Quick winInconsistent optional chaining between helper functions.
getCreateTypeSqlsuses optional chaining (sql?.includes) on line 442, butgetDropTypeSqlsdoes not use it on line 437. Since both helpers are used in similar contexts and accessstub.argsin the same way, they should handle potentialundefinedvalues consistently.♻️ Suggested fix for consistency
function getDropTypeSqls(stub) { - return stub.args.map(([sql]) => sql).filter(sql => sql.includes('DROP TYPE')); + return stub.args.map(([sql]) => sql).filter(sql => sql?.includes('DROP TYPE')); }🤖 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/postgres/enum.test.js` around lines 435 - 443, The two helper functions getDropTypeSqls and getCreateTypeSqls are inconsistent about guarding against undefined SQL strings; make them consistent by applying the same optional chaining check when calling includes (either use sql?.includes in getDropTypeSqls to match getCreateTypeSqls, or remove the ? in getCreateTypeSqls) so both functions access stub.args safely; update the getDropTypeSqls implementation that maps stub.args to use sql?.includes('DROP TYPE') to mirror getCreateTypeSqls.
🤖 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.
Nitpick comments:
In `@packages/core/test/unit/dialects/postgres/enum.test.js`:
- Around line 435-443: The two helper functions getDropTypeSqls and
getCreateTypeSqls are inconsistent about guarding against undefined SQL strings;
make them consistent by applying the same optional chaining check when calling
includes (either use sql?.includes in getDropTypeSqls to match
getCreateTypeSqls, or remove the ? in getCreateTypeSqls) so both functions
access stub.args safely; update the getDropTypeSqls implementation that maps
stub.args to use sql?.includes('DROP TYPE') to mirror getCreateTypeSqls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd834949-9002-47b4-8ac1-0591e84958ec
📒 Files selected for processing (2)
packages/core/test/unit/dialects/postgres/enum.test.jspackages/postgres/src/query-interface.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/postgres/src/query-interface.js
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
PostgreSQL enum types are named database objects. Sequelize has always auto-generated those names as
enum_<table>_<column>, which works fine for a single column but makes it impossible to share one enum type across multiple tables, or to place it in a schema that differs from the table's schema. This PR addsnameandschemaoptions toDataTypes.ENUMto address both cases.Closes #2577
List of Breaking Changes
Summary by CodeRabbit
name) and custom schemas (schema); named enums (including ARRAY-of-ENUM) are created before use and change-column casts reference qualified enum types.name/schema.