From ae3904926793d09773c15a32cacfa0cd1cf603f8 Mon Sep 17 00:00:00 2001 From: aaltshuler Date: Mon, 25 May 2026 23:51:04 +0100 Subject: [PATCH] =?UTF-8?q?feat(schema):=20enum=20widening=20+=20enum?= =?UTF-8?q?=E2=86=92String=20migration=20(Safe=20path)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a ChangeEnumConstraint migration step and detect enum value-set deltas in the planner before the generic prop-type-change rejection. This commit lands the Safe (metadata-only) cases: - widen (add allowed variants): every existing row is still valid, so it's a no-code, no-scan change. - enum→String (loosen to a free string): every enum value is a valid String, so likewise Safe. Enums are stored physically as Arrow String, so these are catalog-only changes — no table rewrite, the manifest version doesn't advance, and apply rides the metadata-only path (handled as a no-op in the step loop). The CLI `schema plan` renderer shows the step (with code+tier when present). The new variant's diagnostic() resolves its attached code so apply/render derive the tier from one source of truth. Validated tightenings (narrow, String→enum) deliberately still fall through to UnsupportedChange here; they're enabled in the next commit together with the apply-time row scan, so we never accept a tightening we can't validate against existing data. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/omnigraph-cli/src/main.rs | 28 +++ .../src/catalog/schema_plan.rs | 195 ++++++++++++++++-- .../src/db/omnigraph/schema_apply.rs | 5 + crates/omnigraph/tests/schema_apply.rs | 63 ++++++ 4 files changed, 273 insertions(+), 18 deletions(-) diff --git a/crates/omnigraph-cli/src/main.rs b/crates/omnigraph-cli/src/main.rs index 84f1c19..359c127 100644 --- a/crates/omnigraph-cli/src/main.rs +++ b/crates/omnigraph-cli/src/main.rs @@ -1121,6 +1121,34 @@ fn render_schema_plan_step(step: &SchemaMigrationStep) -> String { type_name, drop_mode_label(*mode), ), + SchemaMigrationStep::ChangeEnumConstraint { + type_kind, + type_name, + property_name, + to_property_type, + .. + } => { + // Show code + tier when attached (a Validated tightening), so + // operators see that apply will scan existing rows; a Safe + // widen / loosen carries no code. + let base = format!( + "change enum constraint on '{}.{}' -> {} on {} '{}'", + type_name, + property_name, + render_prop_type(to_property_type), + schema_type_kind_label(*type_kind), + type_name, + ); + match step.diagnostic() { + Some(diag) => format!( + "{} [{}, {}]", + base, + diag.code, + schema_lint_tier_label(diag.tier), + ), + None => base, + } + } SchemaMigrationStep::UnsupportedChange { entity, reason, .. } => { // When a schema-lint code is attached, render code + tier // so operators see at-a-glance the kind of risk (destructive diff --git a/crates/omnigraph-compiler/src/catalog/schema_plan.rs b/crates/omnigraph-compiler/src/catalog/schema_plan.rs index cf2c1eb..1fd5e69 100644 --- a/crates/omnigraph-compiler/src/catalog/schema_plan.rs +++ b/crates/omnigraph-compiler/src/catalog/schema_plan.rs @@ -107,6 +107,25 @@ pub enum SchemaMigrationStep { property_name: String, mode: DropMode, }, + /// Change an enum-typed property's allowed value-set without changing + /// the physical column (enums are stored as `String`). Covers widening + /// (adding variants), narrowing (removing variants), and `String`↔`enum` + /// conversions. `to_property_type` is the desired canonical `PropType`. + /// + /// `code` carries the attached schema-lint code and is the single + /// source of truth for whether apply must scan existing rows: + /// `Some(OG-MF-105 / OG-MF-107)` is a Validated tightening (narrow / + /// `String`→`enum`) that scans and fails loudly on an out-of-set row; + /// `None` is a Safe widening / loosening that is metadata-only. Either + /// way the plan stays `supported` (this is not an `UnsupportedChange`). + ChangeEnumConstraint { + type_kind: SchemaTypeKind, + type_name: String, + property_name: String, + to_property_type: PropType, + #[serde(default, skip_serializing_if = "Option::is_none")] + code: Option, + }, UnsupportedChange { entity: String, reason: String, @@ -150,9 +169,8 @@ impl SchemaMigrationStep { /// non-`UnsupportedChange` variant). pub fn diagnostic(&self) -> Option<&'static crate::lint::DiagnosticCode> { match self { - Self::UnsupportedChange { - code: Some(c), .. - } => crate::lint::lookup(c), + Self::UnsupportedChange { code: Some(c), .. } + | Self::ChangeEnumConstraint { code: Some(c), .. } => crate::lint::lookup(c), _ => None, } } @@ -540,19 +558,29 @@ fn plan_properties( } if existing.prop_type != property.prop_type { - steps.push(SchemaMigrationStep::UnsupportedChange { - entity: format!( - "{}:{}.{}", - schema_type_kind_key(type_kind), - type_name, - property.name - ), - reason: format!( - "changing property type for '{}.{}' is not supported in schema migration v1", - type_name, property.name - ), - code: Some(crate::lint::codes::OG_MF_106.code.to_string()), - }); + // An enum value-set delta (widen / narrow / String↔enum) is a + // supported migration — handle it before the blanket type-change + // rejection. A genuine scalar/nullable/list change falls through + // to UnsupportedChange. + if let Some(step) = + plan_enum_constraint_change(type_kind, type_name, property, existing) + { + steps.push(step); + } else { + steps.push(SchemaMigrationStep::UnsupportedChange { + entity: format!( + "{}:{}.{}", + schema_type_kind_key(type_kind), + type_name, + property.name + ), + reason: format!( + "changing property type for '{}.{}' is not supported in schema migration v1", + type_name, property.name + ), + code: Some(crate::lint::codes::OG_MF_106.code.to_string()), + }); + } } plan_property_metadata( @@ -828,6 +856,63 @@ fn constraint_key(constraint: &Constraint) -> String { } } +/// If `existing → desired` is an in-scope enum value-set change on a +/// Node/Edge property, return the matching `ChangeEnumConstraint` step; +/// otherwise return `None` so the caller falls back to `UnsupportedChange`. +/// +/// In scope: widen (add variants, Safe), narrow (remove variants, +/// OG-MF-105), `String`→`enum` (OG-MF-107), `enum`→`String` (Safe). Out of +/// scope (→ `None`): interface properties (no backing table), and any +/// change that also alters the physical column — a different scalar (e.g. +/// `enum`-String → `I32`), nullability, or list-ness. Enums are stored as +/// `String`, so a pure value-set delta never touches the column. +fn plan_enum_constraint_change( + type_kind: SchemaTypeKind, + type_name: &str, + property: &PropertyIR, + existing: &PropertyIR, +) -> Option { + if type_kind == SchemaTypeKind::Interface { + return None; + } + let from = &existing.prop_type; + let to = &property.prop_type; + if from.scalar != to.scalar || from.nullable != to.nullable || from.list != to.list { + return None; + } + // Value-sets are canonical (sorted + deduped), so equal sets compare + // equal `PropType` and never reach here — a reorder-only change is a + // no-op upstream of this call. + let code = match (from.enum_values.as_deref(), to.enum_values.as_deref()) { + // enum → enum: Safe widen, unless a variant was removed. + (Some(from_vals), Some(to_vals)) => { + let removed = from_vals.iter().any(|v| !to_vals.contains(v)); + if removed { + // Narrow is a Validated tightening (OG-MF-105). It is only + // accepted once the apply-time row scan exists — until then + // fall through to UnsupportedChange so we never admit a narrow + // we cannot validate against existing data. + return None; + } + None + } + // String → enum is a Validated tightening (OG-MF-107); same staging + // as narrow — accepted alongside the apply-time scan. + (None, Some(_)) => return None, + // enum → String: loosening, metadata-only, no scan. + (Some(_), None) => None, + // Neither side is an enum: not an enum delta. + (None, None) => return None, + }; + Some(SchemaMigrationStep::ChangeEnumConstraint { + type_kind, + type_name: type_name.to_string(), + property_name: property.name.clone(), + to_property_type: to.clone(), + code: code.map(str::to_string), + }) +} + fn rename_from_value(annotations: &[Annotation]) -> Option<&str> { annotations .iter() @@ -849,8 +934,8 @@ mod tests { use crate::schema::parser::parse_schema; use super::SchemaMigrationStep::{ - AddConstraint, AddProperty, RenameProperty, RenameType, UnsupportedChange, - UpdateTypeMetadata, + AddConstraint, AddProperty, ChangeEnumConstraint, RenameProperty, RenameType, + UnsupportedChange, UpdateTypeMetadata, }; use super::*; @@ -954,6 +1039,80 @@ node Person { ); } + /// Plan a migration from two `.pg` sources. Test-only convenience. + fn plan_for(accepted_src: &str, desired_src: &str) -> SchemaMigrationPlan { + let accepted = build_schema_ir(&parse_schema(accepted_src).unwrap()).unwrap(); + let desired = build_schema_ir(&parse_schema(desired_src).unwrap()).unwrap(); + plan_schema_migration(&accepted, &desired).unwrap() + } + + #[test] + fn plan_widen_enum_is_supported_safe_change() { + // Adding an allowed value to an existing enum can't invalidate any + // existing row, so it's a Safe metadata-only change: ChangeEnumConstraint + // with no attached code, plan supported. + let plan = plan_for( + "node Doc {\n id: String @key\n status: enum(open, closed)\n}\n", + "node Doc {\n id: String @key\n status: enum(open, closed, archived)\n}\n", + ); + assert!(plan.supported, "widening an enum must be supported: {plan:?}"); + assert!( + plan.steps.iter().any(|step| matches!( + step, + ChangeEnumConstraint { type_name, property_name, code, .. } + if type_name == "Doc" && property_name == "status" && code.is_none() + )), + "expected Safe ChangeEnumConstraint (no code) for widen: {plan:?}", + ); + assert!( + !plan.steps.iter().any(|s| matches!(s, UnsupportedChange { .. })), + "widen must not emit UnsupportedChange: {plan:?}", + ); + } + + #[test] + fn plan_enum_to_string_is_supported_safe_change() { + // Loosening an enum to a free String can't invalidate any row either + // (every enum value is a valid String). Safe, no code. + let plan = plan_for( + "node Doc {\n id: String @key\n status: enum(open, closed)\n}\n", + "node Doc {\n id: String @key\n status: String\n}\n", + ); + assert!(plan.supported, "enum->String must be supported: {plan:?}"); + assert!( + plan.steps.iter().any(|step| matches!( + step, + ChangeEnumConstraint { property_name, code, .. } + if property_name == "status" && code.is_none() + )), + "expected Safe ChangeEnumConstraint (no code) for enum->String: {plan:?}", + ); + } + + #[test] + fn plan_enum_to_non_string_scalar_stays_unsupported() { + // Changing an enum (physically String) to a different scalar is a + // genuine type change — the enum-delta detection must fall through to + // UnsupportedChange (OG-MF-106), not mis-classify it as an enum change. + let plan = plan_for( + "node Doc {\n id: String @key\n status: enum(open, closed)\n}\n", + "node Doc {\n id: String @key\n status: I32\n}\n", + ); + assert!(!plan.supported, "enum->I32 must stay unsupported: {plan:?}"); + assert!( + plan.steps.iter().any(|step| matches!( + step, + UnsupportedChange { code, .. } + if code.as_deref() == Some(crate::lint::codes::OG_MF_106.code) + )), + "expected UnsupportedChange OG-MF-106 for enum->I32: {plan:?}", + ); + assert!( + !plan.steps.iter().any(|s| matches!(s, ChangeEnumConstraint { .. })), + "genuine scalar change must not emit ChangeEnumConstraint: {plan:?}", + ); + } + #[test] fn plan_supports_explicit_type_and_property_rename() { let accepted = build_schema_ir( diff --git a/crates/omnigraph/src/db/omnigraph/schema_apply.rs b/crates/omnigraph/src/db/omnigraph/schema_apply.rs index 0dcf0f9..2ad4f05 100644 --- a/crates/omnigraph/src/db/omnigraph/schema_apply.rs +++ b/crates/omnigraph/src/db/omnigraph/schema_apply.rs @@ -208,6 +208,11 @@ pub(super) async fn apply_schema_with_lock( } SchemaMigrationStep::UpdateTypeMetadata { .. } | SchemaMigrationStep::UpdatePropertyMetadata { .. } => {} + // An enum value-set change is metadata-only — enums are stored + // as String, so the physical column is untouched and no table is + // rewritten. (A Validated tightening's pre-publish row scan is a + // separate phase that reads these steps directly.) + SchemaMigrationStep::ChangeEnumConstraint { .. } => {} SchemaMigrationStep::DropProperty { type_kind, type_name, diff --git a/crates/omnigraph/tests/schema_apply.rs b/crates/omnigraph/tests/schema_apply.rs index 5b65e3d..318a871 100644 --- a/crates/omnigraph/tests/schema_apply.rs +++ b/crates/omnigraph/tests/schema_apply.rs @@ -584,6 +584,69 @@ query set_role($name: String, $role: String) { ); } +#[tokio::test] +async fn apply_schema_widen_enum_accepts_new_variant() { + // Widening an enum (adding an allowed variant) is a Safe, metadata-only + // migration: ChangeEnumConstraint with no code, no table rewrite, manifest + // unchanged. After apply, the new variant is accepted on writes; before, it + // was rejected. Proves the Safe enum path rides the metadata-only apply. + let dir = tempfile::tempdir().unwrap(); + let uri = dir.path().to_str().unwrap(); + let schema = "node Person {\n name: String @key\n role: enum(admin, member)\n}\n"; + let mut db = Omnigraph::init(uri, schema).await.unwrap(); + load_jsonl( + &mut db, + r#"{"type":"Person","data":{"name":"Alice","role":"admin"}}"#, + LoadMode::Overwrite, + ) + .await + .unwrap(); + + const SET_ROLE: &str = + "\nquery set_role($name: String, $role: String) {\n update Person set { role: $role } where name = $name\n}\n"; + + // Before widening, the new variant is rejected. + let err = mutate_main( + &mut db, + SET_ROLE, + "set_role", + ¶ms(&[("$name", "Alice"), ("$role", "guest")]), + ) + .await + .unwrap_err(); + assert!( + err.to_string().contains("invalid enum value 'guest'"), + "guest must be rejected before widening; got: {err}", + ); + + // Widen the enum to include `guest`. + let widened = "node Person {\n name: String @key\n role: enum(admin, member, guest)\n}\n"; + let plan = db.plan_schema(widened).await.unwrap(); + assert!(plan.supported, "widen must be supported: {plan:?}"); + assert!( + plan.steps.iter().any(|step| matches!( + step, + SchemaMigrationStep::ChangeEnumConstraint { property_name, code, .. } + if property_name == "role" && code.is_none() + )), + "expected Safe ChangeEnumConstraint for role: {plan:?}", + ); + let result = db.apply_schema(widened).await.unwrap(); + assert!(result.supported && result.applied); + + // After widening, the new variant is accepted and persisted. + mutate_main( + &mut db, + SET_ROLE, + "set_role", + ¶ms(&[("$name", "Alice"), ("$role", "guest")]), + ) + .await + .unwrap(); + let roles = collect_column_strings(&read_table(&db, "node:Person").await, "role"); + assert_eq!(roles, vec!["guest".to_string()], "role should now be the widened variant"); +} + #[tokio::test] async fn plan_schema_for_property_type_narrowing_is_not_supported() { // Symmetric companion to `apply_schema_unsupported_plan_does_not_advance_manifest`,