From 157e27d0e3628daf73e3d279551b25bd3e610c6d Mon Sep 17 00:00:00 2001 From: Nicola Krumschmidt Date: Mon, 13 Dec 2021 19:59:01 +0100 Subject: [PATCH] Fix SQLite foreign key constraints when altering a table (#4189) --- lib/dialects/sqlite3/schema/ddl.js | 553 +++++++++--------- .../schema/internal/sqlite-ddl-operations.js | 15 + .../sqlite3/schema/sqlite-compiler.js | 26 +- lib/execution/runner.js | 39 +- lib/schema/compiler.js | 1 + test/integration2/schema/foreign-keys.spec.js | 25 + types/index.d.ts | 2 +- 7 files changed, 365 insertions(+), 296 deletions(-) diff --git a/lib/dialects/sqlite3/schema/ddl.js b/lib/dialects/sqlite3/schema/ddl.js index 74f3b26981..2424d6955d 100644 --- a/lib/dialects/sqlite3/schema/ddl.js +++ b/lib/dialects/sqlite3/schema/ddl.js @@ -12,6 +12,9 @@ const { dropOriginal, renameTable, getTableSql, + isForeignCheckEnabled, + setForeignCheck, + executeForeignCheck, } = require('./internal/sqlite-ddl-operations'); const { parseCreateTable, parseCreateIndex } = require('./internal/parser'); const { @@ -43,365 +46,351 @@ class SQLite3_DDL { getTableSql() { const tableName = this.tableName(); - this.trx.disableProcessing(); - return this.trx.raw(getTableSql(tableName)).then((result) => { - this.trx.enableProcessing(); - return { - createTable: result.filter((create) => create.type === 'table')[0].sql, - createIndices: result - .filter((create) => create.type === 'index') - .map((create) => create.sql), - }; - }); + return this.client.transaction( + async (trx) => { + trx.disableProcessing(); + const result = await trx.raw(getTableSql(tableName)); + trx.enableProcessing(); + + return { + createTable: result.filter((create) => create.type === 'table')[0] + .sql, + createIndices: result + .filter((create) => create.type === 'index') + .map((create) => create.sql), + }; + }, + { connection: this.connection } + ); } - renameTable() { - return this.trx.raw(renameTable(this.alteredName, this.tableName())); + async isForeignCheckEnabled() { + const result = await this.client + .raw(isForeignCheckEnabled()) + .connection(this.connection); + + return result[0].foreign_keys === 1; } - dropOriginal() { - return this.trx.raw(dropOriginal(this.tableName())); + async setForeignCheck(enable) { + await this.client.raw(setForeignCheck(enable)).connection(this.connection); } - copyData(columns) { - return this.trx.raw(copyData(this.tableName(), this.alteredName, columns)); + renameTable(trx) { + return trx.raw(renameTable(this.alteredName, this.tableName())); } - createNewTable(sql) { - return this.trx.raw( - createNewTable(sql, this.tableName(), this.alteredName) - ); + dropOriginal(trx) { + return trx.raw(dropOriginal(this.tableName())); } - alterColumn(columns) { - return this.client.transaction( - async (trx) => { - this.trx = trx; + copyData(trx, columns) { + return trx.raw(copyData(this.tableName(), this.alteredName, columns)); + } - const { createTable, createIndices } = await this.getTableSql(); + createNewTable(trx, sql) { + return trx.raw(createNewTable(sql, this.tableName(), this.alteredName)); + } - const parsedTable = parseCreateTable(createTable); + async alterColumn(columns) { + const { createTable, createIndices } = await this.getTableSql(); - parsedTable.columns = parsedTable.columns.map((column) => { - const newColumnInfo = columns.find((c) => - isEqualId(c.name, column.name) - ); + const parsedTable = parseCreateTable(createTable); - if (newColumnInfo) { - column.type = newColumnInfo.type; - - column.constraints.default = - newColumnInfo.defaultTo !== null - ? { - name: null, - value: newColumnInfo.defaultTo, - expression: false, - } - : null; - - column.constraints.notnull = newColumnInfo.notNull - ? { name: null, conflict: null } - : null; - - column.constraints.null = newColumnInfo.notNull - ? null - : column.constraints.null; - } + parsedTable.columns = parsedTable.columns.map((column) => { + const newColumnInfo = columns.find((c) => isEqualId(c.name, column.name)); - return column; - }); + if (newColumnInfo) { + column.type = newColumnInfo.type; - const newTable = compileCreateTable(parsedTable, this.wrap); + column.constraints.default = + newColumnInfo.defaultTo !== null + ? { + name: null, + value: newColumnInfo.defaultTo, + expression: false, + } + : null; - return this.alter(newTable, createIndices); - }, - { connection: this.connection } - ); - } + column.constraints.notnull = newColumnInfo.notNull + ? { name: null, conflict: null } + : null; - dropColumn(columns) { - return this.client.transaction( - async (trx) => { - this.trx = trx; + column.constraints.null = newColumnInfo.notNull + ? null + : column.constraints.null; + } - const { createTable, createIndices } = await this.getTableSql(); + return column; + }); - const parsedTable = parseCreateTable(createTable); + const newTable = compileCreateTable(parsedTable, this.wrap); - parsedTable.columns = parsedTable.columns.filter( - (parsedColumn) => - parsedColumn.expression || !includesId(columns, parsedColumn.name) - ); + return this.alter(newTable, createIndices); + } - if (parsedTable.columns.length === 0) { - throw new Error('Unable to drop last column from table'); - } + async dropColumn(columns) { + const { createTable, createIndices } = await this.getTableSql(); - parsedTable.constraints = parsedTable.constraints.filter( - (constraint) => { - if ( - constraint.type === 'PRIMARY KEY' || - constraint.type === 'UNIQUE' - ) { - return constraint.columns.every( - (constraintColumn) => - constraintColumn.expression || - !includesId(columns, constraintColumn.name) - ); - } else if (constraint.type === 'FOREIGN KEY') { - return ( - constraint.columns.every( - (constraintColumnName) => - !includesId(columns, constraintColumnName) - ) && - (constraint.references.table !== parsedTable.table || - constraint.references.columns.every( - (referenceColumnName) => - !includesId(columns, referenceColumnName) - )) - ); - } else { - return true; - } - } + const parsedTable = parseCreateTable(createTable); + + parsedTable.columns = parsedTable.columns.filter( + (parsedColumn) => + parsedColumn.expression || !includesId(columns, parsedColumn.name) + ); + + if (parsedTable.columns.length === 0) { + throw new Error('Unable to drop last column from table'); + } + + parsedTable.constraints = parsedTable.constraints.filter((constraint) => { + if (constraint.type === 'PRIMARY KEY' || constraint.type === 'UNIQUE') { + return constraint.columns.every( + (constraintColumn) => + constraintColumn.expression || + !includesId(columns, constraintColumn.name) + ); + } else if (constraint.type === 'FOREIGN KEY') { + return ( + constraint.columns.every( + (constraintColumnName) => !includesId(columns, constraintColumnName) + ) && + (constraint.references.table !== parsedTable.table || + constraint.references.columns.every( + (referenceColumnName) => !includesId(columns, referenceColumnName) + )) ); + } else { + return true; + } + }); - const newColumns = parsedTable.columns.map((column) => column.name); + const newColumns = parsedTable.columns.map((column) => column.name); - const newTable = compileCreateTable(parsedTable, this.wrap); + const newTable = compileCreateTable(parsedTable, this.wrap); - const newIndices = []; - for (const createIndex of createIndices) { - const parsedIndex = parseCreateIndex(createIndex); + const newIndices = []; + for (const createIndex of createIndices) { + const parsedIndex = parseCreateIndex(createIndex); - parsedIndex.columns = parsedIndex.columns.filter( - (parsedColumn) => - parsedColumn.expression || !includesId(columns, parsedColumn.name) - ); + parsedIndex.columns = parsedIndex.columns.filter( + (parsedColumn) => + parsedColumn.expression || !includesId(columns, parsedColumn.name) + ); - if (parsedIndex.columns.length > 0) { - newIndices.push(compileCreateIndex(parsedIndex, this.wrap)); - } - } + if (parsedIndex.columns.length > 0) { + newIndices.push(compileCreateIndex(parsedIndex, this.wrap)); + } + } - return this.alter(newTable, newIndices, newColumns); - }, - { connection: this.connection } - ); + return this.alter(newTable, newIndices, newColumns); } - dropForeign(columns, foreignKeyName) { - return this.client.transaction( - async (trx) => { - this.trx = trx; + async dropForeign(columns, foreignKeyName) { + const { createTable, createIndices } = await this.getTableSql(); - const { createTable, createIndices } = await this.getTableSql(); + const parsedTable = parseCreateTable(createTable); - const parsedTable = parseCreateTable(createTable); + if (!foreignKeyName) { + parsedTable.columns = parsedTable.columns.map((column) => ({ + ...column, + references: includesId(columns, column.name) ? null : column.references, + })); + } - if (!foreignKeyName) { - parsedTable.columns = parsedTable.columns.map((column) => ({ - ...column, - references: includesId(columns, column.name) - ? null - : column.references, - })); + parsedTable.constraints = parsedTable.constraints.filter((constraint) => { + if (constraint.type === 'FOREIGN KEY') { + if (foreignKeyName) { + return ( + !constraint.name || !isEqualId(constraint.name, foreignKeyName) + ); } - parsedTable.constraints = parsedTable.constraints.filter( - (constraint) => { - if (constraint.type === 'FOREIGN KEY') { - if (foreignKeyName) { - return ( - !constraint.name || - !isEqualId(constraint.name, foreignKeyName) - ); - } - - return constraint.columns.every( - (constraintColumnName) => - !includesId(columns, constraintColumnName) - ); - } else { - return true; - } - } + return constraint.columns.every( + (constraintColumnName) => !includesId(columns, constraintColumnName) ); + } else { + return true; + } + }); - const newTable = compileCreateTable(parsedTable, this.wrap); + const newTable = compileCreateTable(parsedTable, this.wrap); - return this.alter(newTable, createIndices); - }, - { connection: this.connection } - ); + return this.alter(newTable, createIndices); } - dropPrimary(constraintName) { - return this.client.transaction( - async (trx) => { - this.trx = trx; - - const { createTable, createIndices } = await this.getTableSql(); - - const parsedTable = parseCreateTable(createTable); - - parsedTable.columns = parsedTable.columns.map((column) => ({ - ...column, - primary: null, - })); - - parsedTable.constraints = parsedTable.constraints.filter( - (constraint) => { - if (constraint.type === 'PRIMARY KEY') { - if (constraintName) { - return ( - !constraint.name || - !isEqualId(constraint.name, constraintName) - ); - } else { - return false; - } - } else { - return true; - } - } - ); + async dropPrimary(constraintName) { + const { createTable, createIndices } = await this.getTableSql(); - const newTable = compileCreateTable(parsedTable, this.wrap); + const parsedTable = parseCreateTable(createTable); - return this.alter(newTable, createIndices); - }, - { connection: this.connection } - ); - } + parsedTable.columns = parsedTable.columns.map((column) => ({ + ...column, + primary: null, + })); - primary(columns, constraintName) { - return this.client.transaction( - async (trx) => { - this.trx = trx; + parsedTable.constraints = parsedTable.constraints.filter((constraint) => { + if (constraint.type === 'PRIMARY KEY') { + if (constraintName) { + return ( + !constraint.name || !isEqualId(constraint.name, constraintName) + ); + } else { + return false; + } + } else { + return true; + } + }); - const { createTable, createIndices } = await this.getTableSql(); + const newTable = compileCreateTable(parsedTable, this.wrap); - const parsedTable = parseCreateTable(createTable); + return this.alter(newTable, createIndices); + } - parsedTable.columns = parsedTable.columns.map((column) => ({ - ...column, - primary: null, - })); + async primary(columns, constraintName) { + const { createTable, createIndices } = await this.getTableSql(); - parsedTable.constraints = parsedTable.constraints.filter( - (constraint) => constraint.type !== 'PRIMARY KEY' - ); + const parsedTable = parseCreateTable(createTable); - parsedTable.constraints.push({ - type: 'PRIMARY KEY', - name: constraintName || null, - columns: columns.map((column) => ({ - name: column, - expression: false, - collation: null, - order: null, - })), - conflict: null, - }); - - const newTable = compileCreateTable(parsedTable, this.wrap); - - return this.alter(newTable, createIndices); - }, - { connection: this.connection } + parsedTable.columns = parsedTable.columns.map((column) => ({ + ...column, + primary: null, + })); + + parsedTable.constraints = parsedTable.constraints.filter( + (constraint) => constraint.type !== 'PRIMARY KEY' ); - } - foreign(foreignInfo) { - return this.client.transaction( - async (trx) => { - this.trx = trx; + parsedTable.constraints.push({ + type: 'PRIMARY KEY', + name: constraintName || null, + columns: columns.map((column) => ({ + name: column, + expression: false, + collation: null, + order: null, + })), + conflict: null, + }); - const { createTable, createIndices } = await this.getTableSql(); + const newTable = compileCreateTable(parsedTable, this.wrap); - const parsedTable = parseCreateTable(createTable); + return this.alter(newTable, createIndices); + } - parsedTable.constraints.push({ - type: 'FOREIGN KEY', - name: foreignInfo.keyName || null, - columns: foreignInfo.column, - references: { - table: foreignInfo.inTable, - columns: foreignInfo.references, - delete: foreignInfo.onDelete || null, - update: foreignInfo.onUpdate || null, - match: null, - deferrable: null, - }, - }); + async foreign(foreignInfo) { + const { createTable, createIndices } = await this.getTableSql(); + + const parsedTable = parseCreateTable(createTable); + + parsedTable.constraints.push({ + type: 'FOREIGN KEY', + name: foreignInfo.keyName || null, + columns: foreignInfo.column, + references: { + table: foreignInfo.inTable, + columns: foreignInfo.references, + delete: foreignInfo.onDelete || null, + update: foreignInfo.onUpdate || null, + match: null, + deferrable: null, + }, + }); - const newTable = compileCreateTable(parsedTable, this.wrap); + const newTable = compileCreateTable(parsedTable, this.wrap); - return this.generateAlterCommands(newTable, createIndices); - }, - { connection: this.connection } - ); + return this.generateAlterCommands(newTable, createIndices); } - setNullable(column, isNullable) { - return this.client.transaction( - async (trx) => { - this.trx = trx; + async setNullable(column, isNullable) { + const { createTable, createIndices } = await this.getTableSql(); - const { createTable, createIndices } = await this.getTableSql(); - - const parsedTable = parseCreateTable(createTable); - const parsedColumn = parsedTable.columns.find((c) => - isEqualId(column, c.name) - ); + const parsedTable = parseCreateTable(createTable); + const parsedColumn = parsedTable.columns.find((c) => + isEqualId(column, c.name) + ); - if (!parsedColumn) { - throw new Error( - `.setNullable: Column ${column} does not exist in table ${this.tableName()}.` - ); - } + if (!parsedColumn) { + throw new Error( + `.setNullable: Column ${column} does not exist in table ${this.tableName()}.` + ); + } - parsedColumn.constraints.notnull = isNullable - ? null - : { name: null, conflict: null }; + parsedColumn.constraints.notnull = isNullable + ? null + : { name: null, conflict: null }; - parsedColumn.constraints.null = isNullable - ? parsedColumn.constraints.null - : null; + parsedColumn.constraints.null = isNullable + ? parsedColumn.constraints.null + : null; - const newTable = compileCreateTable(parsedTable, this.wrap); + const newTable = compileCreateTable(parsedTable, this.wrap); - return this.generateAlterCommands(newTable, createIndices); - }, - { connection: this.connection } - ); + return this.generateAlterCommands(newTable, createIndices); } async alter(newSql, createIndices, columns) { - await this.createNewTable(newSql); - await this.copyData(columns); - await this.dropOriginal(); - await this.renameTable(); + const wasForeignCheckEnabled = await this.isForeignCheckEnabled(); - for (const createIndex of createIndices) { - await this.trx.raw(createIndex); + if (wasForeignCheckEnabled) { + await this.setForeignCheck(false); + } + + try { + await this.client.transaction( + async (trx) => { + await this.createNewTable(trx, newSql); + await this.copyData(trx, columns); + await this.dropOriginal(trx); + await this.renameTable(trx); + + for (const createIndex of createIndices) { + await trx.raw(createIndex); + } + + if (wasForeignCheckEnabled) { + const foreignViolations = await trx.raw(executeForeignCheck()); + + if (foreignViolations.length > 0) { + throw new Error('FOREIGN KEY constraint failed'); + } + } + }, + { connection: this.connection } + ); + } finally { + if (wasForeignCheckEnabled) { + await this.setForeignCheck(true); + } } } - generateAlterCommands(newSql, createIndices, columns) { - const result = []; + async generateAlterCommands(newSql, createIndices, columns) { + const sql = []; + const pre = []; + const post = []; + let check = null; - result.push(createNewTable(newSql, this.tableName(), this.alteredName)); - result.push(copyData(this.tableName(), this.alteredName, columns)); - result.push(dropOriginal(this.tableName())); - result.push(renameTable(this.alteredName, this.tableName())); + sql.push(createNewTable(newSql, this.tableName(), this.alteredName)); + sql.push(copyData(this.tableName(), this.alteredName, columns)); + sql.push(dropOriginal(this.tableName())); + sql.push(renameTable(this.alteredName, this.tableName())); for (const createIndex of createIndices) { - result.push(createIndex); + sql.push(createIndex); + } + + const isForeignCheckEnabled = await this.isForeignCheckEnabled(); + + if (isForeignCheckEnabled) { + pre.push(setForeignCheck(false)); + post.push(setForeignCheck(true)); + + check = executeForeignCheck(); } - return result; + return { pre, sql, check, post }; } } diff --git a/lib/dialects/sqlite3/schema/internal/sqlite-ddl-operations.js b/lib/dialects/sqlite3/schema/internal/sqlite-ddl-operations.js index 2340ae9c00..c51a7611ed 100644 --- a/lib/dialects/sqlite3/schema/internal/sqlite-ddl-operations.js +++ b/lib/dialects/sqlite3/schema/internal/sqlite-ddl-operations.js @@ -22,10 +22,25 @@ function getTableSql(tableName) { return `SELECT type, sql FROM sqlite_master WHERE (type='table' OR (type='index' AND sql IS NOT NULL)) AND tbl_name='${tableName}'`; } +function isForeignCheckEnabled() { + return `PRAGMA foreign_keys`; +} + +function setForeignCheck(enable) { + return `PRAGMA foreign_keys = ${enable ? 'ON' : 'OFF'}`; +} + +function executeForeignCheck() { + return `PRAGMA foreign_key_check`; +} + module.exports = { createNewTable, copyData, dropOriginal, renameTable, getTableSql, + isForeignCheckEnabled, + setForeignCheck, + executeForeignCheck, }; diff --git a/lib/dialects/sqlite3/schema/sqlite-compiler.js b/lib/dialects/sqlite3/schema/sqlite-compiler.js index 12f8c615fb..49fc050553 100644 --- a/lib/dialects/sqlite3/schema/sqlite-compiler.js +++ b/lib/dialects/sqlite3/schema/sqlite-compiler.js @@ -55,21 +55,25 @@ class SchemaCompiler_SQLite3 extends SchemaCompiler { this[query.method].apply(this, query.args); } - const result = []; const commandSources = this.sequence; - for (const commandSource of commandSources) { - const command = commandSource.statementsProducer - ? await commandSource.statementsProducer() - : commandSource.sql; - if (Array.isArray(command)) { - result.push(...command); - } else { - result.push(command); + if (commandSources.length === 1 && commandSources[0].statementsProducer) { + return commandSources[0].statementsProducer(); + } else { + const result = []; + + for (const commandSource of commandSources) { + const command = commandSource.sql; + + if (Array.isArray(command)) { + result.push(...command); + } else { + result.push(command); + } } - } - return { pre: [], sql: result, post: [] }; + return { pre: [], sql: result, check: null, post: [] }; + } } } diff --git a/lib/execution/runner.js b/lib/execution/runner.js index ce6a011b64..346e5ddc47 100644 --- a/lib/execution/runner.js +++ b/lib/execution/runner.js @@ -228,12 +228,47 @@ class Runner { undefined, this.connection ); - const queryObjects = statements.map((statement) => ({ + + const sqlQueryObjects = statements.sql.map((statement) => ({ + sql: statement, + bindings: query.bindings, + })); + const preQueryObjects = statements.pre.map((statement) => ({ sql: statement, bindings: query.bindings, })); + const postQueryObjects = statements.post.map((statement) => ({ + sql: statement, + bindings: query.bindings, + })); + + let results = []; + + await this.queryArray(preQueryObjects); + + try { + await this.client.transaction( + async (trx) => { + const transactionRunner = new Runner(trx.client, this.builder); + transactionRunner.connection = this.connection; + + results = await transactionRunner.queryArray(sqlQueryObjects); + + if (statements.check) { + const foreignViolations = await trx.raw(statements.check); + + if (foreignViolations.length > 0) { + throw new Error('FOREIGN KEY constraint failed'); + } + } + }, + { connection: this.connection } + ); + } finally { + await this.queryArray(postQueryObjects); + } - return this.queryArray(queryObjects); + return results; } const results = []; diff --git a/lib/schema/compiler.js b/lib/schema/compiler.js index 4a891686a4..8b3bbbaa6c 100644 --- a/lib/schema/compiler.js +++ b/lib/schema/compiler.js @@ -106,6 +106,7 @@ class SchemaCompiler { sql: Array.isArray(generatedCommands) ? generatedCommands : [generatedCommands], + check: null, post: [], }; } diff --git a/test/integration2/schema/foreign-keys.spec.js b/test/integration2/schema/foreign-keys.spec.js index 4544cf287a..b77ecaff52 100644 --- a/test/integration2/schema/foreign-keys.spec.js +++ b/test/integration2/schema/foreign-keys.spec.js @@ -269,6 +269,31 @@ describe('Schema', () => { ); expect(fks.length).to.equal(1); }); + + it('can alter a table in sqlite while another table has a foreign key constraint on this table', async () => { + if (!isSQLite(knex)) { + return; + } + + await knex.schema.alterTable('foreign_keys_table_two', (table) => { + table.integer('alter_column'); + }); + await knex.schema.alterTable('foreign_keys_table_one', (table) => { + table.foreign('fkey_two').references('foreign_keys_table_two.id'); + }); + + await knex('foreign_keys_table_two').insert({ alter_column: 1 }); + await knex('foreign_keys_table_one').insert({ + fkey_two: 1, + fkey_three: 1, + }); + + await expect( + knex.schema.alterTable('foreign_keys_table_two', (table) => { + table.dropColumn('alter_column'); + }) + ).to.not.be.eventually.rejected; + }); }); describe('Schema (Foreign keys)', () => { diff --git a/types/index.d.ts b/types/index.d.ts index 183aac1bee..51d5566360 100644 --- a/types/index.d.ts +++ b/types/index.d.ts @@ -1867,7 +1867,7 @@ export declare namespace Knex { readonly [Symbol.toStringTag]: string; } interface ChainableInterface extends Pick, keyof Promise & ExposedPromiseKeys>, StringTagSupport { - generateDdlCommands(): Promise<{ pre: string[], sql: string[], post: string[] }>; + generateDdlCommands(): Promise<{ pre: string[], sql: string[], check: string | null, post: string[] }>; toQuery(): string; options(options: Readonly<{ [key: string]: any }>): this; connection(connection: any): this;