Skip to content

refactor(pg-delta): move execution phase onto BaseChange as change.phase #244

@avallete

Description

@avallete

Summary

Replace the standalone getExecutionPhase(change) helper in packages/pg-delta/src/core/sort/utils.ts with a phase getter on BaseChange. Rename the enum value "create_alter_object""forward". Keep behavior identical.

This concentrates "which execution pass does this change belong to?" on the change itself (high locality), removes a layering violation where generic sort code reaches into concrete AlterTableAlterColumn* classes via instanceof, and makes the classification total (exhaustive over ChangeOperation, no silent fallback).

Motivation

Today the same question — "is this a destructive/reverse-order change, or a forward one?" — is answered in at least four places with three subtly-different rules:

  1. packages/pg-delta/src/core/sort/utils.tsgetExecutionPhase(change) with a mix of operation / scope / drops / instanceof checks and a Safe default fallback at the bottom.
  2. packages/pg-delta/src/core/catalog.diff.ts:220-235 derives "is this a top-level object drop?" via operation === \"drop\" && scope === \"object\".
  3. packages/pg-delta/src/core/sort/logical-sort.ts and sort/custom-constraints.ts pattern-match on (operation, scope) pairs.
  4. packages/pg-delta/src/core/plan/create.ts:365 has hasRoutineChanges as yet another "predicate over a change" — left out of this issue, but flagged as the next candidate (see Out of scope).

The instanceof leaks are the worst smell: sort/utils.ts imports concrete AlterTableAlterColumn* classes from objects/table/changes/ to decide their phase. That's a generic sort module reaching across the codebase into a specific object family. With a getter on the change class, that knowledge colocates with the class itself.

Design (settled)

All load-bearing decisions have been made — no further grilling needed.

Decision Choice
Scope A (narrow) — phase only. operation/scope stay as-is; they're part of the user-facing DSL wire format (see integrations/supabase.ts, integrations/filter/dsl.ts, Plan.filter serialization).
Interface shape Getter on BaseChange with a default implementation + selective overrides. Not strictly abstract — ~150 concrete change classes would need identical 3-line getters otherwise.
Enum rename Phase = \"drop\" | \"forward\" (was \"drop\" | \"create_alter_object\").
Totality Total — default implementation is exhaustive over ChangeOperation via a TypeScript never-check. No Safe default fallback.

The change

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

Add Phase type and default getter:

```typescript
export type Phase = "drop" | "forward";

export abstract class BaseChange {
// … existing abstract operation / objectType / scope …

get phase(): Phase {
switch (this.operation) {
case "drop":
return "drop";
case "create":
return "forward";
case "alter": {
// ALTER goes to drop phase only if it drops a real object, not metadata.
// Metadata drops (acl:, defacl:, comment:, …) don't participate in
// dependency ordering because they don't remove database objects.
const dropsRealObject = this.drops.some((id) => !isMetadataStableId(id));
return dropsRealObject ? "drop" : "forward";
}
default: {
const _: never = this.operation;
return _;
}
}
}
}
```

