Improve error handling in engine_api client using thiserror. Fixes #326 #338
Conversation
There was a problem hiding this comment.
Hey, thank you for your PR! 👋
In general, it's fine. I pointed out some small things that would be great if you could address before merging. Let me know if anything is unclear or if you need a hand with it—happy to help!
Thanks again for your contribution! 😊
crates/engine-api/src/client.rs
Outdated
| @@ -1,6 +1,6 @@ | |||
| use std::{ffi::CString, os::unix::prelude::FileTypeExt}; | |||
|
|
|||
| use anyhow::{Context, Result, anyhow, bail, ensure}; | |||
| use anyhow::Result; | |||
There was a problem hiding this comment.
maybe this can be safely removed
crates/engine-api/src/client.rs
Outdated
| } | ||
| std::io::ErrorKind::PermissionDenied => { | ||
| bail!("No write permission on '{socket}'") | ||
| return Err(EngineClientError::NoWritePermission(socket)); |
There was a problem hiding this comment.
this is not a write permission error, but is a read permission error.
this wrong error was already here before your pr, but now is a good time to fix it
crates/engine-api/src/client.rs
Outdated
| let req = Request::builder() | ||
| .method(Method::GET) | ||
| .uri(uri) | ||
| .body(Either::Right(Empty::<Bytes>::new())) | ||
| .map_err(|err| anyhow!("Error building the request. Reason: {}", err))?; | ||
| .map_err(EngineClientError::HttpError)?; |
There was a problem hiding this comment.
Probably HttpError is not a good error here, it seems more a request builder error
crates/engine-api/src/client.rs
Outdated
|
|
||
| let buf = res.collect().await?.aggregate(); | ||
| let buf = res |
There was a problem hiding this comment.
I noticed that before parsing the json there is no check on the status like
let status = res.status();
match status {
StatusCode::OK => Ok(()),
_ => {
// ...
}same as before now is a good time to fix it
crates/engine-api/src/client.rs
Outdated
|
|
||
| let res = self | ||
| .client | ||
| .request(req) | ||
| .await | ||
| .map_err(|err| anyhow!("Error during the http request: reason {}", err))?; | ||
| .map_err(|err| EngineClientError::HyperRequest(err.to_string()))?; |
There was a problem hiding this comment.
probably is better an enum variant with a #[from] for "hyper errors" (I don't remember the exact type) instead of manually crafting a string
crates/engine-api/src/client.rs
Outdated
| let (ws_stream, _) = client_async("ws://localhost/monitor", stream) | ||
| .await | ||
| .map_err(|err| { | ||
| EngineClientError::WebSocketError(format!("WebSocket initialization failed: {}", err)) | ||
| })?; |
There was a problem hiding this comment.
here probably we should use a variant with #[from] tokio_tungstenite::tungstenite::Error.
you can move the unused WebsocketError::ConnectionError from WebsocketError to EngineClientErrot
|
i hope i did requested changes well :p |
| impl EngineClientError { | ||
| pub fn request_builder_error(err: http::Error) -> Self { | ||
| Self::RequestBuilderError(err) | ||
| } | ||
|
|
||
| pub fn hyper_request_error<E: std::fmt::Display>(err: E) -> Self { | ||
| Self::HyperRequestError(err.to_string()) | ||
| } | ||
| } |
There was a problem hiding this comment.
we don't need this functions
| /// Connection error | ||
| #[error("WebSocket connection error: {0}")] | ||
| ConnectionError(String), |
There was a problem hiding this comment.
this is not used anymore right? you moved to EngineClientError
There was a problem hiding this comment.
it is used in line 260 client.rs events_stream when I delete it from errors it leads to more problems in event stream error handling
| impl From<io::Error> for EngineClientError { | ||
| fn from(err: io::Error) -> Self { | ||
| Self::IoError(err.to_string()) | ||
| } | ||
| } |
There was a problem hiding this comment.
this can be implemented as a #from with thiserror
| impl From<std::str::Utf8Error> for EngineClientError { | ||
| fn from(err: std::str::Utf8Error) -> Self { | ||
| Self::Utf8Error(err.to_string()) | ||
| } | ||
| } |
There was a problem hiding this comment.
this can be implemented as a #from with thiserror
| impl From<std::ffi::NulError> for EngineClientError { | ||
| fn from(err: std::ffi::NulError) -> Self { | ||
| Self::CStringConversion(err.to_string()) | ||
| } | ||
| } |
There was a problem hiding this comment.
not sure if this trait impl is necessary
| /// HTTP error | ||
| #[error("HTTP error: {0}")] | ||
| HttpError(#[from] http::Error), |
| /// Hyper client error | ||
| #[error("Hyper client error: {0}")] | ||
| HyperClientError(#[from] HyperError), | ||
|
|
||
| /// Hyper request error | ||
| #[error("Hyper request error: {0}")] | ||
| HyperRequestError(String), |
There was a problem hiding this comment.
is it possible to merge this 2?
There was a problem hiding this comment.
yes, but I thought HyperClientError and HyperRequestError are different, I can just do one HyperError then instead of these two
There was a problem hiding this comment.
seems like not, HyperClientError is for requesting from client
let res = self
.client
.request(req)
.await
.map_err(EngineClientError::HyperError)?;while HyperRequestError is for collecting value from request, so I am not sure I can merge them into one, because they are different types actually
There was a problem hiding this comment.
so there is will be RequestBuilderError for builders;
CollectResponseError for collecting response;
and HyperError for error during sending constructed request.
is it okay?
|
I removed everything you said, but have a problem, as I said earlier on line 260 |
|
to delete |
Improve engine_api errors
Improve errors in engine_api #326
(this is my second time trying to contribute, feedback is welcome)
cargo fmt;cargo clippy;cargo testand all tests pass;