Refactor API errors (#282)

* make unknown api errors private * fix duplicate ParseError in prelude
elastic-rs · May 7, 2018 · db52638 · db52638
1 parent fdb01e2
commit db52638
Show file tree

Hide file tree

Showing 19 changed files with 142 additions and 172 deletions.
diff --git a/src/elastic/examples/custom_response.rs b/src/elastic/examples/custom_response.rs
@@ -35,7 +35,7 @@ struct Hit {
 
 // Implement `IsOk` for our custom `SearchResponse` so it can be used in the call to `into_response`.
 impl IsOk for SearchResponse {
-    fn is_ok<B: ResponseBody>(head: HttpResponseHead, body: Unbuffered<B>) -> Result<MaybeOkResponse<B>, ParseResponseError> {
+    fn is_ok<B: ResponseBody>(head: HttpResponseHead, body: Unbuffered<B>) -> Result<MaybeOkResponse<B>, ParseError> {
         match head.status() {
             200...299 => Ok(MaybeOkResponse::ok(body)),
             _ => Ok(MaybeOkResponse::err(body)),

diff --git a/src/elastic/src/client/responses/mod.rs b/src/elastic/src/client/responses/mod.rs
@@ -24,8 +24,5 @@ pub use elastic_responses::bulk;
 pub mod prelude {
     /*! A glob import for convenience. */
 
-    pub use super::{BulkErrorsResponse, BulkResponse, CommandResponse, DeleteResponse, GetResponse, IndicesExistsResponse, IndexResponse, PingResponse, SearchResponse, Shards, UpdateResponse};
-
-    pub use super::async::AsyncResponseBuilder;
-    pub use super::sync::SyncResponseBuilder;
+    pub use super::{AsyncResponseBuilder, SyncResponseBuilder, BulkErrorsResponse, BulkResponse, CommandResponse, DeleteResponse, GetResponse, IndexResponse, IndicesExistsResponse, PingResponse, SearchResponse, Shards, UpdateResponse};
 }
diff --git a/src/elastic/src/client/responses/parse.rs b/src/elastic/src/client/responses/parse.rs
@@ -18,7 +18,7 @@ struct MyResponse {
 }
 
 impl IsOk for MyResponse {
-    fn is_ok<B>(head: HttpResponseHead, body: Unbuffered<B>) -> Result<MaybeOkResponse<B>, ParseResponseError>
+    fn is_ok<B>(head: HttpResponseHead, body: Unbuffered<B>) -> Result<MaybeOkResponse<B>, ParseError>
         where B: ResponseBody
     {
         match head.status() {
@@ -48,7 +48,7 @@ The `MyResponse` type can then be used for deserialising a concrete response:
 #    took: u64
 # }
 # impl IsOk for MyResponse {
-#     fn is_ok<B>(head: HttpResponseHead, body: Unbuffered<B>) -> Result<MaybeOkResponse<B>, ParseResponseError>
+#     fn is_ok<B>(head: HttpResponseHead, body: Unbuffered<B>) -> Result<MaybeOkResponse<B>, ParseError>
 #         where B: ResponseBody
 #     {
 #         match head.status() {
@@ -90,4 +90,4 @@ See the [`IsOk`][IsOk] trait for more details.
 pub(crate) use elastic_responses::parse;
 
 pub use elastic_responses::parsing::{Buffered, HttpResponseHead, IsOk, MaybeBufferedResponse, MaybeOkResponse, ResponseBody, Unbuffered};
-pub use elastic_responses::error::ParseResponseError;
+pub use elastic_responses::error::ParseError;
diff --git a/src/elastic/src/client/sender/sniffed_nodes/nodes_info.rs b/src/elastic/src/client/sender/sniffed_nodes/nodes_info.rs
@@ -2,7 +2,7 @@
 
 use std::fmt;
 use serde::de::{Deserialize, Deserializer, MapAccess, Visitor};
-use client::responses::parse::{HttpResponseHead, IsOk, MaybeOkResponse, ParseResponseError, ResponseBody, Unbuffered};
+use client::responses::parse::{HttpResponseHead, IsOk, MaybeOkResponse, ParseError, ResponseBody, Unbuffered};
 
 use std::iter::IntoIterator;
 use std::vec::IntoIter;
@@ -32,7 +32,7 @@ impl IntoIterator for NodesInfoResponse {
 }
 
 impl IsOk for NodesInfoResponse {
-    fn is_ok<B: ResponseBody>(head: HttpResponseHead, unbuffered: Unbuffered<B>) -> Result<MaybeOkResponse<B>, ParseResponseError> {
+    fn is_ok<B: ResponseBody>(head: HttpResponseHead, unbuffered: Unbuffered<B>) -> Result<MaybeOkResponse<B>, ParseError> {
         match head.status() {
             200...299 => Ok(MaybeOkResponse::ok(unbuffered)),
             _ => Ok(MaybeOkResponse::err(unbuffered)),

diff --git a/src/responses/src/bulk.rs b/src/responses/src/bulk.rs
@@ -521,7 +521,7 @@ pub enum Action {
 }
 
 impl<TIndex, TType, TId> IsOk for BulkResponse<TIndex, TType, TId> {
-    fn is_ok<B: ResponseBody>(head: HttpResponseHead, body: Unbuffered<B>) -> Result<MaybeOkResponse<B>, ParseResponseError> {
+    fn is_ok<B: ResponseBody>(head: HttpResponseHead, body: Unbuffered<B>) -> Result<MaybeOkResponse<B>, ParseError> {
         match head.status() {
             200...299 => Ok(MaybeOkResponse::ok(body)),
             _ => Ok(MaybeOkResponse::err(body)),
@@ -530,7 +530,7 @@ impl<TIndex, TType, TId> IsOk for BulkResponse<TIndex, TType, TId> {
 }
 
 impl<TIndex, TType, TId> IsOk for BulkErrorsResponse<TIndex, TType, TId> {
-    fn is_ok<B: ResponseBody>(head: HttpResponseHead, body: Unbuffered<B>) -> Result<MaybeOkResponse<B>, ParseResponseError> {
+    fn is_ok<B: ResponseBody>(head: HttpResponseHead, body: Unbuffered<B>) -> Result<MaybeOkResponse<B>, ParseError> {
         match head.status() {
             200...299 => Ok(MaybeOkResponse::ok(body)),
             _ => Ok(MaybeOkResponse::err(body)),

diff --git a/src/responses/src/command.rs b/src/responses/src/command.rs
@@ -23,7 +23,7 @@ impl CommandResponse {
 }
 
 impl IsOk for CommandResponse {
-    fn is_ok<B: ResponseBody>(head: HttpResponseHead, body: Unbuffered<B>) -> Result<MaybeOkResponse<B>, ParseResponseError> {
+    fn is_ok<B: ResponseBody>(head: HttpResponseHead, body: Unbuffered<B>) -> Result<MaybeOkResponse<B>, ParseError> {
         match head.status() {
             200...299 => Ok(MaybeOkResponse::ok(body)),
             _ => Ok(MaybeOkResponse::err(body)),

diff --git a/src/responses/src/delete.rs b/src/responses/src/delete.rs
@@ -54,7 +54,7 @@ impl DeleteResponse {
 }
 
 impl IsOk for DeleteResponse {
-    fn is_ok<B: ResponseBody>(head: HttpResponseHead, body: Unbuffered<B>) -> Result<MaybeOkResponse<B>, ParseResponseError> {
+    fn is_ok<B: ResponseBody>(head: HttpResponseHead, body: Unbuffered<B>) -> Result<MaybeOkResponse<B>, ParseError> {
         match head.status() {
             200...299 | 404 => Ok(MaybeOkResponse::ok(body)),
             _ => Ok(MaybeOkResponse::err(body)),

diff --git a/src/responses/src/error.rs b/src/responses/src/error.rs
@@ -1,24 +1,89 @@
 /*!
-Error types from Elasticsearch
+Error types from Elasticsearch.
 */
 
 use serde::{Deserialize, Deserializer};
 use serde_json::{Error as JsonError, Map, Value};
+use std::error::Error as StdError;
 use std::io::Error as IoError;
+use std::fmt;
+
+mod inner {
+    use std::fmt;
+    use std::error::Error as StdError;
+    use serde_json::{Map, Value};
+
+    use super::ApiError;
 
-quick_error! {
-    /** An error parsing a response stream. */
     #[derive(Debug)]
-    pub enum ParseResponseError {
-        /** The response contains invalid json. */
-        Json(err: JsonError) {
-            from()
+    pub struct UnknownApiError(pub Map<String, Value>);
+
+    impl fmt::Display for UnknownApiError {
+        fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+            (&self.0 as &fmt::Debug).fmt(f)
         }
-        /** The response caused an io error while buffering. */
-        Io(err: IoError) {
-            from()
+    }
+
+    impl StdError for UnknownApiError {
+        fn description(&self) -> &str {
+            "an unknown API error"
+        }
+
+        fn cause(&self) -> Option<&StdError> {
+            None
         }
     }
+
+    pub enum ParsedApiError {
+        Known(ApiError),
+        Unknown(Map<String, Value>)
+    }
+}
+
+pub(crate) use self::inner::*;
+
+/**
+A generic error parsing an API response.
+*/
+#[derive(Debug)]
+pub struct ParseError {
+    inner: Box<StdError + Send + Sync>,
+}
+
+impl ParseError {
+    pub fn new<E>(err: E) -> Self where E: StdError + Send + Sync + 'static {
+        ParseError {
+            inner: Box::new(err)
+        }
+    }
+}
+
+impl From<IoError> for ParseError {
+    fn from(err: IoError) -> Self {
+        ParseError::new(err)
+    }
+}
+
+impl From<JsonError> for ParseError {
+    fn from(err: JsonError) -> Self {
+        ParseError::new(err)
+    }
+}
+
+impl fmt::Display for ParseError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        self.inner.fmt(f)
+    }
+}
+
+impl StdError for ParseError {
+    fn description(&self) -> &str {
+        self.inner.description()
+    }
+
+    fn cause(&self) -> Option<&StdError> {
+        Some(&*self.inner)
+    }
 }
 
 quick_error! {
@@ -30,7 +95,7 @@ quick_error! {
             from()
         }
         /** An error parsing a response body. */
-        Parse(err: ParseResponseError) {
+        Parse(err: ParseError) {
             from()
         }
     }
@@ -69,24 +134,6 @@ quick_error! {
             display("index already exists: '{}'", index)
         }
         /**
-        The request body contains invalid data.
-
-        If a Query DSL query contains invalid JSON or unrecognised properties then Elasticsearch will return a `Parsing` error.
-        */
-        Parsing { line: u64, col: u64, reason: String } {
-            description("request parse error")
-            display("request parse error: '{}' on line: {}, col: {}", reason, line, col)
-        }
-        /**
-        The document mapping contains invalid data.
-
-        If a put mapping request contains invalid JSON or unrecognised properties then Elasticsearch will return a `MapperParsing` error.
-        */
-        MapperParsing { reason: String } {
-            description("mapper parse error")
-            display("mapper parse error: '{}'", reason)
-        }
-        /**
         The request body can't be processed.
 
         Some endpoints that expect certain constraints of a request to hold will return an `ActionRequestValidation` error if those constraints aren't met.
@@ -95,17 +142,6 @@ quick_error! {
             description("action request failed validation")
             display("action request failed validation: '{}'", reason)
         }
-        /**
-        A currently unrecognised error occurred.
-
-        **WARNING:** Don't rely on this variant to capture an error.
-        As new variants are introduced they will no longer be matched by `ApiError::Other`.
-        For this reason, this variant will disappear in the future.
-        */
-        Other(v: Map<String, Value>) {
-            description("error response from Elasticsearch")
-            display("error response from Elasticsearch: {:?}", v)
-        }
         #[doc(hidden)]
         __NonExhaustive {}
     }
@@ -119,13 +155,13 @@ macro_rules! error_key {
 
             match key {
                 Some(v) => v,
-                _ => return ApiError::Other($obj).into()
+                _ => return ParsedApiError::Unknown($obj)
             }
         }
     )
 }
 
-impl<'de> Deserialize<'de> for ApiError {
+impl<'de> Deserialize<'de> for ParsedApiError {
     fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
     where
         D: Deserializer<'de>,
@@ -136,12 +172,12 @@ impl<'de> Deserialize<'de> for ApiError {
     }
 }
 
-impl From<Map<String, Value>> for ApiError {
+impl From<Map<String, Value>> for ParsedApiError {
     fn from(mut value: Map<String, Value>) -> Self {
         let obj = {
             match value.remove("error") {
                 Some(Value::Object(value)) => value,
-                _ => return ApiError::Other(value),
+                _ => return ParsedApiError::Unknown(value),
             }
         };
 
@@ -152,58 +188,40 @@ impl From<Map<String, Value>> for ApiError {
 
             match ty {
                 Some(ty) => ty,
-                _ => return ApiError::Other(obj),
+                _ => return ParsedApiError::Unknown(obj),
             }
         };
 
         match ty.as_ref() {
             "index_not_found_exception" => {
                 let index = error_key!(obj[index]: |v| v.as_str());
 
-                ApiError::IndexNotFound {
+                ParsedApiError::Known(ApiError::IndexNotFound {
                     index: index.into(),
-                }
+                })
             }
             "index_already_exists_exception" => {
                 let index = error_key!(obj[index]: |v| v.as_str());
 
-                ApiError::IndexAlreadyExists {
+                ParsedApiError::Known(ApiError::IndexAlreadyExists {
                     index: index.into(),
-                }
+                })
             }
             "document_missing_exception" => {
                 let index = error_key!(obj[index]: |v| v.as_str());
 
-                ApiError::DocumentMissing {
+                ParsedApiError::Known(ApiError::DocumentMissing {
                     index: index.into(),
-                }
-            }
-            "parsing_exception" => {
-                let line = error_key!(obj[line]: |v| v.as_u64());
-                let col = error_key!(obj[col]: |v| v.as_u64());
-                let reason = error_key!(obj[reason]: |v| v.as_str());
-
-                ApiError::Parsing {
-                    line: line,
-                    col: col,
-                    reason: reason.into(),
-                }
-            }
-            "mapper_parsing_exception" => {
-                let reason = error_key!(obj[reason]: |v| v.as_str());
-
-                ApiError::MapperParsing {
-                    reason: reason.into(),
-                }
+                })
             }
             "action_request_validation_exception" => {
                 let reason = error_key!(obj[reason]: |v| v.as_str());
 
-                ApiError::ActionRequestValidation {
+                ParsedApiError::Known(ApiError::ActionRequestValidation {
                     reason: reason.into(),
-                }
+                })
             }
-            _ => ApiError::Other(obj),
+            _ => ParsedApiError::Unknown(obj),
         }
     }
 }
diff --git a/src/responses/src/get.rs b/src/responses/src/get.rs
@@ -57,7 +57,7 @@ impl<T> GetResponse<T> {
 }
 
 impl<T: DeserializeOwned> IsOk for GetResponse<T> {
-    fn is_ok<B: ResponseBody>(head: HttpResponseHead, body: Unbuffered<B>) -> Result<MaybeOkResponse<B>, ParseResponseError> {
+    fn is_ok<B: ResponseBody>(head: HttpResponseHead, body: Unbuffered<B>) -> Result<MaybeOkResponse<B>, ParseError> {
         match head.status() {
             200...299 => Ok(MaybeOkResponse::ok(body)),
             404 => {

diff --git a/src/responses/src/index.rs b/src/responses/src/index.rs
@@ -50,7 +50,7 @@ impl IndexResponse {
 }
 
 impl IsOk for IndexResponse {
-    fn is_ok<B: ResponseBody>(head: HttpResponseHead, body: Unbuffered<B>) -> Result<MaybeOkResponse<B>, ParseResponseError> {
+    fn is_ok<B: ResponseBody>(head: HttpResponseHead, body: Unbuffered<B>) -> Result<MaybeOkResponse<B>, ParseError> {
         match head.status() {
             200...299 => Ok(MaybeOkResponse::ok(body)),
             _ => Ok(MaybeOkResponse::err(body)),

diff --git a/src/responses/src/indices_exists.rs b/src/responses/src/indices_exists.rs
@@ -19,7 +19,7 @@ impl IndicesExistsResponse {
 }
 
 impl IsOk for IndicesExistsResponse {
-    fn is_ok<B: ResponseBody>(head: HttpResponseHead, body: Unbuffered<B>) -> Result<MaybeOkResponse<B>, ParseResponseError> {
+    fn is_ok<B: ResponseBody>(head: HttpResponseHead, body: Unbuffered<B>) -> Result<MaybeOkResponse<B>, ParseError> {
         match head.status() {
             200...299 => Ok(MaybeOkResponse::ok(json!({ "exists": true }))),
             404 => Ok(MaybeOkResponse::ok(json!({ "exists": false }))),