Skip to content

Commit

Permalink
Fix bugs in replacement logic used when dropping columns in SQLite. (#…
Browse files Browse the repository at this point in the history 
…3476)
  • Loading branch information
@lorefnon @kibertoad
lorefnon authored and kibertoad committed Oct 11, 2019
1 parent f56eaf5 commit 20bd04b
Show file tree
Hide file tree
Showing 2 changed files with 131 additions and 24 deletions.
80 changes: 56 additions & 24 deletions lib/dialects/sqlite3/schema/ddl.js
View file
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
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

0 comments on commit 20bd04b

Please sign in to comment.