diff --git a/apps/desktop/src/lib/schema/schemaDiff.ts b/apps/desktop/src/lib/schema/schemaDiff.ts index 65ac105980..0cdcb5af06 100644 --- a/apps/desktop/src/lib/schema/schemaDiff.ts +++ b/apps/desktop/src/lib/schema/schemaDiff.ts @@ -1208,7 +1208,13 @@ export function groupDiffObjects(objects: SchemaDiffObject[]): OperationGroup[] }, }; - for (const obj of schemaDiffReviewObjects(objects)) { + // Classification is presence-based (DBeaver structure-compare semantics): an + // object that exists on both sides keeps operationType "modify" and carries + // its per-child operations in the drill-down, so only target-only objects + // ("delete") and source-only objects ("create") appear in those groups. A + // both-side table must never be re-bucketed as delete/create just because + // some of its columns or indexes differ. + for (const obj of objects) { groups[obj.operationType][obj.objectKind].push(obj); } @@ -1243,32 +1249,6 @@ export function groupDiffObjects(objects: SchemaDiffObject[]): OperationGroup[] }); } -function schemaDiffReviewObjects(objects: SchemaDiffObject[]): SchemaDiffObject[] { - return objects.flatMap((object) => { - const children = object.children?.filter((child) => child.operationType !== "none") ?? []; - if (object.operationType !== "modify" || children.length === 0) return [object]; - - const operationOrder: DiffOperationType[] = ["modify", "create", "delete"]; - return operationOrder.flatMap((operationType): SchemaDiffObject[] => { - const operationChildren = children.filter((child) => child.operationType === operationType); - if (operationChildren.length === 0) return []; - return [ - { - ...object, - id: `review-${operationType}-${object.id}`, - operationType, - selected: operationChildren.every((child) => child.selected), - sourceName: operationType === "delete" ? undefined : object.sourceName, - targetName: operationType === "create" ? undefined : object.targetName, - children: operationChildren, - parentId: object.id, - changes: operationChildren.flatMap((child) => child.changes ?? []), - }, - ]; - }); - }); -} - function getObjectTypeLabel(kind: DiffObjectKind): string { switch (kind) { case "table": diff --git a/packages/app-tests/schemaDiff.test.ts b/packages/app-tests/schemaDiff.test.ts index fba903cdb8..7350d7aedd 100644 --- a/packages/app-tests/schemaDiff.test.ts +++ b/packages/app-tests/schemaDiff.test.ts @@ -61,7 +61,7 @@ test("uses mysql target database as schema diff deploy qualifier", () => { assert.equal(schemaDiffDeployTargetSchema("sqlite", "main", ""), undefined); }); -test("shows a modified table with a removed index once in the delete group", () => { +test("keeps a modified table with a removed index out of the delete group", () => { const objects = convertToSchemaDiffObjects([ { type: "modified", @@ -74,19 +74,113 @@ test("shows a modified table with a removed index once in the delete group", () }, ]); - const deleteGroup = groupDiffObjects(objects).find((group) => group.operationType === "delete"); - const tables = deleteGroup?.typeGroups.find((group) => group.kind === "table")?.objects ?? []; + const groups = groupDiffObjects(objects); + const deleteGroup = groups.find((group) => group.operationType === "delete"); + const modifyGroup = groups.find((group) => group.operationType === "modify"); - assert.equal(tables.length, 1); - assert.equal(tables[0].name, "users"); - assert.equal(tables[0].sourceDdl, objects[0].sourceDdl); - assert.equal(tables[0].targetDdl, objects[0].targetDdl); - assert.equal(tables[0].children?.[0].id, "idx-users-idx_users_email"); - assert.equal(deleteGroup?.count, 1); + // The table exists on both sides, so it must only be classified as a + // structural modification — never as an object to delete on the target. + assert.deepEqual( + deleteGroup?.typeGroups.map((group) => group.kind), + [], + ); + assert.equal(deleteGroup?.count, 0); + assert.deepEqual( + modifyGroup?.typeGroups.map((group) => ({ kind: group.kind, names: group.objects.map((object) => object.name) })), + [{ kind: "table", names: ["users"] }], + ); + + const table = modifyGroup?.typeGroups[0]?.objects[0]; + assert.equal(table?.sourceDdl, objects[0].sourceDdl); + assert.equal(table?.targetDdl, objects[0].targetDdl); + // The destructive child stays visible under the table drill-down. + assert.equal(table?.children?.[0].id, "idx-users-idx_users_email"); + assert.equal(table?.children?.[0].operationType, "delete"); assert.equal(summarizeSchemaDiffOperations(objects).delete, 1); }); -test("keeps ordinary child changes under the modified table while surfacing delete risks", () => { +test("issue 7225: both-side table with DDL differences is never a delete candidate", () => { + const objects = convertToSchemaDiffObjects([ + { + type: "modified", + objectType: "table", + name: "shared_orders", + ddl: "CREATE TABLE `shared_orders` (`order_no` varchar(64) COMMENT 'order number', `remark` varchar(255));", + targetDdl: "CREATE TABLE `shared_orders` (`order_no` varchar(64) COMMENT 'order no', KEY `idx_amount` (`amount`));", + syncSql: "ALTER TABLE `shared_orders` MODIFY COLUMN `order_no` varchar(64) COMMENT 'order number', ADD COLUMN `remark` varchar(255); DROP INDEX `idx_amount` ON `shared_orders`;", + columns: [ + { type: "modified", name: "order_no", changes: ["comment: order no -> order number"] }, + { type: "added", name: "remark" }, + ], + indexes: [{ type: "removed", name: "idx_amount" }], + }, + ]); + + const groups = groupDiffObjects(objects); + + // Structural comparison happens under "modify"; the delete/create groups + // must stay reserved for presence differences. + for (const operationType of ["create", "delete"] as const) { + const group = groups.find((candidate) => candidate.operationType === operationType); + assert.deepEqual(group?.typeGroups, []); + assert.equal(group?.count, 0); + } + const modifyGroup = groups.find((group) => group.operationType === "modify"); + assert.deepEqual( + modifyGroup?.typeGroups.map((group) => group.objects.map((object) => object.name)), + [["shared_orders"]], + ); + + const table = modifyGroup?.typeGroups[0]?.objects[0]; + assert.equal(table?.id, "table-shared_orders"); + assert.equal(table?.sourceName, "shared_orders"); + assert.equal(table?.targetName, "shared_orders"); + assert.ok(table?.sourceDdl); + assert.ok(table?.targetDdl); + assert.deepEqual( + table?.children?.map((child) => `${child.objectKind}:${child.name}:${child.operationType}`), + ["column:order_no:modify", "column:remark:create", "index:idx_amount:delete"], + ); + assert.deepEqual(summarizeSchemaDiffOperations(objects), { create: 1, modify: 1, delete: 1, none: 0 }); +}); + +test("classifies identical, source-only and target-only tables by presence", () => { + const objects = convertToSchemaDiffObjects([ + { type: "none", objectType: "table", name: "identical" }, + { type: "added", objectType: "table", name: "source_only" }, + { type: "removed", objectType: "table", name: "target_only" }, + ]); + + const groups = groupDiffObjects(objects); + const namesFor = (operationType: string) => + groups + .find((group) => group.operationType === operationType) + ?.typeGroups.flatMap((typeGroup) => typeGroup.objects.map((object) => object.name)); + + assert.deepEqual(namesFor("modify"), []); + assert.deepEqual(namesFor("create"), ["source_only"]); + assert.deepEqual(namesFor("delete"), ["target_only"]); + assert.deepEqual(namesFor("none"), ["identical"]); +}); + +test("case-different table names stay presence-based on case-sensitive servers", () => { + const objects = convertToSchemaDiffObjects([ + { type: "added", objectType: "table", name: "Users" }, + { type: "removed", objectType: "table", name: "users" }, + ]); + + const groups = groupDiffObjects(objects); + const namesFor = (operationType: string) => + groups + .find((group) => group.operationType === operationType) + ?.typeGroups.flatMap((typeGroup) => typeGroup.objects.map((object) => object.name)); + + assert.deepEqual(namesFor("create"), ["Users"]); + assert.deepEqual(namesFor("delete"), ["users"]); + assert.deepEqual(namesFor("modify"), []); +}); + +test("keeps ordinary child changes under the modified table", () => { const objects = convertToSchemaDiffObjects([ { type: "modified", @@ -110,15 +204,15 @@ test("keeps ordinary child changes under the modified table while surfacing dele modifyGroup?.typeGroups.map((group) => ({ kind: group.kind, names: group.objects.map((object) => object.name) })), [{ kind: "table", names: ["users"] }], ); - assert.deepEqual( - createGroup?.typeGroups.map((group) => ({ kind: group.kind, names: group.objects.map((object) => object.name) })), - [{ kind: "table", names: ["users"] }], - ); - assert.deepEqual( - deleteGroup?.typeGroups.map((group) => ({ kind: group.kind, names: group.objects.map((object) => object.name) })), - [{ kind: "table", names: ["users"] }], - ); + // The table exists on both sides: create/delete groups must not list it. + assert.deepEqual(createGroup?.typeGroups, []); + assert.deepEqual(deleteGroup?.typeGroups, []); + const table = modifyGroup?.typeGroups[0]?.objects[0]; + assert.deepEqual( + table?.children?.map((child) => `${child.objectKind}:${child.name}:${child.operationType}`), + ["column:name:modify", "column:nickname:create", "index:idx_legacy:delete"], + ); assert.deepEqual(summarizeSchemaDiffOperations(objects), { create: 1, modify: 1, delete: 1, none: 0 }); }); @@ -168,11 +262,9 @@ test("counts each selectable destructive child under its table context", () => { ]); const deleteGroup = groupDiffObjects(objects).find((group) => group.operationType === "delete"); - assert.equal(deleteGroup?.count, 3); - assert.deepEqual( - deleteGroup?.typeGroups.map((group) => group.kind), - ["table"], - ); + // The destructive children stay under the modified table; the delete group + // only lists target-only objects. + assert.equal(deleteGroup?.count, 0); assert.deepEqual(summarizeSchemaDiffOperations(objects), { create: 0, modify: 0, delete: 3, none: 0 }); });