From 6f34124d430ffecfbb858b7136834e6006da8ee9 Mon Sep 17 00:00:00 2001 From: Fintan Halpenny Date: Wed, 18 Jun 2025 12:32:41 +0200 Subject: [PATCH] radicle: improve config errors Improve the granularity of config errors mentioning the path location where the configuration is expected to be loading from or saving to. --- crates/radicle-cli/examples/rad-config.md | 8 +- crates/radicle-cli/src/commands/config.rs | 38 +++-- crates/radicle-cli/src/terminal.rs | 2 +- crates/radicle/src/profile.rs | 6 +- crates/radicle/src/profile/config.rs | 188 ++++++++++++++++++---- 5 files changed, 188 insertions(+), 54 deletions(-) diff --git a/crates/radicle-cli/examples/rad-config.md b/crates/radicle-cli/examples/rad-config.md index 3800e898..a55cadb7 100644 --- a/crates/radicle-cli/examples/rad-config.md +++ b/crates/radicle-cli/examples/rad-config.md @@ -105,7 +105,7 @@ $ rad config unset web.name ``` (fail) $ rad config get web.name -✗ Error: configuration error: web.name does not exist +✗ Error: web.name does not exist in configuration found at "[..]/.radicle/config.json" ``` Values along the path will be created if necessary. @@ -129,19 +129,19 @@ Values that are required for a valid config can't be deleted. ``` (fail) $ rad config unset node.alias -✗ Error: configuration JSON error: missing field `alias` +✗ Error: writing configuration to "[..]/.radicle/config.json" failed: validation failure due to missing field `alias` ``` Values for changes are being validated. ``` (fail) $ rad config set web.pinned.repositories 5 -✗ Error: configuration JSON error: invalid type: integer `5`, expected a sequence +✗ Error: writing configuration to "[..]/.radicle/config.json" failed: validation failure due to invalid type: integer `5`, expected a sequence ``` The type of the operation is validated. ``` (fail) $ rad config push node.alias eve -✗ Error: the element at the path 'node.alias' is not a JSON array +✗ Error: failed to modify configuration found at "[..]/.radicle/config.json" due to the element at the path 'node.alias' is not a JSON array ``` diff --git a/crates/radicle-cli/src/commands/config.rs b/crates/radicle-cli/src/commands/config.rs index e9d8aaac..cfd32531 100644 --- a/crates/radicle-cli/src/commands/config.rs +++ b/crates/radicle-cli/src/commands/config.rs @@ -5,7 +5,7 @@ use std::str::FromStr; use anyhow::anyhow; use radicle::node::Alias; -use radicle::profile::{Config, ConfigError, ConfigPath, RawConfig}; +use radicle::profile::{config, Config, ConfigPath, RawConfig}; use crate::terminal as term; use crate::terminal::args::{Args, Error, Help}; @@ -149,33 +149,25 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> { Operation::Get(key) => { let mut temp_config = RawConfig::from_file(&path)?; let key: ConfigPath = key.into(); - let value = temp_config - .get_mut(&key) - .ok_or_else(|| ConfigError::Custom(format!("{key} does not exist")))?; + let value = temp_config.get_mut(&key).ok_or_else(|| { + anyhow::anyhow!("{key} does not exist in configuration found at {path:?}") + })?; print_value(value)?; } Operation::Set(key, value) => { - let mut temp_config = RawConfig::from_file(&path)?; - let value = temp_config.set(&key.into(), value.into())?; - temp_config.write(&path)?; + let value = modify(path, |tmp| tmp.set(&key.into(), value.into()))?; print_value(&value)?; } Operation::Push(key, value) => { - let mut temp_config = RawConfig::from_file(&path)?; - let value = temp_config.push(&key.into(), value.into())?; - temp_config.write(&path)?; + let value = modify(path, |tmp| tmp.push(&key.into(), value.into()))?; print_value(&value)?; } Operation::Remove(key, value) => { - let mut temp_config = RawConfig::from_file(&path)?; - let value = temp_config.remove(&key.into(), value.into())?; - temp_config.write(&path)?; + let value = modify(path, |tmp| tmp.remove(&key.into(), value.into()))?; print_value(&value)?; } Operation::Unset(key) => { - let mut temp_config = RawConfig::from_file(&path)?; - let value = temp_config.unset(&key.into())?; - temp_config.write(&path)?; + let value = modify(path, |tmp| tmp.unset(&key.into()))?; print_value(&value)?; } Operation::Init => { @@ -204,6 +196,20 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> { Ok(()) } +fn modify(path: P, modification: M) -> anyhow::Result +where + P: AsRef, + M: FnOnce(&mut RawConfig) -> Result, +{ + let path = path.as_ref(); + let mut temp_config = RawConfig::from_file(path)?; + let value = modification(&mut temp_config).map_err(|err| { + anyhow::anyhow!("failed to modify configuration found at {path:?} due to {err}") + })?; + temp_config.write(path)?; + Ok(value) +} + /// Print a JSON Value. fn print_value(value: &serde_json::Value) -> anyhow::Result<()> { match value { diff --git a/crates/radicle-cli/src/terminal.rs b/crates/radicle-cli/src/terminal.rs index f056bdc6..86712223 100644 --- a/crates/radicle-cli/src/terminal.rs +++ b/crates/radicle-cli/src/terminal.rs @@ -137,7 +137,7 @@ impl Context for DefaultContext { hint: "To setup your radicle profile, run `rad auth`.", } .into()), - Err(radicle::profile::Error::Config(e)) => Err(e.into()), + Err(radicle::profile::Error::LoadConfig(e)) => Err(e.into()), Err(e) => Err(anyhow::anyhow!("Could not load radicle profile: {e}")), } } diff --git a/crates/radicle/src/profile.rs b/crates/radicle/src/profile.rs index 0ca516ab..5ce92246 100644 --- a/crates/radicle/src/profile.rs +++ b/crates/radicle/src/profile.rs @@ -12,7 +12,7 @@ //! pub mod config; -pub use config::{Config, ConfigError, ConfigPath, RawConfig}; +pub use config::{Config, ConfigPath, RawConfig, WriteError}; use std::collections::{BTreeMap, BTreeSet}; use std::path::{Path, PathBuf}; @@ -163,7 +163,9 @@ pub enum Error { #[error(transparent)] Io(#[from] io::Error), #[error(transparent)] - Config(#[from] ConfigError), + InitConfig(#[from] config::InitError), + #[error(transparent)] + LoadConfig(#[from] config::LoadError), #[error(transparent)] Node(#[from] node::Error), #[error(transparent)] diff --git a/crates/radicle/src/profile/config.rs b/crates/radicle/src/profile/config.rs index 2bd439d1..75cbe970 100644 --- a/crates/radicle/src/profile/config.rs +++ b/crates/radicle/src/profile/config.rs @@ -1,5 +1,5 @@ use std::io::Write; -use std::path::Path; +use std::path::{Path, PathBuf}; use std::{fmt, fs, io}; use serde::Serialize as _; @@ -13,13 +13,117 @@ use crate::node::Alias; use crate::{cli, node, web}; #[derive(Debug, Error)] -pub enum ConfigError { - #[error("configuration I/O error: {0}")] - Io(#[from] io::Error), - #[error("configuration JSON error: {0}")] - Json(#[from] json::Error), - #[error("configuration error: {0}")] - Custom(String), +#[error("writing configuration to {path:?} failed: {kind}")] +pub struct WriteError { + path: PathBuf, + kind: WriteErrorKind, +} + +impl WriteError { + fn open_file

