From 215e29beeb660bf38a40d2c2547bf6f4ed996b31 Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Sat, 10 Oct 2026 01:09:39 -0700 Subject: [PATCH 1/2] fix: report every invalid object field in one validation error That branch now collects every field error and, when there is only one, returns the original variant so its message stays the same. Several errors are returned as ValidationError::Multiple, formatted as a bullet per problem with that problem's path and one object and validator summary. Nested Multiple errors are flattened into the parent, with each child receiving the parent path segment. An array still stops at its first bad element, and a union of object shapes still fails as a single NoMatch. Fixes #591 --- crates/common/src/schemas/validator.rs | 467 ++++++++++++++++++++++++- 1 file changed, 458 insertions(+), 9 deletions(-) diff --git a/crates/common/src/schemas/validator.rs b/crates/common/src/schemas/validator.rs index 7dd76c5a52..3448c94713 100644 --- a/crates/common/src/schemas/validator.rs +++ b/crates/common/src/schemas/validator.rs @@ -180,15 +180,26 @@ impl Validator { } }, (Validator::Object(object_validator), ConvexValue::Object(object)) => { + let mut errors = Vec::new(); for (field_name, field_type) in &object_validator.0 { let maybe_value = object.get::(field_name.borrow()); if let Some(value) = maybe_value { - field_type + if let Err(error) = field_type .validator .check_value_internal(value, all_tables_number_to_name) - .map_err(|e| e.with_context(format!(".{field_name}")))? + { + // A nested object already lists every field. Flatten + // those into this object so each one keeps its path. + let error = error.with_context(format!(".{field_name}")); + match error { + ValidationError::Multiple { errors: nested, .. } => { + errors.extend(nested) + }, + other => errors.push(other), + } + } } else if !field_type.optional { - return Err(ValidationError::MissingRequiredField { + errors.push(ValidationError::MissingRequiredField { object: object.clone(), field_name: field_name.clone(), object_validator: object_validator.clone(), @@ -198,7 +209,7 @@ impl Validator { } for field in object.keys() { if !object_validator.0.contains_key::(field.borrow()) { - return Err(ValidationError::ExtraField { + errors.push(ValidationError::ExtraField { object: object.clone(), field_name: field.clone(), object_validator: object_validator.clone(), @@ -206,6 +217,18 @@ impl Validator { }); } } + match errors.len() { + 0 => (), + 1 => return Err(errors.remove(0)), + _ => { + return Err(ValidationError::Multiple { + errors, + object: object.clone(), + object_validator: object_validator.clone(), + context: ValidationContext::new(), + }); + }, + } }, (Validator::Union(validators), value) => { if validators.len() == 1 { @@ -750,9 +773,7 @@ impl TryFrom for LiteralValidator { } #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] -pub struct ObjectValidator( - pub BTreeMap, -); +pub struct ObjectValidator(pub BTreeMap); #[macro_export] macro_rules! object_validator { @@ -942,6 +963,18 @@ Validator: {validator}" validator: Validator, context: ValidationContext, }, + /// Every invalid field of one object. A single problem keeps its original + /// variant so that message stays unchanged. + #[display( + "{}", + format_multiple_validation_errors(errors, object, object_validator, context) + )] + Multiple { + errors: Vec, + object: ConvexObject, + object_validator: ObjectValidator, + context: ValidationContext, + }, } impl ValidationError { @@ -952,12 +985,428 @@ impl ValidationError { | ValidationError::LiteralValuesDoNotMatch { context, .. } | ValidationError::MissingRequiredField { context, .. } | ValidationError::ExtraField { context, .. } - | ValidationError::NoMatch { context, .. } => context, + | ValidationError::NoMatch { context, .. } + | ValidationError::Multiple { context, .. } => context, + } + } + + fn context_ref(&self) -> &ValidationContext { + match self { + ValidationError::TableNamesDoNotMatch { context, .. } + | ValidationError::SystemTableReference { context, .. } + | ValidationError::LiteralValuesDoNotMatch { context, .. } + | ValidationError::MissingRequiredField { context, .. } + | ValidationError::ExtraField { context, .. } + | ValidationError::NoMatch { context, .. } + | ValidationError::Multiple { context, .. } => context, } } fn with_context(mut self, context: String) -> Self { - self.context().reversed_path.push(context); + // `Multiple` reports each child at its own path, so a segment from an + // array element or parent field has to land on every child. + fn push(error: &mut ValidationError, context: &str) { + if let ValidationError::Multiple { errors, .. } = error { + for child in errors.iter_mut() { + push(child, context); + } + } + error.context().reversed_path.push(context.to_string()); + } + push(&mut self, &context); self } } + +struct MultipleValidationErrorsDisplay<'a> { + errors: &'a [ValidationError], + object: &'a ConvexObject, + object_validator: &'a ObjectValidator, +} + +impl Display for MultipleValidationErrorsDisplay<'_> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + writeln!(f, "Object has multiple validation errors:")?; + for error in self.errors { + write_validation_error_bullet(f, error)?; + } + write!( + f, + "Object: {object}\nValidator: {validator}", + object = self.object, + validator = self.object_validator, + ) + } +} + +fn format_multiple_validation_errors<'a>( + errors: &'a [ValidationError], + object: &'a ConvexObject, + object_validator: &'a ObjectValidator, + _context: &'a ValidationContext, +) -> MultipleValidationErrorsDisplay<'a> { + MultipleValidationErrorsDisplay { + errors, + object, + object_validator, + } +} + +fn write_validation_error_bullet( + f: &mut fmt::Formatter<'_>, + error: &ValidationError, +) -> fmt::Result { + if let ValidationError::Multiple { errors, .. } = error { + for child in errors { + write_validation_error_bullet(f, child)?; + } + return Ok(()); + } + write!(f, "- ")?; + write_validation_error_reason(f, error)?; + let path = error.context_ref().to_string(); + if !path.is_empty() { + write!(f, " {path}")?; + } + writeln!(f) +} + +fn write_validation_error_reason( + f: &mut fmt::Formatter<'_>, + error: &ValidationError, +) -> fmt::Result { + match error { + ValidationError::NoMatch { + value, validator, .. + } => write!(f, "`{value}` does not match validator `{validator}`"), + ValidationError::LiteralValuesDoNotMatch { + value, + literal_validator, + .. + } => write!( + f, + "`{value}` does not match literal validator `v.literal({literal_validator})`." + ), + ValidationError::MissingRequiredField { field_name, .. } => { + write!(f, "missing required field `{field_name}`") + }, + ValidationError::ExtraField { field_name, .. } => { + write!(f, "extra field `{field_name}` that is not in the validator") + }, + ValidationError::TableNamesDoNotMatch { + id, + found_table_name, + validator_table, + .. + } => write!( + f, + "Found ID \"{id}\" from table `{found_table_name}`, which does not match the table \ + name in validator `v.id(\"{validator_table}\")`." + ), + ValidationError::SystemTableReference { + id, + validator_table, + .. + } => write!( + f, + "Found ID \"{id}\" from a system table, which does not match the table name in \ + validator `v.id(\"{validator_table}\")`." + ), + ValidationError::Multiple { .. } => Ok(()), + } +} + +#[cfg(test)] +mod tests { + use std::collections::BTreeMap; + + use value::{ + val, + ConvexValue, + TableMapping, + TableNamespace, + }; + + use super::{ + FieldValidator, + ObjectValidator, + ValidationError, + Validator, + }; + use crate::virtual_system_mapping::VirtualSystemMapping; + + fn object_validator(fields: Vec<(&str, FieldValidator)>) -> anyhow::Result { + let mut map = BTreeMap::new(); + for (name, field) in fields { + map.insert(name.parse()?, field); + } + Ok(Validator::Object(ObjectValidator(map))) + } + + fn check(validator: &Validator, value: &ConvexValue) -> Result<(), ValidationError> { + let table_mapping = TableMapping::new().namespace(TableNamespace::Global); + validator.check_value(value, &table_mapping, &VirtualSystemMapping::default()) + } + + #[test] + fn valid_object_and_absent_optional_pass() -> anyhow::Result<()> { + let validator = object_validator(vec![ + ( + "name", + FieldValidator::required_field_type(Validator::String), + ), + ( + "count", + FieldValidator::required_field_type(Validator::Float64), + ), + ])?; + let value = val!({"name" => "a", "count" => 1.0}); + assert!(check(&validator, &value).is_ok()); + + let validator = object_validator(vec![ + ( + "name", + FieldValidator::required_field_type(Validator::String), + ), + ( + "nickname", + FieldValidator::optional_field_type(Validator::String), + ), + ])?; + let value = val!({"name" => "a"}); + assert!(check(&validator, &value).is_ok()); + Ok(()) + } + + #[test] + fn one_missing_required_field_keeps_single_error() -> anyhow::Result<()> { + let validator = object_validator(vec![ + ( + "name", + FieldValidator::required_field_type(Validator::String), + ), + ( + "count", + FieldValidator::required_field_type(Validator::Float64), + ), + ])?; + let value = val!({"name" => "a"}); + let error = check(&validator, &value).expect_err("missing count"); + assert!(matches!( + error, + ValidationError::MissingRequiredField { .. } + )); + let message = error.to_string(); + assert!(message.contains("Object is missing the required field")); + assert!(message.contains("Consider wrapping the field validator in `v.optional(...)`")); + assert!(message.contains("Object:")); + assert!(message.contains("Validator:")); + assert!(!message.contains("multiple validation errors")); + Ok(()) + } + + #[test] + fn one_extra_field_keeps_single_error() -> anyhow::Result<()> { + let validator = object_validator(vec![ + ( + "name", + FieldValidator::required_field_type(Validator::String), + ), + ( + "count", + FieldValidator::required_field_type(Validator::Float64), + ), + ])?; + let value = val!({"name" => "a", "count" => 1.0, "extra" => true}); + let error = check(&validator, &value).expect_err("extra field"); + assert!(matches!(error, ValidationError::ExtraField { .. })); + let message = error.to_string(); + assert!(message.contains("Object contains extra field")); + assert!(message.contains("Object:")); + assert!(message.contains("Validator:")); + assert!(!message.contains("multiple validation errors")); + Ok(()) + } + + #[test] + fn one_wrong_type_keeps_single_error() -> anyhow::Result<()> { + let validator = object_validator(vec![ + ( + "name", + FieldValidator::required_field_type(Validator::String), + ), + ( + "count", + FieldValidator::required_field_type(Validator::Float64), + ), + ])?; + let value = val!({"name" => 1.0, "count" => 1.0}); + let error = check(&validator, &value).expect_err("wrong type"); + assert!(matches!(error, ValidationError::NoMatch { .. })); + let message = error.to_string(); + assert!(message.contains("Path: .name")); + assert!(message.contains("does not match validator")); + assert!(!message.contains("multiple validation errors")); + Ok(()) + } + + #[test] + fn several_problems_are_one_error() -> anyhow::Result<()> { + let validator = object_validator(vec![ + ( + "name", + FieldValidator::required_field_type(Validator::String), + ), + ( + "count", + FieldValidator::required_field_type(Validator::Float64), + ), + ])?; + let value = val!({"name" => 1.0, "extra" => true}); + let error = check(&validator, &value).expect_err("several problems"); + assert!(matches!(error, ValidationError::Multiple { .. })); + let message = error.to_string(); + assert!(message.contains("Object has multiple validation errors:")); + assert!(message.contains("`1.0` does not match validator `v.string()`")); + assert!(message.contains("Path: .name")); + assert!(message.contains("missing required field `count`")); + assert!(message.contains("extra field `extra` that is not in the validator")); + assert_eq!(message.matches("Object:").count(), 1); + assert_eq!(message.matches("Validator:").count(), 1); + let count_at = message + .find("missing required field `count`") + .expect("count bullet"); + let name_at = message.find("Path: .name").expect("name bullet"); + let extra_at = message + .find("extra field `extra` that is not in the validator") + .expect("extra bullet"); + assert!(count_at < name_at); + assert!(name_at < extra_at); + Ok(()) + } + + #[test] + fn nested_object_failures_are_flattened() -> anyhow::Result<()> { + let payload = object_validator(vec![ + ("a", FieldValidator::required_field_type(Validator::String)), + ("b", FieldValidator::required_field_type(Validator::Float64)), + ])?; + let validator = object_validator(vec![( + "payload", + FieldValidator::required_field_type(payload), + )])?; + let value = val!({ + "payload" => {"a" => 1.0, "b" => "no"}, + "extra" => true, + }); + let error = check(&validator, &value).expect_err("nested failures"); + assert!(matches!(error, ValidationError::Multiple { .. })); + let message = error.to_string(); + assert!(message.contains("Object has multiple validation errors:")); + assert!(message.contains(".payload.a")); + assert!(message.contains(".payload.b")); + assert!(message.contains("extra field `extra` that is not in the validator")); + assert_eq!(message.matches("Object:").count(), 1); + assert_eq!(message.matches("Validator:").count(), 1); + let a_at = message.find(".payload.a").expect("payload.a"); + let b_at = message.find(".payload.b").expect("payload.b"); + let extra_at = message.find("extra field `extra`").expect("extra bullet"); + assert!(a_at < b_at); + assert!(b_at < extra_at); + Ok(()) + } + + #[test] + fn array_reports_only_the_first_bad_element() -> anyhow::Result<()> { + let validator = object_validator(vec![( + "items", + FieldValidator::required_field_type(Validator::Array(Box::new(Validator::String))), + )])?; + let value = val!({"items" => [1.0, 2.0]}); + let error = check(&validator, &value).expect_err("bad array"); + assert!(matches!(error, ValidationError::NoMatch { .. })); + let message = error.to_string(); + assert!(message.contains("Path: .items[0]")); + assert!(!message.contains("[1]")); + assert!(!message.contains("multiple validation errors")); + Ok(()) + } + + #[test] + fn array_element_and_sibling_are_listed_together() -> anyhow::Result<()> { + let validator = object_validator(vec![ + ( + "items", + FieldValidator::required_field_type(Validator::Array(Box::new(Validator::String))), + ), + ( + "name", + FieldValidator::required_field_type(Validator::String), + ), + ])?; + let value = val!({"items" => [1.0, 2.0], "name" => 1.0}); + let error = check(&validator, &value).expect_err("array and sibling"); + assert!(matches!(error, ValidationError::Multiple { .. })); + let message = error.to_string(); + assert!(message.contains("Object has multiple validation errors:")); + assert!(message.contains("Path: .items[0]")); + assert!(message.contains("Path: .name")); + assert!(!message.contains("[1]")); + let items_at = message.find("Path: .items[0]").expect("items"); + let name_at = message.find("Path: .name").expect("name"); + assert!(items_at < name_at); + Ok(()) + } + + #[test] + fn union_field_is_one_line_beside_a_sibling() -> anyhow::Result<()> { + let validator = object_validator(vec![ + ( + "kind", + FieldValidator::required_field_type(Validator::Union(vec![ + Validator::String, + Validator::Float64, + ])), + ), + ( + "name", + FieldValidator::required_field_type(Validator::String), + ), + ])?; + let value = val!({"kind" => true}); + let error = check(&validator, &value).expect_err("union and missing sibling"); + assert!(matches!(error, ValidationError::Multiple { .. })); + let message = error.to_string(); + assert!( + message.contains("`true` does not match validator `v.union(v.string(), v.float64())`") + ); + assert!(message.contains("missing required field `name`")); + assert_eq!( + message + .matches("`true` does not match validator `v.union(v.string(), v.float64())`") + .count(), + 1 + ); + Ok(()) + } + + #[test] + fn union_of_object_shapes_stays_a_single_nomatch() -> anyhow::Result<()> { + let shape_a = object_validator(vec![ + ("a", FieldValidator::required_field_type(Validator::String)), + ("b", FieldValidator::required_field_type(Validator::Float64)), + ])?; + let shape_b = object_validator(vec![( + "c", + FieldValidator::required_field_type(Validator::String), + )])?; + let validator = Validator::Union(vec![shape_a, shape_b]); + let value = val!({"a" => 1.0, "b" => "no", "extra" => true}); + let error = check(&validator, &value).expect_err("union of objects"); + assert!(matches!(error, ValidationError::NoMatch { .. })); + let message = error.to_string(); + assert!(message.contains("does not match validator")); + assert!(!message.contains("multiple validation errors")); + Ok(()) + } +} From 308d9c0be906220d71f51ea72299782fd4777fba Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Wed, 28 Oct 2026 11:18:06 -0700 Subject: [PATCH 2/2] Name the field in nested missing and extra field bullets A missing or extra field's context points at its parent object, so a nested bullet only showed the parent path. Add the field name to the path, and cover it with a nested test. --- crates/common/src/schemas/validator.rs | 33 +++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/crates/common/src/schemas/validator.rs b/crates/common/src/schemas/validator.rs index 3448c94713..349554acbc 100644 --- a/crates/common/src/schemas/validator.rs +++ b/crates/common/src/schemas/validator.rs @@ -1064,7 +1064,19 @@ fn write_validation_error_bullet( } write!(f, "- ")?; write_validation_error_reason(f, error)?; - let path = error.context_ref().to_string(); + // A missing or extra field's context points at its parent object, so add + // the field itself to keep nested bullets from naming only the parent. + let mut context = error.context_ref().clone(); + match error { + ValidationError::MissingRequiredField { field_name, .. } => { + context.reversed_path.insert(0, format!(".{field_name}")); + }, + ValidationError::ExtraField { field_name, .. } => { + context.reversed_path.insert(0, format!(".{field_name}")); + }, + _ => {}, + } + let path = context.to_string(); if !path.is_empty() { write!(f, " {path}")?; } @@ -1316,6 +1328,25 @@ mod tests { Ok(()) } + #[test] + fn nested_missing_and_extra_fields_name_the_field() -> anyhow::Result<()> { + let payload = object_validator(vec![ + ("a", FieldValidator::required_field_type(Validator::String)), + ("b", FieldValidator::required_field_type(Validator::Float64)), + ])?; + let validator = object_validator(vec![( + "payload", + FieldValidator::required_field_type(payload), + )])?; + let value = val!({"payload" => {"a" => "ok", "z" => true}}); + let error = check(&validator, &value).expect_err("nested missing and extra"); + assert!(matches!(error, ValidationError::Multiple { .. })); + let message = error.to_string(); + assert!(message.contains("missing required field `b` Path: .payload.b")); + assert!(message.contains("extra field `z` that is not in the validator Path: .payload.z")); + Ok(()) + } + #[test] fn array_reports_only_the_first_bad_element() -> anyhow::Result<()> { let validator = object_validator(vec![(