From ed970310e266c650d082794e246d8d077a39ce46 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 27 Apr 2022 16:56:36 +0100 Subject: [PATCH] CfgPath: Overhaul API Document that this can contain either a string for expansion, or a literal PathBuf not for expansion. Rename the `from_path` method to `new_literal`: a very important difference is whether it gets expanded - less important than the Rust type. Also, now it takes `Into`, which avoids a needless clone. (We don't change the API in `arti-client` because `&tempfile::Tempdir()` doesn't implement `Into`, so `arti-client` has to have some new `as_ref` calls.) Provide accessors `as_unexpanded_str` and `as_literal_path`. The deserialisation already makes this part of the stable API,l so not pvoding accessors seems just obstructive. They are useful for tests, too. Add tests for the new entrypoints, and for deserialisation of both variants from TOML (via config, or directly) and JSON. --- Cargo.lock | 3 ++ crates/arti-client/src/config.rs | 4 +- crates/tor-config/Cargo.toml | 3 ++ crates/tor-config/src/path.rs | 92 +++++++++++++++++++++++++++++--- 4 files changed, 94 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cac6394c2..8ec1f6b65 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3291,13 +3291,16 @@ dependencies = [ name = "tor-config" version = "0.2.0" dependencies = [ + "config", "derive_builder", "directories", "dirs", "once_cell", "serde", + "serde_json", "shellexpand-fork", "thiserror", + "toml", "tor-basic-utils", "tor-error", "tracing", diff --git a/crates/arti-client/src/config.rs b/crates/arti-client/src/config.rs index 9b31861c5..0b727d87f 100644 --- a/crates/arti-client/src/config.rs +++ b/crates/arti-client/src/config.rs @@ -362,8 +362,8 @@ impl TorClientConfigBuilder { let mut builder = Self::default(); builder .storage() - .cache_dir(CfgPath::from_path(cache_dir)) - .state_dir(CfgPath::from_path(state_dir)); + .cache_dir(CfgPath::new_literal(cache_dir.as_ref())) + .state_dir(CfgPath::new_literal(state_dir.as_ref())); builder } } diff --git a/crates/tor-config/Cargo.toml b/crates/tor-config/Cargo.toml index 4f5e1a558..7951437ba 100644 --- a/crates/tor-config/Cargo.toml +++ b/crates/tor-config/Cargo.toml @@ -28,5 +28,8 @@ tracing = "0.1.18" directories = { version = "4", optional = true } [dev-dependencies] +config = { version = "0.12.0", default-features = false, features = ["toml"] } dirs = "4.0.0" +serde_json = "1.0.50" +toml = "0.5" tracing-test = "0.2" diff --git a/crates/tor-config/src/path.rs b/crates/tor-config/src/path.rs index 2edd45b75..75cb19462 100644 --- a/crates/tor-config/src/path.rs +++ b/crates/tor-config/src/path.rs @@ -27,11 +27,15 @@ use tor_error::{ErrorKind, HasKind}; /// so should use appropriate system-specific overrides under the /// hood. (Some of those overrides are based on environment variables.) /// For more information, see that crate's documentation. +/// +/// Alternatively, a `CfgPath` can contain literal `PathBuf`, which will not be expaneded. #[derive(Clone, Debug, Deserialize, Eq, PartialEq)] #[serde(transparent)] pub struct CfgPath(PathInner); /// Inner implementation of CfgPath +/// +/// `PathInner` exists to avoid making the variants part of the public Rust API #[derive(Clone, Debug, Deserialize, Eq, PartialEq)] #[serde(untagged)] enum PathInner { @@ -101,6 +105,13 @@ impl CfgPath { CfgPath(PathInner::Shell(s)) } + /// Construct a new `CfgPath` designating a literal not-to-be-expanded `PathBuf` + pub fn new_literal>(path: P) -> Self { + CfgPath(PathInner::Literal(LiteralPath { + literal: path.into(), + })) + } + /// Return the path on disk designated by this `CfgPath`. pub fn path(&self) -> Result { match &self.0 { @@ -109,11 +120,28 @@ impl CfgPath { } } - /// Construct a new `CfgPath` from a system path. - pub fn from_path>(path: P) -> Self { - CfgPath(PathInner::Literal(LiteralPath { - literal: path.as_ref().to_owned(), - })) + /// If the `CfgPath` is a string that should be expaneded, return the (unexpanded) string, + /// + /// Before use, this string would have be to expanded. So if you want a path to actually use, + /// call `path` instead. + /// + /// Returns `None` if the `CfgPath` is a literal `PathBuf` not intended for expansion. + pub fn as_unexpanded_str(&self) -> Option<&str> { + match &self.0 { + PathInner::Shell(s) => Some(s), + PathInner::Literal(_) => None, + } + } + + /// If the `CfgPath` designates a literal not-to-be-expanded `Path`, return a reference to it + /// + /// Returns `None` if the `CfgPath` is a string which should be expanded, which is the + /// usual case. + pub fn as_literal_path(&self) -> Option<&Path> { + match &self.0 { + PathInner::Shell(_) => None, + PathInner::Literal(LiteralPath { literal }) => Some(literal), + } } } @@ -268,7 +296,7 @@ mod test { #[test] fn literal() { - let p = CfgPath::from_path(PathBuf::from("${ARTI_CACHE}/literally")); + let p = CfgPath::new_literal(PathBuf::from("${ARTI_CACHE}/literally")); // This doesn't get expanded, since we're using a literal path. assert_eq!( p.path().unwrap().to_str().unwrap(), @@ -277,3 +305,55 @@ mod test { assert_eq!(p.to_string(), "\"${ARTI_CACHE}/literally\" [exactly]"); } } + +#[cfg(test)] +mod test_serde { + use super::*; + + #[derive(Deserialize, Eq, PartialEq, Debug)] + struct TestConfigFile { + p: CfgPath, + } + + fn deser_json(json: &str) -> CfgPath { + dbg!(json); + let TestConfigFile { p } = serde_json::from_str(json).expect("deser json failed"); + p + } + fn deser_toml(toml: &str) -> CfgPath { + dbg!(toml); + let TestConfigFile { p } = toml::from_str(toml).expect("deser toml failed"); + p + } + fn deser_toml_cfg(toml: &str) -> CfgPath { + dbg!(toml); + let cfg = config::File::from_str(toml, config::FileFormat::Toml); + let cfg = config::Config::builder() + .add_source(cfg) + .build() + .expect("parse toml failed"); + dbg!(&cfg); + let TestConfigFile { p } = cfg.try_deserialize().expect("deser cfg failed"); + p + } + + #[test] + fn test_parse() { + fn desers(toml: &str, json: &str) -> Vec { + vec![deser_toml(toml), deser_toml_cfg(toml), deser_json(json)] + } + + for cp in desers(r#"p = "string""#, r#"{ "p": "string" }"#) { + assert_eq!(cp.as_unexpanded_str(), Some("string")); + assert_eq!(cp.as_literal_path(), None); + } + + for cp in desers( + r#"p = { literal = "lit" }"#, + r#"{ "p": {"literal": "lit"} }"#, + ) { + assert_eq!(cp.as_unexpanded_str(), None); + assert_eq!(cp.as_literal_path(), Some(&*PathBuf::from("lit"))); + } + } +}