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..d3c5c8a26 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, so the VSchema change is treated 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..f8f2ab7e7 100644 --- a/pkg/cmd/commands/rollback.go +++ b/pkg/cmd/commands/rollback.go @@ -98,11 +98,9 @@ func (cmd *RollbackCmd) Run(g *Globals) error { } } - // Show unsafe warning if any - if planResult.HasErrors() { - unsafeChanges := planResult.UnsafeChanges() - templates.WriteUnsafeWarningAllowed(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/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..b3b9441d2 --- /dev/null +++ b/pkg/vschema/deletions.go @@ -0,0 +1,177 @@ +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] + desiredAssociations, desiredColumnsByVindex := columnVindexAssociations(desiredTable) + for _, cv := range currentTable.GetColumnVindexes() { + 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 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()), + }) + } + } + + 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. 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.UnmarshalOptions{DiscardUnknown: true}).Unmarshal([]byte(s), &ks); err != nil { + return nil, err + } + return &ks, nil +} + +// 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() { + 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 nil +} + +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..d5f9f25b2 --- /dev/null +++ b/pkg/vschema/deletions_test.go @@ -0,0 +1,294 @@ +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_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. + 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) + 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{