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.

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

Fix handling of multiline SQL in SQLite3 schema #3411

Merged
merged 7 commits into from Oct 6, 2019
@@ -129,7 +129,8 @@ assign(SQLite3_DDL.prototype, {
},

_doReplace(sql, from, to) {
const matched = sql.match(/^CREATE TABLE (\S+) \((.*)\)/);
const oneLineSql = sql.replace(/\s+/g, ' ');
const matched = oneLineSql.match(/^CREATE TABLE (\S+) \((.*)\)/);

const tableName = matched[1];
const defs = matched[2];
@@ -174,13 +175,13 @@ assign(SQLite3_DDL.prototype, {
//
// We have to replace the from+to field with double slashes in case you created your SQlite3
// database with Knex < 0.14.
if (sql.match(/CREATE\sTABLE\s".*"\s\("/)) {
if (oneLineSql.match(/CREATE\sTABLE\s".*"\s\(\s*"/)) {
from = from.replace(/[`]/g, '"');
to = to.replace(/[`]/g, '"');
}

args = args.map(function(item) {
let split = item.split(' ');
let split = item.trim().split(' ');

if (split[0] === from) {
// column definition
@@ -222,7 +223,7 @@ assign(SQLite3_DDL.prototype, {

return item;
});
return sql
return oneLineSql
.replace(/\(.*\)/, () => `(${args.join(', ')})`)
.replace(/,\s*([,)])/, '$1');
},
@@ -4,19 +4,19 @@ const knex = require('../../knex');
const logger = require('./logger');
const config = require('../knexfile');
const fs = require('fs');
const path = require('path');

const Bluebird = require('bluebird');

Object.keys(config).forEach((dialectName) => {
return require('./suite')(logger(knex(config[dialectName])));
});

after(function(done) {
before(function() {
if (config.sqlite3 && config.sqlite3.connection.filename !== ':memory:') {
fs.unlink(config.sqlite3.connection.filename, function() {
done();
});
} else {
done();
fs.copyFileSync(
This conversation was marked as resolved by 6uliver

This comment has been minimized.

Copy link
@kibertoad

kibertoad Aug 26, 2019

Collaborator

do we need to do the copying for the whole suite and not just one test?

This comment has been minimized.

Copy link
@6uliver

6uliver Aug 26, 2019

Author Contributor

I was thinking about copying in the test body but if the test fails it will not remove the file. We could copy in the test body and delete in the teardown function but I think it's better if they are in the same place.

path.resolve(__dirname, '../sample.sqlite3'),
This conversation was marked as resolved by 6uliver

This comment has been minimized.

Copy link
@kibertoad

kibertoad Aug 24, 2019

Collaborator

Considering that this is a very specific db sample, can name be made more clear? E. g. "multilineCreateMaster.sqlite3"

This comment has been minimized.

Copy link
@6uliver

6uliver Aug 26, 2019

Author Contributor

Done.

config.sqlite3.connection.filename
);
}
});
@@ -72,6 +72,18 @@ module.exports = function(knex) {
});
}

if (knex.client.driverName === 'sqlite3') {
it('should not fail rename-and-drop-column with multiline sql from legacy db', async () => {
const config = {
directory:
'test/integration/migrate/rename-and-drop-column-with-multiline-sql-from-legacy-db',
};

await knex.migrate.latest(config);
await knex.migrate.rollback(config);
});
}

it('should not fail drop-and-recreate-column operation when using async/await', () => {
return knex.migrate
.latest({
@@ -0,0 +1,11 @@
exports.up = function(knex) {
return knex.schema.table('TestTableCreatedWithDBBrowser', function(table) {
table.renameColumn('id', 'testId');
});
};

exports.down = function(knex) {
return knex.schema.table('TestTableCreatedWithDBBrowser', function(table) {
table.renameColumn('testId', 'id');
});
};
@@ -0,0 +1,11 @@
exports.up = function(knex) {
return knex.schema.table('TestTableCreatedWithDBBrowser', function(table) {
table.dropColumn('description');
});
};

exports.down = function(knex) {
return knex.schema.table('TestTableCreatedWithDBBrowser', function(table) {
table.text('description');
});
};
BIN +44 KB test/sample.sqlite3
Binary file not shown.
@@ -67,3 +67,35 @@ it('can rename column with back sticks', function() {
'CREATE TABLE `accounts` (`id` varchar(24) not null primary key, `about_me` varchar(24))'
);
});

it('can rename column with multiline and tabulated sql statement', function() {
const client = sinon.stub();
const tableCompiler = sinon.stub();
const pragma = sinon.stub();
const connection = sinon.stub();
const ddl = new DDL(client, tableCompiler, pragma, connection);

const sql =
'CREATE TABLE `accounts` (\n\n`id`\tvarchar(24) not null primary key,\r\n`about`\t \t\t \tvarchar(24)\n\n)';

const newSql = ddl._doReplace(sql, '`about`', '`about_me`');
newSql.should.eql(
'CREATE TABLE `accounts` (`id` varchar(24) not null primary key, `about_me` varchar(24))'
);
});

it('can drop column with multiline and tabulated sql statement', function() {
const client = sinon.stub();
const tableCompiler = sinon.stub();
const pragma = sinon.stub();
const connection = sinon.stub();
const ddl = new DDL(client, tableCompiler, pragma, connection);

const sql =
'CREATE TABLE `accounts` (\n\n`id`\tvarchar(24) not null primary key,\r\n`about`\t \tvarchar(24)\n)';

const newSql = ddl._doReplace(sql, '`about`', '');
newSql.should.eql(
'CREATE TABLE `accounts` (`id` varchar(24) not null primary key)'
);
});
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.