Skip to content

Commit

Permalink
Fix tests for RETURNING and enable them for SQLite. (#4934)
Browse files Browse the repository at this point in the history
  • Loading branch information
@kibertoad
kibertoad committed Jan 16, 2022
1 parent 622b751 commit 4a0d82f
Show file tree
Hide file tree
Showing 6 changed files with 98 additions and 50 deletions.
2 changes: 1 addition & 1 deletion lib/dialects/postgres/query/pg-querycompiler.js
View file
Expand Up @@ -40,7 +40,7 @@ class QueryCompiler_PG extends QueryCompiler {
if (returning) sql += this._returning(returning);

return {
sql: sql,
sql,
returning,
};
}
Expand Down
18 changes: 15 additions & 3 deletions lib/dialects/sqlite3/index.js
View file
Expand Up @@ -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':
Expand All @@ -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;
}
}
}

Expand Down
40 changes: 32 additions & 8 deletions lib/dialects/sqlite3/query/sqlite-querycompiler.js
View file
Expand Up @@ -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;
Expand All @@ -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)})`;
Expand Down Expand Up @@ -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 = [];
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand Down
32 changes: 16 additions & 16 deletions test/integration2/query/insert/inserts.spec.js
View file
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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.',
Expand Down Expand Up @@ -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']
);
});
Expand Down Expand Up @@ -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']
);
});
Expand Down Expand Up @@ -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']
);
});
Expand Down Expand Up @@ -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']
);
});
Expand Down Expand Up @@ -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']
);
});
Expand Down Expand Up @@ -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']
);
});
Expand Down Expand Up @@ -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']
);
});
Expand Down Expand Up @@ -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']
);
});
Expand Down Expand Up @@ -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']
);
});
Expand Down Expand Up @@ -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']
);
});
Expand Down
48 changes: 30 additions & 18 deletions test/integration2/query/misc/additional.spec.js
View file
Expand Up @@ -20,6 +20,7 @@ const {
isPgBased,
isPgNative,
isCockroachDB,
isBetterSQLite3,
} = require('../../../util/db-helpers');
const { DRIVER_NAMES: drivers } = require('../../../util/constants');
const {
Expand Down Expand Up @@ -53,11 +54,6 @@ describe('Additional', function () {
await createTestTableTwo(knex);
});

beforeEach(async () => {
await knex('accounts').truncate();
await insertAccounts(knex);
});

after(async () => {
await knex.destroy();
});
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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'])
Expand Down

0 comments on commit 4a0d82f

Please sign in to comment.