Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix tests for RETURNING and enable them for SQLite. #4934

Merged
merged 6 commits into from Jan 16, 2022
Merged

Fix tests for RETURNING and enable them for SQLite. #4934

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fcb1cc3
Fix tests for RETURNING and enable them for SQLite.
kibertoad Jan 14, 2022
96927cb
Remove it.only
kibertoad Jan 14, 2022
264b4df
Merge branch 'master' of https://github.com/knex/knex into feat/sqlit…
kibertoad Jan 14, 2022
917ec42
Adjust implementation to use RETURNING for better-sqlite and not brea…
kibertoad Jan 15, 2022
e978e6c
Fix SQLite tests
kibertoad Jan 15, 2022
d64bbb0
Fix unit tests
kibertoad Jan 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/dialects/postgres/query/pg-querycompiler.js
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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