Improve error reporting for JSON decoding

* Use serde_path_to_error dependency for path to error * Stop using untagged ApiResponse enum, and manually check for errors instead
theduke · Sep 5, 2022 · 1945a97 · 1945a97
1 parent 4abe48c
commit 1945a97
Show file tree

Hide file tree

Showing 5 changed files with 79 additions and 46 deletions.
diff --git a/Cargo.toml b/Cargo.toml
@@ -22,6 +22,7 @@ url = "2.1.0"
 log = "0.4.5"
 futures = "0.3.4"
 tokio = { version = "1.0.1", default-features = false, features = ["sync", "time"] }
+serde_path_to_error = "0.1.8"
 
 [dev-dependencies]
 tokio = { version = "1.0.1", features = ["macros"]}

diff --git a/src/async_client.rs b/src/async_client.rs
@@ -7,6 +7,7 @@ use serde::de::DeserializeOwned;
 use std::collections::VecDeque;
 
 use super::Error;
+use crate::error::JsonDecodeError;
 use crate::types::*;
 
 /// Asynchronous client for the crates.io API.
@@ -168,28 +169,38 @@ impl Client {
         let time = tokio::time::Instant::now();
         let res = self.client.get(url.clone()).send().await?;
 
-        let result = match res.status() {
-            StatusCode::NOT_FOUND => Err(Error::NotFound(super::error::NotFoundError {
-                url: url.to_string(),
-            })),
-            StatusCode::FORBIDDEN => {
-                let reason = res.text().await.unwrap_or_default();
-                Err(Error::PermissionDenied(
-                    super::error::PermissionDeniedError { reason },
-                ))
-            }
-            _ if !res.status().is_success() => {
-                Err(Error::from(res.error_for_status().unwrap_err()))
-            }
-            _ => res.json::<ApiResponse<T>>().await.map_err(Error::from),
-        };
+        if !res.status().is_success() {
+            let err = match res.status() {
+                StatusCode::NOT_FOUND => Error::NotFound(super::error::NotFoundError {
+                    url: url.to_string(),
+                }),
+                StatusCode::FORBIDDEN => {
+                    let reason = res.text().await.unwrap_or_default();
+                    Error::PermissionDenied(super::error::PermissionDeniedError { reason })
+                }
+                _ => Error::from(res.error_for_status().unwrap_err()),
+            };
+
+            return Err(err);
+        }
+
+        let content = res.text().await?;
 
+        // Free up the lock
         (*lock) = Some(time);
 
-        match result? {
-            ApiResponse::Ok(t) => Ok(t),
-            ApiResponse::Err(err) => Err(Error::Api(err)),
+        // First, check for api errors.
+
+        if let Ok(errors) = serde_json::from_str::<ApiErrors>(&content) {
+            return Err(Error::Api(errors));
         }
+
+        let jd = &mut serde_json::Deserializer::from_str(&content);
+        serde_path_to_error::deserialize::<_, T>(jd).map_err(|err| {
+            Error::JsonDecode(JsonDecodeError {
+                message: format!("Could not decode JSON: {err} (path: {})", err.path()),
+            })
+        })
     }
 
     /// Retrieve a summary containing crates.io wide information.

diff --git a/src/error.rs b/src/error.rs
@@ -12,6 +12,8 @@ pub enum Error {
     NotFound(NotFoundError),
     /// No permission to access the resource.
     PermissionDenied(PermissionDeniedError),
+    /// JSON decoding of API response failed.
+    JsonDecode(JsonDecodeError),
     /// Error returned by the crates.io API directly.
     Api(crate::types::ApiErrors),
 }
@@ -36,6 +38,7 @@ impl std::fmt::Display for Error {
 
                 write!(f, "API Error ({})", inner)
             }
+            Error::JsonDecode(err) => write!(f, "Could not decode API JSON response: {err}"),
         }
     }
 }
@@ -48,6 +51,7 @@ impl std::error::Error for Error {
             Error::NotFound(_) => None,
             Error::PermissionDenied(_) => None,
             Error::Api(_) => None,
+            Error::JsonDecode(err) => Some(err),
         }
     }
 
@@ -76,6 +80,20 @@ impl From<url::ParseError> for Error {
     }
 }
 
