fix(pg-delta): order view drops before column type rewrites

.changeset/cool-windows-worry.md

@@ -0,0 +1,5 @@
+---
+"@supabase/pg-delta": patch
+---
+
+Order dependent view drops before column type rewrites, and preserve view or materialized-view metadata, including ACL adjustments, when those dependents are dropped and recreated during replacement.

packages/pg-delta/CLAUDE.md

@@ -92,8 +92,9 @@ Keep cycle handling split by the scope of information it needs:
 - **Unbreakable graph cycles belong in sort-phase change injection**. If the emitted statements are valid but topological sorting discovers a hard dependency cycle that cannot be solved by weak-edge filtering, implement the narrow pattern in `src/core/sort/cycle-breakers.ts` (`tryBreakCycleByChangeInjection`). Existing examples: injecting explicit FK constraint drops for dropped-table FK cycles and rebuilding `AlterTableDropColumn` for publication-column cycles on surviving tables.
 - **`expandReplaceDependencies()` only computes replacement closure**. It may report metadata such as which tables were promoted to replacement pairs, but it should not own unrelated cycle-pruning policy.
 - **`src/core/sort/dependency-filter.ts` is a narrow last resort**. Use it only for safe edge filtering where the emitted statements are already valid and only the graph edge is artificial. Do not extend sort-phase filtering to paper over plans that would still fail at apply time.
+- **In-place mutations that invalidate dependents declare `invalidates`, not a graph hack**. When a change keeps an object's identity but rewrites it so dependents bound to the old definition must be dropped before it and rebuilt after (the canonical case is `AlterTableAlterColumnType`, whose `ALTER COLUMN ... TYPE` forces a PostgreSQL table rewrite), override the `invalidates` getter on the change (sibling to `creates`/`drops`/`requires` in `base.change.ts`) to return the affected stable id. `buildGraphData` folds `invalidates` into the drop-phase producer set exactly like `drops`, so the existing `pg_depend` edges order each dependent's teardown ahead of the mutation. This is ordering-only: `invalidates` does not feed `Change.drops`, so phase assignment (`getExecutionPhase`), filtering, fingerprints, and serialization are unchanged, and recreation order needs no help because the create phase always runs after the entire drop phase. Prefer this over adding a change-type `instanceof` to the otherwise generic `graph-builder.ts`.
 
-Rule of thumb: if the fix changes a valid final `Change[]` before graph construction, it is post-diff; if it reacts to a concrete unbreakable dependency cycle and needs to inject or rebuild changes, it belongs in the sort-phase cycle breakers; if it needs only one object's semantics, it belongs in that object's `diff*`; if it only removes a graph edge without changing emitted SQL, it belongs in the sort filter.
+Rule of thumb: if the fix changes a valid final `Change[]` before graph construction, it is post-diff; if it reacts to a concrete unbreakable dependency cycle and needs to inject or rebuild changes, it belongs in the sort-phase cycle breakers; if it needs only one object's semantics, it belongs in that object's `diff*`; if it only removes a graph edge without changing emitted SQL, it belongs in the sort filter; if a change mutates an object in place such that its dependents must be torn down first, it declares `invalidates`.
 
 ## Key Concepts
 

packages/pg-delta/src/core/catalog.diff.test.ts

