Repository: knex/knex
Pull request #4967 (2022-01-25 08:27:43)

Add alterType and update index.d.ts for alter function
- Fix https://github.com/knex/knex/pull/4730/files#r791157891
- And I think we are good for an 1.0.2 (doc here : https://github.com/knex/documentation/pull/385)

OlivierCavadenti: @intech do you think I can do something for cockroachdb test ? Or I need to revert for now ?
intech: > @intech do you think I can do something for cockroachdb test ? Or I need to revert for now ?

Quickly studied the error and can not say how to get around it. I'll ask the cockroachdb engineers for advice and get back.


Patch:
diff --git a/lib/dialects/mysql/query/mysql-querycompiler.js b/lib/dialects/mysql/query/mysql-querycompiler.js
index dfcb086b..52dd18b2 100644
--- a/lib/dialects/mysql/query/mysql-querycompiler.js
+++ b/lib/dialects/mysql/query/mysql-querycompiler.js
@@ -80,12 +80,14 @@ class QueryCompiler_MySQL extends QueryCompiler {
 
   // Update method, including joins, wheres, order & limits.
   update() {
+    const withSQL = this.with();
     const join = this.join();
     const updates = this._prepUpdate(this.single.update);
     const where = this.where();
     const order = this.order();
     const limit = this.limit();
     return (
+      withSQL +
       `update ${this.tableName}` +
       (join ? ` ${join}` : '') +
       ' set ' +
diff --git a/lib/dialects/postgres/schema/pg-tablecompiler.js b/lib/dialects/postgres/schema/pg-tablecompiler.js
index 484b30cb..3bb2d7d9 100644
--- a/lib/dialects/postgres/schema/pg-tablecompiler.js
+++ b/lib/dialects/postgres/schema/pg-tablecompiler.js
@@ -91,25 +91,28 @@ 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: [],
     });
+
+    const alterNullable = col.columnBuilder.alterNullable;
     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::' : '::'
-      }${type})`,
-      bindings: [],
-    });
+
+    const alterType = col.columnBuilder.alterType;
+    if (alterType) {
+      this.pushQuery({
+        sql: `alter table ${quotedTableName} alter column ${colName} type ${type} using (${colName}${
+          isEnum ? '::text::' : '::'
+        }${type})`,
+        bindings: [],
+      });
+    }
 
     const defaultTo = col.modified['defaultTo'];
     if (defaultTo) {
diff --git a/lib/schema/columnbuilder.js b/lib/schema/columnbuilder.js
index 6c8f605d..685872c0 100644
--- a/lib/schema/columnbuilder.js
+++ b/lib/schema/columnbuilder.js
@@ -110,9 +110,13 @@ AlterMethods.alterType = function (type) {
 };
 
 // Set column method to alter (default is add).
-AlterMethods.alter = function ({ alterNullable = true } = {}) {
+AlterMethods.alter = function ({
+  alterNullable = true,
+  alterType = true,
+} = {}) {
   this._method = 'alter';
   this.alterNullable = alterNullable;
+  this.alterType = alterType;
 
   return this;
 };
diff --git a/types/index.d.ts b/types/index.d.ts
index 6ce9b12e..11fe6c72 100644
--- a/types/index.d.ts
+++ b/types/index.d.ts
@@ -2210,7 +2210,7 @@ export declare namespace Knex {
     notNullable(): ColumnBuilder;
     nullable(): ColumnBuilder;
     comment(value: string): ColumnBuilder;
-    alter(): ColumnBuilder;
+    alter(options: Readonly<{alterNullable?: boolean, alterType?: boolean}>): ColumnBuilder;
     queryContext(context: any): ColumnBuilder;
     after(columnName: string): ColumnBuilder;
     first(): ColumnBuilder;


Test patch:
diff --git a/test/integration2/query/select/selects.spec.js b/test/integration2/query/select/selects.spec.js
index 57661ed5..4fe4c881 100644
--- a/test/integration2/query/select/selects.spec.js
+++ b/test/integration2/query/select/selects.spec.js
@@ -32,6 +32,7 @@ const {
 const {
   assertNumberArrayStrict,
   assertJsonEquals,
+  assertNumber,
 } = require('../../../util/assertHelper');
 const {
   getAllDbs,
@@ -1255,31 +1256,42 @@ describe('Selects', function () {
           );
       });
 
-      describe('with (not) materialized tests', () => {
-        before(async function () {
-          if (!isPostgreSQL(knex) && !isSQLite(knex)) {
-            return this.skip();
-          }
-          await knex('test_default_table').truncate();
-          await knex('test_default_table').insert([
-            { string: 'something', tinyint: 1 },
-          ]);
+      describe('"with" tests', () => {
+        beforeEach(async () => {
+          await knex.schema
+            .dropTableIfExists('with_table')
+            .createTable('with_table', function (table) {
+              table.integer('test');
+            });
+          await knex('with_table').insert([{ test: 1 }]);
+        });
+
+        it('with', async () => {
+          const withQuery = await knex
+            .with('t', knex('with_table'))
+            .from('t')
+            .first();
+          assertNumber(knex, withQuery.test, 1);
         });
 
         it('with materialized', async function () {
+          if (!isPostgreSQL(knex) && !isSQLite(knex)) {
+            return this.skip();
+          }
           const materialized = await knex('t')
-            .withMaterialized('t', knex('test_default_table'))
-            .from('t')
+            .withMaterialized('t', knex('with_table'))
             .first();
-          expect(materialized.tinyint).to.equal(1);
+          assertNumber(knex, materialized.test, 1);
         });
 
         it('with not materialized', async function () {
+          if (!isPostgreSQL(knex) && !isSQLite(knex)) {
+            return this.skip();
+          }
           const notMaterialized = await knex('t')
-            .withNotMaterialized('t', knex('test_default_table'))
-            .from('t')
+            .withNotMaterialized('t', knex('with_table'))
             .first();
-          expect(notMaterialized.tinyint).to.equal(1);
+          assertNumber(knex, notMaterialized.test, 1);
         });
       });
 
diff --git a/test/integration2/query/update/updates.spec.js b/test/integration2/query/update/updates.spec.js
index 6a71d313..53a49267 100644
--- a/test/integration2/query/update/updates.spec.js
+++ b/test/integration2/query/update/updates.spec.js
@@ -407,6 +407,22 @@ describe('Updates', function () {
             );
           });
       });
+
+      it('with update query', async function () {
+        await knex
+          .with('withClause', function () {
+            this.select('last_name')
+              .from('accounts')
+              .where('email', '=', 'test1@example.com');
+          })
+          .update({ last_name: 'olivier' })
+          .where('last_name', '=', 'User')
+          .from('accounts');
+        const results = await knex('accounts')
+          .from('accounts')
+          .where('email', '=', 'test1@example.com');
+        expect(results[0].last_name).to.equal('olivier');
+      });
     });
   });
 });
diff --git a/test/integration2/schema/misc.spec.js b/test/integration2/schema/misc.spec.js
index 05954190..0b640da4 100644
--- a/test/integration2/schema/misc.spec.js
+++ b/test/integration2/schema/misc.spec.js
@@ -1694,6 +1694,7 @@ describe('Schema (misc)', () => {
           before(() => {
             if (isMysql(knex)) {
               return knex.schema
+                .dropTableIfExists('add_column_test_mysql')
                 .createTable('add_column_test_mysql', (tbl) => {
                   tbl.integer('field_foo');
                   tbl.integer('field_bar');
diff --git a/test/integration2/schema/set-nullable.spec.js b/test/integration2/schema/set-nullable.spec.js
index d7357b17..a1a7d49e 100644
--- a/test/integration2/schema/set-nullable.spec.js
+++ b/test/integration2/schema/set-nullable.spec.js
@@ -8,6 +8,7 @@ const {
   isMysql,
   isBetterSQLite3,
   isOracle,
+  isCockroachDB,
 } = require('../../util/db-helpers');
 const { getAllDbs, getKnexForDb } = require('../util/knex-instance-provider');
 
@@ -76,7 +77,7 @@ describe('Schema', () => {
           });
 
           it('should throw error if alter a not nullable column with primary key #4401', async function () {
-            if (!isPostgreSQL(knex)) {
+            if (!(isPostgreSQL(knex) || isCockroachDB(knex))) {
               this.skip();
             }
             await knex.schema.dropTableIfExists('primary_table_null');
@@ -91,15 +92,25 @@ describe('Schema', () => {
             } 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'
-            );
+            if (isCockroachDB(knex)) {
+              // TODO: related comment in issue https://github.com/cockroachdb/cockroach/issues/49329#issuecomment-1022120446
+              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 index'
+              );
+            } else {
+              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)) {
+            // TODO: related issue for cockroach https://github.com/cockroachdb/cockroach/issues/47636
+            if (!(isPostgreSQL(knex) || isCockroachDB(knex))) {
               this.skip();
             }
+            // With CoackroachDb we don't alter type (not supported).
+            const alterType = isPostgreSQL(knex);
             await knex.schema.dropTableIfExists('primary_table_null');
             await knex.schema.createTable('primary_table_null', (table) => {
               table.integer('id_not_nullable_primary').notNullable().primary();
@@ -108,7 +119,7 @@ describe('Schema', () => {
             await knex.schema.table('primary_table_null', (table) => {
               table
                 .integer('id_not_nullable_primary')
-                .alter({ alterNullable: false });
+                .alter({ alterNullable: false, alterType: alterType });
             });
           });
         });
diff --git a/test/unit/query/builder.js b/test/unit/query/builder.js
index b32e1344..6fbf1640 100644
--- a/test/unit/query/builder.js
+++ b/test/unit/query/builder.js
@@ -9486,7 +9486,6 @@ describe('QueryBuilder', () => {
       }
     );
   });
-
   it("wrapped 'with' clause update", () => {
     testsql(
       qb()
@@ -9497,6 +9496,8 @@ describe('QueryBuilder', () => {
         .where('email', '=', 'foo')
         .from('users'),
       {
+        mysql:
+          'with `withClause` as (select `foo` from `users`) update `users` set `foo` = ? where `email` = ?',
         mssql:
           'with [withClause] as (select [foo] from [users]) update [users] set [foo] = ? where [email] = ?;select @@rowcount',
         sqlite3:
diff --git a/test/unit/schema-builder/postgres.js b/test/unit/schema-builder/postgres.js
index 56c1e6eb..2361781c 100644
--- a/test/unit/schema-builder/postgres.js
+++ b/test/unit/schema-builder/postgres.js
@@ -230,6 +230,26 @@ describe('PostgreSQL SchemaBuilder', function () {
     );
   });
 
+  it('should not alter type when alterType is false', function () {
+    tableSql = client
+      .schemaBuilder()
+      .table('users', function () {
+        this.string('foo').default('foo').alter({ alterType: 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" drop not null'
+    );
+    expect(tableSql[2].sql).to.equal(
+      'alter table "users" alter column "foo" set default \'foo\''
+    );
+  });
+
   it('alter table with schema', function () {
     tableSql = client
       .schemaBuilder()