+/// Error returned when the JSON returned by the API could not be decoded.
+#[derive(Debug)]
+pub struct JsonDecodeError {
+    pub(crate) message: String,
+}
+
+impl std::fmt::Display for JsonDecodeError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "Could not decode JSON: {}", self.message)
+    }
+}
+
+impl std::error::Error for JsonDecodeError {}
+
 /// Error returned when a resource could not be found.
 #[derive(Debug)]
 pub struct NotFoundError {

diff --git a/src/sync_client.rs b/src/sync_client.rs
@@ -5,7 +5,7 @@ use log::trace;
 use reqwest::{blocking::Client as HttpClient, header, StatusCode, Url};
 use serde::de::DeserializeOwned;
 
-use crate::types::*;
+use crate::{error::JsonDecodeError, types::*};
 
 /// A synchronous client for the crates.io API.
 pub struct SyncClient {
@@ -72,28 +72,38 @@ impl SyncClient {
         let time = std::time::Instant::now();
 
         let res = self.client.get(url.clone()).send()?;
-        let result = match res.status() {
-            StatusCode::NOT_FOUND => Err(Error::NotFound(super::error::NotFoundError {
-                url: url.to_string(),
-            })),
-            StatusCode::FORBIDDEN => {
-                let reason = res.text().unwrap_or_default();
-                Err(Error::PermissionDenied(
-                    super::error::PermissionDeniedError { reason },
-                ))
-            }
-            _ if !res.status().is_success() => {
-                Err(Error::from(res.error_for_status().unwrap_err()))
-            }
-            _ => res.json::<ApiResponse<T>>().map_err(Error::from),
-        };
+
+        if !res.status().is_success() {
+            let err = match res.status() {
+                StatusCode::NOT_FOUND => Error::NotFound(super::error::NotFoundError {
+                    url: url.to_string(),
+                }),
+                StatusCode::FORBIDDEN => {
+                    let reason = res.text().unwrap_or_default();
+                    Error::PermissionDenied(super::error::PermissionDeniedError { reason })
+                }
+                _ => Error::from(res.error_for_status().unwrap_err()),
+            };
+
+            return Err(err);
+        }
 
         *lock = Some(time);
 
-        match result? {
-            ApiResponse::Ok(t) => Ok(t),
-            ApiResponse::Err(err) => Err(Error::Api(err)),
+        let content = res.text()?;
+
+        // First, check for api errors.
+
+        if let Ok(errors) = serde_json::from_str::<ApiErrors>(&content) {
+            return Err(Error::Api(errors));
         }
+
+        let jd = &mut serde_json::Deserializer::from_str(&content);
+        serde_path_to_error::deserialize::<_, T>(jd).map_err(|err| {
+            Error::JsonDecode(JsonDecodeError {
+                message: format!("Could not decode JSON: {err} (path: {})", err.path()),
+            })
+        })
     }
 
     /// Retrieve a summary containing crates.io wide information.

diff --git a/src/types.rs b/src/types.rs
@@ -28,13 +28,6 @@ impl std::fmt::Display for ApiError {
     }
 }
 
-#[derive(Deserialize, Debug, Clone, PartialEq, Eq)]
-#[serde(untagged)]
-pub(crate) enum ApiResponse<T> {
-    Err(ApiErrors),
-    Ok(T),
-}
-
 /// Used to specify the sort behaviour of the `Client::crates()` method.
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub enum Sort {
@@ -292,7 +285,7 @@ pub struct Crate {
     pub keywords: Option<Vec<String>>,
     pub versions: Option<Vec<u64>>,
     pub max_version: String,
-    pub max_stable_version: String,
+    pub max_stable_version: Option<String>,
     pub links: CrateLinks,
     pub created_at: DateTime<Utc>,
     pub updated_at: DateTime<Utc>,
@@ -569,7 +562,7 @@ pub struct FullCrate {
     pub repository: Option<String>,
     pub total_downloads: u64,
     pub max_version: String,
-    pub max_stable_version: String,
+    pub max_stable_version: Option<String>,
     pub created_at: DateTime<Utc>,
     pub updated_at: DateTime<Utc>,