From 1b92832a5fe19f67ac4198da97c9c81711f22b95 Mon Sep 17 00:00:00 2001 From: Armand Parajon Date: Tue, 18 Aug 2026 20:57:05 +0800 Subject: [PATCH 1/3] feat(vitess): treat VSchema deletions as unsafe changes Removing a vindex, a table routing entry, or a column-vindex association changes Vitess query routing the moment the VSchema is applied: the vindex stops being used for routing, lookup rows stop being maintained, and queries that depended on it can fail or scatter. That is as dangerous as destructive DDL, but VSchema changes previously bypassed the unsafe gate entirely. The engine now compares the current and desired VSchema structurally (pkg/vschema Deletions, via the Vitess keyspace proto) and records any removals in plan change metadata. UnsafeChanges() surfaces them alongside unsafe table changes, so the existing PR comment and CLI allow-unsafe opt-in gates apply. Detection fails closed: an unparseable VSchema fails the plan, and undecodable deletion metadata reports an unsafe change rather than passing silently. Additions-only VSchema changes remain safe. The CLI apply gate now keys on UnsafeChanges() rather than lint error severity, matching the webhook gates. This closes two CLI gaps: VSchema removals carry no lint finding, and the DROP-fallback unsafe classification never consulted lint either. The plan comment appends namespace-level VSchema removals to the Issues section in the sharded view too, where the per-shard unsafe derivation would otherwise omit them. A new preview scenario (Vitess plan with an unsafe VSchema removal) renders the section in TEMPLATES.md. Co-Authored-By: Claude Fable 5 --- TEMPLATES.md | 58 +++++ docs/lint-and-safety-levels.md | 12 + docs/namespaces.md | 5 + e2e/local/vitess_test.go | 46 ++++ pkg/apitypes/apitypes.go | 8 +- pkg/apitypes/vschema.go | 79 ++++++- pkg/apitypes/vschema_test.go | 73 ++++++ pkg/cmd/commands/apply.go | 4 +- pkg/cmd/commands/rollback.go | 5 +- pkg/cmd/internal/templates/preview.go | 1 + pkg/cmd/internal/templates/preview_comment.go | 1 + .../internal/templates/preview_dispatch.go | 2 + pkg/engine/planetscale/plan.go | 29 +++ pkg/engine/planetscale/plan_vschema_test.go | 39 ++++ pkg/vschema/deletions.go | 134 +++++++++++ pkg/vschema/deletions_test.go | 218 ++++++++++++++++++ pkg/webhook/plan.go | 44 +++- pkg/webhook/templates/preview.go | 59 +++++ 18 files changed, 795 insertions(+), 22 deletions(-) create mode 100644 pkg/apitypes/vschema_test.go create mode 100644 pkg/engine/planetscale/plan_vschema_test.go create mode 100644 pkg/vschema/deletions.go create mode 100644 pkg/vschema/deletions_test.go diff --git a/TEMPLATES.md b/TEMPLATES.md index 8ae908346..a625a538e 100644 --- a/TEMPLATES.md +++ b/TEMPLATES.md @@ -506,6 +506,64 @@ CREATE TABLE `addresses` ( ๐Ÿ“‹ **Plan**: **2** tables to create, **2** vschema updates +--- + +โ–ถ๏ธ **To apply** all schema changes from this PR, comment: +``` +schemabot apply -e staging +``` + + + +
+Vitess Plan: VSchema Removal (Unsafe) + + +## Schema Change Plan โ€” Staging + +**Database**: `commerce` | **Type**: `Vitess` + +*Requested by @jackjackbits at 2026-01-01 00:00:00 UTC ยท planned from [`abcdef1`](https://github.com/block/schemabot/commit/abcdef1234567890abcdef1234567890abcdef12)* + +#### Keyspace: `commerce_sharded` +#### VSchema +```diff +--- a/commerce_sharded.json ++++ b/commerce_sharded.json +@@ -3,10 +3,6 @@ + "hash": { + "type": "hash" +- }, +- "customers_email_lookup": { +- "type": "consistent_lookup_unique", +- "params": { +- "table": "customers_email_lookup", +- "from": "email", +- "to": "keyspace_id" +- }, +- "owner": "customers" + } + }, +@@ -18,8 +14,4 @@ + { + "column": "id", + "name": "hash" +- }, +- { +- "column": "email", +- "name": "customers_email_lookup" + } + ] + } +``` + +โš ๏ธ **Issues**: **2** unsafe changes detected +- `commerce_sharded/vschema.json`: lookup vindex `customers_email_lookup` is removed: Vitess immediately stops maintaining its rows in backing table `customers_email_lookup`, queries routed through it can fail or scatter, and the lookup data goes stale +- `commerce_sharded/vschema.json`: table `customers` no longer uses vindex `customers_email_lookup`: routing for queries on its columns changes immediately and lookup rows stop being maintained + +๐Ÿ“‹ **Plan**: **1** vschema update + + --- โ–ถ๏ธ **To apply** all schema changes from this PR, comment: diff --git a/docs/lint-and-safety-levels.md b/docs/lint-and-safety-levels.md index 98f1f35d6..c6648c4fe 100644 --- a/docs/lint-and-safety-levels.md +++ b/docs/lint-and-safety-levels.md @@ -74,6 +74,18 @@ classified unsafe when any of the following hold: - a lint rule raised it at error severity (for example dropping an index that was never made invisible). +A Vitess VSchema change is unsafe when it removes anything from the current +VSchema: a vindex definition, a table's routing entry, or a table's +column-vindex association. A removal changes query routing the moment the +VSchema is applied โ€” Vitess stops using the removed vindex, a lookup vindex's +backing table stops being maintained and its rows go stale, and queries that +depended on the removed entry can fail or scatter. Deleting a vindex is as +dangerous as dropping a table, so it takes the same `--allow-unsafe` +acknowledgment. Additions-only VSchema changes (new vindexes, new tables, new +column-vindex associations) are not unsafe. Removals are detected structurally +by comparing the current and desired VSchema documents; a VSchema that cannot +be parsed fails the plan rather than skipping detection. + Unsafe does not mean broken. An unsafe change will usually apply successfully โ€” the point of the gate is that it is destructive or irreversible, so SchemaBot requires an explicit, auditable acknowledgment (`--allow-unsafe`) instead of diff --git a/docs/namespaces.md b/docs/namespaces.md index db41ed23a..e4b2c23dc 100644 --- a/docs/namespaces.md +++ b/docs/namespaces.md @@ -130,6 +130,11 @@ SchemaBot plans all keyspaces together โ€” a single plan can contain changes acr A plan can have DDL-only changes, VSchema-only changes, or both. +A VSchema change that removes a vindex, a table routing entry, or a table's +column-vindex association is an unsafe change and requires the same +`--allow-unsafe` acknowledgment as destructive DDL โ€” see +[lint-and-safety-levels.md](./lint-and-safety-levels.md#what-unsafe-means). + ## Where to Put the Schema Directory SchemaBot is location-agnostic: config discovery finds `schemabot.yaml` anywhere diff --git a/e2e/local/vitess_test.go b/e2e/local/vitess_test.go index 51cb571b6..52efe02fc 100644 --- a/e2e/local/vitess_test.go +++ b/e2e/local/vitess_test.go @@ -1160,6 +1160,52 @@ func TestVitess_Apply_DropIndex_BlockedWithoutFlag(t *testing.T) { assert.Contains(t, out, "Unsafe Changes Detected") } +// TestVitess_Apply_VSchemaVindexRemoval_BlockedWithoutFlag exercises the +// VSchema safety gate. Removing a vindex from a keyspace's vschema.json is an +// unsafe change: Vitess stops using the vindex for routing the moment the +// VSchema is applied, and queries that depended on it can fail or scatter. +// The plan must disclose the removal as unsafe, an apply without +// --allow-unsafe must be refused before anything starts, and an apply that +// acknowledges the removal with --allow-unsafe must complete. +func TestVitess_Apply_VSchemaVindexRemoval_BlockedWithoutFlag(t *testing.T) { + vitessAvailable(t) + vitessRestoreBaseSchema(t, "staging") + defer vitessRestoreBaseSchema(t, "staging") + binPath := buildCLI(t) + endpoint := schemabotURL(t) + + // Seed a live VSchema carrying an extra xxhash vindex directly via the + // admin endpoint, so planning the base schema (which does not define + // xxhash) produces a vindex removal and nothing else. + vschemaWithXxhash := `{"sharded":true,"vindexes":{"hash":{"type":"hash"},"xxhash":{"type":"xxhash"}},"tables":{"users":{"column_vindexes":[{"column":"id","name":"hash"}],"auto_increment":{"column":"id","sequence":"users_seq"}},"orders":{"column_vindexes":[{"column":"user_id","name":"hash"}],"auto_increment":{"column":"id","sequence":"orders_seq"}},"products":{"column_vindexes":[{"column":"id","name":"hash"}],"auto_increment":{"column":"id","sequence":"products_seq"}}}}` + seedBody := fmt.Sprintf(`{"org":%q,"database":%q,"keyspace":%q,"vschema":%s}`, + "localscale-staging", vitessDB, "testapp_sharded", vschemaWithXxhash) + _, err := localscaleAdminPost(t, "/admin/seed-vschema", seedBody) + require.NoError(t, err, "seed VSchema with extra vindex") + clearSchemaBotState(t) + + baseSchema := newVitessSchemaDir(t, vitessBaseSchema()) + + // The plan discloses the vindex removal as an unsafe change on the + // keyspace's vschema.json. + planOut := e2eutil.RunCLIInDir(t, binPath, baseSchema, "plan", + "-s", ".", "-e", "staging", "--endpoint", endpoint) + e2eutil.AssertContains(t, planOut, "Unsafe Changes Detected") + e2eutil.AssertContains(t, planOut, "xxhash") + e2eutil.AssertContains(t, planOut, "vschema.json") + + // Applying without --allow-unsafe is refused. + out, err := e2eutil.RunCLIWithErrorInDir(t, binPath, baseSchema, "apply", + "-s", ".", "-e", "staging", "--endpoint", endpoint, "-y", "-o", "log") + t.Logf("VSchema vindex removal apply output:\n%s", out) + require.Error(t, err, "expected apply to fail without --allow-unsafe") + assert.Contains(t, out, "Unsafe Changes Detected") + + // Acknowledging the removal with --allow-unsafe lets the apply proceed. + clearSchemaBotState(t) + vitessApplyAndWait(t, baseSchema, "staging") +} + func TestVitess_Apply_DropTable_WithVSchema(t *testing.T) { vitessAvailable(t) clearSchemaBotState(t) diff --git a/pkg/apitypes/apitypes.go b/pkg/apitypes/apitypes.go index 3cf5ff5fe..efbc1ea99 100644 --- a/pkg/apitypes/apitypes.go +++ b/pkg/apitypes/apitypes.go @@ -476,9 +476,10 @@ type UnsafeChange struct { ChangeType string } -// UnsafeChanges returns all table changes marked as unsafe across all -// namespaces. DROP table changes are treated as unsafe even when an engine omits -// IsUnsafe, so destructive table deletion fails closed. +// UnsafeChanges returns all changes marked as unsafe across all namespaces: +// unsafe table changes and VSchema removals. DROP table changes are treated as +// unsafe even when an engine omits IsUnsafe, so destructive table deletion +// fails closed. func (r *PlanResponse) UnsafeChanges() []UnsafeChange { if r == nil { return nil @@ -493,6 +494,7 @@ func (r *PlanResponse) UnsafeChanges() []UnsafeChange { result = append(result, unsafeChange) } } + result = append(result, sc.VSchemaUnsafeChanges()...) } return result } diff --git a/pkg/apitypes/vschema.go b/pkg/apitypes/vschema.go index 4794b131f..f41c57797 100644 --- a/pkg/apitypes/vschema.go +++ b/pkg/apitypes/vschema.go @@ -1,6 +1,13 @@ package apitypes -import "encoding/json" +import ( + "encoding/json" + "fmt" +) + +// VSchemaChangeType is the ChangeType recorded on unsafe changes that stem +// from a VSchema removal rather than table DDL. +const VSchemaChangeType = "vschema" // VSchemaChangesMetadataKey is the progress display-metadata key under which the // engine projects per-keyspace VSchema application state as a JSON-encoded @@ -16,6 +23,76 @@ const ( VSchemaChangedMetadataKey = "vschema_changed" ) +// VSchemaDeletionsMetadataKey is the plan change-metadata key under which +// engines record structural removals in a namespace's VSchema change as a +// JSON-encoded []VSchemaDeletion. A removal changes Vitess query routing the +// moment the VSchema is applied โ€” a deleted vindex stops being used for +// routing and lookups, and queries that depended on it can fail โ€” so any +// recorded deletion makes the plan's VSchema change an unsafe change requiring +// the same operator opt-in as destructive DDL. +const VSchemaDeletionsMetadataKey = "vschema_deletions" + +// VSchemaDeletion is one structural removal in a namespace's VSchema change. +// It mirrors the engine-side deletion type (pkg/vschema); apitypes keeps its +// own copy so this package stays dependency-free. +type VSchemaDeletion struct { + Kind string `json:"kind"` // "vindex", "table", or "column_vindex" + Name string `json:"name"` // vindex name, table name, or "table.vindex" + Reason string `json:"reason"` // operator-facing explanation of the risk +} + +// EncodeVSchemaDeletions marshals VSchema deletions for plan change metadata. +// Returns "" for an empty list so the metadata key is omitted. +func EncodeVSchemaDeletions(deletions []VSchemaDeletion) (string, error) { + if len(deletions) == 0 { + return "", nil + } + b, err := json.Marshal(deletions) + if err != nil { + return "", fmt.Errorf("encode %d VSchema deletions: %w", len(deletions), err) + } + return string(b), nil +} + +// ParseVSchemaDeletions decodes the VSchema deletions recorded in a +// namespace's plan change metadata. Returns nil when the change carries none. +func ParseVSchemaDeletions(metadata map[string]string) ([]VSchemaDeletion, error) { + raw := metadata[VSchemaDeletionsMetadataKey] + if raw == "" { + return nil, nil + } + var deletions []VSchemaDeletion + if err := json.Unmarshal([]byte(raw), &deletions); err != nil { + return nil, fmt.Errorf("decode VSchema deletions metadata: %w", err) + } + return deletions, nil +} + +// VSchemaUnsafeChanges returns the unsafe-change view of this namespace's +// recorded VSchema deletions. Metadata that cannot be decoded fails closed: +// the namespace reports a single unsafe change explaining that deletions are +// present but unreadable, so a corrupt record can never bypass the opt-in +// gate. +func (sc *SchemaChangeResponse) VSchemaUnsafeChanges() []UnsafeChange { + deletions, err := ParseVSchemaDeletions(sc.Metadata) + if err != nil { + return []UnsafeChange{{ + Table: sc.Namespace + "/vschema.json", + Reason: "VSchema deletions were recorded on this plan but could not be decoded; treating the VSchema change as unsafe", + ChangeType: VSchemaChangeType, + }} + } + result := make([]UnsafeChange, 0, len(deletions)) + for _, d := range deletions { + result = append(result, UnsafeChange{ + Table: sc.Namespace + "/vschema.json", + Reason: d.Reason, + ChangeType: VSchemaChangeType, + }) + } + return result +} + // HasVSchemaChange reports whether this namespace's change carries VSchema work. func (sc *SchemaChangeResponse) HasVSchemaChange() bool { return sc.Metadata[VSchemaDiffMetadataKey] != "" || sc.Metadata[VSchemaChangedMetadataKey] == "true" diff --git a/pkg/apitypes/vschema_test.go b/pkg/apitypes/vschema_test.go new file mode 100644 index 000000000..71f83ecfa --- /dev/null +++ b/pkg/apitypes/vschema_test.go @@ -0,0 +1,73 @@ +package apitypes + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// A VSchema change that removes a vindex, table routing entry, or +// column-vindex association gates the apply behind the unsafe opt-in, the +// same as destructive DDL: the removal changes Vitess query routing the +// moment it lands. +func TestPlanResponse_UnsafeChangesIncludesVSchemaDeletions(t *testing.T) { + meta, err := EncodeVSchemaDeletions([]VSchemaDeletion{ + {Kind: "vindex", Name: "email_lookup", Reason: `vindex "email_lookup" is removed: Vitess immediately stops using it`}, + {Kind: "column_vindex", Name: "users.email_lookup", Reason: `table "users" no longer uses vindex "email_lookup"`}, + }) + require.NoError(t, err) + + resp := &PlanResponse{ + Changes: []*SchemaChangeResponse{{ + Namespace: "commerce", + Metadata: map[string]string{ + VSchemaChangedMetadataKey: "true", + VSchemaDeletionsMetadataKey: meta, + }, + }}, + } + + changes := resp.UnsafeChanges() + require.Len(t, changes, 2) + assert.Equal(t, "commerce/vschema.json", changes[0].Table) + assert.Equal(t, VSchemaChangeType, changes[0].ChangeType) + assert.Contains(t, changes[0].Reason, "email_lookup") + assert.Contains(t, changes[1].Reason, `table "users"`) +} + +func TestPlanResponse_UnsafeChangesVSchemaAdditionsOnlyStaysSafe(t *testing.T) { + resp := &PlanResponse{ + Changes: []*SchemaChangeResponse{{ + Namespace: "commerce", + Metadata: map[string]string{ + VSchemaChangedMetadataKey: "true", + VSchemaDiffMetadataKey: "+ added lines only", + }, + }}, + } + assert.Empty(t, resp.UnsafeChanges()) +} + +func TestPlanResponse_UnsafeChangesUndecodableVSchemaDeletionsFailClosed(t *testing.T) { + resp := &PlanResponse{ + Changes: []*SchemaChangeResponse{{ + Namespace: "commerce", + Metadata: map[string]string{ + VSchemaChangedMetadataKey: "true", + VSchemaDeletionsMetadataKey: "{corrupt", + }, + }}, + } + + changes := resp.UnsafeChanges() + require.Len(t, changes, 1) + assert.Equal(t, "commerce/vschema.json", changes[0].Table) + assert.Contains(t, changes[0].Reason, "could not be decoded") +} + +func TestEncodeVSchemaDeletions_EmptyOmitsKey(t *testing.T) { + meta, err := EncodeVSchemaDeletions(nil) + require.NoError(t, err) + assert.Empty(t, meta) +} diff --git a/pkg/cmd/commands/apply.go b/pkg/cmd/commands/apply.go index a0923263a..a72c6c8ba 100644 --- a/pkg/cmd/commands/apply.go +++ b/pkg/cmd/commands/apply.go @@ -141,7 +141,7 @@ func (cmd *ApplyCmd) Run(g *Globals) error { } // Check for unsafe changes - if planResult.HasErrors() && !cmd.AllowUnsafe { + if len(planResult.UnsafeChanges()) > 0 && !cmd.AllowUnsafe { return blockUnsafeApply(planResult, cfg.Database, cmd.Environment, cfg.SchemaDir) } @@ -173,7 +173,7 @@ func (cmd *ApplyCmd) Run(g *Globals) error { OutputPlanResult(planResult, cfg.Database, cmd.Environment, cfg.SchemaDir, true) // Show unsafe warning if --allow-unsafe was used - if planResult.HasErrors() && cmd.AllowUnsafe { + if cmd.AllowUnsafe { templates.WriteUnsafeWarningAllowed(planResult.UnsafeChanges()) } diff --git a/pkg/cmd/commands/rollback.go b/pkg/cmd/commands/rollback.go index a56aba278..b3b3be5ba 100644 --- a/pkg/cmd/commands/rollback.go +++ b/pkg/cmd/commands/rollback.go @@ -99,10 +99,7 @@ func (cmd *RollbackCmd) Run(g *Globals) error { } // Show unsafe warning if any - if planResult.HasErrors() { - unsafeChanges := planResult.UnsafeChanges() - templates.WriteUnsafeWarningAllowed(unsafeChanges) - } + templates.WriteUnsafeWarningAllowed(planResult.UnsafeChanges()) // Show options if any flags are set templates.WriteOptions(cmd.DeferCutover, false) diff --git a/pkg/cmd/internal/templates/preview.go b/pkg/cmd/internal/templates/preview.go index 95b9e8fa7..c65cc7ad5 100644 --- a/pkg/cmd/internal/templates/preview.go +++ b/pkg/cmd/internal/templates/preview.go @@ -125,6 +125,7 @@ const ( PreviewCommentMultiEnvDiff PreviewType = "comment_multi_env_diff" // Multi-env plan (different per env) PreviewCommentMultiEnvLint PreviewType = "comment_multi_env_lint" // Multi-env plan with lint violations PreviewCommentVitessPlan PreviewType = "comment_vitess_plan" // Vitess plan with keyspaces + VSchema + PreviewCommentVitessPlanVSchemaRemoval PreviewType = "comment_vitess_plan_vschema_removal" // Vitess plan with unsafe VSchema removals PreviewCommentVitessApplyPlan PreviewType = "comment_vitess_apply_plan" // Locked Vitess apply-plan with options PreviewCommentMySQLMultiSchema PreviewType = "comment_mysql_multi_schema" // MySQL plan with multiple schema names PreviewCommentHelp PreviewType = "comment_help" // Help command reference comment diff --git a/pkg/cmd/internal/templates/preview_comment.go b/pkg/cmd/internal/templates/preview_comment.go index e3b40f897..acc1e6d63 100644 --- a/pkg/cmd/internal/templates/preview_comment.go +++ b/pkg/cmd/internal/templates/preview_comment.go @@ -146,6 +146,7 @@ func previewCommentPlanAllOutput() { {"RECONCILIATION REQUIRED (IN PROGRESS)", func() { fmt.Print(webhooktemplates.PreviewCommentSchemaReconciliationInProgress()) }}, {"RECONCILIATION REQUIRED (COMPLETED)", func() { fmt.Print(webhooktemplates.PreviewCommentSchemaReconciliationCompleted()) }}, {"VITESS PLAN", func() { fmt.Print(webhooktemplates.PreviewCommentVitessPlan()) }}, + {"VITESS PLAN: VSCHEMA REMOVAL (UNSAFE)", func() { fmt.Print(webhooktemplates.PreviewCommentVitessPlanVSchemaRemoval()) }}, {"SCHEMA CHANGE APPLY (LOCKED + OPTIONS)", func() { fmt.Print(webhooktemplates.PreviewCommentVitessApplyPlan()) }}, {"MYSQL MULTI-SCHEMA PLAN", func() { fmt.Print(webhooktemplates.PreviewCommentMySQLMultiSchema()) }}, {"MULTI-ENV PLAN (IDENTICAL)", func() { fmt.Print(webhooktemplates.PreviewCommentMultiEnvPlan()) }}, diff --git a/pkg/cmd/internal/templates/preview_dispatch.go b/pkg/cmd/internal/templates/preview_dispatch.go index 19c92fdc4..311acc5ba 100644 --- a/pkg/cmd/internal/templates/preview_dispatch.go +++ b/pkg/cmd/internal/templates/preview_dispatch.go @@ -144,6 +144,8 @@ func PreviewCLIOutput(previewType PreviewType) { fmt.Print(webhooktemplates.PreviewCommentMultiEnvPlanLint()) case PreviewCommentVitessPlan: fmt.Print(webhooktemplates.PreviewCommentVitessPlan()) + case PreviewCommentVitessPlanVSchemaRemoval: + fmt.Print(webhooktemplates.PreviewCommentVitessPlanVSchemaRemoval()) case PreviewCommentVitessApplyPlan: fmt.Print(webhooktemplates.PreviewCommentVitessApplyPlan()) case PreviewCommentMySQLMultiSchema: diff --git a/pkg/engine/planetscale/plan.go b/pkg/engine/planetscale/plan.go index c20c1a7b4..223f86337 100644 --- a/pkg/engine/planetscale/plan.go +++ b/pkg/engine/planetscale/plan.go @@ -11,6 +11,7 @@ import ( "github.com/block/spirit/pkg/table" "golang.org/x/sync/errgroup" + "github.com/block/schemabot/pkg/apitypes" "github.com/block/schemabot/pkg/ddl" "github.com/block/schemabot/pkg/engine" "github.com/block/schemabot/pkg/lint" @@ -18,6 +19,27 @@ import ( "github.com/block/schemabot/pkg/vschema" ) +// vschemaDeletionsMetadata detects structural removals between the current and +// desired VSchema and encodes them for plan change metadata. Removals gate the +// apply behind the unsafe opt-in: deleting a vindex, a table routing entry, or +// a column-vindex association changes Vitess query routing the moment the +// VSchema lands, so it is as dangerous as destructive DDL. Returns "" when the +// change removes nothing. +func vschemaDeletionsMetadata(currentRaw, desired string) (string, error) { + deletions, err := vschema.Deletions(currentRaw, desired) + if err != nil { + return "", err + } + if len(deletions) == 0 { + return "", nil + } + converted := make([]apitypes.VSchemaDeletion, len(deletions)) + for i, d := range deletions { + converted[i] = apitypes.VSchemaDeletion{Kind: d.Kind, Name: d.Name, Reason: d.Reason} + } + return apitypes.EncodeVSchemaDeletions(converted) +} + // Plan computes the schema changes needed by diffing current schema against desired. // For each keyspace in the schema files, it fetches the current schema and uses // Spirit's PlanChanges to diff and lint in a single pass. @@ -86,6 +108,13 @@ func (e *Engine) Plan(ctx context.Context, req *engine.PlanRequest) (*engine.Pla if vschemaChanged { sc.Metadata["vschema_changed"] = "true" sc.Metadata["vschema"] = vschema.Diff(currentVSchemaRaw, ns.Files["vschema.json"]) + deletionsMeta, delErr := vschemaDeletionsMetadata(currentVSchemaRaw, ns.Files["vschema.json"]) + if delErr != nil { + return fmt.Errorf("detect VSchema deletions for keyspace %s: %w", ks, delErr) + } + if deletionsMeta != "" { + sc.Metadata[apitypes.VSchemaDeletionsMetadataKey] = deletionsMeta + } if strings.TrimSpace(currentVSchemaRaw) == "" { currentVSchemaRaw = "{}" } diff --git a/pkg/engine/planetscale/plan_vschema_test.go b/pkg/engine/planetscale/plan_vschema_test.go new file mode 100644 index 000000000..6e827effa --- /dev/null +++ b/pkg/engine/planetscale/plan_vschema_test.go @@ -0,0 +1,39 @@ +package planetscale + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/block/schemabot/pkg/apitypes" +) + +func TestVSchemaDeletionsMetadata(t *testing.T) { + current := `{"sharded": true, "vindexes": {"hash": {"type": "hash"}, "email_lookup": {"type": "consistent_lookup_unique"}}, "tables": {"users": {"column_vindexes": [{"column": "id", "name": "hash"}]}}}` + desired := `{"sharded": true, "vindexes": {"hash": {"type": "hash"}}, "tables": {"users": {"column_vindexes": [{"column": "id", "name": "hash"}]}}}` + + meta, err := vschemaDeletionsMetadata(current, desired) + require.NoError(t, err) + require.NotEmpty(t, meta) + + deletions, err := apitypes.ParseVSchemaDeletions(map[string]string{apitypes.VSchemaDeletionsMetadataKey: meta}) + require.NoError(t, err) + require.Len(t, deletions, 1) + assert.Equal(t, "vindex", deletions[0].Kind) + assert.Equal(t, "email_lookup", deletions[0].Name) +} + +func TestVSchemaDeletionsMetadata_NoRemovals(t *testing.T) { + current := `{"tables": {"widgets": {}}}` + desired := `{"tables": {"widgets": {}, "gadgets": {}}}` + + meta, err := vschemaDeletionsMetadata(current, desired) + require.NoError(t, err) + assert.Empty(t, meta) +} + +func TestVSchemaDeletionsMetadata_UnparseableFailsClosed(t *testing.T) { + _, err := vschemaDeletionsMetadata("{corrupt", `{"tables": {}}`) + require.Error(t, err) +} diff --git a/pkg/vschema/deletions.go b/pkg/vschema/deletions.go new file mode 100644 index 000000000..fd4e4db5f --- /dev/null +++ b/pkg/vschema/deletions.go @@ -0,0 +1,134 @@ +package vschema + +import ( + "fmt" + "sort" + "strings" + + "google.golang.org/protobuf/encoding/protojson" + + vschemapb "vitess.io/vitess/go/vt/proto/vschema" +) + +// Deletion kinds, in the order they are reported. +const ( + DeletionKindVindex = "vindex" + DeletionKindTable = "table" + DeletionKindColumnVindex = "column_vindex" +) + +// Deletion describes a structural removal between a current and desired +// VSchema: a vindex definition, a table routing entry, or a table's +// column-vindex association. Removals take effect the moment the VSchema is +// applied โ€” Vitess stops using the removed entry for routing, lookup vindexes +// stop being maintained, and queries that depended on it can fail or scatter โ€” +// so callers treat them as unsafe changes requiring explicit operator opt-in. +type Deletion struct { + Kind string `json:"kind"` + Name string `json:"name"` + Reason string `json:"reason"` +} + +// Deletions returns the structural removals needed to go from the current +// VSchema to the desired one. An empty or blank current VSchema (a new +// keyspace) has nothing to remove. Both documents must parse as VSchema +// keyspace JSON; a document that cannot be parsed returns an error so callers +// can fail closed rather than miss a removal. +func Deletions(current, desired string) ([]Deletion, error) { + if strings.TrimSpace(current) == "" || strings.TrimSpace(current) == "{}" { + return nil, nil + } + + currentKs, err := parseKeyspace(current) + if err != nil { + return nil, fmt.Errorf("parse current VSchema: %w", err) + } + desiredKs, err := parseKeyspace(desired) + if err != nil { + return nil, fmt.Errorf("parse desired VSchema: %w", err) + } + + var deletions []Deletion + + for _, name := range sortedKeys(currentKs.Vindexes) { + if _, ok := desiredKs.Vindexes[name]; !ok { + deletions = append(deletions, Deletion{ + Kind: DeletionKindVindex, + Name: name, + Reason: vindexRemovalReason(name, currentKs.Vindexes[name]), + }) + } + } + + for _, table := range sortedKeys(currentKs.Tables) { + desiredTable, ok := desiredKs.Tables[table] + if !ok { + deletions = append(deletions, Deletion{ + Kind: DeletionKindTable, + Name: table, + Reason: fmt.Sprintf("table %q is removed from the VSchema: Vitess loses its routing entry and queries against it can fail", table), + }) + continue + } + currentTable := currentKs.Tables[table] + desiredVindexes := columnVindexNames(desiredTable) + for _, cv := range currentTable.GetColumnVindexes() { + if _, ok := desiredVindexes[cv.GetName()]; !ok { + deletions = append(deletions, Deletion{ + Kind: DeletionKindColumnVindex, + Name: table + "." + cv.GetName(), + Reason: fmt.Sprintf("table %q no longer uses vindex %q: routing for queries on its columns changes immediately and lookup rows stop being maintained", table, cv.GetName()), + }) + } + } + } + + return deletions, nil +} + +// vindexRemovalReason explains the operational impact of removing a vindex +// definition. Lookup-family vindexes own rows in a backing table, so their +// removal additionally stops that table from being maintained; functional +// vindexes (hash etc.) lose routing only. +func vindexRemovalReason(name string, v *vschemapb.Vindex) string { + if strings.Contains(v.GetType(), "lookup") { + if backing := v.GetParams()["table"]; backing != "" { + return fmt.Sprintf("lookup vindex %q is removed: Vitess immediately stops maintaining its rows in backing table %q, queries routed through it can fail or scatter, and the lookup data goes stale", name, backing) + } + return fmt.Sprintf("lookup vindex %q is removed: Vitess immediately stops maintaining its lookup rows and queries routed through it can fail or scatter", name) + } + return fmt.Sprintf("vindex %q is removed: Vitess immediately stops using it for routing and lookups, and queries that depend on it can fail or scatter", name) +} + +// parseKeyspace decodes a VSchema keyspace JSON document. Unlike Normalize, +// which is a best-effort canonicalizer for display comparison, this parse is +// strict: deletion detection is a safety input and must not silently treat an +// unreadable document as empty. +func parseKeyspace(s string) (*vschemapb.Keyspace, error) { + s = strings.TrimSpace(s) + if s == "" { + s = "{}" + } + var ks vschemapb.Keyspace + if err := protojson.Unmarshal([]byte(s), &ks); err != nil { + return nil, err + } + return &ks, nil +} + +func columnVindexNames(t *vschemapb.Table) map[string]struct{} { + names := make(map[string]struct{}, len(t.GetColumnVindexes())) + for _, cv := range t.GetColumnVindexes() { + names[cv.GetName()] = struct{}{} + } + return names +} + +func sortedKeys[V any](m map[string]V) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} diff --git a/pkg/vschema/deletions_test.go b/pkg/vschema/deletions_test.go new file mode 100644 index 000000000..2bc12aa4c --- /dev/null +++ b/pkg/vschema/deletions_test.go @@ -0,0 +1,218 @@ +package vschema + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const shardedVSchema = `{ + "sharded": true, + "vindexes": { + "hash": {"type": "hash"}, + "email_lookup": { + "type": "consistent_lookup_unique", + "params": {"table": "email_lookup", "from": "email", "to": "keyspace_id"}, + "owner": "users" + } + }, + "tables": { + "users": { + "column_vindexes": [ + {"column": "id", "name": "hash"}, + {"column": "email", "name": "email_lookup"} + ] + }, + "orders": { + "column_vindexes": [ + {"column": "user_id", "name": "hash"} + ] + } + } +}` + +func TestDeletions_NoChange(t *testing.T) { + deletions, err := Deletions(shardedVSchema, shardedVSchema) + require.NoError(t, err) + assert.Empty(t, deletions) +} + +func TestDeletions_EmptyCurrent(t *testing.T) { + // A new keyspace has no current VSchema, so nothing can be removed. + for _, current := range []string{"", " ", "{}"} { + deletions, err := Deletions(current, shardedVSchema) + require.NoError(t, err) + assert.Empty(t, deletions) + } +} + +func TestDeletions_AdditionsOnly(t *testing.T) { + desired := `{ + "sharded": true, + "vindexes": { + "hash": {"type": "hash"}, + "email_lookup": { + "type": "consistent_lookup_unique", + "params": {"table": "email_lookup", "from": "email", "to": "keyspace_id"}, + "owner": "users" + }, + "region_idx": {"type": "hash"} + }, + "tables": { + "users": { + "column_vindexes": [ + {"column": "id", "name": "hash"}, + {"column": "email", "name": "email_lookup"} + ] + }, + "orders": { + "column_vindexes": [ + {"column": "user_id", "name": "hash"}, + {"column": "region", "name": "region_idx"} + ] + }, + "payments": { + "column_vindexes": [ + {"column": "id", "name": "hash"} + ] + } + } + }` + deletions, err := Deletions(shardedVSchema, desired) + require.NoError(t, err) + assert.Empty(t, deletions) +} + +func TestDeletions_RemovedVindex(t *testing.T) { + // email_lookup vindex removed entirely, along with the users column that + // referenced it. + desired := `{ + "sharded": true, + "vindexes": { + "hash": {"type": "hash"} + }, + "tables": { + "users": { + "column_vindexes": [ + {"column": "id", "name": "hash"} + ] + }, + "orders": { + "column_vindexes": [ + {"column": "user_id", "name": "hash"} + ] + } + } + }` + deletions, err := Deletions(shardedVSchema, desired) + require.NoError(t, err) + require.Len(t, deletions, 2) + + assert.Equal(t, DeletionKindVindex, deletions[0].Kind) + assert.Equal(t, "email_lookup", deletions[0].Name) + assert.Contains(t, deletions[0].Reason, `lookup vindex "email_lookup"`) + assert.Contains(t, deletions[0].Reason, `backing table "email_lookup"`) + assert.Contains(t, deletions[0].Reason, "goes stale") + + assert.Equal(t, DeletionKindColumnVindex, deletions[1].Kind) + assert.Equal(t, "users.email_lookup", deletions[1].Name) +} + +func TestDeletions_RemovedTable(t *testing.T) { + desired := `{ + "sharded": true, + "vindexes": { + "hash": {"type": "hash"}, + "email_lookup": { + "type": "consistent_lookup_unique", + "params": {"table": "email_lookup", "from": "email", "to": "keyspace_id"}, + "owner": "users" + } + }, + "tables": { + "users": { + "column_vindexes": [ + {"column": "id", "name": "hash"}, + {"column": "email", "name": "email_lookup"} + ] + } + } + }` + deletions, err := Deletions(shardedVSchema, desired) + require.NoError(t, err) + require.Len(t, deletions, 1) + assert.Equal(t, DeletionKindTable, deletions[0].Kind) + assert.Equal(t, "orders", deletions[0].Name) + assert.Contains(t, deletions[0].Reason, "orders") + assert.Contains(t, deletions[0].Reason, "routing entry") +} + +func TestDeletions_RemovedColumnVindex(t *testing.T) { + // The email_lookup vindex definition survives, but the users table no + // longer routes through it โ€” the association removal alone is unsafe. + desired := `{ + "sharded": true, + "vindexes": { + "hash": {"type": "hash"}, + "email_lookup": { + "type": "consistent_lookup_unique", + "params": {"table": "email_lookup", "from": "email", "to": "keyspace_id"}, + "owner": "users" + } + }, + "tables": { + "users": { + "column_vindexes": [ + {"column": "id", "name": "hash"} + ] + }, + "orders": { + "column_vindexes": [ + {"column": "user_id", "name": "hash"} + ] + } + } + }` + deletions, err := Deletions(shardedVSchema, desired) + require.NoError(t, err) + require.Len(t, deletions, 1) + assert.Equal(t, DeletionKindColumnVindex, deletions[0].Kind) + assert.Equal(t, "users.email_lookup", deletions[0].Name) + assert.Contains(t, deletions[0].Reason, `"users"`) + assert.Contains(t, deletions[0].Reason, `"email_lookup"`) +} + +func TestDeletions_UnparseableCurrentFailsClosed(t *testing.T) { + _, err := Deletions("{not json", shardedVSchema) + require.Error(t, err) + assert.Contains(t, err.Error(), "parse current VSchema") +} + +func TestDeletions_UnparseableDesiredFailsClosed(t *testing.T) { + _, err := Deletions(shardedVSchema, "{not json") + require.Error(t, err) + assert.Contains(t, err.Error(), "parse desired VSchema") +} + +func TestDeletions_UnshardedTableRemoval(t *testing.T) { + // Unsharded keyspaces list tables without vindexes; removing one still + // drops its routing entry. + current := `{"tables": {"widgets": {}, "gadgets": {}}}` + desired := `{"tables": {"widgets": {}}}` + deletions, err := Deletions(current, desired) + require.NoError(t, err) + require.Len(t, deletions, 1) + assert.Equal(t, DeletionKindTable, deletions[0].Kind) + assert.Equal(t, "gadgets", deletions[0].Name) +} + +func TestDeletions_FunctionalVindexRemovalReason(t *testing.T) { + current := `{"sharded": true, "vindexes": {"hash": {"type": "hash"}, "region": {"type": "region_json"}}, "tables": {"users": {"column_vindexes": [{"column": "id", "name": "hash"}]}}}` + desired := `{"sharded": true, "vindexes": {"hash": {"type": "hash"}}, "tables": {"users": {"column_vindexes": [{"column": "id", "name": "hash"}]}}}` + deletions, err := Deletions(current, desired) + require.NoError(t, err) + require.Len(t, deletions, 1) + assert.Contains(t, deletions[0].Reason, "stops using it for routing") + assert.NotContains(t, deletions[0].Reason, "backing table") +} diff --git a/pkg/webhook/plan.go b/pkg/webhook/plan.go index d81fb4801..493cfe602 100644 --- a/pkg/webhook/plan.go +++ b/pkg/webhook/plan.go @@ -811,23 +811,43 @@ func buildPlanCommentData(schema *ghclient.SchemaRequestResult, planResp *apityp data.Changes = append(data.Changes, ksData) } - // Unsafe changes. For a sharded plan, derive them from the per-shard changes - // so an unsafe change confined to one shard (e.g. a column drop on a single - // drifted shard) is still flagged with the shard it applies to โ€” the - // collapsed namespace-level Changes can omit it. Otherwise use the - // namespace-level view. - if unsafe := shardedUnsafeChanges(planResp.Shards); len(unsafe) > 0 { - data.HasUnsafeChanges = true - data.UnsafeChanges = unsafe - } else if unsafeChanges := planResp.UnsafeChanges(); len(unsafeChanges) > 0 { - data.HasUnsafeChanges = true - for _, uc := range unsafeChanges { - data.UnsafeChanges = append(data.UnsafeChanges, templates.UnsafeChangeData{ + // Unsafe changes. For a sharded plan, derive table-level entries from the + // per-shard changes so an unsafe change confined to one shard (e.g. a column + // drop on a single drifted shard) is still flagged with the shard it applies + // to โ€” the collapsed namespace-level Changes can omit it. Otherwise use the + // namespace-level table view. VSchema removals live only on the + // namespace-level change, so they are appended in both views. + unsafe := shardedUnsafeChanges(planResp.Shards) + if len(unsafe) == 0 { + for _, sc := range planResp.Changes { + if sc == nil { + continue + } + for _, t := range sc.TableChanges { + if uc, ok := t.UnsafeChange(); ok { + unsafe = append(unsafe, templates.UnsafeChangeData{ + Table: uc.Table, + Reason: uc.Reason, + }) + } + } + } + } + for _, sc := range planResp.Changes { + if sc == nil { + continue + } + for _, uc := range sc.VSchemaUnsafeChanges() { + unsafe = append(unsafe, templates.UnsafeChangeData{ Table: uc.Table, Reason: uc.Reason, }) } } + if len(unsafe) > 0 { + data.HasUnsafeChanges = true + data.UnsafeChanges = unsafe + } // Blocked changes โ€” the apply commands will reject these. Like the // unsafe view, a sharded plan derives them per shard so a blocked change diff --git a/pkg/webhook/templates/preview.go b/pkg/webhook/templates/preview.go index 52647347c..d21836c98 100644 --- a/pkg/webhook/templates/preview.go +++ b/pkg/webhook/templates/preview.go @@ -940,6 +940,65 @@ func PreviewCommentVitessPlan() string { }) } +// PreviewCommentVitessPlanVSchemaRemoval renders a sample Vitess plan comment +// where the VSchema change removes a lookup vindex and its column-vindex +// association โ€” the removals surface in the Issues section as unsafe changes. +func PreviewCommentVitessPlanVSchemaRemoval() string { + return RenderPlanComment(PlanCommentData{ + Database: "commerce", + SchemaName: "commerce", + Environment: "staging", + HeadSHA: previewHeadSHA, + Repository: previewRepository, + RequestedBy: previewRequestedBy, + IsMySQL: false, + Changes: []KeyspaceChangeData{ + { + Keyspace: "commerce_sharded", + VSchemaChanged: true, + VSchemaDiff: `--- a/commerce_sharded.json ++++ b/commerce_sharded.json +@@ -3,10 +3,6 @@ + "hash": { + "type": "hash" +- }, +- "customers_email_lookup": { +- "type": "consistent_lookup_unique", +- "params": { +- "table": "customers_email_lookup", +- "from": "email", +- "to": "keyspace_id" +- }, +- "owner": "customers" + } + }, +@@ -18,8 +14,4 @@ + { + "column": "id", + "name": "hash" +- }, +- { +- "column": "email", +- "name": "customers_email_lookup" + } + ] + }`, + }, + }, + HasUnsafeChanges: true, + UnsafeChanges: []UnsafeChangeData{ + { + Table: "commerce_sharded/vschema.json", + Reason: `lookup vindex "customers_email_lookup" is removed: Vitess immediately stops maintaining its rows in backing table "customers_email_lookup", queries routed through it can fail or scatter, and the lookup data goes stale`, + }, + { + Table: "commerce_sharded/vschema.json", + Reason: `table "customers" no longer uses vindex "customers_email_lookup": routing for queries on its columns changes immediately and lookup rows stop being maintained`, + }, + }, + }) +} + // PreviewCommentVitessApplyPlan renders a sample locked Vitess apply-plan with options. func PreviewCommentVitessApplyPlan() string { return RenderPlanComment(PlanCommentData{ From 24f70711902c67e73ceb8305fb443c1e2a67aea3 Mon Sep 17 00:00:00 2001 From: Armand Parajon Date: Wed, 19 Aug 2026 01:00:29 +0800 Subject: [PATCH 2/3] fix(vitess): address VSchema unsafe-gate review feedback Tolerate unknown VSchema fields in deletion detection so a newer Vitess does not fail planning, keep the fail-closed decode reason on one line for CLI rendering, and have rollback disclose unsafe changes without claiming an --allow-unsafe flag it does not have. Co-Authored-By: Claude Fable 5 --- pkg/apitypes/vschema.go | 2 +- pkg/cmd/commands/rollback.go | 5 +++-- pkg/vschema/deletions.go | 6 ++++-- pkg/vschema/deletions_test.go | 12 ++++++++++++ 4 files changed, 20 insertions(+), 5 deletions(-) diff --git a/pkg/apitypes/vschema.go b/pkg/apitypes/vschema.go index f41c57797..d3c5c8a26 100644 --- a/pkg/apitypes/vschema.go +++ b/pkg/apitypes/vschema.go @@ -78,7 +78,7 @@ func (sc *SchemaChangeResponse) VSchemaUnsafeChanges() []UnsafeChange { if err != nil { return []UnsafeChange{{ Table: sc.Namespace + "/vschema.json", - Reason: "VSchema deletions were recorded on this plan but could not be decoded; treating the VSchema change as unsafe", + Reason: "VSchema deletions were recorded on this plan but could not be decoded, so the VSchema change is treated as unsafe", ChangeType: VSchemaChangeType, }} } diff --git a/pkg/cmd/commands/rollback.go b/pkg/cmd/commands/rollback.go index b3b3be5ba..f8f2ab7e7 100644 --- a/pkg/cmd/commands/rollback.go +++ b/pkg/cmd/commands/rollback.go @@ -98,8 +98,9 @@ func (cmd *RollbackCmd) Run(g *Globals) error { } } - // Show unsafe warning if any - templates.WriteUnsafeWarningAllowed(planResult.UnsafeChanges()) + // Disclose unsafe changes before the confirmation prompt. Rollback has no + // --allow-unsafe flag; the interactive confirmation is the acknowledgment. + templates.WriteUnsafeChangesWarning(planResult.UnsafeChanges()) // Show options if any flags are set templates.WriteOptions(cmd.DeferCutover, false) diff --git a/pkg/vschema/deletions.go b/pkg/vschema/deletions.go index fd4e4db5f..f4d0035ef 100644 --- a/pkg/vschema/deletions.go +++ b/pkg/vschema/deletions.go @@ -103,14 +103,16 @@ func vindexRemovalReason(name string, v *vschemapb.Vindex) string { // parseKeyspace decodes a VSchema keyspace JSON document. Unlike Normalize, // which is a best-effort canonicalizer for display comparison, this parse is // strict: deletion detection is a safety input and must not silently treat an -// unreadable document as empty. +// unreadable document as empty. Unknown fields are tolerated so a VSchema +// served by a newer Vitess than the vendored proto still parses; malformed +// JSON still fails. func parseKeyspace(s string) (*vschemapb.Keyspace, error) { s = strings.TrimSpace(s) if s == "" { s = "{}" } var ks vschemapb.Keyspace - if err := protojson.Unmarshal([]byte(s), &ks); err != nil { + if err := (protojson.UnmarshalOptions{DiscardUnknown: true}).Unmarshal([]byte(s), &ks); err != nil { return nil, err } return &ks, nil diff --git a/pkg/vschema/deletions_test.go b/pkg/vschema/deletions_test.go index 2bc12aa4c..90b4851d0 100644 --- a/pkg/vschema/deletions_test.go +++ b/pkg/vschema/deletions_test.go @@ -183,6 +183,18 @@ func TestDeletions_RemovedColumnVindex(t *testing.T) { assert.Contains(t, deletions[0].Reason, `"email_lookup"`) } +func TestDeletions_UnknownFieldsTolerated(t *testing.T) { + // A VSchema served by a newer Vitess can carry fields the vendored proto + // does not know about; they must not fail deletion detection. + current := `{"sharded": true, "future_field": {"x": 1}, "vindexes": {"hash": {"type": "hash"}}, "tables": {"users": {"column_vindexes": [{"column": "id", "name": "hash"}]}}}` + desired := `{"sharded": true, "vindexes": {"hash": {"type": "hash"}}, "tables": {}}` + deletions, err := Deletions(current, desired) + require.NoError(t, err) + require.Len(t, deletions, 1) + assert.Equal(t, DeletionKindTable, deletions[0].Kind) + assert.Equal(t, "users", deletions[0].Name) +} + func TestDeletions_UnparseableCurrentFailsClosed(t *testing.T) { _, err := Deletions("{not json", shardedVSchema) require.Error(t, err) From aa4947b7ee76f2ab822cf33bfbff49dab191217b Mon Sep 17 00:00:00 2001 From: Armand Parajon Date: Wed, 19 Aug 2026 10:05:25 +0800 Subject: [PATCH 3/3] fix(vitess): detect column-vindex reassociation as a structural removal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A column-vindex association is identified by the vindex name plus the ordered columns it covers, not the name alone. Moving a vindex to different columns (or reordering a multi-column vindex) removes the old columns' association โ€” routing for queries on them changes immediately โ€” so it now gets the same unsafe disclosure as removing the vindex from the table. Co-Authored-By: Claude Fable 5 --- pkg/vschema/deletions.go | 59 +++++++++++++++++++++++++++----- pkg/vschema/deletions_test.go | 64 +++++++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 9 deletions(-) diff --git a/pkg/vschema/deletions.go b/pkg/vschema/deletions.go index f4d0035ef..b3b9441d2 100644 --- a/pkg/vschema/deletions.go +++ b/pkg/vschema/deletions.go @@ -71,15 +71,29 @@ func Deletions(current, desired string) ([]Deletion, error) { continue } currentTable := currentKs.Tables[table] - desiredVindexes := columnVindexNames(desiredTable) + desiredAssociations, desiredColumnsByVindex := columnVindexAssociations(desiredTable) for _, cv := range currentTable.GetColumnVindexes() { - if _, ok := desiredVindexes[cv.GetName()]; !ok { + if _, ok := desiredAssociations[columnVindexKey(cv)]; ok { + continue + } + // The exact association is gone. A vindex that still appears on the + // table but covers different columns is a reassociation โ€” the old + // columns' association is removed just as surely as if the vindex + // had left the table, so it gets the same unsafe disclosure. + if newColumns, ok := desiredColumnsByVindex[cv.GetName()]; ok { deletions = append(deletions, Deletion{ - Kind: DeletionKindColumnVindex, - Name: table + "." + cv.GetName(), - Reason: fmt.Sprintf("table %q no longer uses vindex %q: routing for queries on its columns changes immediately and lookup rows stop being maintained", table, cv.GetName()), + Kind: DeletionKindColumnVindex, + Name: table + "." + cv.GetName(), + Reason: fmt.Sprintf("table %q moves vindex %q from columns (%s) to (%s): routing for queries on the old columns changes immediately and lookup rows for them stop being maintained", + table, cv.GetName(), strings.Join(columnVindexColumns(cv), ", "), strings.Join(newColumns, "), (")), }) + continue } + deletions = append(deletions, Deletion{ + Kind: DeletionKindColumnVindex, + Name: table + "." + cv.GetName(), + Reason: fmt.Sprintf("table %q no longer uses vindex %q: routing for queries on its columns changes immediately and lookup rows stop being maintained", table, cv.GetName()), + }) } } @@ -118,12 +132,39 @@ func parseKeyspace(s string) (*vschemapb.Keyspace, error) { return &ks, nil } -func columnVindexNames(t *vschemapb.Table) map[string]struct{} { - names := make(map[string]struct{}, len(t.GetColumnVindexes())) +// columnVindexAssociations indexes a table's column-vindex associations two +// ways: the full (vindex, ordered columns) association keys, and the rendered +// column sets each vindex covers. The first answers "is this exact association +// still present", the second lets a reassociation report where the vindex +// moved to. +func columnVindexAssociations(t *vschemapb.Table) (map[string]struct{}, map[string][]string) { + associations := make(map[string]struct{}, len(t.GetColumnVindexes())) + columnsByVindex := make(map[string][]string, len(t.GetColumnVindexes())) for _, cv := range t.GetColumnVindexes() { - names[cv.GetName()] = struct{}{} + associations[columnVindexKey(cv)] = struct{}{} + columnsByVindex[cv.GetName()] = append(columnsByVindex[cv.GetName()], strings.Join(columnVindexColumns(cv), ", ")) + } + return associations, columnsByVindex +} + +// columnVindexKey identifies one column-vindex association: the vindex name +// plus the ordered columns it covers. Column order is part of the identity โ€” +// a multi-column vindex maps its columns positionally, so reordering them +// changes routing the same way re-pointing the vindex does. +func columnVindexKey(cv *vschemapb.ColumnVindex) string { + return cv.GetName() + "(" + strings.Join(columnVindexColumns(cv), ",") + ")" +} + +// columnVindexColumns returns the ordered columns an association covers, +// honoring the legacy single-column field when the list form is absent. +func columnVindexColumns(cv *vschemapb.ColumnVindex) []string { + if columns := cv.GetColumns(); len(columns) > 0 { + return columns + } + if column := cv.GetColumn(); column != "" { + return []string{column} } - return names + return nil } func sortedKeys[V any](m map[string]V) []string { diff --git a/pkg/vschema/deletions_test.go b/pkg/vschema/deletions_test.go index 90b4851d0..d5f9f25b2 100644 --- a/pkg/vschema/deletions_test.go +++ b/pkg/vschema/deletions_test.go @@ -183,6 +183,70 @@ func TestDeletions_RemovedColumnVindex(t *testing.T) { assert.Contains(t, deletions[0].Reason, `"email_lookup"`) } +func TestDeletions_ReassociatedColumnVindex(t *testing.T) { + // The users table keeps vindex email_lookup but moves it from the email + // column to phone. The old column's association is removed just as surely + // as if the vindex had left the table โ€” routing for queries on email + // changes immediately โ€” so the reassociation is disclosed as unsafe. + desired := `{ + "sharded": true, + "vindexes": { + "hash": {"type": "hash"}, + "email_lookup": { + "type": "consistent_lookup_unique", + "params": {"table": "email_lookup", "from": "email", "to": "keyspace_id"}, + "owner": "users" + } + }, + "tables": { + "users": { + "column_vindexes": [ + {"column": "id", "name": "hash"}, + {"column": "phone", "name": "email_lookup"} + ] + }, + "orders": { + "column_vindexes": [ + {"column": "user_id", "name": "hash"} + ] + } + } + }` + deletions, err := Deletions(shardedVSchema, desired) + require.NoError(t, err) + require.Len(t, deletions, 1) + assert.Equal(t, DeletionKindColumnVindex, deletions[0].Kind) + assert.Equal(t, "users.email_lookup", deletions[0].Name) + assert.Contains(t, deletions[0].Reason, "moves vindex") + assert.Contains(t, deletions[0].Reason, "(email)") + assert.Contains(t, deletions[0].Reason, "(phone)") +} + +func TestDeletions_ReorderedMultiColumnVindex(t *testing.T) { + // A multi-column vindex maps its columns positionally, so reordering the + // columns changes routing and is disclosed like any other reassociation. + current := `{"sharded": true, "vindexes": {"multi": {"type": "multicol"}}, "tables": {"events": {"column_vindexes": [{"columns": ["tenant_id", "region"], "name": "multi"}]}}}` + desired := `{"sharded": true, "vindexes": {"multi": {"type": "multicol"}}, "tables": {"events": {"column_vindexes": [{"columns": ["region", "tenant_id"], "name": "multi"}]}}}` + deletions, err := Deletions(current, desired) + require.NoError(t, err) + require.Len(t, deletions, 1) + assert.Equal(t, DeletionKindColumnVindex, deletions[0].Kind) + assert.Equal(t, "events.multi", deletions[0].Name) + assert.Contains(t, deletions[0].Reason, "(tenant_id, region)") + assert.Contains(t, deletions[0].Reason, "(region, tenant_id)") +} + +func TestDeletions_LegacyColumnFieldEquivalent(t *testing.T) { + // The legacy single-column field and the single-entry columns list express + // the same association; a document normalized from one form to the other + // must not be flagged as a removal. + current := `{"sharded": true, "vindexes": {"hash": {"type": "hash"}}, "tables": {"users": {"column_vindexes": [{"column": "id", "name": "hash"}]}}}` + desired := `{"sharded": true, "vindexes": {"hash": {"type": "hash"}}, "tables": {"users": {"column_vindexes": [{"columns": ["id"], "name": "hash"}]}}}` + deletions, err := Deletions(current, desired) + require.NoError(t, err) + assert.Empty(t, deletions) +} + func TestDeletions_UnknownFieldsTolerated(t *testing.T) { // A VSchema served by a newer Vitess can carry fields the vendored proto // does not know about; they must not fail deletion detection.