@@ -0,0 +1,173 @@
+import { describe, expect, test } from "bun:test";
+import { diffCatalogs } from "./catalog.diff.ts";
+import { Catalog, createEmptyCatalog } from "./catalog.model.ts";
+import { Role, type RoleProps } from "./objects/role/role.model.ts";
+import {
+  GrantViewPrivileges,
+  RevokeViewPrivileges,
+} from "./objects/view/changes/view.privilege.ts";
+import { View, type ViewProps } from "./objects/view/view.model.ts";
+
+const idColumn: ViewProps["columns"][number] = {
+  name: "id",
+  position: 1,
+  data_type: "integer",
+  data_type_str: "integer",
+  is_custom_type: false,
+  custom_type_type: null,
+  custom_type_category: null,
+  custom_type_schema: null,
+  custom_type_name: null,
+  not_null: false,
+  is_identity: false,
+  is_identity_always: false,
+  is_generated: false,
+  collation: null,
+  default: null,
+  comment: null,
+};
+
+const nameColumn: ViewProps["columns"][number] = {
+  name: "name",
+  position: 2,
+  data_type: "text",
+  data_type_str: "text",
+  is_custom_type: false,
+  custom_type_type: null,
+  custom_type_category: null,
+  custom_type_schema: null,
+  custom_type_name: null,
+  not_null: false,
+  is_identity: false,
+  is_identity_always: false,
+  is_generated: false,
+  collation: null,
+  default: null,
+  comment: null,
+};
+
+const baseView: ViewProps = {
+  schema: "public",
+  name: "replaced_view",
+  definition: "SELECT id FROM source_table",
+  row_security: false,
+  force_row_security: false,
+  has_indexes: false,
+  has_rules: false,
+  has_triggers: false,
+  has_subclasses: false,
+  is_populated: true,
+  replica_identity: "d",
+  is_partition: false,
+  options: null,
+  partition_bound: null,
+  owner: "postgres",
+  comment: null,
+  columns: [idColumn],
+  privileges: [],
+};
+
+const makeView = (override: Partial<ViewProps> = {}) =>
+  new View({
+    ...baseView,
+    ...override,
+    columns: override.columns ?? [...baseView.columns],
+    privileges: override.privileges ?? [...baseView.privileges],
+  });
+
+const makeRole = (name: string, override: Partial<RoleProps> = {}) =>
+  new Role({
+    name,
+    is_superuser: false,
+    can_inherit: true,
+    can_create_roles: false,
+    can_create_databases: false,
+    can_login: true,
+    can_replicate: false,
+    connection_limit: null,
+    can_bypass_rls: false,
+    config: null,
+    comment: null,
+    members: [],
+    default_privileges: [],
+    security_labels: [],
+    ...override,
+  });
+
+describe("catalog.diff", () => {
+  test("keeps replacement-created view grants through dropped-target privilege filtering", async () => {
+    const baseline = await createEmptyCatalog(170000, "postgres");
+    const mainView = makeView();
+    const branchView = makeView({
+      definition: "SELECT id, name FROM source_table",
+      columns: [...mainView.columns, nameColumn],
+      privileges: [
+        { grantee: "view_reader", privilege: "SELECT", grantable: false },
+      ],
+    });
+
+    const main = new Catalog({
+      // oxlint-disable-next-line typescript/no-misused-spread
+      ...baseline,
+      views: { [mainView.stableId]: mainView },
+    });
+    const branch = new Catalog({
+      // oxlint-disable-next-line typescript/no-misused-spread
+      ...baseline,
+      views: { [branchView.stableId]: branchView },
+    });
+
+    const changes = diffCatalogs(main, branch);
+
+    expect(
+      changes.some((change) => change instanceof GrantViewPrivileges),
+    ).toBe(true);
+  });
+
+  test("keeps replacement-created view revokes through dropped-target privilege filtering", async () => {
+    const baseline = await createEmptyCatalog(170000, "postgres");
+    const mainView = makeView();
+    const branchView = makeView({
+      definition: "SELECT id, name FROM source_table",
+      columns: [...mainView.columns, nameColumn],
+    });
+    const postgres = makeRole("postgres", {
+      default_privileges: [
+        {
+          in_schema: "public",
+          objtype: "r",
+          grantee: "view_reader",
+          privileges: [{ privilege: "SELECT", grantable: false }],
+          is_implicit: false,
+        },
+      ],
+    });
+    const viewReader = makeRole("view_reader");
+
+    const roles = {
+      [postgres.stableId]: postgres,
+      [viewReader.stableId]: viewReader,
+    };
+    const main = new Catalog({
+      // oxlint-disable-next-line typescript/no-misused-spread
+      ...baseline,
+      roles,
+      views: { [mainView.stableId]: mainView },
+    });
+    const branch = new Catalog({
+      // oxlint-disable-next-line typescript/no-misused-spread
+      ...baseline,
+      roles,
+      views: { [branchView.stableId]: branchView },
+    });
+
+    // The recreated view inherits SELECT from default privileges, but the
+    // branch model wants no explicit reader ACL. The replacement filter must
+    // keep the generated REVOKE even though the old view stable id is dropped.
+    const changes = diffCatalogs(main, branch);
+
+    expect(
+      changes.some((change) => change instanceof RevokeViewPrivileges),
+    ).toBe(true);
+  });
+});

packages/pg-delta/src/core/catalog.diff.ts

