From 7417429d918673da0a4736c9b863811ca9986f67 Mon Sep 17 00:00:00 2001 From: Bryan Oltman Date: Thu, 17 Aug 2023 08:36:10 -0400 Subject: [PATCH] fix: log more friendly error message in case of no internet connection (#67) * fix: log more friendly error message in case of no internet connection * fix typo * coverage --- library/Cargo.toml | 2 + library/src/network.rs | 130 ++++++++++++++++++++++++++++++++++++++--- 2 files changed, 124 insertions(+), 8 deletions(-) diff --git a/library/Cargo.toml b/library/Cargo.toml index c09f2c4..32757e8 100644 --- a/library/Cargo.toml +++ b/library/Cargo.toml @@ -24,6 +24,8 @@ comde = { version = "0.2.3", default-features = false, features = [ ] } # For decoding the hex-encoded hashes in Patch network responses. hex = "0.4.3" +# Used to construct mock responses. +http = "0.2.9" libc = "0.2.98" # For error!(), info!(), etc macros. `print` will not show up on Android. log = "0.4.14" diff --git a/library/src/network.rs b/library/src/network.rs index d230391..b713238 100644 --- a/library/src/network.rs +++ b/library/src/network.rs @@ -1,6 +1,7 @@ // This file's job is to deal with the update_server and network side // of the updater library. +use anyhow::bail; use serde::{Deserialize, Serialize}; use std::fs::File; use std::io::Write; @@ -26,7 +27,7 @@ pub type PatchCheckRequestFn = fn(&str, PatchCheckRequest) -> anyhow::Result anyhow::Result>; pub type PatchInstallSuccessFn = fn(&str, CreatePatchInstallEventRequest) -> anyhow::Result<()>; -/// A container for network clalbacks which can be mocked out for testing. +/// A container for network callbacks which can be mocked out for testing. #[derive(Clone)] pub struct NetworkHooks { /// The function to call to send a patch check request. @@ -53,12 +54,12 @@ fn patch_check_request_throws( _url: &str, _request: PatchCheckRequest, ) -> anyhow::Result { - anyhow::bail!("please set a patch_check_request_fn"); + bail!("please set a patch_check_request_fn"); } #[cfg(test)] fn download_file_throws(_url: &str) -> anyhow::Result> { - anyhow::bail!("please set a download_file_fn"); + bail!("please set a download_file_fn"); } #[cfg(test)] @@ -66,7 +67,7 @@ pub fn patch_install_success_throws( _url: &str, _request: CreatePatchInstallEventRequest, ) -> anyhow::Result<()> { - anyhow::bail!("please set a patch_install_success_fn"); + bail!("please set a patch_install_success_fn"); } impl Default for NetworkHooks { @@ -95,29 +96,60 @@ pub fn patch_check_request_default( request: PatchCheckRequest, ) -> anyhow::Result { let client = reqwest::blocking::Client::new(); - let response = client.post(url).json(&request).send()?.json()?; + let result = client.post(url).json(&request).send(); + let response = handle_network_result(result)?.json()?; Ok(response) } #[cfg(not(test))] pub fn download_file_default(url: &str) -> anyhow::Result> { let client = reqwest::blocking::Client::new(); - let response = client.get(url).send()?; + let result = client.get(url).send(); + let response = handle_network_result(result)?; let bytes = response.bytes()?; // Patch files are small (e.g. 50kb) so this should be ok to copy into memory. Ok(bytes.to_vec()) } -#[cfg(not(test))] pub fn patch_install_success_default( url: &str, request: CreatePatchInstallEventRequest, ) -> anyhow::Result<()> { let client = reqwest::blocking::Client::new(); - let _ = client.post(url).json(&request).send()?; + let result = client.post(url).json(&request).send(); + handle_network_result(result)?; Ok(()) } +/// Handles the result of a network request, returning the response if it was +/// successful, an error if it was not, or a special error if the network +/// request failed due to a lack of internet connection. +fn handle_network_result( + result: Result, +) -> anyhow::Result { + use std::error::Error; + + match result { + Ok(response) => { + if response.status().is_success() { + return Ok(response); + } else { + bail!("Request failed with status: {}", response.status()) + } + } + Err(e) => match e.source() { + Some(source) + if source + .to_string() + .contains("failed to lookup address information") => + { + bail!("Patch check request failed due to network error. Please check your internet connection."); + } + _ => bail!(e), + }, + } +} + #[cfg(test)] /// Unit tests can call this to mock out the network calls. pub fn testing_set_network_hooks( @@ -308,6 +340,8 @@ pub fn download_to_path( mod tests { use crate::network::PatchCheckResponse; + use super::{patches_events_url, PatchInstallEvent}; + #[test] fn check_patch_request_response_deserialization() { let data = r###" @@ -380,4 +414,84 @@ mod tests { assert!(debug.contains("download_file_fn")); assert!(debug.contains("patch_install_success_fn")); } + + #[test] + fn handle_network_result_ok() { + let http_response = http::response::Builder::new() + .status(200) + .body("".to_string()) + .unwrap(); + let response = reqwest::blocking::Response::from(http_response); + + let result = super::handle_network_result(Ok(response)); + + assert!(result.is_ok()); + } + + #[test] + fn handle_network_result_http_status_not_ok() { + let http_response = http::response::Builder::new() + .status(500) + .body("".to_string()) + .unwrap(); + let response = reqwest::blocking::Response::from(http_response); + + let result = super::handle_network_result(Ok(response)); + + assert!(result.is_err()); + let err = result.err().unwrap(); + assert_eq!( + err.to_string(), + "Request failed with status: 500 Internal Server Error" + ); + } + + #[test] + fn handle_network_result_no_internet() { + let result = super::patch_install_success_default( + // Make the request to a non-existent URL, which will trigger the + // same error as a lack of internet connection. + &patches_events_url("http://asdfasdfasdfasdfasdf.asdfasdf"), + super::CreatePatchInstallEventRequest { + event: PatchInstallEvent::new( + "app_id".to_string(), + "arch".to_string(), + "client_id".to_string(), + 2, + "platform".to_string(), + "release_version".to_string(), + ), + }, + ); + + assert!(result.is_err()); + let error = result.err().unwrap(); + assert_eq!(error.to_string(), "Patch check request failed due to network error. Please check your internet connection.") + } + + #[test] + fn handle_network_result_unknown_error() { + let result = super::patch_install_success_default( + // Make the request to an incorrectly formatted URL, which will + // trigger the same error as a lack of internet connection. + &patches_events_url("asdfasdf"), + super::CreatePatchInstallEventRequest { + event: PatchInstallEvent::new( + "app_id".to_string(), + "arch".to_string(), + "client_id".to_string(), + 2, + "platform".to_string(), + "release_version".to_string(), + ), + }, + ); + + assert!(result.is_err()); + let error = result.err().unwrap(); + assert_eq!( + error.to_string(), + "builder error: relative URL without a base" + ) + } }