Merge pull request #1293 from dbgate/feature/FK-test

Feature/fk test
2026-04-17 21:26:00 +00:00 · 2025-12-09 16:22:29 +01:00
parent eda70def2a 377cd64556
commit 7a8ff89c5c
8 changed files with 250 additions and 66 deletions
--- a/integration-tests/tests/alter-table.spec.js
+++ b/integration-tests/tests/alter-table.spec.js
@@ -26,13 +26,15 @@ function pickImportantTableInfo(engine, table) {
      .map(props =>
        _.omitBy(props, (v, k) => k == 'defaultValue' && v == 'NULL' && engine.setNullDefaultInsteadOfDrop)
      ),
-    // foreignKeys: table.foreignKeys
-    //   .sort((a, b) => a.refTableName.localeCompare(b.refTableName))
-    //   .map(fk => ({
-    //     constraintType: fk.constraintType,
-    //     refTableName: fk.refTableName,
-    //     columns: fk.columns.map(col => ({ columnName: col.columnName, refColumnName: col.refColumnName })),
-    //   })),
+
+      // TODO:
+    foreignKeys: table.foreignKeys
+      .sort((a, b) => a.refTableName.localeCompare(b.refTableName))
+      .map(fk => ({
+        constraintType: fk.constraintType,
+        refTableName: fk.refTableName,
+        columns: fk.columns.map(col => ({ columnName: col.columnName, refColumnName: col.refColumnName })),
+      })),
  };
 }

@@ -103,6 +105,7 @@ async function testTableDiff(engine, conn, driver, mangle, changedTable = 't1')

  await driver.script(conn, sql);

+  // TODO:
  // if (!engine.skipIncrementalAnalysis) {
  //   const structure2RealIncremental = await driver.analyseIncremental(conn, structure1Source);
  //   checkTableStructure(engine, tget(structure2RealIncremental), tget(structure2));
@@ -116,6 +119,7 @@ async function testTableDiff(engine, conn, driver, mangle, changedTable = 't1')

 const TESTED_COLUMNS = ['col_pk', 'col_std', 'col_def', 'col_fk', 'col_ref', 'col_idx', 'col_uq'];
 // const TESTED_COLUMNS = ['col_pk'];
+// const TESTED_COLUMNS = ['col_fk'];
 // const TESTED_COLUMNS = ['col_idx'];
 // const TESTED_COLUMNS = ['col_def'];
 // const TESTED_COLUMNS = ['col_std'];
