diff --git a/crates/aingle_cortex/src/rest/dag.rs b/crates/aingle_cortex/src/rest/dag.rs index 0bdd304..6afcc20 100644 --- a/crates/aingle_cortex/src/rest/dag.rs +++ b/crates/aingle_cortex/src/rest/dag.rs @@ -387,12 +387,10 @@ pub async fn get_dag_export( ) -> Result { use axum::response::IntoResponse; - let format = aingle_graph::dag::ExportFormat::from_str(&query.format).ok_or_else(|| { - Error::InvalidInput(format!( - "Unknown format '{}'. Use: dot, mermaid, json", - query.format - )) - })?; + let format: aingle_graph::dag::ExportFormat = query + .format + .parse() + .map_err(|e: aingle_graph::dag::UnknownExportFormat| Error::InvalidInput(e.to_string()))?; let graph = state.graph.read().await; let dag_graph = graph diff --git a/crates/aingle_graph/src/dag/export.rs b/crates/aingle_graph/src/dag/export.rs index 11b02bc..349ede5 100644 --- a/crates/aingle_graph/src/dag/export.rs +++ b/crates/aingle_graph/src/dag/export.rs @@ -40,24 +40,41 @@ pub enum ExportFormat { Json, } -impl ExportFormat { - /// Parse from string (case-insensitive). - /// - /// Shadows `std::str::FromStr::from_str`, which clippy rightly flags: a - /// caller writing `ExportFormat::from_str` cannot tell which one they get, - /// and this one returns `Option` where the trait returns `Result`. - /// - /// The fix is to implement `FromStr` properly with an error type and drop - /// this inherent method. It is cheap — there is exactly one caller outside - /// this module's own tests — but it changes a public signature, so it is a - /// deliberate change with its own review and not a line in a lint sweep. - #[allow(clippy::should_implement_trait)] - pub fn from_str(s: &str) -> Option { +/// The string offered to [`ExportFormat`]'s `FromStr` named no known format. +/// +/// Carries the offending input and knows the accepted spellings, so a caller +/// does not have to keep its own copy of the list — which is how a REST handler +/// ends up advertising formats the parser no longer accepts. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct UnknownExportFormat(pub String); + +impl std::fmt::Display for UnknownExportFormat { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!( + f, + "unknown export format '{}'. Use: dot (or graphviz), mermaid (or md), json", + self.0 + ) + } +} + +impl std::error::Error for UnknownExportFormat {} + +/// Parse from string (case-insensitive). +/// +/// This was an inherent `from_str` returning `Option`, which shadowed +/// `std::str::FromStr::from_str` and returned a different shape from it: a +/// caller reading `ExportFormat::from_str(..)` could not tell which one they +/// were getting, and got no reason for the failure either way. +impl std::str::FromStr for ExportFormat { + type Err = UnknownExportFormat; + + fn from_str(s: &str) -> Result { match s.to_lowercase().as_str() { - "dot" | "graphviz" => Some(Self::Dot), - "mermaid" | "md" => Some(Self::Mermaid), - "json" => Some(Self::Json), - _ => None, + "dot" | "graphviz" => Ok(Self::Dot), + "mermaid" | "md" => Ok(Self::Mermaid), + "json" => Ok(Self::Json), + _ => Err(UnknownExportFormat(s.to_string())), } } } @@ -319,15 +336,28 @@ mod tests { #[test] fn test_export_format_parsing() { - assert_eq!(ExportFormat::from_str("dot"), Some(ExportFormat::Dot)); - assert_eq!(ExportFormat::from_str("DOT"), Some(ExportFormat::Dot)); - assert_eq!(ExportFormat::from_str("graphviz"), Some(ExportFormat::Dot)); + assert_eq!("dot".parse(), Ok(ExportFormat::Dot)); + assert_eq!("DOT".parse(), Ok(ExportFormat::Dot)); + assert_eq!("graphviz".parse(), Ok(ExportFormat::Dot)); + assert_eq!("mermaid".parse(), Ok(ExportFormat::Mermaid)); + assert_eq!("md".parse(), Ok(ExportFormat::Mermaid)); + assert_eq!("json".parse(), Ok(ExportFormat::Json)); assert_eq!( - ExportFormat::from_str("mermaid"), - Some(ExportFormat::Mermaid) + "xml".parse::(), + Err(UnknownExportFormat("xml".to_string())) ); - assert_eq!(ExportFormat::from_str("json"), Some(ExportFormat::Json)); - assert_eq!(ExportFormat::from_str("xml"), None); + } + + #[test] + fn an_unknown_format_says_what_it_would_have_accepted() { + // The list used to live in the REST handler, where it could drift away + // from the parser without anything noticing. + let e = "xml".parse::().unwrap_err(); + let msg = e.to_string(); + assert!(msg.contains("xml"), "{msg}"); + for accepted in ["dot", "graphviz", "mermaid", "md", "json"] { + assert!(msg.contains(accepted), "{accepted} missing from: {msg}"); + } } #[test] diff --git a/crates/aingle_graph/src/dag/mod.rs b/crates/aingle_graph/src/dag/mod.rs index 970d8ee..5a03056 100644 --- a/crates/aingle_graph/src/dag/mod.rs +++ b/crates/aingle_graph/src/dag/mod.rs @@ -33,7 +33,7 @@ pub use action::{ pub use backend::SledDagBackend; pub use backend::{DagBackend, MemoryDagBackend}; pub use canonical::{CanonicalAction, CANONICAL_SPEC, HASH_ALG, SIGNATURE_ALG, SIGNED_MESSAGE}; -pub use export::{DagGraph, ExportFormat}; +pub use export::{DagGraph, ExportFormat, UnknownExportFormat}; pub use pruning::{PruneResult, RetentionPolicy}; #[cfg(feature = "dag-sign")] pub use signing::{DagSigningKey, DagVerifyingKey, VerifyResult};