diff --git a/mlf-cli/src/fetch.rs b/mlf-cli/src/fetch.rs index 4091c96..8cd438c 100644 --- a/mlf-cli/src/fetch.rs +++ b/mlf-cli/src/fetch.rs @@ -448,8 +448,11 @@ async fn fetch_specific_lexicon( println!(" → Saved JSON (checksum verified)"); // Convert to MLF - let mlf_content = crate::generate::mlf::generate_mlf_from_json(&fetched.lexicon) + let converted = crate::generate::mlf::generate_mlf_from_json(&fetched.lexicon) .map_err(|e| FetchError::ConversionError(format!("{:?}", e)))?; + for warning in &converted.warnings { + eprintln!(" ⚠ {}: {}", warning.namespace, warning.message); + } let mut mlf_path = mlf_dir.join("lexicons/mlf"); for segment in nsid.split('.') { @@ -460,7 +463,7 @@ async fn fetch_specific_lexicon( if let Some(parent) = mlf_path.parent() { std::fs::create_dir_all(parent)?; } - std::fs::write(&mlf_path, mlf_content)?; + std::fs::write(&mlf_path, converted.mlf)?; println!(" → Converted to MLF"); Ok(()) @@ -531,8 +534,11 @@ async fn fetch_lexicon_with_lock(nsid: &str, project_root: &std::path::Path, loc println!(" → Saved JSON to {}", json_path.display()); // Convert to MLF - let mlf_content = crate::generate::mlf::generate_mlf_from_json(&fetched.lexicon) + let converted = crate::generate::mlf::generate_mlf_from_json(&fetched.lexicon) .map_err(|e| FetchError::ConversionError(format!("{:?}", e)))?; + for warning in &converted.warnings { + eprintln!(" ⚠ {}: {}", warning.namespace, warning.message); + } // Save MLF file let mut mlf_path = mlf_dir.join("lexicons/mlf"); @@ -544,7 +550,7 @@ async fn fetch_lexicon_with_lock(nsid: &str, project_root: &std::path::Path, loc if let Some(parent) = mlf_path.parent() { std::fs::create_dir_all(parent)?; } - std::fs::write(&mlf_path, mlf_content)?; + std::fs::write(&mlf_path, converted.mlf)?; println!(" → Converted to MLF at {}", mlf_path.display()); // Calculate hash and extract dependencies for lockfile diff --git a/mlf-cli/src/generate/mlf.rs b/mlf-cli/src/generate/mlf.rs index 39e75bf..f3499b8 100644 --- a/mlf-cli/src/generate/mlf.rs +++ b/mlf-cli/src/generate/mlf.rs @@ -1,8 +1,29 @@ use miette::Diagnostic; use serde_json::Value; +use std::cell::RefCell; use std::path::PathBuf; use thiserror::Error; +/// A non-fatal issue surfaced by the Lexicon→MLF converter. Produced +/// when the source lexicon is malformed in a way we can recover from +/// (typically: missing spec-required fields we coerce to empty / +/// fall-back values). Callers decide whether to print, log, or ignore. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ConversionWarning { + /// Namespace of the lexicon the warning came from. + pub namespace: String, + /// Human-readable description of what we coerced and why. + pub message: String, +} + +/// Output of [`generate_mlf_from_json`]. Carries both the rendered MLF +/// and any non-fatal warnings accumulated during conversion. +#[derive(Debug, Clone)] +pub struct MlfGenerateOutput { + pub mlf: String, + pub warnings: Vec, +} + #[derive(Error, Debug, Diagnostic)] pub enum MlfGenerateError { #[error("Failed to read file: {path}")] @@ -131,13 +152,20 @@ pub fn run(input_patterns: Vec, output_dir: Option, flat: bool) } }; - let mlf_content = match generate_mlf_from_json(&json) { - Ok(content) => content, + let output = match generate_mlf_from_json(&json) { + Ok(output) => output, Err(e) => { errors.push((file_path.display().to_string(), format!("{:?}", e))); continue; } }; + for warning in &output.warnings { + eprintln!( + "warning ({}): {}", + warning.namespace, warning.message + ); + } + let mlf_content = output.mlf; // Extract namespace from JSON "id" field let namespace = json @@ -197,7 +225,7 @@ pub fn run(input_patterns: Vec, output_dir: Option, flat: bool) Ok(()) } -pub fn generate_mlf_from_json(json: &Value) -> Result { +pub fn generate_mlf_from_json(json: &Value) -> Result { let mut output = String::new(); // Extract NSID to get the last segment for "main" definitions @@ -219,6 +247,7 @@ pub fn generate_mlf_from_json(json: &Value) -> Result // Create a context to pass the current namespace to type generation let ctx = ConversionContext { current_namespace: nsid.to_string(), + warnings: RefCell::new(Vec::new()), }; // Process all definitions @@ -264,11 +293,27 @@ pub fn generate_mlf_from_json(json: &Value) -> Result } } - Ok(output) + Ok(MlfGenerateOutput { + mlf: output, + warnings: ctx.warnings.into_inner(), + }) } struct ConversionContext { current_namespace: String, + /// Non-fatal issues accumulated during conversion. Callers receive + /// these via [`MlfGenerateOutput`] and decide what to do with them + /// (print to stderr, collect for a summary, suppress). + warnings: RefCell>, +} + +impl ConversionContext { + fn warn(&self, message: impl Into) { + self.warnings.borrow_mut().push(ConversionWarning { + namespace: self.current_namespace.clone(), + message: message.into(), + }); + } } /// Reserved words in MLF that need to be escaped @@ -877,11 +922,25 @@ fn render_array( ctx: &ConversionContext, indent_level: usize, ) -> Result { - let items = type_def - .get("items") - .ok_or_else(|| MlfGenerateError::InvalidLexicon { - message: "Missing 'items' in array type".to_string(), - })?; + // `items` is spec-required on `array` types. Handle a missing field + // leniently — fall back to `unknown` as the item type, warn — to + // match the lenient handling of `object` without `properties` and + // empty-refs unions. Malformed-but-publishable lexicons stay + // convertible instead of blocking the whole authority. + let fallback_items = Value::Object(serde_json::Map::from_iter([( + "type".to_string(), + Value::String("unknown".to_string()), + )])); + let items = match type_def.get("items") { + Some(v) => v, + None => { + ctx.warn( + "array type is missing `items` field; \ + treating item type as `unknown` (ATProto spec lists `items` as required)", + ); + &fallback_items + } + }; let item = generate_type(items, ctx, indent_level)?; let base = format!("{}[]", item.into_array_base()); Ok(Rendered::with_constraints(&base, type_def, indent_level)) @@ -895,12 +954,26 @@ fn render_object_inline( let obj = type_def.as_object().ok_or_else(|| MlfGenerateError::InvalidLexicon { message: "Object type definition is not a JSON object".to_string(), })?; - let properties = obj - .get("properties") - .and_then(|v| v.as_object()) - .ok_or_else(|| MlfGenerateError::InvalidLexicon { - message: "Missing 'properties' in object type".to_string(), - })?; + + // The spec lists `properties` as required on object types, but real- + // world lexicons (e.g. blog.pckt.richtext.facet marker defs) publish + // empty objects with no `properties` field at all. Accept that + // leniently — semantically missing `properties` is equivalent to + // `properties: {}` — but surface a warning so authors know their + // lexicon isn't strictly spec-compliant. + let empty_map = serde_json::Map::new(); + let properties = match obj.get("properties") { + Some(v) => v.as_object().ok_or_else(|| MlfGenerateError::InvalidLexicon { + message: "`properties` in object type must be a JSON object".to_string(), + })?, + None => { + ctx.warn( + "object type is missing `properties` field; \ + treating as empty (ATProto spec lists `properties` as required)", + ); + &empty_map + } + }; let required = string_array(obj, "required"); let nullable = string_array(obj, "nullable"); @@ -977,6 +1050,21 @@ fn render_union( message: "Missing 'refs' in union type".to_string(), })?; + let closed = type_def.get("closed").and_then(|v| v.as_bool()).unwrap_or(false); + + // An open union with zero refs is malformed per the ATProto spec — it + // names no valid types at all. Real-world lexicons (e.g. + // blog.pckt.content) publish this shape anyway; rather than refusing + // to convert we fall back to `unknown` with a warning, matching the + // lenient handling of `object` types without `properties`. + if refs.is_empty() && !closed { + ctx.warn( + "open union has no `refs`; emitting `unknown` as a placeholder \ + (ATProto spec lists `refs` as required on union types)", + ); + return Ok(Rendered::atom("unknown")); + } + // Each entry in `refs` is a string (local `#defName` or external // `namespace#defName`), not a nested type object. let parts: Vec = refs @@ -988,7 +1076,6 @@ fn render_union( }) .collect(); - let closed = type_def.get("closed").and_then(|v| v.as_bool()).unwrap_or(false); let mut text = parts.join(" | "); if closed { text.push_str(" | !"); diff --git a/tests/lexicon_to_mlf/lenient_array_without_items/expected.mlf b/tests/lexicon_to_mlf/lenient_array_without_items/expected.mlf new file mode 100644 index 0000000..dba5d2f --- /dev/null +++ b/tests/lexicon_to_mlf/lenient_array_without_items/expected.mlf @@ -0,0 +1,2 @@ +def type tags = unknown[]; + diff --git a/tests/lexicon_to_mlf/lenient_array_without_items/expected_warnings.txt b/tests/lexicon_to_mlf/lenient_array_without_items/expected_warnings.txt new file mode 100644 index 0000000..3540c6e --- /dev/null +++ b/tests/lexicon_to_mlf/lenient_array_without_items/expected_warnings.txt @@ -0,0 +1 @@ +com.example.arraynoitems: array type is missing `items` field; treating item type as `unknown` (ATProto spec lists `items` as required) diff --git a/tests/lexicon_to_mlf/lenient_array_without_items/input.json b/tests/lexicon_to_mlf/lenient_array_without_items/input.json new file mode 100644 index 0000000..6d16269 --- /dev/null +++ b/tests/lexicon_to_mlf/lenient_array_without_items/input.json @@ -0,0 +1,9 @@ +{ + "lexicon": 1, + "id": "com.example.arraynoitems", + "defs": { + "tags": { + "type": "array" + } + } +} diff --git a/tests/lexicon_to_mlf/lenient_empty_union_refs/expected.mlf b/tests/lexicon_to_mlf/lenient_empty_union_refs/expected.mlf new file mode 100644 index 0000000..38badcd --- /dev/null +++ b/tests/lexicon_to_mlf/lenient_empty_union_refs/expected.mlf @@ -0,0 +1,2 @@ +def type items = unknown[]; + diff --git a/tests/lexicon_to_mlf/lenient_empty_union_refs/expected_warnings.txt b/tests/lexicon_to_mlf/lenient_empty_union_refs/expected_warnings.txt new file mode 100644 index 0000000..1696f44 --- /dev/null +++ b/tests/lexicon_to_mlf/lenient_empty_union_refs/expected_warnings.txt @@ -0,0 +1 @@ +com.example.emptyunion: open union has no `refs`; emitting `unknown` as a placeholder (ATProto spec lists `refs` as required on union types) diff --git a/tests/lexicon_to_mlf/lenient_empty_union_refs/input.json b/tests/lexicon_to_mlf/lenient_empty_union_refs/input.json new file mode 100644 index 0000000..7d6a9e4 --- /dev/null +++ b/tests/lexicon_to_mlf/lenient_empty_union_refs/input.json @@ -0,0 +1,14 @@ +{ + "lexicon": 1, + "id": "com.example.emptyunion", + "defs": { + "items": { + "type": "array", + "items": { + "type": "union", + "refs": [], + "closed": false + } + } + } +} diff --git a/tests/lexicon_to_mlf/lenient_object_without_properties/expected.mlf b/tests/lexicon_to_mlf/lenient_object_without_properties/expected.mlf new file mode 100644 index 0000000..0c4c112 --- /dev/null +++ b/tests/lexicon_to_mlf/lenient_object_without_properties/expected.mlf @@ -0,0 +1,4 @@ +/// Marker for bold text +def type bold = { +}; + diff --git a/tests/lexicon_to_mlf/lenient_object_without_properties/expected_warnings.txt b/tests/lexicon_to_mlf/lenient_object_without_properties/expected_warnings.txt new file mode 100644 index 0000000..dd12b80 --- /dev/null +++ b/tests/lexicon_to_mlf/lenient_object_without_properties/expected_warnings.txt @@ -0,0 +1 @@ +com.example.marker: object type is missing `properties` field; treating as empty (ATProto spec lists `properties` as required) diff --git a/tests/lexicon_to_mlf/lenient_object_without_properties/input.json b/tests/lexicon_to_mlf/lenient_object_without_properties/input.json new file mode 100644 index 0000000..72e8b9b --- /dev/null +++ b/tests/lexicon_to_mlf/lenient_object_without_properties/input.json @@ -0,0 +1,10 @@ +{ + "lexicon": 1, + "id": "com.example.marker", + "defs": { + "bold": { + "type": "object", + "description": "Marker for bold text" + } + } +} diff --git a/tests/lexicon_to_mlf_integration.rs b/tests/lexicon_to_mlf_integration.rs index 180655a..51728c4 100644 --- a/tests/lexicon_to_mlf_integration.rs +++ b/tests/lexicon_to_mlf_integration.rs @@ -3,6 +3,9 @@ // Each subdirectory of `lexicon_to_mlf/` is a single test case containing: // - `input.json`: the Lexicon JSON to convert // - `expected.mlf`: the expected MLF source produced by the converter +// - `expected_warnings.txt` (optional): the expected warnings, one +// per line in `: ` form. When absent, the test +// asserts no warnings were produced. use mlf_cli::generate::mlf::generate_mlf_from_json; use serde_json::Value; @@ -18,11 +21,35 @@ fn run_case(input_path: &Path) -> datatest_stable::Result<()> { let output = generate_mlf_from_json(&json).map_err(|e| format!("{:?}", e))?; let expected = fs::read_to_string(test_dir.join("expected.mlf"))?; + if output.mlf != expected { + return Err(format!( + "MLF mismatch:\n--- expected ---\n{}\n--- got ---\n{}", + expected, output.mlf + ) + .into()); + } - if output != expected { + let warnings_path = test_dir.join("expected_warnings.txt"); + let expected_warnings = if warnings_path.exists() { + fs::read_to_string(&warnings_path)? + } else { + String::new() + }; + let actual_warnings = output + .warnings + .iter() + .map(|w| format!("{}: {}", w.namespace, w.message)) + .collect::>() + .join("\n"); + let actual_warnings = if actual_warnings.is_empty() { + String::new() + } else { + format!("{}\n", actual_warnings) + }; + if actual_warnings != expected_warnings { return Err(format!( - "Output mismatch:\n--- expected ---\n{}\n--- got ---\n{}", - expected, output + "Warnings mismatch:\n--- expected ---\n{}\n--- got ---\n{}", + expected_warnings, actual_warnings ) .into()); }