diff --git a/crates/omnigraph-compiler/src/catalog/schema_plan.rs b/crates/omnigraph-compiler/src/catalog/schema_plan.rs index a20820d..cf2c1eb 100644 --- a/crates/omnigraph-compiler/src/catalog/schema_plan.rs +++ b/crates/omnigraph-compiler/src/catalog/schema_plan.rs @@ -897,6 +897,63 @@ node Person { })); } + #[test] + fn plan_supports_adding_a_nullable_enum_property() { + // An enum is just a String-typed PropType carrying `enum_values`, + // so adding a *nullable* one rides the same additive path as any + // other nullable property: it must emit an AddProperty step (not + // UnsupportedChange), with the enum value-set preserved on the + // step's PropType. The required-enum case is rejected exactly like + // any required addition — see `plan_rejects_required_property_addition` + // (the OG-MF-103 backfill path is type-agnostic). + let accepted = build_schema_ir( + &parse_schema( + r#" +node Person { + name: String @key +} +"#, + ) + .unwrap(), + ) + .unwrap(); + let desired = build_schema_ir( + &parse_schema( + r#" +node Person { + name: String @key + role: enum(admin, guest, member)? +} +"#, + ) + .unwrap(), + ) + .unwrap(); + + let plan = plan_schema_migration(&accepted, &desired).unwrap(); + assert!(plan.supported, "additive nullable enum must be supported: {plan:?}"); + assert!( + plan.steps.contains(&AddProperty { + type_kind: SchemaTypeKind::Node, + type_name: "Person".to_string(), + property_name: "role".to_string(), + property_type: PropType::enum_type( + vec!["admin".to_string(), "guest".to_string(), "member".to_string()], + true, + ), + }), + "expected AddProperty with the enum value-set preserved: {plan:?}", + ); + // Negative: an enum add must not be mistaken for a type change. + assert!( + !plan + .steps + .iter() + .any(|step| matches!(step, UnsupportedChange { .. })), + "nullable enum add must not emit UnsupportedChange: {plan:?}", + ); + } + #[test] fn plan_supports_explicit_type_and_property_rename() { let accepted = build_schema_ir( diff --git a/crates/omnigraph/tests/schema_apply.rs b/crates/omnigraph/tests/schema_apply.rs index cc0cae2..5b65e3d 100644 --- a/crates/omnigraph/tests/schema_apply.rs +++ b/crates/omnigraph/tests/schema_apply.rs @@ -60,6 +60,61 @@ async fn apply_schema_noop_returns_not_applied() { assert!(result.steps.is_empty()); } +#[tokio::test] +async fn apply_schema_metadata_only_change_persists_and_reports_applied() { + // A plan whose only step is metadata-class (here `UpdateTypeMetadata` + // from an added `@description`) contributes nothing to the manifest's + // table set, so `manifest_changes` is empty and the Lance manifest + // version does not advance. The migration is still durable: the new + // `_schema.pg` / `_schema.ir.json` / `__schema_state.json` are written + // and renamed into place, `applied` is true, and a reopen sees the new + // schema as the accepted contract (re-planning the same schema is a + // no-op). Enum *widen* rides exactly this path, so pin it here. + // + // Note: `commit_changes_with_actor` still appends a commit-graph node at + // the unchanged manifest version for any metadata-only apply. That is + // pre-existing behavior shared by every `@description`-style change and + // is out of scope here — not introduced by enum migration. + let dir = tempfile::tempdir().unwrap(); + let mut db = init_and_load(&dir).await; + let people_before = count_rows(&db, "node:Person").await; + + let desired = TEST_SCHEMA.replace( + "node Person {", + "node Person @description(\"people in the graph\") {", + ); + + let plan = db.plan_schema(&desired).await.unwrap(); + assert!(plan.supported, "metadata-only plan must be supported: {plan:?}"); + assert!( + plan.steps.iter().any(|step| matches!( + step, + SchemaMigrationStep::UpdateTypeMetadata { + type_kind: SchemaTypeKind::Node, + name, + .. + } if name == "Person" + )), + "expected UpdateTypeMetadata for Person: {plan:?}", + ); + + let result = db.apply_schema(&desired).await.unwrap(); + assert!(result.supported && result.applied, "metadata-only apply must report applied"); + assert_eq!(count_rows(&db, "node:Person").await, people_before); + + // Reopen: the new schema is the accepted contract (passes the + // schema-state validity check) and re-planning the same source is a + // no-op, proving the contract files were persisted. + let uri = dir.path().to_str().unwrap().to_string(); + drop(db); + let reopened = Omnigraph::open(&uri).await.unwrap(); + let replan = reopened.plan_schema(&desired).await.unwrap(); + assert!( + replan.supported && replan.steps.is_empty(), + "after reopen, re-planning the persisted schema must be a no-op: {replan:?}", + ); +} + #[tokio::test] async fn apply_schema_rejects_when_non_main_branch_exists() { let dir = tempfile::tempdir().unwrap(); @@ -418,6 +473,117 @@ async fn apply_schema_rejects_adding_a_required_property_without_backfill() { ); } +#[tokio::test] +async fn apply_schema_adds_a_nullable_enum_property_and_enforces_value_set() { + // The additive-enum contract end-to-end: + // 1. Adding a nullable enum property plans as AddProperty (supported). + // 2. Apply advances the manifest and backfills existing rows with NULL + // for the new column (the stage_overwrite rewrite path). + // 3. The enum value-set declared by the migration is enforced on + // subsequent writes — out-of-set rejected, in-set accepted — proving + // the added enum is a real enum, not just an unconstrained String. + let dir = tempfile::tempdir().unwrap(); + let mut db = init_and_load(&dir).await; + let people_before = count_rows(&db, "node:Person").await; + assert!(people_before > 0, "fixture should seed Person rows"); + let before_version = db + .snapshot_of(ReadTarget::branch("main")) + .await + .unwrap() + .version(); + + // Add `role: enum(admin, guest, member)?` to Person (nullable → no backfill). + let desired = TEST_SCHEMA.replace( + " age: I32?\n}", + " age: I32?\n role: enum(admin, guest, member)?\n}", + ); + + // (1) Plan: supported, emits AddProperty for Person.role, no UnsupportedChange. + let plan = db.plan_schema(&desired).await.unwrap(); + assert!(plan.supported, "nullable enum add must be supported: {plan:?}"); + assert!( + plan.steps.iter().any(|step| matches!( + step, + SchemaMigrationStep::AddProperty { + type_kind: SchemaTypeKind::Node, + type_name, + property_name, + .. + } if type_name == "Person" && property_name == "role" + )), + "expected AddProperty for Person.role: {plan:?}", + ); + assert!( + !plan + .steps + .iter() + .any(|step| matches!(step, SchemaMigrationStep::UnsupportedChange { .. })), + "nullable enum add must not emit UnsupportedChange: {plan:?}", + ); + + let result = db.apply_schema(&desired).await.unwrap(); + assert!(result.supported && result.applied); + + // (2) Manifest advanced; row count unchanged; `role` column present and + // NULL for every pre-existing row. + let after_version = db + .snapshot_of(ReadTarget::branch("main")) + .await + .unwrap() + .version(); + assert!(after_version > before_version, "manifest must advance after add"); + assert_eq!(count_rows(&db, "node:Person").await, people_before); + + let person_batches = read_table(&db, "node:Person").await; + assert!( + person_batches + .iter() + .any(|b| b.schema().column_with_name("role").is_some()), + "Person dataset schema must include the new 'role' column after add", + ); + assert!( + collect_column_strings(&person_batches, "role").is_empty(), + "every pre-existing Person row must read NULL for the added enum column", + ); + + // (3) Enum value-set enforced on writes against the migrated schema. + const SET_ROLE: &str = r#" +query set_role($name: String, $role: String) { + update Person set { role: $role } where name = $name +} +"#; + + // Out-of-set value rejected (same surface as validators.rs). + let err = mutate_main( + &mut db, + SET_ROLE, + "set_role", + ¶ms(&[("$name", "Alice"), ("$role", "superadmin")]), + ) + .await + .unwrap_err(); + assert!( + err.to_string().contains("invalid enum value 'superadmin'"), + "added enum must reject out-of-set values; got: {err}", + ); + + // In-set value accepted and persisted. + mutate_main( + &mut db, + SET_ROLE, + "set_role", + ¶ms(&[("$name", "Alice"), ("$role", "admin")]), + ) + .await + .unwrap(); + let roles_after = collect_column_strings(&read_table(&db, "node:Person").await, "role"); + assert_eq!( + roles_after, + vec!["admin".to_string()], + "only Alice's role should be set, to the accepted enum value", + ); +} + #[tokio::test] async fn plan_schema_for_property_type_narrowing_is_not_supported() { // Symmetric companion to `apply_schema_unsupported_plan_does_not_advance_manifest`,