Refactor error types to use thiserror and internalize BuildError

Signed-off-by: Thomas Farr <[email protected]>

CHANGELOG.md

@@ -2,6 +2,8 @@
 Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 
 ## [Unreleased]
+### ⚠️ Breaking Changes ⚠️
+- Internalized the `BuildError` type, consolidating on the `Error` type ([#228](https://github.com/opensearch-project/opensearch-rs/pull/228))
 
 ### Added
 

opensearch/Cargo.toml

@@ -39,6 +39,7 @@ url = "2.1"
 serde = { version = "1", features = ["derive"] }
 serde_json = "1"
 serde_with = "3"
+thiserror = "1"
 void = "1.0.2"
 aws-credential-types = { version = "1", optional = true }
 aws-sigv4 = { version = "1", optional = true }

opensearch/src/auth.rs

@@ -97,11 +97,11 @@ impl std::convert::TryFrom<&aws_types::SdkConfig> for Credentials {
     fn try_from(value: &aws_types::SdkConfig) -> Result<Self, Self::Error> {
         let credentials = value
             .credentials_provider()
-            .ok_or_else(|| super::error::lib("SdkConfig does not have a credentials_provider"))?
+            .ok_or(crate::http::aws_auth::AwsSigV4Error::MissingCredentialsProvider)?
             .clone();
         let region = value
             .region()
-            .ok_or_else(|| super::error::lib("SdkConfig does not have a region"))?
+            .ok_or(crate::http::aws_auth::AwsSigV4Error::MissingRegion)?
             .clone();
         Ok(Credentials::AwsSigV4(credentials, region))
     }

opensearch/src/cert.rs

@@ -233,7 +233,10 @@ impl Certificate {
                 END_CERTIFICATE if begin => {
                     begin = false;
                     cert.push(line);
-                    certs.push(reqwest::Certificate::from_pem(cert.join("\n").as_bytes())?);
+                    certs.push(
+                        reqwest::Certificate::from_pem(cert.join("\n").as_bytes())
+                            .map_err(CertificateError::MalformedCertificate)?,
+                    );
                     cert = Vec::new();
                 }
                 _ if begin => cert.push(line),
@@ -242,17 +245,15 @@ impl Certificate {
         }
 
         if certs.is_empty() {
-            Err(crate::error::lib(
-                "could not find PEM certificate in input data",
-            ))
+            Err(CertificateError::MissingPemCertificate.into())
         } else {
             Ok(Self(certs))
         }
     }
 
     /// Create a `Certificate` from a binary DER encoded certificate.
     pub fn from_der(der: &[u8]) -> Result<Self, Error> {
-        Ok(Self(vec![reqwest::Certificate::from_der(der)?]))
+        Ok(Self(vec![reqwest::Certificate::from_der(der).map_err(CertificateError::MalformedCertificate)?]))
     }
 
     /// Append a `Certificate` to the chain.
@@ -279,3 +280,11 @@ impl Deref for Certificate {
         &self.0
     }
 }
+
+#[derive(Debug, thiserror::Error)]
+pub(crate) enum CertificateError {
+    #[error("could not find PEM certificate in input data")]
+    MissingPemCertificate,
+    #[error("malformed certificate: {0}")]
+    MalformedCertificate(reqwest::Error),
+}

opensearch/src/error.rs

@@ -32,125 +32,76 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-use crate::http::{transport::BuildError, StatusCode};
-use std::{error, fmt, io};
+
+use crate::{
+    cert::CertificateError,
+    http::{transport, StatusCode},
+};
+
+#[allow(unused)]
+pub(crate) type BoxError = Box<dyn std::error::Error + Send + Sync>;
 
 /// An error with the client.
 ///
 /// Errors that can occur include IO and parsing errors, as well as specific
-/// errors from Elasticsearch and internal errors from the client.
-#[derive(Debug)]
-pub struct Error {
-    kind: Kind,
+/// errors from OpenSearch and internal errors from the client.
+#[derive(Debug, thiserror::Error)]
+#[error(transparent)]
+pub struct Error(Kind);
+
+impl<E> From<E> for Error
+where
+    Kind: From<E>,
+{
+    fn from(error: E) -> Self {
+        Self(Kind::from(error))
+    }
 }
 
-#[derive(Debug)]
-enum Kind {
-    /// An error building the client
-    Build(BuildError),
-
-    /// A general error from this library
-    Lib(String),
-
-    /// HTTP library error
-    Http(reqwest::Error),
-
-    /// IO error
-    Io(io::Error),
-
-    /// JSON error
-    Json(serde_json::error::Error),
-}
+#[derive(Debug, thiserror::Error)]
+pub(crate) enum Kind {
+    #[error("transport builder error: {0}")]
+    TransportBuilder(#[from] transport::BuildError),
 
-impl From<io::Error> for Error {
-    fn from(err: io::Error) -> Error {
-        Error {
-            kind: Kind::Io(err),
-        }
-    }
-}
+    #[error("certificate error: {0}")]
+    Certificate(#[from] CertificateError),
 
-impl From<reqwest::Error> for Error {
-    fn from(err: reqwest::Error) -> Error {
-        Error {
-            kind: Kind::Http(err),
-        }
-    }
-}
+    #[error("reqwest error: {0}")]
+    Reqwest(#[from] reqwest::Error),
 
-impl From<serde_json::error::Error> for Error {
-    fn from(err: serde_json::error::Error) -> Error {
-        Error {
-            kind: Kind::Json(err),
-        }
-    }
-}
+    #[error("URL parse error: {0}")]
+    UrlParse(#[from] url::ParseError),
 
-impl From<url::ParseError> for Error {
-    fn from(err: url::ParseError) -> Error {
-        Error {
-            kind: Kind::Lib(err.to_string()),
-        }
-    }
-}
+    #[error("IO error: {0}")]
+    Io(#[from] std::io::Error),
 
-impl From<BuildError> for Error {
-    fn from(err: BuildError) -> Error {
-        Error {
-            kind: Kind::Build(err),
-        }
-    }
-}
+    #[error("JSON error: {0}")]
+    Json(#[from] serde_json::error::Error),
 
-pub(crate) fn lib(err: impl Into<String>) -> Error {
-    Error {
-        kind: Kind::Lib(err.into()),
-    }
+    #[cfg(feature = "aws-auth")]
+    #[error("AwsSigV4 error: {0}")]
+    AwsSigV4(#[from] crate::http::aws_auth::AwsSigV4Error),
 }
 
 impl Error {
     /// The status code, if the error was generated from a response
     pub fn status_code(&self) -> Option<StatusCode> {
-        match &self.kind {
-            Kind::Http(err) => err.status(),
+        match &self.0 {
+            Kind::Reqwest(err) => err.status(),
             _ => None,
         }
     }
 
     /// Returns true if the error is related to a timeout
     pub fn is_timeout(&self) -> bool {
-        match &self.kind {
-            Kind::Http(err) => err.is_timeout(),
+        match &self.0 {
+            Kind::Reqwest(err) => err.is_timeout(),
             _ => false,
         }
     }
 
     /// Returns true if the error is related to serialization or deserialization
     pub fn is_json(&self) -> bool {
-        matches!(self.kind, Kind::Json(_))
-    }
-}
-
-impl error::Error for Error {
-    fn source(&self) -> Option<&(dyn error::Error + 'static)> {
-        match &self.kind {
-            Kind::Build(err) => Some(err),
-            Kind::Lib(_) => None,
-            Kind::Http(err) => Some(err),
-            Kind::Io(err) => Some(err),
-            Kind::Json(err) => Some(err),
-        }
-    }
-}
-
-impl fmt::Display for Error {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        match &self.kind {
-            Kind::Build(err) => err.fmt(f),
-            Kind::Lib(err) => err.fmt(f),
-            Kind::Http(err) => err.fmt(f),
-            Kind::Io(err) => err.fmt(f),
-            Kind::Json(err) => err.fmt(f),
-        }
+        matches!(self.0, Kind::Json(_))
     }
 }

opensearch/src/http/aws_auth.rs

@@ -9,7 +9,7 @@
  * GitHub history for details.
  */
 
-use crate::http::headers::HeaderValue;
+use crate::{http::headers::HeaderValue, BoxError};
 use aws_credential_types::provider::{ProvideCredentials, SharedCredentialsProvider};
 use aws_sigv4::{
     http_request::{
@@ -21,15 +21,32 @@ use aws_smithy_runtime_api::client::identity::Identity;
 use aws_types::{region::Region, sdk_config::SharedTimeSource};
 use reqwest::Request;
 
-pub async fn sign_request(
+#[derive(Debug, thiserror::Error)]
+pub(crate) enum AwsSigV4Error {
+    #[error("SdkConfig is does not have a credentials provider configured")]
+    MissingCredentialsProvider,
+    #[error("SdkConfig is does not have a region configured")]
+    MissingRegion,
+    #[error("signing error: {0}")]
+    SigningError(#[source] BoxError),
+}
+
+fn signing_error<E: Into<BoxError>>(e: E) -> AwsSigV4Error {
+    AwsSigV4Error::SigningError(e.into())
+}
+
+pub(crate) async fn sign_request(
     request: &mut Request,
     credentials_provider: &SharedCredentialsProvider,
     service_name: &str,
     region: &Region,
     time_source: &SharedTimeSource,
-) -> Result<(), Box<dyn std::error::Error + Send + Sync>> {
+) -> Result<(), AwsSigV4Error> {
     let identity = {
-        let c = credentials_provider.provide_credentials().await?;
+        let c = credentials_provider
+            .provide_credentials()
+            .await
+            .map_err(signing_error)?;
         let e = c.expiry();
         Identity::new(c, e)
     };
@@ -47,7 +64,8 @@ pub async fn sign_request(
             .region(region.as_ref())
             .time(time_source.now())
             .settings(signing_settings)
-            .build()?;
+            .build()
+            .map_err(signing_error)?;
         SigningParams::V4(p)
     };
 
@@ -68,11 +86,11 @@ pub async fn sign_request(
             None => SignableBody::Bytes(&[]),
         };
 
-        SignableRequest::new(method, uri, headers, body)?
+        SignableRequest::new(method, uri, headers, body).map_err(signing_error)?
     };
 
     let (new_headers, new_query_params) = {
-        let (instructions, _) = sign(signable_request, &params)?.into_parts();
+        let (instructions, _) = sign(signable_request, &params).map_err(signing_error)?.into_parts();
         instructions.into_parts()
     };
 

opensearch/src/http/headers.rs

@@ -28,7 +28,7 @@
  * GitHub history for details.
  */
 
-//! HTTP header names and values, including those specific to Elasticsearch
+//! HTTP header names and values, including those specific to OpenSearch
 
 pub use reqwest::header::*;
 

opensearch/src/http/mod.rs

@@ -38,10 +38,10 @@ pub mod request;
 pub mod response;
 pub mod transport;
 
-pub use reqwest::StatusCode;
+pub use reqwest::{self, Request, StatusCode};
 pub use url::Url;
 
-/// Http methods supported by Elasticsearch
+/// Http methods supported by OpenSearch
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum Method {
     /// get