mirror of
https://github.com/ModernRelay/omnigraph.git
synced 2026-06-09 01:35:18 +02:00
feat(schema): enum widening + enum→String migration (Safe path)
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) <noreply@anthropic.com>
This commit is contained in:
parent
df30aa6935
commit
ae39049267
4 changed files with 273 additions and 18 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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<String>,
|
||||
},
|
||||
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<SchemaMigrationStep> {
|
||||
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(
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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`,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue