Repository: knex/knex
Pull request #4730 (2021-10-13 06:23:55)

Alter `nullable` constraint when `alterNullable` is set to true
As discussed in the https://github.com/knex/knex/issues/4401 Issue, I created a PR to make it so nullable alterations only apply when `({ alterNullable: true })` is provided as a param to `.alter()` function (which is a default value, since we do not want to break the previous behaviour).

@elhigu please help further :)


Cpaczek: This is also an issue on CockroachDB. See: strapi/strapi#11738 
kibertoad: @OlivierCavadenti this might also look good in 1.0.0
intech: @OlivierCavadenti before doing anything, check out this post https://github.com/strapi/strapi/issues/11738#issuecomment-1007860155
ethan-gallant: Can we get this merged? It's causing quite a headache for various projects depending on knex. 
kibertoad: @ethan-gallant tests are missing and there are conflicts. wiuld you like to help?
OlivierCavadenti: > @OlivierCavadenti before doing anything, check out this post [strapi/strapi#11738 (comment)](https://github.com/strapi/strapi/issues/11738#issuecomment-1007860155)

I can check tomorrow I think. I need to reread the thread to undersant all the problems ^^.

Patch:
diff --git a/lib/dialects/postgres/schema/pg-tablecompiler.js b/lib/dialects/postgres/schema/pg-tablecompiler.js
index 0a29b75a..484b30cb 100644
--- a/lib/dialects/postgres/schema/pg-tablecompiler.js
+++ b/lib/dialects/postgres/schema/pg-tablecompiler.js
@@ -92,14 +92,18 @@ class TableCompiler_PG extends TableCompiler {
     // To alter enum columns they must be cast to text first
     const isEnum = col.type === 'enu';
 
+    const alterNullable = col.columnBuilder.alterNullable;
+
     this.pushQuery({
       sql: `alter table ${quotedTableName} alter column ${colName} drop default`,
       bindings: [],
     });
-    this.pushQuery({
-      sql: `alter table ${quotedTableName} alter column ${colName} drop not null`,
-      bindings: [],
-    });
+    if (alterNullable) {
+      this.pushQuery({
+        sql: `alter table ${quotedTableName} alter column ${colName} drop not null`,
+        bindings: [],
+      });
+    }
     this.pushQuery({
       sql: `alter table ${quotedTableName} alter column ${colName} type ${type} using (${colName}${
         isEnum ? '::text::' : '::'
@@ -116,12 +120,14 @@ class TableCompiler_PG extends TableCompiler {
       });
     }
 
-    const nullable = col.modified['nullable'];
-    if (nullable && nullable[0] === false) {
-      this.pushQuery({
-        sql: `alter table ${quotedTableName} alter column ${colName} set not null`,
-        bindings: [],
-      });
+    if (alterNullable) {
+      const nullable = col.modified['nullable'];
+      if (nullable && nullable[0] === false) {
+        this.pushQuery({
+          sql: `alter table ${quotedTableName} alter column ${colName} set not null`,
+          bindings: [],
+        });
+      }
     }
   }
 
diff --git a/lib/schema/columnbuilder.js b/lib/schema/columnbuilder.js
index 83fffaf3..6c8f605d 100644
--- a/lib/schema/columnbuilder.js
+++ b/lib/schema/columnbuilder.js
@@ -110,8 +110,9 @@ AlterMethods.alterType = function (type) {
 };
 
 // Set column method to alter (default is add).
-AlterMethods.alter = function () {
+AlterMethods.alter = function ({ alterNullable = true } = {}) {
   this._method = 'alter';
+  this.alterNullable = alterNullable;
 
   return this;
 };


Test patch:
diff --git a/test/integration2/schema/misc.spec.js b/test/integration2/schema/misc.spec.js
index e29791d3..05954190 100644
--- a/test/integration2/schema/misc.spec.js
+++ b/test/integration2/schema/misc.spec.js
@@ -556,8 +556,9 @@ describe('Schema (misc)', () => {
               await knex.schema
                 .dropTableIfExists('native_enum_test')
                 .raw('DROP TYPE IF EXISTS "foo_type"')
-                .raw('DROP SCHEMA IF EXISTS "test" CASCADE')
-          }});
+                .raw('DROP SCHEMA IF EXISTS "test" CASCADE');
+            }
+          });
 
           it('uses native type with schema', function () {
             if (!isPgBased(knex)) {
@@ -1710,13 +1711,13 @@ describe('Schema (misc)', () => {
                       .increments('field_nondefault_increments')
                       .comment('Comment on increment col');
                   })
-                )
+                );
             }
           });
 
           after(() => {
             if (isMysql(knex)) {
-              knex.schema.dropTable('add_column_test_mysql')
+              knex.schema.dropTable('add_column_test_mysql');
             }
           });
 
@@ -2230,7 +2231,7 @@ describe('Schema (misc)', () => {
               });
           });
 
-          it('should throw when referencing a non-existent column', function() {
+          it('should throw when referencing a non-existent column', function () {
             if (!isSQLite(knex)) {
               return this.skip();
             }
diff --git a/test/integration2/schema/set-nullable.spec.js b/test/integration2/schema/set-nullable.spec.js
index d73a160b..d7357b17 100644
--- a/test/integration2/schema/set-nullable.spec.js
+++ b/test/integration2/schema/set-nullable.spec.js
@@ -74,6 +74,43 @@ describe('Schema', () => {
               id_not_nullable: null,
             });
           });
+
+          it('should throw error if alter a not nullable column with primary key #4401', async function () {
+            if (!isPostgreSQL(knex)) {
+              this.skip();
+            }
+            await knex.schema.dropTableIfExists('primary_table_null');
+            await knex.schema.createTable('primary_table_null', (table) => {
+              table.integer('id_not_nullable_primary').notNullable().primary();
+            });
+            let error;
+            try {
+              await knex.schema.table('primary_table_null', (table) => {
+                table.integer('id_not_nullable_primary').alter();
+              });
+            } catch (e) {
+              error = e;
+            }
+            expect(error.message).to.eq(
+              'alter table "primary_table_null" alter column "id_not_nullable_primary" drop not null - column "id_not_nullable_primary" is in a primary key'
+            );
+          });
+
+          it('should not throw error if alter a not nullable column with primary key with alterNullable is false #4401', async function () {
+            if (!isPostgreSQL(knex)) {
+              this.skip();
+            }
+            await knex.schema.dropTableIfExists('primary_table_null');
+            await knex.schema.createTable('primary_table_null', (table) => {
+              table.integer('id_not_nullable_primary').notNullable().primary();
+            });
+            // This alter doesn't throw any error now.
+            await knex.schema.table('primary_table_null', (table) => {
+              table
+                .integer('id_not_nullable_primary')
+                .alter({ alterNullable: false });
+            });
+          });
         });
 
         describe('dropNullable', () => {
diff --git a/test/unit/schema-builder/postgres.js b/test/unit/schema-builder/postgres.js
index 951d3be5..56c1e6eb 100644
--- a/test/unit/schema-builder/postgres.js
+++ b/test/unit/schema-builder/postgres.js
@@ -187,6 +187,49 @@ describe('PostgreSQL SchemaBuilder', function () {
     );
   });
 
+  it('should not alter nullable when alterNullable is false', function () {
+    tableSql = client
+      .schemaBuilder()
+      .table('users', function () {
+        this.string('foo').default('foo').alter({ alterNullable: false });
+      })
+      .toSQL();
+
+    equal(3, tableSql.length);
+    expect(tableSql[0].sql).to.equal(
+      'alter table "users" alter column "foo" drop default'
+    );
+    expect(tableSql[1].sql).to.equal(
+      'alter table "users" alter column "foo" type varchar(255) using ("foo"::varchar(255))'
+    );
+    expect(tableSql[2].sql).to.equal(
+      'alter table "users" alter column "foo" set default \'foo\''
+    );
+  });
+
+  it('should alter nullable when alterNullable is true', function () {
+    tableSql = client
+      .schemaBuilder()
+      .table('users', function () {
+        this.string('foo').default('foo').alter({ alterNullable: true });
+      })
+      .toSQL();
+
+    equal(4, tableSql.length);
+    expect(tableSql[0].sql).to.equal(
+      'alter table "users" alter column "foo" drop default'
+    );
+    expect(tableSql[1].sql).to.equal(
+      'alter table "users" alter column "foo" drop not null'
+    );
+    expect(tableSql[2].sql).to.equal(
+      'alter table "users" alter column "foo" type varchar(255) using ("foo"::varchar(255))'
+    );
+    expect(tableSql[3].sql).to.equal(
+      'alter table "users" alter column "foo" set default \'foo\''
+    );
+  });
+
   it('alter table with schema', function () {
     tableSql = client
       .schemaBuilder()