From 2c6880ce509249db6015f3b4cf186b2fbe0ca492 Mon Sep 17 00:00:00 2001 From: Orual Date: Sun, 4 Jan 2026 20:55:19 -0500 Subject: [PATCH] better lexicon parsing errors --- Cargo.lock | 1 + crates/jacquard-lexicon/Cargo.toml | 1 + crates/jacquard-lexicon/src/corpus.rs | 329 +++++++++++++++++- crates/jacquard-lexicon/src/error.rs | 43 ++- .../fixtures/error_cases/broken_lexicon.json | 15 + .../fixtures/test_lexicons/not_a_lexicon.json | 7 + 6 files changed, 369 insertions(+), 27 deletions(-) create mode 100644 crates/jacquard-lexicon/tests/fixtures/error_cases/broken_lexicon.json create mode 100644 crates/jacquard-lexicon/tests/fixtures/test_lexicons/not_a_lexicon.json diff --git a/Cargo.lock b/Cargo.lock index 66114a07..34d6987c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2506,6 +2506,7 @@ dependencies = [ "serde", "serde_ipld_dagcbor", "serde_json", + "serde_path_to_error", "serde_repr", "serde_with", "sha2", diff --git a/crates/jacquard-lexicon/Cargo.toml b/crates/jacquard-lexicon/Cargo.toml index 63f16ec7..c9a4039a 100644 --- a/crates/jacquard-lexicon/Cargo.toml +++ b/crates/jacquard-lexicon/Cargo.toml @@ -27,6 +27,7 @@ prettyplease = { workspace = true, optional = true } proc-macro2 = { workspace = true, optional = true } quote = { workspace = true, optional = true } serde.workspace = true +serde_path_to_error = "0.1" serde_ipld_dagcbor.workspace = true serde_json.workspace = true serde_repr.workspace = true diff --git a/crates/jacquard-lexicon/src/corpus.rs b/crates/jacquard-lexicon/src/corpus.rs index 6f54522c..03df61c3 100644 --- a/crates/jacquard-lexicon/src/corpus.rs +++ b/crates/jacquard-lexicon/src/corpus.rs @@ -1,11 +1,260 @@ -use crate::ref_utils::RefPath; -use crate::error::Result; +use crate::error::{CodegenError, Result}; use crate::lexicon::{LexUserType, LexiconDoc}; +use crate::ref_utils::RefPath; use jacquard_common::{into_static::IntoStatic, smol_str::SmolStr}; use std::collections::BTreeMap; use std::fs; use std::path::Path; +/// Check if content looks like a lexicon file. +/// +/// A file is considered a lexicon if it contains a `"lexicon"` key at the top level +/// or one level down (for some wrapper formats). This allows us to distinguish +/// "not a lexicon at all" (skip silently) from "broken lexicon" (report error). +fn is_lexicon_content(content: &str) -> bool { + // Quick string scan first (fast path for non-JSON or unrelated JSON) + if !content.contains("\"lexicon\"") { + return false; + } + + // Parse to Value and check structure + if let Ok(value) = serde_json::from_str::(content) { + // Top-level lexicon field + if value.get("lexicon").is_some() { + return true; + } + // One level down (some wrapper formats) + if let Some(obj) = value.as_object() { + for v in obj.values() { + if v.get("lexicon").is_some() { + return true; + } + } + } + } + false +} + +/// Raw lexicon doc for two-phase parsing - defs are kept as raw JSON Values +/// so we can deserialize each separately with better error tracking. +#[derive(Debug, serde::Deserialize)] +struct RawLexiconDoc<'s> { + pub lexicon: crate::lexicon::Lexicon, + #[serde(borrow)] + pub id: jacquard_common::CowStr<'s>, + pub revision: Option, + #[serde(borrow)] + pub description: Option>, + pub defs: BTreeMap, +} + +/// Helper to create a parse error with path context. +fn make_parse_error( + file_path: &Path, + json_path: &str, + message: String, + content: &str, +) -> CodegenError { + CodegenError::ParseError { + path: file_path.to_path_buf(), + json_path: Some(json_path.to_string()), + message, + src: Some(content.to_string()), + span: None, + } +} + +/// Recursively parse properties with path tracking. +/// Returns parsed properties or an error with the full path. +fn parse_properties_deep( + props_value: &serde_json::Value, + base_path: &str, + file_path: &Path, + content: &str, +) -> std::result::Result>, CodegenError> +{ + let props_obj = props_value.as_object().ok_or_else(|| { + make_parse_error( + file_path, + base_path, + "expected object for properties".to_string(), + content, + ) + })?; + + let mut parsed_props = BTreeMap::new(); + for (prop_name, prop_value) in props_obj { + let prop_path = format!("{}.{}", base_path, prop_name); + + // Try to parse this property + let parsed: crate::lexicon::LexObjectProperty = + serde_path_to_error::deserialize(prop_value).map_err(|e| { + let inner_path = e.path().to_string(); + let full_path = if inner_path.is_empty() { + prop_path.clone() + } else { + format!("{}.{}", prop_path, inner_path) + }; + make_parse_error(file_path, &full_path, e.inner().to_string(), content) + })?; + + parsed_props.insert(SmolStr::new(prop_name), parsed.into_static()); + } + + Ok(parsed_props) +} + +/// Parse an object-like def with deep property tracking. +fn parse_object_deep( + value: &serde_json::Value, + base_path: &str, + file_path: &Path, + content: &str, +) -> std::result::Result, CodegenError> { + use crate::lexicon::LexObject; + + let obj = value.as_object().ok_or_else(|| { + make_parse_error(file_path, base_path, "expected object".to_string(), content) + })?; + + // Parse properties deeply if present + let properties = if let Some(props) = obj.get("properties") { + let props_path = format!("{}.properties", base_path); + parse_properties_deep(props, &props_path, file_path, content)? + } else { + BTreeMap::new() + }; + + // Parse the rest of the object normally + let description = obj + .get("description") + .and_then(|v| v.as_str()) + .map(|s| jacquard_common::CowStr::copy_from_str(s)); + let required: Option> = obj + .get("required") + .map(|v| serde_json::from_value(v.clone())) + .transpose() + .map_err(|e| make_parse_error(file_path, &format!("{}.required", base_path), e.to_string(), content))?; + let nullable: Option> = obj + .get("nullable") + .map(|v| serde_json::from_value(v.clone())) + .transpose() + .map_err(|e| make_parse_error(file_path, &format!("{}.nullable", base_path), e.to_string(), content))?; + + Ok(LexObject { + description, + required, + nullable, + properties, + }) +} + +/// Parse a def with deep path tracking for nested structures. +fn parse_def_deep( + def_name: &str, + value: &serde_json::Value, + file_path: &Path, + content: &str, +) -> std::result::Result, CodegenError> { + let base_path = format!("defs.{}", def_name); + + // Check the type field to determine how to parse + let type_str = value + .get("type") + .and_then(|v| v.as_str()) + .unwrap_or("object"); + + match type_str { + "object" => { + let obj = parse_object_deep(value, &base_path, file_path, content)?; + Ok(LexUserType::Object(obj)) + } + "record" => { + // Records have a nested record.properties structure + if let Some(record_value) = value.get("record") { + let record_path = format!("{}.record", base_path); + let inner_obj = parse_object_deep(record_value, &record_path, file_path, content)?; + + // Parse the rest of the record + let obj = value.as_object().ok_or_else(|| { + make_parse_error(file_path, &base_path, "expected object".to_string(), content) + })?; + + let description = obj + .get("description") + .and_then(|v| v.as_str()) + .map(|s| jacquard_common::CowStr::copy_from_str(s)); + let key: Option> = obj + .get("key") + .and_then(|v| v.as_str()) + .map(|s| jacquard_common::CowStr::copy_from_str(s)); + + Ok(LexUserType::Record(crate::lexicon::LexRecord { + description, + key, + record: crate::lexicon::LexRecordRecord::Object(inner_obj), + })) + } else { + // Fallback to normal parsing if no record field + serde_path_to_error::deserialize(value) + .map(|v: LexUserType| v.into_static()) + .map_err(|e| make_parse_error(file_path, &base_path, e.inner().to_string(), content)) + } + } + // For other types (query, procedure, etc.), use the simpler approach for now + // Could be extended later + _ => serde_path_to_error::deserialize(value) + .map(|v: LexUserType| v.into_static()) + .map_err(|e| { + let inner_path = e.path().to_string(); + let full_path = if inner_path.is_empty() { + base_path + } else { + format!("{}.{}", base_path, inner_path) + }; + make_parse_error(file_path, &full_path, e.inner().to_string(), content) + }), + } +} + +/// Parse a lexicon with rich error context using deep recursive parsing. +/// +/// This parses the document structure recursively, tracking paths through: +/// - defs → def_name → properties → prop_name → nested fields +/// +/// This gives us detailed error paths like "defs.main.properties.count.default" +fn parse_lexicon_with_context( + content: &str, + path: &Path, +) -> std::result::Result, CodegenError> { + // Phase 1: Parse the top-level structure with defs as raw Values + let raw_doc: RawLexiconDoc = serde_json::from_str(content).map_err(|e| { + CodegenError::ParseError { + path: path.to_path_buf(), + json_path: None, + message: e.to_string(), + src: Some(content.to_string()), + span: None, + } + })?; + + // Phase 2: Parse each def with deep path tracking + let mut parsed_defs = BTreeMap::new(); + for (def_name, def_value) in raw_doc.defs { + let parsed_def = parse_def_deep(&def_name, &def_value, path, content)?; + parsed_defs.insert(def_name, parsed_def); + } + + // Reconstruct the full LexiconDoc + Ok(LexiconDoc { + lexicon: raw_doc.lexicon, + id: raw_doc.id.into_static(), + revision: raw_doc.revision, + description: raw_doc.description.map(|d| d.into_static()), + defs: parsed_defs, + }) +} + /// Registry of all loaded lexicons for reference resolution #[derive(Debug, Clone)] pub struct LexiconCorpus { @@ -32,14 +281,17 @@ impl LexiconCorpus { for schema_path in schemas { let content = fs::read_to_string(schema_path.as_ref())?; - // Try to parse as lexicon doc - skip files that aren't lexicon schemas - let doc: LexiconDoc = match serde_json::from_str(&content) { - Ok(doc) => doc, - Err(_) => continue, // Skip non-lexicon JSON files - }; + // Check if this file is trying to be a lexicon + if !is_lexicon_content(&content) { + // Not a lexicon, skip silently + continue; + } + + // This IS a lexicon - parse with good error reporting + let doc = parse_lexicon_with_context(&content, schema_path.as_ref())?; let nsid = SmolStr::from(doc.id.to_string()); - corpus.docs.insert(nsid.clone(), doc.into_static()); + corpus.docs.insert(nsid.clone(), doc); corpus.sources.insert(nsid, content); } @@ -167,4 +419,65 @@ mod tests { assert!(!corpus.ref_exists("com.example.fake")); assert!(!corpus.ref_exists("app.bsky.feed.post#nonexistent")); } + + #[test] + fn test_non_lexicon_json_skipped_silently() { + // The test_lexicons directory contains not_a_lexicon.json which should be skipped + let corpus = LexiconCorpus::load_from_dir("tests/fixtures/test_lexicons") + .expect("should succeed even with non-lexicon JSON files"); + + // The non-lexicon file should not be in the corpus + assert!(corpus.get("some random config").is_none()); + + // But valid lexicons should still load + assert!(corpus.get("app.bsky.feed.post").is_some()); + } + + #[test] + fn test_is_lexicon_content_detection() { + // Not a lexicon - no "lexicon" key + assert!(!is_lexicon_content(r#"{"name": "test", "version": "1.0"}"#)); + + // Not a lexicon - invalid JSON + assert!(!is_lexicon_content("not json at all")); + + // Is a lexicon - has "lexicon" at top level + assert!(is_lexicon_content(r#"{"lexicon": 1, "id": "test.foo"}"#)); + + // Is a lexicon - has "lexicon" one level down + assert!(is_lexicon_content( + r#"{"wrapper": {"lexicon": 1, "id": "test.foo"}}"# + )); + } + + #[test] + fn test_broken_lexicon_returns_error_with_path() { + let result = LexiconCorpus::load_from_dir("tests/fixtures/error_cases"); + + // Should fail because broken_lexicon.json is a lexicon (has "lexicon" key) + // but has invalid structure + let err = result.expect_err("should fail on broken lexicon"); + let err_str = err.to_string(); + + // Error should include the full path to the broken property + assert!( + err_str.contains("defs.main.properties.count"), + "error should contain path to the broken property, got: {}", + err_str + ); + + // Error should also include the actual error message + assert!( + err_str.contains("expected i64"), + "error should describe the type mismatch, got: {}", + err_str + ); + + // Error should mention the file + assert!( + err_str.contains("broken_lexicon.json"), + "error should mention the file, got: {}", + err_str + ); + } } diff --git a/crates/jacquard-lexicon/src/error.rs b/crates/jacquard-lexicon/src/error.rs index ffb06334..a8b77a97 100644 --- a/crates/jacquard-lexicon/src/error.rs +++ b/crates/jacquard-lexicon/src/error.rs @@ -3,6 +3,15 @@ use std::io; use std::path::PathBuf; use thiserror::Error; +fn format_parse_error(path: &PathBuf, json_path: Option<&str>, message: &str) -> String { + match json_path { + Some(jp) if !jp.is_empty() => { + format!("failed to parse lexicon {}: at {}: {}", path.display(), jp, message) + } + _ => format!("failed to parse lexicon {}: {}", path.display(), message), + } +} + /// Errors that can occur during lexicon code generation #[derive(Debug, Error, Diagnostic)] #[non_exhaustive] @@ -12,16 +21,18 @@ pub enum CodegenError { Io(#[from] io::Error), /// Failed to parse lexicon JSON - #[error("Failed to parse lexicon JSON in {}", path.display())] + #[error("{}", format_parse_error(path, json_path.as_deref(), message))] #[diagnostic( code(lexicon::parse_error), help("Check that the lexicon file is valid JSON and follows the lexicon schema") )] ParseError { - #[source] - source: serde_json::Error, /// Path to the file that failed to parse path: PathBuf, + /// JSON path where the error occurred (from serde_path_to_error) + json_path: Option, + /// The underlying error message + message: String, /// Source text that failed to parse #[source_code] src: Option, @@ -90,35 +101,29 @@ pub enum CodegenError { impl CodegenError { /// Create a parse error with context - pub fn parse_error(source: serde_json::Error, path: impl Into) -> Self { + pub fn parse_error(message: impl Into, path: impl Into) -> Self { Self::ParseError { - source, path: path.into(), + json_path: None, + message: message.into(), src: None, span: None, } } - /// Create a parse error with source text - pub fn parse_error_with_source( - source: serde_json::Error, + /// Create a parse error with source text and JSON path + pub fn parse_error_with_context( + message: impl Into, path: impl Into, + json_path: Option, src: String, ) -> Self { - // Try to extract error location from serde_json error - let span = if let Some(line) = source.line().checked_sub(1) { - let col = source.column().saturating_sub(1); - // Approximate byte offset (not perfect but good enough for display) - Some((line * 80 + col, 1).into()) - } else { - None - }; - Self::ParseError { - source, path: path.into(), + json_path, + message: message.into(), src: Some(src), - span, + span: None, } } diff --git a/crates/jacquard-lexicon/tests/fixtures/error_cases/broken_lexicon.json b/crates/jacquard-lexicon/tests/fixtures/error_cases/broken_lexicon.json new file mode 100644 index 00000000..227daf4e --- /dev/null +++ b/crates/jacquard-lexicon/tests/fixtures/error_cases/broken_lexicon.json @@ -0,0 +1,15 @@ +{ + "lexicon": 1, + "id": "test.broken.lexicon", + "defs": { + "main": { + "type": "object", + "properties": { + "count": { + "type": "integer", + "default": "not_a_number" + } + } + } + } +} diff --git a/crates/jacquard-lexicon/tests/fixtures/test_lexicons/not_a_lexicon.json b/crates/jacquard-lexicon/tests/fixtures/test_lexicons/not_a_lexicon.json new file mode 100644 index 00000000..88443e86 --- /dev/null +++ b/crates/jacquard-lexicon/tests/fixtures/test_lexicons/not_a_lexicon.json @@ -0,0 +1,7 @@ +{ + "name": "some random config", + "version": "1.0.0", + "settings": { + "enabled": true + } +} -- 2.51.2