Repository: knex/knex
Pull request #4887 (2021-12-12 22:20:44)

Added support for MSSQL unique constraint
MSSQL supports both unique indexes and unique constraints. One of the issues we found was unique index (current implementation) cannot be used with foreign key relationships. We must use unique constraints instead.

This is all based on issue #4825

I thought I'd give it a go and update the way MSSQL handles `table.unique`. This is my first PR here so please let me know if there's anything missing, which I'm sure there is.

I created a new test which seems to work fine 🤞 

kibertoad: Can you create a PR for https://github.com/knex/documentation with documentation?
Also TypeScript types need to be updated
mlevit: @kibertoad could you point me in the right direction? I'm no TS developer, so not really sure what to look at.
kibertoad: @mlevit I think somewhere around here: https://github.com/knex/knex/blob/2763e47129837111fc5fd4dd5fb19996580bc01d/types/index.d.ts#L2078
mlevit: @kibertoad @OlivierCavadenti are we happy with the argument name `constraint`? Is it descriptive enough or should we change it?
kibertoad: @mlevit Something more explicitly boolean `isContraint` or `useConstraint` might be less surprising, but I defer to @OlivierCavadenti here.
mlevit: My only issue is the argument is only usable for MSSQL... so not really sure what the best play here is. Would `createMssqlConstraint` be too verbose? `isConstraint` is good, but sort of doesn't make sense for other DBs.
kibertoad: @mlevit We don't do db-specific naming for db-specific parameters. Note in documentation is sufficient
mlevit: Documentation updated https://github.com/knex/documentation/pull/373
OlivierCavadenti: > @kibertoad @OlivierCavadenti are we happy with the argument name `constraint`? Is it descriptive enough or should we change it?

useConstraint seems nice in my opinion, more descriptive. 
mlevit: > > @kibertoad @OlivierCavadenti are we happy with the argument name `constraint`? Is it descriptive enough or should we change it?
> 
> useConstraint seems nice in my opinion, more descriptive.

Done.

