Skip to content

Commit

Permalink
Breaking change: provide more context in error::Error
Browse files Browse the repository at this point in the history 
We'd like top provide the caller with more context
to, say, print key portions of a response in
debug mode or emit metrics.

Unfortunatley, the key reqwest Response methods
consume self and make the value largely
useless for an error that wraps it.

So let's drop the Response part of copy over
the request URL, header map and body. The first
two do not consume self.
  • Loading branch information
@michaelklishin
michaelklishin committed Dec 25, 2024
1 parent f8f6300 commit 779a58b
Show file tree
Hide file tree
Showing 3 changed files with 160 additions and 147 deletions.
169 changes: 77 additions & 92 deletions src/api.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
#![allow(clippy::result_large_err)]

use backtrace::Backtrace;
use reqwest::{
header::{HeaderMap, HeaderValue},
Expand All @@ -21,9 +23,7 @@ use serde_json::{json, Map, Value};
use std::fmt;

use crate::error::Error;
use crate::error::Error::{
ClientErrorResponse, InvalidHeaderValue, RequestError, ServerErrorResponse,
};
use crate::error::Error::{ClientErrorResponse, NotFound, ServerErrorResponse};
use crate::responses::MessageList;
use crate::{
commons::{BindingDestinationType, UserLimitTarget, VirtualHostLimitTarget},
Expand All @@ -35,54 +35,11 @@ use crate::{
responses::{self, BindingInfo, DefinitionSet},
};

type HttpClientResponse = reqwest::Response;
type HttpClientError = Error<HttpClientResponse, StatusCode, reqwest::Error, Backtrace>;
pub type HttpClientResponse = reqwest::Response;
pub type HttpClientError = crate::error::HttpClientError;

pub type Result<T> = std::result::Result<T, HttpClientError>;

impl From<reqwest::Error> for HttpClientError {
fn from(req_err: reqwest::Error) -> Self {
match req_err.status() {
None => RequestError {
error: req_err,
backtrace: Backtrace::new(),
},
Some(status_code) => {
if status_code.is_client_error() {
return ClientErrorResponse {
status_code,
// reqwest::Error does not provide access to the associated
// response, if any
response: None,
backtrace: Backtrace::new(),
};
};

if status_code.is_server_error() {
return ServerErrorResponse {
status_code,
// reqwest::Error does not provide access to the associated
// response, if any
response: None,
backtrace: Backtrace::new(),
};
};

RequestError {
error: req_err,
backtrace: Backtrace::new(),
}
}
}
}
}

impl From<reqwest::header::InvalidHeaderValue> for HttpClientError {
fn from(err: reqwest::header::InvalidHeaderValue) -> Self {
InvalidHeaderValue { error: err }
}
}

/// A `ClientBuilder` can be used to create a `Client` with custom configuration.
///
/// Example
Expand Down Expand Up @@ -1229,8 +1186,8 @@ where
async fn http_get<S>(
&self,
path: S,
client_expect_code_error: Option<StatusCode>,
server_expect_code_error: Option<StatusCode>,
client_code_to_accept_or_ignore: Option<StatusCode>,
server_code_to_accept_or_ignore: Option<StatusCode>,
) -> Result<HttpClientResponse>
where
S: AsRef<str>,
Expand All @@ -1241,20 +1198,22 @@ where
.basic_auth(&self.username, Some(&self.password))
.send()
.await?;
let response = self.ok_or_status_code_error(
response,
client_expect_code_error,
server_expect_code_error,
)?;
let response = self
.ok_or_status_code_error(
response,
client_code_to_accept_or_ignore,
server_code_to_accept_or_ignore,
)
.await?;
Ok(response)
}

async fn http_put<S, T>(
&self,
path: S,
payload: &T,
client_expect_code_error: Option<StatusCode>,
server_expect_code_error: Option<StatusCode>,
client_code_to_accept_or_ignore: Option<StatusCode>,
server_code_to_accept_or_ignore: Option<StatusCode>,
) -> Result<HttpClientResponse>
where
S: AsRef<str>,
Expand All @@ -1267,20 +1226,22 @@ where
.basic_auth(&self.username, Some(&self.password))
.send()
.await?;
let response = self.ok_or_status_code_error(
response,
client_expect_code_error,
server_expect_code_error,
)?;
let response = self
.ok_or_status_code_error(
response,
client_code_to_accept_or_ignore,
server_code_to_accept_or_ignore,
)
.await?;
Ok(response)
}

async fn http_post<S, T>(
&self,
path: S,
payload: &T,
client_expect_code_error: Option<StatusCode>,
server_expect_code_error: Option<StatusCode>,
client_code_to_accept_or_ignore: Option<StatusCode>,
server_code_to_accept_or_ignore: Option<StatusCode>,
) -> Result<HttpClientResponse>
where
S: AsRef<str>,
Expand All @@ -1293,19 +1254,21 @@ where
.basic_auth(&self.username, Some(&self.password))
.send()
.await?;
let response = self.ok_or_status_code_error(
response,
client_expect_code_error,
server_expect_code_error,
)?;
let response = self
.ok_or_status_code_error(
response,
client_code_to_accept_or_ignore,
server_code_to_accept_or_ignore,
)
.await?;
Ok(response)
}

async fn http_delete<S>(
&self,
path: S,
client_expect_code_error: Option<StatusCode>,
server_expect_code_error: Option<StatusCode>,
client_code_to_accept_or_ignore: Option<StatusCode>,
server_code_to_accept_or_ignore: Option<StatusCode>,
) -> Result<HttpClientResponse>
where
S: AsRef<str>,
Expand All @@ -1316,20 +1279,22 @@ where
.basic_auth(&self.username, Some(&self.password))
.send()
.await?;
let response = self.ok_or_status_code_error(
response,
client_expect_code_error,
server_expect_code_error,
)?;
let response = self
.ok_or_status_code_error(
response,
client_code_to_accept_or_ignore,
server_code_to_accept_or_ignore,
)
.await?;
Ok(response)
}

