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

Simplify reinsert logic when altering a table in SQLite #4272

Merged
merged 1 commit into from Feb 3, 2021
Merged

Simplify reinsert logic when altering a table in SQLite #4272

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
64 changes: 19 additions & 45 deletions lib/dialects/sqlite3/schema/ddl.js
View file
Open in desktop
Expand Up @@ -12,12 +12,10 @@ const omit = require('lodash/omit');
const { nanonum } = require('../../../util/nanoid');
const { COMMA_NO_PAREN_REGEX } = require('../../../constants');
const {
createTempTable,
createNewTable,
copyAllData,
copyData,
dropTempTable,
dropOriginal,
reinsertData,
copyData,
renameTable,
getTableSql,
} = require('./internal/sqlite-ddl-operations');
Expand Down Expand Up @@ -65,31 +63,16 @@ class SQLite3_DDL {
});
}

async renameTable() {
return this.trx.raw(renameTable(this.tableName(), this.alteredName));
renameTable() {
return this.trx.raw(renameTable(this.alteredName, this.tableName()));
}

dropOriginal() {
return this.trx.raw(dropOriginal(this.tableName()));
}

dropTempTable() {
return this.trx.raw(dropTempTable(this.alteredName));
}

async copyData() {
async copyData(iterator) {
const commands = await copyData(
this.trx,
this.tableName(),
this.alteredName
);
for (const command of commands) {
await this.trx.raw(command);
}
}

async reinsertData(iterator) {
const commands = await reinsertData(
this.trx,
iterator,
this.tableName(),
Expand All @@ -100,9 +83,9 @@ class SQLite3_DDL {
}
}

createTempTable(createTable) {
createNewTable(sql) {
return this.trx.raw(
createTempTable(createTable, this.tableName(), this.alteredName)
createNewTable(sql, this.tableName(), this.alteredName)
);
}

Expand Down Expand Up @@ -267,9 +250,7 @@ class SQLite3_DDL {
fromPairs(columns.map((column) => [column, column]))
)
);
return this.reinsertMapped(createTable, newSql, (row) =>
omit(row, ...mappedColumns)
);
return this.alter(newSql, (row) => omit(row, ...mappedColumns));
});
},
{ connection: this.connection }
Expand Down Expand Up @@ -320,7 +301,7 @@ class SQLite3_DDL {

const newSql = oneLineSql.replace(defs, updatedDefs);

return this.reinsertMapped(createTable, newSql, (row) => {
return this.alter(newSql, (row) => {
return row;
});
},
Expand Down Expand Up @@ -367,7 +348,7 @@ class SQLite3_DDL {

const newSql = oneLineSql.replace(defs, updatedDefs);

return this.reinsertMapped(createTable, newSql, (row) => {
return this.alter(newSql, (row) => {
return row;
});
},
Expand Down Expand Up @@ -411,7 +392,7 @@ class SQLite3_DDL {
newColumnDefinitions
);

return this.reinsertMapped(tableInfo, newSQL, (row) => {
return this.alter(newSQL, (row) => {
return row;
});
},
Expand Down Expand Up @@ -464,7 +445,7 @@ class SQLite3_DDL {
newColumnDefinitions.join(', ')
);

return await this.generateReinsertCommands(tableInfo, newSQL, (row) => {
return await this.generateAlterCommands(newSQL, (row) => {
return row;
});
},
Expand All @@ -479,29 +460,22 @@ class SQLite3_DDL {
* It'll be helpful to refactor this file heavily to combine/optimize some of these calls
*/

reinsertMapped(createTable, newSql, mapRow) {
alter(newSql, mapRow) {
return Promise.resolve()
.then(() => this.createTempTable(createTable))
.then(() => this.copyData())
.then(() => this.createNewTable(newSql))
.then(() => this.copyData(mapRow))
.then(() => this.dropOriginal())
.then(() => this.trx.raw(newSql))
.then(() => this.reinsertData(mapRow))
.then(() => this.dropTempTable());
.then(() => this.renameTable());
}

async generateReinsertCommands(createTable, newSql, mapRow) {
async generateAlterCommands(newSql, mapRow) {
const result = [];
result.push(
createTempTable(createTable, this.tableName(), this.alteredName)
);

result.push(createNewTable(newSql, this.tableName(), this.alteredName));
result.push(copyAllData(this.tableName(), this.alteredName));
result.push(dropOriginal(this.tableName()));
result.push(renameTable(this.alteredName, this.tableName()));

result.push(newSql);

result.push(copyAllData(this.alteredName, this.tableName()));
result.push(dropTempTable(this.alteredName));
return result;
}
}
Expand Down
24 changes: 6 additions & 18 deletions lib/dialects/sqlite3/schema/internal/sqlite-ddl-operations.js
View file
Open in desktop
Expand Up @@ -13,20 +13,14 @@ function insertChunked(trx, chunkSize, target, iterator, existingData) {
return result;
}

function createTempTable(createTable, tablename, alteredName) {
return createTable.sql.replace(tablename, alteredName);
function createNewTable(sql, tablename, alteredName) {
return sql.replace(tablename, alteredName);
}

// ToDo To be removed
async function copyData(trx, tableName, alteredName) {
async function copyData(trx, iterator, tableName, alteredName) {
const existingData = await trx.raw(`SELECT * FROM "${tableName}"`);
return insertChunked(trx, 20, alteredName, identity, existingData);
}

// ToDo To be removed
async function reinsertData(trx, iterator, tableName, alteredName) {
const existingData = await trx.raw(`SELECT * FROM "${alteredName}"`);
return insertChunked(trx, 20, tableName, iterator, existingData);
return insertChunked(trx, 20, alteredName, iterator, existingData);
}

function copyAllData(sourceTable, targetTable) {
Expand All @@ -37,10 +31,6 @@ function dropOriginal(tableName) {
return `DROP TABLE "${tableName}"`;
}

function dropTempTable(alteredName) {
return `DROP TABLE "${alteredName}"`;
}

function renameTable(tableName, alteredName) {
return `ALTER TABLE "${tableName}" RENAME TO "${alteredName}"`;
}
Expand All @@ -51,11 +41,9 @@ function getTableSql(tableName) {

module.exports = {
copyAllData,
copyData,
createTempTable,
createNewTable,
dropOriginal,
dropTempTable,
reinsertData,
copyData,
renameTable,
getTableSql,
};
10 changes: 5 additions & 5 deletions test/integration/schema/index.js
View file
Open in desktop
Expand Up @@ -1366,7 +1366,7 @@ module.exports = (knex) => {
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 " +
'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") )'
Expand All @@ -1375,24 +1375,24 @@ module.exports = (knex) => {
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, ' +
'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, ' +
'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))'
'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)'
'CREATE TABLE "TEST"(I5 integer)'
);
let lastColDeletionError;
await knex.schema
Expand Down
18 changes: 6 additions & 12 deletions test/integration2/schema/foreign-keys.spec.js
View file
Open in desktop
Expand Up @@ -66,12 +66,10 @@ describe('Schema', () => {

if (isSQLite(knex)) {
expect(queries).to.eql([
'CREATE TABLE `_knex_temp_alter111` (`id` integer not null primary key autoincrement, `fkey_two` integer not null, `fkey_three` integer not null)',
'CREATE TABLE `_knex_temp_alter111` (`id` integer not null primary key autoincrement, `fkey_two` integer not null, `fkey_three` integer not null, CONSTRAINT fk_fkey_threeee FOREIGN KEY (`fkey_three`) REFERENCES `foreign_keys_table_three` (`id`))',
'INSERT INTO _knex_temp_alter111 SELECT * FROM foreign_keys_table_one;',
'DROP TABLE "foreign_keys_table_one"',
'CREATE TABLE `foreign_keys_table_one` (`id` integer not null primary key autoincrement, `fkey_two` integer not null, `fkey_three` integer not null, CONSTRAINT fk_fkey_threeee FOREIGN KEY (`fkey_three`) REFERENCES `foreign_keys_table_three` (`id`))',
'INSERT INTO foreign_keys_table_one SELECT * FROM _knex_temp_alter111;',
'DROP TABLE "_knex_temp_alter111"',
'ALTER TABLE "_knex_temp_alter111" RENAME TO "foreign_keys_table_one"',
]);
}

Expand Down Expand Up @@ -105,12 +103,10 @@ describe('Schema', () => {
const queries = await builder.generateDdlCommands();

expect(queries).to.eql([
'CREATE TABLE `_knex_temp_alter111` (`id` integer not null primary key autoincrement, `fkey_two` integer not null, `fkey_three` integer not null)',
'CREATE TABLE `_knex_temp_alter111` (`id` integer not null primary key autoincrement, `fkey_two` integer not null, `fkey_three` integer not null, CONSTRAINT fk_fkey_threeee FOREIGN KEY (`fkey_three`) REFERENCES `foreign_keys_table_three` (`id`) ON DELETE CASCADE)',
'INSERT INTO _knex_temp_alter111 SELECT * FROM foreign_keys_table_one;',
'DROP TABLE "foreign_keys_table_one"',
'CREATE TABLE `foreign_keys_table_one` (`id` integer not null primary key autoincrement, `fkey_two` integer not null, `fkey_three` integer not null, CONSTRAINT fk_fkey_threeee FOREIGN KEY (`fkey_three`) REFERENCES `foreign_keys_table_three` (`id`) ON DELETE CASCADE)',
'INSERT INTO foreign_keys_table_one SELECT * FROM _knex_temp_alter111;',
'DROP TABLE "_knex_temp_alter111"',
'ALTER TABLE "_knex_temp_alter111" RENAME TO "foreign_keys_table_one"',
]);
});

Expand All @@ -133,12 +129,10 @@ describe('Schema', () => {
const queries = await builder.generateDdlCommands();

expect(queries).to.eql([
'CREATE TABLE `_knex_temp_alter111` (`id` integer not null primary key autoincrement, `fkey_two` integer not null, `fkey_three` integer not null)',
'CREATE TABLE `_knex_temp_alter111` (`id` integer not null primary key autoincrement, `fkey_two` integer not null, `fkey_three` integer not null, CONSTRAINT fk_fkey_threeee FOREIGN KEY (`fkey_three`) REFERENCES `foreign_keys_table_three` (`id`) ON UPDATE CASCADE)',
'INSERT INTO _knex_temp_alter111 SELECT * FROM foreign_keys_table_one;',
'DROP TABLE "foreign_keys_table_one"',
'CREATE TABLE `foreign_keys_table_one` (`id` integer not null primary key autoincrement, `fkey_two` integer not null, `fkey_three` integer not null, CONSTRAINT fk_fkey_threeee FOREIGN KEY (`fkey_three`) REFERENCES `foreign_keys_table_three` (`id`) ON UPDATE CASCADE)',
'INSERT INTO foreign_keys_table_one SELECT * FROM _knex_temp_alter111;',
'DROP TABLE "_knex_temp_alter111"',
'ALTER TABLE "_knex_temp_alter111" RENAME TO "foreign_keys_table_one"',
]);
});

Expand Down