@@ -217,19 +217,39 @@ export function diffCatalogs(
     ...diffForeignTables(diffContext, main.foreignTables, branch.foreignTables),
   );
 
-  // Filter privilege REVOKEs for objects that are being dropped
-  // Avoid emitting redundant REVOKE statements for targets that will no longer exist.
+  // Filter privilege changes for objects that are only being dropped.
+  // Avoid emitting redundant ACL statements for targets that will no longer exist.
   const droppedObjectStableIds = new Set<string>();
+  const createdStableIds = new Set<string>();
   for (const change of changes) {
     if (change.operation === "drop" && change.scope === "object") {
       for (const dep of change.requires) {
         droppedObjectStableIds.add(dep);
       }
     }
+    if (change.operation === "create" && change.scope === "object") {
+      for (const dep of change.creates) {
+        createdStableIds.add(dep);
+      }
+    }
   }
+  // A pure DROP does not need ACL cleanup: the target object is going away.
+  // A replacement is different: it has both DROP and CREATE for the same stable
+  // id, and its privilege ALTERs describe the ACL state of the newly created
+  // object. Keep all of them, including REVOKE/REVOKE GRANT OPTION generated to
+  // subtract privileges inherited from ALTER DEFAULT PRIVILEGES at create time.
+  const replacementStableIds = new Set(
+    [...droppedObjectStableIds].filter((id) => createdStableIds.has(id)),
+  );
   let filteredChanges = changes.filter((change) => {
     if (change.operation === "alter" && change.scope === "privilege") {
-      return !droppedObjectStableIds.has(getPrivilegeTargetStableId(change));
+      const targetStableId = getPrivilegeTargetStableId(change);
+      // Checking only privilege creates would keep replacement GRANTs but drop
+      // replacement REVOKEs, so preserve by replacement target stable id instead.
+      if (replacementStableIds.has(targetStableId)) {
+        return true;
+      }
+      return !droppedObjectStableIds.has(targetStableId);
     }
     return true;
   });
