Repository: knex/knex
Pull request #4652 (2021-08-27 01:29:05)

fix(oracledb): withRecursive: omit invalid RECURSIVE keyword, include column list [#4514]
Before, the Oracle query compiler included an erroneous RECURSIVE keyword, and you could not include a column list, as required in Oracle SQL when writing an rCTE.

Now, both are fixed.

# TODO
- [x] Agree `with` args validation function with how they are used in `withWrapped`. Right now you could pass validation but fail during usage, because I'm not interpreting the 2 vs 3 arg versions the same in both places.
- [ ] Open docs PR with corresponding changes

kibertoad: @jeremy-w API proposed in https://github.com/knex/knex/issues/4514 looks sensible. Do we need anything beyond that?
jeremy-w: That should do it. It’s the three-arg variant with a column array in between the alias & builder/raw/callback variant I included in the list. Just wanted confirmation on the intended API changes before proceeding with QueryBuilder changes. Thanks!
jeremy-w: The integration test for oracledb is failing with master (b5225179ce8760c61418b4bd1c2cbb1a742c662a) for me right now:

```
  320 passing (24s)
  47 pending
  2 failing

  1) Schema
       alter nullable
         oracledb
           setNullable
             sets column to be nullable:
     Error: SELECT 1 - ORA-00923: FROM keyword not found where expected
  

  2) Schema
       alter nullable
         oracledb
           dropNullable
             sets column to be not nullable:
     Error: SELECT 1 - ORA-00923: FROM keyword not found where expected
```

I ran the integration test suite with my changes, and they do not introduce any further breakage. 😬 

Patch:
diff --git a/lib/dialects/oracledb/index.js b/lib/dialects/oracledb/index.js
index daa8d921..0e18ef38 100644
--- a/lib/dialects/oracledb/index.js
+++ b/lib/dialects/oracledb/index.js
@@ -10,6 +10,7 @@ const values = require('lodash/values');
 
 const Formatter = require('../../formatter');
 const QueryCompiler = require('./query/oracledb-querycompiler');
+const TableCompiler = require('./schema/oracledb-tablecompiler');
 const ColumnCompiler = require('./schema/oracledb-columncompiler');
 const { BlobHelper, ReturningHelper, isConnectionError } = require('./utils');
 const Transaction = require('./transaction');
@@ -58,6 +59,10 @@ class Client_Oracledb extends Client_Oracle {
     return new QueryCompiler(this, builder, formatter);
   }
 
+  tableCompiler() {
+    return new TableCompiler(this, ...arguments);
+  }
+
   columnCompiler() {
     return new ColumnCompiler(this, ...arguments);
   }
diff --git a/lib/dialects/oracledb/query/oracledb-querycompiler.js b/lib/dialects/oracledb/query/oracledb-querycompiler.js
index 0259bae4..05589953 100644
--- a/lib/dialects/oracledb/query/oracledb-querycompiler.js
+++ b/lib/dialects/oracledb/query/oracledb-querycompiler.js
@@ -201,6 +201,29 @@ class Oracledb_Compiler extends Oracle_Compiler {
     return sql;
   }
 
+  with() {
+    // WITH RECURSIVE is a syntax error in Oracle SQL.
+    // So mark all statements as non-recursive, generate the SQL, then restore.
+    // This approach ensures any changes in base class with() get propagated here.
+    const undoList = [];
+    if (this.grouped.with) {
+      for (const stmt of this.grouped.with) {
+        if (stmt.recursive) {
+          undoList.push(stmt);
+          stmt.recursive = false;
+        }
+      }
+    }
+
+    const result = super.with();
+
+    // Restore the recursive markings, in case this same query gets cloned and passed to other drivers.
+    for (const stmt of undoList) {
+      stmt.recursive = true;
+    }
+    return result;
+  }
+
   _addReturningToSqlAndConvert(sql, outBinding, tableName, returning) {
     const self = this;
     const res = {
diff --git a/lib/dialects/oracledb/schema/oracledb-tablecompiler.js b/lib/dialects/oracledb/schema/oracledb-tablecompiler.js
new file mode 100644
index 00000000..4f7d5fa9
--- /dev/null
+++ b/lib/dialects/oracledb/schema/oracledb-tablecompiler.js
@@ -0,0 +1,19 @@
+const TableCompiler_Oracle = require('../../oracle/schema/oracle-tablecompiler');
+
+class TableCompiler_Oracledb extends TableCompiler_Oracle {
+  constructor(client, tableBuilder) {
+    super(client, tableBuilder);
+  }
+
+  _setNullableState(column, isNullable) {
+    const nullability = isNullable ? 'NULL' : 'NOT NULL';
+    const sql = `alter table ${this.tableName()} modify (${this.formatter.wrap(
+      column
+    )} ${nullability})`;
+    return this.pushQuery({
+      sql: sql,
+    });
+  }
+}
+
+module.exports = TableCompiler_Oracledb;
diff --git a/lib/query/querybuilder.js b/lib/query/querybuilder.js
index 7b7e76ef..8a8dd1f1 100644
--- a/lib/query/querybuilder.js
+++ b/lib/query/querybuilder.js
@@ -114,17 +114,22 @@ class Builder extends EventEmitter {
   // With
   // ------
 
-  with(alias, statement) {
-    validateWithArgs(alias, statement, 'with');
-    return this.withWrapped(alias, statement);
+  with(alias, statementOrColumnList, nothingOrStatement) {
+    validateWithArgs(alias, statementOrColumnList, nothingOrStatement, 'with');
+    return this.withWrapped(alias, statementOrColumnList, nothingOrStatement);
   }
 
   // Helper for compiling any advanced `with` queries.
-  withWrapped(alias, query) {
+  withWrapped(alias, statementOrColumnList, nothingOrStatement) {
+    const [query, columnList] =
+      typeof nothingOrStatement === 'undefined'
+        ? [statementOrColumnList, undefined]
+        : [nothingOrStatement, statementOrColumnList];
     this._statements.push({
       grouping: 'with',
       type: 'withWrapped',
       alias: alias,
+      columnList,
       value: query,
     });
     return this;
@@ -133,14 +138,23 @@ class Builder extends EventEmitter {
   // With Recursive
   // ------
 
-  withRecursive(alias, statement) {
-    validateWithArgs(alias, statement, 'withRecursive');
-    return this.withRecursiveWrapped(alias, statement);
+  withRecursive(alias, statementOrColumnList, nothingOrStatement) {
+    validateWithArgs(
+      alias,
+      statementOrColumnList,
+      nothingOrStatement,
+      'withRecursive'
+    );
+    return this.withRecursiveWrapped(
+      alias,
+      statementOrColumnList,
+      nothingOrStatement
+    );
   }
 
   // Helper for compiling any advanced `withRecursive` queries.
-  withRecursiveWrapped(alias, query) {
-    this.withWrapped(alias, query);
+  withRecursiveWrapped(alias, statementOrColumnList, nothingOrStatement) {
+    this.withWrapped(alias, statementOrColumnList, nothingOrStatement);
     this._statements[this._statements.length - 1].recursive = true;
     return this;
   }
@@ -1392,19 +1406,46 @@ class Builder extends EventEmitter {
   }
 }
 
-const validateWithArgs = function (alias, statement, method) {
+const isValidStatementArg = (statement) =>
+  typeof statement === 'function' ||
+  statement instanceof Builder ||
+  (statement && statement.isRawInstance);
+
+const validateWithArgs = function (
+  alias,
+  statementOrColumnList,
+  nothingOrStatement,
+  method
+) {
+  const [query, columnList] =
+      typeof nothingOrStatement === 'undefined'
+        ? [statementOrColumnList, undefined]
+        : [nothingOrStatement, statementOrColumnList];
   if (typeof alias !== 'string') {
     throw new Error(`${method}() first argument must be a string`);
   }
-  if (
-    typeof statement === 'function' ||
-    statement instanceof Builder ||
-    (statement && statement.isRawInstance)
-  ) {
+
+  if (isValidStatementArg(query) && typeof columnList === 'undefined') {
+    // Validated as two-arg variant (alias, statement).
+    return;
+  }
+
+  // Attempt to interpret as three-arg variant (alias, columnList, statement).
+  const isNonEmptyNameList =
+    Array.isArray(columnList) &&
+    columnList.length > 0 &&
+    columnList.every((it) => typeof it === 'string');
+  if (!isNonEmptyNameList) {
+    throw new Error(
+      `${method}() second argument must be a statement or non-empty column name list.`
+    );
+  }
+
+  if (isValidStatementArg(query)) {
     return;
   }
   throw new Error(
-    `${method}() second argument must be a function / QueryBuilder or a raw`
+    `${method}() third argument must be a function / QueryBuilder or a raw when its second argument is a column name list`
   );
 };
 
diff --git a/lib/query/querycompiler.js b/lib/query/querycompiler.js
index e446b3b8..02090c8d 100644
--- a/lib/query/querycompiler.js
+++ b/lib/query/querycompiler.js
@@ -1087,6 +1087,16 @@ class QueryCompiler {
       this.client,
       this.bindingsHolder
     );
+    const columnList = statement.columnList
+      ? '(' +
+        columnize_(
+          statement.columnList,
+          this.builder,
+          this.client,
+          this.bindingsHolder
+        ) +
+        ')'
+      : '';
     return (
       (val &&
         columnize_(
@@ -1095,6 +1105,7 @@ class QueryCompiler {
           this.client,
           this.bindingsHolder
         ) +
+          columnList +
           ' as (' +
           val +
           ')') ||
diff --git a/package.json b/package.json
index 02dd4e61..b9b7dbe0 100644
--- a/package.json
+++ b/package.json
@@ -26,6 +26,7 @@
     "test:mssql": "cross-env DB=mssql npm run test:db",
     "test:mysql": "cross-env DB=mysql npm run test:db",
     "test:mysql2": "cross-env DB=mysql2 npm run test:db",
+    "test:oracledb": "cross-env DB=oracledb npm run test:db",
     "test:sqlite": "cross-env DB=sqlite3 npm run test:db",
     "test:postgres": "cross-env DB=postgres npm run test:db",
     "test:tape": "node test/tape/index.js | tap-spec",
@@ -37,6 +38,8 @@
     "db:stop:postgres": "docker-compose -f scripts/docker-compose.yml down",
     "db:start:mysql": "docker-compose -f scripts/docker-compose.yml up --build -d mysql && docker-compose -f scripts/docker-compose.yml up waitmysql",
     "db:stop:mysql": "docker-compose -f scripts/docker-compose.yml down",
+    "db:start:oracle": "docker-compose -f scripts/docker-compose.yml up --build -d oracledbxe && docker-compose -f scripts/docker-compose.yml up waitoracledbxe",
+    "db:stop:oracle": "docker-compose -f scripts/docker-compose.yml down",
     "stress:init": "docker-compose -f scripts/stress-test/docker-compose.yml up --no-start && docker-compose -f scripts/stress-test/docker-compose.yml start",
     "stress:test": "node scripts/stress-test/knex-stress-test.js | grep -A 5 -B 60 -- '- STATS '",
     "stress:destroy": "docker-compose -f scripts/stress-test/docker-compose.yml stop"
diff --git a/types/index.d.ts b/types/index.d.ts
index a33841a5..eeb3502c 100644
--- a/types/index.d.ts
+++ b/types/index.d.ts
@@ -1275,6 +1275,11 @@ export declare namespace Knex {
       TRecord,
       TResult
     >;
+    (alias: string, columnList: string[], raw: Raw | QueryBuilder): QueryBuilder<TRecord, TResult>;
+    (alias: string, columnList: string[], sql: string, bindings?: readonly Value[] | Object): QueryBuilder<
+      TRecord,
+      TResult
+    >;
   }
 
   interface WithSchema<TRecord = any, TResult = unknown[]> {
@@ -1287,6 +1292,12 @@ export declare namespace Knex {
       alias: string,
       callback: (queryBuilder: QueryBuilder) => any
     ): QueryBuilder<TRecord, TResult>;
+    (alias: string, columnList: string[], queryBuilder: QueryBuilder): QueryBuilder<TRecord, TResult>;
+    (
+      alias: string,
+      columnList: string[],
+      callback: (queryBuilder: QueryBuilder) => any
+    ): QueryBuilder<TRecord, TResult>;
   }
 
   interface Where<TRecord = any, TResult = unknown>


Test patch:
diff --git a/test-tsd/types.test-d.ts b/test-tsd/types.test-d.ts
index 41dcaeb1..0732e6ff 100644
--- a/test-tsd/types.test-d.ts
+++ b/test-tsd/types.test-d.ts
@@ -115,3 +115,36 @@ expectAssignable<QueryBuilder>(
     .merge(['active', 'id'])
     .debug(true)
 );
+
+knexInstance.withWrapped('qb', knexInstance.select('column').from('table'))
+knexInstance.withWrapped('callback', (qb) => qb.select('column').from('table'))
+
+knexInstance.withWrapped('columnList+qb', ['columnName'], (qb) => qb.select('column').from('table'))
+knexInstance.withWrapped('columnList+callback', ['columnName'], (qb) => qb.select('column').from('table'))
+
+// FIXME: The withRaw function does not exist any more. with handles raw directly now.
+knexInstance.withRaw('raw', knexInstance.raw('raw'))
+knexInstance.withRaw('sql', 'just sql')
+knexInstance.withRaw('sql+bindingsObj', 'sql with named bindings', {x: 1})
+knexInstance.withRaw('sql+bindingsArr', 'sql with positional bindings', [1])
+
+knexInstance.withRaw('columnList+raw', ['columnName'], knexInstance.raw('raw'))
+knexInstance.withRaw('columnList+sql', ['columnName'], 'just sql')
+knexInstance.withRaw('columnList+sql+bindingsObj', ['columnName'], 'sql with named bindings', {x: 1})
+knexInstance.withRaw('columnList+sql+bindingsArr', ['columnName'], 'sql with positional bindings', [1])
+
+// the With type is used both for with and withRecursive. With extends both withWrapped and withRaw, so should support all the same variants:
+// those inherited from WithWrapped
+knexInstance.with('qb', knexInstance.select('column').from('table'))
+knexInstance.with('callback', (qb) => qb.select('column').from('table'))
+knexInstance.with('columnList+qb', ['columnName'], (qb) => qb.select('column').from('table'))
+knexInstance.with('columnList+callback', ['columnName'], (qb) => qb.select('column').from('table'))
+// those inherited from withRaw
+knexInstance.with('raw', knexInstance.raw('raw'))
+knexInstance.with('sql', 'just sql')
+knexInstance.with('sql+bindingsObj', 'sql with named bindings', {x: 1})
+knexInstance.with('sql+bindingsArr', 'sql with positional bindings', [1])
+knexInstance.with('columnList+raw', ['columnName'], knexInstance.raw('raw'))
+knexInstance.with('columnList+sql', ['columnName'], 'just sql')
+knexInstance.with('columnList+sql+bindingsObj', ['columnName'], 'sql with named bindings', {x: 1})
+knexInstance.with('columnList+sql+bindingsArr', ['columnName'], 'sql with positional bindings', [1])
diff --git a/test/integration/query/selects.js b/test/integration/query/selects.js
index ef2a8fd2..3bcd0665 100644
--- a/test/integration/query/selects.js
+++ b/test/integration/query/selects.js
@@ -1215,42 +1215,52 @@ module.exports = function (knex) {
     });
 
     describe('recursive CTE support', function () {
-      before(async function() {
-        await knex.schema.dropTableIfExists('rcte')
+      before(async function () {
+        await knex.schema.dropTableIfExists('rcte');
         await knex.schema.createTable('rcte', (table) => {
-          table.string('name')
-          table.string('parentName').nullable()
-        })
+          table.string('name');
+          table.string('parentName').nullable();
+        });
 
         // We will check later that this name was found by chaining up parentId using an rCTE.
-        await knex('rcte').insert({name: 'parent'})
-        let parentName = 'parent'
+        await knex('rcte').insert({ name: 'parent' });
+        let parentName = 'parent';
         for (const name of ['child', 'grandchild']) {
-          await knex('rcte').insert({name, parentName})
-          parentName = name
+          await knex('rcte').insert({ name, parentName });
+          parentName = name;
         }
 
         // We will check later that this name is not returned.
-        await knex('rcte').insert({name: 'nope'})
-      })
+        await knex('rcte').insert({ name: 'nope' });
+      });
       it('supports recursive CTEs', async function () {
-        // FIXME: Oracle requires to omit RECURSIVE. [#4514]
-        if (isOracle(knex)) {
-          return this.skip()
-        }
+        const results = await knex
+          .withRecursive('family', ['name', 'parentName'], (qb) => {
+            qb.select('name', 'parentName')
+              .from('rcte')
+              .where({ name: 'grandchild' })
+              .unionAll((qb) =>
+                qb
+                  .select('rcte.name', 'rcte.parentName')
+                  .from('rcte')
+                  .join(
+                    'family',
+                    knex.ref('family.parentName'),
+                    knex.ref('rcte.name')
+                  )
+              );
+          })
+          .select('name')
+          .from('family');
+        const names = results.map(({ name }) => name);
 
-        const results = await knex.withRecursive('family', (qb) => {
-          qb.select('name', 'parentName').from('rcte').where({name: 'grandchild'}).unionAll(
-            (qb) => qb.select('rcte.name', 'rcte.parentName').from('rcte').join('family', knex.ref('family.parentName'), knex.ref('rcte.name'))
-          )
-        }).select('name').from('family')
-        const names = results.map(({name}) => name)
-
-        expect(names).to.have.length('parent child grandchild'.split(' ').length)
-        expect(names).to.contain('parent')
-        expect(names).not.to.contain('nope')
-      })
-    })
+        expect(names).to.have.length(
+          'parent child grandchild'.split(' ').length
+        );
+        expect(names).to.contain('parent');
+        expect(names).not.to.contain('nope');
+      });
+    });
 
     it('supports the <> operator', function () {
       return knex('accounts').where('id', '<>', 2).select('email', 'logins');
diff --git a/test/integration2/schema/set-nullable.spec.js b/test/integration2/schema/set-nullable.spec.js
index 612a63b3..4061b279 100644
--- a/test/integration2/schema/set-nullable.spec.js
+++ b/test/integration2/schema/set-nullable.spec.js
@@ -1,7 +1,12 @@
 const chai = require('chai');
 chai.use(require('chai-as-promised'));
 const expect = chai.expect;
-const { isSQLite, isPostgreSQL, isMysql } = require('../../util/db-helpers');
+const {
+  isSQLite,
+  isPostgreSQL,
+  isMysql,
+  isOracle,
+} = require('../../util/db-helpers');
 const { getAllDbs, getKnexForDb } = require('../util/knex-instance-provider');
 
 describe('Schema', () => {
@@ -52,11 +57,12 @@ describe('Schema', () => {
             });
 
             let errorMessage;
-            if (isPostgreSQL()) {
+            if (isPostgreSQL(knex)) {
               errorMessage = 'violates not-null constraint';
-            }
-            if (isMysql()) {
+            } else if (isMysql(knex)) {
               errorMessage = 'cannot be null';
+            } else if (isOracle(knex)) {
+              errorMessage = 'ORA-01400: cannot insert NULL into';
             }
 
             await expect(
diff --git a/test/unit/query/builder.js b/test/unit/query/builder.js
index a240cc82..e364f9c2 100644
--- a/test/unit/query/builder.js
+++ b/test/unit/query/builder.js
@@ -9401,7 +9401,28 @@ describe('QueryBuilder', () => {
         // FIXME: oracledb does not allow the RECURSIVE keyword, but does require a list of column aliases for a recursive query. [#4514]
         // https://github.com/knex/knex/issues/4514#issuecomment-903727391
         oracledb:
-          'with recursive "firstWithClause" as (with recursive "firstWithSubClause" as ((select "foo" from "users") "foz") select * from "firstWithSubClause"), "secondWithClause" as (with recursive "secondWithSubClause" as ((select "bar" from "users") "baz") select * from "secondWithSubClause") select * from "secondWithClause"',
+          'with "firstWithClause" as (with "firstWithSubClause" as ((select "foo" from "users") "foz") select * from "firstWithSubClause"), "secondWithClause" as (with "secondWithSubClause" as ((select "bar" from "users") "baz") select * from "secondWithSubClause") select * from "secondWithClause"',
+      }
+    );
+  });
+
+  it('Oracle: withRecursive with column list', function () {
+    testsql(
+      qb()
+        .withRecursive('hasColumns', ['id', 'nickname'], function () {
+          this.select('id', 'nickname')
+            .from('users')
+            .unionAll(function () {
+              this.select('id', 'firstname')
+                .from('users')
+                .join('hasColumns', 'hasColumns.nickname', 'users.firstname');
+            });
+        })
+        .select('name')
+        .from('hasColumns'),
+      {
+        oracledb:
+          'with "hasColumns"("id", "nickname") as (select "id", "nickname" from "users" union all select "id", "firstname" from "users" inner join "hasColumns" on "hasColumns"."nickname" = "users"."firstname") select "name" from "hasColumns"',
       }
     );
   });