From cbda45e61084d254da753be5376c8e368991eaae Mon Sep 17 00:00:00 2001 From: Alex Groleau Date: Thu, 12 Jun 2025 22:27:49 -0400 Subject: [PATCH] fixed conditional errors with false schemas and unevaluatedProperties --- src/lib.rs | 91 ++++++++++++++++++++++++++++++++++----- src/tests.rs | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 197 insertions(+), 11 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index ad3116e..9a47e1d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -91,25 +91,40 @@ fn cache_json_schema(schema_id: &str, schema: JsonB, strict: bool) -> JsonB { } } -// Helper function to recursively apply strict validation to all objects in a schema +// Helper function to apply strict validation to a schema +// +// This recursively adds unevaluatedProperties: false to object-type schemas, +// but SKIPS schemas inside if/then/else to avoid breaking conditional validation. fn apply_strict_validation(schema: &mut Value) { + apply_strict_validation_recursive(schema, false); +} + +fn apply_strict_validation_recursive(schema: &mut Value, inside_conditional: bool) { match schema { Value::Object(map) => { - // If this is an object type schema, add unevaluatedProperties: false - if let Some(Value::String(t)) = map.get("type") { - if t == "object" && !map.contains_key("unevaluatedProperties") && !map.contains_key("additionalProperties") { - map.insert("unevaluatedProperties".to_string(), Value::Bool(false)); + // Skip adding strict validation if we're inside a conditional + if !inside_conditional { + // Add strict validation to object schemas only at top level + if let Some(Value::String(t)) = map.get("type") { + if t == "object" && !map.contains_key("unevaluatedProperties") && !map.contains_key("additionalProperties") { + // At top level, use unevaluatedProperties: false + // This considers all evaluated properties from all schemas + map.insert("unevaluatedProperties".to_string(), Value::Bool(false)); + } } } + // Recurse into all properties - for (_, value) in map.iter_mut() { - apply_strict_validation(value); + for (key, value) in map.iter_mut() { + // Mark when we're inside conditional branches + let in_conditional = inside_conditional || matches!(key.as_str(), "if" | "then" | "else"); + apply_strict_validation_recursive(value, in_conditional); } } Value::Array(arr) => { - // Recurse into array items + // Recurse into array items for item in arr.iter_mut() { - apply_strict_validation(item); + apply_strict_validation_recursive(item, inside_conditional); } } _ => {} @@ -139,7 +154,9 @@ fn validate_json_schema(schema_id: &str, instance: JsonB) -> JsonB { let mut error_list = Vec::new(); collect_errors(&validation_error, &mut error_list); let errors = format_errors(error_list, &instance_value, schema_id); - JsonB(json!({ "errors": errors })) + // Filter out FALSE_SCHEMA errors if there are other validation errors + let filtered_errors = filter_false_schema_errors(errors); + JsonB(json!({ "errors": filtered_errors })) } } } @@ -154,6 +171,19 @@ fn collect_errors(error: &ValidationError, errors_list: &mut Vec) { ErrorKind::Group | ErrorKind::AllOf | ErrorKind::AnyOf | ErrorKind::Not | ErrorKind::OneOf(_) ); + // Special handling for FalseSchema - if it has causes, use those instead + if matches!(&error.kind, ErrorKind::FalseSchema) { + if !error.causes.is_empty() { + // FalseSchema often wraps more specific errors in if/then conditionals + for cause in &error.causes { + collect_errors(cause, errors_list); + } + return; + } + // If FalseSchema has no causes, it's likely from unevaluatedProperties + // We'll handle it as a leaf error below + } + if error.causes.is_empty() && !is_structural { // Handle errors with multiple fields specially match &error.kind { @@ -223,6 +253,21 @@ fn collect_errors(error: &ValidationError, errors_list: &mut Vec) { }); } } + ErrorKind::Pattern { got, want } => { + // Special handling for pattern errors to include the pattern in the message + let display_value = if got.len() > 50 { + format!("{}...", &got[..50]) + } else { + got.to_string() + }; + + errors_list.push(Error { + path: error.instance_location.to_string(), + code: "PATTERN_VIOLATED".to_string(), + message: format!("Value does not match pattern: {}", want), + cause: format!("'{}' does not match pattern {}", display_value, want), + }); + } _ => { // Handle all other error types normally let original_message = format!("{}", error.kind); @@ -403,6 +448,32 @@ fn convert_error_kind(kind: &ErrorKind) -> (String, String) { } } +// Filter out FALSE_SCHEMA errors if there are other validation errors +fn filter_false_schema_errors(errors: Vec) -> Vec { + // Check if there are any non-FALSE_SCHEMA errors + let has_non_false_schema = errors.iter().any(|e| { + e.get("code") + .and_then(|c| c.as_str()) + .map(|code| code != "FALSE_SCHEMA") + .unwrap_or(false) + }); + + if has_non_false_schema { + // Filter out FALSE_SCHEMA errors + errors.into_iter() + .filter(|e| { + e.get("code") + .and_then(|c| c.as_str()) + .map(|code| code != "FALSE_SCHEMA") + .unwrap_or(true) + }) + .collect() + } else { + // Keep all errors (they're all FALSE_SCHEMA) + errors + } +} + // Formats errors according to DropError structure fn format_errors(errors: Vec, instance: &Value, schema_id: &str) -> Vec { // Deduplicate by instance_path and format as DropError diff --git a/src/tests.rs b/src/tests.rs index ad25a6d..8ca8c01 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -335,7 +335,7 @@ fn test_validate_json_schema_oneof_validation_errors() { let result_empty_obj = validate_json_schema(schema_id, jsonb(invalid_empty_obj)); // Now we expect 2 errors because required fields are split into individual errors assert_failure_with_json!(result_empty_obj, 2); - let errors_empty = result_empty_obj.0["errors"].as_array().expect("Expected error array for empty object"); + let errors_empty = result_empty_obj.0["errors"].as_array().unwrap(); assert_eq!(errors_empty.len(), 2, "Expected two errors for missing required fields"); // Check that we have errors for both missing fields @@ -628,6 +628,121 @@ fn test_auto_strict_validation() { let result_permissive = validate_json_schema(schema_id_permissive, jsonb(instance_with_extra)); assert_success_with_json!(result_permissive, "Instance with extra property should pass when additionalProperties is explicitly true, even with strict=true"); + + // Test 7: Schema with conditionals (if/then/else) should NOT add unevaluatedProperties to conditional branches + let schema_id_conditional = "conditional_strict_test"; + let conditional_schema = json!({ + "type": "object", + "properties": { + "kind": { "type": "string", "enum": ["checking", "savings"] }, + "creating": { "type": "boolean" } + }, + "if": { + "properties": { + "creating": { "const": true } + } + }, + "then": { + "properties": { + "account_number": { + "type": "string", + "pattern": "^[0-9]{4,17}$" + }, + "routing_number": { + "type": "string", + "pattern": "^[0-9]{9}$" + } + }, + "required": ["account_number", "routing_number"] + } + }); + + let _ = cache_json_schema(schema_id_conditional, jsonb(conditional_schema), true); // strict=true + + // Valid data with properties from both main schema and then clause + let valid_conditional = json!({ + "kind": "checking", + "creating": true, + "account_number": "1234567890", + "routing_number": "123456789" + }); + + let result_conditional = validate_json_schema(schema_id_conditional, jsonb(valid_conditional)); + assert_success_with_json!(result_conditional, "Conditional properties should be recognized as evaluated"); + + // Invalid: extra property not defined anywhere + let invalid_conditional = json!({ + "kind": "checking", + "creating": true, + "account_number": "1234567890", + "routing_number": "123456789", + "extra": "not allowed" + }); + + let result_invalid_conditional = validate_json_schema(schema_id_conditional, jsonb(invalid_conditional)); + assert_failure_with_json!(result_invalid_conditional, 1, "Schema validation always fails"); + let errors_conditional = result_invalid_conditional.0["errors"].as_array().unwrap(); + assert_eq!(errors_conditional[0]["code"], "FALSE_SCHEMA"); + assert_eq!(errors_conditional[0]["details"]["path"], "/extra"); + + // Test the specific edge case: pattern validation failure in a conditional branch + // We filter out FALSE_SCHEMA errors when there are other validation errors + let pattern_failure = json!({ + "kind": "checking", + "creating": true, + "account_number": "22", // Too short - will fail pattern validation + "routing_number": "123456789" // Valid, but would be unevaluated - filtered out + }); + + let result_pattern = validate_json_schema(schema_id_conditional, jsonb(pattern_failure)); + let pattern_errors = result_pattern.0["errors"].as_array().unwrap(); + + // We expect only 1 error: PATTERN_VIOLATED for account_number + // FALSE_SCHEMA for routing_number is filtered out because there's a real validation error + assert_failure_with_json!(result_pattern, 1); + assert_eq!(pattern_errors[0]["code"], "PATTERN_VIOLATED"); + assert_eq!(pattern_errors[0]["details"]["path"], "/account_number"); + + // Test case where both fields have pattern violations + let both_pattern_failures = json!({ + "kind": "checking", + "creating": true, + "account_number": "22", // Too short - will fail pattern validation + "routing_number": "123" // Too short - will fail pattern validation + }); + + let result_both = validate_json_schema(schema_id_conditional, jsonb(both_pattern_failures)); + let both_errors = result_both.0["errors"].as_array().unwrap(); + + // We expect 2 errors: both PATTERN_VIOLATED + assert_failure_with_json!(result_both, 2); + + assert!(both_errors.iter().any(|e| + e["code"] == "PATTERN_VIOLATED" && + e["details"]["path"] == "/account_number" + ), "Should have pattern violation for account_number"); + + assert!(both_errors.iter().any(|e| + e["code"] == "PATTERN_VIOLATED" && + e["details"]["path"] == "/routing_number" + ), "Should have pattern violation for routing_number"); + + // Test case where there are only FALSE_SCHEMA errors (no other validation errors) + let only_false_schema = json!({ + "kind": "checking", + "creating": true, + "account_number": "1234567890", // Valid + "routing_number": "123456789", // Valid + "extra": "not allowed" // Will cause FALSE_SCHEMA + }); + + let result_only_false = validate_json_schema(schema_id_conditional, jsonb(only_false_schema)); + let only_false_errors = result_only_false.0["errors"].as_array().unwrap(); + + // We expect 1 FALSE_SCHEMA error since there are no other validation errors + assert_failure_with_json!(result_only_false, 1); + assert_eq!(only_false_errors[0]["code"], "FALSE_SCHEMA"); + assert_eq!(only_false_errors[0]["details"]["path"], "/extra"); } #[pg_test]