From 34aaa60a44ec8cff1fe9646d9d9fd976a97f9373 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C4=99drzej=20Stuczy=C5=84ski?= Date: Tue, 9 Sep 2025 14:47:44 +0100 Subject: [PATCH] feat: attempt to add more details to reqwest errors --- .../client-core/src/client/base_client/mod.rs | 17 +- .../validator-client/src/nym_api/mod.rs | 18 +- common/http-api-client/src/lib.rs | 203 +++++++++++++----- contracts/Cargo.lock | 2 +- .../src/client.rs | 28 ++- nym-wallet/Cargo.lock | 21 ++ 6 files changed, 204 insertions(+), 85 deletions(-) diff --git a/common/client-core/src/client/base_client/mod.rs b/common/client-core/src/client/base_client/mod.rs index 26aa71626d..8fba2dcb99 100644 --- a/common/client-core/src/client/base_client/mod.rs +++ b/common/client-core/src/client/base_client/mod.rs @@ -756,15 +756,8 @@ where let mut nym_api_urls = config.get_nym_api_endpoints(); nym_api_urls.shuffle(&mut thread_rng()); - let mut builder = - nym_http_api_client::Client::builder(nym_api_urls[0].clone()).map_err(|e| { - ClientCoreError::NymApiQueryFailure { - source: - nym_validator_client::nym_api::error::NymAPIError::GenericRequestFailure( - e.to_string(), - ), - } - })?; + let mut builder = nym_http_api_client::Client::builder(nym_api_urls[0].clone()) + .map_err(|source| ClientCoreError::NymApiQueryFailure { source })?; if let Some(user_agent) = user_agent { builder = builder.with_user_agent(user_agent); @@ -774,11 +767,7 @@ where builder .build() - .map_err(|e| ClientCoreError::NymApiQueryFailure { - source: nym_validator_client::nym_api::error::NymAPIError::GenericRequestFailure( - e.to_string(), - ), - }) + .map_err(|source| ClientCoreError::NymApiQueryFailure { source }) } async fn determine_key_rotation_state( diff --git a/common/client-libs/validator-client/src/nym_api/mod.rs b/common/client-libs/validator-client/src/nym_api/mod.rs index c802ed7e37..dcd77c1b18 100644 --- a/common/client-libs/validator-client/src/nym_api/mod.rs +++ b/common/client-libs/validator-client/src/nym_api/mod.rs @@ -273,9 +273,9 @@ pub trait NymApiClientExt: ApiClient { if !metadata.consistency_check(&res.metadata) { // Create a custom error for inconsistent metadata - return Err(NymAPIError::EndpointFailure { - status: reqwest::StatusCode::INTERNAL_SERVER_ERROR, - error: "Inconsistent paged metadata".to_string(), + return Err(NymAPIError::InternalResponseInconsistency { + url: self.api_url().clone(), + details: "Inconsistent paged metadata".to_string(), }); } @@ -314,9 +314,9 @@ pub trait NymApiClientExt: ApiClient { .await?; if !metadata.consistency_check(&res.metadata) { - return Err(NymAPIError::EndpointFailure { - status: reqwest::StatusCode::INTERNAL_SERVER_ERROR, - error: "Inconsistent paged metadata".to_string(), + return Err(NymAPIError::InternalResponseInconsistency { + url: self.api_url().clone(), + details: "Inconsistent paged metadata".to_string(), }); } @@ -357,9 +357,9 @@ pub trait NymApiClientExt: ApiClient { .await?; if !metadata.consistency_check(&res.metadata) { - return Err(NymAPIError::EndpointFailure { - status: reqwest::StatusCode::INTERNAL_SERVER_ERROR, - error: "Inconsistent paged metadata".to_string(), + return Err(NymAPIError::InternalResponseInconsistency { + url: self.api_url().clone(), + details: "Inconsistent paged metadata".to_string(), }); } diff --git a/common/http-api-client/src/lib.rs b/common/http-api-client/src/lib.rs index e27ea0b02a..31aa6ceb19 100644 --- a/common/http-api-client/src/lib.rs +++ b/common/http-api-client/src/lib.rs @@ -51,7 +51,7 @@ //! Up, //! } //! -//! # type Err = HttpClientError; +//! # type Err = HttpClientError; //! # async fn run() -> Result<(), Err> { //! // This will POST a body of `{"lang":"rust","body":"json"}` //! let mut map = HashMap::new(); @@ -140,6 +140,7 @@ pub use inventory; pub use reqwest; pub use reqwest::ClientBuilder as ReqwestClientBuilder; pub use reqwest::StatusCode; +use std::error::Error; pub mod registry; @@ -244,43 +245,125 @@ impl SerializationFormat { } } +#[allow(missing_docs)] +#[derive(Debug)] +pub struct ReqwestErrorWrapper(reqwest::Error); + +impl Display for ReqwestErrorWrapper { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + if self.0.is_connect() { + write!(f, "failed to connect: ")?; + } + if self.0.is_timeout() { + write!(f, "timed out: ")?; + } + if self.0.is_redirect() { + if let Some(final_stop) = self.0.url() { + write!(f, "redirect loop at {final_stop}: ")?; + } + } + + self.0.fmt(f)?; + if let Some(status_code) = self.0.status() { + write!(f, " status: {status_code}")?; + } else { + write!(f, " unknown status code")?; + } + + if let Some(source) = self.0.source() { + write!(f, " source: {source}")?; + } else { + write!(f, " unknown lower-level error source")?; + } + + Ok(()) + } +} + +impl std::error::Error for ReqwestErrorWrapper {} + /// The Errors that may occur when creating or using an HTTP client. #[derive(Debug, Error)] +#[allow(clippy::large_enum_variant)] // TODO: box relevant variants #[allow(missing_docs)] pub enum HttpClientError { - #[error("there was an issue with the REST request: {source}")] - ReqwestClientError { - #[from] + #[error("failed to construct inner reqwest client: {source}")] + ReqwestBuildError { + #[source] source: reqwest::Error, }, + #[error("failed to parse {raw} as a valid URL: {source}")] + MalformedUrl { + raw: String, + #[source] + source: reqwest::Error, + }, + + #[error("failed to send request for {url}: {source}")] + RequestSendFailure { + url: reqwest::Url, + #[source] + source: ReqwestErrorWrapper, + }, + + #[error("failed to read response body from {url}: {source}")] + ResponseReadFailure { + url: reqwest::Url, + headers: HeaderMap, + status: StatusCode, + #[source] + source: ReqwestErrorWrapper, + }, + #[error("failed to deserialize received response: {source}")] ResponseDeserialisationFailure { source: serde_json::Error }, #[error("provided url is malformed: {source}")] - MalformedUrl { + UrlParseFailure { #[from] source: url::ParseError, }, - #[error("the requested resource could not be found")] - NotFound, + #[error("the requested resource could not be found at {url}")] + NotFound { url: reqwest::Url }, - #[error("request failed with error message: {0}")] - GenericRequestFailure(String), + #[error("attempted to use domain fronting and clone a request containing stream data")] + AttemptedToCloneStreamRequest, - #[error("the request failed with status '{status}'. no additional error message provided")] - RequestFailure { status: StatusCode }, + // #[error("request failed with error message: {0}")] + // GenericRequestFailure(String), + // + #[error("the request for {url} failed with status '{status}'. no additional error message provided. response headers: {headers:?}")] + RequestFailure { + url: reqwest::Url, + status: StatusCode, + headers: HeaderMap, + }, - #[error("the returned response was empty. status: '{status}'")] - EmptyResponse { status: StatusCode }, + #[error( + "the returned response from {url} was empty. status: '{status}'. response headers: {headers:?}" + )] + EmptyResponse { + url: reqwest::Url, + status: StatusCode, + headers: HeaderMap, + }, - #[error("failed to resolve request. status: '{status}', additional error message: {error}")] - EndpointFailure { status: StatusCode, error: String }, + #[error("failed to resolve request for {url}. status: '{status}'. response headers: {headers:?}. additional error message: {error}")] + EndpointFailure { + url: reqwest::Url, + status: StatusCode, + headers: HeaderMap, + error: String, + }, #[error("failed to decode response body: {message} from {content}")] ResponseDecodeFailure { message: String, content: String }, + #[error("failed to resolve request to {url} due to data inconsistency: {details}")] + InternalResponseInconsistency { url: ::url::Url, details: String }, + #[error("Failed to encode bincode: {0}")] Bincode(#[from] bincode::Error), @@ -298,24 +381,16 @@ pub enum HttpClientError { RequestTimeout, } +#[allow(missing_docs)] impl HttpClientError { - /// Returns true if the error is a timeout. - pub fn is_timeout(&self) -> bool { - match self { - HttpClientError::ReqwestClientError { source } => source.is_timeout(), - #[cfg(target_arch = "wasm32")] - HttpClientError::RequestTimeout => true, - _ => false, - } + pub fn reqwest_client_build_error(source: reqwest::Error) -> Self { + HttpClientError::ReqwestBuildError { source } } - /// Returns the HTTP status code if available. - pub fn status_code(&self) -> Option { - match self { - HttpClientError::RequestFailure { status } => Some(*status), - HttpClientError::EmptyResponse { status } => Some(*status), - HttpClientError::EndpointFailure { status, .. } => Some(*status), - _ => None, + pub fn request_send_error(url: reqwest::Url, source: reqwest::Error) -> Self { + HttpClientError::RequestSendFailure { + url, + source: ReqwestErrorWrapper(source), } } } @@ -629,7 +704,9 @@ impl ClientBuilder { builder = builder.dns_resolver(Arc::new(HickoryDnsResolver::default())); } - builder.build()? + builder + .build() + .map_err(HttpClientError::reqwest_client_build_error)? }; let client = Client { @@ -895,14 +972,15 @@ impl ApiClientCore for Client { // try_clone may fail if the body is a stream in which case using retries is not advised. let r = request .try_clone() - .ok_or(HttpClientError::GenericRequestFailure( - "failed to send request".to_string(), - ))?; + .ok_or(HttpClientError::AttemptedToCloneStreamRequest)?; // apply any changes based on the current state of the client wrt. hosts, // fronting domains, etc. - let mut req = r.build()?; + let mut req = r + .build() + .map_err(HttpClientError::reqwest_client_build_error)?; self.apply_hosts_to_req(&mut req); + let url = req.url().clone(); #[cfg(target_arch = "wasm32")] let response: Result = { @@ -919,7 +997,7 @@ impl ApiClientCore for Client { match response { Ok(resp) => return Ok(resp), - Err(e) => { + Err(err) => { // if we have multiple urls, update to the next self.update_host(); @@ -931,21 +1009,20 @@ impl ApiClientCore for Client { front.retry_enable(); if !was_enabled && front.is_enabled() { tracing::info!( - "Domain fronting activated after connection failure: {}", - e + "Domain fronting activated after connection failure: {err}", ); } } if attempts < self.retry_limit { - warn!("Retrying request due to http error: {}", e); + warn!("Retrying request due to http error: {err}"); attempts += 1; continue; } // if we have exhausted our attempts, return the error #[allow(clippy::useless_conversion)] // conversion considered useless in wasm - return Err(e.into()); + return Err(HttpClientError::request_send_error(url, err)); } } } @@ -1301,36 +1378,52 @@ where T: DeserializeOwned, { let status = res.status(); - tracing::trace!("Status: {} (success: {})", &status, status.is_success()); + let headers = res.headers().clone(); + let url = res.url().clone(); + + tracing::trace!("status: {status} (success: {})", status.is_success()); + tracing::trace!("headers: {headers:?}"); if !allow_empty { if let Some(0) = res.content_length() { - return Err(HttpClientError::EmptyResponse { status }); + return Err(HttpClientError::EmptyResponse { + url, + status, + headers, + }); } } - let headers = res.headers().clone(); - tracing::trace!("headers: {:?}", headers); if res.status().is_success() { // internally reqwest is first retrieving bytes and then performing parsing via serde_json // (and similarly does the same thing for text()) - let full = res.bytes().await?; + let full = res + .bytes() + .await + .map_err(|source| HttpClientError::ResponseReadFailure { + url, + headers: headers.clone(), + status, + source: ReqwestErrorWrapper(source), + })?; decode_raw_response(&headers, full) } else if res.status() == StatusCode::NOT_FOUND { - Err(HttpClientError::NotFound) + Err(HttpClientError::NotFound { url }) } else { let Ok(plaintext) = res.text().await else { - return Err(HttpClientError::RequestFailure { status }); + return Err(HttpClientError::RequestFailure { + url, + status, + headers, + }); }; - if let Ok(request_error) = serde_json::from_str(&plaintext) { - Err(HttpClientError::EndpointFailure { - status, - error: request_error, - }) - } else { - Err(HttpClientError::GenericRequestFailure(plaintext)) - } + Err(HttpClientError::EndpointFailure { + url, + status, + headers, + error: plaintext, + }) } } diff --git a/contracts/Cargo.lock b/contracts/Cargo.lock index 11389591a5..67097b0840 100644 --- a/contracts/Cargo.lock +++ b/contracts/Cargo.lock @@ -744,7 +744,7 @@ version = "0.1.0" dependencies = [ "cosmwasm-std", "quote", - "syn 1.0.109", + "syn 2.0.98", ] [[package]] diff --git a/nym-credential-proxy/nym-credential-proxy-requests/src/client.rs b/nym-credential-proxy/nym-credential-proxy-requests/src/client.rs index 72f241525c..4623a7cc92 100644 --- a/nym-credential-proxy/nym-credential-proxy-requests/src/client.rs +++ b/nym-credential-proxy/nym-credential-proxy-requests/src/client.rs @@ -26,7 +26,10 @@ pub fn new_client( base_url: impl IntoUrl, bearer_token: impl Into, ) -> Result { - let url = base_url.into_url()?; + let raw = base_url.as_str().to_string(); + let url = base_url + .into_url() + .map_err(|source| VpnApiClientError::MalformedUrl { raw, source })?; Ok(VpnApiClient { inner: Client::builder(url)? .with_user_agent(format!( @@ -99,7 +102,12 @@ impl NymVpnApiClient for VpnApiClient { .inner .create_get_request(path, NO_PARAMS)? .bearer_auth(&self.bearer_token) - .send(); + .build() + .map_err(VpnApiClientError::reqwest_client_build_error)?; + + let url = req.url().clone(); + + let req = reqwest::Client::new().execute(req); // the only reason for that target lock is so that I could call this method from an ephemeral test // running in non-wasm mode (since I wanted to use tokio) @@ -110,8 +118,9 @@ impl NymVpnApiClient for VpnApiClient { .map_err(|_timeout| HttpClientError::RequestTimeout)??; #[cfg(not(target_arch = "wasm32"))] - let res = req.await?; - + let res = req + .await + .map_err(|source| VpnApiClientError::request_send_error(url, source))?; parse_response(res, false).await } @@ -131,7 +140,12 @@ impl NymVpnApiClient for VpnApiClient { .inner .create_post_request(path, params, json_body)? .bearer_auth(&self.bearer_token) - .send(); + .build() + .map_err(VpnApiClientError::reqwest_client_build_error)?; + + let url = req.url().clone(); + + let req = reqwest::Client::new().execute(req); // the only reason for that target lock is so that I could call this method from an ephemeral test // running in non-wasm mode (since I wanted to use tokio) @@ -142,7 +156,9 @@ impl NymVpnApiClient for VpnApiClient { .map_err(|_timeout| HttpClientError::RequestTimeout)??; #[cfg(not(target_arch = "wasm32"))] - let res = req.await?; + let res = req + .await + .map_err(|source| VpnApiClientError::request_send_error(url, source))?; parse_response(res, false).await } diff --git a/nym-wallet/Cargo.lock b/nym-wallet/Cargo.lock index 04c5b2f974..e2d2411920 100644 --- a/nym-wallet/Cargo.lock +++ b/nym-wallet/Cargo.lock @@ -3384,6 +3384,15 @@ dependencies = [ "generic-array", ] +[[package]] +name = "inventory" +version = "0.3.21" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bc61209c082fbeb19919bee74b176221b27223e27b65d781eb91af24eb1fb46e" +dependencies = [ + "rustversion", +] + [[package]] name = "io-uring" version = "0.7.10" @@ -4216,9 +4225,11 @@ dependencies = [ "encoding_rs", "hickory-resolver", "http 1.3.1", + "inventory", "itertools 0.14.0", "mime", "nym-bin-common", + "nym-http-api-client-macro", "nym-http-api-common", "once_cell", "reqwest 0.12.15", @@ -4232,6 +4243,16 @@ dependencies = [ "wasmtimer", ] +[[package]] +name = "nym-http-api-client-macro" +version = "0.1.0" +dependencies = [ + "proc-macro-crate 3.3.0", + "proc-macro2", + "quote", + "syn 2.0.100", +] + [[package]] name = "nym-http-api-common" version = "0.1.0"