Fix unenforced field constraints (#133) and unique-on-tenant-root (#134); acton-service 0.39.0

CHANGELOG.md

@@ -111,6 +111,16 @@ is pre-1.0; breaking changes bump the **minor** version per
 
 ### Changed
 
+- Upgraded `acton-service` to **0.39.0** in `schema-forge-acton`,
+  `schema-forge-backend`, `schema-forge-cli`, and `schema-forge-mssql`. The
+  release is additive: it introduces a SAML 2.0 service provider behind a new
+  `saml` feature and changes nothing in the features this workspace already
+  enables. SchemaForge does **not** turn `saml` on yet — acton-service ships
+  the SP as a library (`SamlServiceProvider`, config, replay/pending stores),
+  not as mounted routes, so consuming it means wiring
+  `/saml/metadata`, `/saml/login`, and `/saml/acs`, plumbing
+  `[auth.saml]` config, and deciding how an assertion maps onto a `User`
+  entity and a tenant. Tracked separately.
 - Upgraded `acton-service` to **0.26** in `schema-forge-acton`,
   `schema-forge-cli`, and `schema-forge-backend`. 0.26's new
   `crypto-aws-lc-rs` feature (enabled by default in our build) propagates
@@ -122,6 +132,36 @@ is pre-1.0; breaking changes bump the **minor** version per
 
 ### Fixed
 
+- **`enum`, `text(max:)`, and `integer(min:/max:)` are now enforced
+  in-process.** They were declared in the DSL but never checked before the
+  write reached the database, so the only thing refusing them was the
+  generated `CHECK` constraint (or `VARCHAR(n)`) — and a check violation is
+  not mapped to a client error. A caller sending `kind: "gamma"` to an
+  `enum("alpha", "beta")` field got `502 backend_unavailable`, a retryable
+  status for a request that could never succeed, with a raw driver message
+  and no field name. `FieldType::check_value` now checks every declared
+  constraint and the write path answers `422 validation_failed` naming the
+  field and, for an enum, the allowed variants. The check runs at the last
+  seam before the backend, so it also covers values produced by `@default`
+  and `@compute` rules and by `before_*` hooks — not only client JSON.
+  Nothing about which writes succeed changes; only the status code, the
+  message, and where the refusal happens.
+  **Fixes [#133](https://github.com/Govcraft/schemaforge/issues/133).**
+- **`unique` on a `@tenant(root)` schema is enforced again.** The unique
+  index was scoped to `(_tenant, field)` for every schema carrying any
+  `@tenant` annotation, root included. On a root schema that does not weaken
+  the constraint, it removes it: `_tenant` is NULL on a platform-level
+  create and PostgreSQL treats NULLs as distinct, so two organizations could
+  carry the same `short_code` and the migration would still apply cleanly.
+  Root-tenant rows are also the rows most likely to need global uniqueness —
+  they *are* the tenants, so their identifying fields have to be unique
+  table-wide by definition, and there is no outer tenant to scope them to.
+  The scoping decision now runs off the new
+  `SchemaDefinition::unique_scoped_by_tenant`, which is true only for
+  `@tenant(parent: ...)`. `is_tenanted` keeps its old meaning and still
+  drives the `_tenant` column; the two questions were being answered by one
+  predicate. Affects the PostgreSQL and SurrealDB backends alike.
+  **Fixes [#134](https://github.com/Govcraft/schemaforge/issues/134).**
 - `schema-forge-backend` was still pinned to `acton-service 0.23` while
   the rest of the workspace had moved to 0.26.1. The dual-version
   trait mismatch refused to compile (`filter_visible`/`can_modify`/
@@ -157,6 +197,54 @@ is pre-1.0; breaking changes bump the **minor** version per
 
 ### Migration
 
+#### `unique` on a tenant root (#134)
+
+New databases need nothing. An **existing** database applied before this fix
+still carries the tenant-scoped index, and the schema diff cannot see the
+difference: the stored schema is unchanged, so no `AddUnique` step is
+emitted and the stale index stays. The scope changed in the code, not in the
+schema.
+
+Check for it, then replace it. For each `@tenant(root)` schema with a
+`unique` field, on PostgreSQL:
+
+```sql
+-- Confirm the stale shape: indexdef will name (_tenant, <field>).
+SELECT indexdef FROM pg_indexes WHERE indexname = 'uq_Organization_short_code';
+
+-- Find duplicates the broken index let through, and resolve them first —
+-- the ALTER below will fail while any remain.
+SELECT short_code, count(*) FROM "Organization"
+GROUP BY short_code HAVING count(*) > 1;
+
+DROP INDEX "uq_Organization_short_code";
+ALTER TABLE "Organization"
+  ADD CONSTRAINT "uq_Organization_short_code" UNIQUE ("short_code");
+```
+
+The old index and the new constraint share a name, so drop before adding.
+On SurrealDB the equivalent is `REMOVE INDEX uq_Organization_short_code ON
+Organization;` followed by the `DEFINE INDEX ... FIELDS short_code UNIQUE;`
+that `schemaforge apply` would now emit.
+
+Run the duplicate query before scheduling the change. A deployment that has
+been live on the broken index may already hold rows that global uniqueness
+would reject, and that is a data decision, not a migration step.
+
+#### Constraint violations now return 422 (#133)
+
+No schema or config change. Clients that were treating a `502` from a write
+as "backend down, retry" will now see `422` for a value that violates a
+declared `enum`, `text(max:)`, or `integer(min:/max:)` constraint. That is
+the point — the request was never going to succeed — but any retry logic
+keyed on the old status should be checked.
+
+Projects that worked around this by declaring a `@require` CEL rule
+alongside the column constraint (`integer(min: 1, max: 5) required
+@require("size >= 1 && size <= 5", "...")`) can drop the rule; the column
+declaration now produces a `422` on its own. Keeping it is harmless — it
+simply fires first, with its own message.
+
 #### Demo-user seeding (security fix)
 
 Operators upgrading from `schema-forge-cli` 0.27.x:

crates/schema-forge-acton/Cargo.toml

@@ -11,7 +11,7 @@ serde = { version = "1", features = ["derive"] }
 serde_json = "1"
 chrono = { version = "0.4", features = ["serde"] }
 tokio = { version = "1", features = ["sync"] }
-acton-service = { version = "0.38.0", default-features = false, features = ["http", "observability", "otel-metrics", "journald", "governor", "resilience", "audit", "openapi", "auth", "crypto-aws-lc-rs", "grpc", "tls", "windows-auth"] }
+acton-service = { version = "0.39.0", default-features = false, features = ["http", "observability", "otel-metrics", "journald", "governor", "resilience", "audit", "openapi", "auth", "crypto-aws-lc-rs", "grpc", "tls", "windows-auth"] }
 schema-forge-dsl = { path = "../schema-forge-dsl" }
 schema-forge-surrealdb = { path = "../schema-forge-surrealdb", optional = true }
 schema-forge-postgres = { path = "../schema-forge-postgres", optional = true }

crates/schema-forge-acton/src/routes/entities.rs

@@ -11,7 +11,8 @@ use axum::Json;
 use schema_forge_backend::entity::Entity;
 use schema_forge_core::query::{validate_filter, FieldPath, Filter, SortOrder};
 use schema_forge_core::types::{
-    Cardinality, DynamicValue, EntityId, FieldType, SchemaDefinition, SchemaName,
+    Cardinality, ConstraintViolation, DynamicValue, EntityId, FieldType, SchemaDefinition,
+    SchemaName,
 };
 use serde::{Deserialize, Serialize};
 use tokio::sync::oneshot;
@@ -633,14 +634,50 @@ pub fn json_to_entity_fields_with_mode(
 /// this to a 422), never truncated or silently accepted.
 fn enforce_bytes_max_size(bytes: &[u8], max_size: Option<usize>) -> Result<(), String> {
     match max_size {
-        Some(max) if bytes.len() > max => Err(format!(
-            "bytes value of {} bytes exceeds the field's max_size of {max} bytes",
-            bytes.len()
-        )),
+        Some(max) if bytes.len() > max => Err(ConstraintViolation::BytesTooLarge {
+            len: bytes.len(),
+            max,
+        }
+        .to_string()),
         _ => Ok(()),
     }
 }
 
+/// Check every value in a write against the constraints declared on its
+/// field's type, collecting all violations into a single 422.
+///
+/// Runs at the last seam before the backend, which is what makes it
+/// complete: by this point the field map holds client JSON, `@default` and
+/// `@compute` rule output, server-injected columns, and anything a
+/// `before_*` hook substituted. Checking earlier would leave the later
+/// sources unguarded, and an unguarded violation reaches the database, whose
+/// refusal arrives as an untyped driver error and surfaces as a 502 — a
+/// retryable status for a request that can never succeed. See #133.
+///
+/// Values whose names are not in the schema (`_tenant` and friends) carry no
+/// declared constraints and are skipped.
+fn check_field_constraints(
+    schema: &SchemaDefinition,
+    fields: &BTreeMap<String, DynamicValue>,
+) -> Result<(), ForgeError> {
+    let details: Vec<String> = fields
+        .iter()
+        .filter_map(|(name, value)| {
+            let field_def = schema.field(name)?;
+            field_def
+                .field_type
+                .check_value(value)
+                .err()
+                .map(|violation| format!("field '{name}': {violation}"))
+        })
+        .collect();
+    if details.is_empty() {
+        Ok(())
+    } else {
+        Err(ForgeError::ValidationFailed { details })
+    }
+}
+
 fn convert_json_with_type_hint(
     value: &serde_json::Value,
     field_type: &FieldType,
@@ -2086,6 +2123,7 @@ pub async fn create_entity(
         claims.as_ref(),
         FieldFilterDirection::Write,
     );
+    check_field_constraints(&schema_def, &entity.fields)?;
 
     // Create entity via actor (supervised backend call)
     let (tx, rx) = oneshot::channel();
@@ -2804,6 +2842,7 @@ pub async fn update_entity(
         claims.as_ref(),
         FieldFilterDirection::Write,
     );
+    check_field_constraints(&schema_def, &entity.fields)?;
 
     // Update entity via actor
     let (tx, rx) = oneshot::channel();
@@ -3101,6 +3140,7 @@ pub async fn patch_entity(
             claims.as_ref(),
             FieldFilterDirection::Write,
         );
+        check_field_constraints(&schema_def, &entity.fields)?;
         let (tx, rx) = oneshot::channel();
         forge
             .send(UpdateEntity {
@@ -3387,6 +3427,121 @@ mod tests {
         .unwrap()
     }
 
+    // ---- check_field_constraints (#133) ----
+
+    /// The neutral repro from #133: every constraint the DSL can express,
+    /// on one schema.
+    fn make_constrained_schema() -> SchemaDefinition {
+        use schema_forge_core::types::{EnumVariants, IntegerConstraints};
+        SchemaDefinition::new(
+            SchemaId::new(),
+            SchemaName::new("Widget").unwrap(),
+            vec![
+                FieldDefinition::new(
+                    FieldName::new("name").unwrap(),
+                    FieldType::Text(TextConstraints::with_max_length(10)),
+                ),
+                FieldDefinition::new(
+                    FieldName::new("size").unwrap(),
+                    FieldType::Integer(IntegerConstraints::with_range(1, 5).unwrap()),
+                ),
+                FieldDefinition::new(
+                    FieldName::new("kind").unwrap(),
+                    FieldType::Enum(
+                        EnumVariants::new(vec!["alpha".into(), "beta".into()]).unwrap(),
+                    ),
+                ),
+            ],
+            vec![],
+        )
+        .unwrap()
+    }
+
+    fn constraint_errors(fields: &[(&str, DynamicValue)]) -> Vec<String> {
+        let map: BTreeMap<String, DynamicValue> = fields
+            .iter()
+            .map(|(k, v)| ((*k).to_string(), v.clone()))
+            .collect();
+        match check_field_constraints(&make_constrained_schema(), &map) {
+            Ok(()) => Vec::new(),
+            Err(ForgeError::ValidationFailed { details }) => details,
+            Err(other) => panic!("expected ValidationFailed, got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn check_field_constraints_accepts_a_conforming_write() {
+        assert!(constraint_errors(&[
+            ("name", DynamicValue::Text("ok".into())),
+            ("size", DynamicValue::Integer(3)),
+            ("kind", DynamicValue::Enum("alpha".into())),
+        ])
+        .is_empty());
+    }
+
+    #[test]
+    fn check_field_constraints_rejects_an_unknown_enum_variant() {
+        let errors = constraint_errors(&[("kind", DynamicValue::Enum("gamma".into()))]);
+        assert_eq!(errors.len(), 1);
+        assert!(
+            errors[0].contains("field 'kind'")
+                && errors[0].contains("gamma")
+                && errors[0].contains("alpha, beta"),
+            "the 422 must name the field and the allowed variants, got: {}",
+            errors[0]
+        );
+    }
+
+    #[test]
+    fn check_field_constraints_rejects_an_over_length_text() {
+        let errors = constraint_errors(&[(
+            "name",
+            DynamicValue::Text("this is far too long".into()),
+        )]);
+        assert_eq!(errors.len(), 1);
+        assert!(errors[0].contains("field 'name'"), "got: {}", errors[0]);
+    }
+
+    #[test]
+    fn check_field_constraints_rejects_an_out_of_range_integer() {
+        assert_eq!(constraint_errors(&[("size", DynamicValue::Integer(0))]).len(), 1);
+        assert_eq!(constraint_errors(&[("size", DynamicValue::Integer(9))]).len(), 1);
+    }
+
+    #[test]
+    fn check_field_constraints_reports_every_violation_at_once() {
+        // One round trip should tell the caller everything that is wrong,
+        // the way the existing type and required-field errors already do.
+        let errors = constraint_errors(&[
+            ("name", DynamicValue::Text("this is far too long".into())),
+            ("size", DynamicValue::Integer(0)),
+            ("kind", DynamicValue::Enum("gamma".into())),
+        ]);
+        assert_eq!(errors.len(), 3, "got: {errors:?}");
+    }
+
+    #[test]
+    fn check_field_constraints_skips_fields_not_in_the_schema() {
+        // Server-injected columns like `_tenant` carry no declared
+        // constraints and must pass straight through.
+        assert!(constraint_errors(&[
+            ("_tenant", DynamicValue::Text("org_0123456789".repeat(10))),
+        ])
+        .is_empty());
+    }
+
+    #[test]
+    fn check_field_constraints_ignores_nulls() {
+        // Nullability is the `required` modifier's job, enforced in
+        // `json_to_entity_fields_with_mode`.
+        assert!(constraint_errors(&[
+            ("kind", DynamicValue::Null),
+            ("size", DynamicValue::Null),
+            ("name", DynamicValue::Null),
+        ])
+        .is_empty());
+    }
+
     #[test]
     fn json_to_entity_fields_basic() {
         let schema = make_test_schema();

crates/schema-forge-backend/Cargo.toml

@@ -4,7 +4,7 @@ version = "0.13.0"
 edition = "2021"
 
 [dependencies]
-acton-service = { version = "0.38.0", default-features = false, features = ["crypto-aws-lc-rs"] }
+acton-service = { version = "0.39.0", default-features = false, features = ["crypto-aws-lc-rs"] }
 argon2 = { version = "0.5", features = ["std"] }
 async-trait = "0.1.89"
 chrono = { version = "0.4.44", features = ["serde"] }

crates/schema-forge-cli/Cargo.toml

@@ -27,7 +27,7 @@ console = "0.15"
 dialoguer = "0.11"
 glob = "0.3"
 axum = { version = "0.8" }
-acton-service = { version = "0.38.0", default-features = false, features = ["http", "observability", "otel-metrics", "journald", "governor", "resilience", "audit", "openapi", "auth", "crypto-aws-lc-rs", "windows-auth"] }
+acton-service = { version = "0.39.0", default-features = false, features = ["http", "observability", "otel-metrics", "journald", "governor", "resilience", "audit", "openapi", "auth", "crypto-aws-lc-rs", "windows-auth"] }
 heck = "0.5.0"
 minijinja = "2.19.0"
 tracing = "0.1.44"