Repository: knex/knex
Pull request #4934 (2022-01-14 20:59:42)

Fix tests for RETURNING and enable them for SQLite.
Functionality doesn't seem to work yet

kibertoad: @OlivierCavadenti Could you take a look at this and see if I'm missing something and there is actually a way to get necessary data from SQLite? The way I see it, 
`connection[callMethod](obj.sql, obj.bindings, function (err, response) {`
part of SQLite dialect is returning `undefined` response and we don't actually get anything back. Wonder if we need to do something additionally to make SQLite to return data for inserts.
kibertoad: Related ticket: https://github.com/microsoft/vscode-node-sqlite3/issues/15
OlivierCavadenti: > @OlivierCavadenti Could you take a look at this and see if I'm missing something and there is actually a way to get necessary data from SQLite? The way I see it, `connection[callMethod](obj.sql, obj.bindings, function (err, response) {` part of SQLite dialect is returning `undefined` response and we don't actually get anything back. Wonder if we need to do something additionally to make SQLite to return data for inserts.

Like you I made some tests and I dont find anything to get back the results... maybe something missing in the library.
kibertoad: @OlivierCavadenti I will restore warnings for 1.0.0 then, and hopefully we'll get some response from vscode sqlite team
I'll also try with better-sqlite, maybe that one works.

Patch:
diff --git a/lib/dialects/postgres/query/pg-querycompiler.js b/lib/dialects/postgres/query/pg-querycompiler.js
index 3e1dac3b..779ce5c3 100644
--- a/lib/dialects/postgres/query/pg-querycompiler.js
+++ b/lib/dialects/postgres/query/pg-querycompiler.js
@@ -40,7 +40,7 @@ class QueryCompiler_PG extends QueryCompiler {
     if (returning) sql += this._returning(returning);
 
     return {
-      sql: sql,
+      sql,
       returning,
     };
   }
