Skip to content

Migrate screen commands to v4.1 API and improve error messages #294

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
514sid wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Migrate screen commands to v4.1 API and improve error messages #294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c1c306d
Feat: surface API error messages from response body
514sid Jun 6, 2026
1ef6872
Feat: migrate screen commands to v4.1 API
514sid Jun 6, 2026
f5a22da
Fix: apply rustfmt formatting
514sid Jun 6, 2026
37bb1de
Merge master (csv-output) into feature/v4.1-screens
514sid Jun 8, 2026
a910187
Add select query params for v4.1 screen nested data
514sid Jun 8, 2026
f6472d1
Extract api_error_from_body helper and apply it to get, delete, and p…
514sid Jun 11, 2026
8f47815
Apply rustfmt
514sid Jun 11, 2026
f316b10
Use Display instead of Debug for all user-facing error messages
514sid Jun 11, 2026
3e59dad
Add tests for api_error_from_body key variants and HTTP error paths
514sid Jun 11, 2026
e747a37
Address follow-up review: empty-body safety and shared select const
514sid Jul 23, 2026
66853bc
Fix screen select relationships: screens_ -> screen_ prefix
514sid Jul 23, 2026
fe7276d
Make add-screen test mock the real {id} response shape
514sid Jul 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 14 additions & 13 deletions src/cli.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,7 @@ pub fn handle_command_execution_result<T: Formatter>(
)
}
_ => {
error!("Error occurred: {e:?}");
error!("Error occurred: {e}");
}
}
std::process::exit(1);
Expand Down Expand Up @@ -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);
}
},
Expand Down Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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}")
}
}
}
Expand Down Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -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);
}
}
Expand All @@ -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);
}
}
Expand All @@ -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);
}
}
Expand All @@ -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);
}
}
Expand All @@ -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);
}
}
Expand Down Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -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", "*,screen_configs(*),screen_pings(*),screen_reports(*),screen_statuses(*)")
.header("user-agent", format!("screenly-cli {}", env!("CARGO_PKG_VERSION")))
.header("Authorization", "Token token");
then
Expand Down
76 changes: 66 additions & 10 deletions src/commands/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}")]
Expand Down Expand Up @@ -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::<serde_json::Value>(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,
Expand All @@ -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()?)?)
}
Expand All @@ -216,14 +230,22 @@ pub fn post<T: Serialize + ?Sized>(

// 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);
}

Ok(serde_json::from_str(&response.text()?)?)
// A 201/200 with an empty body (e.g. if the server ever stops honoring
// `Prefer: return=representation`) must not fail the call after the
// resource has already been created.
let body = response.text()?;
if body.trim().is_empty() {
return Ok(serde_json::Value::Null);
}
Ok(serde_json::from_str(&body)?)
}

pub fn delete(authentication: &Authentication, endpoint: &str) -> anyhow::Result<(), CommandError> {
Expand All @@ -233,8 +255,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(())
}
Expand All @@ -257,8 +280,9 @@ pub fn patch<T: Serialize + ?Sized>(

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 {
Expand Down Expand Up @@ -659,8 +683,40 @@ 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#"[
Expand Down
Loading
Loading