Patch:
diff --git a/lib/dialects/mssql/schema/mssql-tablecompiler.js b/lib/dialects/mssql/schema/mssql-tablecompiler.js
index 3c5bc9db..c4470954 100644
--- a/lib/dialects/mssql/schema/mssql-tablecompiler.js
+++ b/lib/dialects/mssql/schema/mssql-tablecompiler.js
@@ -279,13 +279,14 @@ class TableCompiler_MSSQL extends TableCompiler {
    * Create a unique index.
    *
    * @param {string | string[]} columns
-   * @param {string | {indexName: undefined | string, deferrable?: 'not deferrable'|'deferred'|'immediate' }} indexName
+   * @param {string | {indexName: undefined | string, deferrable?: 'not deferrable'|'deferred'|'immediate', useConstraint?: true|false }} indexName
    */
   unique(columns, indexName) {
     /** @type {string | undefined} */
     let deferrable;
+    let useConstraint = false;
     if (isObject(indexName)) {
-      ({ indexName, deferrable } = indexName);
+      ({ indexName, deferrable, useConstraint } = indexName);
     }
     if (deferrable && deferrable !== 'not deferrable') {
       this.client.logger.warn(
@@ -304,13 +305,23 @@ class TableCompiler_MSSQL extends TableCompiler {
       .map((column) => this.formatter.columnize(column) + ' IS NOT NULL')
       .join(' AND ');
 
-    // make unique constraint that allows null https://stackoverflow.com/a/767702/360060
-    // to be more or less compatible with other DBs (if any of the columns is NULL then "duplicates" are allowed)
-    this.pushQuery(
-      `CREATE UNIQUE INDEX ${indexName} ON ${this.tableName()} (${this.formatter.columnize(
-        columns
-      )}) WHERE ${whereAllTheColumnsAreNotNull}`
-    );
+    if (useConstraint) {
+      // mssql supports unique indexes and unique constraints.
+      // unique indexes cannot be used with foreign key relationships hence unique constraints are used instead.
+      this.pushQuery(
+        `ALTER TABLE ${this.tableName()} ADD CONSTRAINT ${indexName} UNIQUE (${this.formatter.columnize(
+          columns
+        )})`
+      );
+    } else {
+      // make unique constraint that allows null https://stackoverflow.com/a/767702/360060
+      // to be more or less compatible with other DBs (if any of the columns is NULL then "duplicates" are allowed)
+      this.pushQuery(
+        `CREATE UNIQUE INDEX ${indexName} ON ${this.tableName()} (${this.formatter.columnize(
+          columns
+        )}) WHERE ${whereAllTheColumnsAreNotNull}`
+      );
+    }
   }
 
   // Compile a drop index command.
diff --git a/types/index.d.ts b/types/index.d.ts
index 53bd9f7c..183aac1b 100644
--- a/types/index.d.ts
+++ b/types/index.d.ts
@@ -2075,7 +2075,7 @@ export declare namespace Knex {
     ): TableBuilder;
     setNullable(column: string): TableBuilder;
     dropNullable(column: string): TableBuilder;
-    unique(columnNames: readonly (string | Raw)[], options?: Readonly<{indexName?: string, storageEngineIndexType?: string, deferrable?: deferrableType}>): TableBuilder;
+    unique(columnNames: readonly (string | Raw)[], options?: Readonly<{indexName?: string, storageEngineIndexType?: string, deferrable?: deferrableType, useConstraint?: boolean}>): TableBuilder;
     /** @deprecated */
     unique(columnNames: readonly (string | Raw)[], indexName?: string): TableBuilder;
     foreign(column: string, foreignKeyName?: string): ForeignConstraintBuilder;


Test patch:
diff --git a/test/integration2/dialects/mssql.spec.js b/test/integration2/dialects/mssql.spec.js
index 934d40e4..25c488d4 100644
--- a/test/integration2/dialects/mssql.spec.js
+++ b/test/integration2/dialects/mssql.spec.js
@@ -170,7 +170,7 @@ describe('MSSQL dialect', () => {
           });
         });
 
-        describe('unique table constraint with options object', () => {
+        describe('unique table index with options object', () => {
           const tableName = 'test_unique_index_options';
           before(async () => {
             await knex.schema.createTable(tableName, function () {
@@ -188,7 +188,39 @@ describe('MSSQL dialect', () => {
             await knex.schema.alterTable(tableName, function () {
               this.unique(['x', 'y'], { indexName });
             });
-            expect(
+            await expect(
+              knex
+                .insert([
+                  { x: 1, y: 1 },
+                  { x: 1, y: 1 },
+                ])
+                .into(tableName)
+            ).to.eventually.be.rejectedWith(new RegExp(indexName));
+          });
+        });
+
+        describe('unique table constraint with options object', () => {
+          const tableName = 'test_unique_constraint_options';
+          before(async () => {
+            await knex.schema.createTable(tableName, function () {
+              this.integer('x').notNull();
+              this.integer('y').notNull();
+            });
+          });
+
+          after(async () => {
+            await knex.schema.dropTable(tableName);
+          });
+
+          it('accepts indexName and constraint in options object', async () => {
+            const indexName = `UK_${tableName}_x_y`;
+            await knex.schema.alterTable(tableName, function () {
+              this.unique(['x', 'y'], {
+                indexName: indexName,
+                useConstraint: true,
+              });
+            });
+            await expect(
               knex
                 .insert([
                   { x: 1, y: 1 },
diff --git a/test/unit/schema-builder/mssql.js b/test/unit/schema-builder/mssql.js
index 709ff441..b635bd1c 100644
--- a/test/unit/schema-builder/mssql.js
+++ b/test/unit/schema-builder/mssql.js
@@ -570,6 +570,20 @@ describe('MSSQL SchemaBuilder', function () {
     );
   });
 
+  it('test adding unique constraint', function () {
+    tableSql = client
+      .schemaBuilder()
+      .table('users', function () {
+        this.unique('foo', { indexName: 'bar', useConstraint: true });
+      })
+      .toSQL();
+
+    equal(1, tableSql.length);
+    expect(tableSql[0].sql).to.equal(
+      'ALTER TABLE [users] ADD CONSTRAINT [bar] UNIQUE ([foo])'
+    );
+  });
+
   it('test adding index', function () {
     tableSql = client
       .schemaBuilder()