(path: P, err: io::Error) -> Self + where + P: AsRef, + { + Self { + path: path.as_ref().to_path_buf(), + kind: WriteErrorKind::OpenFile { err }, + } + } + + fn to_json

(path: P, err: serde_json::Error) -> Self + where + P: AsRef, + { + Self { + path: path.as_ref().to_path_buf(), + kind: WriteErrorKind::ToJson { err }, + } + } + + fn write_file

(path: P, err: io::Error) -> Self + where + P: AsRef, + { + Self { + path: path.as_ref().to_path_buf(), + kind: WriteErrorKind::WriteFile { err }, + } + } + + fn validate

(path: P, err: serde_json::Error) -> Self + where + P: AsRef, + { + Self { + path: path.as_ref().to_path_buf(), + kind: WriteErrorKind::Validate { err }, + } + } + + fn serialize_json

(path: P, err: serde_json::Error) -> Self + where + P: AsRef, + { + Self { + path: path.as_ref().to_path_buf(), + kind: WriteErrorKind::SerializeJson { err }, + } + } +} + +#[derive(Debug, Error)] +pub enum WriteErrorKind { + #[error("could not open due to {err}")] + OpenFile { + #[source] + err: io::Error, + }, + #[error("could not convert to JSON due to {err}")] + ToJson { + #[source] + err: json::Error, + }, + #[error("could not write to file due to {err}")] + WriteFile { + #[source] + err: io::Error, + }, + #[error("validation failure due to {err}")] + Validate { + #[source] + err: serde_json::Error, + }, + #[error("could not serialize due to {err}")] + SerializeJson { + #[source] + err: serde_json::Error, + }, +} + +#[derive(Debug, Error)] +pub enum InitError { + #[error("failed to initialize configuration file {path:?}: {err}")] + Write { + path: PathBuf, + #[source] + err: WriteError, + }, +} + +#[derive(Debug, Error)] +pub enum LoadError { + #[error("failed to open configuration file {path:?}: {err}, perhaps you need to initialise one `rad config init --alias `")] + File { + path: PathBuf, + #[source] + err: io::Error, + }, + #[error("failed to parse JSON of configuration file {path:?}: {err}")] + Json { + path: PathBuf, + #[source] + err: serde_json::Error, + }, } /// Local radicle configuration. @@ -59,15 +163,25 @@ impl Config { } /// Initialize a new configuration. Fails if the path already exists. - pub fn init(alias: Alias, path: &Path) -> Result { + pub fn init(alias: Alias, path: &Path) -> Result { let cfg = Config::new(alias); - cfg.write(path)?; + cfg.write(path).map_err(|err| InitError::Write { + path: path.to_path_buf(), + err, + })?; Ok(cfg) } /// Load a configuration from the given path. - pub fn load(path: &Path) -> Result { - let mut cfg: Self = json::from_reader(fs::File::open(path)?)?; + pub fn load(path: &Path) -> Result { + let file = fs::File::open(path).map_err(|err| LoadError::File { + path: path.to_path_buf(), + err, + })?; + let mut cfg: Self = json::from_reader(file).map_err(|err| LoadError::Json { + path: path.to_path_buf(), + err, + })?; // Handle deprecated policy configuration. // Nb. This will override "seedingPolicy" if set! This code should be removed after 1.0. @@ -87,15 +201,16 @@ impl Config { } /// Write configuration to disk. - pub fn write(&self, path: &Path) -> Result<(), ConfigError> { - let value = json::to_value(self)?; + pub fn write(&self, path: &Path) -> Result<(), WriteError> { + let value = json::to_value(self).map_err(|err| WriteError::to_json(path, err))?; let tmp = RawConfig(value); let file = fs::OpenOptions::new() .create_new(true) .write(true) - .open(path)?; + .open(path) + .map_err(|err| WriteError::open_file(path, err))?; - tmp.write_file(file) + tmp.write_file(path, file) } /// Get the user alias. @@ -124,9 +239,9 @@ pub enum ModifyError { impl RawConfig { /// Creates a temporary configuration, by reading a configuration file from disk. - pub fn from_file(path: &Path) -> Result { - let file = fs::File::open(path)?; - let config = json::from_reader(file)?; + pub fn from_file(path: &Path) -> Result { + let file = fs::File::open(path).map_err(|err| WriteError::open_file(path, err))?; + let config = json::from_reader(file).map_err(|err| WriteError::to_json(path, err))?; Ok(RawConfig(config)) } @@ -219,26 +334,37 @@ impl RawConfig { /// Writes the configuration, including extra values, to disk. Errors if the config is not /// valid. - pub fn write(&self, path: &Path) -> Result<(), ConfigError> { - let _valid_config: Config = self.clone().try_into()?; + pub fn write(&self, path: &Path) -> Result<(), WriteError> { + let _valid_config: Config = self + .clone() + .try_into() + .map_err(|err| WriteError::validate(path, err))?; let file = fs::OpenOptions::new() .create(true) .write(true) .truncate(true) - .open(path)?; + .open(path) + .map_err(|err| WriteError::open_file(path, err))?; - self.write_file(file) + self.write_file(path, file) } /// Write to an open file. - fn write_file(&self, mut file: fs::File) -> Result<(), ConfigError> { - let _valid_config: Config = self.clone().try_into()?; + fn write_file(&self, path: &Path, mut file: fs::File) -> Result<(), WriteError> { + let _valid_config: Config = self + .clone() + .try_into() + .map_err(|err| WriteError::validate(path, err))?; let formatter = json::ser::PrettyFormatter::with_indent(b" "); let mut serializer = json::Serializer::with_formatter(&file, formatter); - self.0.serialize(&mut serializer)?; - file.write_all(b"\n")?; - file.sync_all()?; + self.0 + .serialize(&mut serializer) + .map_err(|err| WriteError::serialize_json(path, err))?; + file.write_all(b"\n") + .map_err(|err| WriteError::write_file(path, err))?; + file.sync_all() + .map_err(|err| WriteError::write_file(path, err))?; Ok(()) } @@ -268,11 +394,11 @@ impl RawConfig { } } -impl TryInto for RawConfig { +impl TryFrom for Config { type Error = json::Error; - fn try_into(self) -> Result { - json::from_value(self.0) + fn try_from(raw: RawConfig) -> Result { + json::from_value(raw.0) } }