From 81587313eb8a6da0eada61704e924f013f639d6d Mon Sep 17 00:00:00 2001 From: t8y2 <1156263951@qq.com> Date: Fri, 4 Sep 2026 00:37:27 +0800 Subject: [PATCH] fix(schema-diff): disambiguate duplicate child IDs Closes #8067 --- .../schema/schemaDiffSelection.spec.ts | 55 ++++++++- apps/desktop/src/lib/schema/schemaDiff.ts | 114 ++++++++++-------- 2 files changed, 121 insertions(+), 48 deletions(-) diff --git a/apps/desktop/src/lib/__tests__/schema/schemaDiffSelection.spec.ts b/apps/desktop/src/lib/__tests__/schema/schemaDiffSelection.spec.ts index d03ba4c85c..294ac1a692 100644 --- a/apps/desktop/src/lib/__tests__/schema/schemaDiffSelection.spec.ts +++ b/apps/desktop/src/lib/__tests__/schema/schemaDiffSelection.spec.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { convertToSchemaDiffObjects, normalizeSchemaDiffDependencyGraph, selectSchemaDiffInput, selectSchemaDiffInputForObject, setSchemaDiffObjectSelected, type SchemaDiffPreparation } from "../../schema/schemaDiff"; +import { convertToSchemaDiffObjects, normalizeSchemaDiffDependencyGraph, schemaDiffObjectSelectionState, selectSchemaDiffInput, selectSchemaDiffInputForObject, setSchemaDiffObjectSelected, type SchemaDiffPreparation } from "../../schema/schemaDiff"; function createPreparation(): SchemaDiffPreparation { return { @@ -24,6 +24,31 @@ function createPreparation(): SchemaDiffPreparation { }; } +function createDuplicateForeignKeyPreparation(): SchemaDiffPreparation { + return { + diffs: [ + { + type: "modified", + objectType: "table", + name: "orders", + foreignKeys: [ + { + type: "removed", + name: "fk_customer", + target: { name: "fk_customer", column: "legacy_customer_id", ref_table: "customers", ref_column: "id" }, + }, + { + type: "added", + name: "fk_customer", + source: { name: "fk_customer", column: "customer_id", ref_table: "customers", ref_column: "id" }, + }, + ], + }, + ], + syncSql: "-- orders", + }; +} + describe("schema diff SQL selection projections", () => { it("keeps the focused table SQL separate from the full selected SQL", () => { const result = createPreparation(); @@ -62,6 +87,34 @@ describe("schema diff SQL selection projections", () => { expect(focusedInput.diffs[0]?.columns?.map((column) => column.name)).toEqual(["a_col", "a_other_col"]); }); + it("selects duplicate-name foreign keys independently and projects only the selected one", () => { + const result = createDuplicateForeignKeyPreparation(); + const objects = convertToSchemaDiffObjects(result.diffs); + const tableObject = objects[0]!; + const [legacyForeignKey, currentForeignKey] = tableObject.children ?? []; + const foreignKeyIds = tableObject.children?.map((child) => child.id) ?? []; + + expect(foreignKeyIds).toHaveLength(2); + expect(new Set(foreignKeyIds).size).toBe(2); + expect(schemaDiffObjectSelectionState(tableObject)).toEqual({ checked: true, indeterminate: false }); + + expect(legacyForeignKey && setSchemaDiffObjectSelected(objects, legacyForeignKey.id, false)).toBe(true); + expect(legacyForeignKey?.selected).toBe(false); + expect(currentForeignKey?.selected).toBe(true); + expect(tableObject.selected).toBe(false); + expect(schemaDiffObjectSelectionState(tableObject)).toEqual({ checked: false, indeterminate: true }); + + const selectedInput = selectSchemaDiffInput(result, objects); + expect(selectedInput.diffs[0]?.foreignKeys?.map((foreignKey) => foreignKey.source?.column)).toEqual(["customer_id"]); + + expect(currentForeignKey && setSchemaDiffObjectSelected(objects, currentForeignKey.id, false)).toBe(true); + expect(schemaDiffObjectSelectionState(tableObject)).toEqual({ checked: false, indeterminate: false }); + expect(selectSchemaDiffInput(result, objects).diffs).toEqual([]); + + const rebuiltObjects = convertToSchemaDiffObjects(result.diffs); + expect(rebuiltObjects[0]?.children?.map((child) => child.id)).toEqual(foreignKeyIds); + }); + it("returns an empty input for an object that is no longer present", () => { const result = createPreparation(); const objects = convertToSchemaDiffObjects(result.diffs); diff --git a/apps/desktop/src/lib/schema/schemaDiff.ts b/apps/desktop/src/lib/schema/schemaDiff.ts index 766f49cfb5..6b2e3253a1 100644 --- a/apps/desktop/src/lib/schema/schemaDiff.ts +++ b/apps/desktop/src/lib/schema/schemaDiff.ts @@ -391,20 +391,26 @@ function tableObjectId(tableName: string): string { return `table-${tableName}`; } -function columnObjectId(tableName: string, columnName: string): string { - return `col-${tableName}-${columnName}`; +type SchemaDiffChildKind = "column" | "index" | "foreignKey" | "trigger"; + +const SCHEMA_DIFF_CHILD_ID_PREFIX: Record = { + column: "col", + index: "idx", + foreignKey: "fk", + trigger: "trg", +}; + +function diffChildObjectId(kind: SchemaDiffChildKind, tableName: string, childName: string, siblings: readonly { name: string }[], index: number): string { + const baseId = `${SCHEMA_DIFF_CHILD_ID_PREFIX[kind]}-${tableName}-${childName}`; + const duplicateCount = siblings.filter((sibling) => sibling.name === childName).length; + if (duplicateCount < 2) return baseId; + + const occurrence = siblings.slice(0, index).filter((sibling) => sibling.name === childName).length; + return `${baseId}-${occurrence}`; } -function indexObjectId(tableName: string, indexName: string): string { - return `idx-${tableName}-${indexName}`; -} - -function foreignKeyObjectId(tableName: string, foreignKeyName: string): string { - return `fk-${tableName}-${foreignKeyName}`; -} - -function triggerObjectId(tableName: string, triggerName: string): string { - return `trg-${tableName}-${triggerName}`; +function findDiffChildIndex(kind: SchemaDiffChildKind, tableName: string, siblings: readonly { name: string }[] | undefined, objectId: string): number { + return (siblings ?? []).findIndex((sibling, index) => diffChildObjectId(kind, tableName, sibling.name, siblings ?? [], index) === objectId); } function tableOptionObjectId(tableName: string): string { @@ -613,11 +619,15 @@ export function convertToSchemaDiffObjects(tableDiffs: TableDiff[], functionDiff const isRenamed = diff.type === "renamed"; const newName = isRenamed && renameCandidates ? (renameCandidates.find((rc) => rc.sourceName === diff.name)?.targetName ?? diff.name) : undefined; const tableId = tableObjectId(diff.name); + const columns = diff.columns ?? []; + const indexes = diff.indexes ?? []; + const foreignKeys = diff.foreignKeys ?? []; + const triggers = diff.triggers ?? []; const children: SchemaDiffObject[] = opType === "modify" ? [ - ...(diff.columns?.map((column) => ({ - id: columnObjectId(diff.name, column.name), + ...columns.map((column, index) => ({ + id: diffChildObjectId("column", diff.name, column.name, columns, index), operationType: getOperationType(column.type), objectKind: "column" as DiffObjectKind, name: column.name, @@ -627,9 +637,9 @@ export function convertToSchemaDiffObjects(tableDiffs: TableDiff[], functionDiff changes: column.changes, parentId: tableId, parentName: diff.name, - })) || []), - ...(diff.indexes?.map((index) => ({ - id: indexObjectId(diff.name, index.name), + })), + ...indexes.map((index, childIndex) => ({ + id: diffChildObjectId("index", diff.name, index.name, indexes, childIndex), // A modified index is one DROP + CREATE deploy unit. operationType: index.type === "modified" ? ("delete" as DiffOperationType) : getOperationType(index.type), objectKind: "index" as DiffObjectKind, @@ -640,9 +650,9 @@ export function convertToSchemaDiffObjects(tableDiffs: TableDiff[], functionDiff changes: index.changes, parentId: tableId, parentName: diff.name, - })) || []), - ...(diff.foreignKeys?.map((foreignKey) => ({ - id: foreignKeyObjectId(diff.name, foreignKey.name), + })), + ...foreignKeys.map((foreignKey, index) => ({ + id: diffChildObjectId("foreignKey", diff.name, foreignKey.name, foreignKeys, index), // Modified foreign keys are also dropped before they are recreated. operationType: foreignKey.type === "modified" ? ("delete" as DiffOperationType) : getOperationType(foreignKey.type), objectKind: "foreignKey" as DiffObjectKind, @@ -653,9 +663,9 @@ export function convertToSchemaDiffObjects(tableDiffs: TableDiff[], functionDiff changes: foreignKey.changes, parentId: tableId, parentName: diff.name, - })) || []), - ...(diff.triggers?.map((trigger) => ({ - id: triggerObjectId(diff.name, trigger.name), + })), + ...triggers.map((trigger, index) => ({ + id: diffChildObjectId("trigger", diff.name, trigger.name, triggers, index), operationType: getOperationType(trigger.type), objectKind: "trigger" as DiffObjectKind, name: trigger.name, @@ -665,7 +675,7 @@ export function convertToSchemaDiffObjects(tableDiffs: TableDiff[], functionDiff changes: trigger.changes, parentId: tableId, parentName: diff.name, - })) || []), + })), ...(diff.sourceTableComment !== diff.targetTableComment ? [ { @@ -805,10 +815,10 @@ export function selectSchemaDiffInput(result: SchemaDiffPreparation, objects: Sc return tableObject.selected ? [{ ...diff, syncSql: undefined }] : []; } - const columns = diff.columns?.filter((column) => selectedIds.has(columnObjectId(diff.name, column.name))) ?? []; - const indexes = diff.indexes?.filter((index) => selectedIds.has(indexObjectId(diff.name, index.name))) ?? []; - const foreignKeys = diff.foreignKeys?.filter((foreignKey) => selectedIds.has(foreignKeyObjectId(diff.name, foreignKey.name))) ?? []; - const triggers = diff.triggers?.filter((trigger) => selectedIds.has(triggerObjectId(diff.name, trigger.name))) ?? []; + const columns = diff.columns?.filter((column, index) => selectedIds.has(diffChildObjectId("column", diff.name, column.name, diff.columns ?? [], index))) ?? []; + const indexes = diff.indexes?.filter((index, childIndex) => selectedIds.has(diffChildObjectId("index", diff.name, index.name, diff.indexes ?? [], childIndex))) ?? []; + const foreignKeys = diff.foreignKeys?.filter((foreignKey, index) => selectedIds.has(diffChildObjectId("foreignKey", diff.name, foreignKey.name, diff.foreignKeys ?? [], index))) ?? []; + const triggers = diff.triggers?.filter((trigger, index) => selectedIds.has(diffChildObjectId("trigger", diff.name, trigger.name, diff.triggers ?? [], index))) ?? []; const includeTableOptions = selectedIds.has(tableOptionObjectId(diff.name)); if (columns.length === 0 && indexes.length === 0 && foreignKeys.length === 0 && triggers.length === 0 && !includeTableOptions) { @@ -1126,17 +1136,25 @@ export function setSchemaDiffObjectSelectedWithDependencies(objects: SchemaDiffO if (!tableObject || !tableDiff) return; setSchemaDiffObjectSelected(objects, id, value); - const child = (kind: DiffObjectKind, name: string) => tableObject.children?.find((candidate) => candidate.objectKind === kind && candidate.name === name); + const child = (kind: SchemaDiffChildKind, collection: readonly Child[] | undefined, index: number) => { + const candidate = collection?.[index]; + if (!candidate) return undefined; + + const childId = diffChildObjectId(kind, tableObject.name, candidate.name, collection, index); + return tableObject.children?.find((candidateObject) => candidateObject.id === childId); + }; const applyColumn = (name: string, value: boolean) => { - const columnDiff = tableDiff.columns?.find((column) => column.name === name); - const columnObject = child("column", name); + const columnIndex = tableDiff.columns?.findIndex((column) => column.name === name) ?? -1; + const columnDiff = columnIndex < 0 ? undefined : tableDiff.columns?.[columnIndex]; + const columnObject = columnIndex < 0 ? undefined : child("column", tableDiff.columns, columnIndex); if (!columnDiff || !columnObject) return; if (value && !["added", "renamed"].includes(columnDiff.type)) return; apply(columnObject.id, value); }; if (object.objectKind === "column") { - const columnDiff = tableDiff.columns?.find((column) => column.name === object.name); + const columnIndex = findDiffChildIndex("column", tableObject.name, tableDiff.columns, object.id); + const columnDiff = columnIndex < 0 ? undefined : tableDiff.columns?.[columnIndex]; if (!columnDiff) return; if (value && columnDiff.type === "added" && columnDiff.addPosition && typeof columnDiff.addPosition === "object") { @@ -1144,47 +1162,49 @@ export function setSchemaDiffObjectSelectedWithDependencies(objects: SchemaDiffO } if (value && columnDiff.type === "removed") { - for (const index of tableDiff.indexes ?? []) { - if (!["removed", "modified"].includes(index.type) || !index.target?.columns.includes(columnDiff.name)) continue; - const indexObject = child("index", index.name); + for (const [index, indexDiff] of (tableDiff.indexes ?? []).entries()) { + if (!["removed", "modified"].includes(indexDiff.type) || !indexDiff.target?.columns.includes(columnDiff.name)) continue; + const indexObject = child("index", tableDiff.indexes, index); if (indexObject) apply(indexObject.id, true); } - for (const foreignKey of tableDiff.foreignKeys ?? []) { + for (const [index, foreignKey] of (tableDiff.foreignKeys ?? []).entries()) { if (!["removed", "modified"].includes(foreignKey.type) || foreignKey.target?.column !== columnDiff.name) continue; - const foreignKeyObject = child("foreignKey", foreignKey.name); + const foreignKeyObject = child("foreignKey", tableDiff.foreignKeys, index); if (foreignKeyObject) apply(foreignKeyObject.id, true); } } if (!value && ["added", "renamed"].includes(columnDiff.type)) { - for (const dependentColumn of tableDiff.columns ?? []) { + for (const [index, dependentColumn] of (tableDiff.columns ?? []).entries()) { if (dependentColumn.type !== "added" || !dependentColumn.addPosition || typeof dependentColumn.addPosition !== "object" || dependentColumn.addPosition.after !== columnDiff.name) continue; - const dependentObject = child("column", dependentColumn.name); + const dependentObject = child("column", tableDiff.columns, index); if (dependentObject) apply(dependentObject.id, false); } - for (const index of tableDiff.indexes ?? []) { - const sourceColumns = [...(index.source?.columns ?? []), ...(index.source?.included_columns ?? [])]; - if (!["added", "modified"].includes(index.type) || !sourceColumns.includes(columnDiff.name)) continue; - const indexObject = child("index", index.name); + for (const [index, indexDiff] of (tableDiff.indexes ?? []).entries()) { + const sourceColumns = [...(indexDiff.source?.columns ?? []), ...(indexDiff.source?.included_columns ?? [])]; + if (!["added", "modified"].includes(indexDiff.type) || !sourceColumns.includes(columnDiff.name)) continue; + const indexObject = child("index", tableDiff.indexes, index); if (indexObject) apply(indexObject.id, false); } - for (const foreignKey of tableDiff.foreignKeys ?? []) { + for (const [index, foreignKey] of (tableDiff.foreignKeys ?? []).entries()) { if (!["added", "modified"].includes(foreignKey.type) || foreignKey.source?.column !== columnDiff.name) continue; - const foreignKeyObject = child("foreignKey", foreignKey.name); + const foreignKeyObject = child("foreignKey", tableDiff.foreignKeys, index); if (foreignKeyObject) apply(foreignKeyObject.id, false); } } } if (value && object.objectKind === "index") { - const index = tableDiff.indexes?.find((candidate) => candidate.name === object.name); + const indexIndex = findDiffChildIndex("index", tableObject.name, tableDiff.indexes, object.id); + const index = indexIndex < 0 ? undefined : tableDiff.indexes?.[indexIndex]; for (const columnName of [...(index?.source?.columns ?? []), ...(index?.source?.included_columns ?? [])]) { applyColumn(columnName, true); } } if (value && object.objectKind === "foreignKey") { - const foreignKey = tableDiff.foreignKeys?.find((candidate) => candidate.name === object.name); + const foreignKeyIndex = findDiffChildIndex("foreignKey", tableObject.name, tableDiff.foreignKeys, object.id); + const foreignKey = foreignKeyIndex < 0 ? undefined : tableDiff.foreignKeys?.[foreignKeyIndex]; if (foreignKey?.source?.column) applyColumn(foreignKey.source.column, true); } };