From 1ca5a9287e7350048caace59f0efc899d9cc9a6c Mon Sep 17 00:00:00 2001 From: ApiliumDevTeam Date: Wed, 29 Jul 2026 12:00:19 +0200 Subject: [PATCH] refactor(dag): ExportFormat implements FromStr instead of shadowing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ExportFormat::from_str` was an inherent method returning `Option`, which shadows `std::str::FromStr::from_str` and returns 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. Implement the trait with an error that carries the offending input and knows the accepted spellings. The list of valid formats lived in the REST handler, where it could drift away from the parser with nothing to catch it — it already advertised "dot, mermaid, json" while the parser also accepted "graphviz" and "md". Now the type owns it and a test asserts the message names every spelling the parser takes. One caller migrated. `cargo clippy --workspace --all-targets -- -D warnings` and `cargo fmt --all --check` are both clean with no `#[allow]` left anywhere from this series. --- crates/aingle_cortex/src/rest/dag.rs | 10 ++-- crates/aingle_graph/src/dag/export.rs | 78 ++++++++++++++++++--------- crates/aingle_graph/src/dag/mod.rs | 2 +- 3 files changed, 59 insertions(+), 31 deletions(-) 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};