fix(schema-diff): keep both-side tables out of delete group

Closes #7225
2026-10-19 16:16:45 +00:00 · 2026-08-26 20:52:38 +08:00
parent 10df1b07d1
commit 8382d85e19
2 changed files with 122 additions and 50 deletions
@@ -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":
@@ -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 });
 });