Merge pull request #304 from hadley/custom-hint

Allow for custom hints/error messages

Author: cscheid

crates/quarto-yaml-validation/src/diagnostic.rs

@@ -45,6 +45,10 @@ pub struct ValidationDiagnostic {
     /// Source location with filename and byte offsets/line numbers
     pub source_range: Option<SourceRange>,
 
+    /// Author-supplied hint override from the schema's `errorMessage`
+    /// annotation. When present, it replaces the auto-generated hint.
+    pub custom_hint: Option<String>,
+
     /// Internal: DiagnosticMessage for text rendering
     diagnostic: DiagnosticMessage,
 }
@@ -90,9 +94,22 @@ impl ValidationDiagnostic {
         self.kind.message()
     }
 
-    /// Get hints (lazily generated from kind)
+    /// Get hints. An author-supplied `errorMessage` override (if any) replaces
+    /// the auto-generated hint; otherwise the hint is derived from the kind.
     pub fn hints(&self) -> Vec<String> {
-        Self::suggest_fixes_from_kind(&self.kind)
+        Self::effective_hints(&self.kind, self.custom_hint.as_deref())
+    }
+
+    /// Compute the effective hints: the authored override if present, else the
+    /// auto-generated hints for this error kind.
+    fn effective_hints(
+        kind: &crate::error::ValidationErrorKind,
+        custom_hint: Option<&str>,
+    ) -> Vec<String> {
+        match custom_hint {
+            Some(hint) => vec![hint.to_string()],
+            None => Self::suggest_fixes_from_kind(kind),
+        }
     }
 
     /// Create a new ValidationDiagnostic from a ValidationError
@@ -135,6 +152,7 @@ impl ValidationDiagnostic {
             instance_path,
             schema_path: error.schema_path.segments().to_vec(),
             source_range,
+            custom_hint: error.custom_hint.clone(),
             diagnostic,
         }
     }
@@ -164,7 +182,7 @@ impl ValidationDiagnostic {
         // Include human-readable fields for convenience
         obj["message"] = json!(self.kind.message());
 
-        let hints = Self::suggest_fixes_from_kind(&self.kind);
+        let hints = Self::effective_hints(&self.kind, self.custom_hint.as_deref());
         if !hints.is_empty() {
             obj["hints"] = json!(hints);
         }
@@ -209,8 +227,8 @@ impl ValidationDiagnostic {
             builder = builder.add_info(format!("Schema constraint: {}", error.schema_path));
         }
 
-        // Add hints
-        for hint in Self::suggest_fixes_from_kind(&error.kind) {
+        // Add hints (authored `errorMessage` override wins over generated ones)
+        for hint in Self::effective_hints(&error.kind, error.custom_hint.as_deref()) {
             builder = builder.add_hint(hint);
         }
 

crates/quarto-yaml-validation/src/error.rs

@@ -311,6 +311,11 @@ pub struct ValidationError {
     pub yaml_node: Option<YamlWithSourceInfo>,
     /// Source location (file, line, column) for error reporting
     pub location: Option<SourceLocation>,
+    /// Author-supplied hint override, taken from the `errorMessage` annotation
+    /// on the schema node where the failure occurred. When present, this
+    /// replaces the auto-generated hint line; the factual primary message
+    /// (from [`ValidationErrorKind::message`]) is left intact.
+    pub custom_hint: Option<String>,
 }
 
 impl fmt::Display for ValidationError {
@@ -337,6 +342,7 @@ impl ValidationError {
             schema_path: SchemaPath::new(),
             yaml_node: None,
             location: None,
+            custom_hint: None,
         }
     }
 

crates/quarto-yaml-validation/src/validator.rs

@@ -31,6 +31,13 @@ pub struct ValidationContext<'a> {
     instance_path: InstancePath,
     /// Current schema path (e.g., ["properties", "format"])
     schema_path: SchemaPath,
+    /// Stack of `errorMessage` overrides, one frame per schema node currently
+    /// being validated (innermost last). Each frame is the node's own
+    /// `errorMessage` (or `None`). `add_error` copies the top frame into the
+    /// produced error's `custom_hint`, so the override binds strictly to the
+    /// schema node where the failure occurs — a child node without its own
+    /// `errorMessage` masks (does not inherit) an enclosing one.
+    custom_hint_stack: Vec<Option<String>>,
     /// Collected validation errors
     errors: Vec<ValidationError>,
 }
@@ -43,18 +50,36 @@ impl<'a> ValidationContext<'a> {
             source_ctx,
             instance_path: InstancePath::new(),
             schema_path: SchemaPath::new(),
+            custom_hint_stack: Vec::new(),
             errors: Vec::new(),
         }
     }
 
     /// Add an error to the context
     pub fn add_error(&mut self, kind: ValidationErrorKind, node: &YamlWithSourceInfo) {
-        let error = ValidationError::new(kind, self.instance_path.clone())
+        let mut error = ValidationError::new(kind, self.instance_path.clone())
             .with_schema_path(self.schema_path.clone())
             .with_yaml_node(node.clone(), self.source_ctx);
+        // Bind the override of the schema node currently being validated (if
+        // any) to this error.
+        error.custom_hint = self.custom_hint_stack.last().cloned().flatten();
         self.errors.push(error);
     }
 