diff --git a/lib/dialects/sqlite3/index.js b/lib/dialects/sqlite3/index.js
index 82653b8c..0fed4b0f 100644
--- a/lib/dialects/sqlite3/index.js
+++ b/lib/dialects/sqlite3/index.js
@@ -162,7 +162,7 @@ class Client_SQLite3 extends Client {
   // Ensures the response is returned in the same format as other clients.
   processResponse(obj, runner) {
     const ctx = obj.context;
-    const { response } = obj;
+    const { response, returning } = obj;
     if (obj.output) return obj.output.call(runner, response);
     switch (obj.method) {
       case 'select':
@@ -171,14 +171,26 @@ class Client_SQLite3 extends Client {
         return response[0];
       case 'pluck':
         return map(response, obj.pluck);
-      case 'insert':
+      case 'insert': {
+        if (returning) {
+          if (response) {
+            return response;
+          }
+
+          // ToDo Implement after https://github.com/microsoft/vscode-node-sqlite3/issues/15 is resolved
+          this.logger.warn(
+            'node-sqlite3 does not currently support RETURNING clause'
+          );
+        }
         return [ctx.lastID];
+      }
       case 'del':
       case 'update':
       case 'counter':
         return ctx.changes;
-      default:
+      default: {
         return response;
+      }
     }
   }
 
diff --git a/lib/dialects/sqlite3/query/sqlite-querycompiler.js b/lib/dialects/sqlite3/query/sqlite-querycompiler.js
index 950054d0..6096eab5 100644
--- a/lib/dialects/sqlite3/query/sqlite-querycompiler.js
+++ b/lib/dialects/sqlite3/query/sqlite-querycompiler.js
@@ -20,8 +20,6 @@ class QueryCompiler_SQLite3 extends QueryCompiler {
   constructor(client, builder, formatter) {
     super(client, builder, formatter);
 
-    const { returning } = this.single;
-
     // The locks are not applicable in SQLite3
     this.forShare = emptyStr;
     this.forKeyShare = emptyStr;
@@ -44,20 +42,28 @@ class QueryCompiler_SQLite3 extends QueryCompiler {
         insertValues[0] &&
         isEmpty(insertValues[0])
       ) {
-        return sql + this._emptyInsertValue;
+        return {
+          sql: sql + this._emptyInsertValue,
+        };
       }
     } else if (typeof insertValues === 'object' && isEmpty(insertValues)) {
-      return sql + this._emptyInsertValue;
+      return {
+        sql: sql + this._emptyInsertValue,
+      };
     }
 
     const insertData = this._prepInsert(insertValues);
 
     if (isString(insertData)) {
-      return sql + insertData;
+      return {
+        sql: sql + insertData,
+      };
     }
 
     if (insertData.columns.length === 0) {
-      return '';
+      return {
+        sql: '',
+      };
     }
 
     sql += `(${this.formatter.columnize(insertData.columns)})`;
@@ -93,7 +99,15 @@ class QueryCompiler_SQLite3 extends QueryCompiler {
         if (wheres) sql += ` ${wheres}`;
       }
 
-      return sql;
+      const { returning } = this.single;
+      if (returning) {
+        sql += this._returning(returning);
+      }
+
+      return {
+        sql,
+        returning,
+      };
     }
 
     const blocks = [];
@@ -126,7 +140,13 @@ class QueryCompiler_SQLite3 extends QueryCompiler {
         ' where true' + this._merge(merge.updates, onConflict, insertValues);
     }
 
-    return sql;
+    const { returning } = this.single;
+    if (returning) sql += this._returning(returning);
+
+    return {
+      sql,
+      returning,
+    };
   }
 
   _ignore(columns) {
@@ -182,6 +202,10 @@ class QueryCompiler_SQLite3 extends QueryCompiler {
     }
   }
 
+  _returning(value) {
+    return value ? ` returning ${this.formatter.columnize(value)}` : '';
+  }
+
   // Compile a truncate table statement into SQL.
   truncate() {
     const { table } = this.single;


Test patch:
diff --git a/test/integration2/query/insert/inserts.spec.js b/test/integration2/query/insert/inserts.spec.js
index f735f9e8..e722edc9 100644
--- a/test/integration2/query/insert/inserts.spec.js
+++ b/test/integration2/query/insert/inserts.spec.js
@@ -116,7 +116,7 @@ describe('Inserts', function () {
             );
             tester(
               'sqlite3',
-              'insert into `accounts` (`about`, `created_at`, `email`, `first_name`, `last_name`, `logins`, `updated_at`) values (?, ?, ?, ?, ?, ?, ?)',
+              'insert into `accounts` (`about`, `created_at`, `email`, `first_name`, `last_name`, `logins`, `updated_at`) values (?, ?, ?, ?, ?, ?, ?) returning `id`',
               [
                 'Lorem ipsum Dolore labore incididunt enim.',
                 TEST_TIMESTAMP,
@@ -253,7 +253,7 @@ describe('Inserts', function () {
             );
             tester(
               'sqlite3',
-              'insert into `accounts` (`about`, `created_at`, `email`, `first_name`, `last_name`, `logins`, `updated_at`) select ? as `about`, ? as `created_at`, ? as `email`, ? as `first_name`, ? as `last_name`, ? as `logins`, ? as `updated_at` union all select ? as `about`, ? as `created_at`, ? as `email`, ? as `first_name`, ? as `last_name`, ? as `logins`, ? as `updated_at`',
+              'insert into `accounts` (`about`, `created_at`, `email`, `first_name`, `last_name`, `logins`, `updated_at`) select ? as `about`, ? as `created_at`, ? as `email`, ? as `first_name`, ? as `last_name`, ? as `logins`, ? as `updated_at` union all select ? as `about`, ? as `created_at`, ? as `email`, ? as `first_name`, ? as `last_name`, ? as `logins`, ? as `updated_at` returning `id`',
               [
                 'Lorem ipsum Dolore labore incididunt enim.',
                 TEST_TIMESTAMP,
@@ -471,7 +471,7 @@ describe('Inserts', function () {
             );
             tester(
               'sqlite3',
-              'insert into `accounts` (`about`, `created_at`, `email`, `first_name`, `last_name`, `logins`, `updated_at`) select ? as `about`, ? as `created_at`, ? as `email`, ? as `first_name`, ? as `last_name`, ? as `logins`, ? as `updated_at` union all select ? as `about`, ? as `created_at`, ? as `email`, ? as `first_name`, ? as `last_name`, ? as `logins`, ? as `updated_at`',
+              'insert into `accounts` (`about`, `created_at`, `email`, `first_name`, `last_name`, `logins`, `updated_at`) select ? as `about`, ? as `created_at`, ? as `email`, ? as `first_name`, ? as `last_name`, ? as `logins`, ? as `updated_at` union all select ? as `about`, ? as `created_at`, ? as `email`, ? as `first_name`, ? as `last_name`, ? as `logins`, ? as `updated_at` returning `id`',
               [
                 'Lorem ipsum Dolore labore incididunt enim.',
                 TEST_TIMESTAMP,
@@ -602,7 +602,7 @@ describe('Inserts', function () {
             );
             tester(
               'sqlite3',
-              'insert into `accounts` (`about`, `created_at`, `email`, `first_name`, `last_name`, `logins`, `updated_at`) values (?, ?, ?, ?, ?, ?, ?)',
+              'insert into `accounts` (`about`, `created_at`, `email`, `first_name`, `last_name`, `logins`, `updated_at`) values (?, ?, ?, ?, ?, ?, ?) returning `id`',
               [
                 'Lorem ipsum Dolore labore incididunt enim.',
                 TEST_TIMESTAMP,
@@ -714,7 +714,7 @@ describe('Inserts', function () {
             );
             tester(
               'sqlite3',
-              'insert into `accounts` (`about`, `created_at`, `email`, `first_name`, `last_name`, `logins`, `updated_at`) values (?, ?, ?, ?, ?, ?, ?)',
+              'insert into `accounts` (`about`, `created_at`, `email`, `first_name`, `last_name`, `logins`, `updated_at`) values (?, ?, ?, ?, ?, ?, ?) returning `id`',
               [
                 'Lorem ipsum Dolore labore incididunt enim.',
                 TEST_TIMESTAMP,
@@ -1021,7 +1021,7 @@ describe('Inserts', function () {
             );
             tester(
               'sqlite3',
-              'insert into `test_table_two` (`account_id`, `details`, `status`) values (?, ?, ?)',
+              'insert into `test_table_two` (`account_id`, `details`, `status`) values (?, ?, ?) returning `account_id`, `details`',
               [
                 10,
                 'Lorem ipsum Minim nostrud Excepteur consectetur enim ut qui sint in veniam in nulla anim do cillum sunt voluptate Duis non incididunt.',
@@ -1382,7 +1382,7 @@ describe('Inserts', function () {
               );
               tester(
                 'sqlite3',
-                'insert into `upsert_tests` (`email`, `name`) values (?, ?) on conflict (`email`) do nothing',
+                'insert into `upsert_tests` (`email`, `name`) values (?, ?) on conflict (`email`) do nothing returning `email`',
                 ['ignoretest1@example.com', 'AFTER']
               );
             });
@@ -1444,7 +1444,7 @@ describe('Inserts', function () {
               );
               tester(
                 'sqlite3',
-                'insert into `upsert_tests` (`email`, `name`) values (?, ?) on conflict do nothing',
+                'insert into `upsert_tests` (`email`, `name`) values (?, ?) on conflict do nothing returning `email`',
                 ['ignoretest1@example.com', 'AFTER']
               );
             });
@@ -1512,7 +1512,7 @@ describe('Inserts', function () {
               );
               tester(
                 'sqlite3',
-                'insert into `upsert_composite_key_tests` (`email`, `name`, `org`) values (?, ?, ?) on conflict (`org`, `email`) do nothing',
+                'insert into `upsert_composite_key_tests` (`email`, `name`, `org`) values (?, ?, ?) on conflict (`org`, `email`) do nothing returning `email`',
                 ['ignoretest1@example.com', 'AFTER', 'acme-inc']
               );
             });
@@ -1571,7 +1571,7 @@ describe('Inserts', function () {
               );
               tester(
                 'sqlite3',
-                'insert into `upsert_tests` (`email`, `name`) values (?, ?) on conflict (`email`) do update set `email` = excluded.`email`, `name` = excluded.`name`',
+                'insert into `upsert_tests` (`email`, `name`) values (?, ?) on conflict (`email`) do update set `email` = excluded.`email`, `name` = excluded.`name` returning `email`',
                 ['mergetest1@example.com', 'AFTER']
               );
             });
@@ -1628,7 +1628,7 @@ describe('Inserts', function () {
               );
               tester(
                 'sqlite3',
-                'insert into `upsert_tests` (`email`, `name`) values (?, ?) on conflict (`email`) do update set `email` = excluded.`email`, `name` = excluded.`name` where `upsert_tests`.`role` = ?',
+                'insert into `upsert_tests` (`email`, `name`) values (?, ?) on conflict (`email`) do update set `email` = excluded.`email`, `name` = excluded.`name` where `upsert_tests`.`role` = ? returning `email`',
                 ['mergetest1@example.com', 'AFTER', 'tester']
               );
             });
@@ -1694,7 +1694,7 @@ describe('Inserts', function () {
               );
               tester(
                 'sqlite3',
-                'insert into `upsert_tests` (`email`, `name`) values (?, ?) on conflict (`email`) do update set `email` = excluded.`email`, `name` = excluded.`name` where `upsert_tests`.`role` = ?',
+                'insert into `upsert_tests` (`email`, `name`) values (?, ?) on conflict (`email`) do update set `email` = excluded.`email`, `name` = excluded.`name` where `upsert_tests`.`role` = ? returning `email`',
                 ['mergetest1@example.com', 'AFTER', 'fake-role']
               );
             });
@@ -1770,7 +1770,7 @@ describe('Inserts', function () {
               );
               tester(
                 'sqlite3',
-                "insert into `upsert_tests` (`email`, `name`) values (?, (SELECT name FROM (SELECT * FROM upsert_tests) AS t WHERE email = 'mergesource@example.com')) on conflict (`email`) do update set `email` = excluded.`email`, `name` = excluded.`name`",
+                "insert into `upsert_tests` (`email`, `name`) values (?, (SELECT name FROM (SELECT * FROM upsert_tests) AS t WHERE email = 'mergesource@example.com')) on conflict (`email`) do update set `email` = excluded.`email`, `name` = excluded.`name` returning `email`",
                 ['mergedest@example.com']
               );
             });
@@ -1838,7 +1838,7 @@ describe('Inserts', function () {
               );
               tester(
                 'sqlite3',
-                'insert into `upsert_tests` (`email`, `name`) values (?, ?) on conflict (`email`) do update set `name` = (SELECT name FROM upsert_value_source)',
+                'insert into `upsert_tests` (`email`, `name`) values (?, ?) on conflict (`email`) do update set `name` = (SELECT name FROM upsert_value_source) returning `email`',
                 ['mergedest@example.com', 'SHOULD NOT BE USED']
               );
             });
@@ -1906,7 +1906,7 @@ describe('Inserts', function () {
               );
               tester(
                 'sqlite3',
-                'insert into `upsert_tests` (`email`, `name`) values (?, ?) on conflict (`email`) do update set `name` = excluded.`name`',
+                'insert into `upsert_tests` (`email`, `name`) values (?, ?) on conflict (`email`) do update set `name` = excluded.`name` returning `email`',
                 ['mergedest@example.com', 'SHOULD BE USED']
               );
             });
@@ -1971,7 +1971,7 @@ describe('Inserts', function () {
               );
               tester(
                 'sqlite3',
-                'insert into `upsert_tests` (`email`, `name`) select ? as `email`, ? as `name` union all select ? as `email`, ? as `name` where true on conflict (`email`) do update set `email` = excluded.`email`, `name` = excluded.`name`',
+                'insert into `upsert_tests` (`email`, `name`) select ? as `email`, ? as `name` union all select ? as `email`, ? as `name` where true on conflict (`email`) do update set `email` = excluded.`email`, `name` = excluded.`name` returning `email`',
                 ['two@example.com', 'AFTER', 'three@example.com', 'AFTER']
               );
             });
diff --git a/test/integration2/query/misc/additional.spec.js b/test/integration2/query/misc/additional.spec.js
index 6ff33337..2bb9e6ab 100644
--- a/test/integration2/query/misc/additional.spec.js
+++ b/test/integration2/query/misc/additional.spec.js
@@ -20,6 +20,7 @@ const {
   isPgBased,
   isPgNative,
   isCockroachDB,
+  isBetterSQLite3,
 } = require('../../../util/db-helpers');
 const { DRIVER_NAMES: drivers } = require('../../../util/constants');
 const {
@@ -53,11 +54,6 @@ describe('Additional', function () {
         await createTestTableTwo(knex);
       });
 
-      beforeEach(async () => {
-        await knex('accounts').truncate();
-        await insertAccounts(knex);
-      });
-
       after(async () => {
         await knex.destroy();
       });
@@ -148,16 +144,23 @@ describe('Additional', function () {
         });
 
         it('should process response done through a stream', (done) => {
-          let response;
-          const stream = knex('accounts').limit(1).stream();
+          knex('accounts')
+            .truncate()
+            .then(() => {
+              return insertAccounts(knex, 'accounts');
+            })
+            .then(() => {
+              let response;
+              const stream = knex('accounts').limit(1).stream();
 
-          stream.on('data', (res) => {
-            response = res;
-          });
-          stream.on('finish', () => {
-            expect(response.callCount).to.equal(1);
-            done();
-          });
+              stream.on('data', (res) => {
+                response = res;
+              });
+              stream.on('finish', () => {
+                expect(response.callCount).to.equal(1);
+                done();
+              });
+            });
         });
 
         it('should pass query context for responses through a stream', (done) => {
@@ -234,9 +237,6 @@ describe('Additional', function () {
 
       describe('returning with wrapIdentifier and postProcessResponse` (TODO: fix to work on all possible dialects)', function () {
         const origHooks = {};
-        if (!isPostgreSQL(knex) || !isMssql(knex)) {
-          return;
-        }
 
         before('setup custom hooks', () => {
           origHooks.postProcessResponse =
@@ -273,15 +273,27 @@ describe('Additional', function () {
         });
 
         it('should return the correct column when a single property is given to returning', () => {
+          if (!isPostgreSQL(knex) && !isMssql(knex) && !isBetterSQLite3(knex)) {
+            return;
+          }
+
           return knex('accounts_foo')
             .insert({ balance_foo: 123 })
             .returning('balance_foo')
             .then((res) => {
-              expect(res).to.eql([123]);
+              expect(res).to.eql([
+                {
+                  balance_foo: 123,
+                },
+              ]);
             });
         });
 
         it('should return the correct columns when multiple properties are given to returning', () => {
+          if (!isPostgreSQL(knex) && !isMssql(knex) && !isBetterSQLite3(knex)) {
+            return;
+          }
+
           return knex('accounts_foo')
             .insert({ balance_foo: 123, email_foo: 'foo@bar.com' })
             .returning(['balance_foo', 'email_foo'])
diff --git a/test/unit/query/builder.js b/test/unit/query/builder.js
index bcf23c0a..511b7841 100644
--- a/test/unit/query/builder.js
+++ b/test/unit/query/builder.js
@@ -208,7 +208,7 @@ describe('Custom identifier wrapping', () => {
           bindings: ['foo', 'taylor', 'bar', 'dayle'],
         },
         sqlite3: {
-          sql: 'insert into `users_wrapper_was_here` (`email_wrapper_was_here`, `name_wrapper_was_here`) select ? as `email_wrapper_was_here`, ? as `name_wrapper_was_here` union all select ? as `email_wrapper_was_here`, ? as `name_wrapper_was_here`',
+          sql: 'insert into `users_wrapper_was_here` (`email_wrapper_was_here`, `name_wrapper_was_here`) select ? as `email_wrapper_was_here`, ? as `name_wrapper_was_here` union all select ? as `email_wrapper_was_here`, ? as `name_wrapper_was_here` returning `id_wrapper_was_here`',
         },
         pg: {
           sql: 'insert into "users_wrapper_was_here" ("email_wrapper_was_here", "name_wrapper_was_here") values (?, ?), (?, ?) returning "id_wrapper_was_here"',
@@ -260,7 +260,7 @@ describe('Custom identifier wrapping', () => {
           bindings: ['foo', 'taylor', 'bar', 'dayle'],
         },
         sqlite3: {
-          sql: 'insert into `users_wrapper_was_here` (`email_wrapper_was_here`, `name_wrapper_was_here`) select ? as `email_wrapper_was_here`, ? as `name_wrapper_was_here` union all select ? as `email_wrapper_was_here`, ? as `name_wrapper_was_here`',
+          sql: 'insert into `users_wrapper_was_here` (`email_wrapper_was_here`, `name_wrapper_was_here`) select ? as `email_wrapper_was_here`, ? as `name_wrapper_was_here` union all select ? as `email_wrapper_was_here`, ? as `name_wrapper_was_here` returning `id_wrapper_was_here`, `name_wrapper_was_here`',
           bindings: ['foo', 'taylor', 'bar', 'dayle'],
         },
         pg: {
@@ -5422,7 +5422,7 @@ describe('QueryBuilder', () => {
           bindings: ['foo', 'taylor', 'bar', 'dayle'],
         },
         sqlite3: {
-          sql: 'insert into `users` (`email`, `name`) select ? as `email`, ? as `name` union all select ? as `email`, ? as `name`',
+          sql: 'insert into `users` (`email`, `name`) select ? as `email`, ? as `name` union all select ? as `email`, ? as `name` returning `id`',
         },
         pg: {
           sql: 'insert into "users" ("email", "name") values (?, ?), (?, ?) returning "id"',
@@ -5473,7 +5473,7 @@ describe('QueryBuilder', () => {
           bindings: ['foo', 'taylor', 'bar', 'dayle'],
         },
         sqlite3: {
-          sql: 'insert into `users` (`email`, `name`) select ? as `email`, ? as `name` union all select ? as `email`, ? as `name`',
+          sql: 'insert into `users` (`email`, `name`) select ? as `email`, ? as `name` union all select ? as `email`, ? as `name` returning `id`, `name`',
           bindings: ['foo', 'taylor', 'bar', 'dayle'],
         },
         pg: {