@@ -179,11 +183,25 @@ describe('Alter table', () => {
  )(
    'Drop column - %s - %s',
    testWrapper(async (conn, driver, column, engine) => {
-      await testTableDiff(engine, conn, driver, tbl => (tbl.columns = tbl.columns.filter(x => x.columnName != column)));
+      await testTableDiff(engine, conn, driver, 
+        tbl => {
+          tbl.columns = tbl.columns.filter(x => x.columnName != column);
+          tbl.foreignKeys = tbl.foreignKeys
+            .map(fk => ({
+              ...fk, 
+              columns: fk.columns.filter(col => col.columnName != column)
+            }))
+            .filter(fk => fk.columns.length > 0);
+        }
+      );
    })
  );

-  test.each(createEnginesColumnsSource(engines.filter(x => !x.skipNullable && !x.skipChangeNullability)))(
+  test.each(
+    createEnginesColumnsSource(engines.filter(x => !x.skipNullability && !x.skipChangeNullability)).filter(
+      ([_label, col]) => !col.endsWith('_pk')
+    )
+  )(
    'Change nullability - %s - %s',
    testWrapper(async (conn, driver, column, engine) => {
      await testTableDiff(
@@ -202,7 +220,11 @@ describe('Alter table', () => {
        engine,
        conn,
        driver,
-        tbl => (tbl.columns = tbl.columns.map(x => (x.columnName == column ? { ...x, columnName: 'col_renamed' } : x)))
+        tbl => {
+          tbl.columns = tbl.columns.map(x => (x.columnName == column ? { ...x, columnName: 'col_renamed' } : x));
+          tbl.foreignKeys = tbl.foreignKeys.map(fk => ({...fk, columns: fk.columns.map(col => col.columnName == column ? { ...col, columnName: 'col_renamed' } : col)
+          }));
+        }
      );
    })
  );
--- a/integration-tests/docker-compose.yaml
+++ b/integration-tests/docker-compose.yaml
@@ -44,7 +44,7 @@ services:
  #     - 15942:9042
  #
  # clickhouse:
-  #   image: bitnami/clickhouse:24.8.4
+  #   image: bitnamilegacy/clickhouse:24.8.4
  #   restart: always
  #   ports:
  #     - 15005:8123
--- a/integration-tests/tools.js
+++ b/integration-tests/tools.js
@@ -22,7 +22,9 @@ async function connect(engine, database) {
  if (engine.generateDbFile) {
    const conn = await driver.connect({
      ...connection,
-      databaseFile: (engine.databaseFileLocationOnServer ?? 'dbtemp/') + database,
+      databaseFile:
+        (engine.databaseFileLocationOnServer ?? (process.env.CITEST ? 'dbtemp/' : 'integration-tests/dbtemp/')) +
+        database,
    });
    return conn;
  } else {
--- a/packages/tools/src/SqlDumper.ts
+++ b/packages/tools/src/SqlDumper.ts
@@ -26,6 +26,7 @@ import _isDate from 'lodash/isDate';
 import _isArray from 'lodash/isArray';
 import _isPlainObject from 'lodash/isPlainObject';
 import _keys from 'lodash/keys';
+import _cloneDeep from 'lodash/cloneDeep';
 import uuidv1 from 'uuid/v1';

 export class SqlDumper implements AlterProcessor {
@@ -667,6 +668,68 @@ export class SqlDumper implements AlterProcessor {
    }
  }

+  sanitizeTableConstraints(table: TableInfo): TableInfo {
+    // Create a deep copy of the table
+    const sanitized = _cloneDeep(table);
+    
+    // Get the set of existing column names
+    const existingColumns = new Set(sanitized.columns.map(col => col.columnName));
+    
+    // Filter primary key columns to only include existing columns
+    if (sanitized.primaryKey) {
+      const validPkColumns = sanitized.primaryKey.columns.filter(col => existingColumns.has(col.columnName));
+      if (validPkColumns.length === 0) {
+        // If no valid columns remain, remove the primary key entirely
+        sanitized.primaryKey = null;
+      } else if (validPkColumns.length < sanitized.primaryKey.columns.length) {
+        // Update primary key with only valid columns
+        sanitized.primaryKey = {
+          ...sanitized.primaryKey,
+          columns: validPkColumns
+        };
+      }
+    }
+    
+    // Filter sorting key columns to only include existing columns
+    if (sanitized.sortingKey) {
+      const validSkColumns = sanitized.sortingKey.columns.filter(col => existingColumns.has(col.columnName));
+      if (validSkColumns.length === 0) {
+        sanitized.sortingKey = null;
+      } else if (validSkColumns.length < sanitized.sortingKey.columns.length) {
+        sanitized.sortingKey = {
+          ...sanitized.sortingKey,
+          columns: validSkColumns
+        };
+      }
+    }
+    
+    // Filter foreign keys to only include those with all columns present
+    if (sanitized.foreignKeys) {
+      sanitized.foreignKeys = sanitized.foreignKeys.filter(fk =>
+        fk.columns.every(col => existingColumns.has(col.columnName))
+      );
+    }
+    
+    // Filter indexes to only include those with all columns present
+    if (sanitized.indexes) {
+      sanitized.indexes = sanitized.indexes.filter(idx =>
+        idx.columns.every(col => existingColumns.has(col.columnName))
+      );
+    }
+    
+    // Filter unique constraints to only include those with all columns present
+    if (sanitized.uniques) {
+      sanitized.uniques = sanitized.uniques.filter(uq =>
+        uq.columns.every(col => existingColumns.has(col.columnName))
+      );
+    }
+    
+    // Filter dependencies (references from other tables) - these should remain as-is
+    // since they don't affect the CREATE TABLE statement for this table
+    
+    return sanitized;
+  }
+
  recreateTable(oldTable: TableInfo, newTable: TableInfo) {
    if (!oldTable.pairingId || !newTable.pairingId || oldTable.pairingId != newTable.pairingId) {
      throw new Error('Recreate is not possible: oldTable.paringId != newTable.paringId');
@@ -681,48 +744,51 @@ export class SqlDumper implements AlterProcessor {
      }))
      .filter(x => x.newcol);

+    // Create a sanitized version of newTable with constraints that only reference existing columns
+    const sanitizedNewTable = this.sanitizeTableConstraints(newTable);
+
    if (this.driver.supportsTransactions) {
      this.dropConstraints(oldTable, true);
      this.renameTable(oldTable, tmpTable);

-      this.createTable(newTable);
+      this.createTable(sanitizedNewTable);

-      const autoinc = newTable.columns.find(x => x.autoIncrement);
+      const autoinc = sanitizedNewTable.columns.find(x => x.autoIncrement);
      if (autoinc) {
-        this.allowIdentityInsert(newTable, true);
+        this.allowIdentityInsert(sanitizedNewTable, true);
      }

      this.putCmd(
        '^insert ^into %f (%,i) select %,i ^from %f',
-        newTable,
+        sanitizedNewTable,
        columnPairs.map(x => x.newcol.columnName),
        columnPairs.map(x => x.oldcol.columnName),
        { ...oldTable, pureName: tmpTable }
      );

      if (autoinc) {
-        this.allowIdentityInsert(newTable, false);
+        this.allowIdentityInsert(sanitizedNewTable, false);
      }

      if (this.dialect.dropForeignKey) {
-        newTable.dependencies.forEach(cnt => this.createConstraint(cnt));
+        sanitizedNewTable.dependencies.forEach(cnt => this.createConstraint(cnt));
      }

      this.dropTable({ ...oldTable, pureName: tmpTable });
    } else {
      // we have to preserve old table as long as possible
-      this.createTable({ ...newTable, pureName: tmpTable });
+      this.createTable({ ...sanitizedNewTable, pureName: tmpTable });

      this.putCmd(
        '^insert ^into %f (%,i) select %,s ^from %f',
-        { ...newTable, pureName: tmpTable },
+        { ...sanitizedNewTable, pureName: tmpTable },
        columnPairs.map(x => x.newcol.columnName),
        columnPairs.map(x => x.oldcol.columnName),
        oldTable
      );

      this.dropTable(oldTable);
-      this.renameTable({ ...newTable, pureName: tmpTable }, newTable.pureName);
+      this.renameTable({ ...sanitizedNewTable, pureName: tmpTable }, newTable.pureName);
    }
  }

--- a/packages/tools/src/alterPlan.ts
+++ b/packages/tools/src/alterPlan.ts
@@ -91,8 +91,8 @@ interface AlterOperation_RenameConstraint {
 }
 interface AlterOperation_RecreateTable {
  operationType: 'recreateTable';
-  table: TableInfo;
-  operations: AlterOperation[];
+  oldTable: TableInfo;
+  newTable: TableInfo;
 }
 interface AlterOperation_FillPreloadedRows {
  operationType: 'fillPreloadedRows';
@@ -249,11 +249,11 @@ export class AlterPlan {
    });
  }

-  recreateTable(table: TableInfo, operations: AlterOperation[]) {
+  recreateTable(oldTable: TableInfo, newTable: TableInfo) {
    this.operations.push({
      operationType: 'recreateTable',
-      table,
-      operations,
+      oldTable,
+      newTable,
    });
    this.recreates.tables += 1;
  }
@@ -337,7 +337,13 @@ export class AlterPlan {
            return opRes;
          }),
          op,
-        ];
+        ].filter(op => {
+          // filter duplicated drops
+          const existingDrop = this.operations.find(
+            o => o.operationType == 'dropConstraint' && o.oldObject === op['oldObject']
+          );
+          return existingDrop == null;
+        });
        return res;
      }

@@ -498,53 +504,121 @@ export class AlterPlan {
        return [];
      }

-      const table = this.wholeNewDb.tables.find(
+      const oldTable = this.wholeOldDb.tables.find(
+        x => x.pureName == op[objectField].pureName && x.schemaName == op[objectField].schemaName
+      );
+      const newTable = this.wholeNewDb.tables.find(
        x => x.pureName == op[objectField].pureName && x.schemaName == op[objectField].schemaName
      );
      this.recreates.tables += 1;
      return [
        {
          operationType: 'recreateTable',
-          table,
-          operations: [op],
+          oldTable,
+          newTable,
+          // operations: [op],
        },
      ];
    }
    return null;
  }