isMetadataStableId currently lives in packages/pg-delta/src/core/sort/utils.ts. Move it alongside Phase — either inside base.change.ts or in a new sibling core/stable-id.ts (implementer's call; logical-sort.ts also imports it for a different purpose). Do not have base.change.ts import from sort/ — that's backwards layering.

2. Per-class overrides

The default rule handles everything except three AlterTableAlterColumn* classes whose phase is data-dependent. In packages/pg-delta/src/core/objects/table/changes/table.alter.ts:

```typescript
// AlterTableAlterColumnDropDefault
get phase(): Phase {
return "drop";
}

// AlterTableAlterColumnDropIdentity
get phase(): Phase {
return "drop";
}

// AlterTableAlterColumnType
get phase(): Phase {
return this.column.is_custom_type ? "forward" : "drop";
}
```

Verify before writing these overrides: if any of the three classes already includes a non-metadata stableId in its drops getter (e.g. a sequence or user-defined type stableId), the default rule catches it and the override is redundant. Check each class's drops getter; only add overrides that are actually load-bearing. Dead overrides are a smell.

3. Deletions

  • packages/pg-delta/src/core/sort/utils.ts: delete getExecutionPhase and the Phase type. Delete the AlterTableAlterColumn* imports (the layering violation). If isMetadataStableId moves out, this file empties entirely — delete it and update sort/logical-sort.ts's import.
  • packages/pg-delta/src/core/sort/sort-changes.ts: replace getExecutionPhase(changeItem)changeItem.phase.
  • packages/pg-delta/src/core/sort/logical-sort.ts: replace getExecutionPhase(change)change.phase.
  • Rename \"create_alter_object\"\"forward\" in every Record<Phase, …> literal, every type annotation, every test. ~5-10 call sites. grep -rn create_alter_object packages/pg-delta/src packages/pg-delta/tests to find them.

4. catalog.diff.ts:220-235 — leave alone (with a comment)

The operation === \"drop\" && scope === \"object\" predicate there is not asking "what phase?" — it's asking "is this a top-level object drop (as opposed to a column/constraint drop inside an ALTER)?". An AlterTableDropColumn has phase === \"drop\" too, but the site doesn't want it. Different predicate. Add a one-line comment explaining the distinction; do not replace with phase === \"drop\".

Tests

Refactor preserving behavior — per AGENTS.md Test-Driven Fixes section ("Refactors that claim to preserve behavior: if there is doubt, pin the current behavior with a passing test first, then refactor"):

  1. New packages/pg-delta/src/core/objects/base.change.test.ts — table-driven tests for get phase(). Cover each (operation, scope) combination, plus ALTER with metadata-only drops, ALTER with non-metadata drops. Exhaustiveness is enforced by the never-check at compile time; no runtime test needed for that.
  2. Three focused override tests in packages/pg-delta/src/core/objects/table/changes/table.alter.test.ts — one for AlterTableAlterColumnDropDefault, one for DropIdentity, two for AlterTableAlterColumnType (is_custom_type: true → forward, false → drop).
  3. Existing sort tests must pass unchanged. packages/pg-delta/src/core/sort/sort-changes.test.ts (if present), logical-sort.test.ts, topological-sort.test.ts — this is the behavior-preservation signal.
  4. No new integration tests required. The refactor is pure rewiring of an internal predicate. An integration regression would show up in existing roundtrip coverage (sequence-owned-default tests exercise AlterTableAlterColumnDropDefault → drop-phase; view recreation exercises the default rule).

Changeset

This is a refactor with no user-visible behavior change, so per AGENTS.md ("add a changeset for any fix, feat, or user-facing change") a changeset is not strictly required. Maintainer may still choose to add a patch bump to keep the release notes rich.

CONTEXT.md

This is the first documented architecture term in the repo (no CONTEXT.md exists today). As part of this PR, create CONTEXT.md at the repo root with four terms:

```markdown

Context

Shared vocabulary used across `pg-delta` and `pg-topo`.

pg-delta

catalog — a resolved snapshot of one database's schema: all objects, plus
the `pg_depend` rows that link them. Produced by `extractCatalog(pool)` or
deserialized from a snapshot.

change — one unit of schema mutation (e.g. `CreateTable`, `AlterViewSetOptions`).
Every change has `operation`, `objectType`, `scope`, `phase`, and `serialize()`.

phase — which execution pass a change runs in: `"drop"` (reverse
dependency order, against the main catalog) or `"forward"` (forward
dependency order, against the branch catalog). Determined by the change
itself via `change.phase`.

plan — the artifact `createPlan` produces: the ordered list of SQL
statements, plus risk classification and source/target fingerprints.
```

Kept deliberately minimal — future candidates (object-kind adapter, integration preset, etc.) will grow it.

Out of scope

  • hasRoutineChanges in plan/create.ts. Different axis (SQL prologue, not execution phase). Fold into the same module in a follow-up once this lands. Candidate B of the original design discussion.
  • isMetadataStableId prefix scheme. Stringly-typed classification that also smells, but it's about stableId modeling, not change classification. Separate opportunity.
  • Per-object diff consolidation (the much bigger refactor — candidate fix: test diffs between postgres version #1 of the original discussion). Not blocked by this; can proceed independently.

Acceptance criteria

  • change.phase returns the same value as getExecutionPhase(change) did, for every change type currently exercised by the test suite.
  • getExecutionPhase and the free-standing Phase export from sort/utils.ts are deleted.
  • sort/utils.ts no longer imports from objects/table/changes/.
  • The enum value is \"drop\" \| \"forward\" everywhere; no \"create_alter_object\" remains.
  • base.change.test.ts exercises the default getter with an exhaustive (operation, scope) matrix.
  • Per-class override tests exist for each override actually added (dead overrides removed).
  • catalog.diff.ts:220-235 is unchanged except for an added comment clarifying why it can't use change.phase.
  • CONTEXT.md exists at repo root with the four terms above.
  • bun run test:pg-delta passes locally.
  • bun run format-and-lint --write --unsafe && bun run check-types && bun run knip --fix clean.

Notes for the implementer

  • Branch naming: refactor/pg-delta-change-phase or similar.
  • PR title: refactor(pg-delta): move execution phase onto BaseChange (Conventional Commits, required by lint-pull-request workflow).
  • PR body: link back to this issue with Closes #<this issue number>.
  • Two-commit shape welcome: refactor(pg-delta): introduce change.phase getter + docs: add CONTEXT.md — but a single squashed commit is fine too.

Metadata

Metadata

Labels

supabase/pg-toolbelt🛠️ ChoreInternal maintenance, refactoring, tooling, dependencies, or performance work

Type

No type
No fields configured for issues without a type.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions