From 0a27ca6c3d1c4cdd400e54109641a82d5987e872 Mon Sep 17 00:00:00 2001 From: Roland Rodriguez Date: Fri, 25 Sep 2026 14:55:25 -0600 Subject: [PATCH 01/21] chore(release): prepare v0.46.1 --- CHANGELOG.md | 7 ++++++- Cargo.lock | 4 ++-- crates/schema-forge-acton/Cargo.toml | 2 +- crates/schema-forge-cli/Cargo.toml | 2 +- 4 files changed, 10 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 277163d..99251ea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,10 +7,15 @@ is pre-1.0; breaking changes bump the **minor** version per ## [Unreleased] -## [0.46.0] - 2026-09-25 +## [0.46.1] - 2026-09-25 + +This release includes the changes prepared for v0.46.0. The v0.46.0 source tag +is retained, but its binary release was not published. ### Runtime and API behavior +- Apply target schema, record, and field authorization consistently when resolving + related display labels and derived collection IDs, including export labels. - Validate tenant declarations consistently across startup, CLI schema application, and runtime schema changes. Applications with a tenant root must annotate every application schema; built-in system schemas remain shared. diff --git a/Cargo.lock b/Cargo.lock index e0642af..da8ae5e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7599,7 +7599,7 @@ dependencies = [ [[package]] name = "schema-forge-acton" -version = "0.45.0" +version = "0.45.1" dependencies = [ "acton-service", "arc-swap", @@ -7687,7 +7687,7 @@ dependencies = [ [[package]] name = "schema-forge-cli" -version = "0.46.0" +version = "0.46.1" dependencies = [ "acton-service", "assert_cmd", diff --git a/crates/schema-forge-acton/Cargo.toml b/crates/schema-forge-acton/Cargo.toml index e3f790f..0dfc7fb 100644 --- a/crates/schema-forge-acton/Cargo.toml +++ b/crates/schema-forge-acton/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "schema-forge-acton" -version = "0.45.0" +version = "0.45.1" edition = "2021" [dependencies] diff --git a/crates/schema-forge-cli/Cargo.toml b/crates/schema-forge-cli/Cargo.toml index 2dda8f7..c94b95c 100644 --- a/crates/schema-forge-cli/Cargo.toml +++ b/crates/schema-forge-cli/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "schema-forge-cli" -version = "0.46.0" +version = "0.46.1" edition = "2021" [[bin]] From f46ce3aa788576aed5c18917680eb3c964ec0385 Mon Sep 17 00:00:00 2001 From: Roland Rodriguez Date: Fri, 25 Sep 2026 14:56:48 -0600 Subject: [PATCH 02/21] fix(auth): authorize related rows before read enrichment --- .../schema-forge-acton/src/routes/entities.rs | 82 +++-- .../tests/relation_read_authorization.rs | 310 ++++++++++++++++++ 2 files changed, 350 insertions(+), 42 deletions(-) create mode 100644 crates/schema-forge-acton/tests/relation_read_authorization.rs diff --git a/crates/schema-forge-acton/src/routes/entities.rs b/crates/schema-forge-acton/src/routes/entities.rs index 4483c2f..57fea4a 100644 --- a/crates/schema-forge-acton/src/routes/entities.rs +++ b/crates/schema-forge-acton/src/routes/entities.rs @@ -1730,34 +1730,46 @@ async fn execute_entity_query( } /// Apply an operator's record policy before Cedar checks on related rows. -async fn filter_public_relation_entities_with_policy( +pub(super) async fn filter_relation_entities_with_policy( record_policy: Option<&dyn schema_forge_backend::auth::RecordAccessPolicy>, policy_store: &Arc, schema: &SchemaDefinition, + claims: Option<&Claims>, entities: Vec, ) -> Vec { let entities = match record_policy { - Some(policy) => policy.filter_visible_optional(schema, None, entities).await, + Some(policy) => { + policy + .filter_visible_optional(schema, claims, entities) + .await + } None => entities, }; - filter_public_relation_entities(policy_store, schema, entities) + filter_relation_entities(policy_store, schema, claims, entities) } -/// Apply public read authorization to complete related rows before enrichment. +/// Apply caller read authorization to complete related rows before enrichment. /// Both display text and inverse child IDs must respect the target's policies. -fn filter_public_relation_entities( +fn filter_relation_entities( policy_store: &Arc, schema: &SchemaDefinition, + claims: Option<&Claims>, entities: Vec, ) -> Vec { - if check_schema_access(policy_store, schema, None, AccessAction::Read).is_err() { + if check_schema_access(policy_store, schema, claims, AccessAction::Read).is_err() { return Vec::new(); } entities .into_iter() .filter_map(|mut entity| { - if !authorize(policy_store, None, ActionVerb::Read, schema, Some(&entity)) - .is_ok_and(|decision| decision.is_allow()) + if !authorize( + policy_store, + claims, + ActionVerb::Read, + schema, + Some(&entity), + ) + .is_ok_and(|decision| decision.is_allow() && decision.errors.is_empty()) { return None; } @@ -1765,7 +1777,7 @@ fn filter_public_relation_entities( policy_store, &mut entity, schema, - None, + claims, FieldFilterDirection::Read, ); entity @@ -1847,8 +1859,8 @@ async fn resolve_relation_displays( continue; } - // Batched IN query for just the display field. No total_count — - // internal lookup, never paginated. + // Fetch complete rows so record policies and Cedar can inspect all + // attributes before display-field filtering. No count is needed. let id_values: Vec = distinct_ids.into_iter().map(DynamicValue::Text).collect(); let mut display_query = schema_forge_core::query::Query::new(target_def.id.clone()) @@ -1857,9 +1869,6 @@ async fn resolve_relation_displays( values: id_values, }) .without_total_count(); - if claims.is_some() { - display_query.projection = Some(vec!["id".to_string(), display_field.clone()]); - } // Apply tenant scope so we never leak rows the caller couldn't // otherwise see through a direct list call. inject_tenant_scope(&mut display_query, claims, tenant_config, target_def); @@ -1875,7 +1884,7 @@ async fn resolve_relation_displays( return Ok(HashMap::new()); } - let record_access_policy = if claims.is_none() { + let record_access_policy = { let (tx, rx) = oneshot::channel(); forge .send(GetRecordAccessPolicy { @@ -1883,8 +1892,6 @@ async fn resolve_relation_displays( }) .await; ask_forge(rx).await? - } else { - None }; let record_access_policy = record_access_policy.as_deref(); @@ -1905,17 +1912,14 @@ async fn resolve_relation_displays( _ => return (source_fields, HashMap::new()), }; let mut id_to_display: HashMap = HashMap::new(); - let targets = if claims.is_none() { - filter_public_relation_entities_with_policy( - record_access_policy, - policy_store, - target_def, - target_result.entities, - ) - .await - } else { - target_result.entities - }; + let targets = filter_relation_entities_with_policy( + record_access_policy, + policy_store, + target_def, + claims, + target_result.entities, + ) + .await; for target_entity in targets { if let Some(display_value) = target_entity.field(&display_field) { let rendered = display_value_to_string(display_value); @@ -2413,17 +2417,14 @@ async fn populate_derived_collections( continue; }; - // Query: SELECT id, FROM WHERE IN - // (parent_ids). Internal lookup — no total_count needed. + // Fetch complete matching child rows for authorization before extracting + // their IDs and FK. Internal lookup needs no total count. let mut child_query = schema_forge_core::query::Query::new(target_def.id.clone()) .with_filter(Filter::In { path: FieldPath::single(&fk_field_name), values: parent_id_values.clone(), }) .without_total_count(); - if claims.is_some() { - child_query.projection = Some(vec!["id".to_string(), fk_field_name.clone()]); - } inject_tenant_scope(&mut child_query, claims, tenant_config, target_def); jobs.push((target_def, parent_field_name, fk_field_name, child_query)); @@ -2433,7 +2434,7 @@ async fn populate_derived_collections( return Ok(()); } - let record_access_policy = if claims.is_none() { + let record_access_policy = { let (tx, rx) = oneshot::channel(); forge .send(GetRecordAccessPolicy { @@ -2441,8 +2442,6 @@ async fn populate_derived_collections( }) .await; ask_forge(rx).await? - } else { - None }; let record_access_policy = record_access_policy.as_deref(); @@ -2458,17 +2457,16 @@ async fn populate_derived_collections( }) .await; let entities_opt = match ask_forge(rx).await { - Ok(Ok(r)) => Some(if claims.is_none() { - filter_public_relation_entities_with_policy( + Ok(Ok(r)) => Some( + filter_relation_entities_with_policy( record_access_policy, policy_store, target_def, + claims, r.entities, ) - .await - } else { - r.entities - }), + .await, + ), _ => None, }; (parent_field_name, fk_field_name, entities_opt) diff --git a/crates/schema-forge-acton/tests/relation_read_authorization.rs b/crates/schema-forge-acton/tests/relation_read_authorization.rs new file mode 100644 index 0000000..5d76799 --- /dev/null +++ b/crates/schema-forge-acton/tests/relation_read_authorization.rs @@ -0,0 +1,310 @@ +//! Related labels and inverse IDs obey the same read authority as direct reads. +use std::collections::{BTreeMap, HashMap}; +use std::future::Future; +use std::pin::Pin; +use std::sync::Arc; + +use acton_service::{config::Config, middleware::Claims, prelude::ActorHandleInterface}; +use axum::{ + body::Body, + http::{Method, Request, StatusCode}, + Router, +}; +use http_body_util::BodyExt; +use schema_forge_acton::{ + config::SchemaForgeConfig, + messages::{InitForge, ReplyChannel}, + routes::forge_routes, + ForgeActor, +}; +use schema_forge_backend::{auth::RecordAccessPolicy, entity::Entity, SchemaBackend}; +use schema_forge_core::types::{DynamicValue, EntityId, FieldName, SchemaDefinition}; +use schema_forge_surrealdb::SurrealBackend; +use serde_json::{json, Value}; +use tokio::sync::oneshot; +use tower::ServiceExt; + +struct OperatorPolicy; +impl RecordAccessPolicy for OperatorPolicy { + fn filter_visible<'a>( + &'a self, + schema: &'a SchemaDefinition, + claims: &'a Claims, + entities: Vec, + ) -> Pin> + Send + 'a>> { + Box::pin(async move { + assert_eq!(claims.sub, "user:viewer"); + entities + .into_iter() + .filter_map(|mut entity| { + if schema.name.as_str() == "Child" { + if entity.field("blocked") == Some(&DynamicValue::Boolean(true)) { + return None; + } + if entity.field("redacted") == Some(&DynamicValue::Boolean(true)) { + entity.fields.remove("label"); + } + } + Some(entity) + }) + .collect() + }) + } + fn can_modify<'a>( + &'a self, + _: &'a SchemaDefinition, + _: &'a Claims, + _: &'a Entity, + ) -> Pin + Send + 'a>> { + Box::pin(async { false }) + } + fn can_delete<'a>( + &'a self, + _: &'a SchemaDefinition, + _: &'a Claims, + _: &'a Entity, + ) -> Pin + Send + 'a>> { + Box::pin(async { false }) + } +} + +async fn fixture( + read_role: &str, + label_annotation: &str, + parent_annotation: &str, + custom_policy: &str, + operator: bool, +) -> Router { + let mut schemas = schema_forge_dsl::parse(&format!(r#" + @access(read: ["clerk"], write: ["manager"]) + schema Parent {{ title: text selected: -> Child denied: -> Child missing: -> Child linked: -> Child[] children: -> Child[] }} + @display("label") + @access(read: ["{read_role}"], write: ["manager"]) + schema Child {{ label: text {label_annotation} parent: -> Parent {parent_annotation} owner: text @owner blocked: boolean required redacted: boolean required }} + "#)).unwrap(); + schemas[0] + .fields + .iter_mut() + .find(|f| f.name.as_str() == "children") + .unwrap() + .derived_from = Some(FieldName::new("parent").unwrap()); + let backend = Arc::new( + SurrealBackend::connect_memory("related", "related") + .await + .unwrap(), + ); + for schema in &schemas { + let plan = schema_forge_core::migration::DiffEngine::create_new(schema); + backend + .apply_migration(&schema.name, &plan.steps) + .await + .unwrap(); + backend.store_schema_metadata(schema).await.unwrap(); + } + let parent = Entity::with_id( + EntityId::new("parent_one"), + schemas[0].name.clone(), + BTreeMap::from([ + ("title".into(), DynamicValue::Text("Parent".into())), + ( + "selected".into(), + DynamicValue::Ref(EntityId::new("child_good")), + ), + ( + "denied".into(), + DynamicValue::Ref(EntityId::new("child_denied")), + ), + ( + "missing".into(), + DynamicValue::Ref(EntityId::new("child_absent")), + ), + ( + "linked".into(), + DynamicValue::RefArray(vec![ + EntityId::new("child_good"), + EntityId::new("child_denied"), + EntityId::new("child_redacted"), + EntityId::new("child_absent"), + ]), + ), + ]), + ); + backend.create(&parent).await.unwrap(); + for (id, owner, blocked, redacted, label) in [ + ("child_good", "viewer", false, false, "Visible"), + ("child_denied", "other", true, false, "Private"), + ("child_redacted", "viewer", false, true, "Redacted"), + ] { + let child = Entity::with_id( + EntityId::new(id), + schemas[1].name.clone(), + BTreeMap::from([ + ("label".into(), DynamicValue::Text(label.into())), + ("parent".into(), DynamicValue::Ref(parent.id.clone())), + ("owner".into(), DynamicValue::Text(owner.into())), + ("blocked".into(), DynamicValue::Boolean(blocked)), + ("redacted".into(), DynamicValue::Boolean(redacted)), + ]), + ); + backend.create(&child).await.unwrap(); + } + let policy_dir = tempfile::tempdir().unwrap(); + std::fs::write(policy_dir.path().join("related.cedar"), custom_policy).unwrap(); + let store = schema_forge_acton::authz::PolicyStoreSnapshot::from_schemas( + &schemas, + Some(policy_dir.path()), + schema_forge_acton::authz::RoleRanks::empty(), + schema_forge_acton::authz::PrincipalClaimMappings::default(), + ) + .unwrap(); + let service = acton_service::service_builder::ServiceBuilder::new() + .with_config(Config::::default()) + .with_actor::() + .with_actor::() + .build(); + let (tx, rx) = oneshot::channel(); + service + .state() + .actor::() + .unwrap() + .send(InitForge { + registry: schemas + .into_iter() + .map(|s| (s.name.as_str().to_string(), s)) + .collect(), + backend, + tenant_config: None, + record_access_policy: if operator { + Some(Arc::new(OperatorPolicy)) + } else { + None + }, + hook_dispatcher: None, + storage_registry: Default::default(), + policy_store: Some(Arc::new(schema_forge_acton::authz::PolicyStore::new(store))), + custom_policies_dir: None, + reply: ReplyChannel::new(tx), + }) + .await; + tokio::time::timeout(std::time::Duration::from_secs(5), rx) + .await + .unwrap() + .unwrap(); + forge_routes().with_state(service.state().clone()) +} + +async fn read(app: &Router, method: Method, path: &str) -> Value { + let claims = Claims { + sub: "user:viewer".into(), + roles: vec!["clerk".into()], + perms: vec![], + exp: 9_999_999_999, + iat: None, + jti: None, + iss: None, + aud: None, + email: None, + username: None, + custom: HashMap::new(), + }; + let mut request = Request::builder() + .method(method) + .uri(path) + .header("content-type", "application/json") + .body(Body::from("{}")) + .unwrap(); + request.extensions_mut().insert(claims); + let response = app.clone().oneshot(request).await.unwrap(); + let status = response.status(); + let body: Value = + serde_json::from_slice(&response.into_body().collect().await.unwrap().to_bytes()).unwrap(); + assert_eq!(status, StatusCode::OK, "{body}"); + body +} + +async fn parent_views(app: &Router) -> Vec { + let mut views = vec![]; + for (method, path) in [ + (Method::GET, "/schemas/Parent/entities/parent_one"), + (Method::GET, "/schemas/Parent/entities"), + (Method::POST, "/schemas/Parent/entities/query"), + ] { + let body = read(app, method, path).await; + views.push(if body.get("entities").is_some() { + body["entities"][0]["fields"].clone() + } else { + body["fields"].clone() + }); + } + views +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn schema_role_denial_hides_related_labels_and_inverse_ids_on_every_read() { + let app = fixture("manager", "", "", "", false).await; + for fields in parent_views(&app).await { + assert!(fields.get("selected__display").is_none(), "{fields}"); + assert!(fields.get("linked__display").is_none(), "{fields}"); + assert!(fields.get("missing__display").is_none(), "{fields}"); + assert_eq!(fields["children"], json!([])); + } +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn related_rows_use_complete_owner_and_custom_policy_attributes() { + for policy in [ + r#"forbid(principal, action == Action::"ReadChild", resource is Child) when { !context.resource_is_placeholder && resource has owner && resource.owner != principal.id };"#, + r#"forbid(principal, action == Action::"ReadChild", resource is Child) when { !context.resource_is_placeholder && resource.blocked };"#, + ] { + let app = fixture("clerk", "", "", policy, false).await; + for fields in parent_views(&app).await { + assert_eq!(fields["selected__display"], "Visible", "{fields}"); + assert!(fields.get("denied__display").is_none(), "{fields}"); + assert!(fields.get("missing__display").is_none(), "{fields}"); + let children = fields["children"].as_array().unwrap(); + assert_eq!(children.len(), 2, "{fields}"); + assert!(children.contains(&json!("child_good"))); + assert!(!children.contains(&json!("child_denied"))); + assert!( + !fields["linked__display"].to_string().contains("Private"), + "{fields}" + ); + } + } +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn operator_policy_filters_related_rows_and_display_fields() { + let app = fixture("clerk", "", "", "", true).await; + for fields in parent_views(&app).await { + assert_eq!(fields["selected__display"], "Visible", "{fields}"); + assert!(fields.get("denied__display").is_none(), "{fields}"); + let labels = fields["linked__display"].to_string(); + assert!( + !labels.contains("Private") && !labels.contains("Redacted"), + "{fields}" + ); + let children = fields["children"].as_array().unwrap(); + assert_eq!(children.len(), 2, "{fields}"); + assert!(!children.contains(&json!("child_denied"))); + } +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn target_field_restrictions_apply_before_labels_and_inverse_ids() { + for label_annotation in [r#"@field_access(read: ["manager"])"#, "@hidden"] { + let app = fixture( + "clerk", + label_annotation, + r#"@field_access(read: ["manager"])"#, + "", + false, + ) + .await; + for fields in parent_views(&app).await { + assert!(fields.get("selected__display").is_none(), "{fields}"); + assert!(fields.get("linked__display").is_none(), "{fields}"); + assert_eq!(fields["children"], json!([])); + } + } +} From 7ba7dd4043a4b0a506ddae84436c1349c1687c98 Mon Sep 17 00:00:00 2001 From: Roland Rodriguez Date: Fri, 25 Sep 2026 14:58:00 -0600 Subject: [PATCH 03/21] test(auth): use stable valid identifiers for related records --- .../tests/relation_read_authorization.rs | 33 +++++++++++-------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/crates/schema-forge-acton/tests/relation_read_authorization.rs b/crates/schema-forge-acton/tests/relation_read_authorization.rs index 5d76799..3d1a31c 100644 --- a/crates/schema-forge-acton/tests/relation_read_authorization.rs +++ b/crates/schema-forge-acton/tests/relation_read_authorization.rs @@ -24,6 +24,10 @@ use serde_json::{json, Value}; use tokio::sync::oneshot; use tower::ServiceExt; +fn fixture_id(prefix: &str) -> EntityId { + EntityId::parse(&format!("{prefix}_00000000000000000000000000")).unwrap() +} + struct OperatorPolicy; impl RecordAccessPolicy for OperatorPolicy { fn filter_visible<'a>( @@ -102,29 +106,29 @@ async fn fixture( backend.store_schema_metadata(schema).await.unwrap(); } let parent = Entity::with_id( - EntityId::new("parent_one"), + fixture_id("parent_one"), schemas[0].name.clone(), BTreeMap::from([ ("title".into(), DynamicValue::Text("Parent".into())), ( "selected".into(), - DynamicValue::Ref(EntityId::new("child_good")), + DynamicValue::Ref(fixture_id("child_good")), ), ( "denied".into(), - DynamicValue::Ref(EntityId::new("child_denied")), + DynamicValue::Ref(fixture_id("child_denied")), ), ( "missing".into(), - DynamicValue::Ref(EntityId::new("child_absent")), + DynamicValue::Ref(fixture_id("child_absent")), ), ( "linked".into(), DynamicValue::RefArray(vec![ - EntityId::new("child_good"), - EntityId::new("child_denied"), - EntityId::new("child_redacted"), - EntityId::new("child_absent"), + fixture_id("child_good"), + fixture_id("child_denied"), + fixture_id("child_redacted"), + fixture_id("child_absent"), ]), ), ]), @@ -136,7 +140,7 @@ async fn fixture( ("child_redacted", "viewer", false, true, "Redacted"), ] { let child = Entity::with_id( - EntityId::new(id), + fixture_id(id), schemas[1].name.clone(), BTreeMap::from([ ("label".into(), DynamicValue::Text(label.into())), @@ -225,7 +229,10 @@ async fn read(app: &Router, method: Method, path: &str) -> Value { async fn parent_views(app: &Router) -> Vec { let mut views = vec![]; for (method, path) in [ - (Method::GET, "/schemas/Parent/entities/parent_one"), + ( + Method::GET, + "/schemas/Parent/entities/parent_one_00000000000000000000000000", + ), (Method::GET, "/schemas/Parent/entities"), (Method::POST, "/schemas/Parent/entities/query"), ] { @@ -263,8 +270,8 @@ async fn related_rows_use_complete_owner_and_custom_policy_attributes() { assert!(fields.get("missing__display").is_none(), "{fields}"); let children = fields["children"].as_array().unwrap(); assert_eq!(children.len(), 2, "{fields}"); - assert!(children.contains(&json!("child_good"))); - assert!(!children.contains(&json!("child_denied"))); + assert!(children.contains(&json!(fixture_id("child_good")))); + assert!(!children.contains(&json!(fixture_id("child_denied")))); assert!( !fields["linked__display"].to_string().contains("Private"), "{fields}" @@ -286,7 +293,7 @@ async fn operator_policy_filters_related_rows_and_display_fields() { ); let children = fields["children"].as_array().unwrap(); assert_eq!(children.len(), 2, "{fields}"); - assert!(!children.contains(&json!("child_denied"))); + assert!(!children.contains(&json!(fixture_id("child_denied")))); } } From 736a333de1ce0ed69bc2c6ac7da244fa7590b198 Mon Sep 17 00:00:00 2001 From: Roland Rodriguez Date: Fri, 25 Sep 2026 14:58:46 -0600 Subject: [PATCH 04/21] fix(auth): authorize original related rows before operator redaction --- .../schema-forge-acton/src/routes/entities.rs | 44 +++++++------------ .../tests/relation_read_authorization.rs | 24 +++++++++- 2 files changed, 38 insertions(+), 30 deletions(-) diff --git a/crates/schema-forge-acton/src/routes/entities.rs b/crates/schema-forge-acton/src/routes/entities.rs index 57fea4a..4100da5 100644 --- a/crates/schema-forge-acton/src/routes/entities.rs +++ b/crates/schema-forge-acton/src/routes/entities.rs @@ -1729,7 +1729,8 @@ async fn execute_entity_query( }) } -/// Apply an operator's record policy before Cedar checks on related rows. +/// Authorize complete related rows before operator and field-level redaction. +/// Both display text and inverse child IDs must respect the target's policies. pub(super) async fn filter_relation_entities_with_policy( record_policy: Option<&dyn schema_forge_backend::auth::RecordAccessPolicy>, policy_store: &Arc, @@ -1737,6 +1738,18 @@ pub(super) async fn filter_relation_entities_with_policy( claims: Option<&Claims>, entities: Vec, ) -> Vec { + if check_schema_access(policy_store, schema, claims, AccessAction::Read).is_err() { + return Vec::new(); + } + // Operator policies may redact fields used by Cedar conditions. Evaluate + // the unmodified resource first so redaction cannot change the decision. + let entities = entities + .into_iter() + .filter(|entity| { + authorize(policy_store, claims, ActionVerb::Read, schema, Some(entity)) + .is_ok_and(|decision| decision.is_allow() && decision.errors.is_empty()) + }) + .collect(); let entities = match record_policy { Some(policy) => { policy @@ -1745,34 +1758,9 @@ pub(super) async fn filter_relation_entities_with_policy( } None => entities, }; - filter_relation_entities(policy_store, schema, claims, entities) -} - -/// Apply caller read authorization to complete related rows before enrichment. -/// Both display text and inverse child IDs must respect the target's policies. -fn filter_relation_entities( - policy_store: &Arc, - schema: &SchemaDefinition, - claims: Option<&Claims>, - entities: Vec, -) -> Vec { - if check_schema_access(policy_store, schema, claims, AccessAction::Read).is_err() { - return Vec::new(); - } entities .into_iter() - .filter_map(|mut entity| { - if !authorize( - policy_store, - claims, - ActionVerb::Read, - schema, - Some(&entity), - ) - .is_ok_and(|decision| decision.is_allow() && decision.errors.is_empty()) - { - return None; - } + .map(|mut entity| { filter_entity_fields( policy_store, &mut entity, @@ -1783,7 +1771,7 @@ fn filter_relation_entities( entity .fields .retain(|name, _| !schema.field(name).is_some_and(|field| field.is_hidden())); - Some(entity) + entity }) .collect() } diff --git a/crates/schema-forge-acton/tests/relation_read_authorization.rs b/crates/schema-forge-acton/tests/relation_read_authorization.rs index 3d1a31c..6815a20 100644 --- a/crates/schema-forge-acton/tests/relation_read_authorization.rs +++ b/crates/schema-forge-acton/tests/relation_read_authorization.rs @@ -28,7 +28,9 @@ fn fixture_id(prefix: &str) -> EntityId { EntityId::parse(&format!("{prefix}_00000000000000000000000000")).unwrap() } -struct OperatorPolicy; +struct OperatorPolicy { + redact_authorization_attribute: bool, +} impl RecordAccessPolicy for OperatorPolicy { fn filter_visible<'a>( &'a self, @@ -42,6 +44,9 @@ impl RecordAccessPolicy for OperatorPolicy { .into_iter() .filter_map(|mut entity| { if schema.name.as_str() == "Child" { + if self.redact_authorization_attribute { + entity.fields.remove("blocked"); + } if entity.field("blocked") == Some(&DynamicValue::Boolean(true)) { return None; } @@ -179,7 +184,9 @@ async fn fixture( backend, tenant_config: None, record_access_policy: if operator { - Some(Arc::new(OperatorPolicy)) + Some(Arc::new(OperatorPolicy { + redact_authorization_attribute: !custom_policy.is_empty(), + })) } else { None }, @@ -315,3 +322,16 @@ async fn target_field_restrictions_apply_before_labels_and_inverse_ids() { } } } + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn operator_redaction_cannot_erase_attributes_before_cedar_read_checks() { + let policy = r#"forbid(principal, action == Action::"ReadChild", resource is Child) when { !context.resource_is_placeholder && resource.blocked };"#; + let app = fixture("clerk", "", "", policy, true).await; + for fields in parent_views(&app).await { + assert_eq!(fields["selected__display"], "Visible", "{fields}"); + assert!(fields.get("denied__display").is_none(), "{fields}"); + let children = fields["children"].as_array().unwrap(); + assert_eq!(children.len(), 2, "{fields}"); + assert!(!children.contains(&json!(fixture_id("child_denied")))); + } +} From 05617710f6011361f902aec4b29a953d9295c169 Mon Sep 17 00:00:00 2001 From: Roland Rodriguez Date: Fri, 25 Sep 2026 14:58:46 -0600 Subject: [PATCH 05/21] fix(export): authorize target rows before rendering relation labels --- .../schema-forge-acton/src/routes/export.rs | 23 +- .../tests/export_integration.rs | 251 ++++++++++++++++++ 2 files changed, 265 insertions(+), 9 deletions(-) diff --git a/crates/schema-forge-acton/src/routes/export.rs b/crates/schema-forge-acton/src/routes/export.rs index 6574c56..823f04b 100644 --- a/crates/schema-forge-acton/src/routes/export.rs +++ b/crates/schema-forge-acton/src/routes/export.rs @@ -544,9 +544,7 @@ pub async fn prepare_export( // Resolve relation displays, then strip read-restricted fields per row // (dynamic half of the intersection). - let display_map = - resolve_export_displays(forge, schema_def, &visible, &columns, claims, tenant_config) - .await?; + let display_map = resolve_export_displays(ctx, schema_def, &visible, &columns).await?; for entity in &mut visible { filter_entity_fields( policy_store, @@ -1233,15 +1231,14 @@ fn may_read_export_job(owner: Option<&str>, caller: Option<&str>) -> bool { /// `@exportable` (present in `columns`) and relations get resolved; everything /// else is absent (the serializer then falls back to the raw id). async fn resolve_export_displays( - forge: &acton_service::prelude::ActorHandle, + ctx: &ExportContext<'_>, schema: &SchemaDefinition, visible: &[Entity], columns: &[(String, Option)], - claims: Option<&Claims>, - tenant_config: &Option, ) -> Result>, ForgeError> { use std::collections::HashSet; + let forge = ctx.forge; let mut out: HashMap> = HashMap::new(); if visible.is_empty() { return Ok(out); @@ -1288,8 +1285,8 @@ async fn resolve_export_displays( values: id_values, }) .without_total_count(); - display_query.projection = Some(vec!["id".to_string(), display_field.clone()]); - inject_tenant_scope(&mut display_query, claims, tenant_config, &target_def); + // Authorization needs the complete row, including owner and policy fields. + inject_tenant_scope(&mut display_query, ctx.claims, ctx.tenant_config, &target_def); let (tx, rx) = oneshot::channel(); forge @@ -1299,9 +1296,17 @@ async fn resolve_export_displays( }) .await; let display_result = ask_forge(rx).await?.map_err(ForgeError::from)?; + let targets = super::entities::filter_relation_entities_with_policy( + ctx.record_access_policy.as_deref(), + ctx.policy_store, + &target_def, + ctx.claims, + display_result.entities, + ) + .await; let mut id_to_display: HashMap = HashMap::new(); - for target_entity in display_result.entities { + for target_entity in targets { if let Some(value) = target_entity.field(&display_field) { id_to_display.insert(target_entity.id.to_string(), display_scalar(value)); } diff --git a/crates/schema-forge-acton/tests/export_integration.rs b/crates/schema-forge-acton/tests/export_integration.rs index ee45527..2e48813 100644 --- a/crates/schema-forge-acton/tests/export_integration.rs +++ b/crates/schema-forge-acton/tests/export_integration.rs @@ -710,3 +710,254 @@ async fn empty_result_streams_header_only_csv() { assert_eq!(text.lines().count(), 1); assert_eq!(text.lines().next().unwrap(), "name,notes"); } + +struct ExportTargetPolicy; + +impl schema_forge_backend::auth::RecordAccessPolicy for ExportTargetPolicy { + fn filter_visible<'a>( + &'a self, + _schema: &'a SchemaDefinition, + claims: &'a Claims, + entities: Vec, + ) -> std::pin::Pin< + Box< + dyn std::future::Future> + Send + 'a, + >, + > { + Box::pin(async move { + assert_eq!(claims.sub, "user:test-user"); + entities + .into_iter() + .filter(|entity| { + entity.field("label") + != Some(&schema_forge_core::types::DynamicValue::Text( + "operator-secret".into(), + )) + }) + .collect() + }) + } + + fn can_modify<'a>( + &'a self, + _schema: &'a SchemaDefinition, + _claims: &'a Claims, + _entity: &'a schema_forge_backend::entity::Entity, + ) -> std::pin::Pin + Send + 'a>> { + Box::pin(async { false }) + } + + fn can_delete<'a>( + &'a self, + _schema: &'a SchemaDefinition, + _claims: &'a Claims, + _entity: &'a schema_forge_backend::entity::Entity, + ) -> std::pin::Pin + Send + 'a>> { + Box::pin(async { false }) + } +} + +#[tokio::test] +async fn export_relation_labels_respect_target_schema_row_field_and_operator_access() { + use schema_forge_acton::authz::{ + PolicyStore, PolicyStoreSnapshot, PrincipalClaimMappings, RoleRanks, + }; + use schema_forge_acton::routes::export::{materialize_export, prepare_export, ExportContext}; + + let schemas = schema_forge_dsl::parse( + r#" + @access(read: ["manager"]) + @display("label") + schema RestrictedTarget { label: text } + @access(read: ["viewer"]) + @display("label") + schema FieldTarget { label: text @field_access(read: ["manager"]) } + @access(read: ["viewer"]) + @display("label") + schema HiddenTarget { label: text @hidden } + @access(read: ["viewer"]) + @display("label") + schema RowTarget { label: text blocked: boolean } + @access(read: ["viewer"]) + @display("label") + schema VisibleTarget { label: text } + @access(read: ["viewer"]) + schema ExportLinks { + denied: -> RestrictedTarget @exportable + field_denied: -> FieldTarget @exportable + hidden: -> HiddenTarget @exportable + row_denied: -> RowTarget @exportable + allowed: -> RowTarget @exportable + many: -> RowTarget[] @exportable + operator_denied: -> VisibleTarget @exportable + } + "#, + ) + .unwrap(); + let backend = Arc::new( + SurrealBackend::connect_memory("test", "export-targets") + .await + .unwrap(), + ); + for schema in &schemas { + provision(&backend, schema).await; + } + let registry = schemas + .iter() + .map(|schema| (schema.name.to_string(), schema.clone())) + .collect(); + let state = build_state_with_config(backend, registry, SchemaForgeConfig::default()).await; + let admin = app_with_claims(state.clone(), make_claims(&["platform_admin"])); + let mut ids = HashMap::new(); + for (key, schema, fields) in [ + ( + "denied", + "RestrictedTarget", + serde_json::json!({"label": "schema-secret"}), + ), + ( + "field_denied", + "FieldTarget", + serde_json::json!({"label": "field-secret"}), + ), + ( + "hidden", + "HiddenTarget", + serde_json::json!({"label": "hidden-secret"}), + ), + ( + "row_denied", + "RowTarget", + serde_json::json!({"label": "row-secret", "blocked": true}), + ), + ( + "allowed", + "RowTarget", + serde_json::json!({"label": "readable-label", "blocked": false}), + ), + ( + "operator_denied", + "VisibleTarget", + serde_json::json!({"label": "operator-secret"}), + ), + ] { + let (status, result) = json_request( + &admin, + Method::POST, + &format!("/schemas/{schema}/entities"), + Some(serde_json::json!({"fields": fields})), + ) + .await; + assert_eq!(status, StatusCode::CREATED, "{result}"); + ids.insert(key, result["id"].as_str().unwrap().to_string()); + } + let mut fields = serde_json::Map::new(); + for (key, id) in &ids { + fields.insert((*key).into(), serde_json::json!(id)); + } + fields.insert( + "many".into(), + serde_json::json!([ids["row_denied"], ids["allowed"]]), + ); + let (status, result) = json_request( + &admin, + Method::POST, + "/schemas/ExportLinks/entities", + Some(serde_json::json!({"fields": fields})), + ) + .await; + assert_eq!(status, StatusCode::CREATED, "{result}"); + + let policies = tempfile::tempdir().unwrap(); + std::fs::write( + policies.path().join("rows.cedar"), + r#" + forbid(principal, action == Action::"ReadRowTarget", resource is RowTarget) + when { !context.resource_is_placeholder && resource has blocked && resource.blocked }; + "#, + ) + .unwrap(); + let policy_store = Arc::new(PolicyStore::new( + PolicyStoreSnapshot::from_schemas( + &schemas, + Some(policies.path()), + RoleRanks::empty(), + PrincipalClaimMappings::default(), + ) + .unwrap(), + )); + let record_policy: Option> = + Some(Arc::new(ExportTargetPolicy)); + let forge = state.actor::().unwrap(); + let claims = make_claims(&["viewer"]); + let tenant_config = None; + let context = ExportContext { + forge: &forge, + claims: Some(&claims), + tenant_config: &tenant_config, + policy_store: &policy_store, + record_access_policy: &record_policy, + }; + let schema = schemas + .iter() + .find(|schema| schema.name.as_str() == "ExportLinks") + .unwrap(); + let prepared = prepare_export(&context, schema, None, None, 100) + .await + .unwrap(); + assert_eq!(prepared.entities.len(), 1); + for field in [ + "denied", + "field_denied", + "hidden", + "row_denied", + "operator_denied", + ] { + assert!( + !prepared.display_map.contains_key(field), + "unauthorized label for {field}" + ); + } + assert_eq!( + prepared.display_map["allowed"][&ids["allowed"]], + "readable-label" + ); + assert_eq!(prepared.display_map["many"].len(), 1); + assert_eq!( + prepared.display_map["many"][&ids["allowed"]], + "readable-label" + ); + + for format in [ExportFormat::Csv, ExportFormat::Ndjson, ExportFormat::Xlsx] { + let artifact = materialize_export(&context, schema, format, None, None, 100) + .await + .unwrap(); + let text = if format == ExportFormat::Xlsx { + use std::io::Read; + let mut archive = zip::ZipArchive::new(std::io::Cursor::new(artifact.bytes)).unwrap(); + let mut text = String::new(); + for index in 0..archive.len() { + let mut entry = archive.by_index(index).unwrap(); + if entry.name().ends_with(".xml") { + entry.read_to_string(&mut text).unwrap(); + } + } + text + } else { + String::from_utf8(artifact.bytes).unwrap() + }; + assert!(text.contains("readable-label")); + for secret in [ + "schema-secret", + "field-secret", + "hidden-secret", + "row-secret", + "operator-secret", + ] { + assert!( + !text.contains(secret), + "unauthorized label in {format:?}: {secret}" + ); + } + } +} From bd3aa449878129f7813c862dce09049f0e4dbd11 Mon Sep 17 00:00:00 2001 From: Roland Rodriguez Date: Fri, 25 Sep 2026 15:02:54 -0600 Subject: [PATCH 06/21] chore(release): version migration fixes for v0.46.1 --- Cargo.lock | 4 ++-- crates/schema-forge-core/Cargo.toml | 2 +- crates/schema-forge-postgres/Cargo.toml | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index da8ae5e..7812fb3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7731,7 +7731,7 @@ dependencies = [ [[package]] name = "schema-forge-core" -version = "0.19.0" +version = "0.19.1" dependencies = [ "base64", "chrono", @@ -7772,7 +7772,7 @@ dependencies = [ [[package]] name = "schema-forge-postgres" -version = "0.14.0" +version = "0.14.1" dependencies = [ "arc-swap", "argon2", diff --git a/crates/schema-forge-core/Cargo.toml b/crates/schema-forge-core/Cargo.toml index 43837fc..2b29928 100644 --- a/crates/schema-forge-core/Cargo.toml +++ b/crates/schema-forge-core/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "schema-forge-core" -version = "0.19.0" +version = "0.19.1" edition = "2021" [dependencies] diff --git a/crates/schema-forge-postgres/Cargo.toml b/crates/schema-forge-postgres/Cargo.toml index 9242d23..d165ae0 100644 --- a/crates/schema-forge-postgres/Cargo.toml +++ b/crates/schema-forge-postgres/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "schema-forge-postgres" -version = "0.14.0" +version = "0.14.1" edition = "2021" [dependencies] From a15b14c0183b905007ef9801ac6db000176e183d Mon Sep 17 00:00:00 2001 From: Roland Rodriguez Date: Fri, 25 Sep 2026 15:02:46 -0600 Subject: [PATCH 07/21] fix(site): render relation labels and omit unused field helpers --- .../src/commands/site/context.rs | 24 ++++++++ .../src/commands/site/vendor.rs | 9 +++ .../src/app/pages/detail.generated.tsx.jinja | 4 ++ .../src/app/pages/list.generated.tsx.jinja | 6 ++ .../playwright/tests/relation-labels.spec.ts | 59 +++++++++++++++++++ .../tests/site_e2e/rendering.schema | 26 ++++++++ crates/schema-forge-cli/tests/site_e2e/run.sh | 5 +- .../schema-forge-cli/tests/site_generate.rs | 34 +++++++++++ 8 files changed, 166 insertions(+), 1 deletion(-) create mode 100644 crates/schema-forge-cli/tests/site_e2e/playwright/tests/relation-labels.spec.ts create mode 100644 crates/schema-forge-cli/tests/site_e2e/rendering.schema diff --git a/crates/schema-forge-cli/src/commands/site/context.rs b/crates/schema-forge-cli/src/commands/site/context.rs index 7305a87..8e257c1 100644 --- a/crates/schema-forge-cli/src/commands/site/context.rs +++ b/crates/schema-forge-cli/src/commands/site/context.rs @@ -153,6 +153,12 @@ pub struct EntityView { /// always render through `formatFieldValue`, so nested relations do /// not require the import. pub has_relation_link: bool, + /// Detail rendering includes at least one generic formatter call. + pub detail_uses_formatter: bool, + /// Detail rendering includes a generic field's empty-value guard. + pub detail_uses_is_empty: bool, + /// Visible list cells include at least one generic formatter call. + pub list_uses_formatter: bool, /// `true` iff any top-level field is `kind == "json"`. The /// `normalize…Payload` helper only consults its `form` argument inside /// the JSON branch (to surface parse errors via `setError`); when no @@ -219,6 +225,16 @@ impl EntityView { } let has_form_fields = fields.iter().any(|field| !field.computed && !field.derived); + let detail_uses_is_empty = fields.iter().any(|field| { + field.kind != "composite" && field.kind != "file" && !uses_relation_label(field) + }); + let detail_uses_formatter = detail_uses_is_empty + || fields.iter().any(|field| field.kind == "composite" && !field.sub_fields.is_empty()); + let list_uses_formatter = fields.iter().any(|field| { + matches!(field.list_placement.as_str(), "primary" | "column") + && field.kind != "enum" + && !uses_relation_label(field) + }); let has_form_controls = fields.iter().any(has_form_control); let pascal = name.to_pascal_case(); Ok(Self { @@ -233,6 +249,9 @@ impl EntityView { display_field, has_relation_one, has_relation_link, + detail_uses_formatter, + detail_uses_is_empty, + list_uses_formatter, has_json_field, has_file_field, has_form_file_field, @@ -242,6 +261,11 @@ impl EntityView { } } +fn uses_relation_label(field: &FieldView) -> bool { + matches!(field.kind.as_str(), "relation_one" | "relation_many") + && field.relation_display_field.is_some() +} + fn has_form_file(field: &FieldView) -> bool { !field.computed && !field.derived diff --git a/crates/schema-forge-cli/src/commands/site/vendor.rs b/crates/schema-forge-cli/src/commands/site/vendor.rs index 460b849..2df2025 100644 --- a/crates/schema-forge-cli/src/commands/site/vendor.rs +++ b/crates/schema-forge-cli/src/commands/site/vendor.rs @@ -555,6 +555,15 @@ type RelationRow = { id: string; [key: string]: unknown } /// 2. first string-valued field we encounter /// 3. the entity id itself function labelFor(row: RelationRow, displayField?: string): string { + if (displayField) { + const display = row[`${displayField}__display`] + if (typeof display === "string" && display.length > 0) return display + if (Array.isArray(display)) { + const ids = Array.isArray(row[displayField]) ? row[displayField] as unknown[] : [] + const labels = ids.map((id, index) => typeof display[index] === "string" ? display[index] : String(id)) + if (labels.length > 0) return labels.join(", ") + } + } if (displayField && typeof row[displayField] === "string") { return `${row[displayField] as string}` } diff --git a/crates/schema-forge-cli/templates/site/src/app/pages/detail.generated.tsx.jinja b/crates/schema-forge-cli/templates/site/src/app/pages/detail.generated.tsx.jinja index fead402..e509c76 100644 --- a/crates/schema-forge-cli/templates/site/src/app/pages/detail.generated.tsx.jinja +++ b/crates/schema-forge-cli/templates/site/src/app/pages/detail.generated.tsx.jinja @@ -16,18 +16,22 @@ import { Link } from "react-router-dom" import { AttachmentDownload, type FileAttachment } from "@/components/ui/file-upload" {%- endif %} import type { {{ entity.pascal }} } from "@/generated/entity-types" +{%- if entity.detail_uses_formatter %} import { formatFieldValue } from "@/generated/formatters" +{%- endif %} function specNum(n: number): string { return String(n).padStart(2, "0") } +{% if entity.detail_uses_is_empty %} function isEmpty(v: unknown): boolean { if (v === null || v === undefined) return true if (typeof v === "string" && v.trim() === "") return true if (Array.isArray(v) && v.length === 0) return true return false } +{% endif %} export function {{ entity.pascal }}DetailRows({ data }: { data: {{ entity.pascal }} }) { return ( diff --git a/crates/schema-forge-cli/templates/site/src/app/pages/list.generated.tsx.jinja b/crates/schema-forge-cli/templates/site/src/app/pages/list.generated.tsx.jinja index c026c62..12fd06f 100644 --- a/crates/schema-forge-cli/templates/site/src/app/pages/list.generated.tsx.jinja +++ b/crates/schema-forge-cli/templates/site/src/app/pages/list.generated.tsx.jinja @@ -8,7 +8,9 @@ import { Link } from "react-router-dom" import type { ColumnDef } from "@tanstack/react-table" import type { {{ entity.pascal }} } from "@/generated/entity-types" +{%- if entity.list_uses_formatter %} import { formatFieldValue } from "@/generated/formatters" +{%- endif %} // Semantic color tokens from `@enum_colors(...)` mapped to Tailwind badge // classes. Kept inline so every referenced class is picked up by the JIT @@ -86,6 +88,10 @@ export const columns: ColumnDef<{{ entity.pascal }}>[] = [ > {%- if f.kind == "enum" %} +{%- elif f.kind == "relation_one" and f.relation_display_field %} + {row.original.{{ f.leaf }}__display ?? row.original.{{ f.leaf }} ?? "—"} +{%- elif f.kind == "relation_many" and f.relation_display_field %} + {(row.original.{{ f.leaf }} ?? []).map((id, index) => row.original.{{ f.leaf }}__display?.[index] ?? id).join(", ") || "—"} {%- else %} {formatFieldValue(row.original.{{ f.leaf }}, { kind: "{{ f.kind }}", diff --git a/crates/schema-forge-cli/tests/site_e2e/playwright/tests/relation-labels.spec.ts b/crates/schema-forge-cli/tests/site_e2e/playwright/tests/relation-labels.spec.ts new file mode 100644 index 0000000..e7f81bc --- /dev/null +++ b/crates/schema-forge-cli/tests/site_e2e/playwright/tests/relation-labels.spec.ts @@ -0,0 +1,59 @@ +import { expect, test, type Page } from "@playwright/test" + +async function session(page: Page) { + await page.addInitScript(() => { + sessionStorage.setItem("schemaforge.token", "relation-test-token") + sessionStorage.setItem("schemaforge.token_expires_at", new Date(Date.now() + 60 * 60 * 1000).toISOString()) + sessionStorage.setItem("schemaforge.roles", JSON.stringify(["member"])) + }) + await page.route("**/api/v1/forge/auth/me", route => route.fulfill({ json: { + user_id: "member", roles: ["member"], tenant_chain: [], active_tenant: null, + } })) + await page.route("**/api/v1/forge/schemas", route => route.fulfill({ json: { + schemas: ["PrimaryJoin", "ExplicitJoin", "ManyJoin", "JoinReference"].map(name => ({ + name, annotations: [], permissions: { create: true }, + })), count: 4, + } })) +} + +for (const [schema, path] of [["PrimaryJoin", "primary-join"], ["ExplicitJoin", "explicit-join"]]) { + test(`${schema} primary relation label links to its own row with an ID fallback`, async ({ page }) => { + await session(page) + await page.route(`**/api/v1/forge/schemas/${schema}/entities**`, route => route.fulfill({ json: { + entities: [ + { id: "join_one", schema, fields: { company: "company_one", company__display: "Blue widget" } }, + { id: "join_two", schema, fields: { company: "company_two" } }, + ], count: 2, permissions: { create: true }, + } })) + await page.goto(`/app/${path}`) + await expect(page.getByRole("link", { name: "Blue widget", exact: true })).toHaveAttribute("href", `/app/${path}/join_one`) + await expect(page.getByRole("link", { name: "company_two", exact: true })).toHaveAttribute("href", `/app/${path}/join_two`) + }) +} + +test("many primary relation labels retain per-item ID fallbacks", async ({ page }) => { + await session(page) + await page.route("**/api/v1/forge/schemas/ManyJoin/entities**", route => route.fulfill({ json: { + entities: [{ id: "join_many", schema: "ManyJoin", fields: { + companies: ["company_one", "company_two"], companies__display: ["Blue widget", null], + } }], count: 1, permissions: { create: true }, + } })) + await page.goto("/app/many-join") + await expect(page.getByRole("link", { name: "Blue widget, company_two", exact: true })).toHaveAttribute("href", "/app/many-join/join_many") +}) + +test("relation picker prefers the display companion of a relation-valued display field", async ({ page }) => { + await session(page) + await page.route("**/api/v1/forge/schemas/PrimaryJoin/entities**", route => route.fulfill({ json: { + entities: [ + { id: "join_one", schema: "PrimaryJoin", fields: { company: "company_one", company__display: "Blue widget" } }, + { id: "join_two", schema: "PrimaryJoin", fields: { company: "company_two" } }, + ], count: 2, + } })) + await page.goto("/app/join-reference/new") + const picker = page.getByRole("combobox", { name: /^entry/i }) + await expect(picker.locator('option[value="join_one"]')).toHaveText("Blue widget") + await expect(picker.locator('option[value="join_two"]')).toHaveText("company_two") + await picker.selectOption("join_one") + await expect(picker).toHaveValue("join_one") +}) diff --git a/crates/schema-forge-cli/tests/site_e2e/rendering.schema b/crates/schema-forge-cli/tests/site_e2e/rendering.schema new file mode 100644 index 0000000..4f5e882 --- /dev/null +++ b/crates/schema-forge-cli/tests/site_e2e/rendering.schema @@ -0,0 +1,26 @@ +// Generation fixtures use mocked browser APIs and need no storage backend. +@display("company") +schema PrimaryJoin { + company: -> Company required +} + +schema HiddenJoin { + company: -> Company required +} + +schema ExplicitJoin { + company: -> Company required @list(primary) +} + +@display("companies") +schema ManyJoin { + companies: -> Company[] +} + +schema JoinReference { + entry: -> PrimaryJoin +} + +schema FileOnly { + document: file(bucket: "documents", max_size: "5MB", mime: ["application/pdf"]) +} diff --git a/crates/schema-forge-cli/tests/site_e2e/run.sh b/crates/schema-forge-cli/tests/site_e2e/run.sh index 406d430..5c395dd 100755 --- a/crates/schema-forge-cli/tests/site_e2e/run.sh +++ b/crates/schema-forge-cli/tests/site_e2e/run.sh @@ -23,6 +23,9 @@ SITE_DIR="$TMP_ROOT/site" mkdir -p "$SCHEMAS_DIR" cp "$SCRIPT_DIR/demo.schema" "$SCHEMAS_DIR/demo.schema" +SITE_SCHEMAS_DIR="$TMP_ROOT/site-schemas" +mkdir -p "$SITE_SCHEMAS_DIR" +cp "$SCRIPT_DIR/demo.schema" "$SCRIPT_DIR/rendering.schema" "$SITE_SCHEMAS_DIR/" # ---------- pick two free ports ---------- pick_port() { @@ -42,7 +45,7 @@ cargo build --package schema-forge-cli --bin schemaforge --quiet # ---------- generate the site ---------- ./target/debug/schemaforge site generate \ - --schema-dir "$SCHEMAS_DIR" \ + --schema-dir "$SITE_SCHEMAS_DIR" \ --out-dir "$SITE_DIR" \ --name 'Acme "Operations" & ' \ --title-suffix 'Workspace' diff --git a/crates/schema-forge-cli/tests/site_generate.rs b/crates/schema-forge-cli/tests/site_generate.rs index 708d471..aba23b4 100644 --- a/crates/schema-forge-cli/tests/site_generate.rs +++ b/crates/schema-forge-cli/tests/site_generate.rs @@ -760,3 +760,37 @@ fn generated_extended_fields_and_authority_survive_regeneration() { "// user-owned edit shell\n" ); } + +#[test] +fn relation_and_file_only_pages_emit_only_used_formatters() { + let tmp = TempDir::new().unwrap(); + let schema_dir = tmp.path().join("schemas"); + let fixtures = format!( + "{}\n{}", + include_str!("site_e2e/demo.schema"), + include_str!("site_e2e/rendering.schema") + ); + write_schemas(&schema_dir, &fixtures); + for (name, path) in [ + ("PrimaryJoin", "primary-join"), + ("HiddenJoin", "hidden-join"), + ("ExplicitJoin", "explicit-join"), + ("ManyJoin", "many-join"), + ("FileOnly", "file-only"), + ] { + let out_dir = tmp.path().join(path); + run_generate(&schema_dir, &out_dir, name, &[]).assert().success(); + let detail = fs::read_to_string(out_dir.join(format!("src/app/pages/{path}/detail.generated.tsx"))).unwrap(); + let list = fs::read_to_string(out_dir.join(format!("src/app/pages/{path}/list.generated.tsx"))).unwrap(); + assert!(!detail.contains("formatFieldValue"), "{name}: {detail}"); + assert!(!detail.contains("function isEmpty"), "{name}: {detail}"); + assert!(!list.contains("formatFieldValue"), "{name}: {list}"); + if matches!(name, "PrimaryJoin" | "ExplicitJoin") { + assert!(list.contains("row.original.company__display ?? row.original.company")); + assert!(list.contains(&format!("/app/{path}/${{row.original.id}}"))); + } + if name == "ManyJoin" { + assert!(list.contains("row.original.companies__display?.[index] ?? id")); + } + } +} From 843717549945a7f0a31194e7c794c150ab2589ea Mon Sep 17 00:00:00 2001 From: Roland Rodriguez Date: Fri, 25 Sep 2026 15:05:00 -0600 Subject: [PATCH 08/21] fix(api): resolve authorized nested relation display labels --- .../schema-forge-acton/src/routes/entities.rs | 351 +++++++++++------- .../schema-forge-acton/src/routes/export.rs | 141 +------ .../tests/relation_read_authorization.rs | 172 ++++++++- 3 files changed, 417 insertions(+), 247 deletions(-) diff --git a/crates/schema-forge-acton/src/routes/entities.rs b/crates/schema-forge-acton/src/routes/entities.rs index 4100da5..ef5651a 100644 --- a/crates/schema-forge-acton/src/routes/entities.rs +++ b/crates/schema-forge-acton/src/routes/entities.rs @@ -1783,7 +1783,7 @@ pub(super) async fn filter_relation_entities_with_policy( /// target schema has no `@display(...)`, or whose target schema isn't /// registered, simply don't appear in the result — callers must treat a /// missing entry as "fall back to the raw ID". -async fn resolve_relation_displays( +pub(super) async fn resolve_relation_displays( forge: &acton_service::prelude::ActorHandle, policy_store: &Arc, parent_schema: &SchemaDefinition, @@ -1791,145 +1791,242 @@ async fn resolve_relation_displays( claims: Option<&Claims>, tenant_config: &Option, ) -> Result>, ForgeError> { - // Group relation fields by target schema so we do one DB query per - // target no matter how many source fields point at it. - let mut targets: HashMap> = - HashMap::new(); - for field in &parent_schema.fields { - if field.is_hidden() { - continue; - } - if let FieldType::Relation { target, .. } = &field.field_type { - targets - .entry(target.as_str().to_string()) - .or_default() - .push(field); - } - } - if targets.is_empty() { - return Ok(HashMap::new()); - } - - // Resolve every target schema in a single batched actor round-trip - // rather than sending one GetSchema per target. - let target_names: Vec = targets.keys().cloned().collect(); - let target_defs = fetch_schemas_batch(forge, target_names).await?; + let (tx, rx) = oneshot::channel(); + forge + .send(GetRecordAccessPolicy { + reply: ReplyChannel::new(tx), + }) + .await; + let record_policy = ask_forge(rx).await?; + resolve_relation_displays_with_policy( + forge, + policy_store, + parent_schema, + visible_entities, + claims, + tenant_config, + record_policy.as_deref(), + ) + .await +} - // Build per-target query jobs, skipping targets with no display field, - // no registered schema, or no referenced IDs. - let mut jobs: Vec<( - &SchemaDefinition, - Vec, // source field names sharing this target - String, // display_field - schema_forge_core::query::Query, - )> = Vec::with_capacity(targets.len()); +pub(super) async fn resolve_relation_displays_with_policy( + forge: &acton_service::prelude::ActorHandle, + policy_store: &Arc, + parent_schema: &SchemaDefinition, + visible_entities: &[Entity], + claims: Option<&Claims>, + tenant_config: &Option, + record_access_policy: Option<&dyn schema_forge_backend::auth::RecordAccessPolicy>, +) -> Result { + let context = RelationDisplayContext { + forge, + policy_store, + claims, + tenant_config, + record_access_policy, + }; + resolve_relation_displays_at_depth(&context, parent_schema, visible_entities, 0).await +} - for (target_name, fields_pointing_at_target) in targets { - let Some(target_def) = target_defs.get(&target_name) else { - continue; - }; - let Some(display_field) = target_def.display_field().map(|s| s.to_string()) else { - continue; - }; +// A fixed hop budget bounds cycles and pathological display chains. Unresolved +// labels are absent, so callers retain the ID they already had as a fallback. +const MAX_DISPLAY_HOPS: usize = 3; +type RelationDisplayMap = HashMap>; + +#[derive(Clone, Copy)] +struct RelationDisplayContext<'a> { + forge: &'a acton_service::prelude::ActorHandle, + policy_store: &'a Arc, + claims: Option<&'a Claims>, + tenant_config: &'a Option, + record_access_policy: Option<&'a dyn schema_forge_backend::auth::RecordAccessPolicy>, +} - // Collect distinct IDs referenced by any of these fields across all rows. - let mut distinct_ids: HashSet = HashSet::new(); - for field in &fields_pointing_at_target { - let field_name = field.name.as_str(); - for entity in visible_entities { - let Some(value) = entity.field(field_name) else { - continue; - }; - collect_relation_ids(value, &mut distinct_ids); +fn resolve_relation_displays_at_depth<'a>( + context: &'a RelationDisplayContext<'a>, + parent_schema: &'a SchemaDefinition, + visible_entities: &'a [Entity], + depth: usize, +) -> futures::future::BoxFuture<'a, Result> { + Box::pin(async move { + let RelationDisplayContext { + forge, + policy_store, + claims, + tenant_config, + record_access_policy, + } = *context; + if depth >= MAX_DISPLAY_HOPS || visible_entities.is_empty() { + return Ok(HashMap::new()); + } + // Group relation fields by target schema so we do one DB query per + // target no matter how many source fields point at it. + let mut targets: HashMap> = + HashMap::new(); + for field in &parent_schema.fields { + if field.is_hidden() + || (depth > 0 && parent_schema.display_field() != Some(field.name.as_str())) + { + continue; + } + if let FieldType::Relation { target, .. } = &field.field_type { + targets + .entry(target.as_str().to_string()) + .or_default() + .push(field); } } - if distinct_ids.is_empty() { - continue; + if targets.is_empty() { + return Ok(HashMap::new()); } - // Fetch complete rows so record policies and Cedar can inspect all - // attributes before display-field filtering. No count is needed. - let id_values: Vec = - distinct_ids.into_iter().map(DynamicValue::Text).collect(); - let mut display_query = schema_forge_core::query::Query::new(target_def.id.clone()) - .with_filter(Filter::In { - path: FieldPath::single("id"), - values: id_values, - }) - .without_total_count(); - // Apply tenant scope so we never leak rows the caller couldn't - // otherwise see through a direct list call. - inject_tenant_scope(&mut display_query, claims, tenant_config, target_def); + // Resolve every target schema in a single batched actor round-trip + // rather than sending one GetSchema per target. + let target_names: Vec = targets.keys().cloned().collect(); + let target_defs = fetch_schemas_batch(forge, target_names).await?; + + // Build per-target query jobs, skipping targets with no display field, + // no registered schema, or no referenced IDs. + let mut jobs: Vec<( + &SchemaDefinition, + Vec, // source field names sharing this target + String, // display_field + schema_forge_core::query::Query, + )> = Vec::with_capacity(targets.len()); + + for (target_name, fields_pointing_at_target) in targets { + let Some(target_def) = target_defs.get(&target_name) else { + continue; + }; + let Some(display_field) = target_def.display_field().map(|s| s.to_string()) else { + continue; + }; - let source_fields: Vec = fields_pointing_at_target - .iter() - .map(|f| f.name.as_str().to_string()) - .collect(); - jobs.push((target_def, source_fields, display_field, display_query)); - } + // Collect distinct IDs referenced by any of these fields across all rows. + let mut distinct_ids: HashSet = HashSet::new(); + for field in &fields_pointing_at_target { + let field_name = field.name.as_str(); + for entity in visible_entities { + let Some(value) = entity.field(field_name) else { + continue; + }; + collect_relation_ids(value, &mut distinct_ids); + } + } + if distinct_ids.is_empty() { + continue; + } - if jobs.is_empty() { - return Ok(HashMap::new()); - } + // Fetch complete rows so record policies and Cedar can inspect all + // attributes before display-field filtering. No count is needed. + let id_values: Vec = + distinct_ids.into_iter().map(DynamicValue::Text).collect(); + let mut display_query = schema_forge_core::query::Query::new(target_def.id.clone()) + .with_filter(Filter::In { + path: FieldPath::single("id"), + values: id_values, + }) + .without_total_count(); + // Apply tenant scope so we never leak rows the caller couldn't + // otherwise see through a direct list call. + inject_tenant_scope(&mut display_query, claims, tenant_config, target_def); - let record_access_policy = { - let (tx, rx) = oneshot::channel(); - forge - .send(GetRecordAccessPolicy { - reply: ReplyChannel::new(tx), - }) - .await; - ask_forge(rx).await? - }; - let record_access_policy = record_access_policy.as_deref(); + let source_fields: Vec = fields_pointing_at_target + .iter() + .map(|f| f.name.as_str().to_string()) + .collect(); + jobs.push((target_def, source_fields, display_field, display_query)); + } - // Fire all per-target queries concurrently. The Postgres pool has - // multiple connections so these genuinely run in parallel instead of - // serializing one-by-one on the actor mailbox. - let futures_iter = jobs.into_iter().map( - |(target_def, source_fields, display_field, query)| async move { - let (tx, rx) = oneshot::channel(); - forge - .send(QueryEntities { - query, - reply: ReplyChannel::new(tx), - }) + if jobs.is_empty() { + return Ok(HashMap::new()); + } + + // Fire all per-target queries concurrently. The Postgres pool has + // multiple connections so these genuinely run in parallel instead of + // serializing one-by-one on the actor mailbox. + let futures_iter = jobs.into_iter().map( + |(target_def, source_fields, display_field, query)| async move { + let (tx, rx) = oneshot::channel(); + forge + .send(QueryEntities { + query, + reply: ReplyChannel::new(tx), + }) + .await; + let target_result = match ask_forge(rx).await { + Ok(Ok(r)) => r, + _ => return (source_fields, HashMap::new()), + }; + let mut id_to_display: HashMap = HashMap::new(); + let targets = filter_relation_entities_with_policy( + record_access_policy, + policy_store, + target_def, + claims, + target_result.entities, + ) .await; - let target_result = match ask_forge(rx).await { - Ok(Ok(r)) => r, - _ => return (source_fields, HashMap::new()), - }; - let mut id_to_display: HashMap = HashMap::new(); - let targets = filter_relation_entities_with_policy( - record_access_policy, - policy_store, - target_def, - claims, - target_result.entities, - ) - .await; - for target_entity in targets { - if let Some(display_value) = target_entity.field(&display_field) { - let rendered = display_value_to_string(display_value); - id_to_display.insert(target_entity.id.as_str().to_string(), rendered); + let relation_display = target_def + .field(&display_field) + .is_some_and(|field| matches!(field.field_type, FieldType::Relation { .. })); + let nested = if relation_display { + resolve_relation_displays_at_depth(context, target_def, &targets, depth + 1) + .await + .unwrap_or_default() + } else { + HashMap::new() + }; + for target_entity in targets { + if let Some(display_value) = target_entity.field(&display_field) { + let rendered = if relation_display { + let mut response = entity_to_response(&target_entity, target_def); + apply_relation_displays( + &mut response, + target_def, + &target_entity, + &nested, + ); + response + .fields + .get(&format!("{display_field}__display")) + .and_then(|value| match value { + serde_json::Value::String(label) => Some(label.clone()), + serde_json::Value::Array(labels) => { + let labels: Vec<_> = labels + .iter() + .filter_map(serde_json::Value::as_str) + .collect(); + (!labels.is_empty()).then(|| labels.join(", ")) + } + _ => None, + }) + } else { + Some(display_value_to_string(display_value)) + }; + if let Some(rendered) = rendered { + id_to_display.insert(target_entity.id.as_str().to_string(), rendered); + } + } } - } - (source_fields, id_to_display) - }, - ); - let completed = futures::future::join_all(futures_iter).await; + (source_fields, id_to_display) + }, + ); + let completed = futures::future::join_all(futures_iter).await; - let mut result: HashMap> = HashMap::new(); - for (source_fields, id_to_display) in completed { - if id_to_display.is_empty() { - continue; - } - for field_name in source_fields { - result.insert(field_name, id_to_display.clone()); + let mut result: HashMap> = HashMap::new(); + for (source_fields, id_to_display) in completed { + if id_to_display.is_empty() { + continue; + } + for field_name in source_fields { + result.insert(field_name, id_to_display.clone()); + } } - } - Ok(result) + Ok(result) + }) } /// Fetch several schema definitions in a single `GetSchemasBatch` actor @@ -2511,6 +2608,12 @@ fn display_value_to_string(value: &DynamicValue) -> String { DynamicValue::Integer(n) => n.to_string(), DynamicValue::Float(n) => n.to_string(), DynamicValue::Boolean(b) => b.to_string(), + DynamicValue::Ref(id) => id.to_string(), + DynamicValue::RefArray(ids) => ids + .iter() + .map(ToString::to_string) + .collect::>() + .join(", "), other => other.to_string(), } } diff --git a/crates/schema-forge-acton/src/routes/export.rs b/crates/schema-forge-acton/src/routes/export.rs index 823f04b..cf675ee 100644 --- a/crates/schema-forge-acton/src/routes/export.rs +++ b/crates/schema-forge-acton/src/routes/export.rs @@ -40,10 +40,8 @@ use schema_forge_backend::entity::Entity; use schema_forge_core::export::{ to_cell, to_ndjson, to_xlsx_cell, CellOptions, RelationDisplay, XlsxCell, }; -use schema_forge_core::query::{validate_filter, Filter, Query}; -use schema_forge_core::types::{ - DynamicValue, EntityId, ExportFormat, FieldType, SchemaDefinition, SchemaName, -}; +use schema_forge_core::query::{validate_filter, Query}; +use schema_forge_core::types::{EntityId, ExportFormat, SchemaDefinition, SchemaName}; use serde::Deserialize; use tokio::sync::oneshot; use tracing::instrument; @@ -70,7 +68,7 @@ const ACTOR_TIMEOUT: Duration = Duration::from_secs(5); /// never widen what leaves. #[derive(Debug, Deserialize)] pub struct ExportRequestBody { - /// Raw JSON filter — converted to a [`Filter`] using schema type hints, + /// Raw JSON filter — converted to a [`schema_forge_core::query::Filter`] using schema type hints, /// identical to the query endpoint. #[serde(default)] pub filter: Option, @@ -1236,129 +1234,28 @@ async fn resolve_export_displays( visible: &[Entity], columns: &[(String, Option)], ) -> Result>, ForgeError> { - use std::collections::HashSet; - - let forge = ctx.forge; - let mut out: HashMap> = HashMap::new(); - if visible.is_empty() { - return Ok(out); - } - - for (field_name, _) in columns { - let Some(field) = schema.field(field_name) else { - continue; - }; - let FieldType::Relation { target, .. } = &field.field_type else { - continue; - }; - - // Distinct referenced IDs for this relation column across all rows. - let mut ids: HashSet = HashSet::new(); - for entity in visible { - if let Some(value) = entity.field(field_name) { - collect_ref_ids(value, &mut ids); - } - } - if ids.is_empty() { - continue; - } - - // Look up the target schema to find its @display field. - let (tx, rx) = oneshot::channel(); - forge - .send(GetSchema { - name: target.as_str().to_string(), - reply: ReplyChannel::new(tx), - }) - .await; - let Some(target_def) = ask_forge(rx).await? else { - continue; - }; - let Some(display_field) = target_def.display_field().map(str::to_string) else { - continue; - }; - - let id_values: Vec = ids.into_iter().map(DynamicValue::Text).collect(); - let mut display_query = Query::new(target_def.id.clone()) - .with_filter(Filter::In { - path: schema_forge_core::query::FieldPath::single("id"), - values: id_values, - }) - .without_total_count(); - // Authorization needs the complete row, including owner and policy fields. - inject_tenant_scope(&mut display_query, ctx.claims, ctx.tenant_config, &target_def); - - let (tx, rx) = oneshot::channel(); - forge - .send(QueryEntities { - query: display_query, - reply: ReplyChannel::new(tx), - }) - .await; - let display_result = ask_forge(rx).await?.map_err(ForgeError::from)?; - let targets = super::entities::filter_relation_entities_with_policy( - ctx.record_access_policy.as_deref(), - ctx.policy_store, - &target_def, - ctx.claims, - display_result.entities, - ) - .await; - - let mut id_to_display: HashMap = HashMap::new(); - for target_entity in targets { - if let Some(value) = target_entity.field(&display_field) { - id_to_display.insert(target_entity.id.to_string(), display_scalar(value)); - } - } - if !id_to_display.is_empty() { - out.insert(field_name.clone(), id_to_display); - } - } - - Ok(out) -} - -/// Collect referenced relation IDs from a stored relation value into `out`. -fn collect_ref_ids(value: &DynamicValue, out: &mut std::collections::HashSet) { - match value { - DynamicValue::Ref(id) => { - out.insert(id.to_string()); - } - DynamicValue::RefArray(ids) => { - for id in ids { - out.insert(id.to_string()); - } - } - DynamicValue::Text(s) => { - out.insert(s.clone()); - } - DynamicValue::Array(items) => { - for item in items { - collect_ref_ids(item, out); - } - } - _ => {} - } -} - -/// Stringify a `@display` field value for the relation display map. -fn display_scalar(value: &DynamicValue) -> String { - match value { - DynamicValue::Text(s) => s.clone(), - DynamicValue::Integer(n) => n.to_string(), - DynamicValue::Float(n) => n.to_string(), - DynamicValue::Boolean(b) => b.to_string(), - other => other.to_string(), - } + let mut projected = schema.clone(); + projected + .fields + .retain(|field| columns.iter().any(|(name, _)| name == field.name.as_str())); + super::entities::resolve_relation_displays_with_policy( + ctx.forge, + ctx.policy_store, + &projected, + visible, + ctx.claims, + ctx.tenant_config, + ctx.record_access_policy.as_deref(), + ) + .await } #[cfg(test)] mod tests { use super::*; use schema_forge_core::types::{ - Annotation, ExportFlatten, ExportFormat as EF, FieldAnnotation, FieldDefinition, FieldName, - FieldType, SchemaId, SchemaName, TextConstraints, + Annotation, DynamicValue, ExportFlatten, ExportFormat as EF, FieldAnnotation, + FieldDefinition, FieldName, FieldType, SchemaId, SchemaName, TextConstraints, }; use std::collections::BTreeMap; diff --git a/crates/schema-forge-acton/tests/relation_read_authorization.rs b/crates/schema-forge-acton/tests/relation_read_authorization.rs index 6815a20..544083e 100644 --- a/crates/schema-forge-acton/tests/relation_read_authorization.rs +++ b/crates/schema-forge-acton/tests/relation_read_authorization.rs @@ -17,7 +17,7 @@ use schema_forge_acton::{ routes::forge_routes, ForgeActor, }; -use schema_forge_backend::{auth::RecordAccessPolicy, entity::Entity, SchemaBackend}; +use schema_forge_backend::{auth::RecordAccessPolicy, entity::Entity, EntityStore, SchemaBackend}; use schema_forge_core::types::{DynamicValue, EntityId, FieldName, SchemaDefinition}; use schema_forge_surrealdb::SurrealBackend; use serde_json::{json, Value}; @@ -335,3 +335,173 @@ async fn operator_redaction_cannot_erase_attributes_before_cedar_read_checks() { assert!(!children.contains(&json!(fixture_id("child_denied")))); } } + +async fn nested_fixture(product_role: &str) -> Router { + let mut schemas = schema_forge_dsl::parse(&format!(r#" + @access(read: ["clerk"]) + schema Parent {{ selected: -> Line @exportable missing: -> Line @exportable cycle: -> Cycle @exportable children: -> Line[] }} + @access(read: ["clerk"]) @display("product") + schema Line {{ product: -> Product parent: -> Parent }} + @access(read: ["{product_role}"]) @display("label") + schema Product {{ label: text }} + @access(read: ["clerk"]) @display("next") + schema Cycle {{ next: -> Cycle }} + "#)).unwrap(); + schemas[0] + .fields + .iter_mut() + .find(|field| field.name.as_str() == "children") + .unwrap() + .derived_from = Some(FieldName::new("parent").unwrap()); + schemas[0] + .annotations + .push(schema_forge_core::types::Annotation::Export { + formats: vec![schema_forge_core::types::ExportFormat::Csv], + bundle_files: false, + max_rows: 100, + }); + let backend = Arc::new( + SurrealBackend::connect_memory("nested", "nested") + .await + .unwrap(), + ); + for schema in &schemas { + let plan = schema_forge_core::migration::DiffEngine::create_new(schema); + backend + .apply_migration(&schema.name, &plan.steps) + .await + .unwrap(); + backend.store_schema_metadata(schema).await.unwrap(); + } + for (index, id, fields) in [ + ( + 0, + "parent_one", + vec![ + ("selected", DynamicValue::Ref(fixture_id("line_good"))), + ("missing", DynamicValue::Ref(fixture_id("line_missing"))), + ("cycle", DynamicValue::Ref(fixture_id("cycle_one"))), + ], + ), + ( + 1, + "line_good", + vec![ + ("product", DynamicValue::Ref(fixture_id("product_one"))), + ("parent", DynamicValue::Ref(fixture_id("parent_one"))), + ], + ), + ( + 1, + "line_missing", + vec![("product", DynamicValue::Ref(fixture_id("product_absent")))], + ), + ( + 2, + "product_one", + vec![("label", DynamicValue::Text("Blue widget".into()))], + ), + ( + 3, + "cycle_one", + vec![("next", DynamicValue::Ref(fixture_id("cycle_one")))], + ), + ] { + let entity = Entity::with_id( + fixture_id(id), + schemas[index].name.clone(), + fields + .into_iter() + .map(|(key, value)| (key.into(), value)) + .collect(), + ); + backend.create(&entity).await.unwrap(); + } + let service = acton_service::service_builder::ServiceBuilder::new() + .with_config(Config::::default()) + .with_actor::() + .with_actor::() + .build(); + let (tx, rx) = oneshot::channel(); + service + .state() + .actor::() + .unwrap() + .send(InitForge { + registry: schemas + .into_iter() + .map(|schema| (schema.name.to_string(), schema)) + .collect(), + backend, + tenant_config: None, + record_access_policy: None, + hook_dispatcher: None, + storage_registry: Default::default(), + policy_store: None, + custom_policies_dir: None, + reply: ReplyChannel::new(tx), + }) + .await; + tokio::time::timeout(std::time::Duration::from_secs(5), rx) + .await + .unwrap() + .unwrap(); + forge_routes().with_state(service.state().clone()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn nested_display_hops_authorize_targets_and_bound_missing_or_cyclic_paths() { + for role in ["clerk", "manager"] { + let app = nested_fixture(role).await; + for fields in parent_views(&app).await { + if role == "clerk" { + assert_eq!(fields["selected__display"], "Blue widget", "{fields}"); + assert_eq!( + fields["children__display"], + json!(["Blue widget"]), + "{fields}" + ); + } else { + assert!(fields.get("selected__display").is_none(), "{fields}"); + assert!(fields.get("children__display").is_none(), "{fields}"); + } + assert!(fields.get("missing__display").is_none(), "{fields}"); + assert!(fields.get("cycle__display").is_none(), "{fields}"); + assert!(!fields.to_string().contains("ref(")); + } + let claims = Claims { + sub: "user:viewer".into(), + roles: vec!["clerk".into()], + perms: vec![], + exp: 9_999_999_999, + iat: None, + jti: None, + iss: None, + aud: None, + email: None, + username: None, + custom: HashMap::new(), + }; + let mut request = Request::builder() + .method(Method::POST) + .uri("/schemas/Parent/entities/export") + .header("content-type", "application/json") + .body(Body::from(r#"{"format":"csv"}"#)) + .unwrap(); + request.extensions_mut().insert(claims); + let response = app.clone().oneshot(request).await.unwrap(); + assert_eq!(response.status(), StatusCode::OK); + let csv = String::from_utf8( + response + .into_body() + .collect() + .await + .unwrap() + .to_bytes() + .to_vec(), + ) + .unwrap(); + assert_eq!(csv.contains("Blue widget"), role == "clerk", "{csv}"); + assert!(!csv.contains("ref("), "{csv}"); + } +} From 6f7d8c0c9f8b6c0041a82421d19d59402cd861e0 Mon Sep 17 00:00:00 2001 From: Roland Rodriguez Date: Fri, 25 Sep 2026 15:07:17 -0600 Subject: [PATCH 09/21] docs(release): describe follow-up migration and site fixes --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 99251ea..8ffd42c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,11 @@ is retained, but its binary release was not published. ### Database and operator fixes +- Add PostgreSQL literal defaults with their columns so existing rows are filled. + Required-field transitions backfill before enforcing `NOT NULL`; plans without + a usable literal backfill value are refused before changes are applied. +- Report stored/derived inverse-collection transitions as destructive migrations. + Cross-schema changes in collection meaning require a reviewed batch migration. - PostgreSQL planning and inspection connections perform no bookkeeping DDL. Fresh databases plan as empty registries, and read-only roles can inspect existing metadata without schema creation privileges. @@ -56,6 +61,9 @@ is retained, but its binary release was not published. ### Generated sites +- Generate compilable list and detail pages for relation-only and file-only + schemas. Primary relation cells and relation pickers use display labels, and + nested display relations resolve through bounded, authorized lookups. - Carry the active tenant on entity, invitation, and file requests, including requests retried after a token refresh. - Show readable API errors and avoid duplicate global notifications when pages From 62ed3bf498b13ddf7528dfecf434e0cd491e64ea Mon Sep 17 00:00:00 2001 From: Roland Rodriguez Date: Fri, 25 Sep 2026 15:07:31 -0600 Subject: [PATCH 10/21] chore(release): version cross-backend migration corrections --- Cargo.lock | 4 ++-- crates/schema-forge-mssql/Cargo.toml | 2 +- crates/schema-forge-surrealdb/Cargo.toml | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7812fb3..c3eeb02 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7756,7 +7756,7 @@ dependencies = [ [[package]] name = "schema-forge-mssql" -version = "0.6.0" +version = "0.6.1" dependencies = [ "acton-service", "bb8", @@ -7810,7 +7810,7 @@ dependencies = [ [[package]] name = "schema-forge-surrealdb" -version = "0.14.0" +version = "0.14.1" dependencies = [ "chrono", "schema-forge-backend", diff --git a/crates/schema-forge-mssql/Cargo.toml b/crates/schema-forge-mssql/Cargo.toml index 416803c..2ffe029 100644 --- a/crates/schema-forge-mssql/Cargo.toml +++ b/crates/schema-forge-mssql/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "schema-forge-mssql" -version = "0.6.0" +version = "0.6.1" edition = "2021" [dependencies] diff --git a/crates/schema-forge-surrealdb/Cargo.toml b/crates/schema-forge-surrealdb/Cargo.toml index 2f9ba8a..83af54a 100644 --- a/crates/schema-forge-surrealdb/Cargo.toml +++ b/crates/schema-forge-surrealdb/Cargo.toml @@ -1,7 +1,7 @@ [package] rust-version = "1.97.1" name = "schema-forge-surrealdb" -version = "0.14.0" +version = "0.14.1" edition = "2021" [dependencies] From c09dcf2bccdb2dd1db367c4784d3ba1947c02f94 Mon Sep 17 00:00:00 2001 From: Roland Rodriguez Date: Fri, 25 Sep 2026 15:07:52 -0600 Subject: [PATCH 11/21] fix(migration): backfill required fields and gate inverse storage changes --- crates/schema-forge-acton/src/extension.rs | 11 ++ .../schema-forge-acton/src/routes/schemas.rs | 85 +++++++++- crates/schema-forge-cli/src/commands/parse.rs | 20 +++ .../schema-forge-cli/tests/cli_integration.rs | 23 +++ .../src/inverse_relations.rs | 30 +++- crates/schema-forge-core/src/migration.rs | 130 ++++++++++++-- .../tests/migration_defaults_and_storage.rs | 155 +++++++++++++++++ .../schema-forge-dsl/tests/migration_hints.rs | 16 ++ crates/schema-forge-postgres/src/codegen.rs | 65 +++---- .../tests/migration_safety.rs | 158 ++++++++++++++++++ docs/migrations/safe-schema-changes.md | 43 +++++ skills/schemaforge/dsl-reference.md | 8 +- 12 files changed, 681 insertions(+), 63 deletions(-) create mode 100644 crates/schema-forge-core/tests/migration_defaults_and_storage.rs diff --git a/crates/schema-forge-acton/src/extension.rs b/crates/schema-forge-acton/src/extension.rs index 9a8b5c1..d7c833c 100644 --- a/crates/schema-forge-acton/src/extension.rs +++ b/crates/schema-forge-acton/src/extension.rs @@ -432,6 +432,17 @@ impl SchemaForgeExtension { } })?; for schema in paired { + if let Some(stored) = registry.get(schema.name.as_str()) { + let plan = schema_forge_core::migration::DiffEngine::plan_update(stored, &schema) + .map_err(|error| ForgeError::Internal { + message: error.to_string(), + })?; + if plan.has_destructive_steps() { + return Err(ForgeError::Internal { + message: format!("inverse collection storage transition requires an explicit batch migration before startup: {plan}"), + }); + } + } registry.insert(schema.name.as_str().to_string(), schema); } diff --git a/crates/schema-forge-acton/src/routes/schemas.rs b/crates/schema-forge-acton/src/routes/schemas.rs index cedf4e6..88f3f14 100644 --- a/crates/schema-forge-acton/src/routes/schemas.rs +++ b/crates/schema-forge-acton/src/routes/schemas.rs @@ -37,11 +37,8 @@ const ACTOR_TIMEOUT: Duration = Duration::from_secs(5); /// `target`. Isolated in its own function so both `create_schema` and /// `update_schema` share the exact same logic. /// -/// Note: this only updates `target`. If adding/updating `target` would -/// cause an *existing* schema's stored `-> X[]` field to become derived -/// (because the new schema provides the inverse FK), that existing schema -/// is not rewritten here — the change takes effect on the next daemon -/// restart, which re-pairs the entire registry in `build_init`. +/// Cross-schema pairing changes are rejected by the policy preflight below; +/// they require a reviewed batch migration, not a target-only metadata write. async fn pair_with_registry( forge: &acton_service::prelude::ActorHandle, target: &mut SchemaDefinition, @@ -93,6 +90,51 @@ async fn pair_with_registry( Ok(registry) } +// A single-schema REST transaction cannot migrate sibling storage. Refuse +// changes that would silently alter sibling reads on this or the next start. +fn reject_sibling_pairing_changes( + existing: &std::collections::HashMap, + proposed: &mut [SchemaDefinition], + target: &str, +) -> Result<(), ForgeError> { + if !proposed.iter().any(|schema| schema.name.as_str() == target) { + for schema in proposed.iter() { + for field in &schema.fields { + if field.is_derived() + && matches!(&field.field_type, FieldType::Relation { target: related, .. } if related.as_str() == target) + { + return Err(ForgeError::ValidationFailed { + details: vec![format!("schema '{target}' is used by derived collection '{}.{}'; remove the collection in a reviewed batch migration before deleting its target", schema.name, field.name)], + }); + } + } + } + } + schema_forge_core::inverse_relations::pair_inverse_relations(proposed).map_err(|error| { + ForgeError::ValidationFailed { + details: vec![error.to_string()], + } + })?; + for schema in proposed { + if schema.name.as_str() == target { + continue; + } + if let Some(previous) = existing.get(schema.name.as_str()) { + for field in &schema.fields { + if previous + .field(field.name.as_str()) + .is_some_and(|old| old.derived_from != field.derived_from) + { + return Err(ForgeError::ValidationFailed { + details: vec![format!("schema change alters inverse collection '{}.{}'; apply the complete schema batch with schemaforge migrate/apply and explicitly approve destructive storage changes", schema.name, field.name)], + }); + } + } + } + } + Ok(()) +} + /// Dry-run the Cedar policy bundle that would result from inserting (or /// removing, when `removing` is `true`) `target` into the current registry. /// @@ -134,6 +176,8 @@ async fn precheck_policy_bundle( proposed.push(target.clone()); } + reject_sibling_pairing_changes(&expected_registry, &mut proposed, target.name.as_str())?; + if tenant_structure(&proposed)? != current_tenant_structure { return Err(ForgeError::ValidationFailed { details: vec!["tenant hierarchy changes require applying schema files and restarting serve so actor and middleware configuration change together".into()] }); } @@ -1032,6 +1076,37 @@ pub async fn delete_schema( mod tests { use super::*; + #[test] + fn single_schema_change_refuses_implicit_sibling_storage_transitions() { + let old = schema_forge_dsl::parse( + "schema Team { members: -> Person[] } schema Person { name: text }", + ) + .unwrap(); + let registry = old + .iter() + .map(|schema| (schema.name.to_string(), schema.clone())) + .collect(); + let mut changed = schema_forge_dsl::parse( + "schema Team { members: -> Person[] } schema Person { name: text backup_for: -> Team }", + ) + .unwrap(); + let error = reject_sibling_pairing_changes(®istry, &mut changed, "Person").unwrap_err(); + assert!(error.to_string().contains("Team.members")); + let paired_registry = changed + .iter() + .map(|schema| (schema.name.to_string(), schema.clone())) + .collect(); + let mut removed_fk = old; + assert!( + reject_sibling_pairing_changes(&paired_registry, &mut removed_fk, "Person").is_err() + ); + let mut deleted_child = vec![changed[0].clone()]; + assert!( + reject_sibling_pairing_changes(&paired_registry, &mut deleted_child, "Person").is_err() + ); + assert!(reject_sibling_pairing_changes(&paired_registry, &mut changed, "Person").is_ok()); + } + #[test] fn parse_field_type_simple_text() { let result = parse_field_type(&serde_json::json!("Text")).unwrap(); diff --git a/crates/schema-forge-cli/src/commands/parse.rs b/crates/schema-forge-cli/src/commands/parse.rs index 7463a98..36b99cb 100644 --- a/crates/schema-forge-cli/src/commands/parse.rs +++ b/crates/schema-forge-cli/src/commands/parse.rs @@ -23,6 +23,7 @@ pub async fn run( let mut total_errors = 0usize; let mut all_file_results: Vec = Vec::new(); let mut had_errors = false; + let mut all_schemas = Vec::new(); for file in &files { let source_text = std::fs::read_to_string(file).map_err(|e| CliError::Io { @@ -48,6 +49,8 @@ pub async fn run( println!("{printed}"); } + all_schemas.extend(schemas); + if output.mode == OutputMode::Json { all_file_results.push(serde_json::json!({ "file": filename, @@ -92,6 +95,23 @@ pub async fn run( } } + if !had_errors { + if let Err(error) = + schema_forge_core::inverse_relations::pair_inverse_relations(&mut all_schemas) + { + had_errors = true; + total_errors += 1; + if output.mode == OutputMode::Json { + all_file_results.push(serde_json::json!({ + "file": "(schema batch)", "schemas": 0, + "errors": [{"message": error.to_string()}], + })); + } else { + output.warn(&error.to_string()); + } + } + } + // Summary match output.mode { OutputMode::Human => { diff --git a/crates/schema-forge-cli/tests/cli_integration.rs b/crates/schema-forge-cli/tests/cli_integration.rs index acccecd..1fe8ccc 100644 --- a/crates/schema-forge-cli/tests/cli_integration.rs +++ b/crates/schema-forge-cli/tests/cli_integration.rs @@ -707,3 +707,26 @@ fn serve_rejects_invalid_host_before_connecting() { .failure() .stderr(predicate::str::contains("invalid IP address syntax")); } + +#[test] +fn parse_reports_ambiguous_inverse_relations_across_files() { + let dir = TempDir::new().unwrap(); + fs::write( + dir.path().join("team.schema"), + "schema Team { members: -> Person[] }", + ) + .unwrap(); + fs::write( + dir.path().join("person.schema"), + "schema Person { home: -> Team backup_for: -> Team }", + ) + .unwrap(); + schema_forge() + .args(["--format", "json", "parse", dir.path().to_str().unwrap()]) + .assert() + .failure() + .stdout(predicate::str::contains( + "ambiguous inverse relation for Team.members", + )) + .stdout(predicate::str::contains("\"errors\": 1")); +} diff --git a/crates/schema-forge-core/src/inverse_relations.rs b/crates/schema-forge-core/src/inverse_relations.rs index 0362538..e33d8ed 100644 --- a/crates/schema-forge-core/src/inverse_relations.rs +++ b/crates/schema-forge-core/src/inverse_relations.rs @@ -74,11 +74,14 @@ pub fn pair_inverse_relations( cardinality: Cardinality::One, } = &field.field_type { - child_fks.entry(target.as_str().to_string()).or_default().push(( - schema_idx, - schema.name.as_str().to_string(), - field.name.as_str().to_string(), - )); + child_fks + .entry(target.as_str().to_string()) + .or_default() + .push(( + schema_idx, + schema.name.as_str().to_string(), + field.name.as_str().to_string(), + )); } } } @@ -131,6 +134,23 @@ pub fn pair_inverse_relations( } } + // Clear stale pairings only after the whole pass succeeds. Removing the + // last FK must return the parent field to stored semantics as well. + let names: std::collections::HashSet<_> = schemas.iter().map(|s| s.name.clone()).collect(); + for schema in schemas.iter_mut() { + for field in &mut schema.fields { + if let FieldType::Relation { + target, + cardinality: Cardinality::Many, + } = &field.field_type + { + if names.contains(target) { + field.derived_from = None; + } + } + } + } + for (parent_idx, field_idx, fk_field_name) in pairings { // Re-construct FieldName for the child FK. We know it's valid // because it already exists on a parsed schema. diff --git a/crates/schema-forge-core/src/migration.rs b/crates/schema-forge-core/src/migration.rs index 041f644..2b9d2bb 100644 --- a/crates/schema-forge-core/src/migration.rs +++ b/crates/schema-forge-core/src/migration.rs @@ -521,9 +521,69 @@ impl DiffEngine { } } } + Self::validate_required_backfills(old, new)?; Ok(()) } + // Existing rows need a deterministic, type-compatible value before a new + // NOT NULL constraint can be installed. CEL defaults run during writes, + // not DDL; even constant CEL expressions require explicit manual backfill. + fn validate_required_backfills( + old: &crate::types::SchemaDefinition, + new: &crate::types::SchemaDefinition, + ) -> Result<(), MigrationError> { + for field in &new.fields { + if !field.is_required() || field.is_derived() { + continue; + } + let old_field = old.field(field.name.as_str()).or_else(|| { + field.annotations.iter().find_map(|annotation| { + if let crate::types::FieldAnnotation::RenamedFrom { name } = annotation { + old.field(name.as_str()) + } else { + None + } + }) + }); + let needs_backfill = + old_field.is_none_or(|previous| !previous.is_required() || previous.is_derived()); + if needs_backfill && Self::backfill_value(field).is_none() { + return Err(MigrationError::RequiredFieldWithoutDefault { + field_name: field.name.to_string(), + }); + } + } + Ok(()) + } + + fn backfill_value(field: &FieldDefinition) -> Option { + let default = Self::extract_default(&field.modifiers)?; + let value = match (&field.field_type, default) { + (FieldType::Text(_) | FieldType::RichText, DefaultValue::String(value)) => { + DynamicValue::Text(value.clone()) + } + (FieldType::Enum(_), DefaultValue::String(value)) => DynamicValue::Enum(value.clone()), + (FieldType::Integer(_), DefaultValue::Integer(value)) => DynamicValue::Integer(*value), + (FieldType::Float(_), DefaultValue::Float(value)) => { + let number = value.parse::().ok()?; + if !number.is_finite() { + return None; + } + DynamicValue::Float(number) + } + (FieldType::Float(_), DefaultValue::Integer(value)) => { + DynamicValue::Float(value.to_string().parse().ok()?) + } + (FieldType::DateTime, DefaultValue::String(value)) => { + DynamicValue::DateTime(chrono::DateTime::parse_from_rfc3339(value).ok()?.to_utc()) + } + (FieldType::Boolean, DefaultValue::Boolean(value)) => DynamicValue::Boolean(*value), + _ => return None, + }; + field.field_type.check_value(&value).ok()?; + Some(value) + } + /// Compute an unchecked structural diff of two schema definitions. /// /// This compatibility API does not validate tenancy transitions or rename hints. @@ -710,6 +770,20 @@ impl DiffEngine { // Emit RenameField for valid rename pairs for (old_name, new_name) in renames { if let Some(old_field) = old.field(old_name.as_str()) { + if let Some(new_field) = new.field(new_name.as_str()) { + if Self::diff_storage( + old_field, + new_field, + new.unique_scoped_by_tenant(), + steps, + ) { + continue; + } + // Neither version has a physical column to rename. + if old_field.is_derived() && new_field.is_derived() { + continue; + } + } steps.push(MigrationStep::RenameField { old_name: old_name.clone(), new_name: new_name.clone(), @@ -756,6 +830,12 @@ impl DiffEngine { continue; // already handled above } if let Some(old_field) = old.field(new_field.name.as_str()) { + if Self::diff_storage(old_field, new_field, per_tenant, steps) { + continue; + } + if old_field.is_derived() && new_field.is_derived() { + continue; + } if old_field.field_type != new_field.field_type { Self::emit_change_type(old_field, new_field, steps); } @@ -763,6 +843,27 @@ impl DiffEngine { } } + // Pairing can change because another schema changed, even when this + // field's DSL type is identical. Explicitly remove the old storage so + // every transition is destructive, visible and gated by all callers. + fn diff_storage( + old: &FieldDefinition, + new: &FieldDefinition, + per_tenant: bool, + steps: &mut Vec, + ) -> bool { + if old.derived_from == new.derived_from + && (!old.is_derived() || old.field_type == new.field_type) + { + return false; + } + steps.push(MigrationStep::RemoveRelation { + name: old.name.clone(), + }); + Self::emit_add_field(new, per_tenant, steps); + true + } + fn diff_modifiers_with_renames( old: &crate::types::SchemaDefinition, new: &crate::types::SchemaDefinition, @@ -791,19 +892,18 @@ impl DiffEngine { }; if let Some(old_field) = old_field { + if old_field.is_derived() { + continue; // newly created storage already carries its modifiers + } // For renamed fields, we compare old type with new type // (type changes are handled in diff_fields_with_renames) // Modifier diffing uses the new field's name for step output - let old_type = &old_field.field_type; - let new_type = &new_field.field_type; - if old_type == new_type { - Self::diff_field_modifiers( - old_field, - new_field, - new.unique_scoped_by_tenant(), - steps, - ); - } + Self::diff_field_modifiers( + old_field, + new_field, + new.unique_scoped_by_tenant(), + steps, + ); } } } @@ -825,6 +925,12 @@ impl DiffEngine { // Required changes if !old_required && new_required { + if let Some(default_value) = Self::backfill_value(new_field) { + steps.push(MigrationStep::BackfillRequired { + field: new_field.name.clone(), + default_value, + }); + } steps.push(MigrationStep::AddRequired { field: new_field.name.clone(), }); @@ -1028,7 +1134,7 @@ impl fmt::Display for MigrationError { Self::RequiredFieldWithoutDefault { field_name } => { write!( f, - "required field '{field_name}' was added without a default value for backfill" + "required field '{field_name}' needs a usable literal default(...) for existing-row backfill; CEL @default expressions are not evaluated during migration. Add the field as optional, backfill existing rows manually, then install the required constraint and schema metadata manually" ) } Self::UnsupportedTypeConversion { @@ -1996,7 +2102,7 @@ mod tests { MigrationError::RequiredFieldWithoutDefault { field_name: "email".into(), }, - "required field 'email' was added without a default value for backfill", + "required field 'email' needs a usable literal default(...) for existing-row backfill; CEL @default expressions are not evaluated during migration. Add the field as optional, backfill existing rows manually, then install the required constraint and schema metadata manually", ), ( MigrationError::UnsupportedTypeConversion { diff --git a/crates/schema-forge-core/tests/migration_defaults_and_storage.rs b/crates/schema-forge-core/tests/migration_defaults_and_storage.rs new file mode 100644 index 0000000..08431c9 --- /dev/null +++ b/crates/schema-forge-core/tests/migration_defaults_and_storage.rs @@ -0,0 +1,155 @@ +use schema_forge_core::{ + inverse_relations::pair_inverse_relations, + migration::{DiffEngine, MigrationError, MigrationStep}, + types::{ + Cardinality, DefaultValue, DynamicValue, FieldDefinition, FieldModifier, FieldName, + FieldType, SchemaDefinition, SchemaId, SchemaName, TextConstraints, + }, +}; + +fn text(name: &str) -> FieldDefinition { + FieldDefinition::new( + FieldName::new(name).unwrap(), + FieldType::Text(TextConstraints::unconstrained()), + ) +} +fn schema(name: &str, fields: Vec) -> SchemaDefinition { + SchemaDefinition::new( + SchemaId::new(), + SchemaName::new(name).unwrap(), + fields, + vec![], + ) + .unwrap() +} +fn required_default(mut field: FieldDefinition) -> FieldDefinition { + field.modifiers = vec![ + FieldModifier::Required, + FieldModifier::Default { + value: DefaultValue::String("draft".into()), + }, + ]; + field +} + +#[test] +fn required_changes_need_usable_defaults_but_fresh_and_unchanged_schemas_do_not() { + let old = schema("Widget", vec![text("name")]); + let mut new = old.clone(); + let mut required = text("status"); + required.modifiers.push(FieldModifier::Required); + new.fields.push(required.clone()); + assert!(matches!( + DiffEngine::plan_update(&old, &new), + Err(MigrationError::RequiredFieldWithoutDefault { .. }) + )); + assert!(!DiffEngine::create_new(&new).is_empty()); + assert!(DiffEngine::plan_update(&new, &new).unwrap().is_empty()); + required.modifiers.push(FieldModifier::Default { + value: DefaultValue::Integer(0), + }); + new.fields[1] = required; + assert!( + DiffEngine::plan_update(&old, &new).is_err(), + "wrong-type literal cannot backfill text" + ); + new.fields[1] = required_default(text("status")); + assert!(DiffEngine::plan_update(&old, &new).unwrap().is_safe()); +} + +#[test] +fn optional_to_required_backfills_before_installing_constraint() { + let old = schema("Widget", vec![text("status")]); + let new = schema("Widget", vec![required_default(text("status"))]); + let plan = DiffEngine::plan_update(&old, &new).unwrap(); + assert!( + matches!(&plan.steps[0], MigrationStep::BackfillRequired { field, default_value: DynamicValue::Text(value) } if field.as_str() == "status" && value == "draft") + ); + assert!(matches!(&plan.steps[1], MigrationStep::AddRequired { .. })); + let mut no_default = new.clone(); + no_default.fields[0].modifiers = vec![FieldModifier::Required]; + assert!(DiffEngine::plan_update(&old, &no_default).is_err()); +} + +fn relation(name: &str, target: &str, cardinality: Cardinality) -> FieldDefinition { + FieldDefinition::new( + FieldName::new(name).unwrap(), + FieldType::Relation { + target: SchemaName::new(target).unwrap(), + cardinality, + }, + ) +} + +#[test] +fn changes_in_child_schema_surface_parent_storage_loss_and_recreation() { + let team = schema( + "Team", + vec![relation("members", "Person", Cardinality::Many)], + ); + let person = schema("Person", vec![text("name")]); + let mut batch = vec![team.clone(), person]; + batch[1] + .fields + .push(relation("backup_for", "Team", Cardinality::One)); + pair_inverse_relations(&mut batch).unwrap(); + let derived = batch[0].clone(); + let plan = DiffEngine::plan_update(&team, &derived).unwrap(); + assert!(plan.has_destructive_steps()); + assert!( + matches!(&plan.steps[..], [MigrationStep::RemoveRelation { name }] if name.as_str() == "members") + ); + assert!(DiffEngine::plan_update(&derived, &derived) + .unwrap() + .is_empty()); + + batch[1].fields.pop(); + pair_inverse_relations(&mut batch).unwrap(); + assert!( + !batch[0].fields[0].is_derived(), + "re-pairing must clear stale metadata" + ); + let plan = DiffEngine::plan_update(&derived, &batch[0]).unwrap(); + assert!(plan.has_destructive_steps()); + assert!(matches!( + &plan.steps[..], + [ + MigrationStep::RemoveRelation { .. }, + MigrationStep::AddRelation { .. } + ] + )); +} + +#[test] +fn renamed_collection_storage_transition_does_not_rename_nonexistent_column() { + let old = schema( + "Team", + vec![relation("members", "Person", Cardinality::Many)], + ); + let mut new = old.clone(); + new.fields[0].name = FieldName::new("people").unwrap(); + new.fields[0].derived_from = Some(FieldName::new("team").unwrap()); + new.fields[0] + .annotations + .push(schema_forge_core::types::FieldAnnotation::RenamedFrom { + name: FieldName::new("members").unwrap(), + }); + let plan = DiffEngine::plan_update(&old, &new).unwrap(); + assert!( + matches!(&plan.steps[..], [MigrationStep::RemoveRelation { name }] if name.as_str() == "members") + ); + let mut restored = new.clone(); + restored.fields[0].name = FieldName::new("restored").unwrap(); + restored.fields[0].derived_from = None; + restored.fields[0].annotations = vec![schema_forge_core::types::FieldAnnotation::RenamedFrom { + name: FieldName::new("people").unwrap(), + }]; + let plan = DiffEngine::plan_update(&new, &restored).unwrap(); + assert!(matches!( + &plan.steps[..], + [ + MigrationStep::RemoveRelation { .. }, + MigrationStep::AddRelation { .. } + ] + )); +} diff --git a/crates/schema-forge-dsl/tests/migration_hints.rs b/crates/schema-forge-dsl/tests/migration_hints.rs index 1366694..57e73aa 100644 --- a/crates/schema-forge-dsl/tests/migration_hints.rs +++ b/crates/schema-forge-dsl/tests/migration_hints.rs @@ -56,3 +56,19 @@ fn all_tenant_transitions_require_explicit_manual_migration() { } } } + +#[test] +fn cel_defaults_are_not_implicitly_evaluated_for_existing_rows() { + let old = schema("schema Widget { name: text }"); + for expression in ["0", "now()", "fields.other"] { + let new = schema(&format!( + r#"schema Widget {{ name: text priority: integer required @default("{expression}") }}"# + )); + let error = DiffEngine::plan_update(&old, &new).unwrap_err(); + assert!(error + .to_string() + .contains("CEL @default expressions are not evaluated")); + assert!(!DiffEngine::create_new(&new).is_empty()); + assert!(DiffEngine::plan_update(&new, &new).unwrap().is_empty()); + } +} diff --git a/crates/schema-forge-postgres/src/codegen.rs b/crates/schema-forge-postgres/src/codegen.rs index e440256..0992c50 100644 --- a/crates/schema-forge-postgres/src/codegen.rs +++ b/crates/schema-forge-postgres/src/codegen.rs @@ -50,44 +50,13 @@ pub fn migration_step_to_sql(table: &str, step: &MigrationStep) -> Vec { vec![format!("DROP TABLE IF EXISTS \"{table}\" CASCADE;")] } MigrationStep::AddField { field } => { - let pg_type = field_type_to_pg(&field.field_type); - let mut constraints = - field_check_constraints(table, field.name.as_ref(), &field.field_type); - - if field.is_required() { - constraints.push("NOT NULL".to_string()); - } - - let constraint_str = if constraints.is_empty() { - String::new() - } else { - format!(" {}", constraints.join(" ")) - }; - + // PostgreSQL only fills existing rows when DEFAULT is part of + // ADD COLUMN, before the NOT NULL constraint is enforced. + let (column, extra_statements) = field_to_column_def(table, field); let mut stmts = vec![format!( - "ALTER TABLE \"{table}\" ADD COLUMN IF NOT EXISTS \"{}\" {pg_type}{constraint_str};", - field.name + "ALTER TABLE \"{table}\" ADD COLUMN IF NOT EXISTS {column};" )]; - - // Add default value - for modifier in &field.modifiers { - if let FieldModifier::Default { value } = modifier { - let literal = default_value_to_sql(value); - stmts.push(format!( - "ALTER TABLE \"{table}\" ALTER COLUMN \"{}\" SET DEFAULT {literal};", - field.name - )); - } - } - - // Add index - if field.is_indexed() { - let idx_name = format!("idx_{table}_{}", field.name); - stmts.push(format!( - "CREATE INDEX IF NOT EXISTS \"{idx_name}\" ON \"{table}\" (\"{}\");", - field.name - )); - } + stmts.extend(extra_statements); stmts } @@ -627,9 +596,27 @@ mod tests { ), }; let stmts = migration_step_to_sql("Contact", &step); - assert_eq!(stmts.len(), 2); - assert!(stmts[0].contains("\"status\" TEXT")); - assert!(stmts[1].contains("SET DEFAULT 'active'")); + assert_eq!(stmts.len(), 1); + assert!(stmts[0].contains("\"status\" TEXT DEFAULT 'active'")); + } + + #[test] + fn add_required_field_default_is_inline_and_escaped() { + let step = MigrationStep::AddField { + field: FieldDefinition::with_modifiers( + FieldName::new("status").unwrap(), + FieldType::Text(TextConstraints::unconstrained()), + vec![ + FieldModifier::Required, + FieldModifier::Default { + value: DefaultValue::String("it's ready".into()), + }, + ], + ), + }; + let statements = migration_step_to_sql("Widget", &step); + assert_eq!(statements.len(), 1); + assert!(statements[0].contains("NOT NULL DEFAULT 'it''s ready'")); } #[test] diff --git a/crates/schema-forge-postgres/tests/migration_safety.rs b/crates/schema-forge-postgres/tests/migration_safety.rs index cbea470..659e3de 100644 --- a/crates/schema-forge-postgres/tests/migration_safety.rs +++ b/crates/schema-forge-postgres/tests/migration_safety.rs @@ -91,6 +91,8 @@ async fn relations_have_consistent_integrity_and_legacy_constraints_are_repaired } async fn exercise(backend: &PgBackend) { + defaults_populate_existing_rows_before_required_constraints(backend).await; + inverse_transitions_drop_and_recreate_storage(backend).await; atomic_schema_failure_preserves_data_and_metadata(backend).await; // Pet precedes Owner, and both reference each other. let mut pet = definition("Pet", Some(("owner", "Owner"))); @@ -423,3 +425,159 @@ async fn atomic_schema_failure_preserves_data_and_metadata(backend: &PgBackend) .unwrap() .is_none()); } + +async fn defaults_populate_existing_rows_before_required_constraints(backend: &PgBackend) { + use schema_forge_core::types::{DefaultValue, FieldModifier}; + let original = definition("DefaultMigration", None); + backend + .apply_schema_change( + &original.name, + &DiffEngine::create_new(&original).steps, + Some(&original), + ) + .await + .unwrap(); + let row = backend + .create(&Entity::new( + original.name.clone(), + BTreeMap::from([("label".into(), DynamicValue::Text("kept".into()))]), + )) + .await + .unwrap(); + let mut proposed = original.clone(); + for (name, required) in [("required_status", true), ("optional_status", false)] { + let mut field = FieldDefinition::new( + FieldName::new(name).unwrap(), + FieldType::Text(TextConstraints::unconstrained()), + ); + field.modifiers.push(FieldModifier::Default { + value: DefaultValue::String("draft".into()), + }); + if required { + field.modifiers.push(FieldModifier::Required); + } + proposed.fields.push(field); + } + let plan = DiffEngine::plan_update(&original, &proposed).unwrap(); + backend + .apply_schema_change(&original.name, &plan.steps, Some(&proposed)) + .await + .unwrap(); + let read = backend.get(&original.name, &row.id).await.unwrap(); + assert_eq!( + read.fields["required_status"], + DynamicValue::Text("draft".into()) + ); + assert_eq!( + read.fields["optional_status"], + DynamicValue::Text("draft".into()) + ); + // Explicit NULLs are backfilled, while populated values are preserved. + sqlx::query("UPDATE \"DefaultMigration\" SET optional_status = NULL") + .execute(backend.pool()) + .await + .unwrap(); + let kept = backend + .create(&Entity::new( + original.name.clone(), + BTreeMap::from([ + ("label".into(), DynamicValue::Text("other".into())), + ("required_status".into(), DynamicValue::Text("live".into())), + ("optional_status".into(), DynamicValue::Text("live".into())), + ]), + )) + .await + .unwrap(); + let before = proposed.clone(); + proposed.fields[2].modifiers.push(FieldModifier::Required); + let plan = DiffEngine::plan_update(&before, &proposed).unwrap(); + backend + .apply_schema_change(&original.name, &plan.steps, Some(&proposed)) + .await + .unwrap(); + assert_eq!( + backend.get(&original.name, &row.id).await.unwrap().fields["optional_status"], + DynamicValue::Text("draft".into()) + ); + assert_eq!( + backend.get(&original.name, &kept.id).await.unwrap().fields["optional_status"], + DynamicValue::Text("live".into()) + ); + assert!( + sqlx::query("UPDATE \"DefaultMigration\" SET optional_status = NULL") + .execute(backend.pool()) + .await + .is_err() + ); +} + +async fn inverse_transitions_drop_and_recreate_storage(backend: &PgBackend) { + let mut person = definition("InversePerson", None); + let mut team = definition("InverseTeam", None); + team.fields.push(FieldDefinition::new( + FieldName::new("members").unwrap(), + FieldType::Relation { + target: person.name.clone(), + cardinality: Cardinality::Many, + }, + )); + for schema in [&person, &team] { + backend + .apply_schema_change( + &schema.name, + &DiffEngine::create_new(schema).steps, + Some(schema), + ) + .await + .unwrap(); + } + sqlx::query( + "INSERT INTO \"InverseTeam\" (id, members) VALUES ('team_saved', ARRAY['person_saved'])", + ) + .execute(backend.pool()) + .await + .unwrap(); + let old_person = person.clone(); + person.fields.push(FieldDefinition::new( + FieldName::new("team").unwrap(), + FieldType::Relation { + target: team.name.clone(), + cardinality: Cardinality::One, + }, + )); + let plan = DiffEngine::plan_update(&old_person, &person).unwrap(); + backend + .apply_schema_change(&person.name, &plan.steps, Some(&person)) + .await + .unwrap(); + let stored_team = team.clone(); + team.fields[1].derived_from = Some(FieldName::new("team").unwrap()); + let plan = DiffEngine::plan_update(&stored_team, &team).unwrap(); + assert!(plan.has_destructive_steps()); + backend + .apply_schema_change(&team.name, &plan.steps, Some(&team)) + .await + .unwrap(); + let count: i64 = sqlx::query_scalar("SELECT count(*) FROM information_schema.columns WHERE table_schema = current_schema() AND table_name = 'InverseTeam' AND column_name = 'members'").fetch_one(backend.pool()).await.unwrap(); + assert_eq!(count, 0); + let plan = DiffEngine::plan_update(&person, &old_person).unwrap(); + backend + .apply_schema_change(&person.name, &plan.steps, Some(&old_person)) + .await + .unwrap(); + let plan = DiffEngine::plan_update(&team, &stored_team).unwrap(); + assert!(plan.has_destructive_steps()); + backend + .apply_schema_change(&team.name, &plan.steps, Some(&stored_team)) + .await + .unwrap(); + let old_ids: Option> = + sqlx::query_scalar("SELECT members FROM \"InverseTeam\" WHERE id = 'team_saved'") + .fetch_one(backend.pool()) + .await + .unwrap(); + assert_eq!( + old_ids, None, + "returning to stored semantics must not resurrect the discarded IDs" + ); +} diff --git a/docs/migrations/safe-schema-changes.md b/docs/migrations/safe-schema-changes.md index 5f43e32..a6e377f 100644 --- a/docs/migrations/safe-schema-changes.md +++ b/docs/migrations/safe-schema-changes.md @@ -193,3 +193,46 @@ destructive plan refuses the whole batch unless `--force` is present. Dry runs remain available without force. Interactive users can still approve or skip individual destructive schemas. Backend failures during execution can still leave earlier schemas committed; preflight is not a transaction for the batch. + +## Required fields and existing rows + +PostgreSQL adds literal `default(...)` values inline with the new column, so +both optional and required additions populate existing rows. Changing an +optional field to required emits a NULL backfill before installing `NOT NULL`; +non-NULL values remain unchanged. + +For an existing schema, adding a required field or making an optional field +required needs a usable literal default compatible with the declared field type +and constraints. Planning refuses the change otherwise, even if the table is +currently empty or all rows already have values. Fresh schema creation and +unchanged required fields remain valid without defaults. This is a conservative +schema-only preflight; it does not inspect existing data. + +CEL `@default("...")` rules run during entity writes and are not evaluated for +migration backfill, including constant expressions such as `@default("0")`. +Use `default(0)` for an integer migration default instead. If no literal is +appropriate, add the field as optional and perform a reviewed manual data, +constraint, and schema-metadata migration. Destructive approval does not bypass +this refusal. + +## Inverse collections and storage changes + +A collection `Team.members: -> Person[]` is stored when Person has no to-one +relation back to Team. Exactly one such relation makes it a derived collection; +two or more are ambiguous and rejected by `parse` as well as write commands. + +Adding or removing a child foreign key can therefore change the parent's +storage even when the parent's source text is unchanged. Plans now include a +destructive `REMOVE RELATION` for the affected parent field. Stored-to-derived +removes the unused column and its IDs; it does not translate those IDs into +child foreign keys. Derived-to-stored removes any legacy orphan column and +creates an empty stored column. Changing a derived collection's inverse key or +target is also destructive. Migrate relationships explicitly before approving +these changes if their membership must be retained. + +The usual destructive gates apply to CLI batches and startup. Direct runtime +startup refuses unresolved storage transitions in persisted metadata. REST +single-schema changes that would alter a sibling schema's pairing are refused; +apply the complete schema batch during a maintenance window, review the parent +and child plans, then restart. Batch preflight is not a transaction across all +schemas, so keep concurrent writers offline for the migration. diff --git a/skills/schemaforge/dsl-reference.md b/skills/schemaforge/dsl-reference.md index e47a2bd..80fa19e 100644 --- a/skills/schemaforge/dsl-reference.md +++ b/skills/schemaforge/dsl-reference.md @@ -258,14 +258,18 @@ schema Contact { show as `[]`, never `null`. - Writes to the derived field are rejected with `422` — persist the relationship by writing `Contact.company` on the child. -- Migrations never emit a column for a derived field. Nothing to drift. +- New derived fields have no physical column. Changing an existing field's + stored/derived status is a destructive migration: the old column is removed, + and returning to stored semantics creates an empty column. Stored IDs are + not converted into child foreign keys. Review the complete parent/child batch + before using `--force` or `serve --allow-destructive-migrations`. If the target schema has **no** FK pointing back, `-> X[]` keeps its older stored-array behavior (use it for many-to-many / tag-style lists where both sides are independent). Two FKs from the same child back to the same parent is rejected at -schema-load time with an "ambiguous inverse" error. Fix it by removing +`parse` and schema-load time with an "ambiguous inverse" error. Fix it by removing the duplicate FK — the DSL does not currently support an `@inverse` annotation to disambiguate. From 8f89a5b690a74e27af6eaa93b5b06367791e1794 Mon Sep 17 00:00:00 2001 From: Roland Rodriguez Date: Fri, 25 Sep 2026 15:08:21 -0600 Subject: [PATCH 12/21] fix(migration): share validated literal defaults with backends --- crates/schema-forge-core/src/migration.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/schema-forge-core/src/migration.rs b/crates/schema-forge-core/src/migration.rs index 2b9d2bb..0b9cc72 100644 --- a/crates/schema-forge-core/src/migration.rs +++ b/crates/schema-forge-core/src/migration.rs @@ -556,7 +556,12 @@ impl DiffEngine { Ok(()) } - fn backfill_value(field: &FieldDefinition) -> Option { + /// Resolve a type-compatible literal migration default, checking its constraints. + /// + /// Returns `None` when no supported literal is available. CEL default rules + /// are deliberately not evaluated: they belong to the entity-write pipeline. + /// Backends use this same conversion when backfilling newly added fields. + pub fn backfill_value(field: &FieldDefinition) -> Option { let default = Self::extract_default(&field.modifiers)?; let value = match (&field.field_type, default) { (FieldType::Text(_) | FieldType::RichText, DefaultValue::String(value)) => { From a34a973bcbd66ff9dcabd0a727ada3a4627614ab Mon Sep 17 00:00:00 2001 From: Roland Rodriguez Date: Fri, 25 Sep 2026 15:08:51 -0600 Subject: [PATCH 13/21] ci(mssql): run unit and migration regression suites --- .github/workflows/mssql-integration.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/mssql-integration.yml b/.github/workflows/mssql-integration.yml index f032783..4327d76 100644 --- a/.github/workflows/mssql-integration.yml +++ b/.github/workflows/mssql-integration.yml @@ -98,3 +98,6 @@ jobs: - name: Deny SQL Server lints if: matrix.version == 2019 run: cargo clippy -p schema-forge-mssql --all-targets -- -D warnings + - name: Run SQL Server unit and migration tests + if: matrix.version == 2019 + run: cargo nextest run -p schema-forge-mssql --no-fail-fast From 7859648d224fa0663d5f834068c351283372337e Mon Sep 17 00:00:00 2001 From: Roland Rodriguez Date: Fri, 25 Sep 2026 15:11:13 -0600 Subject: [PATCH 14/21] fix(mssql): apply migration backfills and remove relation data --- crates/schema-forge-mssql/src/backend.rs | 121 +++++++++++- crates/schema-forge-mssql/tests/sql_server.rs | 187 ++++++++++++++++++ 2 files changed, 303 insertions(+), 5 deletions(-) diff --git a/crates/schema-forge-mssql/src/backend.rs b/crates/schema-forge-mssql/src/backend.rs index 2be7fcb..44ca20c 100644 --- a/crates/schema-forge-mssql/src/backend.rs +++ b/crates/schema-forge-mssql/src/backend.rs @@ -4,7 +4,7 @@ use std::collections::BTreeMap; use acton_service::config::DatabaseConfig; use acton_service::mssql::{create_pool, MssqlPool}; use schema_forge_backend::{BackendError, Entity, EntityStore, QueryResult, SchemaBackend}; -use schema_forge_core::migration::{MigrationStep, ValueTransform}; +use schema_forge_core::migration::{DiffEngine, MigrationStep, ValueTransform}; use schema_forge_core::query::{ AggregateOp, AggregateQuery, AggregateResult, FieldPath, Filter, Query, SortOrder, }; @@ -71,12 +71,33 @@ impl MssqlBackend { fn migration_statements( schema_name: &SchemaName, steps: &[MigrationStep], -) -> (String, Vec) { +) -> Result<(String, Vec), BackendError> { let table = quote(schema_name.as_str()); let mut statements = Vec::new(); let mut parameters = Vec::new(); for step in steps { match step { + MigrationStep::RemoveRelation { name } | MigrationStep::RemoveField { name } => { + let parameter = parameters.len() + 1; + parameters.push(format!("$.\"{name}\"")); + statements.push(format!("UPDATE [dbo].{table} SET [data] = JSON_MODIFY([data], @P{parameter}, NULL);")); + } + MigrationStep::BackfillRequired { field, default_value } => { + append_backfill(&table, field.as_str(), default_value, &mut statements, &mut parameters)?; + } + MigrationStep::AddField { field } => { + if let Some(value) = DiffEngine::backfill_value(field) { + append_backfill(&table, field.name.as_str(), &value, &mut statements, &mut parameters)?; + } else if field.modifiers.iter().any(|modifier| matches!(modifier, schema_forge_core::types::FieldModifier::Default { .. })) { + return Err(BackendError::MigrationFailed { step: step.to_string(), reason: "literal default is incompatible with the field type".into() }); + } + if field.is_required() { + append_required_check(&table, field.name.as_str(), &mut statements, &mut parameters); + } + } + MigrationStep::AddRequired { field } => { + append_required_check(&table, field.as_str(), &mut statements, &mut parameters); + } MigrationStep::RenameField { old_name, new_name } => { let old_parameter = parameters.len() + 1; let new_parameter = old_parameter + 1; @@ -102,7 +123,47 @@ fn migration_statements( _ => {} } } - (statements.join("\n"), parameters) + Ok((statements.join("\n"), parameters)) +} + +/// Preserve existing non-null values while filling absent or tagged-null fields. +fn append_backfill( + table: &str, + field: &str, + value: &DynamicValue, + statements: &mut Vec, + parameters: &mut Vec, +) -> Result<(), BackendError> { + if matches!(value, DynamicValue::Null) + || matches!(value, DynamicValue::Float(number) if !number.is_finite()) + { + return Err(BackendError::MigrationFailed { + step: format!("backfill '{field}'"), + reason: "required backfill must have a non-null, finite value".into(), + }); + } + let path = parameters.len() + 1; + let tag = path + 1; + let default = path + 2; + parameters.extend([ + format!("$.\"{field}\""), + format!("$.\"{field}\".type"), + serde_json::to_string(value).map_err(json_error)?, + ]); + statements.push(format!("UPDATE [dbo].{table} SET [data] = JSON_MODIFY([data], @P{path}, JSON_QUERY(@P{default})) WHERE JSON_QUERY([data], @P{path}) IS NULL OR JSON_VALUE([data], @P{tag}) = N'Null';")); + Ok(()) +} + +fn append_required_check( + table: &str, + field: &str, + statements: &mut Vec, + parameters: &mut Vec, +) { + let path = parameters.len() + 1; + let tag = path + 1; + parameters.extend([format!("$.\"{field}\""), format!("$.\"{field}\".type")]); + statements.push(format!("IF EXISTS (SELECT 1 FROM [dbo].{table} WITH (UPDLOCK, HOLDLOCK) WHERE JSON_QUERY([data], @P{path}) IS NULL OR JSON_VALUE([data], @P{tag}) = N'Null') THROW 50002, 'required field has missing or null values', 1;")); } fn transaction_batch(statements: &str) -> String { @@ -171,7 +232,7 @@ impl SchemaBackend for MssqlBackend { } } self.validate_migration(name, steps).await?; - let (mut sql, mut parameters) = migration_statements(name, steps); + let (mut sql, mut parameters) = migration_statements(name, steps)?; let name_parameter = parameters.len() + 1; parameters.push(name.to_string()); if let Some(definition) = definition { @@ -204,7 +265,7 @@ impl SchemaBackend for MssqlBackend { steps: &[MigrationStep], ) -> Result<(), BackendError> { self.validate_migration(schema_name, steps).await?; - let (sql, parameters) = migration_statements(schema_name, steps); + let (sql, parameters) = migration_statements(schema_name, steps)?; if sql.is_empty() { return Ok(()); } @@ -698,6 +759,56 @@ mod tests { use super::{aggregate_value, matches_filter, quote, sort_entities}; + #[test] + fn invalid_required_default_rejects_entire_migration_before_sql_execution() { + use schema_forge_core::migration::MigrationStep; + use schema_forge_core::types::{ + DefaultValue, FieldDefinition, FieldModifier, FieldName, FieldType, + }; + let field = FieldDefinition::with_modifiers( + FieldName::new("active").unwrap(), + FieldType::Boolean, + vec![ + FieldModifier::Required, + FieldModifier::Default { + value: DefaultValue::String("invalid".into()), + }, + ], + ); + let result = super::migration_statements( + &SchemaName::new("Item").unwrap(), + &[ + MigrationStep::RemoveField { + name: FieldName::new("old").unwrap(), + }, + MigrationStep::AddField { field }, + ], + ); + assert!(matches!( + result, + Err(schema_forge_backend::BackendError::MigrationFailed { .. }) + )); + } + + #[test] + fn backfill_values_are_bound_as_tagged_json_instead_of_interpolated_sql() { + use schema_forge_core::{migration::MigrationStep, types::FieldName}; + let value = DynamicValue::Text("'quoted' \"name\" \n text".into()); + let (sql, bindings) = super::migration_statements( + &SchemaName::new("Item").unwrap(), + &[MigrationStep::BackfillRequired { + field: FieldName::new("label").unwrap(), + default_value: value.clone(), + }], + ) + .unwrap(); + assert!(!sql.contains("quoted")); + assert_eq!( + serde_json::from_str::(&bindings[2]).unwrap(), + value + ); + } + fn entity(name: &str, score: i64) -> Entity { Entity::new( SchemaName::new("Player").unwrap(), diff --git a/crates/schema-forge-mssql/tests/sql_server.rs b/crates/schema-forge-mssql/tests/sql_server.rs index 3d7922b..4dc25a8 100644 --- a/crates/schema-forge-mssql/tests/sql_server.rs +++ b/crates/schema-forge-mssql/tests/sql_server.rs @@ -68,6 +68,7 @@ async fn connects_and_initializes_metadata(image_tag: &str) { data_correctness::exercise(&backend).await; migration_renames::exercise(&backend).await; rename_collision_rolls_back_entire_plan(&backend).await; + migration_backfills_and_relation_removal(&backend).await; } async fn rename_collision_rolls_back_entire_plan(backend: &MssqlBackend) { @@ -398,3 +399,189 @@ async fn sparse_updates_preserve_other_fields_and_explicit_null(backend: &MssqlB let empty = Entity::with_id(initial.id, schema.name, BTreeMap::new()); assert_eq!(backend.update(&empty).await.unwrap(), updated); } + +async fn migration_backfills_and_relation_removal(backend: &MssqlBackend) { + use schema_forge_core::types::{Cardinality, DefaultValue, FieldModifier}; + let name = SchemaName::new("MigrationValues").unwrap(); + let schema = SchemaDefinition::new( + SchemaId::new(), + name.clone(), + vec![ + FieldDefinition::new( + FieldName::new("value").unwrap(), + FieldType::Text(TextConstraints::unconstrained()), + ), + FieldDefinition::new( + FieldName::new("links").unwrap(), + FieldType::Relation { + target: name.clone(), + cardinality: Cardinality::Many, + }, + ), + ], + vec![], + ) + .unwrap(); + backend + .apply_schema_change(&name, &DiffEngine::create_new(&schema).steps, Some(&schema)) + .await + .unwrap(); + let missing = backend + .create(&Entity::new(name.clone(), BTreeMap::new())) + .await + .unwrap(); + let null = backend + .create(&Entity::new( + name.clone(), + BTreeMap::from([ + ("value".into(), DynamicValue::Null), + ( + "links".into(), + DynamicValue::RefArray(vec![missing.id.clone()]), + ), + ]), + )) + .await + .unwrap(); + let present = backend + .create(&Entity::new( + name.clone(), + BTreeMap::from([("value".into(), DynamicValue::Text("preserve".into()))]), + )) + .await + .unwrap(); + let literal = format!("O'Reilly {{value}} {}", "x".repeat(4500)); + let mut updated = schema.clone(); + updated.fields[0].modifiers = vec![ + FieldModifier::Required, + FieldModifier::Default { + value: DefaultValue::String(literal.clone()), + }, + ]; + let defaults = [ + ( + "count", + FieldType::Integer(IntegerConstraints::unconstrained()), + DefaultValue::Integer(7), + DynamicValue::Integer(7), + ), + ( + "active", + FieldType::Boolean, + DefaultValue::Boolean(true), + DynamicValue::Boolean(true), + ), + ( + "rate", + FieldType::Float(schema_forge_core::types::FloatConstraints::unconstrained()), + DefaultValue::float("1.25").unwrap(), + DynamicValue::Float(1.25), + ), + ( + "status", + FieldType::Enum( + schema_forge_core::types::EnumVariants::new(vec!["open".into(), "closed".into()]) + .unwrap(), + ), + DefaultValue::String("open".into()), + DynamicValue::Enum("open".into()), + ), + ( + "created", + FieldType::DateTime, + DefaultValue::String("2026-01-01T00:00:00Z".into()), + DynamicValue::DateTime("2026-01-01T00:00:00Z".parse().unwrap()), + ), + ]; + let mut steps = vec![ + MigrationStep::BackfillRequired { + field: FieldName::new("value").unwrap(), + default_value: DynamicValue::Text(literal.clone()), + }, + MigrationStep::AddRequired { + field: FieldName::new("value").unwrap(), + }, + ]; + for (field_name, field_type, default, _) in &defaults { + let field = FieldDefinition::with_modifiers( + FieldName::new(*field_name).unwrap(), + field_type.clone(), + vec![ + FieldModifier::Required, + FieldModifier::Default { + value: default.clone(), + }, + ], + ); + steps.push(MigrationStep::AddField { + field: field.clone(), + }); + updated.fields.push(field); + } + backend + .apply_schema_change(&name, &steps, Some(&updated)) + .await + .unwrap(); + for (row, expected) in [ + (&missing, literal.as_str()), + (&null, literal.as_str()), + (&present, "preserve"), + ] { + let loaded = backend.get(&name, &row.id).await.unwrap(); + assert_eq!( + loaded.field("value"), + Some(&DynamicValue::Text(expected.into())) + ); + for (field, _, _, expected) in &defaults { + assert_eq!(loaded.field(field), Some(expected)); + } + } + backend + .apply_migration( + &name, + &[ + MigrationStep::RemoveRelation { + name: FieldName::new("links").unwrap(), + }, + MigrationStep::AddRelation { + name: FieldName::new("links").unwrap(), + target: name.clone(), + cardinality: Cardinality::Many, + }, + ], + ) + .await + .unwrap(); + assert!( + backend + .get(&name, &null.id) + .await + .unwrap() + .field("links") + .is_none(), + "removed relation data must never reappear on re-add" + ); + let result = backend + .apply_migration( + &name, + &[ + MigrationStep::RemoveField { + name: FieldName::new("value").unwrap(), + }, + MigrationStep::AddRequired { + field: FieldName::new("value").unwrap(), + }, + ], + ) + .await; + assert!(result.is_err(), "required check must reject missing data"); + assert_eq!( + backend + .get(&name, &present.id) + .await + .unwrap() + .field("value"), + Some(&DynamicValue::Text("preserve".into())), + "failed plan must roll back all prior edits" + ); +} From cf54a331328eaa2f0ecc762211026323bb010485 Mon Sep 17 00:00:00 2001 From: Roland Rodriguez Date: Fri, 25 Sep 2026 15:12:32 -0600 Subject: [PATCH 15/21] fix(site): associate relation picker with its field label --- .../templates/site/src/app/pages/edit.generated.tsx.jinja | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/schema-forge-cli/templates/site/src/app/pages/edit.generated.tsx.jinja b/crates/schema-forge-cli/templates/site/src/app/pages/edit.generated.tsx.jinja index 6179d26..dbcb27a 100644 --- a/crates/schema-forge-cli/templates/site/src/app/pages/edit.generated.tsx.jinja +++ b/crates/schema-forge-cli/templates/site/src/app/pages/edit.generated.tsx.jinja @@ -90,6 +90,7 @@ import { /> {%- elif f.kind == "relation_one" %} Date: Fri, 25 Sep 2026 15:12:27 -0600 Subject: [PATCH 16/21] fix(surrealdb): preserve field constraints during modifier migrations --- crates/schema-forge-surrealdb/src/backend.rs | 187 ++++++++---- crates/schema-forge-surrealdb/src/codegen.rs | 53 ++-- .../tests/migration_modifiers.rs | 289 ++++++++++++++++++ 3 files changed, 448 insertions(+), 81 deletions(-) create mode 100644 crates/schema-forge-surrealdb/tests/migration_modifiers.rs diff --git a/crates/schema-forge-surrealdb/src/backend.rs b/crates/schema-forge-surrealdb/src/backend.rs index 34a1280..b1df18e 100644 --- a/crates/schema-forge-surrealdb/src/backend.rs +++ b/crates/schema-forge-surrealdb/src/backend.rs @@ -11,7 +11,9 @@ use schema_forge_backend::error::BackendError; use schema_forge_backend::traits::{EntityStore, SchemaBackend}; use schema_forge_core::migration::MigrationStep; use schema_forge_core::query::{AggregateQuery, AggregateResult, Query}; -use schema_forge_core::types::{DynamicValue, EntityId, FieldType, SchemaDefinition, SchemaName}; +use schema_forge_core::types::{ + DynamicValue, EntityId, FieldDefinition, FieldModifier, FieldType, SchemaDefinition, SchemaName, +}; use surrealdb::engine::any::Any; use surrealdb::types::ToSql; use surrealdb::Surreal; @@ -313,40 +315,28 @@ impl SurrealBackend { steps: &[MigrationStep], ) -> Result, BackendError> { let table = schema_name.as_str(); - let needs_enum_metadata = steps - .iter() - .any(|step| matches!(step, MigrationStep::RenameField { .. })) - || steps.iter().any(|step| { - matches!( - step, - MigrationStep::ChangeType { - old_type: FieldType::Enum(_), - new_type: FieldType::Enum(_), - .. - } - ) - }); - let metadata = if needs_enum_metadata { - self.load_schema_metadata(schema_name).await? - } else { - None - }; + let metadata = self.load_schema_metadata(schema_name).await?; + let mut fields: BTreeMap = metadata + .as_ref() + .map(|schema| { + schema + .fields + .iter() + .map(|field| (field.name.to_string(), field.clone())) + .collect() + }) + .unwrap_or_default(); let mut statements = Vec::new(); let mut rename_cleanup = Vec::new(); for step in steps { let mut compiled = if let MigrationStep::RenameField { old_name, new_name } = step { - let schema = metadata - .as_ref() - .ok_or_else(|| BackendError::MigrationFailed { - step: step.to_string(), - reason: "rename requires stored schema metadata".into(), - })?; - let field = schema.field(old_name.as_str()).ok_or_else(|| { - BackendError::MigrationFailed { - step: step.to_string(), - reason: "rename source missing from stored metadata".into(), - } - })?; + let field = + fields + .get(old_name.as_str()) + .ok_or_else(|| BackendError::MigrationFailed { + step: step.to_string(), + reason: "rename source missing from stored metadata".into(), + })?; // SurrealDB's transaction-local field refresh after REMOVE FIELD // can strip unrelated object data on a later UPDATE. Complete // every data write before removing renamed source definitions. @@ -355,35 +345,25 @@ impl SurrealBackend { table, field, new_name, - schema.unique_scoped_by_tenant(), + metadata + .as_ref() + .is_some_and(SchemaDefinition::unique_scoped_by_tenant), ) } else { migration_step_to_surql(table, step) }; - if let MigrationStep::ChangeType { - name, - old_type: FieldType::Enum(_), - new_type: new_type @ FieldType::Enum(_), - .. - } = step - { - let original_name = steps - .iter() - .find_map(|candidate| match candidate { - MigrationStep::RenameField { old_name, new_name } if new_name == name => { - Some(old_name) - } - _ => None, - }) - .unwrap_or(name); - let field = metadata.as_ref().and_then(|schema| schema.field(original_name.as_str())).ok_or_else(|| BackendError::MigrationFailed { step: step.to_string(), reason: "enum migration requires stored field metadata to preserve required/default modifiers".into() })?; - let mut field = field.clone(); - field.name = name.clone(); - field.field_type = new_type.clone(); - compiled.pop(); + if let Some(field) = advance_field_definition(&mut fields, step)? { + // ChangeType may first rewrite removed enum variants. Keep that + // data rewrite, then restore the entire field definition. + if matches!(step, MigrationStep::ChangeType { .. }) { + compiled.pop(); + } else { + compiled.clear(); + } compiled.extend( crate::codegen::define_field_stmts(table, &field) .into_iter() + .filter(|sql| sql.starts_with("DEFINE FIELD ")) .map(|sql| sql.replacen("DEFINE FIELD ", "DEFINE FIELD OVERWRITE ", 1)), ); } @@ -394,6 +374,107 @@ impl SurrealBackend { } } +// Track definitions in migration order, so a rename/type change followed by +// required/default changes retains the new name and type as well as all +// unrelated modifiers. Modifier DDL cannot safely be compiled without metadata. +fn advance_field_definition( + fields: &mut BTreeMap, + step: &MigrationStep, +) -> Result, BackendError> { + match step { + MigrationStep::CreateSchema { + fields: created, .. + } => { + fields.extend( + created + .iter() + .map(|field| (field.name.to_string(), field.clone())), + ); + } + MigrationStep::AddField { field } => { + fields.insert(field.name.to_string(), field.clone()); + } + MigrationStep::RenameField { old_name, new_name } => { + if let Some(mut field) = fields.remove(old_name.as_str()) { + field.name = new_name.clone(); + fields.insert(new_name.to_string(), field); + } + } + MigrationStep::RemoveField { name } | MigrationStep::RemoveRelation { name } => { + fields.remove(name.as_str()); + } + MigrationStep::AddRelation { + name, + target, + cardinality, + } => { + fields.insert( + name.to_string(), + FieldDefinition::new( + name.clone(), + FieldType::Relation { + target: target.clone(), + cardinality: *cardinality, + }, + ), + ); + } + MigrationStep::AddRequired { field } + | MigrationStep::RemoveRequired { field } + | MigrationStep::SetDefault { field, .. } + | MigrationStep::RemoveDefault { field } => { + let definition = + fields + .get_mut(field.as_str()) + .ok_or_else(|| BackendError::MigrationFailed { + step: step.to_string(), + reason: "field modifier migration requires stored field metadata".into(), + })?; + match step { + MigrationStep::AddRequired { .. } => { + if !definition.is_required() { + definition.modifiers.push(FieldModifier::Required); + } + } + MigrationStep::RemoveRequired { .. } => { + definition + .modifiers + .retain(|modifier| !matches!(modifier, FieldModifier::Required)); + } + MigrationStep::SetDefault { value, .. } => { + definition + .modifiers + .retain(|modifier| !matches!(modifier, FieldModifier::Default { .. })); + definition.modifiers.push(FieldModifier::Default { + value: value.clone(), + }); + } + MigrationStep::RemoveDefault { .. } => { + definition + .modifiers + .retain(|modifier| !matches!(modifier, FieldModifier::Default { .. })); + } + _ => unreachable!(), + } + return Ok(Some(definition.clone())); + } + MigrationStep::ChangeType { name, new_type, .. } => { + if let Some(definition) = fields.get_mut(name.as_str()) { + definition.field_type = new_type.clone(); + return Ok(Some(definition.clone())); + } + if matches!(new_type, FieldType::Enum(_)) { + return Err(BackendError::MigrationFailed { + step: step.to_string(), + reason: "enum migration requires stored field metadata".into(), + }); + } + } + _ => {} + } + Ok(None) +} + impl SchemaBackend for SurrealBackend { async fn apply_schema_change( &self, diff --git a/crates/schema-forge-surrealdb/src/codegen.rs b/crates/schema-forge-surrealdb/src/codegen.rs index 745165f..47f69ce 100644 --- a/crates/schema-forge-surrealdb/src/codegen.rs +++ b/crates/schema-forge-surrealdb/src/codegen.rs @@ -39,7 +39,19 @@ pub fn migration_step_to_surql(table: &str, step: &MigrationStep) -> Vec MigrationStep::DropSchema { name: _ } => { vec![format!("REMOVE TABLE {table};")] } - MigrationStep::AddField { field } => define_field_stmts(table, field), + MigrationStep::AddField { field } => { + let mut statements = define_field_stmts(table, field); + for modifier in &field.modifiers { + if let FieldModifier::Default { value } = modifier { + let literal = default_value_to_surql(value); + statements.push(format!( + "UPDATE {table} SET {name} = {literal} WHERE {name} = NONE OR {name} = NULL;", + name = field.name, + )); + } + } + statements + } MigrationStep::RemoveField { name } => { vec![format!("REMOVE FIELD {name} ON {table};")] } @@ -117,7 +129,7 @@ pub fn migration_step_to_surql(table: &str, step: &MigrationStep) -> Vec } }, MigrationStep::RemoveRelation { name } => { - vec![format!("REMOVE FIELD {name} ON {table};")] + vec![format!("REMOVE FIELD IF EXISTS {name} ON {table};")] } MigrationStep::BackfillRequired { field, @@ -125,33 +137,15 @@ pub fn migration_step_to_surql(table: &str, step: &MigrationStep) -> Vec } => { let literal = crate::query::dynamic_value_to_surql_literal(default_value); vec![format!( - "UPDATE {table} SET {field} = {literal} WHERE {field} = NONE;" - )] - } - MigrationStep::AddRequired { field } => { - // Re-define the field with a NOT NONE assertion. - // Since we do not have the full field type here, use a flexible assertion. - vec![format!( - "DEFINE FIELD OVERWRITE {field} ON {table} ASSERT $value != NONE;" - )] - } - MigrationStep::RemoveRequired { field } => { - // Re-define the field without the assertion. Use `any` type to be permissive. - vec![format!( - "DEFINE FIELD OVERWRITE {field} ON {table} TYPE any;" + "UPDATE {table} SET {field} = {literal} WHERE {field} = NONE OR {field} = NULL;" )] } - MigrationStep::SetDefault { field, value } => { - let literal = default_value_to_surql(value); - vec![format!( - "DEFINE FIELD OVERWRITE {field} ON {table} DEFAULT {literal};" - )] - } - MigrationStep::RemoveDefault { field } => { - // Re-define without VALUE clause. - vec![format!( - "DEFINE FIELD OVERWRITE {field} ON {table} TYPE any;" - )] + MigrationStep::AddRequired { .. } + | MigrationStep::RemoveRequired { .. } + | MigrationStep::SetDefault { .. } + | MigrationStep::RemoveDefault { .. } => { + // Retain type, constraints and other modifiers through stored metadata. + vec!["THROW 'field modifier migration requires stored field metadata; execute through SchemaBackend';".into()] } MigrationStep::AddUnique { field, per_tenant } => { vec![add_unique_surql(table, field.as_ref(), *per_tenant)] @@ -731,7 +725,10 @@ mod tests { let stmts = migration_step_to_surql("Contact", &step); assert_eq!( stmts, - vec!["DEFINE FIELD status ON Contact TYPE option DEFAULT 'active';"] + vec![ + "DEFINE FIELD status ON Contact TYPE option DEFAULT 'active';", + "UPDATE Contact SET status = 'active' WHERE status = NONE OR status = NULL;", + ] ); } diff --git a/crates/schema-forge-surrealdb/tests/migration_modifiers.rs b/crates/schema-forge-surrealdb/tests/migration_modifiers.rs new file mode 100644 index 0000000..a83d72e --- /dev/null +++ b/crates/schema-forge-surrealdb/tests/migration_modifiers.rs @@ -0,0 +1,289 @@ +//! Field modifier migrations retain complete definitions and remain atomic. +use std::collections::BTreeMap; + +use schema_forge_backend::{Entity, EntityStore, SchemaBackend}; +use schema_forge_core::{ + migration::{DiffEngine, MigrationStep}, + types::*, +}; +use schema_forge_surrealdb::SurrealBackend; + +async fn fixture(namespace: &str) -> (SurrealBackend, SchemaDefinition, Entity) { + let backend = SurrealBackend::connect_memory(namespace, namespace) + .await + .unwrap(); + let schema = SchemaDefinition::new( + SchemaId::new(), + SchemaName::new("Widget").unwrap(), + vec![FieldDefinition::with_modifiers( + FieldName::new("status").unwrap(), + FieldType::Text(TextConstraints::with_max_length(8)), + vec![FieldModifier::Indexed], + )], + vec![], + ) + .unwrap(); + backend + .apply_schema_change( + &schema.name, + &DiffEngine::create_new(&schema).steps, + Some(&schema), + ) + .await + .unwrap(); + let row = Entity::new(schema.name.clone(), BTreeMap::new()); + backend.create(&row).await.unwrap(); + (backend, schema, row) +} + +#[tokio::test] +async fn backfill_and_required_default_changes_preserve_type_and_constraints() { + let (backend, mut schema, row) = fixture("modifiers").await; + let field = schema.fields[0].name.clone(); + let steps = [ + MigrationStep::BackfillRequired { + field: field.clone(), + default_value: DynamicValue::Text("draft".into()), + }, + MigrationStep::AddRequired { + field: field.clone(), + }, + MigrationStep::SetDefault { + field: field.clone(), + value: DefaultValue::String("draft".into()), + }, + ]; + schema.fields[0].modifiers.extend([ + FieldModifier::Required, + FieldModifier::Default { + value: DefaultValue::String("draft".into()), + }, + ]); + backend + .apply_schema_change(&schema.name, &steps, Some(&schema)) + .await + .unwrap(); + assert_eq!( + backend + .get(&schema.name, &row.id) + .await + .unwrap() + .field("status"), + Some(&DynamicValue::Text("draft".into())) + ); + let defaulted = Entity::new(schema.name.clone(), BTreeMap::new()); + backend.create(&defaulted).await.unwrap(); + assert_eq!( + backend + .get(&schema.name, &defaulted.id) + .await + .unwrap() + .field("status"), + Some(&DynamicValue::Text("draft".into())) + ); + for value in [ + DynamicValue::Text("longer-than-eight".into()), + DynamicValue::Integer(7), + ] { + let invalid = Entity::new( + schema.name.clone(), + BTreeMap::from([("status".into(), value)]), + ); + assert!(backend.create(&invalid).await.is_err()); + } + schema.fields[0] + .modifiers + .retain(|modifier| !matches!(modifier, FieldModifier::Default { .. })); + backend + .apply_schema_change( + &schema.name, + &[MigrationStep::RemoveDefault { + field: field.clone(), + }], + Some(&schema), + ) + .await + .unwrap(); + assert!(backend + .create(&Entity::new(schema.name.clone(), BTreeMap::new())) + .await + .is_err()); + schema.fields[0] + .modifiers + .retain(|modifier| !matches!(modifier, FieldModifier::Required)); + backend + .apply_schema_change( + &schema.name, + &[MigrationStep::RemoveRequired { field }], + Some(&schema), + ) + .await + .unwrap(); + backend + .create(&Entity::new(schema.name.clone(), BTreeMap::new())) + .await + .unwrap(); + assert!(backend + .create(&Entity::new( + schema.name.clone(), + BTreeMap::from([("status".into(), DynamicValue::Text("still-too-long".into()))]) + )) + .await + .is_err()); +} + +#[tokio::test] +async fn rename_then_required_default_changes_use_the_renamed_definition() { + let (backend, mut schema, row) = fixture("renamedmodifiers").await; + let old_name = schema.fields[0].name.clone(); + let new_name = FieldName::new("phase").unwrap(); + let steps = [ + MigrationStep::RenameField { + old_name, + new_name: new_name.clone(), + }, + MigrationStep::BackfillRequired { + field: new_name.clone(), + default_value: DynamicValue::Text("ready".into()), + }, + MigrationStep::AddRequired { + field: new_name.clone(), + }, + MigrationStep::SetDefault { + field: new_name.clone(), + value: DefaultValue::String("ready".into()), + }, + ]; + // apply_migration intentionally exercises intermediate step compilation; + // schema metadata remains old until the caller commits the final definition. + backend.apply_migration(&schema.name, &steps).await.unwrap(); + schema.fields[0].name = new_name; + schema.fields[0].modifiers.extend([ + FieldModifier::Required, + FieldModifier::Default { + value: DefaultValue::String("ready".into()), + }, + ]); + backend.store_schema_metadata(&schema).await.unwrap(); + assert_eq!( + backend + .get(&schema.name, &row.id) + .await + .unwrap() + .field("phase"), + Some(&DynamicValue::Text("ready".into())) + ); + assert!(backend + .create(&Entity::new( + schema.name.clone(), + BTreeMap::from([( + "phase".into(), + DynamicValue::Text("longer-than-eight".into()) + )]) + )) + .await + .is_err()); +} + +#[tokio::test] +async fn failed_required_change_rolls_back_prior_backfill() { + let (backend, schema, row) = fixture("modifierrollback").await; + let mut proposed = schema.clone(); + proposed.fields[0].modifiers.extend([ + FieldModifier::Required, + FieldModifier::Default { + value: DefaultValue::String("draft".into()), + }, + ]); + let steps = [ + MigrationStep::BackfillRequired { + field: schema.fields[0].name.clone(), + default_value: DynamicValue::Text("draft".into()), + }, + MigrationStep::AddRequired { + field: schema.fields[0].name.clone(), + }, + MigrationStep::SetDefault { + field: schema.fields[0].name.clone(), + value: DefaultValue::String("draft".into()), + }, + ]; + backend + .client() + .query("DEFINE FIELD definition ON _schema_metadata TYPE string ASSERT $value = $original;") + .bind(("original", serde_json::to_string(&schema).unwrap())) + .await + .unwrap() + .check() + .unwrap(); + assert!(backend + .apply_schema_change(&schema.name, &steps, Some(&proposed)) + .await + .is_err()); + assert_eq!( + backend + .get(&schema.name, &row.id) + .await + .unwrap() + .field("status"), + None + ); + assert_eq!( + backend.load_schema_metadata(&schema.name).await.unwrap(), + Some(schema) + ); +} + +#[tokio::test] +async fn relation_cleanup_tolerates_a_derived_field_without_storage() { + let (backend, schema, _) = fixture("relationcleanup").await; + let name = FieldName::new("members").unwrap(); + backend + .apply_migration( + &schema.name, + &[ + MigrationStep::RemoveRelation { name: name.clone() }, + MigrationStep::AddRelation { + name, + target: schema.name.clone(), + cardinality: Cardinality::Many, + }, + ], + ) + .await + .unwrap(); +} + +#[tokio::test] +async fn new_fields_with_defaults_fill_existing_rows() { + let (backend, mut schema, row) = fixture("addeddefaults").await; + let mut steps = Vec::new(); + for (name, required) in [("required_status", true), ("optional_status", false)] { + let mut modifiers = vec![FieldModifier::Default { + value: DefaultValue::String("draft".into()), + }]; + if required { + modifiers.push(FieldModifier::Required); + } + let field = FieldDefinition::with_modifiers( + FieldName::new(name).unwrap(), + FieldType::Text(TextConstraints::with_max_length(8)), + modifiers, + ); + steps.push(MigrationStep::AddField { + field: field.clone(), + }); + schema.fields.push(field); + } + backend + .apply_schema_change(&schema.name, &steps, Some(&schema)) + .await + .unwrap(); + let loaded = backend.get(&schema.name, &row.id).await.unwrap(); + for name in ["required_status", "optional_status"] { + assert_eq!( + loaded.field(name), + Some(&DynamicValue::Text("draft".into())) + ); + } +} From a5891e987c7d76f6b2707ac41c63b76129756a54 Mon Sep 17 00:00:00 2001 From: Roland Rodriguez Date: Fri, 25 Sep 2026 15:12:40 -0600 Subject: [PATCH 17/21] fix(site): omit unused detail symbols for hidden-only fields --- .../src/commands/site/context.rs | 10 ++++- .../src/app/pages/detail.generated.tsx.jinja | 4 +- .../tests/site_e2e/rendering.schema | 11 ++++++ .../schema-forge-cli/tests/site_generate.rs | 38 +++++++++++++++++-- 4 files changed, 58 insertions(+), 5 deletions(-) diff --git a/crates/schema-forge-cli/src/commands/site/context.rs b/crates/schema-forge-cli/src/commands/site/context.rs index 8e257c1..c68d2c3 100644 --- a/crates/schema-forge-cli/src/commands/site/context.rs +++ b/crates/schema-forge-cli/src/commands/site/context.rs @@ -153,6 +153,8 @@ pub struct EntityView { /// always render through `formatFieldValue`, so nested relations do /// not require the import. pub has_relation_link: bool, + /// Detail rendering reads data from at least one visible field or child. + pub detail_uses_data: bool, /// Detail rendering includes at least one generic formatter call. pub detail_uses_formatter: bool, /// Detail rendering includes a generic field's empty-value guard. @@ -225,11 +227,16 @@ impl EntityView { } let has_form_fields = fields.iter().any(|field| !field.computed && !field.derived); + let detail_uses_data = fields + .iter() + .any(|field| field.kind != "composite" || !field.sub_fields.is_empty()); let detail_uses_is_empty = fields.iter().any(|field| { field.kind != "composite" && field.kind != "file" && !uses_relation_label(field) }); let detail_uses_formatter = detail_uses_is_empty - || fields.iter().any(|field| field.kind == "composite" && !field.sub_fields.is_empty()); + || fields + .iter() + .any(|field| field.kind == "composite" && !field.sub_fields.is_empty()); let list_uses_formatter = fields.iter().any(|field| { matches!(field.list_placement.as_str(), "primary" | "column") && field.kind != "enum" @@ -249,6 +256,7 @@ impl EntityView { display_field, has_relation_one, has_relation_link, + detail_uses_data, detail_uses_formatter, detail_uses_is_empty, list_uses_formatter, diff --git a/crates/schema-forge-cli/templates/site/src/app/pages/detail.generated.tsx.jinja b/crates/schema-forge-cli/templates/site/src/app/pages/detail.generated.tsx.jinja index e509c76..655df5d 100644 --- a/crates/schema-forge-cli/templates/site/src/app/pages/detail.generated.tsx.jinja +++ b/crates/schema-forge-cli/templates/site/src/app/pages/detail.generated.tsx.jinja @@ -20,9 +20,11 @@ import type { {{ entity.pascal }} } from "@/generated/entity-types" import { formatFieldValue } from "@/generated/formatters" {%- endif %} +{% if entity.fields %} function specNum(n: number): string { return String(n).padStart(2, "0") } +{% endif %} {% if entity.detail_uses_is_empty %} function isEmpty(v: unknown): boolean { @@ -33,7 +35,7 @@ function isEmpty(v: unknown): boolean { } {% endif %} -export function {{ entity.pascal }}DetailRows({ data }: { data: {{ entity.pascal }} }) { +export function {{ entity.pascal }}DetailRows({% if entity.detail_uses_data %}{ data }{% else %}_props{% endif %}: { data: {{ entity.pascal }} }) { return ( <> {%- for f in entity.fields %} diff --git a/crates/schema-forge-cli/tests/site_e2e/rendering.schema b/crates/schema-forge-cli/tests/site_e2e/rendering.schema index 4f5e882..6d467d6 100644 --- a/crates/schema-forge-cli/tests/site_e2e/rendering.schema +++ b/crates/schema-forge-cli/tests/site_e2e/rendering.schema @@ -24,3 +24,14 @@ schema JoinReference { schema FileOnly { document: file(bucket: "documents", max_size: "5MB", mime: ["application/pdf"]) } + + +schema HiddenOnly { + secret: text @hidden +} + +schema HiddenComposite { + details: composite { + secret: text @hidden + } +} diff --git a/crates/schema-forge-cli/tests/site_generate.rs b/crates/schema-forge-cli/tests/site_generate.rs index aba23b4..32ef66a 100644 --- a/crates/schema-forge-cli/tests/site_generate.rs +++ b/crates/schema-forge-cli/tests/site_generate.rs @@ -779,9 +779,15 @@ fn relation_and_file_only_pages_emit_only_used_formatters() { ("FileOnly", "file-only"), ] { let out_dir = tmp.path().join(path); - run_generate(&schema_dir, &out_dir, name, &[]).assert().success(); - let detail = fs::read_to_string(out_dir.join(format!("src/app/pages/{path}/detail.generated.tsx"))).unwrap(); - let list = fs::read_to_string(out_dir.join(format!("src/app/pages/{path}/list.generated.tsx"))).unwrap(); + run_generate(&schema_dir, &out_dir, name, &[]) + .assert() + .success(); + let detail = + fs::read_to_string(out_dir.join(format!("src/app/pages/{path}/detail.generated.tsx"))) + .unwrap(); + let list = + fs::read_to_string(out_dir.join(format!("src/app/pages/{path}/list.generated.tsx"))) + .unwrap(); assert!(!detail.contains("formatFieldValue"), "{name}: {detail}"); assert!(!detail.contains("function isEmpty"), "{name}: {detail}"); assert!(!list.contains("formatFieldValue"), "{name}: {list}"); @@ -794,3 +800,29 @@ fn relation_and_file_only_pages_emit_only_used_formatters() { } } } + +#[test] +fn details_without_visible_values_keep_props_without_unused_bindings() { + let tmp = TempDir::new().unwrap(); + let schema_dir = tmp.path().join("schemas"); + write_schemas(&schema_dir, "schema HiddenOnly { secret: text @hidden } schema HiddenComposite { details: composite { secret: text @hidden } }"); + for (name, path, has_rows) in [ + ("HiddenOnly", "hidden-only", false), + ("HiddenComposite", "hidden-composite", true), + ] { + let out_dir = tmp.path().join(path); + run_generate(&schema_dir, &out_dir, name, &[]) + .assert() + .success(); + let detail = + fs::read_to_string(out_dir.join(format!("src/app/pages/{path}/detail.generated.tsx"))) + .unwrap(); + assert!( + detail.contains(&format!("DetailRows(_props: {{ data: {name} }}")), + "{detail}" + ); + assert_eq!(detail.contains("function specNum"), has_rows, "{detail}"); + assert!(!detail.contains("formatFieldValue"), "{detail}"); + assert!(!detail.contains("function isEmpty"), "{detail}"); + } +} From 2ad12092aa2f484a7944460ca8680f1796fcd6ce Mon Sep 17 00:00:00 2001 From: Roland Rodriguez Date: Fri, 25 Sep 2026 15:15:50 -0600 Subject: [PATCH 18/21] fix(surrealdb): discard removed relation values and escape defaults --- crates/schema-forge-surrealdb/src/codegen.rs | 11 +- .../tests/migration_modifiers.rs | 179 +++++++++++++++++- 2 files changed, 185 insertions(+), 5 deletions(-) diff --git a/crates/schema-forge-surrealdb/src/codegen.rs b/crates/schema-forge-surrealdb/src/codegen.rs index 47f69ce..32f7e94 100644 --- a/crates/schema-forge-surrealdb/src/codegen.rs +++ b/crates/schema-forge-surrealdb/src/codegen.rs @@ -129,7 +129,12 @@ pub fn migration_step_to_surql(table: &str, step: &MigrationStep) -> Vec } }, MigrationStep::RemoveRelation { name } => { - vec![format!("REMOVE FIELD IF EXISTS {name} ON {table};")] + vec![ + // Relax required/default rules before clearing stored references. + format!("DEFINE FIELD OVERWRITE {name} ON {table} TYPE any;"), + format!("UPDATE {table} UNSET {name};"), + format!("REMOVE FIELD IF EXISTS {name} ON {table};"), + ] } MigrationStep::BackfillRequired { field, @@ -408,7 +413,9 @@ pub(crate) fn define_field_stmts(table: &str, field: &FieldDefinition) -> Vec String { use schema_forge_core::types::DefaultValue; match value { - DefaultValue::String(s) => format!("'{s}'"), + DefaultValue::String(s) => crate::query::dynamic_value_to_surql_literal( + &schema_forge_core::types::DynamicValue::Text(s.clone()), + ), DefaultValue::Integer(i) => i.to_string(), DefaultValue::Float(s) => s.clone(), DefaultValue::Boolean(b) => b.to_string(), diff --git a/crates/schema-forge-surrealdb/tests/migration_modifiers.rs b/crates/schema-forge-surrealdb/tests/migration_modifiers.rs index a83d72e..b25280c 100644 --- a/crates/schema-forge-surrealdb/tests/migration_modifiers.rs +++ b/crates/schema-forge-surrealdb/tests/migration_modifiers.rs @@ -260,14 +260,14 @@ async fn new_fields_with_defaults_fill_existing_rows() { let mut steps = Vec::new(); for (name, required) in [("required_status", true), ("optional_status", false)] { let mut modifiers = vec![FieldModifier::Default { - value: DefaultValue::String("draft".into()), + value: DefaultValue::String("O'Brien\\x".into()), }]; if required { modifiers.push(FieldModifier::Required); } let field = FieldDefinition::with_modifiers( FieldName::new(name).unwrap(), - FieldType::Text(TextConstraints::with_max_length(8)), + FieldType::Text(TextConstraints::with_max_length(40)), modifiers, ); steps.push(MigrationStep::AddField { @@ -283,7 +283,180 @@ async fn new_fields_with_defaults_fill_existing_rows() { for name in ["required_status", "optional_status"] { assert_eq!( loaded.field(name), - Some(&DynamicValue::Text("draft".into())) + Some(&DynamicValue::Text("O'Brien\\x".into())) ); } } + +#[tokio::test] +async fn removing_and_readding_relation_does_not_restore_values_or_lose_other_fields() { + let backend = SurrealBackend::connect_memory("relationvalues", "relationvalues") + .await + .unwrap(); + let mut schema = SchemaDefinition::new( + SchemaId::new(), + SchemaName::new("RelationValues").unwrap(), + vec![ + FieldDefinition::new( + FieldName::new("label").unwrap(), + FieldType::Text(TextConstraints::unconstrained()), + ), + FieldDefinition::new(FieldName::new("details").unwrap(), FieldType::Json), + FieldDefinition::new( + FieldName::new("members").unwrap(), + FieldType::Relation { + target: SchemaName::new("RelationValues").unwrap(), + cardinality: Cardinality::Many, + }, + ), + ], + vec![], + ) + .unwrap(); + backend + .apply_schema_change( + &schema.name, + &DiffEngine::create_new(&schema).steps, + Some(&schema), + ) + .await + .unwrap(); + let related = Entity::new( + schema.name.clone(), + BTreeMap::from([("label".into(), DynamicValue::Text("target".into()))]), + ); + backend.create(&related).await.unwrap(); + let details = + DynamicValue::Json(serde_json::json!({"nested": {"keep": "value"}, "items": [1, 2]})); + let row = Entity::new( + schema.name.clone(), + BTreeMap::from([ + ("label".into(), DynamicValue::Text("unchanged".into())), + ("details".into(), details.clone()), + ( + "members".into(), + DynamicValue::RefArray(vec![related.id.clone()]), + ), + ]), + ); + backend.create(&row).await.unwrap(); + let field = FieldName::new("members").unwrap(); + let added = FieldDefinition::with_modifiers( + FieldName::new("phase").unwrap(), + FieldType::Text(TextConstraints::unconstrained()), + vec![FieldModifier::Default { + value: DefaultValue::String("ready".into()), + }], + ); + let steps = [ + MigrationStep::RemoveRelation { + name: field.clone(), + }, + MigrationStep::AddRelation { + name: field, + target: schema.name.clone(), + cardinality: Cardinality::Many, + }, + MigrationStep::AddField { + field: added.clone(), + }, + ]; + schema.fields.push(added); + backend + .apply_schema_change(&schema.name, &steps, Some(&schema)) + .await + .unwrap(); + let loaded = backend.get(&schema.name, &row.id).await.unwrap(); + assert!( + loaded + .field("members") + .is_none_or(|value| matches!(value, DynamicValue::Null)), + "old relation resurrected: {loaded:?}" + ); + assert_eq!( + loaded.field("label"), + Some(&DynamicValue::Text("unchanged".into())) + ); + assert_eq!(loaded.field("details"), Some(&details)); + assert_eq!( + loaded.field("phase"), + Some(&DynamicValue::Text("ready".into())) + ); + assert_eq!( + backend + .get(&schema.name, &related.id) + .await + .unwrap() + .field("label"), + Some(&DynamicValue::Text("target".into())) + ); +} + +#[tokio::test] +async fn removing_required_relation_clears_values_before_recreating_optional_storage() { + let backend = SurrealBackend::connect_memory("requiredrelation", "requiredrelation") + .await + .unwrap(); + let name = SchemaName::new("RequiredRelation").unwrap(); + let field = FieldName::new("parent").unwrap(); + let mut schema = SchemaDefinition::new( + SchemaId::new(), + name.clone(), + vec![ + FieldDefinition::new( + FieldName::new("label").unwrap(), + FieldType::Text(TextConstraints::unconstrained()), + ), + FieldDefinition::with_modifiers( + field.clone(), + FieldType::Relation { + target: name.clone(), + cardinality: Cardinality::One, + }, + vec![FieldModifier::Required], + ), + ], + vec![], + ) + .unwrap(); + backend + .apply_schema_change(&name, &DiffEngine::create_new(&schema).steps, Some(&schema)) + .await + .unwrap(); + let id = EntityId::new("requiredrelation"); + let row = Entity::with_id( + id.clone(), + name.clone(), + BTreeMap::from([ + ("parent".into(), DynamicValue::Ref(id.clone())), + ("label".into(), DynamicValue::Text("keep".into())), + ]), + ); + backend.create(&row).await.unwrap(); + schema.fields[1].modifiers.clear(); + backend + .apply_schema_change( + &name, + &[ + MigrationStep::RemoveRelation { + name: field.clone(), + }, + MigrationStep::AddRelation { + name: field, + target: name.clone(), + cardinality: Cardinality::One, + }, + ], + Some(&schema), + ) + .await + .unwrap(); + let loaded = backend.get(&name, &id).await.unwrap(); + assert!(loaded + .field("parent") + .is_none_or(|value| matches!(value, DynamicValue::Null))); + assert_eq!( + loaded.field("label"), + Some(&DynamicValue::Text("keep".into())) + ); +} From bddc796fdd3f700a767d71213ff3c1d338cdcd79 Mon Sep 17 00:00:00 2001 From: Roland Rodriguez Date: Fri, 25 Sep 2026 15:17:56 -0600 Subject: [PATCH 19/21] ci: retain independent runtime checks after storage failures --- .github/workflows/postgres-conditional.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/postgres-conditional.yml b/.github/workflows/postgres-conditional.yml index f4903f5..dd40e93 100644 --- a/.github/workflows/postgres-conditional.yml +++ b/.github/workflows/postgres-conditional.yml @@ -53,8 +53,10 @@ jobs: - name: Run storage concurrency cases run: cargo nextest run -p schema-forge-backend -p schema-forge-postgres --run-ignored all --no-fail-fast - name: Prepare disposable HTTP namespace + if: ${{ !cancelled() }} run: psql -X -v ON_ERROR_STOP=1 -c 'CREATE SCHEMA conditional_http' - name: Run complete runtime and schema suites + if: ${{ !cancelled() }} run: cargo nextest run -p schema-forge-acton -p schema-forge-surrealdb -p schema-forge-core -p schema-forge-dsl --features schema-forge-acton/postgres,schema-forge-acton/graphql --no-fail-fast - name: Run HTTP authorization and concurrency cases if: ${{ !cancelled() }} From c636af3342d202d53c50859a8f4b0d214ca58c7a Mon Sep 17 00:00:00 2001 From: Roland Rodriguez Date: Fri, 25 Sep 2026 15:18:49 -0600 Subject: [PATCH 20/21] test(postgres): preserve drift fixture with valid required default --- crates/schema-forge-postgres/tests/cedar_read.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/crates/schema-forge-postgres/tests/cedar_read.rs b/crates/schema-forge-postgres/tests/cedar_read.rs index 2115b3e..5d594f0 100644 --- a/crates/schema-forge-postgres/tests/cedar_read.rs +++ b/crates/schema-forge-postgres/tests/cedar_read.rs @@ -6,7 +6,7 @@ use schema_forge_backend::{Entity, EntityStore, SchemaBackend}; use schema_forge_core::migration::DiffEngine; use schema_forge_core::query::{FieldPath, Filter, Query, SortOrder}; use schema_forge_core::types::{ - Cardinality, DynamicValue, EntityId, EnumVariants, FieldAnnotation, FieldDefinition, + Cardinality, DefaultValue, DynamicValue, EntityId, EnumVariants, FieldAnnotation, FieldDefinition, FieldModifier, FieldName, FieldType, SchemaDefinition, SchemaId, SchemaName, TextConstraints, }; use schema_forge_postgres::PgBackend; @@ -194,7 +194,15 @@ async fn absent_tenant_and_schema_or_physical_drift_are_rechecked() { .is_none()); execute(&backend, "ALTER TABLE \"CountProof\" DROP COLUMN _tenant").await; let mut changed = schema.clone(); - changed.fields[0].modifiers.push(FieldModifier::Required); + // Keep this a metadata-only change so the later direct NULL write + // exercises re-certification of physical drift. The proposed required + // field must still satisfy the migration planner's backfill contract. + changed.fields[0].modifiers.extend([ + FieldModifier::Required, + FieldModifier::Default { + value: DefaultValue::String("fallback".into()), + }, + ]); backend.store_schema_metadata(&changed).await.unwrap(); assert!(backend .query_cedar_compatible(&schema, &query, &scope) From 322e412495c46fc0237acbe04f09e96fec952191 Mon Sep 17 00:00:00 2001 From: Roland Rodriguez Date: Fri, 25 Sep 2026 15:23:34 -0600 Subject: [PATCH 21/21] test(auth): retain anonymous relation checks with unified helper --- .../src/routes/public_relations_tests.rs | 54 +++++++++++-------- 1 file changed, 33 insertions(+), 21 deletions(-) diff --git a/crates/schema-forge-acton/src/routes/public_relations_tests.rs b/crates/schema-forge-acton/src/routes/public_relations_tests.rs index 6cff58b..6128de0 100644 --- a/crates/schema-forge-acton/src/routes/public_relations_tests.rs +++ b/crates/schema-forge-acton/src/routes/public_relations_tests.rs @@ -1,4 +1,4 @@ -use super::filter_public_relation_entities; +use super::filter_relation_entities_with_policy; use crate::authz::{PolicyStore, PolicyStoreSnapshot, PrincipalClaimMappings, RoleRanks}; use schema_forge_backend::entity::Entity; use schema_forge_core::types::{ @@ -68,19 +68,25 @@ fn row(schema: &SchemaDefinition, published: bool) -> Entity { ) } -#[test] -fn anonymous_enrichment_cannot_disclose_private_target_rows() { +#[tokio::test] +async fn anonymous_enrichment_cannot_disclose_private_target_rows() { let schema = schema(false, false); - let rows = - filter_public_relation_entities(&store(&schema, None), &schema, vec![row(&schema, true)]); + let rows = filter_relation_entities_with_policy( + None, + &store(&schema, None), + &schema, + None, + vec![row(&schema, true)], + ) + .await; assert!( rows.is_empty(), "private target IDs and displays must be absent" ); } -#[test] -fn anonymous_enrichment_checks_full_rows_before_selecting_display_or_child_ids() { +#[tokio::test] +async fn anonymous_enrichment_checks_full_rows_before_selecting_display_or_child_ids() { let schema = schema(true, false); let policy = r#" forbid(principal, action == Action::"ReadRelated", resource is Related) @@ -88,22 +94,31 @@ when { resource has published && !resource.published }; "#; let published = row(&schema, true); let id = published.id.clone(); - let rows = filter_public_relation_entities( + let rows = filter_relation_entities_with_policy( + None, &store(&schema, Some(policy)), &schema, + None, vec![row(&schema, false), published], - ); + ) + .await; assert_eq!(rows.len(), 1); assert_eq!(rows[0].id, id); assert!(rows[0].field("label").is_some()); assert!(rows[0].field("secret").is_none()); } -#[test] -fn anonymous_enrichment_scrubs_restricted_display_and_foreign_key_values() { +#[tokio::test] +async fn anonymous_enrichment_scrubs_restricted_display_and_foreign_key_values() { let schema = schema(true, true); - let rows = - filter_public_relation_entities(&store(&schema, None), &schema, vec![row(&schema, true)]); + let rows = filter_relation_entities_with_policy( + None, + &store(&schema, None), + &schema, + None, + vec![row(&schema, true)], + ) + .await; assert_eq!(rows.len(), 1); assert!( rows[0].field("label").is_none(), @@ -148,18 +163,15 @@ async fn anonymous_enrichment_honors_custom_record_policy_denial() { let schema = schema(true, false); let store = store(&schema, None); let entity = row(&schema, true); - let without_custom = super::filter_public_relation_entities_with_policy( - None, - &store, - &schema, - vec![entity.clone()], - ) - .await; + let without_custom = + filter_relation_entities_with_policy(None, &store, &schema, None, vec![entity.clone()]) + .await; assert_eq!(without_custom.len(), 1, "Cedar permits this target"); - let restricted = super::filter_public_relation_entities_with_policy( + let restricted = filter_relation_entities_with_policy( Some(&AuthenticatedOnlyPolicy), &store, &schema, + None, vec![entity], ) .await;