+    /// Execute a function with the current schema node's `errorMessage` pushed
+    /// as a frame. A `None` frame is pushed when the node has no `errorMessage`,
+    /// which masks any enclosing override — overrides bind strictly to the node
+    /// that declares them.
+    pub fn with_custom_hint<F, R>(&mut self, hint: Option<&str>, f: F) -> R
+    where
+        F: FnOnce(&mut Self) -> R,
+    {
+        self.custom_hint_stack.push(hint.map(str::to_string));
+        let result = f(self);
+        self.custom_hint_stack.pop();
+        result
+    }
+
     /// Execute a function with a new instance path segment
     pub fn with_instance_path<F, R>(&mut self, segment: PathSegment, f: F) -> R
     where
@@ -163,6 +188,17 @@ fn validate_generic(
     value: &YamlWithSourceInfo,
     schema: &Schema,
     context: &mut ValidationContext,
+) -> ValidationResult<()> {
+    // Bind this schema node's `errorMessage` (if any) for the duration of its
+    // validation, so any failure raised here picks it up as a custom hint.
+    let hint = schema.annotations().error_message.as_deref();
+    context.with_custom_hint(hint, |context| validate_generic_inner(value, schema, context))
+}
+
+fn validate_generic_inner(
+    value: &YamlWithSourceInfo,
+    schema: &Schema,
+    context: &mut ValidationContext,
 ) -> ValidationResult<()> {
     match schema {
         Schema::True => Ok(()),

crates/quarto-yaml-validation/tests/integration/validation_diagnostic.rs

@@ -326,3 +326,154 @@ object:
     // Verify the error has proper source_range pointing to same file
     assert_eq!(json["source_range"]["filename"], "user.yaml");
 }
+
+#[test]
+fn test_custom_error_message_overrides_pattern_hint() {
+    // A string schema with a non-obvious pattern plus an authored errorMessage.
+    let custom_hint = r#"Must be "naive" or a standard time zone in the form Area/Location"#;
+    let schema_yaml = quarto_yaml::parse(&format!(
+        r#"
+string:
+  pattern: "^(naive|UTC)$"
+  errorMessage: '{custom_hint}'
+"#
+    ))
+    .unwrap();
+    let schema = Schema::from_yaml(&schema_yaml).unwrap();
+
+    let doc_content = r#"PST"#;
+    let doc = quarto_yaml::parse_file(doc_content, "tz.yaml").unwrap();
+    let source_ctx = create_test_context("tz.yaml", doc_content);
+    let registry = quarto_yaml_validation::SchemaRegistry::new();
+
+    let result = validate(&doc, &schema, &registry, &source_ctx);
+    assert!(result.is_err(), "Validation should fail for pattern mismatch");
+
+    let error = result.unwrap_err();
+    assert_eq!(
+        error.custom_hint.as_deref(),
+        Some(custom_hint),
+        "custom_hint should be populated from the schema's errorMessage"
+    );
+
+    let diagnostic = ValidationDiagnostic::from_validation_error(&error, &source_ctx);
+
+    // The authored message replaces the generic pattern hint.
+    let hints = diagnostic.hints();
+    assert_eq!(
+        hints,
+        vec![custom_hint.to_string()],
+        "the authored errorMessage should be the only hint"
+    );
+    assert!(
+        !hints.iter().any(|h| h.contains("matches the expected format")),
+        "the generic pattern hint must not appear"
+    );
+
+    // The factual primary message is left intact.
+    assert!(
+        diagnostic.message().contains("does not match pattern"),
+        "primary message should still report the factual failure, got: {}",
+        diagnostic.message()
+    );
+
+    // JSON output carries the authored hint too.
+    let json = diagnostic.to_json();
+    assert_eq!(json["hints"][0], custom_hint);
+
+    // Text output includes the authored message.
+    let text = diagnostic.to_text(&source_ctx);
+    assert!(
+        text.contains(custom_hint),
+        "text output should include the authored errorMessage, got:\n{}",
+        text
+    );
+}
+
+#[test]
+fn test_custom_error_message_applies_to_any_failure_at_node() {
+    // The override should apply to whatever failure occurs at the annotated
+    // node, not just pattern mismatches. Here a type mismatch (number, not
+    // string) trips the same authored message.
+    let schema_yaml = quarto_yaml::parse(
+        r#"
+string:
+  pattern: "^[a-z]+$"
+  errorMessage: 'must be a lowercase identifier'
+"#,
+    )
+    .unwrap();
+    let schema = Schema::from_yaml(&schema_yaml).unwrap();
+
+    let doc_content = r#"42"#;
+    let doc = quarto_yaml::parse_file(doc_content, "id.yaml").unwrap();
+    let source_ctx = create_test_context("id.yaml", doc_content);
+    let registry = quarto_yaml_validation::SchemaRegistry::new();
+
+    let result = validate(&doc, &schema, &registry, &source_ctx);
+    assert!(result.is_err());
+
+    let error = result.unwrap_err();
+    assert_eq!(error.custom_hint.as_deref(), Some("must be a lowercase identifier"));
+
+    let diagnostic = ValidationDiagnostic::from_validation_error(&error, &source_ctx);
+    assert_eq!(
+        diagnostic.hints(),
+        vec!["must be a lowercase identifier".to_string()]
+    );
+}
+
+#[test]
+fn test_no_custom_error_message_uses_generic_hint() {
+    // Without errorMessage, the generic hint is still produced.
+    let schema_yaml = quarto_yaml::parse(
+        r#"
+string:
+  pattern: "^[0-9]+$"
+"#,
+    )
+    .unwrap();
+    let schema = Schema::from_yaml(&schema_yaml).unwrap();
+
+    let doc_content = r#"abc"#;
+    let doc = quarto_yaml::parse_file(doc_content, "p.yaml").unwrap();
+    let source_ctx = create_test_context("p.yaml", doc_content);
+    let registry = quarto_yaml_validation::SchemaRegistry::new();
+
+    let error = validate(&doc, &schema, &registry, &source_ctx).unwrap_err();
+    assert_eq!(error.custom_hint, None);
+
+    let diagnostic = ValidationDiagnostic::from_validation_error(&error, &source_ctx);
+    assert_eq!(
+        diagnostic.hints(),
+        vec!["Check that the string matches the expected format?".to_string()]
+    );
+}
+
+#[test]
+fn test_custom_error_message_innermost_node_wins() {
+    // An object property's own errorMessage should win over the outer
+    // object's — the override binds to the schema node where the failure
+    // occurs.
+    let schema_yaml = quarto_yaml::parse(
+        r#"
+object:
+  errorMessage: 'outer object message'
+  properties:
+    tz:
+      string:
+        pattern: "^UTC$"
+        errorMessage: 'inner tz message'
+"#,
+    )
+    .unwrap();
+    let schema = Schema::from_yaml(&schema_yaml).unwrap();
+
+    let doc_content = "tz: PST";
+    let doc = quarto_yaml::parse_file(doc_content, "doc.yaml").unwrap();
+    let source_ctx = create_test_context("doc.yaml", doc_content);
+    let registry = quarto_yaml_validation::SchemaRegistry::new();
+
+    let error = validate(&doc, &schema, &registry, &source_ctx).unwrap_err();
+    assert_eq!(error.custom_hint.as_deref(), Some("inner tz message"));
+}