feat(postgres): support unique nulls not distinct #10986
Conversation
a-fournier
force-pushed
the
add-null-not-distinct
branch
4 times, most recently
from
f2ec614 to
12c02e2
Compare
a-fournier
force-pushed
the
add-null-not-distinct
branch
2 times, most recently
from
b30d76c to
a503e5b
Compare
a-fournier
force-pushed
the
add-null-not-distinct
branch
from
a503e5b to
6de5e99
Compare
|
Hi @pleerock, all tests seem to pass but the Oracle CI reaches the timeout. |
|
Oh, dear! It would've been good to have this feature running. Nice work, @a-fournier! |
|
Nice! Any progress on this? |
The new PostgreSQL version helped me with my issue. So I found a way around it |
|
This is an awesome PR, anything we can do to get it merged? :) |
|
@a-fournier thanks for this contribution! I'll review this soon. If you can do a few things it will improve this PR:
|
michaelbromley
left a comment
michaelbromley
left a comment
There was a problem hiding this comment.
Thanks for this great contribution!
I agree with Mike's suggestions above - if you are able to do those then this should be good to merge.
|
I'll see if I can get it done during the weekend – that is, if @a-fournier doesn't mind, of course... (I'm already excited thinking about the possibility of my first non-doc related commit) |
mguida22
changed the title
Only Postgres v15+ has support for indnullsnotdistinct. If we are running a version before 15 we will always return false. https://www.postgresql.org/docs/15/catalog-pg-index.html
|
Yeah, I think this is possible to run tests on several Postgres versions |
|
Yeah, we just need to add another workflow in Would be great to keep |
There was a problem hiding this comment.
This is a good feature but I think the design is too closely tied to the PostgreSQL implementation. Although NULLS NOT DISTINCT is a PostgreSQL syntax extension, the concept can be applied to other databases.
I know in SQL Server that it works in reverse; by default a unique index treats nulls as equivalent, and you have to create a filtered index to obtain the SQL standard behavior (treating nulls as distinct values). It would be great if we could make it work there as well. See https://stackoverflow.com/a/767702/2168021
Rather than a boolean option, what if it was more like:
nulls?: 'distinct' | 'equivalent'
| * throwOnUnsupported is set to false. | ||
| */ | ||
| isNullsNotDistinctSupported({ | ||
| throwOnUnsupported = true, |
There was a problem hiding this comment.
I would remove this parameter and gracefully handle it in each situation (i.e. without throwing).
| @Unique("unique_constraint_nulls_not_distinct", ["value"], { | ||
| nullsNotDistinct: true, | ||
| }) | ||
| export class ConstraintEntityNullsNotDistinct { |
There was a problem hiding this comment.
I would place each entity in its own file
| unique: true, | ||
| nullsNotDistinct: false, | ||
| }) | ||
| export class IndexEntityNullsDistinct { |
There was a problem hiding this comment.
same as before, i would place each entity in its own file.
| @PrimaryGeneratedColumn() | ||
| id: number | ||
|
|
||
| @Column({ type: "text", nullable: true }) |
There was a problem hiding this comment.
We can maybe remove type and let it be inferred from the string type of the value property.
Then, if other drivers would support this feature, they would only need to add the driver to the list of supported drivers in the test file.
| describe("indices > nulls not distinct", () => { | ||
| let connections: DataSource[] = [] | ||
| before(async () => { | ||
| try { |
There was a problem hiding this comment.
there shouldn't be any try-catch here :)
| closeTestingConnections(connections) | ||
| } | ||
| }) | ||
| describe("constraints", () => { |
There was a problem hiding this comment.
One empty line between each describe / it test would make it easier to spot the tests.
| const table = await queryRunner.getTable( | ||
| "constraint_entity_nulls_distinct", | ||
| ) | ||
| table?.uniques[0].nullsNotDistinct?.should.be.equal(false) |
There was a problem hiding this comment.
Would be great to use chai.expect, it's closer to natural language:
expect(table?.uniques[0].nullsNotDistinct).to.equal(false)
gioboa
left a comment
gioboa
left a comment
There was a problem hiding this comment.
is this PR still valid? may I ask you to address the feedback please?
commit: |
WalkthroughIntroduces a nullsNotDistinct option across decorators, metadata, schema-builder, and entity schema to support PostgreSQL 15+ UNIQUE NULLS NOT DISTINCT for unique constraints and indexes. Postgres driver/query runner gain capability detection and SQL emission/parsing for the flag. Functional tests validate constraints and indexes behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Decorator as Decorators<br/>(@Unique/@Index)
participant Meta as Metadata/Args
participant SB as Schema Builder
participant QR as PostgresQueryRunner
participant PG as PostgreSQL
Dev->>Decorator: Define unique/index { nullsNotDistinct }
Decorator->>Meta: Emit *MetadataArgs* with nullsNotDistinct
Meta->>SB: Build TableUnique/TableIndex with flag
SB->>QR: Create/Alter table/index plan
QR->>QR: Check driver.isNullsNotDistinctSupported()
alt Supported (PG >= 15)
QR->>PG: CREATE ... UNIQUE ... NULLS NOT DISTINCT
else Unsupported
QR->>QR: Omit clause or throw (based on path)
end
sequenceDiagram
autonumber
participant QR as PostgresQueryRunner
participant Driver as PostgresDriver
participant PG as PostgreSQL
QR->>Driver: isNullsNotDistinctSupported({throwOnUnsupported?})
Driver->>PG: Query server version (cached)
PG-->>Driver: Version
alt version >= 15
Driver-->>QR: true
else version < 15
alt throwOnUnsupported
Driver-->>QR: throw TypeORMError
else
Driver-->>QR: false
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/schema-builder/table/TableColumn.ts (1)
201-234: clone() drops isNullsNotDistinct; property lost on copy.This will cause schema-diff surprises. Include it in the cloned options.
return new TableColumn(<TableColumnOptions>{ name: this.name, type: this.type, length: this.length, width: this.width, charset: this.charset, collation: this.collation, precision: this.precision, scale: this.scale, zerofill: this.zerofill, unsigned: this.unsigned, enum: this.enum, enumName: this.enumName, primaryKeyConstraintName: this.primaryKeyConstraintName, asExpression: this.asExpression, generatedType: this.generatedType, default: this.default, onUpdate: this.onUpdate, isNullable: this.isNullable, isGenerated: this.isGenerated, generationStrategy: this.generationStrategy, generatedIdentity: this.generatedIdentity, isPrimary: this.isPrimary, isUnique: this.isUnique, + isNullsNotDistinct: this.isNullsNotDistinct, isArray: this.isArray, comment: this.comment, spatialFeatureType: this.spatialFeatureType, srid: this.srid, })src/schema-builder/table/TableUnique.ts (1)
54-60: Preserve nullsNotDistinct in TableUnique.clone().clone() currently omits nullsNotDistinct, causing copied uniques to lose that flag — add it to the returned options.
File: src/schema-builder/table/TableUnique.ts (clone method)
clone(): TableUnique { return new TableUnique(<TableUniqueOptions>{ name: this.name, columnNames: [...this.columnNames], deferrable: this.deferrable, + nullsNotDistinct: this.nullsNotDistinct, }) }src/entity-schema/EntitySchemaTransformer.ts (1)
336-347: Propagate nullsNotDistinct for entity‑schema uniques
- Add
nullsNotDistinct?: booleanto src/entity-schema/EntitySchemaUniqueOptions.ts.- In src/entity-schema/EntitySchemaTransformer.ts include
nullsNotDistinctwhen building UniqueMetadataArgs, e.g.:
nullsNotDistinct: unique.nullsNotDistinct === true ? true : false.- Add tests for entity‑schema uniques (and indexes) asserting the flag is honored.
src/driver/postgres/PostgresQueryRunner.ts (1)
1859-1884: Altering NULLS NOT DISTINCT on an existing UNIQUE needs drop+recreate.When toggling newColumn.isNullsNotDistinct while keeping unique=true, this adds a second constraint with the same generated name, which will fail. Drop the existing unique first, then add the updated one; mirror in downQueries.
- if (newColumn.isUnique === true) { - const uniqueConstraint = new TableUnique({ + if (newColumn.isUnique === true) { + // If a single-column UNIQUE already exists, drop it before re-adding with updated NULLS NOT DISTINCT + const existingUnique = clonedTable.uniques.find( + (u) => + u.columnNames.length === 1 && + u.columnNames[0] === newColumn.name, + ) + if (existingUnique) { + upQueries.push(this.dropUniqueConstraintSql(table, existingUnique)) + // Down: restore the prior definition (including its previous NULLS NOT DISTINCT state) + downQueries.push( + new Query( + `ALTER TABLE ${this.escapePath(table)} ADD CONSTRAINT "${ + existingUnique.name + }" UNIQUE ${ + existingUnique.nullsNotDistinct && + this.driver.isNullsNotDistinctSupported() + ? "NULLS NOT DISTINCT" + : "" + } ("${newColumn.name}")`, + ), + ) + clonedTable.uniques.splice( + clonedTable.uniques.indexOf(existingUnique), + 1, + ) + } + + const uniqueConstraint = new TableUnique({ name: this.connection.namingStrategy.uniqueConstraintName( table, [newColumn.name], ), columnNames: [newColumn.name], }) clonedTable.uniques.push(uniqueConstraint) upQueries.push( new Query( `ALTER TABLE ${this.escapePath(table)} ADD CONSTRAINT "${ uniqueConstraint.name }" UNIQUE ${ newColumn.isNullsNotDistinct && this.driver.isNullsNotDistinctSupported() ? "NULLS NOT DISTINCT" : "" } ("${newColumn.name}")`, ), ) downQueries.push( new Query( `ALTER TABLE ${this.escapePath( table, )} DROP CONSTRAINT "${uniqueConstraint.name}"`, ), ) } else {
🧹 Nitpick comments (15)
src/decorator/Index.ts (1)
136-137: Set nullsNotDistinct only when the index is unique.Avoid setting a meaningless flag on non-unique indexes.
- nullsNotDistinct: - options && options.nullsNotDistinct ? true : false, + nullsNotDistinct: + options && options.unique && options.nullsNotDistinct + ? true + : false,src/metadata/UniqueMetadata.ts (1)
42-47: Docstring wording is reversed (nulls are NOT distinct).Change to accurately reflect semantics, e.g., “Treats NULL values as not distinct (PostgreSQL 15+).”
Apply:
- /** - * Indicates if column handles null values as distinct. - * Works only in PostgreSQL 15 and above. - */ + /** + * Treats NULL values as not distinct (NULLS NOT DISTINCT). + * Works only in PostgreSQL 15 and above. + */src/decorator/Unique.ts (1)
103-105: LGTM; minor style nit (optional).You can simplify boolean normalization:
nullsNotDistinct: !!options?.nullsNotDistinct.src/schema-builder/options/TableUniqueOptions.ts (1)
25-30: Fix docstring semantics (NOT distinct).Align wording with PostgreSQL feature name.
- /** - * Indicates if column handles null values as distinct. - * Works only in PostgreSQL 15 and above. - */ + /** + * Treats NULL values as not distinct (NULLS NOT DISTINCT). + * Works only in PostgreSQL 15 and above. + */src/entity-schema/EntitySchemaIndexOptions.ts (1)
29-34: Docstring semantics inverted; update to “not distinct.”Keep terminology consistent across surfaces.
- /** - * Indicates if column handles null values as distinct. - * Works only in PostgreSQL 15 and above. - */ + /** + * Treats NULL values as not distinct (NULLS NOT DISTINCT). + * Works only in PostgreSQL 15 and above. + */src/schema-builder/table/TableUnique.ts (1)
30-35: Docstring semantics inverted; use “not distinct.”- /** - * Indicates if column handles null values as distinct. - * Works only in PostgreSQL 15 and above. - */ + /** + * Treats NULL values as not distinct (NULLS NOT DISTINCT). + * Works only in PostgreSQL 15 and above. + */src/schema-builder/options/TableIndexOptions.ts (1)
24-29: Clarify docstring to reflect NULLS NOT DISTINCT semantics.Replace "Indicates if column handles null values as distinct." with "Treats NULL values as not distinct (NULLS NOT DISTINCT)." Update in src/schema-builder/options/TableIndexOptions.ts (lines 24–29) and the matching occurrences in src/schema-builder/table/TableColumn.ts and src/schema-builder/table/TableIndex.ts.
- /** - * Indicates if column handles null values as distinct. - * Works only in PostgreSQL 15 and above. - */ + /** + * Treats NULL values as not distinct (NULLS NOT DISTINCT). + * Works only in PostgreSQL 15 and above. + */src/schema-builder/table/TableIndex.ts (1)
29-34: Fix docstring: NULLs are “not distinct,” not “distinct”.Wording is inverted. Suggest clarifying the semantics and scope.
- /** - * Indicates if column handles null values as distinct. - * Works only in PostgreSQL 15 and above. - */ + /** + * Treats NULL values as not distinct (NULLs compare equal for uniqueness). + * PostgreSQL 15+ only. + */src/metadata/IndexMetadata.ts (1)
31-36: Fix docstring: clarify “NULLS NOT DISTINCT”.Align description with PostgreSQL semantics and constrain scope.
- /** - * Indicates if column handles null values as distinct. - * Works only in PostgreSQL 15 and above. - */ + /** + * Treats NULL values as not distinct (NULLs compare equal for uniqueness). + * PostgreSQL 15+ only. + */test/functional/indices/nulls-not-distinct/nulls-not-distinct.ts (4)
56-61: Release QueryRunner in tests.Avoids open runners across tests.
const queryRunner = connection.createQueryRunner() const table = await queryRunner.getTable( "constraint_entity_nulls_not_distinct", ) table?.uniques[0].nullsNotDistinct?.should.be.equal(true) + await queryRunner.release()
66-71: Release QueryRunner in tests.Same leak pattern here.
const queryRunner = connection.createQueryRunner() const table = await queryRunner.getTable( "constraint_entity_nulls_distinct", ) table?.uniques[0].nullsNotDistinct?.should.be.equal(false) + await queryRunner.release()
87-92: Release QueryRunner in tests.Same adjustment for index case.
const queryRunner = connection.createQueryRunner() const table = await queryRunner.getTable( "index_entity_nulls_not_distinct", ) table?.indices[0].isNullsNotDistinct?.should.be.equal(true) + await queryRunner.release()
97-102: Release QueryRunner in tests.Same adjustment for index false case.
const queryRunner = connection.createQueryRunner() const table = await queryRunner.getTable( "index_entity_nulls_distinct", ) table?.indices[0].isNullsNotDistinct?.should.be.equal(false) + await queryRunner.release()src/driver/postgres/PostgresQueryRunner.ts (2)
3257-3259: Tiny SQL literal nit: add space after ELSE.Cosmetic; improves readability and avoids any parser edge cases.
- `CASE "ix"."indisunique" WHEN 't' THEN 'TRUE' ELSE'FALSE' END AS "is_unique", ` + + `CASE "ix"."indisunique" WHEN 't' THEN 'TRUE' ELSE 'FALSE' END AS "is_unique", ` +
3421-3423: Same tiny nit in tables indices SELECT.- `CASE "ix"."indisunique" WHEN 't' THEN 'TRUE' ELSE'FALSE' END AS "is_unique", ` + + `CASE "ix"."indisunique" WHEN 't' THEN 'TRUE' ELSE 'FALSE' END AS "is_unique", ` +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/decorator/Index.ts(1 hunks)src/decorator/Unique.ts(1 hunks)src/decorator/options/IndexOptions.ts(1 hunks)src/decorator/options/UniqueOptions.ts(1 hunks)src/driver/postgres/PostgresDriver.ts(1 hunks)src/driver/postgres/PostgresQueryRunner.ts(13 hunks)src/entity-schema/EntitySchemaIndexOptions.ts(1 hunks)src/entity-schema/EntitySchemaTransformer.ts(1 hunks)src/metadata-args/IndexMetadataArgs.ts(1 hunks)src/metadata-args/UniqueMetadataArgs.ts(1 hunks)src/metadata/IndexMetadata.ts(2 hunks)src/metadata/UniqueMetadata.ts(2 hunks)src/schema-builder/options/TableColumnOptions.ts(1 hunks)src/schema-builder/options/TableIndexOptions.ts(1 hunks)src/schema-builder/options/TableUniqueOptions.ts(1 hunks)src/schema-builder/table/TableColumn.ts(2 hunks)src/schema-builder/table/TableIndex.ts(4 hunks)src/schema-builder/table/TableUnique.ts(3 hunks)test/functional/indices/nulls-not-distinct/entity/ConstraintEntity.ts(1 hunks)test/functional/indices/nulls-not-distinct/entity/IndexEntity.ts(1 hunks)test/functional/indices/nulls-not-distinct/nulls-not-distinct.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/driver/postgres/PostgresDriver.ts (2)
src/util/VersionUtils.ts (1)
VersionUtils(1-23)src/error/TypeORMError.ts (1)
TypeORMError(1-17)
test/functional/indices/nulls-not-distinct/entity/IndexEntity.ts (2)
test/functional/indices/nulls-not-distinct/entity/ConstraintEntity.ts (2)
Entity(6-16)Entity(18-28)src/decorator/columns/Column.ts (1)
Column(134-220)
test/functional/indices/nulls-not-distinct/nulls-not-distinct.ts (2)
test/utils/test-utils.ts (2)
createTestingConnections(388-482)closeTestingConnections(487-499)src/util/VersionUtils.ts (1)
VersionUtils(1-23)
test/functional/indices/nulls-not-distinct/entity/ConstraintEntity.ts (4)
test/functional/indices/nulls-not-distinct/entity/IndexEntity.ts (2)
Entity(6-20)Entity(22-36)src/decorator/Unique.ts (1)
Unique(43-108)src/decorator/columns/PrimaryGeneratedColumn.ts (1)
PrimaryGeneratedColumn(55-119)src/decorator/columns/Column.ts (1)
Column(134-220)
🪛 GitHub Check: formatting
test/functional/indices/nulls-not-distinct/nulls-not-distinct.ts
[failure] 41-41:
Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator
⏰ 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). (19)
- GitHub Check: tests-linux (20) / mongodb
- GitHub Check: tests-linux (20) / postgres (14)
- GitHub Check: tests-linux (20) / sqlite
- GitHub Check: tests-linux (20) / sqljs
- GitHub Check: tests-linux (20) / mssql
- GitHub Check: tests-linux (18) / postgres (17)
- GitHub Check: tests-linux (20) / better-sqlite3
- GitHub Check: tests-linux (18) / postgres (14)
- GitHub Check: tests-linux (18) / sqlite
- GitHub Check: tests-linux (18) / mysql_mariadb
- GitHub Check: tests-linux (18) / sqljs
- GitHub Check: tests-linux (18) / mysql_mariadb_latest
- GitHub Check: tests-linux (18) / better-sqlite3
- GitHub Check: tests-linux (18) / oracle
- GitHub Check: tests-linux (18) / mssql
- GitHub Check: tests-linux (18) / sap
- GitHub Check: tests-windows / sqljs
- GitHub Check: tests-windows / sqlite
- GitHub Check: tests-windows / better-sqlite3
🔇 Additional comments (18)
src/driver/postgres/PostgresDriver.ts (1)
1442-1461: Make capability check pure; don’t throw here.Prior feedback suggested removing the parameter and not throwing. Let callers decide whether to error.
- isNullsNotDistinctSupported({ - throwOnUnsupported = true, - }: { throwOnUnsupported?: boolean } = {}): boolean { - if (VersionUtils.isGreaterOrEqual(this.version, "15")) { - return true - } - if (throwOnUnsupported) { - throw new TypeORMError( - "nullsNotDistinct is only supported in PostgreSQL 15 and above.", - ) - } - return false - } + // Pure capability check: returns true iff PostgreSQL >= 15. + isNullsNotDistinctSupported(): boolean { + return VersionUtils.isGreaterOrEqual(this.version, "15") + }src/metadata-args/IndexMetadataArgs.ts (1)
25-30: Clarify semantics and scope (only relevant for unique indexes).Also aligns with a prior nit about wording. Apply across similar comments.
- /** - * Indicates if column handles null values as distinct. - * Works only in PostgreSQL 15 and above. - */ + /** + * Treat NULL values as not distinct (equal) in uniqueness checks. + * Only relevant when `unique: true`. PostgreSQL 15+ only. + */ nullsNotDistinct?: booleansrc/entity-schema/EntitySchemaTransformer.ts (1)
305-307: Index flag propagation LGTM.
nullsNotDistinctflows correctly intoIndexMetadataArgs.test/functional/indices/nulls-not-distinct/entity/IndexEntity.ts (1)
1-36: Split entities into separate files for clarity (repeat).Same feedback as earlier review—one entity per file keeps fixtures focused.
src/schema-builder/table/TableUnique.ts (1)
69-77: Creation path propagation LGTM.
TableUnique.createcorrectly forwardsuniqueMetadata.nullsNotDistinct.src/metadata/UniqueMetadata.ts (1)
92-93: Propagation verified — no action required.
UniqueMetadata.nullsNotDistinct is assigned from args and rounds‑trip into TableUnique/TableIndex/TableColumn and DDL generation (PostgresQueryRunner emits/reads "NULLS NOT DISTINCT" and PostgresDriver enforces support).src/schema-builder/table/TableIndex.ts (1)
81-81: Propagation LGTM.Constructor/init, clone(), and static create() correctly carry isNullsNotDistinct through.
Also applies to: 102-102, 126-126
src/metadata/IndexMetadata.ts (1)
156-157: Option wiring LGTM; confirm intent for non-unique indexes.Setting isNullsNotDistinct regardless of unique=true is fine at metadata level, but DDL must ignore/forbid it when not unique.
Would you like us to hard‑guard at SQL emission time so the clause is only appended for unique indexes?
test/functional/indices/nulls-not-distinct/entity/ConstraintEntity.ts (3)
6-16: Entity for nullsNotDistinct=true: OK.Definition matches the test’s expectations.
18-28: Entity for nullsNotDistinct=false: OK.Covers the “false” branch explicitly.
6-28: Split entities into separate files (test readability).Keeps fixtures small and discoverable.
test/functional/indices/nulls-not-distinct/nulls-not-distinct.ts (2)
20-37: Avoid try/catch in before hook (skip unsupported).Prefer version‑gating/conditional test skip over asserting on thrown errors in hooks.
44-44: Minor test style: spacing and assertions.Add one empty line between tests; consider chai.expect for readability.
src/driver/postgres/PostgresQueryRunner.ts (5)
1100-1108: Add-column UNIQUE DDL: NULLS NOT DISTINCT handled correctly.SQL placement after UNIQUE is valid and gated by capability.
3315-3323: View index reflection: flag mapping LGTM.isNullsNotDistinct mapped from catalog output as expected.
4069-4071: Table index reflection: flag mapping LGTM.Consistent with views path.
4124-4130: CREATE TABLE uniques: correct emission with gating.Appending NULLS NOT DISTINCT under capability is correct.
3411-3418: Capability-safe SELECT lists LGTM.Avoids referencing pg_index.indnullsnotdistinct on <15; nice.
Also applies to: 3247-3254
| /** | ||
| * By default, NULL values are not treated as distinct entries. | ||
| * Specifying NULLS NOT DISTINCT on unique indexes | ||
| * will cause NULL values to be treated distinctly. | ||
| * | ||
| * Only applicable when unique: true. | ||
| * Works only in PostgreSQL 15 and above. | ||
| */ | ||
| nullsNotDistinct?: boolean | ||
|
|
There was a problem hiding this comment.
Correct description: default vs effect of NULLS NOT DISTINCT.
- /**
- * By default, NULL values are not treated as distinct entries.
- * Specifying NULLS NOT DISTINCT on unique indexes
- * will cause NULL values to be treated distinctly.
- *
- * Only applicable when unique: true.
- * Works only in PostgreSQL 15 and above.
- */
+ /**
+ * PostgreSQL by default treats NULLs as distinct in unique checks
+ * (allowing multiple rows with NULLs). When set, emits `NULLS NOT DISTINCT`
+ * so NULLs are considered equal and duplicates are rejected.
+ *
+ * Only applicable when `unique: true`. PostgreSQL 15+ only.
+ */
nullsNotDistinct?: boolean📝 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.
| /** | |
| * By default, NULL values are not treated as distinct entries. | |
| * Specifying NULLS NOT DISTINCT on unique indexes | |
| * will cause NULL values to be treated distinctly. | |
| * | |
| * Only applicable when unique: true. | |
| * Works only in PostgreSQL 15 and above. | |
| */ | |
| nullsNotDistinct?: boolean | |
| /** | |
| * PostgreSQL by default treats NULLs as distinct in unique checks | |
| * (allowing multiple rows with NULLs). When set, emits `NULLS NOT DISTINCT` | |
| * so NULLs are considered equal and duplicates are rejected. | |
| * | |
| * Only applicable when `unique: true`. PostgreSQL 15+ only. | |
| */ | |
| nullsNotDistinct?: boolean |
🤖 Prompt for AI Agents
In src/decorator/options/IndexOptions.ts around lines 10 to 19, the comment
incorrectly describes the default behavior and the effect of NULLS NOT DISTINCT;
update the doc to state that by default NULL values are treated as distinct
(multiple NULLs allowed for unique indexes), and that specifying
nullsNotDistinct: true adds NULLS NOT DISTINCT so NULLs are treated as not
distinct (i.e., NULLs are considered equal and uniqueness will prevent multiple
NULLs), and keep the note about PostgreSQL 15+ and applicability only when
unique: true.
|
|
||
| /** | ||
| * By default, NULL values are not treated as distinct entries. | ||
| * Specifying NULLS NOT DISTINCT on unique constraints | ||
| * will cause NULL values to be treated distinctly. | ||
| * | ||
| * Works only in PostgreSQL 15 and above. | ||
| */ | ||
| nullsNotDistinct?: boolean |
There was a problem hiding this comment.
Correct misleading description of default behavior and effect.
By default PostgreSQL treats NULLs as distinct; enabling this option makes NULLs not distinct. Please fix wording.
- /**
- * By default, NULL values are not treated as distinct entries.
- * Specifying NULLS NOT DISTINCT on unique constraints
- * will cause NULL values to be treated distinctly.
- *
- * Works only in PostgreSQL 15 and above.
- */
+ /**
+ * By default in PostgreSQL, NULLs are considered distinct in unique checks
+ * (multiple rows with NULLs are allowed). Setting this option emits
+ * `NULLS NOT DISTINCT`, making NULLs equal for uniqueness.
+ * PostgreSQL 15+ only.
+ */
nullsNotDistinct?: boolean📝 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.
| /** | |
| * By default, NULL values are not treated as distinct entries. | |
| * Specifying NULLS NOT DISTINCT on unique constraints | |
| * will cause NULL values to be treated distinctly. | |
| * | |
| * Works only in PostgreSQL 15 and above. | |
| */ | |
| nullsNotDistinct?: boolean | |
| /** | |
| * By default in PostgreSQL, NULLs are considered distinct in unique checks | |
| * (multiple rows with NULLs are allowed). Setting this option emits | |
| * `NULLS NOT DISTINCT`, making NULLs equal for uniqueness. | |
| * PostgreSQL 15+ only. | |
| */ | |
| nullsNotDistinct?: boolean |
🤖 Prompt for AI Agents
In src/decorator/options/UniqueOptions.ts around lines 11 to 19, the doc comment
incorrectly states the default behavior and effect of the option; update the
comment to say that PostgreSQL by default treats NULL values as distinct and
that setting nullsNotDistinct?: boolean will cause NULLs to be treated as NOT
DISTINCT (i.e., considered equal) for the unique constraint; keep the note that
this only applies to PostgreSQL 15 and above and make the wording concise and
unambiguous.
| }(${columns}) ${ | ||
| index.isNullsNotDistinct && | ||
| this.driver.isNullsNotDistinctSupported() | ||
| ? "NULLS NOT DISTINCT" | ||
| : "" | ||
| } ${index.where ? "WHERE " + index.where : ""}`, | ||
| ) |
There was a problem hiding this comment.
Guard clause: emit NULLS NOT DISTINCT only for UNIQUE indexes.
PostgreSQL accepts the clause only with UNIQUE indexes. Current code would append it even when isUnique=false, producing invalid SQL.
- }(${columns}) ${
- index.isNullsNotDistinct &&
- this.driver.isNullsNotDistinctSupported()
- ? "NULLS NOT DISTINCT"
- : ""
- } ${index.where ? "WHERE " + index.where : ""}`,
+ }(${columns}) ${
+ index.isUnique &&
+ index.isNullsNotDistinct &&
+ this.driver.isNullsNotDistinctSupported()
+ ? "NULLS NOT DISTINCT"
+ : ""
+ } ${index.where ? "WHERE " + index.where : ""}`,📝 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.
| }(${columns}) ${ | |
| index.isNullsNotDistinct && | |
| this.driver.isNullsNotDistinctSupported() | |
| ? "NULLS NOT DISTINCT" | |
| : "" | |
| } ${index.where ? "WHERE " + index.where : ""}`, | |
| ) | |
| }(${columns}) ${ | |
| - index.isNullsNotDistinct && | |
| index.isUnique && | |
| index.isNullsNotDistinct && | |
| this.driver.isNullsNotDistinctSupported() | |
| ? "NULLS NOT DISTINCT" | |
| : "" | |
| } ${index.where ? "WHERE " + index.where : ""}`, | |
| ) |
🤖 Prompt for AI Agents
In src/driver/postgres/PostgresQueryRunner.ts around lines 4407 to 4413, the
code unconditionally emits "NULLS NOT DISTINCT" when index.isNullsNotDistinct is
true and the driver supports it, but PostgreSQL only allows that clause for
UNIQUE indexes; update the condition so "NULLS NOT DISTINCT" is appended only
when index.isUnique is true AND index.isNullsNotDistinct is true AND
this.driver.isNullsNotDistinctSupported() returns true, leaving the rest of the
index SQL construction unchanged.
| index.isNullsNotDistinct && | ||
| this.driver.isNullsNotDistinctSupported() | ||
| ? "NULLS NOT DISTINCT" | ||
| : "" | ||
| } ${index.where ? "WHERE " + index.where : ""}`, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Same guard for view indexes.
Mirror the uniqueness check when creating indexes on views/materialized views.
- }" ON ${this.escapePath(view)} (${columns}) ${
- index.isNullsNotDistinct &&
- this.driver.isNullsNotDistinctSupported()
- ? "NULLS NOT DISTINCT"
- : ""
- } ${index.where ? "WHERE " + index.where : ""}`,
+ }" ON ${this.escapePath(view)} (${columns}) ${
+ index.isUnique &&
+ index.isNullsNotDistinct &&
+ this.driver.isNullsNotDistinctSupported()
+ ? "NULLS NOT DISTINCT"
+ : ""
+ } ${index.where ? "WHERE " + index.where : ""}`,📝 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.
| index.isNullsNotDistinct && | |
| this.driver.isNullsNotDistinctSupported() | |
| ? "NULLS NOT DISTINCT" | |
| : "" | |
| } ${index.where ? "WHERE " + index.where : ""}`, | |
| ) | |
| } | |
| index.isUnique && | |
| index.isNullsNotDistinct && | |
| this.driver.isNullsNotDistinctSupported() | |
| ? "NULLS NOT DISTINCT" | |
| : "" | |
| } ${index.where ? "WHERE " + index.where : ""}`, | |
| ) | |
| } |
🤖 Prompt for AI Agents
In src/driver/postgres/PostgresQueryRunner.ts around lines 4427 to 4433, the
index creation logic for views/materialized views is missing the same uniqueness
guard used elsewhere; update the view index SQL generation to mirror the
uniqueness check (i.e., only emit "UNIQUE" when the index.isUnique flag is true
and when uniqueness is supported for view indexes), applying the same
conditional used for regular table indexes and ensuring the rest of the index
options (NULLS NOT DISTINCT, WHERE clause) are preserved unchanged.
| /** | ||
| * Indicates if column handles null values as distinct. | ||
| * Works only in PostgreSQL 15 and above. | ||
| */ | ||
| nullsNotDistinct?: boolean |
There was a problem hiding this comment.
Fix semantics: NULLs become not distinct (equal) when enabled.
- /**
- * Indicates if column handles null values as distinct.
- * Works only in PostgreSQL 15 and above.
- */
+ /**
+ * Treat NULL values as not distinct (equal) in the unique constraint.
+ * PostgreSQL 15+ only.
+ */
nullsNotDistinct?: boolean📝 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.
| /** | |
| * Indicates if column handles null values as distinct. | |
| * Works only in PostgreSQL 15 and above. | |
| */ | |
| nullsNotDistinct?: boolean | |
| /** | |
| * Treat NULL values as not distinct (equal) in the unique constraint. | |
| * PostgreSQL 15+ only. | |
| */ | |
| nullsNotDistinct?: boolean |
🤖 Prompt for AI Agents
In src/metadata-args/UniqueMetadataArgs.ts around lines 27 to 31, the JSDoc
currently says the flag "Indicates if column handles null values as distinct"
but the intended semantics are the opposite: when enabled NULLs become not
distinct (treated as equal). Update the comment to state that nullsNotDistinct
marks that NULL values are considered equal (not distinct) in uniqueness checks,
and keep the note that this only works in PostgreSQL 15 and above.
| /** | ||
| * Indicates if column handles null values as distinct. | ||
| * Works only in PostgreSQL 15 and above. | ||
| */ | ||
| isNullsNotDistinct?: boolean | ||
|
|
There was a problem hiding this comment.
Fix semantics: NULLS NOT DISTINCT treats NULLs as equal, not distinct.
The doc currently says "handles null values as distinct" which is the opposite of the feature. Suggest rewording.
- /**
- * Indicates if column handles null values as distinct.
- * Works only in PostgreSQL 15 and above.
- */
+ /**
+ * Treat NULL values as not distinct (i.e., equal) for uniqueness checks.
+ * PostgreSQL 15+ only.
+ */
isNullsNotDistinct?: boolean📝 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.
| /** | |
| * Indicates if column handles null values as distinct. | |
| * Works only in PostgreSQL 15 and above. | |
| */ | |
| isNullsNotDistinct?: boolean | |
| /** | |
| * Treat NULL values as not distinct (i.e., equal) for uniqueness checks. | |
| * PostgreSQL 15+ only. | |
| */ | |
| isNullsNotDistinct?: boolean |
🤖 Prompt for AI Agents
In src/schema-builder/options/TableColumnOptions.ts around lines 54 to 59, the
JSDoc for isNullsNotDistinct is incorrect: it currently says "handles null
values as distinct" but NULLS NOT DISTINCT means NULLs are treated as equal.
Update the comment to state that the option makes NULL values compare as equal
(i.e., NOT DISTINCT) and mention it applies to PostgreSQL 15 and above — for
example: "Indicates whether NULL values are treated as equal (NULLS NOT
DISTINCT). Works only in PostgreSQL 15 and above."
| /** | ||
| * Indicates if column handles null values as distinct. | ||
| * Works only in PostgreSQL 15 and above. | ||
| */ | ||
| isNullsNotDistinct: boolean = false | ||
|
|
There was a problem hiding this comment.
Fix semantics in JSDoc: NULLS NOT DISTINCT = NULLs are equal.
- /**
- * Indicates if column handles null values as distinct.
- * Works only in PostgreSQL 15 and above.
- */
+ /**
+ * Treat NULL values as not distinct (equal) for uniqueness checks.
+ * PostgreSQL 15+ only.
+ */
isNullsNotDistinct: boolean = false📝 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.
| /** | |
| * Indicates if column handles null values as distinct. | |
| * Works only in PostgreSQL 15 and above. | |
| */ | |
| isNullsNotDistinct: boolean = false | |
| /** | |
| * Treat NULL values as not distinct (equal) for uniqueness checks. | |
| * PostgreSQL 15+ only. | |
| */ | |
| isNullsNotDistinct: boolean = false |
🤖 Prompt for AI Agents
In src/schema-builder/table/TableColumn.ts around lines 59 to 64, the JSDoc
incorrectly describes the semantics of NULLS NOT DISTINCT; update the comment to
state that isNullsNotDistinct indicates the column treats NULL values as equal
(i.e., NULLS NOT DISTINCT), not that it treats them as distinct, and keep the
note about PostgreSQL 15+ support and the default value.
🧩 Analysis chain
Confirm column-level flag is actually consumed; otherwise remove.
NULLS NOT DISTINCT is a constraint/index attribute. If column-level isNullsNotDistinct isn’t read during schema diff/introspection, consider dropping it to avoid API surface that has no effect, or wire it through single-column unique creation and diff checks.
Also applies to: 184-184, 201-234
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf "CWD: %s\n\n" "$(pwd)"
echo "---- list src/schema-builder/table ----"
ls -la src/schema-builder/table || true
echo
echo "---- rg: isNullsNotDistinct (case-sensitive) ----"
rg -n --hidden -S "isNullsNotDistinct" -C3 || true
echo
echo "---- rg: .isNullsNotDistinct property access ----"
rg -n --hidden -S "\.isNullsNotDistinct\b" -C3 || true
echo
echo "---- rg: nullsNotDistinct (case-insensitive) ----"
rg -n --hidden -S -i "nullsNotDistinct" -C3 || true
echo
echo "---- rg: literal 'NULLS NOT DISTINCT' ----"
rg -n --hidden -S "NULLS NOT DISTINCT" -C3 || true
echo
echo "---- show src/schema-builder/table/TableColumn.ts (if present) ----"
if [ -f src/schema-builder/table/TableColumn.ts ]; then
sed -n '1,260p' src/schema-builder/table/TableColumn.ts || true
else
echo "File not found: src/schema-builder/table/TableColumn.ts"
rg -n --hidden -S "TableColumn.ts" -C3 || true
fiLength of output: 74179
Keep column-level isNullsNotDistinct but fix cloning
- Confirmed: isNullsNotDistinct is actively used (PostgresQueryRunner builds UNIQUE/INDEX SQL and schema-diff checks newColumn.isNullsNotDistinct !== oldColumn.isNullsNotDistinct).
- Action: TableColumn.clone() omits the flag — add isNullsNotDistinct: this.isNullsNotDistinct to the cloned TableColumnOptions (src/schema-builder/table/TableColumn.ts; constructor sets it at ~line 184; clone() near the file end).
🤖 Prompt for AI Agents
In src/schema-builder/table/TableColumn.ts around the constructor at ~line 184
and the clone() method near the file end (lines ~59-64 reference the property),
the isNullsNotDistinct boolean is declared but not copied when cloning; update
TableColumn.clone() to include isNullsNotDistinct: this.isNullsNotDistinct in
the TableColumnOptions passed to the new TableColumn so cloned instances
preserve the flag used by PostgresQueryRunner and schema-diff checks.
| after(() => { | ||
| if (connections && connections.length > 0) { | ||
| closeTestingConnections(connections) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Await teardown to avoid dangling connections.
closeTestingConnections returns a Promise; not awaiting can leak resources and flake CI.
- after(() => {
- if (connections && connections.length > 0) {
- closeTestingConnections(connections)
- }
- })
+ after(async () => {
+ if (connections && connections.length > 0) {
+ await closeTestingConnections(connections)
+ }
+ })📝 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.
| after(() => { | |
| if (connections && connections.length > 0) { | |
| closeTestingConnections(connections) | |
| } | |
| }) | |
| after(async () => { | |
| if (connections && connections.length > 0) { | |
| await closeTestingConnections(connections) | |
| } | |
| }) |
🧰 Tools
🪛 GitHub Check: formatting
[failure] 41-41:
Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator
🤖 Prompt for AI Agents
In test/functional/indices/nulls-not-distinct/nulls-not-distinct.ts around lines
39 to 43, the test teardown calls closeTestingConnections(connections) without
awaiting its returned Promise which can leak connections and flake CI; change
the after hook to either be async and await closeTestingConnections(connections)
or return the Promise from the after callback so the test runner waits for
teardown completion.
gioboa
left a comment
gioboa
left a comment
There was a problem hiding this comment.
@a-fournier thanks for opening this PR.
I'm closing this for now because it's stale and with conflicts, feel free to create a new PR with your improvements. 💪
11 participants
Closes: #9827
Description of change
This MR fix adds the nullsNotDistinct option for unique constraints and indexes.
This option is available with Postgresql 15 to handle all null values as the same value.
https://www.postgresql.org/docs/current/indexes-unique.html
Example :
A Table(id, value), without a unique null not distinct on the value column
You'll be able to create (1, NULL) and (2, NULL) even if it should be unique
A Table(id, value), with a unique null not distinct on the value column
If you create (1, NULL) and then (2, NULL), you'll get a duplicate key error
Pull-Request Checklist
masterbranchnpm run formatto apply prettier formattingnpm run testpasses with this changeFixes #9827Summary by CodeRabbit
New Features
Tests