@@ -238,6 +258,7 @@ export function diffCatalogs(
     changes: filteredChanges,
     mainCatalog: main,
     branchCatalog: branch,
+    diffContext,
   });
   filteredChanges = normalizePostDiffChanges({
     changes: expandedDependencies.changes,

packages/pg-delta/src/core/expand-replace-dependencies.ts

@@ -1,11 +1,13 @@
 import type { Catalog } from "./catalog.model.ts";
 import type { Change } from "./change.types.ts";
+import type { ObjectDiffContext } from "./objects/diff-context.ts";
 import { CreateDomain } from "./objects/domain/changes/domain.create.ts";
 import { DropDomain } from "./objects/domain/changes/domain.drop.ts";
 import { CreateIndex } from "./objects/index/changes/index.create.ts";
 import { DropIndex } from "./objects/index/changes/index.drop.ts";
 import { CreateMaterializedView } from "./objects/materialized-view/changes/materialized-view.create.ts";
 import { DropMaterializedView } from "./objects/materialized-view/changes/materialized-view.drop.ts";
+import { buildCreateMaterializedViewChanges } from "./objects/materialized-view/materialized-view.diff.ts";
 import { CreateProcedure } from "./objects/procedure/changes/procedure.create.ts";
 import { DropProcedure } from "./objects/procedure/changes/procedure.drop.ts";
 import { CreateCommentOnRlsPolicy } from "./objects/rls-policy/changes/rls-policy.comment.ts";
@@ -24,6 +26,7 @@ import { DropRange } from "./objects/type/range/changes/range.drop.ts";
 import { stableId } from "./objects/utils.ts";
 import { CreateView } from "./objects/view/changes/view.create.ts";
 import { DropView } from "./objects/view/changes/view.drop.ts";
+import { buildCreateViewChanges } from "./objects/view/view.diff.ts";
 
 type ResolvedObject =
   | {
@@ -95,10 +98,15 @@ export function expandReplaceDependencies({
   changes,
   mainCatalog,
   branchCatalog,
+  diffContext,
 }: {
   changes: Change[];
   mainCatalog: Catalog;
   branchCatalog: Catalog;
+  diffContext?: Pick<
+    ObjectDiffContext,
+    "version" | "currentUser" | "defaultPrivilegeState"
+  >;
 }): ExpandReplaceDependenciesResult {
   const createdIds = new Set<string>();
   const droppedIds = new Set<string>();
@@ -244,6 +252,7 @@ export function expandReplaceDependencies({
       const replacementChanges = buildReplaceChanges(resolved, {
         addDrop,
         addCreate,
+        diffContext,
       });
       if (!replacementChanges) continue;
 
@@ -444,9 +453,16 @@ function resolveObjectForStableId(
 
 function buildReplaceChanges(
   resolved: ResolvedObject,
-  options: { addDrop: boolean; addCreate: boolean },
+  options: {
+    addDrop: boolean;
+    addCreate: boolean;
+    diffContext?: Pick<
+      ObjectDiffContext,
+      "version" | "currentUser" | "defaultPrivilegeState"
+    >;
+  },
 ): Change[] | null {
-  const { addDrop, addCreate } = options;
+  const { addDrop, addCreate, diffContext } = options;
 
   if (!addDrop && !addCreate) return null;
 
@@ -485,15 +501,23 @@ function buildReplaceChanges(
     case "view":
       return [
         ...(addDrop ? [new DropView({ view: resolved.main })] : []),
-        ...(addCreate ? [new CreateView({ view: resolved.branch })] : []),
+        ...(addCreate
+          ? buildCreateViewReplacementChanges(resolved.branch, diffContext)
+          : []),
       ];
     case "materialized_view":
       return [
         ...(addDrop
           ? [new DropMaterializedView({ materializedView: resolved.main })]
           : []),
         ...(addCreate
-          ? [new CreateMaterializedView({ materializedView: resolved.branch })]
+          ? diffContext
+            ? buildCreateMaterializedViewChanges(diffContext, resolved.branch)
+            : [
+                new CreateMaterializedView({
+                  materializedView: resolved.branch,
+                }),
+              ]
           : []),
       ];
     case "index":
@@ -573,3 +597,20 @@ function buildReplaceChanges(
       return null;
   }
 }
+
+function buildCreateViewReplacementChanges(
+  view: Catalog["views"][string],
+  diffContext:
+    | Pick<
+        ObjectDiffContext,
+        "version" | "currentUser" | "defaultPrivilegeState"
+      >
+    | undefined,
+): Change[] {
+  // Dependency-closure replacements synthesize a create without going through
+  // `diffViews`, so replay the same owner/comment/security-label/ACL metadata
+  // that a normal non-alterable view replacement would emit.
+  return diffContext
+    ? buildCreateViewChanges(diffContext, view)
+    : [new CreateView({ view })];
+}

packages/pg-delta/src/core/objects/base.change.ts

@@ -51,6 +51,21 @@ export abstract class BaseChange {
     return [];
   }
 
+  /**
+   * Stable identifiers this change invalidates in place.
+   *
+   * Unlike `drops`, the object keeps its identity. This is an ordering-only
+   * signal for mutations that rewrite an existing object in a way that requires
+   * dependents bound to the old definition to be dropped before the mutation
+   * and rebuilt afterward.
+   *
+   * Defaults to an empty array. Override in subclasses that invalidate
+   * dependents without dropping the object.
+   */
+  get invalidates(): string[] {
+    return [];
+  }
+
   /**
    * Stable identifiers this change requires to exist beforehand.
    *

packages/pg-delta/src/core/objects/materialized-view/materialized-view.diff.test.ts

@@ -106,17 +106,18 @@ describe.concurrent("materialized-view.diff", () => {
     expect(changes[0]).toBeInstanceOf(AlterMaterializedViewChangeOwner);
   });
 
-  test("drop + create on non-alterable change", () => {
+  test("drop + create with metadata on non-alterable change", () => {
     const main = new MaterializedView(base);
     const branch = new MaterializedView({ ...base, definition: "select 2" });
     const changes = diffMaterializedViews(
       testContext,
       { [main.stableId]: main },
       { [branch.stableId]: branch },
     );
-    expect(changes).toHaveLength(2);
+    expect(changes).toHaveLength(3);
     expect(changes[0]).toBeInstanceOf(DropMaterializedView);
     expect(changes[1]).toBeInstanceOf(CreateMaterializedView);
+    expect(changes[2]).toBeInstanceOf(AlterMaterializedViewChangeOwner);
   });
 
   test("alter storage parameters: set and reset", () => {