From 11524bb232822fa6250913829c94cb02f4375053 Mon Sep 17 00:00:00 2001 From: Alex Clarke Date: Wed, 30 Sep 2026 22:02:46 -0600 Subject: [PATCH] feat: all CLI-based errors now come out in the same JSON format to further improve scripting and LLM integration --- src/network/mod.rs | 36 +++++- src/network/network_tests.rs | 104 +++++++++++++++- src/utils.rs | 47 +++++++- src/utils_tests.rs | 124 +++++++++++++++++++- tests/servarr_name_cli_integration_tests.rs | 51 ++++++++ 5 files changed, 350 insertions(+), 12 deletions(-) diff --git a/src/network/mod.rs b/src/network/mod.rs index 8cf353ca..ba29037a 100644 --- a/src/network/mod.rs +++ b/src/network/mod.rs @@ -1,4 +1,5 @@ -use std::fmt::Debug; +use std::error::Error; +use std::fmt::{self, Debug}; use std::sync::{Arc, OnceLock}; use anyhow::{Result, anyhow}; @@ -156,8 +157,22 @@ impl<'a, 'b> Network<'a, 'b> { let error_body = format_error_body(&response_body); error!("Request failed. Received {status} response code with body: {response_body}"); - self.app.lock().await.handle_error(anyhow!("Request failed. Received {status} response code with body: {error_body}")); - Err(anyhow!("Request failed. Received {status} response code with body: {error_body}")) + let message = format!("Request failed. Received {status} response code with body: {error_body}"); + let body = if response_body.is_empty() { + None + } else { + Some( + serde_json::from_str::(&response_body) + .unwrap_or(Value::String(response_body)), + ) + }; + let network_error = NetworkError { + message, + status: status.as_u16(), + body, + }; + self.app.lock().await.handle_error(network_error.clone().into()); + Err(network_error.into()) } } Err(e) => { @@ -304,6 +319,21 @@ fn format_error_body(response_body: &str) -> String { re.replace_all(&body, " ").to_string() } +#[derive(Clone, Debug)] +pub struct NetworkError { + pub message: String, + pub status: u16, + pub body: Option, +} + +impl fmt::Display for NetworkError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.message) + } +} + +impl Error for NetworkError {} + #[derive(Clone, Copy, Debug, Display, PartialEq, Eq)] pub enum RequestMethod { Get, diff --git a/src/network/network_tests.rs b/src/network/network_tests.rs index 98f31151..2e0ad701 100644 --- a/src/network/network_tests.rs +++ b/src/network/network_tests.rs @@ -10,7 +10,7 @@ mod tests { use reqwest::header::HeaderMap; use rstest::rstest; use serde::{Deserialize, Serialize}; - use serde_json::json; + use serde_json::{Value, json}; use tokio::sync::{Mutex, mpsc}; use tokio_util::sync::CancellationToken; @@ -22,7 +22,9 @@ mod tests { use crate::network::network_tests::test_utils::test_network; use crate::network::radarr_network::RadarrEvent; use crate::network::sonarr_network::SonarrEvent; - use crate::network::{Network, NetworkEvent, NetworkTrait, RequestMethod, RequestProps}; + use crate::network::{ + Network, NetworkError, NetworkEvent, NetworkTrait, RequestMethod, RequestProps, + }; #[tokio::test] async fn test_handle_network_event_radarr_event() { @@ -398,6 +400,104 @@ mod tests { ); } + #[tokio::test] + async fn test_handle_request_non_success_code_downcasts_to_network_error_with_json_body() { + let (async_server, app_arc, server) = mock_api(RequestMethod::Get, 404, true).await; + let mut network = test_network(&app_arc); + + let resp = network + .handle_request::<(), Test>( + RequestProps { + uri: format!("{}/test", server.url()), + method: RequestMethod::Get, + body: None, + api_token: "test1234".to_owned(), + ignore_status_code: false, + custom_headers: HeaderMap::new(), + }, + |_, _| (), + ) + .await; + + async_server.assert_async().await; + let network_error = resp.unwrap_err().downcast::().unwrap(); + assert_str_eq!( + network_error.message, + r#"Request failed. Received 404 Not Found response code with body: { "value": "Test" }"# + ); + assert_eq!(network_error.status, 404); + assert_some_eq_x!(network_error.body.as_ref(), &json!({ "value": "Test" })); + } + + #[tokio::test] + async fn test_handle_request_non_success_code_downcasts_to_network_error_with_string_body() { + let mut server = Server::new_async().await; + let async_server = server + .mock("GET", "/test") + .match_header("X-Api-Key", "test1234") + .with_status(503) + .with_body("Service Unavailable") + .create_async() + .await; + let app_arc = Arc::new(Mutex::new(App::test_default())); + let mut network = test_network(&app_arc); + + let resp = network + .handle_request::<(), Test>( + RequestProps { + uri: format!("{}/test", server.url()), + method: RequestMethod::Get, + body: None, + api_token: "test1234".to_owned(), + ignore_status_code: false, + custom_headers: HeaderMap::new(), + }, + |_, _| (), + ) + .await; + + async_server.assert_async().await; + let network_error = resp.unwrap_err().downcast::().unwrap(); + assert_str_eq!( + network_error.message, + "Request failed. Received 503 Service Unavailable response code with body: Service Unavailable" + ); + assert_eq!(network_error.status, 503); + assert_some_eq_x!( + network_error.body.as_ref(), + &Value::String("Service Unavailable".to_owned()) + ); + } + + #[tokio::test] + async fn test_handle_request_non_success_code_downcasts_to_network_error_with_empty_body() { + let (async_server, app_arc, server) = mock_api(RequestMethod::Post, 404, false).await; + let mut network = test_network(&app_arc); + + let resp = network + .handle_request::<(), Test>( + RequestProps { + uri: format!("{}/test", server.url()), + method: RequestMethod::Post, + body: None, + api_token: "test1234".to_owned(), + ignore_status_code: false, + custom_headers: HeaderMap::new(), + }, + |_, _| (), + ) + .await; + + async_server.assert_async().await; + let network_error = resp.unwrap_err().downcast::().unwrap(); + assert_str_eq!( + network_error.message, + r#"Request failed. Received 404 Not Found response code with body: "# + ); + assert_eq!(network_error.status, 404); + assert_none!(network_error.body); + } + #[rstest] #[tokio::test] async fn test_call_api( diff --git a/src/utils.rs b/src/utils.rs index 5c2cf0c4..43519f65 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -6,7 +6,7 @@ use std::sync::Arc; use std::time::Duration; use anyhow::Result; -use anyhow::{Context, anyhow}; +use anyhow::{Context, Error, anyhow}; use colored::Colorize; use indicatif::{ProgressBar, ProgressStyle}; use log::{LevelFilter, error}; @@ -18,13 +18,15 @@ use log4rs::config::{Appender, Root}; use log4rs::encode::pattern::PatternEncoder; use regex::Regex; use reqwest::{Certificate, Client}; +use serde::Serialize; +use serde_json::Value; use tokio::sync::Mutex; use tokio_util::sync::CancellationToken; use crate::app::{App, AppConfig, log_and_print_error}; use crate::cli::{self, Command}; use crate::models::Route; -use crate::network::Network; +use crate::network::{Network, NetworkError}; use crate::ui::theme::ThemeDefinitionsWrapper; #[cfg(test)] @@ -282,6 +284,36 @@ pub(super) fn render_spinner() -> ProgressBar { pb } +#[derive(Serialize)] +struct CliError<'a> { + error: CliErrorDetails<'a>, +} + +#[derive(Serialize)] +struct CliErrorDetails<'a> { + message: String, + status: Option, + body: Option<&'a Value>, +} + +fn format_cli_error(error: &Error) -> String { + let details = match error.downcast_ref::() { + Some(network_error) => CliErrorDetails { + message: network_error.message.clone(), + status: Some(network_error.status), + body: network_error.body.as_ref(), + }, + None => CliErrorDetails { + message: error.to_string(), + status: None, + body: None, + }, + }; + + serde_json::to_string_pretty(&CliError { error: details }) + .expect("CLI error details are infallibly serializable") +} + pub(super) async fn start_cli_with_spinner( config: AppConfig, reqwest_client: Client, @@ -295,7 +327,8 @@ pub(super) async fn start_cli_with_spinner( let mut app = app.lock().await; app.cli_mode = true; select_cli_configuration(&mut app, &config, &command, servarr_name).unwrap_or_else(|error| { - log_and_print_error(error.to_string()); + error!("{error}"); + eprintln!("{}", format_cli_error(&error)); process::exit(1); }); } @@ -309,7 +342,7 @@ pub(super) async fn start_cli_with_spinner( } Err(e) => { pb.finish(); - eprintln!("error: {}", e.to_string().red()); + eprintln!("{}", format_cli_error(&e)); process::exit(1); } } @@ -328,7 +361,8 @@ pub(super) async fn start_cli_no_spinner( let mut app = app.lock().await; app.cli_mode = true; select_cli_configuration(&mut app, &config, &command, servarr_name).unwrap_or_else(|error| { - log_and_print_error(error.to_string()); + error!("{error}"); + eprintln!("{}", format_cli_error(&error)); process::exit(1); }); } @@ -339,7 +373,7 @@ pub(super) async fn start_cli_no_spinner( println!("{output}"); } Err(e) => { - eprintln!("error: {}", e.to_string().red()); + eprintln!("{}", format_cli_error(&e)); process::exit(1); } } @@ -360,6 +394,7 @@ pub fn select_cli_configuration( (Route::Radarr(..), Command::Radarr(_)) | (Route::Sonarr(..), Command::Sonarr(_)) | (Route::Lidarr(..), Command::Lidarr(_)) + | (Route::Readarr(..), Command::Readarr(_)) ) }); app.server_tabs.index = selected_index.ok_or_else(|| { diff --git a/src/utils_tests.rs b/src/utils_tests.rs index cad55608..07336545 100644 --- a/src/utils_tests.rs +++ b/src/utils_tests.rs @@ -1,6 +1,8 @@ #[cfg(test)] mod tests { + use anyhow::{Error, anyhow}; use clap::Parser; + use serde_json::json; use std::fs::{self, File}; use std::io::{BufRead, BufReader, Seek, SeekFrom, Write}; @@ -11,7 +13,10 @@ mod tests { use crate::{ Cli, app::{App, AppConfig, ServarrConfig}, - utils::{convert_runtime, format_size, select_cli_configuration, was_log_rotated}, + network::NetworkError, + utils::{ + convert_runtime, format_cli_error, format_size, select_cli_configuration, was_log_rotated, + }, }; #[test] @@ -64,6 +69,123 @@ mod tests { assert_eq!(minutes, 34); } + #[test] + fn test_format_cli_error_network_error_with_json_body() { + let error = Error::new(NetworkError { + message: "Request failed. Received 404 Not Found response code with body: Not Found" + .to_owned(), + status: 404, + body: Some(json!({ "status": 404, "title": "Not Found" })), + }); + + let output = format_cli_error(&error); + + assert_str_eq!( + output, + r#"{ + "error": { + "message": "Request failed. Received 404 Not Found response code with body: Not Found", + "status": 404, + "body": { + "status": 404, + "title": "Not Found" + } + } +}"# + ); + } + + #[test] + fn test_format_cli_error_network_error_with_string_body() { + let error = Error::new(NetworkError { + message: + "Request failed. Received 503 Service Unavailable response code with body: Service Unavailable" + .to_owned(), + status: 503, + body: Some(json!("Service Unavailable")), + }); + + let output = format_cli_error(&error); + + assert_str_eq!( + output, + r#"{ + "error": { + "message": "Request failed. Received 503 Service Unavailable response code with body: Service Unavailable", + "status": 503, + "body": "Service Unavailable" + } +}"# + ); + } + + #[test] + fn test_format_cli_error_network_error_with_null_body() { + let error = Error::new(NetworkError { + message: "Request failed. Received 404 Not Found response code with body: ".to_owned(), + status: 404, + body: None, + }); + + let output = format_cli_error(&error); + + assert_str_eq!( + output, + r#"{ + "error": { + "message": "Request failed. Received 404 Not Found response code with body: ", + "status": 404, + "body": null + } +}"# + ); + } + + #[test] + fn test_format_cli_error_network_error_wrapped_in_context() { + let error = Error::new(NetworkError { + message: "Request failed. Received 404 Not Found response code with body: Not Found" + .to_owned(), + status: 404, + body: Some(json!({ "status": 404, "title": "Not Found" })), + }) + .context("failed to fetch system status"); + + let output = format_cli_error(&error); + + assert_str_eq!( + output, + r#"{ + "error": { + "message": "Request failed. Received 404 Not Found response code with body: Not Found", + "status": 404, + "body": { + "status": 404, + "title": "Not Found" + } + } +}"# + ); + } + + #[test] + fn test_format_cli_error_plain_anyhow_error() { + let error = anyhow!("Failed to send request. connection refused "); + + let output = format_cli_error(&error); + + assert_str_eq!( + output, + r#"{ + "error": { + "message": "Failed to send request. connection refused ", + "status": null, + "body": null + } +}"# + ); + } + #[test] fn test_was_log_rotated_returns_false_when_file_has_not_rotated() { let path = std::env::temp_dir().join("managarr_test_no_rotation.log"); diff --git a/tests/servarr_name_cli_integration_tests.rs b/tests/servarr_name_cli_integration_tests.rs index a27e0c08..99e4ec98 100644 --- a/tests/servarr_name_cli_integration_tests.rs +++ b/tests/servarr_name_cli_integration_tests.rs @@ -86,6 +86,57 @@ fn named_instance_of_different_servarr_type_rejects_command() { sonarr_mock.assert(); } +#[test] +fn named_readarr_instance_selected_for_readarr_command() { + let mut radarr_server = Server::new(); + let radarr_mock = radarr_server + .mock("GET", "/api/v3/system/status") + .match_header("X-Api-Key", "test-token") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"version":"wrong-instance","startTime":"2024-01-01T00:00:00Z"}"#) + .expect(0) + .create(); + let mut readarr_server = Server::new(); + let readarr_mock = readarr_server + .mock("GET", "/api/v1/system/status") + .match_header("X-Api-Key", "test-token") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"version":"expected-instance","startTime":"2024-01-01T00:00:00Z"}"#) + .expect(1) + .create(); + let config = TemporaryConfig::new(&format!( + "radarr:\n - name: Servarr\n uri: {}\n api_token: test-token\nreadarr:\n - name: Servarr\n uri: {}\n api_token: test-token\n", + radarr_server.url(), + readarr_server.url() + )); + + let mut command = cargo_bin_cmd!("managarr"); + let assertion = command + .arg("--config-file") + .arg(config.path()) + .args([ + "--disable-spinner", + "--servarr-name", + "Servarr", + "readarr", + "get", + "system-status", + ]) + .assert() + .success(); + let stdout = str::from_utf8(&assertion.get_output().stdout).unwrap(); + + assert!( + stdout.contains("\"version\": \"expected-instance\""), + "unexpected CLI output: {stdout}" + ); + assert!(!stdout.contains("wrong-instance")); + readarr_mock.assert(); + radarr_mock.assert(); +} + #[test] fn same_named_servarrs_select_command_matching_instance() { let mut radarr_server = Server::new();