-  _groupTableRecreations(): AlterOperation[] {
-    const res = [];
-    const recreates = {};
+  _removeRecreatedTableAlters(): AlterOperation[] {
+    const res: AlterOperation[] = [];
+    const recreates = new Set<string>();
    for (const op of this.operations) {
-      if (op.operationType == 'recreateTable' && op.table) {
-        const existingRecreate = recreates[`${op.table.schemaName}||${op.table.pureName}`];
-        if (existingRecreate) {
-          existingRecreate.operations.push(...op.operations);
-        } else {
-          const recreate = {
-            ...op,
-            operations: [...op.operations],
-          };
-          res.push(recreate);
-          recreates[`${op.table.schemaName}||${op.table.pureName}`] = recreate;
-        }
-      } else {
-        // @ts-ignore
-        const oldObject: TableInfo = op.oldObject || op.object;
-        if (oldObject) {
-          const recreated = recreates[`${oldObject.schemaName}||${oldObject.pureName}`];
-          if (recreated) {
-            recreated.operations.push(op);
-            continue;
-          }
-        }
-        res.push(op);
+      if (op.operationType == 'recreateTable' && op.oldTable && op.newTable) {
+        const key = `${op.oldTable.schemaName}||${op.oldTable.pureName}`;
+        recreates.add(key);
      }
    }
+
+    for (const op of this.operations) {
+      switch (op.operationType) {
+        case 'createColumn':
+        case 'createConstraint':
+          {
+            const key = `${op.newObject.schemaName}||${op.newObject.pureName}`;
+            if (recreates.has(key)) {
+              // skip create inside recreated table
+              continue;
+            }
+          }
+          break;
+        case 'dropColumn':
+        case 'dropConstraint':
+        case 'changeColumn':
+          {
+            const key = `${op.oldObject.schemaName}||${op.oldObject.pureName}`;
+            if (recreates.has(key)) {
+              // skip drop/change inside recreated table
+              continue;
+            }
+          }
+          break;
+        case 'renameColumn':
+          {
+            const key = `${op.object.schemaName}||${op.object.pureName}`;
+            if (recreates.has(key)) {
+              // skip rename inside recreated table
+              continue;
+            }
+          }
+          break;
+      }
+      res.push(op);
+    }
    return res;
  }

+  _groupTableRecreations(): AlterOperation[] {
+    const res = [];
+    const recreates = new Set<string>();
+    for (const op of this.operations) {
+      if (op.operationType == 'recreateTable' && op.oldTable && op.newTable) {
+        const key = `${op.oldTable.schemaName}||${op.oldTable.pureName}`;
+        if (recreates.has(key)) {
+          // prevent duplicate recreates
+          continue;
+        }
+        recreates.add(key);
+      }
+
+      res.push(op);
+    }
+    return res;
+
+    // const res = [];
+    // const recreates = {};
+    // for (const op of this.operations) {
+    //   if (op.operationType == 'recreateTable' && op.table) {
+    //     const existingRecreate = recreates[`${op.table.schemaName}||${op.table.pureName}`];
+    //     if (existingRecreate) {
+    //       existingRecreate.operations.push(...op.operations);
+    //     } else {
+    //       const recreate = {
+    //         ...op,
+    //         operations: [...op.operations],
+    //       };
+    //       res.push(recreate);
+    //       recreates[`${op.table.schemaName}||${op.table.pureName}`] = recreate;
+    //     }
+    //   } else {
+    //     // @ts-ignore
+    //     const oldObject: TableInfo = op.oldObject || op.object;
+    //     if (oldObject) {
+    //       const recreated = recreates[`${oldObject.schemaName}||${oldObject.pureName}`];
+    //       if (recreated) {
+    //         recreated.operations.push(op);
+    //         continue;
+    //       }
+    //     }
+    //     res.push(op);
+    //   }
+    // }
+    // return res;
+  }
+
  _moveForeignKeysToLast(): AlterOperation[] {
    if (!this.dialect.createForeignKey) {
      return this.operations;
@@ -611,6 +685,8 @@ export class AlterPlan {

    // console.log('*****************OPERATIONS3', this.operations);

+    this.operations = this._removeRecreatedTableAlters();
+
    this.operations = this._moveForeignKeysToLast();

    // console.log('*****************OPERATIONS4', this.operations);
@@ -673,16 +749,16 @@ export function runAlterOperation(op: AlterOperation, processor: AlterProcessor)
      break;
    case 'recreateTable':
      {
-        const oldTable = generateTablePairingId(op.table);
-        const newTable = _.cloneDeep(oldTable);
-        const newDb = DatabaseAnalyser.createEmptyStructure();
-        newDb.tables.push(newTable);
-        // console.log('////////////////////////////newTable1', newTable);
-        op.operations.forEach(child => runAlterOperation(child, new DatabaseInfoAlterProcessor(newDb)));
-        // console.log('////////////////////////////op.operations', op.operations);
-        // console.log('////////////////////////////op.table', op.table);
-        // console.log('////////////////////////////newTable2', newTable);
-        processor.recreateTable(oldTable, newTable);
+        // const oldTable = generateTablePairingId(op.table);
+        // const newTable = _.cloneDeep(oldTable);
+        // const newDb = DatabaseAnalyser.createEmptyStructure();
+        // newDb.tables.push(newTable);
+        // // console.log('////////////////////////////newTable1', newTable);
+        // op.operations.forEach(child => runAlterOperation(child, new DatabaseInfoAlterProcessor(newDb)));
+        // // console.log('////////////////////////////op.operations', op.operations);
+        // // console.log('////////////////////////////op.table', op.table);
+        // // console.log('////////////////////////////newTable2', newTable);
+        processor.recreateTable(op.oldTable, op.newTable);
      }
      break;
  }
--- a/plugins/dbgate-plugin-mysql/src/frontend/drivers.js
+++ b/plugins/dbgate-plugin-mysql/src/frontend/drivers.js
@@ -46,6 +46,8 @@ const dialect = {
  dropReferencesWhenDropTable: false,
  requireStandaloneSelectForScopeIdentity: true,

+  dropColumnDependencies: ['dependencies'],
+
  columnProperties: {
    columnComment: true,
    isUnsigned: true,
--- a/plugins/dbgate-plugin-oracle/src/frontend/Dumper.js
+++ b/plugins/dbgate-plugin-oracle/src/frontend/Dumper.js
@@ -64,6 +64,21 @@ class Dumper extends SqlDumper {
    this.putCmd('^alter ^table %f ^rename ^column %i ^to %i', column, column.columnName, newcol);
  }

+  createForeignKeyFore(fk) {
+    if (fk.constraintName != null && !this.dialect.anonymousForeignKey) {
+      this.put('^constraint %i ', fk.constraintName);
+    }
+    this.put(
+      '^foreign ^key (%,i) ^references %f (%,i)',
+      fk.columns.map(x => x.columnName),
+      { schemaName: fk.refSchemaName, pureName: fk.refTableName },
+      fk.columns.map(x => x.refColumnName)
+    );
+    if (fk.deleteAction && fk.deleteAction.toUpperCase() !== 'NO ACTION') {
+      this.put(' ^on ^delete %k', fk.deleteAction);
+    }
+  }
+
  // dropTable(obj, options = {}) {
  //   this.put('^drop ^table');
  //   if (options.testIfExists) this.put(' ^if ^exists');
--- a/plugins/dbgate-plugin-sqlite/src/backend/Analyser.js
+++ b/plugins/dbgate-plugin-sqlite/src/backend/Analyser.js
@@ -129,6 +129,7 @@ class Analyser extends DatabaseAnalyser {
          updateAction: fkcol.on_update,
          deleteAction: fkcol.on_delete,
          constraintName: `FK_${tableName}_${fkcol.id}`,
+          constraintType: 'foreignKey',
        };
        return fk;
      });