diff --git a/src/cli.rs b/src/cli.rs index 1509aa2..2aa7b13 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -488,7 +488,7 @@ pub fn handle_command_execution_result( ) } _ => { - error!("Error occurred: {e:?}"); + error!("Error occurred: {e}"); } } std::process::exit(1); @@ -565,7 +565,7 @@ pub fn handle_cli(cli: &Cli) { std::process::exit(1); } _ => { - error!("Error occurred: {e:?}"); + error!("Error occurred: {e}"); std::process::exit(1); } }, @@ -657,7 +657,7 @@ pub fn handle_cli_screen_command(command: &ScreenCommands, output: OutputFormat) std::process::exit(0); } Err(e) => { - error!("Error occurred: {e:?}"); + error!("Error occurred: {e}"); std::process::exit(1); } } @@ -685,7 +685,7 @@ pub fn handle_cli_playlist_command(command: &PlaylistCommands, output: OutputFor println!("Playlist deleted successfully."); } Err(e) => { - eprintln!("Error occurred when deleting playlist: {e:?}") + eprintln!("Error occurred when deleting playlist: {e}") } }, PlaylistCommands::Append { @@ -729,7 +729,7 @@ pub fn handle_cli_playlist_command(command: &PlaylistCommands, output: OutputFor println!("Playlist updated successfully."); } Err(e) => { - eprintln!("Error occurred when updating playlist: {e:?}") + eprintln!("Error occurred when updating playlist: {e}") } } } @@ -783,7 +783,7 @@ pub fn handle_cli_asset_command(command: &AssetCommands, output: OutputFormat) { std::process::exit(0); } Err(e) => { - error!("Error occurred: {e:?}"); + error!("Error occurred: {e}"); std::process::exit(1); } } @@ -818,7 +818,7 @@ pub fn handle_cli_asset_command(command: &AssetCommands, output: OutputFormat) { info!("Asset updated successfully."); } Err(e) => { - error!("Error occurred: {e:?}"); + error!("Error occurred: {e}"); std::process::exit(1); } } @@ -829,7 +829,7 @@ pub fn handle_cli_asset_command(command: &AssetCommands, output: OutputFormat) { info!("Asset updated successfully."); } Err(e) => { - error!("Error occurred: {e:?}"); + error!("Error occurred: {e}"); std::process::exit(1); } } @@ -844,7 +844,7 @@ pub fn handle_cli_asset_command(command: &AssetCommands, output: OutputFormat) { info!("Asset updated successfully."); } Err(e) => { - error!("Error occurred: {e:?}"); + error!("Error occurred: {e}"); std::process::exit(1); } } @@ -855,7 +855,7 @@ pub fn handle_cli_asset_command(command: &AssetCommands, output: OutputFormat) { info!("Asset updated successfully."); } Err(e) => { - error!("Error occurred: {e:?}"); + error!("Error occurred: {e}"); std::process::exit(1); } } @@ -869,7 +869,7 @@ pub fn handle_cli_asset_command(command: &AssetCommands, output: OutputFormat) { info!("Asset updated successfully."); } Err(e) => { - error!("Error occurred: {e:?}"); + error!("Error occurred: {e}"); std::process::exit(1); } } @@ -999,7 +999,7 @@ pub fn handle_cli_edge_app_command(command: &EdgeAppCommands, output: OutputForm std::process::exit(0); } Err(e) => { - error!("Error occurred: {e:?}"); + error!("Error occurred: {e}"); std::process::exit(1); } } @@ -1247,8 +1247,9 @@ mod tests { let mock_server = MockServer::start(); mock_server.mock(|when, then| { when.method(GET) - .path("/v4/screens") + .path("/v4.1/screens") .query_param("id", "eq.017a5104-524b-33d8-8026-9087b59e7eb5") + .query_param("select", "*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)") .header("user-agent", format!("screenly-cli {}", env!("CARGO_PKG_VERSION"))) .header("Authorization", "Token token"); then diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 8d9d0e3..d4e9f70 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -118,8 +118,10 @@ pub enum CommandError { Parse(#[from] serde_json::Error), #[error("parse error: {0}")] YamlParse(#[from] serde_yaml::Error), - #[error("unknown error: {0}")] + #[error("unexpected response status: {0}")] WrongResponseStatus(u16), + #[error("{0}")] + ApiError(String), #[error("Required field is missing in the response")] MissingField, #[error("Required file is missing in the edge app directory: {0}")] @@ -170,6 +172,17 @@ pub enum CommandError { AppNotFound(String), } +fn api_error_from_body(body: &str, status: StatusCode) -> CommandError { + if let Ok(json) = serde_json::from_str::(body) { + for key in &["error", "message", "detail"] { + if let Some(msg) = json.get(key).and_then(|v| v.as_str()) { + return CommandError::ApiError(msg.to_string()); + } + } + } + CommandError::WrongResponseStatus(status.as_u16()) +} + pub fn get( authentication: &Authentication, endpoint: &str, @@ -189,8 +202,9 @@ pub fn get( debug!("GET {url} -> {status}"); if status != StatusCode::OK { - println!("Response: {:?}", &response.text()); - return Err(CommandError::WrongResponseStatus(status.as_u16())); + let body = response.text().unwrap_or_default(); + debug!("Response: {:?}", &body); + return Err(api_error_from_body(&body, status)); } Ok(serde_json::from_str(&response.text()?)?) } @@ -216,8 +230,9 @@ pub fn post( // Ok, No_Content are acceptable because some of our RPC code returns that. if ![StatusCode::CREATED, StatusCode::OK, StatusCode::NO_CONTENT].contains(&status) { - debug!("Response: {:?}", &response.text()?); - return Err(CommandError::WrongResponseStatus(status.as_u16())); + let body = response.text().unwrap_or_default(); + debug!("Response: {:?}", &body); + return Err(api_error_from_body(&body, status)); } if status == StatusCode::NO_CONTENT { return Ok(serde_json::Value::Null); @@ -233,8 +248,9 @@ pub fn delete(authentication: &Authentication, endpoint: &str) -> anyhow::Result let status = response.status(); if ![StatusCode::OK, StatusCode::NO_CONTENT].contains(&status) { - debug!("Response: {:?}", &response.text()?); - return Err(CommandError::WrongResponseStatus(status.as_u16())); + let body = response.text().unwrap_or_default(); + debug!("Response: {:?}", &body); + return Err(api_error_from_body(&body, status)); } Ok(()) } @@ -257,8 +273,9 @@ pub fn patch( let status = response.status(); if status != StatusCode::OK { - debug!("Response: {:?}", &response.text()?); - return Err(CommandError::WrongResponseStatus(status.as_u16())); + let body = response.text().unwrap_or_default(); + debug!("Response: {:?}", &body); + return Err(api_error_from_body(&body, status)); } if status == StatusCode::NO_CONTENT { @@ -659,8 +676,41 @@ impl Formatter for PlaylistItems { #[cfg(test)] mod tests { + use reqwest::StatusCode; + use super::*; + #[test] + fn test_api_error_from_body_uses_error_key() { + let err = api_error_from_body(r#"{"error": "Invalid pin"}"#, StatusCode::BAD_REQUEST); + assert_eq!(err.to_string(), "Invalid pin"); + } + + #[test] + fn test_api_error_from_body_uses_message_key() { + let err = api_error_from_body(r#"{"message": "Not found"}"#, StatusCode::NOT_FOUND); + assert_eq!(err.to_string(), "Not found"); + } + + #[test] + fn test_api_error_from_body_uses_detail_key() { + let err = api_error_from_body(r#"{"detail": "Permission denied"}"#, StatusCode::FORBIDDEN); + assert_eq!(err.to_string(), "Permission denied"); + } + + #[test] + fn test_api_error_from_body_falls_back_to_status_on_unknown_key() { + let err = + api_error_from_body(r#"{"code": "ERR123"}"#, StatusCode::INTERNAL_SERVER_ERROR); + assert_eq!(err.to_string(), "unexpected response status: 500"); + } + + #[test] + fn test_api_error_from_body_falls_back_to_status_on_non_json() { + let err = api_error_from_body("not json", StatusCode::BAD_GATEWAY); + assert_eq!(err.to_string(), "unexpected response status: 502"); + } + #[test] fn test_assets_csv_round_trip() { let data = r#"[ diff --git a/src/commands/screen.rs b/src/commands/screen.rs index 15f32c2..54ef191 100644 --- a/src/commands/screen.rs +++ b/src/commands/screen.rs @@ -1,7 +1,5 @@ use std::collections::HashMap; -use reqwest::StatusCode; - use crate::authentication::Authentication; use crate::commands; use crate::commands::{CommandError, Screens}; @@ -18,12 +16,12 @@ impl ScreenCommand { pub fn list(&self) -> anyhow::Result { Ok(Screens::new(commands::get( &self.authentication, - "v4/screens", + "v4.1/screens?select=*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)", )?)) } pub fn get(&self, id: &str) -> anyhow::Result { - let endpoint = format!("v4/screens?id=eq.{id}"); + let endpoint = format!("v4.1/screens?id=eq.{id}&select=*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)"); Ok(Screens::new(commands::get( &self.authentication, @@ -36,32 +34,20 @@ impl ScreenCommand { pin: &str, maybe_name: Option, ) -> anyhow::Result { - let url = format!("{}/v3/screens/", &self.authentication.config.url); let mut payload = HashMap::new(); payload.insert("pin".to_string(), pin.to_string()); if let Some(name) = maybe_name { payload.insert("name".to_string(), name); } - let response = self - .authentication - .build_client()? - .post(url) - .json(&payload) - .send()?; - if response.status() != StatusCode::CREATED { - return Err(CommandError::WrongResponseStatus( - response.status().as_u16(), - )); - } - - // Our newer endpoints all return arrays so let's just convert the output from v3 to be the same - let mut array: Vec = Vec::new(); - array.insert(0, serde_json::from_str(&response.text()?)?); - Ok(Screens::new(serde_json::Value::Array(array))) + Ok(Screens::new(commands::post( + &self.authentication, + "v4.1/screens", + &payload, + )?)) } pub fn delete(&self, id: &str) -> anyhow::Result<(), CommandError> { - let endpoint = format!("v3/screens/{id}/"); + let endpoint = format!("v4.1/screens?id=eq.{id}"); commands::delete(&self.authentication, &endpoint) } } @@ -86,7 +72,11 @@ mod tests { let mock_server = MockServer::start(); mock_server.mock(|when, then| { when.method(GET) - .path("/v4/screens") + .path("/v4.1/screens") + .query_param( + "select", + "*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)", + ) .header("Authorization", "Token token") .header( "user-agent", @@ -105,10 +95,11 @@ mod tests { #[test] fn test_add_screen_should_send_correct_request() { let new_screen = serde_json::from_str::("{\"id\":\"017a5104-524b-33d8-8026-9087b59e7eb5\",\"team_id\":\"016343c2-82b8-0000-a121-e30f1035875e\",\"created_at\":\"2021-06-28T05:07:55+00:00\",\"name\":\"Test\",\"is_enabled\":true,\"coords\":[55.22931, 48.90429],\"last_ping\":\"2021-08-25T06:17:20.728+00:00\",\"last_ip\":null,\"local_ip\":\"192.168.1.146\",\"mac\":\"b8:27:eb:d6:83:6f\",\"last_screenshot_time\":\"2021-08-25T06:09:04.399+00:00\",\"uptime\":\"230728.38\",\"load_avg\":\"0.14\",\"signal_strength\":null,\"interface\":\"eth0\",\"debug\":false,\"location\":\"Kamsko-Ust'inskiy rayon, Russia\",\"team\":\"016343c2-82b8-0000-a121-e30f1035875e\",\"timezone\":\"Europe/Moscow\",\"type\":\"hardware\",\"hostname\":\"srly-4shnfrdc5cd2p0p\",\"ws_open\":false,\"status\":\"Offline\",\"last_screenshot\":\"https://us-assets.screenlyapp.com/01CD1W50NR000A28F31W83B1TY/screenshots/01F98G8MJB6FC809MGGYTSWZNN/5267668e6db35498e61b83d4c702dbe8\",\"in_sync\":false,\"software_version\":\"Screenly 2 Player\",\"hardware_version\":\"Raspberry Pi 3B\",\"config\":{\"hdmi_mode\": 34, \"hdmi_boost\": 2, \"hdmi_drive\": 0, \"hdmi_group\": 0, \"verify_ssl\": true, \"audio_output\": \"hdmi\", \"hdmi_timings\": \"\", \"overscan_top\": 0, \"overscan_left\": 0, \"use_composite\": false, \"display_rotate\": 0, \"overscan_right\": 0, \"overscan_scale\": 0, \"overscan_bottom\": 0, \"disable_overscan\": 0, \"shuffle_playlist\": false, \"framebuffer_width\": 0, \"use_composite_pal\": false, \"framebuffer_height\": 0, \"hdmi_force_hotplug\": true, \"use_composite_ntsc\": false, \"hdmi_pixel_encoding\": 0, \"play_history_enabled\": false}}").unwrap(); + let new_screen_array = serde_json::Value::Array(vec![new_screen.clone()]); let mock_server = MockServer::start(); let post_mock = mock_server.mock(|when, then| { when.method(POST) - .path("/v3/screens/") + .path("/v4.1/screens") .header("Authorization", "Token token") .header("content-type", "application/json") .header( @@ -116,7 +107,7 @@ mod tests { format!("screenly-cli {}", env!("CARGO_PKG_VERSION")), ) .json_body(json!({"pin": "test-pin", "name": "test"})); - then.status(201).json_body(new_screen.clone()); + then.status(201).json_body(new_screen_array.clone()); }); let config = Config::new(mock_server.base_url()); @@ -125,7 +116,58 @@ mod tests { let v = screen_command.add("test-pin", Some("test".to_string())); post_mock.assert(); assert!(v.is_ok()); - assert_eq!(v.unwrap().value.as_array().unwrap()[0], new_screen); + assert_eq!(v.unwrap().value, new_screen_array); + } + + #[test] + fn test_add_screen_wrong_pin_should_fail_with_api_error_message() { + let mock_server = MockServer::start(); + mock_server.mock(|when, then| { + when.method(POST).path("/v4.1/screens"); + then.status(400) + .json_body(json!({"code": "P0001", "error": "Invalid pin"})); + }); + + let config = Config::new(mock_server.base_url()); + let authentication = Authentication::new_with_config(config, "token"); + let screen_command = ScreenCommand::new(authentication); + let result = screen_command.add("wrong-pin", None); + assert!(result.is_err()); + assert_eq!(result.unwrap_err().to_string(), "Invalid pin"); + } + + #[test] + fn test_get_screen_error_should_return_api_error_message() { + let mock_server = MockServer::start(); + mock_server.mock(|when, then| { + when.method(GET).path("/v4.1/screens"); + then.status(404) + .json_body(json!({"message": "Screen not found"})); + }); + + let config = Config::new(mock_server.base_url()); + let authentication = Authentication::new_with_config(config, "token"); + let screen_command = ScreenCommand::new(authentication); + let result = screen_command.get("nonexistent-id"); + assert!(result.is_err()); + assert_eq!(result.unwrap_err().to_string(), "Screen not found"); + } + + #[test] + fn test_delete_screen_error_should_return_api_error_message() { + let mock_server = MockServer::start(); + mock_server.mock(|when, then| { + when.method(DELETE).path("/v4.1/screens"); + then.status(403) + .json_body(json!({"detail": "Permission denied"})); + }); + + let config = Config::new(mock_server.base_url()); + let authentication = Authentication::new_with_config(config, "token"); + let screen_command = ScreenCommand::new(authentication); + let result = screen_command.delete("test-id"); + assert!(result.is_err()); + assert_eq!(result.unwrap_err().to_string(), "Permission denied"); } #[test] @@ -133,8 +175,9 @@ mod tests { let mock_server = MockServer::start(); mock_server.mock(|when, then| { when.method(GET) - .path("/v4/screens") + .path("/v4.1/screens") .query_param("id", "eq.017a5104-524b-33d8-8026-9087b59e7eb5") + .query_param("select", "*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)") .header("user-agent", format!("screenly-cli {}", env!("CARGO_PKG_VERSION"))) .header("Authorization", "Token token"); then @@ -155,9 +198,10 @@ mod tests { #[test] fn test_delete_screen_should_send_correct_request() { let mock_server = MockServer::start(); - mock_server.mock(|when, then| { + let delete_mock = mock_server.mock(|when, then| { when.method(DELETE) - .path("/v3/screens/test-id/") + .path("/v4.1/screens") + .query_param("id", "eq.test-id") .header( "user-agent", format!("screenly-cli {}", env!("CARGO_PKG_VERSION")), @@ -169,7 +213,9 @@ mod tests { let config = Config::new(mock_server.base_url()); let authentication = Authentication::new_with_config(config, "token"); let screen_command = ScreenCommand::new(authentication); - assert!(screen_command.delete("test-id").is_ok()); + let result = screen_command.delete("test-id"); + delete_mock.assert(); + assert!(result.is_ok()); } #[test] diff --git a/src/mcp/tests.rs b/src/mcp/tests.rs index 86a4a0b..96690f3 100644 --- a/src/mcp/tests.rs +++ b/src/mcp/tests.rs @@ -26,7 +26,11 @@ fn test_screen_list() { let mock_server = MockServer::start(); mock_server.mock(|when, then| { when.method(GET) - .path("/v4/screens") + .path("/v4.1/screens") + .query_param( + "select", + "*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)", + ) .header("Authorization", "Token test_token"); then.status(200) .json_body(json!([{"id": "screen-1", "name": "Test Screen"}])); @@ -45,8 +49,12 @@ fn test_screen_get() { let mock_server = MockServer::start(); mock_server.mock(|when, then| { when.method(GET) - .path("/v4/screens") + .path("/v4.1/screens") .query_param("id", "eq.screen-uuid") + .query_param( + "select", + "*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)", + ) .header("Authorization", "Token test_token"); then.status(200) .json_body(json!([{"id": "screen-uuid", "name": "My Screen"}])); diff --git a/src/mcp/tools/screen.rs b/src/mcp/tools/screen.rs index 534b5ac..19f4fc2 100644 --- a/src/mcp/tools/screen.rs +++ b/src/mcp/tools/screen.rs @@ -9,7 +9,7 @@ pub struct ScreenTools; impl ScreenTools { /// List all screens. pub fn list(auth: &Authentication) -> Result { - let result = commands::get(auth, "v4/screens") + let result = commands::get(auth, "v4.1/screens?select=*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)") .map_err(|e| format!("Failed to list screens: {}", e))?; serde_json::to_string_pretty(&result) @@ -18,7 +18,7 @@ impl ScreenTools { /// Get a screen by UUID. pub fn get(auth: &Authentication, uuid: &str) -> Result { - let endpoint = format!("v4/screens?id=eq.{}", uuid); + let endpoint = format!("v4.1/screens?id=eq.{}&select=*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)", uuid); let result = commands::get(auth, &endpoint).map_err(|e| format!("Failed to get screen: {}", e))?;