From b1776c55b154737c595c8e47a65dcc3a4bae96fe Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Wed, 5 Aug 2026 20:04:24 +1000 Subject: [PATCH 1/2] Add pkg/plan: one versioned dry-run report for both front doors migrate --dry-run --json and diff --json previously emitted two ad-hoc JSON shapes (router.Plan and a private diffReport). Consolidate them into a single versioned plan.Report (format_version 1) so an orchestrator adapter parses one contract regardless of how the plan was derived. Groundwork for PLAT-38440 (dry-run plan + suggest surface). --- SAFETY.md | 2 +- docs/architecture.md | 1 + docs/low-level-design.md | 1 + docs/testing.md | 1 + internal/cli/diff.go | 112 +++++++------------- internal/cli/diff_integration_test.go | 50 +++++---- internal/cli/dryrun.go | 40 +++---- internal/cli/dryrun_integration_test.go | 45 ++++---- pkg/plan/plan.go | 100 ++++++++++++++++++ pkg/plan/plan_test.go | 133 ++++++++++++++++++++++++ 10 files changed, 342 insertions(+), 143 deletions(-) create mode 100644 pkg/plan/plan.go create mode 100644 pkg/plan/plan_test.go diff --git a/SAFETY.md b/SAFETY.md index dfddac3..c1b5c08 100644 --- a/SAFETY.md +++ b/SAFETY.md @@ -27,7 +27,7 @@ The invariant registry (invariant IDs referenced below) lives in | `pkg/checkpoint` — durable resume state | ✅ core | planned (Phase 8) | ST-1, ST-2 | | slot lifecycle (in `pkg/decode`) — create, reap, lag ceiling | ✅ core | planned (Phase 8) | ST-3 | | `pkg/migration` — orchestrator, **cutover swap + fidelity gate** | ✅ core | planned (Phase 7) | LK-2, LK-4, ST-5 | -| `pkg/statement`, `pkg/planner`, `pkg/schemadiff`, `pkg/router`, `pkg/lint` — classify/diff/route | ❌ periphery¹ | `pkg/statement` (parse boundary), `pkg/schemadiff` (introspect/diff via scratch execute-and-introspect), `pkg/planner` (classifier), and `pkg/router` (backend assignment + availability policy) exist (Phases 2.1–2.4); `pkg/lint` planned | (CO-7 holds at the parse boundary) | +| `pkg/statement`, `pkg/planner`, `pkg/schemadiff`, `pkg/router`, `pkg/plan`, `pkg/lint` — classify/diff/route/report | ❌ periphery¹ | `pkg/statement` (parse boundary), `pkg/schemadiff` (introspect/diff via scratch execute-and-introspect), `pkg/planner` (classifier), `pkg/router` (backend assignment + availability policy), and `pkg/plan` (versioned dry-run plan report) exist (Phases 2.1–2.5); `pkg/lint` planned | (CO-7 holds at the parse boundary) | | `pkg/verdict` — structured outcome contract, rendering, exit codes | ❌ periphery | exists (Phase 1) | — | | `internal/cli` — CLI, flags, help, prompts | ❌ periphery | `migrate`, `status`, `diff`, and `fmt` exist; `lint` is a stub | — | | status / progress / advisory rendering, metrics | ❌ periphery | planned | — | diff --git a/docs/architecture.md b/docs/architecture.md index 39ff6e2..b9f2de7 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -113,6 +113,7 @@ boundary) is defined in [../SAFETY.md](../SAFETY.md). | `pkg/schemadiff` | Execute-and-introspect desired state, introspect the live catalog, and produce an ordered declarative diff | exists | | `pkg/planner` | Classify typed operations and emit safer native SQL | exists | | `pkg/lint` | Policy-level rejection of unsafe or unsupported operations | planned | +| `pkg/plan` | Versioned machine-readable dry-run plan report — the one JSON contract both front doors emit and an orchestrator consumes | exists (Phase 2.5) | | `pkg/router` | Route classified statements to native / copy-and-swap / refuse dispositions; copy-and-swap reports unavailable until that backend lands | exists (Phase 2.4) | | `pkg/executor` | Bounded optimistic native attempt; the `Executor` contract (`Plan`/`Execute`/`Status`/`Abort`) lands in Phase 3 | bounded optimistic attempt exists | | `pkg/table` | PK-range chunkers (single-column fast path, composite), dynamic time-based sizing | Phase 4 | diff --git a/docs/low-level-design.md b/docs/low-level-design.md index 5fbbc37..de6a6d6 100644 --- a/docs/low-level-design.md +++ b/docs/low-level-design.md @@ -579,6 +579,7 @@ pkg/statement/ -> Wasm go-pgquery boundary + typed operation descriptors pkg/schemadiff/ -> execute-and-introspect desired state + live introspection + ordered diff pkg/planner/ -> classify each operation and construct safer native SQL pkg/router/ -> assign classified statements to available backends +pkg/plan/ -> versioned machine-readable dry-run plan report (both front doors) pkg/executor/ -> bounded optimistic native attempt only pkg/dbconn/ -> bounded database connections pkg/preflight/ -> migration preflight checks diff --git a/docs/testing.md b/docs/testing.md index d55e4e7..3de8294 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -159,6 +159,7 @@ this repository's CI. | Parse boundary, typed operations, and advisory rewrites | [pkg/statement](../pkg/statement/statement_test.go), [operation tests](../pkg/statement/ops_test.go) | | Native / copy-and-swap / refuse classification and safer SQL | [pkg/planner](../pkg/planner/planner_test.go) | | Backend routing and copy-and-swap unavailable disposition | [pkg/router](../pkg/router/router_test.go) | +| Plan report contract: exact JSON shape, versioning, field omissions | [pkg/plan](../pkg/plan/plan_test.go) | | Scratch execute-and-introspect, ordered diff, and convergence (`TestDiffConverges`) | [pkg/schemadiff](../pkg/schemadiff/schemadiff_integration_test.go), [diff tests](../pkg/schemadiff/diff_test.go) | | CLI `diff`, `fmt`, and classified `migrate --dry-run`, including applying text output and re-diffing to empty (`TestDiffTextPlanIsExecutableSQL`) | [diff integration](../internal/cli/diff_integration_test.go), [fmt](../internal/cli/diff_test.go), [dry-run integration](../internal/cli/dryrun_integration_test.go) | | Bounded optimistic native attempt and table preflight | [pkg/executor](../pkg/executor/optimistic_integration_test.go), [pkg/preflight](../pkg/preflight/preflight_integration_test.go) | diff --git a/internal/cli/diff.go b/internal/cli/diff.go index e5af19a..6c445f1 100644 --- a/internal/cli/diff.go +++ b/internal/cli/diff.go @@ -10,71 +10,31 @@ import ( "strings" "github.com/block/pg-sprite/pkg/dbconn" + "github.com/block/pg-sprite/pkg/plan" "github.com/block/pg-sprite/pkg/planner" "github.com/block/pg-sprite/pkg/router" "github.com/block/pg-sprite/pkg/schemadiff" "github.com/block/pg-sprite/pkg/statement" ) -// diffReport is the diff command's JSON output contract. -type diffReport struct { - // Schema is the live schema the diff targeted. - Schema string `json:"schema"` - // Table is the desired (and live) table name. - Table string `json:"table"` - // TableExists reports whether the live table was found; when false the - // changes are the full desired schema. - TableExists bool `json:"table_exists"` - // Disposition is the routed plan's aggregate disposition: what would - // happen if the engine executed this plan now. - Disposition router.Disposition `json:"disposition"` - // Changes is the ordered statement plan; empty means the live table - // already matches the desired state. - Changes []plannedChange `json:"changes"` -} - -// plannedChange is one diff statement with its classification and routing: -// the derived SQL plus where the engine would send it and what would run. -type plannedChange struct { - schemadiff.Change - // Route is the planner's aggregate route for the statement. - Route planner.Route `json:"route"` - // Backend is the assigned execution strategy; empty for refusals. - Backend router.Backend `json:"backend,omitempty"` - // Disposition is what execution would do with the statement now. - Disposition router.Disposition `json:"disposition"` - // Decisions are the planner's per-operation classifications. - Decisions []planner.Decision `json:"decisions"` - // ExecSQL is the ordered SQL the native backend would run — the safer - // sequence when the planner constructed one. Empty for non-native - // routes. - ExecSQL []string `json:"exec_sql,omitempty"` -} - // classifyChanges routes every derived change through the shared // classify-and-route pipeline. facts sharpen type-change classification; // the zero value is valid and strictly more conservative. -func classifyChanges(changes []schemadiff.Change, facts planner.Facts) ([]plannedChange, router.Disposition, error) { +func classifyChanges(changes []schemadiff.Change, facts planner.Facts) ([]plan.Statement, router.Disposition, error) { plans := make([]planner.Plan, 0, len(changes)) for _, ch := range changes { - plan, err := planner.Classify(ch.SQL, facts) + classified, err := planner.Classify(ch.SQL, facts) if err != nil { return nil, "", fmt.Errorf("classify derived statement %q: %w", ch.SQL, err) } - plans = append(plans, plan) + plans = append(plans, classified) } routed := router.Route(plans) - planned := make([]plannedChange, 0, len(changes)) + planned := make([]plan.Statement, 0, len(changes)) for i, ch := range changes { - st := routed.Statements[i] - planned = append(planned, plannedChange{ - Change: ch, - Route: st.Route, - Backend: st.Backend, - Disposition: st.Disposition, - Decisions: st.Decisions, - ExecSQL: st.ExecSQL, - }) + ps := plan.FromRouted(routed.Statements[i]) + ps.Destructive = ch.Destructive + planned = append(planned, ps) } return planned, routed.Disposition, nil } @@ -111,7 +71,10 @@ func (c *DiffCmd) run(ctx context.Context, out io.Writer) error { } defer pool.Close() - report := diffReport{Schema: c.Schema, Table: ds.Table, TableExists: true} + report := plan.NewReport(plan.SourceDiff) + report.Schema = c.Schema + report.Table = ds.Table + tableExists := true var changes []schemadiff.Change var facts planner.Facts live, err := schemadiff.Introspect(ctx, pool, c.Schema, ds.Table) @@ -120,7 +83,7 @@ func (c *DiffCmd) run(ctx context.Context, out io.Writer) error { // No live table: the plan is the desired schema itself, qualified // onto the target schema, classified with zero facts (there are no // live columns to sharpen type-change decisions). - report.TableExists = false + tableExists = false if changes, err = qualifiedDesired(ds, c.Schema); err != nil { return err } @@ -136,12 +99,13 @@ func (c *DiffCmd) run(ctx context.Context, out io.Writer) error { return err } } - if report.Changes, report.Disposition, err = classifyChanges(changes, facts); err != nil { + report.TableExists = &tableExists + if report.Statements, report.Disposition, err = classifyChanges(changes, facts); err != nil { return err } logger.Debug("diff derived", - "schema", c.Schema, "table", ds.Table, "changes", len(report.Changes), - "table_exists", report.TableExists, "disposition", string(report.Disposition)) + "schema", c.Schema, "table", ds.Table, "changes", len(report.Statements), + "table_exists", tableExists, "disposition", string(report.Disposition)) if c.JSON { return writeJSON(out, report) @@ -163,15 +127,15 @@ func qualifiedDesired(ds statement.DesiredSchema, schema string) ([]schemadiff.C return changes, nil } -// writeJSON emits the report as JSON. -func writeJSON(out io.Writer, report diffReport) error { - if report.Changes == nil { - report.Changes = []plannedChange{} +// writeJSON emits the plan report as JSON. +func writeJSON(out io.Writer, report plan.Report) error { + if report.Statements == nil { + report.Statements = []plan.Statement{} } enc := json.NewEncoder(out) enc.SetIndent("", " ") if err := enc.Encode(report); err != nil { - return fmt.Errorf("write diff report: %w", err) + return fmt.Errorf("write plan report: %w", err) } return nil } @@ -182,21 +146,21 @@ func writeJSON(out io.Writer, report diffReport) error { // stays valid SQL. Safer sequences appear as comment lines — never // substituted into the script body, which stays the literal convergence // plan (a CONCURRENTLY rewrite could not run inside a transaction block). -func writePlanText(out io.Writer, report diffReport) error { - if len(report.Changes) == 0 { +func writePlanText(out io.Writer, report plan.Report) error { + if len(report.Statements) == 0 { if _, err := fmt.Fprintln(out, "-- no changes: live table matches the desired schema"); err != nil { return fmt.Errorf("write plan: %w", err) } return nil } - if !report.TableExists { + if report.TableExists != nil && !*report.TableExists { if _, err := fmt.Fprintf(out, "-- table %s.%s does not exist; the plan is the full desired schema\n", report.Schema, report.Table); err != nil { return fmt.Errorf("write plan: %w", err) } } - for _, ch := range report.Changes { - if err := writeChangeText(out, ch); err != nil { + for _, ps := range report.Statements { + if err := writeChangeText(out, ps); err != nil { return err } } @@ -204,26 +168,26 @@ func writePlanText(out io.Writer, report diffReport) error { } // writeChangeText emits one annotated statement of the text plan. -func writeChangeText(out io.Writer, ch plannedChange) error { - if _, err := fmt.Fprintf(out, "-- %s\n", annotate(ch)); err != nil { +func writeChangeText(out io.Writer, ps plan.Statement) error { + if _, err := fmt.Fprintf(out, "-- %s\n", annotate(ps)); err != nil { return fmt.Errorf("write plan: %w", err) } - if len(ch.ExecSQL) > 0 && ch.ExecSQL[0] != ch.SQL { + if len(ps.ExecSQL) > 0 && ps.ExecSQL[0] != ps.SQL { if _, err := fmt.Fprintln(out, "-- the engine would run instead:"); err != nil { return fmt.Errorf("write plan: %w", err) } - for _, safer := range ch.ExecSQL { + for _, safer := range ps.ExecSQL { if _, err := fmt.Fprintf(out, "-- %s;\n", safer); err != nil { return fmt.Errorf("write plan: %w", err) } } } - if ch.Destructive { + if ps.Destructive { if _, err := fmt.Fprintln(out, "-- destructive"); err != nil { return fmt.Errorf("write plan: %w", err) } } - if _, err := fmt.Fprintf(out, "%s;\n", ch.SQL); err != nil { + if _, err := fmt.Fprintf(out, "%s;\n", ps.SQL); err != nil { return fmt.Errorf("write plan: %w", err) } return nil @@ -232,18 +196,18 @@ func writeChangeText(out io.Writer, ch plannedChange) error { // annotate renders one statement's route annotation: the route, the // distinct decision reasons, and the availability note for backends this // build does not implement. -func annotate(ch plannedChange) string { +func annotate(ps plan.Statement) string { var reasons []string seen := map[planner.Reason]bool{} - for _, d := range ch.Decisions { + for _, d := range ps.Decisions { if !seen[d.Reason] { seen[d.Reason] = true reasons = append(reasons, string(d.Reason)) } } - s := fmt.Sprintf("%s (%s)", ch.Route, strings.Join(reasons, ", ")) - if ch.Disposition == router.DispositionUnavailable { - s += ": needs the " + string(ch.Backend) + " backend, which is not implemented yet" + s := fmt.Sprintf("%s (%s)", ps.Route, strings.Join(reasons, ", ")) + if ps.Disposition == router.DispositionUnavailable { + s += ": needs the " + string(ps.Backend) + " backend, which is not implemented yet" } return s } diff --git a/internal/cli/diff_integration_test.go b/internal/cli/diff_integration_test.go index a415c3c..13f2c8c 100644 --- a/internal/cli/diff_integration_test.go +++ b/internal/cli/diff_integration_test.go @@ -14,6 +14,7 @@ import ( "github.com/block/pg-sprite/internal/testutil" "github.com/block/pg-sprite/pkg/dbconn" + "github.com/block/pg-sprite/pkg/plan" "github.com/block/pg-sprite/pkg/planner" "github.com/block/pg-sprite/pkg/router" ) @@ -52,15 +53,18 @@ func TestDiffPrintsOrderedPlanJSON(t *testing.T) { var out strings.Builder require.NoError(t, cmd.run(t.Context(), &out)) - var report diffReport + var report plan.Report require.NoError(t, json.Unmarshal([]byte(out.String()), &report)) + assert.Equal(t, plan.FormatVersion, report.FormatVersion) + assert.Equal(t, plan.SourceDiff, report.Source) assert.Equal(t, schema, report.Schema) assert.Equal(t, "events", report.Table) - assert.True(t, report.TableExists) + require.NotNil(t, report.TableExists) + assert.True(t, *report.TableExists) var sqls []string var destructive []bool - for _, ch := range report.Changes { + for _, ch := range report.Statements { sqls = append(sqls, ch.SQL) destructive = append(destructive, ch.Destructive) } @@ -76,8 +80,8 @@ func TestDiffPrintsOrderedPlanJSON(t *testing.T) { // binary-coercible by the live facts, SET NOT NULL and CREATE INDEX // carry their safer native sequences, and the whole plan would execute. assert.Equal(t, router.DispositionExecute, report.Disposition) - routes := make([]planner.Route, 0, len(report.Changes)) - for _, ch := range report.Changes { + routes := make([]planner.Route, 0, len(report.Statements)) + for _, ch := range report.Statements { routes = append(routes, ch.Route) assert.Equal(t, router.BackendNative, ch.Backend, ch.SQL) assert.Equal(t, router.DispositionExecute, ch.Disposition, ch.SQL) @@ -86,14 +90,14 @@ func TestDiffPrintsOrderedPlanJSON(t *testing.T) { assert.Equal(t, []planner.Route{ planner.RouteNative, planner.RouteNative, planner.RouteNative, planner.RouteNative, }, routes) - assert.Equal(t, planner.ReasonBinaryCoercible, report.Changes[1].Decisions[0].Reason, + assert.Equal(t, planner.ReasonBinaryCoercible, report.Statements[1].Decisions[0].Reason, "live column types must feed the classifier") - assert.Equal(t, planner.ReasonSaferIdiom, report.Changes[2].Decisions[0].Reason) - assert.NotEqual(t, []string{report.Changes[2].SQL}, report.Changes[2].ExecSQL, + assert.Equal(t, planner.ReasonSaferIdiom, report.Statements[2].Decisions[0].Reason) + assert.NotEqual(t, []string{report.Statements[2].SQL}, report.Statements[2].ExecSQL, "SET NOT NULL carries its safer native sequence") - assert.Equal(t, planner.ReasonSaferIdiom, report.Changes[3].Decisions[0].Reason) - require.Len(t, report.Changes[3].ExecSQL, 1) - assert.NotEqual(t, report.Changes[3].SQL, report.Changes[3].ExecSQL[0], + assert.Equal(t, planner.ReasonSaferIdiom, report.Statements[3].Decisions[0].Reason) + require.Len(t, report.Statements[3].ExecSQL, 1) + assert.NotEqual(t, report.Statements[3].SQL, report.Statements[3].ExecSQL[0], "CREATE INDEX carries its concurrent rewrite") } @@ -115,11 +119,11 @@ func TestDiffRoutesRewriteToCopyAndSwap(t *testing.T) { var out strings.Builder require.NoError(t, cmd.run(t.Context(), &out)) - var report diffReport + var report plan.Report require.NoError(t, json.Unmarshal([]byte(out.String()), &report)) assert.Equal(t, router.DispositionUnavailable, report.Disposition) - require.Len(t, report.Changes, 1) - ch := report.Changes[0] + require.Len(t, report.Statements, 1) + ch := report.Statements[0] assert.Equal(t, planner.RouteCopyAndSwap, ch.Route) assert.Equal(t, router.BackendCopyAndSwap, ch.Backend) assert.Equal(t, router.DispositionUnavailable, ch.Disposition) @@ -172,10 +176,11 @@ func TestDiffNoChangesEmptyPlan(t *testing.T) { var out strings.Builder require.NoError(t, cmd.run(t.Context(), &out)) - var report diffReport + var report plan.Report require.NoError(t, json.Unmarshal([]byte(out.String()), &report)) - assert.True(t, report.TableExists) - assert.Empty(t, report.Changes) + require.NotNil(t, report.TableExists) + assert.True(t, *report.TableExists) + assert.Empty(t, report.Statements) } func TestDiffMissingTableEmitsFullDesiredSchema(t *testing.T) { @@ -191,11 +196,12 @@ func TestDiffMissingTableEmitsFullDesiredSchema(t *testing.T) { var out strings.Builder require.NoError(t, cmd.run(t.Context(), &out)) - var report diffReport + var report plan.Report require.NoError(t, json.Unmarshal([]byte(out.String()), &report)) - assert.False(t, report.TableExists) + require.NotNil(t, report.TableExists) + assert.False(t, *report.TableExists) var sqls []string - for _, ch := range report.Changes { + for _, ch := range report.Statements { sqls = append(sqls, ch.SQL) } assert.Equal(t, []string{ @@ -226,7 +232,7 @@ func TestDiffTextPlanIsExecutableSQL(t *testing.T) { cmd2.JSON = true var out2 strings.Builder require.NoError(t, cmd2.run(t.Context(), &out2)) - var report diffReport + var report plan.Report require.NoError(t, json.Unmarshal([]byte(out2.String()), &report)) - assert.Empty(t, report.Changes, "executing the text plan must converge the table") + assert.Empty(t, report.Statements, "executing the text plan must converge the table") } diff --git a/internal/cli/dryrun.go b/internal/cli/dryrun.go index 3ebd57a..0ca6a77 100644 --- a/internal/cli/dryrun.go +++ b/internal/cli/dryrun.go @@ -2,14 +2,13 @@ package cli import ( "context" - "encoding/json" "errors" - "fmt" "io" "github.com/jackc/pgx/v5/pgxpool" "github.com/block/pg-sprite/pkg/dbconn" + "github.com/block/pg-sprite/pkg/plan" "github.com/block/pg-sprite/pkg/planner" "github.com/block/pg-sprite/pkg/router" "github.com/block/pg-sprite/pkg/schemadiff" @@ -40,24 +39,27 @@ func (c *MigrateCmd) runDryRun(ctx context.Context, out io.Writer) error { if err != nil { return err } - plan, err := planner.Classify(st.SQL, facts) + classified, err := planner.Classify(st.SQL, facts) if err != nil { return err } - routed := router.Route([]planner.Plan{plan}) + routed := router.Route([]planner.Plan{classified}) logger.Debug("statement routed", - "route", string(plan.Route), "disposition", string(routed.Disposition)) + "route", string(classified.Route), "disposition", string(routed.Disposition)) + + report := plan.NewReport(plan.SourceAlter) + report.Schema = st.Schema + report.Table = st.Table + report.Disposition = routed.Disposition + for _, rs := range routed.Statements { + report.Statements = append(report.Statements, plan.FromRouted(rs)) + } if c.JSON { - enc := json.NewEncoder(out) - enc.SetIndent("", " ") - if err := enc.Encode(routed); err != nil { - return fmt.Errorf("write dry-run plan: %w", err) - } - return nil + return writeJSON(out, report) } - for _, rs := range routed.Statements { - if err := writeChangeText(out, plannedFromRouted(rs)); err != nil { + for _, ps := range report.Statements { + if err := writeChangeText(out, ps); err != nil { return err } } @@ -84,15 +86,3 @@ func dryRunFacts(ctx context.Context, pool *pgxpool.Pool, st statement.Statement } return liveFacts(live), nil } - -// plannedFromRouted adapts a routed statement to the shared text renderer. -func plannedFromRouted(rs router.Statement) plannedChange { - return plannedChange{ - Change: schemadiff.Change{SQL: rs.Statement}, - Route: rs.Route, - Backend: rs.Backend, - Disposition: rs.Disposition, - Decisions: rs.Decisions, - ExecSQL: rs.ExecSQL, - } -} diff --git a/internal/cli/dryrun_integration_test.go b/internal/cli/dryrun_integration_test.go index 2ab3a6e..92ac759 100644 --- a/internal/cli/dryrun_integration_test.go +++ b/internal/cli/dryrun_integration_test.go @@ -11,21 +11,24 @@ import ( "github.com/block/pg-sprite/internal/testutil" "github.com/block/pg-sprite/pkg/dbconn" + "github.com/block/pg-sprite/pkg/plan" "github.com/block/pg-sprite/pkg/planner" "github.com/block/pg-sprite/pkg/router" ) -// dryRunPlan runs migrate --dry-run --json and decodes the routed plan. -func dryRunPlan(t *testing.T, url, alter string) router.Plan { +// dryRunPlan runs migrate --dry-run --json and decodes the plan report. +func dryRunPlan(t *testing.T, url, alter string) plan.Report { t.Helper() cmd := newMigrateCmd(url, alter) cmd.DryRun = true cmd.JSON = true var out strings.Builder require.NoError(t, cmd.run(t.Context(), &out)) - var plan router.Plan - require.NoError(t, json.Unmarshal([]byte(out.String()), &plan)) - return plan + var report plan.Report + require.NoError(t, json.Unmarshal([]byte(out.String()), &report)) + require.Equal(t, plan.FormatVersion, report.FormatVersion) + require.Equal(t, plan.SourceAlter, report.Source) + return report } // A rewrite-requiring change dry-runs to the copy-and-swap backend as @@ -39,10 +42,10 @@ func TestMigrateDryRunRoutesRewriteWithoutExecuting(t *testing.T) { _, err = pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.t (id int PRIMARY KEY)", schema)) require.NoError(t, err) - plan := dryRunPlan(t, url, fmt.Sprintf("ALTER TABLE %s.t ALTER COLUMN id TYPE bigint", schema)) - assert.Equal(t, router.DispositionUnavailable, plan.Disposition) - require.Len(t, plan.Statements, 1) - st := plan.Statements[0] + report := dryRunPlan(t, url, fmt.Sprintf("ALTER TABLE %s.t ALTER COLUMN id TYPE bigint", schema)) + assert.Equal(t, router.DispositionUnavailable, report.Disposition) + require.Len(t, report.Statements, 1) + st := report.Statements[0] assert.Equal(t, planner.RouteCopyAndSwap, st.Route) assert.Equal(t, router.BackendCopyAndSwap, st.Backend) assert.Empty(t, st.ExecSQL) @@ -67,10 +70,10 @@ func TestMigrateDryRunUsesLiveFacts(t *testing.T) { require.NoError(t, err) alter := fmt.Sprintf("ALTER TABLE %s.t ALTER COLUMN name TYPE varchar(50)", schema) - plan := dryRunPlan(t, url, alter) - assert.Equal(t, router.DispositionExecute, plan.Disposition) - require.Len(t, plan.Statements, 1) - st := plan.Statements[0] + report := dryRunPlan(t, url, alter) + assert.Equal(t, router.DispositionExecute, report.Disposition) + require.Len(t, report.Statements, 1) + st := report.Statements[0] assert.Equal(t, planner.RouteNative, st.Route) assert.Equal(t, router.BackendNative, st.Backend) require.Len(t, st.Decisions, 1) @@ -97,10 +100,10 @@ func TestMigrateDryRunSuggestsConcurrentIndex(t *testing.T) { require.NoError(t, err) submitted := fmt.Sprintf("CREATE INDEX t_id_idx ON %s.t (id)", schema) - plan := dryRunPlan(t, url, submitted) - assert.Equal(t, router.DispositionExecute, plan.Disposition) - require.Len(t, plan.Statements, 1) - st := plan.Statements[0] + report := dryRunPlan(t, url, submitted) + assert.Equal(t, router.DispositionExecute, report.Disposition) + require.Len(t, report.Statements, 1) + st := report.Statements[0] assert.Equal(t, planner.RouteNative, st.Route) require.Len(t, st.Decisions, 1) assert.Equal(t, planner.ReasonSaferIdiom, st.Decisions[0].Reason) @@ -119,8 +122,8 @@ func TestMigrateDryRunSuggestsConcurrentIndex(t *testing.T) { func TestMigrateDryRunMissingTableIsConservative(t *testing.T) { url := testutil.StartPostgres(t) - plan := dryRunPlan(t, url, "ALTER TABLE missing ALTER COLUMN v TYPE varchar(50)") - assert.Equal(t, router.DispositionUnavailable, plan.Disposition) - require.Len(t, plan.Statements, 1) - assert.Equal(t, planner.RouteCopyAndSwap, plan.Statements[0].Route) + report := dryRunPlan(t, url, "ALTER TABLE missing ALTER COLUMN v TYPE varchar(50)") + assert.Equal(t, router.DispositionUnavailable, report.Disposition) + require.Len(t, report.Statements, 1) + assert.Equal(t, planner.RouteCopyAndSwap, report.Statements[0].Route) } diff --git a/pkg/plan/plan.go b/pkg/plan/plan.go new file mode 100644 index 0000000..c5dd9d9 --- /dev/null +++ b/pkg/plan/plan.go @@ -0,0 +1,100 @@ +// Package plan defines the machine-readable dry-run plan report: the +// stable JSON contract an operator or orchestrator consumes to decide +// whether and how a change would execute. Both front doors emit it — the +// imperative migrate --dry-run path and the declarative diff path — so a +// consumer parses one shape regardless of how the plan was derived. +package plan + +import ( + "github.com/block/pg-sprite/pkg/planner" + "github.com/block/pg-sprite/pkg/router" +) + +// FormatVersion identifies the report contract. A consumer must reject a +// report whose version it does not understand instead of guessing at the +// field semantics. +const FormatVersion = 1 + +// Source identifies which front door derived the plan. +type Source string + +const ( + // SourceAlter marks a plan derived from a submitted DDL statement + // (migrate --alter --dry-run): the classify-and-route pipeline with + // the diff step skipped. + SourceAlter Source = "alter" + // SourceDiff marks a plan derived from a desired-state schema diff + // (diff --desired): the ordered statements that converge the live + // table on the desired schema. + SourceDiff Source = "diff" +) + +// Statement is one planned statement: the SQL, its classification, and +// what execution would do with it now. +type Statement struct { + // SQL is the submitted statement (alter) or the derived convergence + // statement (diff). + SQL string `json:"sql"` + // Destructive marks statements that drop live structure. + Destructive bool `json:"destructive,omitempty"` + // Route is the planner's aggregate route for the statement. + Route planner.Route `json:"route"` + // Backend is the assigned execution strategy; empty for refusals. + Backend router.Backend `json:"backend,omitempty"` + // Disposition is what execution would do with the statement now. + Disposition router.Disposition `json:"disposition"` + // Decisions are the planner's per-operation classifications. + Decisions []planner.Decision `json:"decisions"` + // ExecSQL is the ordered SQL the native backend would run — the safer + // sequence when the planner constructed one. Empty for non-native + // routes. + ExecSQL []string `json:"exec_sql,omitempty"` +} + +// Report is the dry-run plan for one change against one table. +type Report struct { + // FormatVersion is the report contract version; always FormatVersion. + FormatVersion int `json:"format_version"` + // Source is the front door that derived the plan. + Source Source `json:"source"` + // Schema is the target schema; empty when the submitted statement did + // not qualify one. + Schema string `json:"schema,omitempty"` + // Table is the target table; empty when the statement has no single + // table target (index maintenance). + Table string `json:"table,omitempty"` + // TableExists reports whether the live table was found. It is set + // only by sources that introspect for existence (diff); when false, + // the statements are the full desired schema. + TableExists *bool `json:"table_exists,omitempty"` + // Disposition is the aggregate disposition across all statements: + // what would happen if the engine executed this plan now. + Disposition router.Disposition `json:"disposition"` + // Statements is the ordered plan; empty means there is nothing to do. + Statements []Statement `json:"statements"` +} + +// NewReport returns an empty report for source with the contract version +// stamped and Statements non-nil, so an empty plan serializes as [] rather +// than null. +func NewReport(source Source) Report { + return Report{ + FormatVersion: FormatVersion, + Source: source, + Statements: []Statement{}, + } +} + +// FromRouted converts one routed statement into a plan statement. Fields +// the router does not know (Destructive) stay at their zero value for the +// caller to set. +func FromRouted(rs router.Statement) Statement { + return Statement{ + SQL: rs.Statement, + Route: rs.Route, + Backend: rs.Backend, + Disposition: rs.Disposition, + Decisions: rs.Decisions, + ExecSQL: rs.ExecSQL, + } +} diff --git a/pkg/plan/plan_test.go b/pkg/plan/plan_test.go new file mode 100644 index 0000000..4a46744 --- /dev/null +++ b/pkg/plan/plan_test.go @@ -0,0 +1,133 @@ +package plan_test + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/block/pg-sprite/pkg/plan" + "github.com/block/pg-sprite/pkg/planner" + "github.com/block/pg-sprite/pkg/router" +) + +func TestNewReportStampsVersionAndEmptyStatements(t *testing.T) { + r := plan.NewReport(plan.SourceDiff) + assert.Equal(t, plan.FormatVersion, r.FormatVersion) + assert.Equal(t, plan.SourceDiff, r.Source) + require.NotNil(t, r.Statements) + assert.Empty(t, r.Statements) +} + +func TestFromRoutedMapsEveryRoutedField(t *testing.T) { + rs := router.Statement{ + Plan: planner.Plan{ + Statement: "CREATE INDEX i ON t (c)", + Route: planner.RouteNative, + Decisions: []planner.Decision{{ + Operation: "create index", + Route: planner.RouteNative, + Reason: planner.ReasonSaferIdiom, + SaferSQL: []string{"CREATE INDEX CONCURRENTLY i ON t (c)"}, + }}, + }, + Backend: router.BackendNative, + Disposition: router.DispositionExecute, + ExecSQL: []string{"CREATE INDEX CONCURRENTLY i ON t (c)"}, + } + st := plan.FromRouted(rs) + assert.Equal(t, "CREATE INDEX i ON t (c)", st.SQL) + assert.Equal(t, planner.RouteNative, st.Route) + assert.Equal(t, router.BackendNative, st.Backend) + assert.Equal(t, router.DispositionExecute, st.Disposition) + assert.Equal(t, rs.Decisions, st.Decisions) + assert.Equal(t, rs.ExecSQL, st.ExecSQL) + assert.False(t, st.Destructive, "the router does not know destructiveness") +} + +// The JSON shape is the adapter-facing contract: exact keys, exact +// omissions. A consumer pins format_version 1 against this test. +func TestReportJSONShape(t *testing.T) { + exists := true + r := plan.Report{ + FormatVersion: plan.FormatVersion, + Source: plan.SourceDiff, + Schema: "public", + Table: "t", + TableExists: &exists, + Disposition: router.DispositionRefuse, + Statements: []plan.Statement{ + { + SQL: "DROP INDEX t_c_idx", + Destructive: true, + Route: planner.RouteNative, + Backend: router.BackendNative, + Disposition: router.DispositionExecute, + Decisions: []planner.Decision{{ + Operation: "drop index", + Route: planner.RouteNative, + Reason: planner.ReasonSaferIdiom, + }}, + ExecSQL: []string{"DROP INDEX CONCURRENTLY t_c_idx"}, + }, + { + SQL: "ALTER TABLE t NO SUCH THING", + Route: planner.RouteRefuse, + Disposition: router.DispositionRefuse, + Decisions: []planner.Decision{{ + Operation: "unrecognized", + Route: planner.RouteRefuse, + Reason: planner.ReasonUnsupportedOperation, + }}, + }, + }, + } + raw, err := json.Marshal(r) + require.NoError(t, err) + assert.JSONEq(t, `{ + "format_version": 1, + "source": "diff", + "schema": "public", + "table": "t", + "table_exists": true, + "disposition": "refuse", + "statements": [ + { + "sql": "DROP INDEX t_c_idx", + "destructive": true, + "route": "native", + "backend": "native", + "disposition": "execute", + "decisions": [ + {"operation": "drop index", "route": "native", "reason": "safer-idiom"} + ], + "exec_sql": ["DROP INDEX CONCURRENTLY t_c_idx"] + }, + { + "sql": "ALTER TABLE t NO SUCH THING", + "route": "refuse", + "disposition": "refuse", + "decisions": [ + {"operation": "unrecognized", "route": "refuse", "reason": "unsupported-operation"} + ] + } + ] + }`, string(raw)) +} + +// Optional envelope fields are omitted, not emitted as zero values: an +// alter-source report has no table_exists, and an empty plan serializes +// its statements as []. +func TestReportJSONOmitsUnsetOptionalFields(t *testing.T) { + r := plan.NewReport(plan.SourceAlter) + r.Disposition = router.DispositionExecute + raw, err := json.Marshal(r) + require.NoError(t, err) + assert.JSONEq(t, `{ + "format_version": 1, + "source": "alter", + "disposition": "execute", + "statements": [] + }`, string(raw)) +} From f91a9fcbc5330027683a82312f1090deb69093fd Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Fri, 7 Aug 2026 14:20:38 +1000 Subject: [PATCH 2/2] Address plan-contract review: converge both front doors Destructiveness is derived in the classifier so both doors report it identically by construction; report SQL is canonicalized through the deparser (commented input refused, never silently stripped); the report gains a mandatory fingerprint, server_version, resolved schema, and closed vocabularies pinned by test. Contract documented in docs/plan-report.md with generated examples. Review: block/pg-sprite#8 (two-lens and adversarial passes) --- docs/README.md | 1 + docs/plan-report.md | 256 ++++++++++++++++++++++++ internal/cli/cli.go | 12 ++ internal/cli/diff.go | 17 +- internal/cli/diff_integration_test.go | 8 +- internal/cli/dryrun.go | 33 ++- internal/cli/dryrun_integration_test.go | 62 ++++++ internal/testutil/postgres.go | 19 ++ pkg/plan/docs_test.go | 114 +++++++++++ pkg/plan/plan.go | 89 +++++++- pkg/plan/plan_test.go | 103 +++++++++- pkg/planner/planner.go | 51 ++++- pkg/planner/planner_test.go | 54 +++++ pkg/router/router.go | 21 ++ pkg/router/router_test.go | 20 ++ pkg/schemadiff/diff.go | 21 ++ pkg/schemadiff/diff_test.go | 20 ++ pkg/statement/ops.go | 44 +++- pkg/statement/ops_test.go | 46 ++++- pkg/statement/statement.go | 19 ++ pkg/statement/statement_test.go | 33 +++ 21 files changed, 1005 insertions(+), 38 deletions(-) create mode 100644 docs/plan-report.md create mode 100644 pkg/plan/docs_test.go diff --git a/docs/README.md b/docs/README.md index 774da46..f37509d 100644 --- a/docs/README.md +++ b/docs/README.md @@ -45,6 +45,7 @@ checkpoint/resume, tuned for Aurora. That is the gap this engine targets. | [change-capture-tradeoff.md](change-capture-tradeoff.md) | The canonical **triggers vs logical-decoding** trade-off for copy-and-swap — overhead, failover survival, WAL risk, and whether either lets us drop the checksum/checkpoint (answer: keep the checksum; triggers simplify but don't remove the checkpoint). Any doc proposing logical decoding as the default points here. | | [invariants.md](invariants.md) | The canonical **invariant registry** — testable runtime MUST-statements (correctness, locking, state/resume, refusals, orchestration), each with its enforcement point and source. Mined from this doc set plus [Spirit](https://github.com/block/spirit)'s stated safety invariants and [SchemaBot](https://github.com/block/schemabot)'s control-plane discipline; the build plan's phases carry per-invariant test obligations. | | [tcb-model.md](tcb-model.md) | The **TCB model** — the trusted-computing-base partition of the engine: which components are the small trusted core that enforces the invariant registry vs the untrusted periphery, the never-trust-callers rule, domain types that make illegal states unrepresentable, the in-TCB engineering rules (from TigerBeetle TIGER_STYLE, s2n-tls, qmail, bitcoin-core), the verification ladder, and the per-side AI-assisted development policy. | +| [plan-report.md](plan-report.md) | The **plan report contract** — the versioned JSON shape both front doors emit for dry-run plans: fields, closed vocabularies, the fingerprint identity, required consumer behavior for unknown versions/values, and one generated example per source (pinned by test). | | [testing.md](testing.md) | The **test-suite guide** — how to run the suite (unit, per-major, all supported majors, compose database), current coverage, the remaining executor-phase test obligations, and the vanilla-PostgreSQL-matrix vs real-Aurora validation boundary. | | [schemabot-integration.md](schemabot-integration.md) | The **single home for orchestrator integration** — how SchemaBot (the reference orchestrator) drives the engine: the pluggable-engine overview, the verb mappings, the concrete adapter contract, and the design constraints (OC-* invariants) the integration imposes on the core. | diff --git a/docs/plan-report.md b/docs/plan-report.md new file mode 100644 index 0000000..fb2afa5 --- /dev/null +++ b/docs/plan-report.md @@ -0,0 +1,256 @@ +# The plan report contract + +The plan report is the machine-readable dry-run plan both front doors emit — `migrate +--dry-run --json` (imperative) and `diff --json` (declarative). It is the one JSON shape an +operator or orchestrator consumes to decide whether and how a change would execute. This +document is the contract: the fields, the closed vocabularies, the identity rules, and the +behavior required of a consumer. The Go source of truth is `pkg/plan`; tests in `pkg/plan`, +`pkg/planner`, `pkg/router`, and `pkg/schemadiff` pin everything documented here, including +the examples at the end of this page. + +## Versioning: `format_version` + +Every report carries `format_version`. A consumer that does not recognize the version must +**reject the report** — never guess at field semantics. The version covers more than the +field shape: the closed vocabularies below (sources, routes, reasons, backends, dispositions, +kinds) and the fingerprint serialization are all pinned to it. Adding a vocabulary value or +changing the fingerprint definition is a contract change and bumps `format_version`, even if +no field is added or renamed. + +## Consumer behavior for unknown values + +Every enum field in the report draws from a closed vocabulary listed here. A consumer that +meets a value it does not recognize must **treat the statement as unknown and refuse it** — +never ignore it and proceed. This is the same fail-closed posture the engine itself takes +with SQL it does not fully understand. + +## Report fields + +| Field | Type | Presence | Meaning | +|---|---|---|---| +| `format_version` | int | always | Contract version; reject unknown versions. | +| `source` | string | always | Front door that derived the plan (see Sources). | +| `schema` | string | when resolved | Target schema. The **resolved** name the engine planned against — an unqualified alter reports the schema the engine introspected (`public`), never an empty echo of the submitted text. Absent only when the statement has no single table target. | +| `table` | string | when targeted | Target table; absent for statements with no single table target (index maintenance). | +| `server_version` | string | when connected | The PostgreSQL `server_version` the plan was derived against. Classification is version-sensitive; a stored or forwarded report names the server whose rules produced it. | +| `table_exists` | bool | diff source only | Whether the live table was found. Absent means "not introspected" (alter source); `false` means the plan is the full desired schema. | +| `disposition` | string | always | Aggregate disposition across all statements (see Dispositions). | +| `fingerprint` | string | always | The plan's stable identity (see Fingerprint). | +| `statements` | array | always | The ordered plan; `[]` (never `null`) means nothing to do. | + +## Statement fields + +| Field | Type | Presence | Meaning | +|---|---|---|---| +| `sql` | string | always | The statement in the engine's **canonical rendering**: parsed and reprinted through the PostgreSQL deparser, whichever front door derived it. Never a verbatim echo of submitted text — the same change carries the same string through either door. Commented input is refused rather than silently stripped; optional noise words follow the grammar's canonical spelling. | +| `kind` | string | diff source only | Classifies a diff-derived statement (see Kinds) so a consumer can gate whole classes of change. Absent for the alter source: a submitted statement may carry several operations and has no single kind. | +| `destructive` | bool | always | Marks statements that discard live structure — a dropped column, constraint, or index. Derived from the classifier's decisions, so both sources report it identically by construction. Always emitted, never omitted: a safety flag a consumer gates on must be explicit even when false. | +| `route` | string | always | The planner's aggregate route for the statement (see Routes). | +| `backend` | string | except refusals | The assigned execution strategy (see Backends); absent for refusals. | +| `disposition` | string | always | What execution would do with this statement now (see Dispositions). | +| `decisions` | array | always | The planner's per-operation classifications (below). | +| `exec_sql` | array | native route | The ordered SQL the native backend would run — the safer sequence when the planner constructed one. Absent for non-native routes. | + +## Decision fields + +| Field | Type | Presence | Meaning | +|---|---|---|---| +| `operation` | string | always | Operator-facing label (`DROP COLUMN legacy_status`). Display only — never branch on it. | +| `destructive` | bool | always | Whether this operation discards live structure. Always emitted, never omitted. | +| `route` | string | always | Where the operation goes (see Routes). | +| `reason` | string | always | The typed cause of the routing decision (see Reasons). Automation branches on this, never on prose. | +| `safer_sql` | array | safer-idiom only | The ordered native sequence to run instead of the submitted form, when the planner could construct it. | + +## Closed vocabularies + +### Sources (`source`) + +| Value | Meaning | +|---|---| +| `alter` | Derived from a submitted DDL statement (`migrate --dry-run`). | +| `diff` | Derived from a desired-state schema diff (`diff --desired`). | + +### Routes (`route`) + +| Value | Meaning | +|---|---| +| `native` | PostgreSQL runs it online natively — directly or via the safer idiom in `exec_sql`. | +| `copy-and-swap` | Needs a table rewrite; only the engine's shadow copy + cutover can do it online. | +| `refuse` | No known safe path; not executed. | + +### Reasons (`reason`) + +| Value | Meaning | +|---|---| +| `metadata-only` | A brief ACCESS EXCLUSIVE catalog change, no scan and no rewrite. | +| `online-idiom` | Already the safe native form (CONCURRENTLY, NOT VALID, VALIDATE, USING INDEX). | +| `fast-default` | ADD COLUMN with a constant default — the catalog stores the default, no rewrite. | +| `binary-coercible` | A type change PostgreSQL relabels without a rewrite (widen varchar, varchar to text, widen numeric precision). | +| `safer-idiom` | Native, but the submitted form blocks; `safer_sql` carries the online rewrite when one can be constructed. | +| `volatile-default` | ADD COLUMN whose default the planner cannot prove constant — PostgreSQL rewrites the table. | +| `generated-stored` | Adding a stored generated column computes every row — a full rewrite. | +| `type-rewrite` | A type conversion PostgreSQL cannot relabel — rewrite plus reindex. | +| `relocation` | SET TABLESPACE moves the heap — a rewrite-scale copy. | +| `partition-parent-lock` | Partition attach/detach in its lock-taking form. | +| `unsupported-operation` | The planner does not recognize the operation or knows no safe path for it. | + +### Backends (`backend`) + +| Value | Meaning | +|---|---| +| `native` | Direct PostgreSQL DDL (the safer sequence when one exists). | +| `copy-and-swap` | Shadow-table copy with checksum-gated cutover. | + +### Dispositions (`disposition`) + +| Value | Meaning | +|---|---| +| `execute` | The engine would run it now. | +| `rewrite-required` | Native but blocking as submitted, and no safer sequence could be constructed — resubmit in the online form. | +| `unavailable` | Routed to a backend that is not yet implemented. | +| `refuse` | The planner refused the statement; no backend is assigned. | + +### Kinds (`kind`, diff source only) + +| Value | Meaning | +|---|---| +| `create-table` | Creates the table (missing-table plans only). | +| `drop-index` | Drops an index. | +| `drop-constraint` | Drops a table constraint. | +| `drop-column` | Drops a column. | +| `add-column` | Adds a column. | +| `alter-type` | Changes a column's type. | +| `set-default` | Sets or replaces a column default. | +| `drop-default` | Drops a column default. | +| `set-not-null` | Adds the NOT NULL attribute. | +| `drop-not-null` | Removes the NOT NULL attribute. | +| `add-constraint` | Adds a table constraint. | +| `create-index` | Creates an index. | + +## Fingerprint + +`fingerprint` is the plan's stable identity: `sha256:` plus the hex digest over what would +execute. It exists for one consumer protocol: an approver pins it when the plan is +reviewed, and an executor recomputes it at apply time and refuses on mismatch — that is how +"the plan a reviewer approves is the plan that executes" survives storage and forwarding. +The engine computes and reports the fingerprint on every plan; enforcing the pin at apply +time is the consumer's side of the contract. + +The serialization is exact and pinned by test: for each statement in plan order, hash the +canonical `sql`, `route`, `backend`, and `disposition`, then each `exec_sql` entry — every +field followed by a unit separator (`0x1F`) — and close each statement with a record +separator (`0x1E`). Explanatory fields (`decisions`, `kind`, `destructive`) are excluded: a +reworded reason does not change identity, but a rerouted, resequenced, or rewritten plan +does. An empty plan has a defined identity (the digest of no input). + +This is a **plan identity, not a schema fingerprint**. The engine's schema-state comparisons +only ever compare server-decompiled output against server-decompiled output (see +`pkg/statement`); the plan fingerprint never participates in them. + +## Examples + +Both examples are generated by the real classify-and-route pipeline and pinned by a test in +`pkg/plan` — if the code drifts from this page, CI fails. + +### `source: alter` — `migrate --dry-run --json` + +`ALTER TABLE app.orders DROP COLUMN legacy_status` against a live table: + +```json +{ + "format_version": 1, + "source": "alter", + "schema": "app", + "table": "orders", + "server_version": "16.4", + "disposition": "execute", + "fingerprint": "sha256:acca39fb0630089005cb0ce6519406b1c1cfa8e122aeef044f5502a6b16accbc", + "statements": [ + { + "sql": "ALTER TABLE app.orders DROP legacy_status", + "destructive": true, + "route": "native", + "backend": "native", + "disposition": "execute", + "decisions": [ + { + "operation": "DROP COLUMN legacy_status", + "destructive": true, + "route": "native", + "reason": "metadata-only" + } + ], + "exec_sql": [ + "ALTER TABLE app.orders DROP legacy_status" + ] + } + ] +} +``` + +Note the canonical rendering: the deparser spells `DROP legacy_status` (the grammar treats +`COLUMN` as optional noise), and there is no `table_exists` — the alter source does not +introspect for existence. + +### `source: diff` — `diff --json` + +A desired state that drops an index and adds a column with a constant default: + +```json +{ + "format_version": 1, + "source": "diff", + "schema": "app", + "table": "orders", + "server_version": "16.4", + "table_exists": true, + "disposition": "execute", + "fingerprint": "sha256:cb7ec645948ff1d239ba4ce3f0e051e4e2bcebec799fdd6f8401399d4a246f53", + "statements": [ + { + "sql": "DROP INDEX app.orders_legacy_idx", + "kind": "drop-index", + "destructive": true, + "route": "native", + "backend": "native", + "disposition": "execute", + "decisions": [ + { + "operation": "DROP INDEX app.orders_legacy_idx", + "destructive": true, + "route": "native", + "reason": "safer-idiom", + "safer_sql": [ + "DROP INDEX CONCURRENTLY app.orders_legacy_idx" + ] + } + ], + "exec_sql": [ + "DROP INDEX CONCURRENTLY app.orders_legacy_idx" + ] + }, + { + "sql": "ALTER TABLE app.orders ADD COLUMN region text DEFAULT 'emea'", + "kind": "add-column", + "destructive": false, + "route": "native", + "backend": "native", + "disposition": "execute", + "decisions": [ + { + "operation": "ADD COLUMN region", + "destructive": false, + "route": "native", + "reason": "fast-default" + } + ], + "exec_sql": [ + "ALTER TABLE app.orders ADD COLUMN region text DEFAULT 'emea'" + ] + } + ] +} +``` + +Note `kind` on each statement (diff source only), the blocking `DROP INDEX` replaced by its +CONCURRENTLY form in `exec_sql`, and `table_exists: true`. diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 84972fb..24a14da 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -12,6 +12,7 @@ import ( "time" "github.com/alecthomas/kong" + "github.com/jackc/pgx/v5/pgxpool" "github.com/block/pg-sprite/pkg/dbconn" ) @@ -64,6 +65,17 @@ func (f DBFlags) Config() dbconn.Config { return cfg } +// serverVersion reads the connected server's server_version setting for +// the plan report: classification is version-sensitive, so a stored report +// names the server whose rules produced it. +func serverVersion(ctx context.Context, pool *pgxpool.Pool) (string, error) { + var v string + if err := pool.QueryRow(ctx, "SELECT current_setting('server_version')").Scan(&v); err != nil { + return "", fmt.Errorf("read server_version: %w", err) + } + return v, nil +} + // diag returns the diagnostics logger: debug-level text on stderr (or the // test override) under --debug, a discarding logger otherwise. Diagnostics // never share stdout with command output. diff --git a/internal/cli/diff.go b/internal/cli/diff.go index 684bd21..a85b68b 100644 --- a/internal/cli/diff.go +++ b/internal/cli/diff.go @@ -23,9 +23,16 @@ import ( func classifyChanges(changes []schemadiff.Change, facts planner.Facts) ([]plan.Statement, router.Disposition, error) { plans := make([]planner.Plan, 0, len(changes)) for _, ch := range changes { - classified, err := planner.Classify(ch.SQL, facts) + // Canonicalize before classifying so the report carries the + // engine's canonical rendering — the same string the alter front + // door would report for the same change. + canonical, err := statement.Canonical(ch.SQL) if err != nil { - return nil, "", fmt.Errorf("classify derived statement %q: %w", ch.SQL, err) + return nil, "", fmt.Errorf("canonicalize derived statement %q: %w", ch.SQL, err) + } + classified, err := planner.Classify(canonical, facts) + if err != nil { + return nil, "", fmt.Errorf("classify derived statement %q: %w", canonical, err) } plans = append(plans, classified) } @@ -33,7 +40,7 @@ func classifyChanges(changes []schemadiff.Change, facts planner.Facts) ([]plan.S planned := make([]plan.Statement, 0, len(changes)) for i, ch := range changes { ps := plan.FromRouted(routed.Statements[i]) - ps.Destructive = ch.Destructive + ps.Kind = ch.Kind planned = append(planned, ps) } return planned, routed.Disposition, nil @@ -74,6 +81,9 @@ func (c *DiffCmd) run(ctx context.Context, out io.Writer) error { report := plan.NewReport(plan.SourceDiff) report.Schema = c.Schema report.Table = ds.Table + if report.ServerVersion, err = serverVersion(ctx, pool); err != nil { + return err + } tableExists := true var changes []schemadiff.Change var facts planner.Facts @@ -103,6 +113,7 @@ func (c *DiffCmd) run(ctx context.Context, out io.Writer) error { if report.Statements, report.Disposition, err = classifyChanges(changes, facts); err != nil { return err } + report.Fingerprint = plan.Fingerprint(report.Statements) logger.Debug("diff derived", "schema", c.Schema, "table", ds.Table, "changes", len(report.Statements), "table_exists", tableExists, "disposition", string(report.Disposition)) diff --git a/internal/cli/diff_integration_test.go b/internal/cli/diff_integration_test.go index 8321482..c5cebf6 100644 --- a/internal/cli/diff_integration_test.go +++ b/internal/cli/diff_integration_test.go @@ -71,10 +71,12 @@ func TestDiffPrintsOrderedPlanJSON(t *testing.T) { kinds = append(kinds, ch.Kind) destructive = append(destructive, ch.Destructive) } + // Diff-derived SQL is canonicalized through the engine's parser, so both + // front doors report the same rendering for equivalent statements. assert.Equal(t, []string{ - fmt.Sprintf(`ALTER TABLE "%s"."events" DROP COLUMN "legacy"`, schema), - fmt.Sprintf(`ALTER TABLE "%s"."events" ALTER COLUMN "name" TYPE character varying(50)`, schema), - fmt.Sprintf(`ALTER TABLE "%s"."events" ALTER COLUMN "name" SET NOT NULL`, schema), + fmt.Sprintf("ALTER TABLE %s.events DROP legacy", schema), + fmt.Sprintf("ALTER TABLE %s.events ALTER COLUMN name TYPE varchar(50)", schema), + fmt.Sprintf("ALTER TABLE %s.events ALTER COLUMN name SET NOT NULL", schema), fmt.Sprintf("CREATE INDEX events_name_idx ON %s.events USING btree (name)", schema), }, sqls) assert.Equal(t, []schemadiff.ChangeKind{ diff --git a/internal/cli/dryrun.go b/internal/cli/dryrun.go index 509f4d8..ff20863 100644 --- a/internal/cli/dryrun.go +++ b/internal/cli/dryrun.go @@ -39,7 +39,14 @@ func (c *MigrateCmd) runDryRun(ctx context.Context, out io.Writer) error { if err != nil { return err } - classified, err := planner.Classify(st.SQL(), facts) + // The report carries the engine's canonical rendering, not an echo of + // the submitted text, so both front doors describe the same change + // with the same string (and the fingerprint agrees across them). + canonical, err := statement.Canonical(st.SQL()) + if err != nil { + return err + } + classified, err := planner.Classify(canonical, facts) if err != nil { return err } @@ -48,12 +55,16 @@ func (c *MigrateCmd) runDryRun(ctx context.Context, out io.Writer) error { "route", string(classified.Route), "disposition", string(routed.Disposition)) report := plan.NewReport(plan.SourceAlter) - report.Schema = st.Schema() + report.Schema = resolvedSchema(st) report.Table = st.Table() + if report.ServerVersion, err = serverVersion(ctx, pool); err != nil { + return err + } report.Disposition = routed.Disposition for _, rs := range routed.Statements { report.Statements = append(report.Statements, plan.FromRouted(rs)) } + report.Fingerprint = plan.Fingerprint(report.Statements) if c.JSON { return writeJSON(out, report) @@ -66,6 +77,18 @@ func (c *MigrateCmd) runDryRun(ctx context.Context, out io.Writer) error { return nil } +// resolvedSchema is the schema the engine plans against: the statement's +// qualification, or public — the default the engine introspects — when a +// table-targeted statement leaves it unqualified. The report carries the +// resolved name, never the submitted one: a stored plan must not depend on +// the reader's search_path to say which table it describes. +func resolvedSchema(st statement.Statement) string { + if st.Schema() == "" && st.Table() != "" { + return "public" + } + return st.Schema() +} + // dryRunFacts introspects the statement's target table for classifier // facts. Statements without a single table target (index maintenance) and // missing tables classify with zero facts. @@ -73,11 +96,7 @@ func dryRunFacts(ctx context.Context, pool *pgxpool.Pool, st statement.Statement if st.Table() == "" { return planner.Facts{}, nil } - schema := st.Schema() - if schema == "" { - schema = "public" - } - live, err := schemadiff.Introspect(ctx, pool, schema, st.Table()) + live, err := schemadiff.Introspect(ctx, pool, resolvedSchema(st), st.Table()) switch { case errors.Is(err, schemadiff.ErrTableNotFound): return planner.Facts{}, nil diff --git a/internal/cli/dryrun_integration_test.go b/internal/cli/dryrun_integration_test.go index b2c60eb..4ce0d84 100644 --- a/internal/cli/dryrun_integration_test.go +++ b/internal/cli/dryrun_integration_test.go @@ -146,6 +146,68 @@ func TestMigrateDryRunInlineConstraintIsRewriteRequired(t *testing.T) { assert.Equal(t, 0, columns, "dry-run must not add the column") } +// Both front doors must agree on destructiveness: the same DROP COLUMN is +// destructive whether a human submits it directly (the riskier door) or the +// diff derives it from a reviewed desired-state file. Destructive is derived +// from the classifier's decisions, so the agreement holds by construction. +func TestDryRunAndDiffAgreeOnDestructive(t *testing.T) { + url := testutil.StartPostgres(t) + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url}) + require.NoError(t, err) + defer pool.Close() + schema := testutil.NewSchema(t, pool) + _, err = pool.Exec(t.Context(), fmt.Sprintf( + "CREATE TABLE %s.t (id int PRIMARY KEY, doomed text)", schema)) + require.NoError(t, err) + + alterReport := dryRunPlan(t, url, fmt.Sprintf("ALTER TABLE %s.t DROP COLUMN doomed", schema)) + require.Len(t, alterReport.Statements, 1) + alterSt := alterReport.Statements[0] + assert.True(t, alterSt.Destructive, "the submitted DROP COLUMN must be marked destructive") + + cmd := newDiffCmd(t, url, schema, "CREATE TABLE t (id int PRIMARY KEY);") + cmd.JSON = true + var out strings.Builder + require.NoError(t, cmd.run(t.Context(), &out)) + var diffReport plan.Report + require.NoError(t, json.Unmarshal([]byte(out.String()), &diffReport)) + require.Len(t, diffReport.Statements, 1) + diffSt := diffReport.Statements[0] + + assert.Equal(t, alterSt.Destructive, diffSt.Destructive, "front doors must agree on destructive") + assert.Equal(t, alterSt.Route, diffSt.Route) + assert.Equal(t, alterSt.Backend, diffSt.Backend) + assert.Equal(t, alterSt.Disposition, diffSt.Disposition) + assert.Equal(t, alterSt.SQL, diffSt.SQL, + "both doors must render the same change as the same canonical string") + assert.Equal(t, alterReport.Fingerprint, diffReport.Fingerprint, + "the same plan must carry the same identity through either door") + assert.NotEmpty(t, alterReport.Fingerprint) +} + +// An unqualified statement resolves to the schema the engine actually +// introspected — public — and the report says so: a stored plan must not +// depend on the reader's search_path to name its target. The report also +// stamps the server version its classification was derived against. +func TestDryRunResolvesUnqualifiedSchemaAndStampsServerVersion(t *testing.T) { + url := testutil.StartPostgres(t) + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url}) + require.NoError(t, err) + defer pool.Close() + // A uniquely named table in public: unqualified statements resolve + // there, and the unique name keeps a shared PG_DSN database safe. + table := testutil.NewPublicTable(t, pool, "(id int PRIMARY KEY, v varchar(50))") + + report := dryRunPlan(t, url, fmt.Sprintf("ALTER TABLE %s ALTER COLUMN v TYPE varchar(100)", table)) + assert.Equal(t, "public", report.Schema, + "the report must name the schema the engine introspected, not echo the submitted qualification") + assert.NotEmpty(t, report.ServerVersion, + "the report must stamp the server version its classification came from") + require.Len(t, report.Statements, 1) + assert.Equal(t, planner.ReasonBinaryCoercible, report.Statements[0].Decisions[0].Reason, + "resolving to public must feed the live facts to the classifier") +} + // A dry-run against a table that does not exist classifies with zero facts: // the unprovable type change routes conservatively instead of failing. func TestMigrateDryRunMissingTableIsConservative(t *testing.T) { diff --git a/internal/testutil/postgres.go b/internal/testutil/postgres.go index 1535f61..2c37dbd 100644 --- a/internal/testutil/postgres.go +++ b/internal/testutil/postgres.go @@ -78,3 +78,22 @@ func NewSchema(t *testing.T, pool *pgxpool.Pool) string { }) return name } + +// NewPublicTable creates a uniquely named throwaway table in the public +// schema — for tests that exercise unqualified-statement resolution, where +// a dedicated schema would defeat the point — and returns its name. The +// unique name keeps a shared PG_DSN database safe; cleanup drops the table. +func NewPublicTable(t *testing.T, pool *pgxpool.Pool, columns string) string { + t.Helper() + name := fmt.Sprintf("t_%d_%d", os.Getpid(), schemaSeq.Add(1)) + _, err := pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE public.%s %s", name, columns)) + require.NoError(t, err, "create throwaway public table") + t.Cleanup(func() { + // t.Context is cancelled by cleanup time; strip the cancellation. + _, err := pool.Exec(context.WithoutCancel(t.Context()), fmt.Sprintf("DROP TABLE IF EXISTS public.%s", name)) + if err != nil { + t.Logf("drop throwaway public table %s: %v", name, err) + } + }) + return name +} diff --git a/pkg/plan/docs_test.go b/pkg/plan/docs_test.go new file mode 100644 index 0000000..224addc --- /dev/null +++ b/pkg/plan/docs_test.go @@ -0,0 +1,114 @@ +package plan_test + +import ( + "encoding/json" + "fmt" + "os" + "regexp" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/block/pg-sprite/pkg/plan" + "github.com/block/pg-sprite/pkg/planner" + "github.com/block/pg-sprite/pkg/router" + "github.com/block/pg-sprite/pkg/schemadiff" + "github.com/block/pg-sprite/pkg/statement" +) + +// planReportDoc is the human-facing contract page these tests keep honest. +const planReportDoc = "../../docs/plan-report.md" + +func readDoc(t *testing.T) string { + t.Helper() + raw, err := os.ReadFile(planReportDoc) + require.NoError(t, err) + return string(raw) +} + +// classifyCanonical routes one statement the way both front doors do: +// canonicalize, classify, route. +func classifyCanonical(t *testing.T, sql string) planner.Plan { + t.Helper() + canonical, err := statement.Canonical(sql) + require.NoError(t, err) + classified, err := planner.Classify(canonical, planner.Facts{}) + require.NoError(t, err) + return classified +} + +// The doc's example reports are generated output, not prose: rebuilding +// them through the real classify-and-route pipeline must reproduce the +// published JSON byte for byte (up to JSON equivalence). If this fails, +// regenerate the examples in docs/plan-report.md. +func TestDocExamplesMatchPipelineOutput(t *testing.T) { + doc := readDoc(t) + blocks := regexp.MustCompile("(?s)```json\n(.*?)```").FindAllStringSubmatch(doc, -1) + require.Len(t, blocks, 2, "the doc publishes one example per source") + + alter := plan.NewReport(plan.SourceAlter) + alter.Schema, alter.Table, alter.ServerVersion = "app", "orders", "16.4" + routedA := router.Route([]planner.Plan{ + classifyCanonical(t, "ALTER TABLE app.orders DROP COLUMN legacy_status"), + }) + alter.Disposition = routedA.Disposition + for _, rs := range routedA.Statements { + alter.Statements = append(alter.Statements, plan.FromRouted(rs)) + } + alter.Fingerprint = plan.Fingerprint(alter.Statements) + + diff := plan.NewReport(plan.SourceDiff) + diff.Schema, diff.Table, diff.ServerVersion = "app", "orders", "16.4" + exists := true + diff.TableExists = &exists + routedD := router.Route([]planner.Plan{ + classifyCanonical(t, `DROP INDEX "app"."orders_legacy_idx"`), + classifyCanonical(t, `ALTER TABLE "app"."orders" ADD COLUMN "region" text DEFAULT 'emea'`), + }) + diff.Disposition = routedD.Disposition + kinds := []schemadiff.ChangeKind{schemadiff.ChangeDropIndex, schemadiff.ChangeAddColumn} + for i, rs := range routedD.Statements { + ps := plan.FromRouted(rs) + ps.Kind = kinds[i] + diff.Statements = append(diff.Statements, ps) + } + diff.Fingerprint = plan.Fingerprint(diff.Statements) + + for i, want := range []plan.Report{alter, diff} { + raw, err := json.Marshal(want) + require.NoError(t, err) + assert.JSONEq(t, blocks[i][1], string(raw), + "docs/plan-report.md example %d drifted from pipeline output", i+1) + } +} + +// Every vocabulary value the contract closes over must be documented: a +// constant added to the code without a row in docs/plan-report.md (and a +// format_version decision) fails here. +func TestDocListsEveryVocabularyValue(t *testing.T) { + doc := readDoc(t) + var values []string + for _, s := range plan.Sources() { + values = append(values, string(s)) + } + for _, r := range planner.Routes() { + values = append(values, string(r)) + } + for _, r := range planner.Reasons() { + values = append(values, string(r)) + } + for _, b := range router.Backends() { + values = append(values, string(b)) + } + for _, d := range router.Dispositions() { + values = append(values, string(d)) + } + for _, k := range schemadiff.ChangeKinds() { + values = append(values, string(k)) + } + for _, v := range values { + assert.Contains(t, doc, fmt.Sprintf("| `%s` |", v), + "docs/plan-report.md is missing a vocabulary row for %q", v) + } +} diff --git a/pkg/plan/plan.go b/pkg/plan/plan.go index c5dd9d9..0052195 100644 --- a/pkg/plan/plan.go +++ b/pkg/plan/plan.go @@ -6,8 +6,13 @@ package plan import ( + "crypto/sha256" + "encoding/hex" + "hash" + "github.com/block/pg-sprite/pkg/planner" "github.com/block/pg-sprite/pkg/router" + "github.com/block/pg-sprite/pkg/schemadiff" ) // FormatVersion identifies the report contract. A consumer must reject a @@ -29,14 +34,33 @@ const ( SourceDiff Source = "diff" ) +// Sources returns the closed set of Source values. It is part of the +// plan-report contract (docs/plan-report.md): the set changes only with a +// format_version bump, and a consumer that meets an unrecognized value must +// treat the report as unknown and refuse it. +func Sources() []Source { + return []Source{SourceAlter, SourceDiff} +} + // Statement is one planned statement: the SQL, its classification, and // what execution would do with it now. type Statement struct { - // SQL is the submitted statement (alter) or the derived convergence - // statement (diff). + // SQL is the statement in the engine's canonical rendering: parsed and + // reprinted through the PostgreSQL deparser, whichever front door + // derived it. It is never a verbatim echo of the submitted text, so + // the same change carries the same string through either door. SQL string `json:"sql"` - // Destructive marks statements that drop live structure. - Destructive bool `json:"destructive,omitempty"` + // Kind classifies a diff-derived statement so a consumer can gate + // whole classes of change (see schemadiff.ChangeKind). Empty for the + // alter source: a submitted statement may carry several operations and + // has no single kind. + Kind schemadiff.ChangeKind `json:"kind,omitempty"` + // Destructive marks statements that discard live structure — a dropped + // column, constraint, or index. It is derived from the classifier's + // decisions, so both sources report it identically; it is always + // emitted, never omitted, because a safety flag a consumer gates on + // must be explicit even when false. + Destructive bool `json:"destructive"` // Route is the planner's aggregate route for the statement. Route planner.Route `json:"route"` // Backend is the assigned execution strategy; empty for refusals. @@ -63,6 +87,11 @@ type Report struct { // Table is the target table; empty when the statement has no single // table target (index maintenance). Table string `json:"table,omitempty"` + // ServerVersion is the PostgreSQL server_version the plan was derived + // against. Classification is version-sensitive, so a stored or + // forwarded report names the server whose rules produced it; empty + // only for sources that never connected. + ServerVersion string `json:"server_version,omitempty"` // TableExists reports whether the live table was found. It is set // only by sources that introspect for existence (diff); when false, // the statements are the full desired schema. @@ -70,6 +99,12 @@ type Report struct { // Disposition is the aggregate disposition across all statements: // what would happen if the engine executed this plan now. Disposition router.Disposition `json:"disposition"` + // Fingerprint is the plan's stable identity (see Fingerprint). An + // approver pins it when the plan is reviewed; an executor recomputes it + // at apply time and refuses on mismatch — that is how "the plan a + // reviewer approves is the plan that executes" is enforced across + // storage and forwarding. + Fingerprint string `json:"fingerprint"` // Statements is the ordered plan; empty means there is nothing to do. Statements []Statement `json:"statements"` } @@ -85,11 +120,12 @@ func NewReport(source Source) Report { } } -// FromRouted converts one routed statement into a plan statement. Fields -// the router does not know (Destructive) stay at their zero value for the -// caller to set. +// FromRouted converts one routed statement into a plan statement. +// Destructive is derived from the classifier's decisions — one destructive +// operation makes the statement destructive — so every source that routes +// through the planner reports it identically by construction. func FromRouted(rs router.Statement) Statement { - return Statement{ + st := Statement{ SQL: rs.Statement, Route: rs.Route, Backend: rs.Backend, @@ -97,4 +133,41 @@ func FromRouted(rs router.Statement) Statement { Decisions: rs.Decisions, ExecSQL: rs.ExecSQL, } + for _, d := range rs.Decisions { + if d.Destructive { + st.Destructive = true + break + } + } + return st +} + +// Fingerprint computes the plan's stable identity: "sha256:" plus the hex +// digest over what would execute — each statement's canonical SQL, route, +// backend, disposition, and exec_sql, in plan order. Explanatory fields +// (decisions, kind, destructive) are excluded, so a reworded reason does +// not change identity but a rerouted or resequenced plan does. The exact +// serialization is part of the contract (docs/plan-report.md) and changes +// only with a format_version bump. This is a plan identity, not a schema +// fingerprint: it never participates in schema-state comparison. +func Fingerprint(statements []Statement) string { + h := sha256.New() + for _, st := range statements { + writeField(h, st.SQL) + writeField(h, string(st.Route)) + writeField(h, string(st.Backend)) + writeField(h, string(st.Disposition)) + for _, sql := range st.ExecSQL { + writeField(h, sql) + } + h.Write([]byte{0x1e}) // record separator: one per statement + } + return "sha256:" + hex.EncodeToString(h.Sum(nil)) +} + +// writeField hashes one field with a trailing unit separator, so adjacent +// fields can never collide by concatenation. +func writeField(h hash.Hash, field string) { + h.Write([]byte(field)) + h.Write([]byte{0x1f}) } diff --git a/pkg/plan/plan_test.go b/pkg/plan/plan_test.go index 4a46744..c4f6478 100644 --- a/pkg/plan/plan_test.go +++ b/pkg/plan/plan_test.go @@ -2,6 +2,7 @@ package plan_test import ( "encoding/json" + "fmt" "testing" "github.com/stretchr/testify/assert" @@ -10,6 +11,7 @@ import ( "github.com/block/pg-sprite/pkg/plan" "github.com/block/pg-sprite/pkg/planner" "github.com/block/pg-sprite/pkg/router" + "github.com/block/pg-sprite/pkg/schemadiff" ) func TestNewReportStampsVersionAndEmptyStatements(t *testing.T) { @@ -43,7 +45,27 @@ func TestFromRoutedMapsEveryRoutedField(t *testing.T) { assert.Equal(t, router.DispositionExecute, st.Disposition) assert.Equal(t, rs.Decisions, st.Decisions) assert.Equal(t, rs.ExecSQL, st.ExecSQL) - assert.False(t, st.Destructive, "the router does not know destructiveness") + assert.False(t, st.Destructive, "no destructive decision means a non-destructive statement") +} + +// Destructive is derived from the classifier's decisions — one destructive +// operation makes the whole statement destructive — so both front doors +// report it identically by construction, whichever one built the report. +func TestFromRoutedDerivesDestructiveFromDecisions(t *testing.T) { + rs := router.Statement{ + Plan: planner.Plan{ + Statement: "ALTER TABLE t ADD COLUMN c int, DROP COLUMN doomed", + Route: planner.RouteNative, + Decisions: []planner.Decision{ + {Operation: "ADD COLUMN c", Route: planner.RouteNative, Reason: planner.ReasonMetadataOnly}, + {Operation: "DROP COLUMN doomed", Destructive: true, Route: planner.RouteNative, Reason: planner.ReasonMetadataOnly}, + }, + }, + Backend: router.BackendNative, + Disposition: router.DispositionExecute, + } + assert.True(t, plan.FromRouted(rs).Destructive, + "one destructive decision makes the statement destructive") } // The JSON shape is the adapter-facing contract: exact keys, exact @@ -55,19 +77,22 @@ func TestReportJSONShape(t *testing.T) { Source: plan.SourceDiff, Schema: "public", Table: "t", + ServerVersion: "16.4", TableExists: &exists, Disposition: router.DispositionRefuse, Statements: []plan.Statement{ { SQL: "DROP INDEX t_c_idx", + Kind: schemadiff.ChangeDropIndex, Destructive: true, Route: planner.RouteNative, Backend: router.BackendNative, Disposition: router.DispositionExecute, Decisions: []planner.Decision{{ - Operation: "drop index", - Route: planner.RouteNative, - Reason: planner.ReasonSaferIdiom, + Operation: "drop index", + Destructive: true, + Route: planner.RouteNative, + Reason: planner.ReasonSaferIdiom, }}, ExecSQL: []string{"DROP INDEX CONCURRENTLY t_c_idx"}, }, @@ -83,51 +108,111 @@ func TestReportJSONShape(t *testing.T) { }, }, } + r.Fingerprint = plan.Fingerprint(r.Statements) raw, err := json.Marshal(r) require.NoError(t, err) - assert.JSONEq(t, `{ + assert.JSONEq(t, fmt.Sprintf(`{ "format_version": 1, "source": "diff", "schema": "public", "table": "t", + "server_version": "16.4", "table_exists": true, "disposition": "refuse", + "fingerprint": %q, "statements": [ { "sql": "DROP INDEX t_c_idx", + "kind": "drop-index", "destructive": true, "route": "native", "backend": "native", "disposition": "execute", "decisions": [ - {"operation": "drop index", "route": "native", "reason": "safer-idiom"} + {"operation": "drop index", "destructive": true, "route": "native", "reason": "safer-idiom"} ], "exec_sql": ["DROP INDEX CONCURRENTLY t_c_idx"] }, { "sql": "ALTER TABLE t NO SUCH THING", + "destructive": false, "route": "refuse", "disposition": "refuse", "decisions": [ - {"operation": "unrecognized", "route": "refuse", "reason": "unsupported-operation"} + {"operation": "unrecognized", "destructive": false, "route": "refuse", "reason": "unsupported-operation"} ] } ] - }`, string(raw)) + }`, r.Fingerprint), string(raw)) } // Optional envelope fields are omitted, not emitted as zero values: an // alter-source report has no table_exists, and an empty plan serializes -// its statements as []. +// its statements as []. The fingerprint is never optional — an empty plan +// still has a defined identity. func TestReportJSONOmitsUnsetOptionalFields(t *testing.T) { r := plan.NewReport(plan.SourceAlter) r.Disposition = router.DispositionExecute + r.Fingerprint = plan.Fingerprint(r.Statements) raw, err := json.Marshal(r) require.NoError(t, err) assert.JSONEq(t, `{ "format_version": 1, "source": "alter", "disposition": "execute", + "fingerprint": "sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", "statements": [] }`, string(raw)) } + +// The fingerprint serialization is a pinned contract: a fixed statement +// list must hash to this exact digest. If this test fails, the identity +// definition changed and format_version must be bumped. +func TestFingerprintPinnedDigest(t *testing.T) { + st := plan.Statement{ + SQL: "ALTER TABLE t DROP COLUMN doomed", + Route: planner.RouteNative, + Backend: router.BackendNative, + Disposition: router.DispositionExecute, + } + assert.Equal(t, + "sha256:ec28ea60dfb1894212aa7dbe52ee355ec44b5abd94d0dac550c8e1e64dd76da0", + plan.Fingerprint([]plan.Statement{st})) +} + +// The fingerprint covers what would execute and only that: explanatory +// fields do not change identity; routing, order, and exec_sql do. +func TestFingerprintCoversExecutionNotExplanation(t *testing.T) { + a := plan.Statement{SQL: "ALTER TABLE t ADD COLUMN c int", Route: planner.RouteNative, + Backend: router.BackendNative, Disposition: router.DispositionExecute} + b := plan.Statement{SQL: "ALTER TABLE t DROP COLUMN doomed", Route: planner.RouteNative, + Backend: router.BackendNative, Disposition: router.DispositionExecute} + base := plan.Fingerprint([]plan.Statement{a, b}) + + explained := a + explained.Destructive = true + explained.Kind = schemadiff.ChangeAddColumn + explained.Decisions = []planner.Decision{{Operation: "ADD COLUMN c", Route: planner.RouteNative}} + assert.Equal(t, base, plan.Fingerprint([]plan.Statement{explained, b}), + "decisions, kind, and destructive are explanatory: identity unchanged") + + assert.NotEqual(t, base, plan.Fingerprint([]plan.Statement{b, a}), + "statement order is part of identity") + + rerouted := a + rerouted.Route = planner.RouteCopyAndSwap + rerouted.Backend = router.BackendCopyAndSwap + assert.NotEqual(t, base, plan.Fingerprint([]plan.Statement{rerouted, b}), + "a rerouted statement is a different plan") + + rewritten := a + rewritten.ExecSQL = []string{"CREATE INDEX CONCURRENTLY i ON t (c)"} + assert.NotEqual(t, base, plan.Fingerprint([]plan.Statement{rewritten, b}), + "exec_sql is what runs: identity changes with it") +} + +// Sources is the closed vocabulary a consumer branches on; the set is +// pinned to format_version 1. +func TestSourcesVocabularyPinned(t *testing.T) { + assert.Equal(t, []plan.Source{plan.SourceAlter, plan.SourceDiff}, plan.Sources()) +} diff --git a/pkg/planner/planner.go b/pkg/planner/planner.go index 6d34abf..46c635c 100644 --- a/pkg/planner/planner.go +++ b/pkg/planner/planner.go @@ -49,6 +49,34 @@ const ( RouteRefuse Route = "refuse" ) +// Routes returns the closed set of Route values, in severity order. It is +// part of the plan-report contract (docs/plan-report.md): the set changes +// only with a format_version bump, and a consumer that meets an +// unrecognized value must treat the statement as unknown and refuse it. +func Routes() []Route { + return []Route{RouteNative, RouteCopyAndSwap, RouteRefuse} +} + +// Reasons returns the closed set of Reason values. It is part of the +// plan-report contract (docs/plan-report.md): the set changes only with a +// format_version bump, and a consumer that meets an unrecognized value must +// treat the decision as unknown and refuse it. +func Reasons() []Reason { + return []Reason{ + ReasonMetadataOnly, + ReasonOnlineIdiom, + ReasonFastDefault, + ReasonBinaryCoercible, + ReasonSaferIdiom, + ReasonVolatileDefault, + ReasonGeneratedStored, + ReasonTypeRewrite, + ReasonRelocation, + ReasonPartitionParentLock, + ReasonUnsupportedOperation, + } +} + // worse orders routes for aggregation: refuse > copy-and-swap > native. func worse(a, b Route) Route { rank := map[Route]int{RouteNative: 0, RouteCopyAndSwap: 1, RouteRefuse: 2} @@ -104,6 +132,13 @@ const ( type Decision struct { // Operation is the operator-facing label (display only). Operation string `json:"operation"` + // Destructive marks operations that discard live structure — a dropped + // column, constraint, or index. It is derived from the operation shape + // here, in the one place every front door shares, so a plan reports the + // same statement as destructive no matter how it was submitted. It is + // always emitted, never omitted: a safety flag a consumer gates on must + // be explicit even when false. + Destructive bool `json:"destructive"` // Route is where the operation goes. Route Route `json:"route"` // Reason is why. @@ -178,7 +213,7 @@ func Classify(sql string, facts Facts) (Plan, error) { // classifyOp routes one operation per the reference table. func classifyOp(op statement.Op, st statement.Statement, facts Facts, sql string, single bool) Decision { - d := Decision{Operation: op.Describe()} + d := Decision{Operation: op.Describe(), Destructive: destructiveOp(op.Kind)} switch op.Kind { case statement.OpCreateTable: if op.PartitionOf { @@ -258,6 +293,20 @@ func classifyOp(op statement.Op, st statement.Statement, facts Facts, sql string return d } +// destructiveOp reports whether an operation shape discards live +// structure. A drop is destructive regardless of how it routes: a dropped +// column discards data, a dropped constraint discards a guarantee the +// schema was providing, and a dropped index discards a structure that is +// expensive to rebuild (and, for a unique index, the uniqueness guarantee). +func destructiveOp(kind statement.OpKind) bool { + switch kind { + case statement.OpDropColumn, statement.OpDropConstraint, statement.OpDropIndex: + return true + default: + return false + } +} + // concurrentlyDecision routes an operation that is online in its // CONCURRENTLY form: already concurrent is the idiom; otherwise native with // the concurrent rewrite as the safer sequence. diff --git a/pkg/planner/planner_test.go b/pkg/planner/planner_test.go index 7dc2593..be55f2c 100644 --- a/pkg/planner/planner_test.go +++ b/pkg/planner/planner_test.go @@ -31,6 +31,33 @@ func classifyOne(t *testing.T, sql string) planner.Decision { return plan.Decisions[0] } +// Destructive is a decision-level fact derived from the operation shape: +// drops of columns, constraints, and indexes discard live structure, and +// every front door that routes through the classifier inherits the same +// marking — including DROP INDEX, whose drop discards the index's +// guarantee (uniqueness, for a unique index) however it is submitted. +func TestClassifyMarksDropsDestructive(t *testing.T) { + destructive := []string{ + "ALTER TABLE t DROP COLUMN age", + "ALTER TABLE t DROP CONSTRAINT t_age_check", + "DROP INDEX t_v_idx", + "DROP INDEX CONCURRENTLY t_v_idx", + } + for _, sql := range destructive { + assert.True(t, classifyOne(t, sql).Destructive, sql) + } + nonDestructive := []string{ + "ALTER TABLE t ADD COLUMN age int", + "ALTER TABLE t ALTER COLUMN v50 TYPE varchar(100)", + "ALTER TABLE t ALTER COLUMN age DROP DEFAULT", + "ALTER TABLE t ALTER COLUMN age DROP NOT NULL", + "CREATE INDEX t_v_idx ON t (v50)", + } + for _, sql := range nonDestructive { + assert.False(t, classifyOne(t, sql).Destructive, sql) + } +} + // TestClassifyReferenceRows is the golden mapping: one case per row of // docs/postgres-online-ddl-reference.md. saferSteps is the length of the // expected safer sequence (0 when the decision carries none). @@ -222,3 +249,30 @@ func TestClassifyGeneratedNamesFitIdentifierLimit(t *testing.T) { dA2 := classifyOne(t, fmt.Sprintf("ALTER TABLE %s ALTER COLUMN %s SET NOT NULL", table, colA)) assert.Equal(t, dA.SaferSQL, dA2.SaferSQL) } + +// The route and reason vocabularies are closed sets a consumer branches on; +// both are pinned to plan-report format_version 1 (docs/plan-report.md). A +// new value here without a format_version bump is a contract break. +func TestRoutesVocabularyPinned(t *testing.T) { + assert.Equal(t, []planner.Route{ + planner.RouteNative, + planner.RouteCopyAndSwap, + planner.RouteRefuse, + }, planner.Routes()) +} + +func TestReasonsVocabularyPinned(t *testing.T) { + assert.Equal(t, []planner.Reason{ + planner.ReasonMetadataOnly, + planner.ReasonOnlineIdiom, + planner.ReasonFastDefault, + planner.ReasonBinaryCoercible, + planner.ReasonSaferIdiom, + planner.ReasonVolatileDefault, + planner.ReasonGeneratedStored, + planner.ReasonTypeRewrite, + planner.ReasonRelocation, + planner.ReasonPartitionParentLock, + planner.ReasonUnsupportedOperation, + }, planner.Reasons()) +} diff --git a/pkg/router/router.go b/pkg/router/router.go index a5a721f..38c082e 100644 --- a/pkg/router/router.go +++ b/pkg/router/router.go @@ -26,6 +26,14 @@ const ( BackendCopyAndSwap Backend = "copy-and-swap" ) +// Backends returns the closed set of Backend values. It is part of the +// plan-report contract (docs/plan-report.md): the set changes only with a +// format_version bump, and a consumer that meets an unrecognized value must +// treat the statement as unknown and refuse it. +func Backends() []Backend { + return []Backend{BackendNative, BackendCopyAndSwap} +} + // available reports whether the backend is implemented in this build. This // is the routing policy for backends: copy-and-swap is a known strategy the // planner routes to, but until its executor exists the router marks the @@ -57,6 +65,19 @@ const ( DispositionRefuse Disposition = "refuse" ) +// Dispositions returns the closed set of Disposition values, in severity +// order. It is part of the plan-report contract (docs/plan-report.md): the +// set changes only with a format_version bump, and a consumer that meets an +// unrecognized value must treat the statement as unknown and refuse it. +func Dispositions() []Disposition { + return []Disposition{ + DispositionExecute, + DispositionRewriteRequired, + DispositionUnavailable, + DispositionRefuse, + } +} + // worse orders dispositions for aggregation: // refuse > unavailable > rewrite-required > execute. func worse(a, b Disposition) Disposition { diff --git a/pkg/router/router_test.go b/pkg/router/router_test.go index 6531d4a..92b0e58 100644 --- a/pkg/router/router_test.go +++ b/pkg/router/router_test.go @@ -146,3 +146,23 @@ func TestRouteEmptyPlanExecutes(t *testing.T) { assert.Equal(t, router.DispositionExecute, routed.Disposition, "a plan with nothing to do has nothing blocking execution") } + +// The backend and disposition vocabularies are closed sets a consumer +// branches on; both are pinned to plan-report format_version 1 +// (docs/plan-report.md). A new value here without a format_version bump is +// a contract break. +func TestBackendsVocabularyPinned(t *testing.T) { + assert.Equal(t, []router.Backend{ + router.BackendNative, + router.BackendCopyAndSwap, + }, router.Backends()) +} + +func TestDispositionsVocabularyPinned(t *testing.T) { + assert.Equal(t, []router.Disposition{ + router.DispositionExecute, + router.DispositionRewriteRequired, + router.DispositionUnavailable, + router.DispositionRefuse, + }, router.Dispositions()) +} diff --git a/pkg/schemadiff/diff.go b/pkg/schemadiff/diff.go index 896d636..2d06f4e 100644 --- a/pkg/schemadiff/diff.go +++ b/pkg/schemadiff/diff.go @@ -51,6 +51,27 @@ const ( ChangeCreateIndex ChangeKind = "create-index" ) +// ChangeKinds returns the closed set of ChangeKind values. It is part of +// the plan-report contract (docs/plan-report.md): the set changes only with +// a format_version bump, and a consumer that meets an unrecognized value +// must treat the statement as unknown and refuse it. +func ChangeKinds() []ChangeKind { + return []ChangeKind{ + ChangeCreateTable, + ChangeDropIndex, + ChangeDropConstraint, + ChangeDropColumn, + ChangeAddColumn, + ChangeAlterType, + ChangeSetDefault, + ChangeDropDefault, + ChangeSetNotNull, + ChangeDropNotNull, + ChangeAddConstraint, + ChangeCreateIndex, + } +} + // Change is one derived statement of the ordered plan. type Change struct { // SQL is the literal statement, without a trailing semicolon. diff --git a/pkg/schemadiff/diff_test.go b/pkg/schemadiff/diff_test.go index 7617153..71b92ed 100644 --- a/pkg/schemadiff/diff_test.go +++ b/pkg/schemadiff/diff_test.go @@ -263,3 +263,23 @@ func TestDiffGeneratedColumnAddRendersGenerationExpression(t *testing.T) { `ALTER TABLE "public"."events" ADD COLUMN "name_upper" text GENERATED ALWAYS AS (upper((name)::text)) STORED`, }, sqls(changes)) } + +// The change-kind vocabulary is a closed set a consumer branches on; it is +// pinned to plan-report format_version 1 (docs/plan-report.md). A new value +// here without a format_version bump is a contract break. +func TestChangeKindsVocabularyPinned(t *testing.T) { + assert.Equal(t, []ChangeKind{ + ChangeCreateTable, + ChangeDropIndex, + ChangeDropConstraint, + ChangeDropColumn, + ChangeAddColumn, + ChangeAlterType, + ChangeSetDefault, + ChangeDropDefault, + ChangeSetNotNull, + ChangeDropNotNull, + ChangeAddConstraint, + ChangeCreateIndex, + }, ChangeKinds()) +} diff --git a/pkg/statement/ops.go b/pkg/statement/ops.go index f28a8b2..1277d62 100644 --- a/pkg/statement/ops.go +++ b/pkg/statement/ops.go @@ -2,6 +2,7 @@ package statement import ( "fmt" + "strconv" "strings" pganalyze "github.com/pganalyze/pg_query_go/v6" @@ -136,7 +137,7 @@ func (o Op) Describe() string { case OpDropColumn: return "DROP COLUMN " + o.Column case OpAlterColumnType: - return "ALTER COLUMN " + o.Column + " TYPE " + o.NewType + return "ALTER COLUMN " + o.Column + " TYPE " + formatType(o.NewType, o.NewTypeMods) case OpSetDefault: return "ALTER COLUMN " + o.Column + " SET DEFAULT" case OpDropDefault: @@ -170,16 +171,49 @@ func (o Op) Describe() string { case OpDetachPartition: return "DETACH PARTITION" case OpCreateIndex: - return "CREATE INDEX " + o.Name + // An unnamed CREATE INDEX (the server auto-names it) labels + // without a trailing name; TrimSpace keeps the label clean. + return strings.TrimSpace("CREATE INDEX " + o.Name) case OpDropIndex: - return "DROP INDEX " + o.Name + return strings.TrimSpace("DROP INDEX " + o.Name) case OpReindex: - return "REINDEX " + o.Name + return strings.TrimSpace("REINDEX " + o.Name) default: return "unrecognized operation" } } +// dropIndexNames renders the dropped index names for the operation label: +// each object's qualified name, comma-separated when one statement drops +// several. The name identifies which structure the plan discards, so a +// label without it would leave a destructive decision anonymous. +func dropIndexNames(drop *pganalyze.DropStmt) string { + names := make([]string, 0, len(drop.GetObjects())) + for _, obj := range drop.GetObjects() { + parts := make([]string, 0, len(obj.GetList().GetItems())) + for _, item := range obj.GetList().GetItems() { + parts = append(parts, item.GetString_().GetSval()) + } + names = append(names, strings.Join(parts, ".")) + } + return strings.Join(names, ", ") +} + +// formatType renders a type name with its modifiers — varchar(50), +// numeric(12,2) — exactly as the grammar spells the target type. The +// modifier is what distinguishes a widen from a narrow, so a rendering +// that drops it would collapse changes that route in opposite directions. +func formatType(name string, mods []int32) string { + if len(mods) == 0 { + return name + } + parts := make([]string, len(mods)) + for i, m := range mods { + parts[i] = strconv.FormatInt(int64(m), 10) + } + return name + "(" + strings.Join(parts, ",") + ")" +} + // ParseOps parses one SQL statement and returns its typed operations. An // ALTER TABLE yields one Op per subcommand; every other supported statement // yields exactly one. Statements and subcommands the engine does not @@ -223,7 +257,7 @@ func ParseOps(sql string) ([]Op, error) { if drop.GetRemoveType() != pganalyze.ObjectType_OBJECT_INDEX { return []Op{{Kind: OpUnrecognized}}, nil } - return []Op{{Kind: OpDropIndex, Concurrent: drop.GetConcurrent()}}, nil + return []Op{{Kind: OpDropIndex, Name: dropIndexNames(drop), Concurrent: drop.GetConcurrent()}}, nil case node.GetReindexStmt() != nil: re := node.GetReindexStmt() return []Op{{ diff --git a/pkg/statement/ops_test.go b/pkg/statement/ops_test.go index cadba02..fb146d8 100644 --- a/pkg/statement/ops_test.go +++ b/pkg/statement/ops_test.go @@ -228,12 +228,12 @@ func TestParseOpsShapes(t *testing.T) { { name: "drop index", sql: "DROP INDEX i", - want: statement.Op{Kind: statement.OpDropIndex}, + want: statement.Op{Kind: statement.OpDropIndex, Name: "i"}, }, { name: "drop index concurrently", sql: "DROP INDEX CONCURRENTLY i", - want: statement.Op{Kind: statement.OpDropIndex, Concurrent: true}, + want: statement.Op{Kind: statement.OpDropIndex, Name: "i", Concurrent: true}, }, { name: "reindex", @@ -268,6 +268,24 @@ func TestParseOpsShapes(t *testing.T) { } } +// Describe must render the type modifier: the modifier is what +// distinguishes a widen (varchar(100), routes native) from a narrow +// (varchar(30), routes copy-and-swap), and a label that drops it would +// render changes that route in opposite directions identically. +func TestDescribeRendersTypeModifiers(t *testing.T) { + cases := []struct { + sql string + want string + }{ + {"ALTER TABLE t ALTER COLUMN v TYPE varchar(100)", "ALTER COLUMN v TYPE varchar(100)"}, + {"ALTER TABLE t ALTER COLUMN p TYPE numeric(12,2)", "ALTER COLUMN p TYPE numeric(12,2)"}, + {"ALTER TABLE t ALTER COLUMN v TYPE text", "ALTER COLUMN v TYPE text"}, + } + for _, tc := range cases { + assert.Equal(t, tc.want, parseOneOp(t, tc.sql).Describe(), tc.sql) + } +} + func TestParseOpsMultipleSubcommands(t *testing.T) { ops, err := statement.ParseOps("ALTER TABLE t ADD COLUMN a int, DROP COLUMN b") require.NoError(t, err) @@ -329,3 +347,27 @@ func TestAddNotValidRefusals(t *testing.T) { }) } } + +// A drop-index operation names what it discards: the label carries the +// (qualified) index name so a destructive decision is never anonymous. +func TestParseOpsDropIndexCapturesName(t *testing.T) { + ops, err := statement.ParseOps("DROP INDEX app.orders_legacy_idx") + require.NoError(t, err) + require.Len(t, ops, 1) + assert.Equal(t, statement.OpDropIndex, ops[0].Kind) + assert.Equal(t, "app.orders_legacy_idx", ops[0].Name) + assert.Equal(t, "DROP INDEX app.orders_legacy_idx", ops[0].Describe()) + + multi, err := statement.ParseOps("DROP INDEX a_idx, b_idx") + require.NoError(t, err) + require.Len(t, multi, 1) + assert.Equal(t, "a_idx, b_idx", multi[0].Name) +} + +// An unnamed CREATE INDEX labels cleanly, without a dangling space. +func TestDescribeUnnamedCreateIndexHasNoTrailingSpace(t *testing.T) { + ops, err := statement.ParseOps("CREATE INDEX ON t (c)") + require.NoError(t, err) + require.Len(t, ops, 1) + assert.Equal(t, "CREATE INDEX", ops[0].Describe()) +} diff --git a/pkg/statement/statement.go b/pkg/statement/statement.go index 14d69df..7e04751 100644 --- a/pkg/statement/statement.go +++ b/pkg/statement/statement.go @@ -88,6 +88,25 @@ func (s Statement) Concurrent() bool { return s.concurrent } // exactly one SQL statement. var ErrNotOneStatement = errors.New("input must contain exactly one SQL statement") +// Canonical reprints one statement through the PostgreSQL deparser: +// grammar-canonical spelling, unnecessary quoting dropped. Commented input +// is refused (ErrCommentLoss): the parser drops comments, and reprinting +// must never silently discard content. It is the rendering the plan report +// carries, so both front doors describe the same change with the same +// string. Deparser output never feeds a model comparison or a schema +// fingerprint (see the package comment); the plan report's fingerprint is +// a plan identity, not a schema fingerprint. +func Canonical(sql string) (string, error) { + if err := CheckNoComments(sql); err != nil { + return "", err + } + node, err := parseSingle(sql) + if err != nil { + return "", err + } + return deparseOne(node) +} + // ParseOne parses sql with the PostgreSQL grammar and requires exactly one // statement. A parse failure is surfaced to the caller, never guessed around. func ParseOne(sql string) (Statement, error) { diff --git a/pkg/statement/statement_test.go b/pkg/statement/statement_test.go index 37743f9..1669dc1 100644 --- a/pkg/statement/statement_test.go +++ b/pkg/statement/statement_test.go @@ -175,3 +175,36 @@ func TestKindString(t *testing.T) { assert.Equal(t, "REINDEX", KindReindex.String()) assert.Equal(t, "other", KindOther.String()) } + +// Canonical is the report's one rendering per change: the diff door's +// quoted generation and the alter door's hand-written text converge on the +// same string, so a consumer hashing or displaying report SQL sees one +// spelling regardless of front door. +func TestCanonicalConvergesQuotingAcrossFrontDoors(t *testing.T) { + generated, err := Canonical(`ALTER TABLE "t_1"."t" DROP COLUMN "doomed"`) + require.NoError(t, err) + submitted, err := Canonical("ALTER TABLE t_1.t DROP COLUMN doomed") + require.NoError(t, err) + assert.Equal(t, generated, submitted) + + // Identifiers that need quoting keep it. + kept, err := Canonical(`ALTER TABLE "Mixed Case" DROP COLUMN c`) + require.NoError(t, err) + assert.Contains(t, kept, `"Mixed Case"`) +} + +func TestCanonicalRefusesNotExactlyOneStatement(t *testing.T) { + _, err := Canonical("ALTER TABLE t DROP COLUMN a; ALTER TABLE t DROP COLUMN b") + require.ErrorIs(t, err, ErrNotOneStatement) + _, err = Canonical("not sql") + require.Error(t, err) +} + +// Reprinting through the deparser drops comments, so Canonical refuses +// commented input rather than silently discarding content. +func TestCanonicalRefusesCommentedInput(t *testing.T) { + _, err := Canonical("ALTER TABLE t DROP COLUMN a -- doomed") + require.ErrorIs(t, err, ErrCommentLoss) + _, err = Canonical("ALTER TABLE t /* keep */ DROP COLUMN a") + require.ErrorIs(t, err, ErrCommentLoss) +}