async fn http_delete_with_headers<S>(
&self,
path: S,
headers: HeaderMap,
client_expect_code_error: Option<StatusCode>,
server_expect_code_error: Option<StatusCode>,
client_code_to_accept_or_ignore: Option<StatusCode>,
server_code_to_accept_or_ignore: Option<StatusCode>,
) -> Result<HttpClientResponse>
where
S: AsRef<str>,
Expand All @@ -1341,43 +1306,63 @@ where
.headers(headers)
.send()
.await?;
let response = self.ok_or_status_code_error(
response,
client_expect_code_error,
server_expect_code_error,
)?;
let response = self
.ok_or_status_code_error(
response,
client_code_to_accept_or_ignore,
server_code_to_accept_or_ignore,
)
.await?;
Ok(response)
}

fn ok_or_status_code_error(
async fn ok_or_status_code_error(
&self,
response: HttpClientResponse,
client_expect_code_error: Option<StatusCode>,
server_expect_code_error: Option<StatusCode>,
client_code_to_accept_or_ignore: Option<StatusCode>,
server_code_to_accept_or_ignore: Option<StatusCode>,
) -> Result<HttpClientResponse> {
let status = response.status();
if status == StatusCode::NOT_FOUND {
return Err(NotFound);
}

if status.is_client_error() {
match client_expect_code_error {
match client_code_to_accept_or_ignore {
Some(expect) if status == expect => {}
_ => {
let url = response.url().clone();
let headers = response.headers().clone();
// this consumes `self` and makes the response largely useless to the caller,
// so we copy the key parts into the error first
let body = response.text().await?;
return Err(ClientErrorResponse {
response: Some(response),
url: Some(url),
body: Some(body),
headers: Some(headers),
status_code: status,
backtrace: Backtrace::new(),
})
});
}
}
}

if status.is_server_error() {
match server_expect_code_error {
match server_code_to_accept_or_ignore {
Some(expect) if status == expect => {}
_ => {
let url = response.url().clone();
let headers = response.headers().clone();
// this consumes `self` and makes the response largely useless to the caller,
// so we copy the key parts into the error first
let body = response.text().await?;
return Err(ServerErrorResponse {
response: Some(response),
url: Some(url),
body: Some(body),
headers: Some(headers),
status_code: status,
backtrace: Backtrace::new(),
})
});
}
}
}
Expand Down
73 changes: 22 additions & 51 deletions src/blocking_api.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
#![allow(clippy::result_large_err)]

use crate::error::Error;
use crate::error::Error::{
ClientErrorResponse, InvalidHeaderValue, NotFound, RequestError, ServerErrorResponse,
};
use crate::error::Error::{ClientErrorResponse, NotFound, ServerErrorResponse};
use crate::{
commons::{BindingDestinationType, UserLimitTarget, VirtualHostLimitTarget},
path,
Expand All @@ -35,53 +35,10 @@ use serde_json::{json, Map, Value};
use std::fmt;

pub type HttpClientResponse = reqwest::blocking::Response;
pub type HttpClientError = Error<HttpClientResponse, StatusCode, reqwest::Error, Backtrace>;
pub type HttpClientError = crate::error::HttpClientError;

pub type Result<T> = std::result::Result<T, HttpClientError>;

impl From<reqwest::Error> for HttpClientError {
fn from(req_err: reqwest::Error) -> Self {
match req_err.status() {
None => RequestError {
error: req_err,
backtrace: Backtrace::new(),
},
Some(status_code) => {
if status_code.is_client_error() {
return ClientErrorResponse {
status_code,
// reqwest::Error does not provide access to the associated
// response, if any
response: None,
backtrace: Backtrace::new(),
};
};

if status_code.is_server_error() {
return ServerErrorResponse {
status_code,
// reqwest::Error does not provide access to the associated
// response, if any
response: None,
backtrace: Backtrace::new(),
};
};

RequestError {
error: req_err,
backtrace: Backtrace::new(),
}
}
}
}
}

impl From<reqwest::header::InvalidHeaderValue> for HttpClientError {
fn from(err: reqwest::header::InvalidHeaderValue) -> Self {
InvalidHeaderValue { error: err }
}
}

/// A `ClientBuilder` can be used to create a `Client` with custom configuration.
///
/// Example
Expand Down Expand Up @@ -1220,11 +1177,18 @@ where
match client_code_to_accept_or_ignore {
Some(expect) if status == expect => {}
_ => {
let url = response.url().clone();
let headers = response.headers().clone();
// this consumes `self` and makes the response largely useless to the caller,
// so we copy the key parts into the error first
let body = response.text()?;
return Err(ClientErrorResponse {
response: Some(response),
url: Some(url),
body: Some(body),
headers: Some(headers),
status_code: status,
backtrace: Backtrace::new(),
})
});
}
}
}
Expand All @@ -1233,11 +1197,18 @@ where
match server_code_to_accept_or_ignore {
Some(expect) if status == expect => {}
_ => {
let url = response.url().clone();
let headers = response.headers().clone();
// this consumes `self` and makes the response largely useless to the caller,
// so we copy the key parts into the error first
let body = response.text()?;
return Err(ServerErrorResponse {
response: Some(response),
url: Some(url),
body: Some(body),
headers: Some(headers),
status_code: status,
backtrace: Backtrace::new(),
})
});
}
}
}
Expand Down
Loading

0 comments on commit 779a58b

Please sign in to comment.