From 4378a7852db3410032f52bad955d7daaa9a39224 Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Fri, 13 Dec 2024 14:03:45 +0100 Subject: [PATCH 1/7] fix(database): delete indexes before creating if they already exist --- .../core/database/src/dialects/dialect.ts | 4 +- packages/core/database/src/schema/builder.ts | 76 +++++++++++-------- 2 files changed, 49 insertions(+), 31 deletions(-) diff --git a/packages/core/database/src/dialects/dialect.ts b/packages/core/database/src/dialects/dialect.ts index 9df932948a..7c73f72d39 100644 --- a/packages/core/database/src/dialects/dialect.ts +++ b/packages/core/database/src/dialects/dialect.ts @@ -1,8 +1,10 @@ import type { Database } from '..'; -import type { Schema } from '../schema'; +import type { ForeignKey, Index, Schema } from '../schema'; export interface SchemaInspector { getSchema(): Promise; + getIndexes(tableName: string): Promise; + getForeignKeys(tableName: string): Promise; } export default class Dialect { diff --git a/packages/core/database/src/schema/builder.ts b/packages/core/database/src/schema/builder.ts index d9a2a78ab7..1940abbb37 100644 --- a/packages/core/database/src/schema/builder.ts +++ b/packages/core/database/src/schema/builder.ts @@ -111,9 +111,22 @@ const createHelpers = (db: Database) => { /** * Creates a foreign key on a table */ - const createForeignKey = (tableBuilder: Knex.TableBuilder, foreignKey: ForeignKey) => { + const createForeignKey = ( + tableBuilder: Knex.TableBuilder, + foreignKey: ForeignKey, + existingForeignKeys?: ForeignKey[] + ) => { const { name, columns, referencedColumns, referencedTable, onDelete, onUpdate } = foreignKey; + // Check if it already exists, and if so drop it before creating + // Note that it is safe to drop multiple times with TableBuilder because it only uses it to define the schema + const existingForeignKey = existingForeignKeys?.find((fk) => fk.name === name); + const forceMigration = db.config.settings?.forceMigration; + if (existingForeignKey && forceMigration) { + debug(`Dropping existing foreign key ${name}`); + tableBuilder.dropForeign(existingForeignKey.columns, name); + } + const constraint = tableBuilder .foreign(columns, name) .references(referencedColumns) @@ -140,15 +153,27 @@ const createHelpers = (db: Database) => { /** * Creates an index on a table */ - const createIndex = (tableBuilder: Knex.TableBuilder, index: Index) => { + const createIndex = ( + tableBuilder: Knex.TableBuilder, + index: Index, + existingIndexes?: Index[] + ) => { const { type, columns, name } = index; + // Check if it already exists, and if so drop it before creating + // Note that it is safe to drop multiple times with TableBuilder because it only uses it to define the schema + const existingIndex = existingIndexes?.find((existing) => existing.name === name); + const forceMigration = db.config.settings?.forceMigration; + if (forceMigration && existingIndex) { + dropIndex(tableBuilder, index); + } + switch (type) { case 'primary': { - return tableBuilder.primary(columns, name); + return tableBuilder.primary(columns, { constraintName: name }); } case 'unique': { - return tableBuilder.unique(columns, name); + return tableBuilder.unique(columns, { indexName: name }); } default: { return tableBuilder.index(columns, name, type); @@ -245,10 +270,13 @@ const createHelpers = (db: Database) => { }; const alterTable = async (schemaBuilder: Knex.SchemaBuilder, table: TableDiff['diff']) => { - await schemaBuilder.alterTable(table.name, (tableBuilder) => { - // Delete indexes / fks / columns + await schemaBuilder.alterTable(table.name, async (tableBuilder) => { + // Fetch existing indexes and foreign keys for the table so we can safely delete/create + const existingIndexes = (await db.dialect.schemaInspector.getIndexes(table.name)) || []; + const existingForeignKeys = + (await db.dialect.schemaInspector.getForeignKeys(table.name)) || []; - // Drop foreign keys first to avoid foreign key errors in the following steps + // Pre-delete foreign keys to avoid conflicts when modifying or dropping columns for (const removedForeignKey of table.foreignKeys.removed) { debug(`Dropping foreign key ${removedForeignKey.name} on ${table.name}`); dropForeignKey(tableBuilder, removedForeignKey); @@ -259,30 +287,18 @@ const createHelpers = (db: Database) => { dropForeignKey(tableBuilder, updatedForeignKey.object); } - // for mysql only, dropForeignKey also removes the index, so don't drop it twice - const isMySQL = db.config.connection.client === 'mysql'; - const ignoreForeignKeyNames = isMySQL - ? [ - ...table.foreignKeys.removed.map((fk) => fk.name), - ...table.foreignKeys.updated.map((fk) => fk.name), - ] - : []; - + // NOTE: regular index drops shouldn't be necessary and can probably be deleted for (const removedIndex of table.indexes.removed) { - if (!ignoreForeignKeyNames.includes(removedIndex.name)) { - debug(`Dropping index ${removedIndex.name} on ${table.name}`); - dropIndex(tableBuilder, removedIndex); - } + debug(`Dropping index ${removedIndex.name} on ${table.name}`); + dropIndex(tableBuilder, removedIndex); } for (const updatedIndex of table.indexes.updated) { - if (!ignoreForeignKeyNames.includes(updatedIndex.name)) { - debug(`Dropping updated index ${updatedIndex.name} on ${table.name}`); - dropIndex(tableBuilder, updatedIndex.object); - } + debug(`Dropping updated index ${updatedIndex.name} on ${table.name}`); + dropIndex(tableBuilder, updatedIndex.object); } - // We drop columns after indexes to ensure that it doesn't cascade delete any indexes we expect to exist + // Drop columns after FKs have been removed to avoid FK errors for (const removedColumn of table.columns.removed) { debug(`Dropping column ${removedColumn.name} on ${table.name}`); dropColumn(tableBuilder, removedColumn); @@ -316,22 +332,22 @@ const createHelpers = (db: Database) => { // once the columns have all been updated, we can create indexes again for (const updatedForeignKey of table.foreignKeys.updated) { debug(`Recreating updated foreign key ${updatedForeignKey.name} on ${table.name}`); - createForeignKey(tableBuilder, updatedForeignKey.object); + createForeignKey(tableBuilder, updatedForeignKey.object, existingForeignKeys); } for (const updatedIndex of table.indexes.updated) { debug(`Recreating updated index ${updatedIndex.name} on ${table.name}`); - createIndex(tableBuilder, updatedIndex.object); + createIndex(tableBuilder, updatedIndex.object, existingIndexes); } for (const addedForeignKey of table.foreignKeys.added) { - debug(`Creating foreign keys ${addedForeignKey.name} on ${table.name}`); - createForeignKey(tableBuilder, addedForeignKey); + debug(`Creating foreign key ${addedForeignKey.name} on ${table.name}`); + createForeignKey(tableBuilder, addedForeignKey, existingForeignKeys); } for (const addedIndex of table.indexes.added) { debug(`Creating index ${addedIndex.name} on ${table.name}`); - createIndex(tableBuilder, addedIndex); + createIndex(tableBuilder, addedIndex, existingIndexes); } }); }; From e350eaa6073e65190102b4b798c32c287053cc02 Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Fri, 13 Dec 2024 14:20:58 +0100 Subject: [PATCH 2/7] chore: fix comment --- packages/core/database/src/schema/builder.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/core/database/src/schema/builder.ts b/packages/core/database/src/schema/builder.ts index 1940abbb37..cb282bb1bd 100644 --- a/packages/core/database/src/schema/builder.ts +++ b/packages/core/database/src/schema/builder.ts @@ -276,7 +276,7 @@ const createHelpers = (db: Database) => { const existingForeignKeys = (await db.dialect.schemaInspector.getForeignKeys(table.name)) || []; - // Pre-delete foreign keys to avoid conflicts when modifying or dropping columns + // Drop foreign keys first to avoid foreign key errors in the following steps for (const removedForeignKey of table.foreignKeys.removed) { debug(`Dropping foreign key ${removedForeignKey.name} on ${table.name}`); dropForeignKey(tableBuilder, removedForeignKey); @@ -287,7 +287,10 @@ const createHelpers = (db: Database) => { dropForeignKey(tableBuilder, updatedForeignKey.object); } - // NOTE: regular index drops shouldn't be necessary and can probably be deleted + /** + * NOTE: regular index drops shouldn't be necessary because creates include dropping existing + * but since it doesn't result in any additional queries, we can keep the code for safety + * */ for (const removedIndex of table.indexes.removed) { debug(`Dropping index ${removedIndex.name} on ${table.name}`); dropIndex(tableBuilder, removedIndex); From 27465bf4d78863eadda95504aaa4c16113868d1f Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Fri, 13 Dec 2024 18:33:47 +0100 Subject: [PATCH 3/7] fix: check indexes before transaction --- packages/core/database/src/schema/builder.ts | 29 +++++++++++++++----- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/packages/core/database/src/schema/builder.ts b/packages/core/database/src/schema/builder.ts index cb282bb1bd..5ee52df503 100644 --- a/packages/core/database/src/schema/builder.ts +++ b/packages/core/database/src/schema/builder.ts @@ -73,6 +73,16 @@ export default (db: Database) => { const forceMigration = db.config.settings?.forceMigration; await db.dialect.startSchemaUpdate(); + + // Pre-fetch metadata for all updated tables + const existingMetadata: Record = {}; + for (const table of schemaDiff.tables.updated) { + existingMetadata[table.name] = { + indexes: await db.dialect.schemaInspector.getIndexes(table.name), + foreignKeys: await db.dialect.schemaInspector.getForeignKeys(table.name), + }; + } + await db.connection.transaction(async (trx) => { await this.createTables(schemaDiff.tables.added, trx); @@ -98,7 +108,8 @@ export default (db: Database) => { // alter table const schemaBuilder = this.getSchemaBuilder(trx); - await helpers.alterTable(schemaBuilder, table); + const { indexes, foreignKeys } = existingMetadata[table.name]; + await helpers.alterTable(schemaBuilder, table, { indexes, foreignKeys }); } }); @@ -269,13 +280,17 @@ const createHelpers = (db: Database) => { }); }; - const alterTable = async (schemaBuilder: Knex.SchemaBuilder, table: TableDiff['diff']) => { - await schemaBuilder.alterTable(table.name, async (tableBuilder) => { - // Fetch existing indexes and foreign keys for the table so we can safely delete/create - const existingIndexes = (await db.dialect.schemaInspector.getIndexes(table.name)) || []; - const existingForeignKeys = - (await db.dialect.schemaInspector.getForeignKeys(table.name)) || []; + const alterTable = async ( + schemaBuilder: Knex.SchemaBuilder, + table: TableDiff['diff'], + existingMetadata: { indexes: Index[]; foreignKeys: ForeignKey[] } = { + indexes: [], + foreignKeys: [], + } + ) => { + const { indexes: existingIndexes, foreignKeys: existingForeignKeys } = existingMetadata; + await schemaBuilder.alterTable(table.name, async (tableBuilder) => { // Drop foreign keys first to avoid foreign key errors in the following steps for (const removedForeignKey of table.foreignKeys.removed) { debug(`Dropping foreign key ${removedForeignKey.name} on ${table.name}`); From 6a649cd9b7cff8962f58389981fa6c07c3b7ae83 Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Mon, 16 Dec 2024 09:40:09 +0100 Subject: [PATCH 4/7] revert: reimplement fix for mysql fks --- packages/core/database/src/schema/builder.ts | 40 ++++++++++++++++---- 1 file changed, 32 insertions(+), 8 deletions(-) diff --git a/packages/core/database/src/schema/builder.ts b/packages/core/database/src/schema/builder.ts index 5ee52df503..98d2e033df 100644 --- a/packages/core/database/src/schema/builder.ts +++ b/packages/core/database/src/schema/builder.ts @@ -197,11 +197,17 @@ const createHelpers = (db: Database) => { * @param {Knex.TableBuilder} tableBuilder * @param {Index} index */ - const dropIndex = (tableBuilder: Knex.TableBuilder, index: Index) => { + const dropIndex = (tableBuilder: Knex.TableBuilder, index: Index, existingIndexes?: Index[]) => { if (!db.config.settings?.forceMigration) { return; } + // Check if the index exists in existingIndexes, and return early if it doesn't + if (existingIndexes && !existingIndexes.some((existingIndex) => existingIndex.name === name)) { + debug(`Index ${index.name} not found in existingIndexes. Skipping drop.`); + return; + } + const { type, columns, name } = index; switch (type) { @@ -288,32 +294,50 @@ const createHelpers = (db: Database) => { foreignKeys: [], } ) => { - const { indexes: existingIndexes, foreignKeys: existingForeignKeys } = existingMetadata; + let existingIndexes = [...existingMetadata.indexes]; + const existingForeignKeys = [...existingMetadata.foreignKeys]; + + // In MySQL, dropping a foreign key can also implicitly drop an index with the same name + const isMySQL = db.config.connection.client === 'mysql'; + + // Track dropped foreign keys + const droppedForeignKeyNames: string[] = []; await schemaBuilder.alterTable(table.name, async (tableBuilder) => { // Drop foreign keys first to avoid foreign key errors in the following steps for (const removedForeignKey of table.foreignKeys.removed) { debug(`Dropping foreign key ${removedForeignKey.name} on ${table.name}`); dropForeignKey(tableBuilder, removedForeignKey); + + if (isMySQL) { + droppedForeignKeyNames.push(removedForeignKey.name); + } } for (const updatedForeignKey of table.foreignKeys.updated) { debug(`Dropping updated foreign key ${updatedForeignKey.name} on ${table.name}`); dropForeignKey(tableBuilder, updatedForeignKey.object); + + if (isMySQL) { + droppedForeignKeyNames.push(updatedForeignKey.object.name); + } + } + + // Remove dropped foreign keys from existingIndexes for MySQL + if (isMySQL) { + existingIndexes = existingIndexes.filter( + (index) => !droppedForeignKeyNames.includes(index.name) + ); } - /** - * NOTE: regular index drops shouldn't be necessary because creates include dropping existing - * but since it doesn't result in any additional queries, we can keep the code for safety - * */ for (const removedIndex of table.indexes.removed) { debug(`Dropping index ${removedIndex.name} on ${table.name}`); - dropIndex(tableBuilder, removedIndex); + dropIndex(tableBuilder, removedIndex, existingIndexes); } for (const updatedIndex of table.indexes.updated) { debug(`Dropping updated index ${updatedIndex.name} on ${table.name}`); - dropIndex(tableBuilder, updatedIndex.object); + dropIndex(tableBuilder, updatedIndex.object, existingIndexes); } // Drop columns after FKs have been removed to avoid FK errors From cfda358b7f27015e34e739b8742a2962ae2e7aee Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Mon, 16 Dec 2024 09:41:08 +0100 Subject: [PATCH 5/7] chore: refactor --- packages/core/database/src/schema/builder.ts | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/packages/core/database/src/schema/builder.ts b/packages/core/database/src/schema/builder.ts index 98d2e033df..85b422f165 100644 --- a/packages/core/database/src/schema/builder.ts +++ b/packages/core/database/src/schema/builder.ts @@ -297,9 +297,6 @@ const createHelpers = (db: Database) => { let existingIndexes = [...existingMetadata.indexes]; const existingForeignKeys = [...existingMetadata.foreignKeys]; - // In MySQL, dropping a foreign key can also implicitly drop an index with the same name - const isMySQL = db.config.connection.client === 'mysql'; - // Track dropped foreign keys const droppedForeignKeyNames: string[] = []; @@ -309,22 +306,19 @@ const createHelpers = (db: Database) => { debug(`Dropping foreign key ${removedForeignKey.name} on ${table.name}`); dropForeignKey(tableBuilder, removedForeignKey); - if (isMySQL) { - droppedForeignKeyNames.push(removedForeignKey.name); - } + droppedForeignKeyNames.push(removedForeignKey.name); } for (const updatedForeignKey of table.foreignKeys.updated) { debug(`Dropping updated foreign key ${updatedForeignKey.name} on ${table.name}`); dropForeignKey(tableBuilder, updatedForeignKey.object); - if (isMySQL) { - droppedForeignKeyNames.push(updatedForeignKey.object.name); - } + droppedForeignKeyNames.push(updatedForeignKey.object.name); } + // In MySQL, dropping a foreign key can also implicitly drop an index with the same name // Remove dropped foreign keys from existingIndexes for MySQL - if (isMySQL) { + if (db.config.connection.client === 'mysql') { existingIndexes = existingIndexes.filter( (index) => !droppedForeignKeyNames.includes(index.name) ); From f47bf90b3f2b7c7d585faae4aef29a953c3f535a Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Mon, 16 Dec 2024 10:30:00 +0100 Subject: [PATCH 6/7] chore: add header comment --- packages/core/database/src/schema/builder.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/packages/core/database/src/schema/builder.ts b/packages/core/database/src/schema/builder.ts index 85b422f165..fc360b77f9 100644 --- a/packages/core/database/src/schema/builder.ts +++ b/packages/core/database/src/schema/builder.ts @@ -286,6 +286,18 @@ const createHelpers = (db: Database) => { }); }; + /** + * Alters a database table by applying a set of schema changes including updates to columns, indexes, and foreign keys. + * This function ensures proper ordering of operations to avoid conflicts (e.g., foreign key errors) and handles + * MySQL-specific quirks where dropping a foreign key can implicitly drop an associated index. + * + * @param {Knex.SchemaBuilder} schemaBuilder - Knex SchemaBuilder instance to perform schema operations. + * @param {TableDiff['diff']} table - A diff object representing the schema changes to be applied to the table. + * @param {{ indexes: Index[]; foreignKeys: ForeignKey[] }} existingMetadata - Metadata about existing indexes and + * foreign keys in the table. Used to ensure safe operations and avoid unnecessary modifications. + * - indexes: Array of existing index definitions. + * - foreignKeys: Array of existing foreign key definitions. + */ const alterTable = async ( schemaBuilder: Knex.SchemaBuilder, table: TableDiff['diff'], From 8f227a605a76a9cea2099909c3f618a141e50086 Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Tue, 17 Dec 2024 14:51:17 +0100 Subject: [PATCH 7/7] fix: name must be initialized --- packages/core/database/src/schema/builder.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/core/database/src/schema/builder.ts b/packages/core/database/src/schema/builder.ts index fc360b77f9..f49ce4c0dd 100644 --- a/packages/core/database/src/schema/builder.ts +++ b/packages/core/database/src/schema/builder.ts @@ -202,14 +202,14 @@ const createHelpers = (db: Database) => { return; } + const { type, columns, name } = index; + // Check if the index exists in existingIndexes, and return early if it doesn't - if (existingIndexes && !existingIndexes.some((existingIndex) => existingIndex.name === name)) { + if (existingIndexes && !existingIndexes.some((existingIndex) => existingIndex?.name === name)) { debug(`Index ${index.name} not found in existingIndexes. Skipping drop.`); return; } - const { type, columns, name } = index; - switch (type) { case 'primary': { return tableBuilder.dropPrimary(name);