Repository: knex/knex
Pull request #4639 (2021-08-23 12:44:16)

fix(mssql): Generate valid SQL for withRecursive() [#4514]
mssql forbids use of the RECURSIVE keyword required by some other
dialects.

The approach taken here maximizes subclass reuse and leaking
changes to the query outside `with` generation. If we skipped
restoring the recursive labels using `undoList`, the SQL would
generate fine here, but the test suite fails:
The next-tested driver fails its test, because clone() preserves the
top-level statement's `recursive = false` change, so it's shy one
`RECURSIVE` in the generated SQL.

This addresses the mssql portion of [#4514], but not the oracledb issues.
If this approach to omitting RECURSIVE looks good, I can apply the same to the
oracledb dialect. (Supporting a column alias list as separate argument, as Oracle
requires for recursive CTEs, would require deeper changes.)

jeremy-w: first pass at integration test had errors on a couple platforms i wasn't testing locally:

- postgres: the format of returning must vary across drivers - pg was returning [{id: 1}] rather than just [1]
- mysql: its increments() column is apparently unsigned, and so it refused to let a (signed) integer column reference the id column.

i revised it to omit the keys and fkeys (they weren't the point of the test - just superfluous "realism"), and everything passed the next time around. 👍 

Patch:
diff --git a/lib/dialects/mssql/query/mssql-querycompiler.js b/lib/dialects/mssql/query/mssql-querycompiler.js
index fb29bd2f..2ea5b3fb 100644
--- a/lib/dialects/mssql/query/mssql-querycompiler.js
+++ b/lib/dialects/mssql/query/mssql-querycompiler.js
@@ -31,6 +31,30 @@ class QueryCompiler_MSSQL extends QueryCompiler {
     this._emptyInsertValue = 'default values';
   }
 
+  with() {
+    // WITH RECURSIVE is a syntax error:
+    // SQL Server does not syntactically distinguish recursive and non-recursive CTEs.
+    // 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;
+  }
+
   select() {
     const sql = this.with();
     const statements = components.map((component) => this[component](this));


Test patch:
diff --git a/test/integration/query/selects.js b/test/integration/query/selects.js
index 748394de..ef2a8fd2 100644
--- a/test/integration/query/selects.js
+++ b/test/integration/query/selects.js
@@ -1214,6 +1214,44 @@ module.exports = function (knex) {
         .select('first_name', 'last_name');
     });
 
+    describe('recursive CTE support', function () {
+      before(async function() {
+        await knex.schema.dropTableIfExists('rcte')
+        await knex.schema.createTable('rcte', (table) => {
+          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'
+        for (const name of ['child', 'grandchild']) {
+          await knex('rcte').insert({name, parentName})
+          parentName = name
+        }
+
+        // We will check later that this name is not returned.
+        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', (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')
+      })
+    })
+
     it('supports the <> operator', function () {
       return knex('accounts').where('id', '<>', 2).select('email', 'logins');
     });
diff --git a/test/unit/query/builder.js b/test/unit/query/builder.js
index 3aac961f..e6e45985 100644
--- a/test/unit/query/builder.js
+++ b/test/unit/query/builder.js
@@ -9367,13 +9367,16 @@ describe('QueryBuilder', () => {
         .select('*')
         .from('secondWithClause'),
       {
+        // mssql does not allow the RECURSIVE keyword.
         mssql:
-          'with recursive [firstWithClause] as (with recursive [firstWithSubClause] as ((select [foo] from [users]) as [foz]) select * from [firstWithSubClause]), [secondWithClause] as (with recursive [secondWithSubClause] as ((select [bar] from [users]) as [baz]) select * from [secondWithSubClause]) select * from [secondWithClause]',
+          'with [firstWithClause] as (with [firstWithSubClause] as ((select [foo] from [users]) as [foz]) select * from [firstWithSubClause]), [secondWithClause] as (with [secondWithSubClause] as ((select [bar] from [users]) as [baz]) select * from [secondWithSubClause]) select * from [secondWithClause]',
         sqlite3:
           'with recursive `firstWithClause` as (with recursive `firstWithSubClause` as ((select `foo` from `users`) as `foz`) select * from `firstWithSubClause`), `secondWithClause` as (with recursive `secondWithSubClause` as ((select `bar` from `users`) as `baz`) select * from `secondWithSubClause`) select * from `secondWithClause`',
         pg: 'with recursive "firstWithClause" as (with recursive "firstWithSubClause" as ((select "foo" from "users") as "foz") select * from "firstWithSubClause"), "secondWithClause" as (with recursive "secondWithSubClause" as ((select "bar" from "users") as "baz") select * from "secondWithSubClause") select * from "secondWithClause"',
         'pg-redshift':
           'with recursive "firstWithClause" as (with recursive "firstWithSubClause" as ((select "foo" from "users") as "foz") select * from "firstWithSubClause"), "secondWithClause" as (with recursive "secondWithSubClause" as ((select "bar" from "users") as "baz") select * from "secondWithSubClause") select * from "secondWithClause"',
+        // 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"',
       }