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 bugs in replacement logic used when dropping columns in SQLite. #3476

Merged
merged 1 commit into from Oct 11, 2019
Merged

Fix bugs in replacement logic used when dropping columns in SQLite. #3476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
80 changes: 56 additions & 24 deletions lib/dialects/sqlite3/schema/ddl.js
View file
Open in desktop
Expand Up @@ -14,6 +14,9 @@ const {
omit,
invert,
fromPairs,
some,
negate,
isEmpty
} = require('lodash');

// So altering the schema in SQLite3 is a major pain.
Expand All @@ -40,8 +43,8 @@ assign(SQLite3_DDL.prototype, {
getColumn: async function(column) {
const currentCol = find(this.pragma, (col) => {
return (
this.client.wrapIdentifier(col.name) ===
this.client.wrapIdentifier(column)
this.client.wrapIdentifier(col.name).toLowerCase() ===
this.client.wrapIdentifier(column).toLowerCase()
);
});
if (!currentCol)
Expand Down Expand Up @@ -130,7 +133,7 @@ assign(SQLite3_DDL.prototype, {

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

const tableName = matched[1];
const defs = matched[2];
Expand Down Expand Up @@ -167,27 +170,34 @@ assign(SQLite3_DDL.prototype, {
}
args.push(defs.slice(ptr, i));

const fromIdentifier = from.replace(/[`"]/g, '');
const fromIdentifier = from.replace(/[`"'[\]]/g, '');

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

let normalizedFrom = from;

// Backwards compatible for double quoted sqlite databases
// The "from" and "to" field use backsticks, because this is the default notation for
// SQlite3 since Knex 0.14.
// e.g. from: `about`
// SQLite supports all quoting mechanisms prevalent in all major dialects of SQL
// and preserves the original quoting in sqlite_master.
//
// Also, identifiers are never case sensitive, not even when quoted.
//
// We have to replace the from+to field with double slashes in case you created your SQlite3
// database with Knex < 0.14.
if (item.match(`"${fromIdentifier}"`)) {
normalizedFrom = `"${fromIdentifier}"`;
} else if (item.match(`\`${fromIdentifier}\``)) {
normalizedFrom = `\`${fromIdentifier}\``;
// Ref: https://www.sqlite.org/lang_keywords.html
const fromMatchCandidates = [
new RegExp(`\`${fromIdentifier}\``, 'i'),
new RegExp(`"${fromIdentifier}"`, 'i'),
new RegExp(`'${fromIdentifier}'`, 'i'),
new RegExp(`\\[${fromIdentifier}\\]`, 'i')
];
if (fromIdentifier.match(/^\S+$/)) {
fromMatchCandidates.push(new RegExp(`\\b${fromIdentifier}\\b`, 'i'));
}

if (split[0] === normalizedFrom) {
const doesMatchFromIdentifier = (target) =>
some(fromMatchCandidates, (c) => target.match(c));

const replaceFromIdentifier = (target) =>
fromMatchCandidates.reduce((result, candidate) => result.replace(candidate, to), target);

if (doesMatchFromIdentifier(split[0])) {
// column definition
if (to) {
split[0] = to;
Expand All @@ -203,9 +213,11 @@ assign(SQLite3_DDL.prototype, {
// columns from this table listed between (); replace
// one if it matches
if (/primary|unique/i.test(split[idx])) {
return item.replace(/\(.*\)/, (columns) =>
columns.replace(normalizedFrom, to)
);
const ret = item.replace(/\(.*\)/, replaceFromIdentifier);
// If any member columns are dropped then uniqueness/pk constraint
// can not be retained
if (ret !== item && isEmpty(to)) return '';
return ret;
}

// foreign keys have one or more columns from this table
Expand All @@ -217,18 +229,37 @@ assign(SQLite3_DDL.prototype, {
split = item.split(/ references /i);
// the quoted column names save us from having to do anything
// other than a straight replace here
split[0] = split[0].replace(normalizedFrom, to);
const replacedKeySpec = replaceFromIdentifier(split[0]);

if (split[0] !== replacedKeySpec) {
// If we are removing one or more columns of a foreign
// key, then we should not retain the key at all
if (isEmpty(to)) return '';
else split[0] = replacedKeySpec;
}

if (split[1].slice(0, tableName.length) === tableName) {
split[1] = split[1].replace(/\(.*\)/, (columns) =>
columns.replace(normalizedFrom, to)
);
// self-referential foreign key
const replacedKeyTargetSpec = split[1].replace(/\(.*\)/, replaceFromIdentifier);
if (split[1] !== replacedKeyTargetSpec) {
// If we are removing one or more columns of a foreign
// key, then we should not retain the key at all
if (isEmpty(to)) return '';
else split[1] = replacedKeyTargetSpec;
}
}
return split.join(' references ');
}

return item;
});

args = args.filter(negate(isEmpty));

if (args.length === 0) {
throw new Error('Unable to drop last column from table');
}

return oneLineSql
.replace(/\(.*\)/, () => `(${args.join(', ')})`)
.replace(/,\s*([,)])/, '$1');
Expand All @@ -248,6 +279,7 @@ assign(SQLite3_DDL.prototype, {
if (sql === newSql) {
throw new Error('Unable to find the column to change');
}

const { from: mappedFrom, to: mappedTo } = invert(
this.client.postProcessResponse(
invert({
Expand Down
75 changes: 75 additions & 0 deletions test/integration/schema/index.js
View file
Open in desktop
Expand Up @@ -1382,6 +1382,81 @@ module.exports = function(knex) {
});
}
});

context('when table is created using raw create table', function () {
beforeEach(async function() {
await knex.schema.raw(`create table TEST(
"i0" integer,
'i1' integer,
[i2] integer,
\`i3\` integer,
i4 integer,
I5 integer,
unique(i4, i5),
constraint i0 primary key([i3], "i4"),
unique([i2]),
foreign key (i1) references bar ("i3")
)`);
});

afterEach(function() {
return knex.schema.dropTable('TEST');
});

const getCreateTableExpr = async () => (
await knex.schema.raw('select name, sql from sqlite_master where type = "table" and name = "TEST"')
)[0].sql;

const dropCol = (colName) =>
knex.schema.alterTable('TEST', function(tbl) {
return tbl.dropColumn(colName);
});

const hasCol = (colName) =>
knex.schema.hasColumn('TEST', colName);

it('drops the column', async function() {
await dropCol('i0');
expect(await hasCol('i0')).to.equal(false);
// Constraint i0 should be unaffected:
expect(await getCreateTableExpr()).to.equal(
"CREATE TABLE TEST('i1' integer, [i2] integer, `i3` integer, i4 " +
'integer, I5 integer, unique(i4, i5), constraint i0 primary ' +
'key([i3], "i4"), unique([i2]), foreign key (i1) references bar ' +
'("i3") )'
);
await dropCol('i1');
expect(await hasCol('i1')).to.equal(false);
// Foreign key on i1 should also be dropped:
expect(await getCreateTableExpr()).to.equal(
'CREATE TABLE TEST([i2] integer, `i3` integer, i4 integer, I5 integer, ' +
'unique(i4, i5), constraint i0 primary key([i3], "i4"), unique([i2]))'
);
await dropCol('i2');
expect(await hasCol('i2')).to.equal(false);
expect(await getCreateTableExpr()).to.equal(
'CREATE TABLE TEST(`i3` integer, i4 integer, I5 integer, ' +
'unique(i4, i5), constraint i0 primary key([i3], "i4"))'
);
await dropCol('i3');
expect(await hasCol('i3')).to.equal(false);
expect(await getCreateTableExpr()).to.equal(
'CREATE TABLE TEST(i4 integer, I5 integer, unique(i4, i5))'
);
await dropCol('i4');
expect(await hasCol('i4')).to.equal(false);
expect(await getCreateTableExpr()).to.equal(
'CREATE TABLE TEST(I5 integer)'
);
let lastColDeletionError;
await knex.schema.alterTable('TEST', function(tbl) {
return tbl.dropColumn('i5');
}).catch(e => {
lastColDeletionError = e;
})
expect(lastColDeletionError.message).to.eql('Unable to drop last column from table');
});
});
}
});

Expand Down