Auto merge of #2625 - jtgeibel:fix-naming-of-error-type, r=JohnTitor

Cleanup backend json error structs

Finish some cleanup work for how JSON error messages are generated. All associated types are moved to `errors::json`.

Going forward, there is some direct usage of `Response::builder()` in the middleware. These return text instead of JSON. We should look into this usage to see if some of them should be switched to JSON. For any remaining we could consider a set of `errors::text` helpers.

r? @JohnTitor

Author: bors

src/controllers/util.rs

@@ -2,7 +2,7 @@ use super::prelude::*;
 
 use crate::middleware::current_user::TrustedUserId;
 use crate::models::{ApiToken, User};
-use crate::util::errors::{internal, AppError, AppResult, ChainError, Unauthorized};
+use crate::util::errors::{forbidden, internal, AppResult, ChainError};
 
 #[derive(Debug)]
 pub struct AuthenticatedUser {
@@ -26,7 +26,7 @@ impl AuthenticatedUser {
 }
 
 impl<'a> UserAuthenticationExt for dyn RequestExt + 'a {
-    /// Obtain `AuthenticatedUser` for the request or return an `Unauthorized` error
+    /// Obtain `AuthenticatedUser` for the request or return an `Forbidden` error
     fn authenticate(&self, conn: &PgConnection) -> AppResult<AuthenticatedUser> {
         if let Some(id) = self.extensions().find::<TrustedUserId>() {
             // A trusted user_id was provided by a signed cookie (or a test `MockCookieUser`)
@@ -43,11 +43,10 @@ impl<'a> UserAuthenticationExt for dyn RequestExt + 'a {
                         token_id: Some(token.id),
                     })
                     .chain_error(|| internal("invalid token"))
-                    .chain_error(|| Box::new(Unauthorized) as Box<dyn AppError>)
+                    .chain_error(forbidden)
             } else {
                 // Unable to authenticate the user
-                Err(internal("no cookie session or auth header found"))
-                    .chain_error(|| Box::new(Unauthorized) as Box<dyn AppError>)
+                Err(internal("no cookie session or auth header found")).chain_error(forbidden)
             }
         }
     }

src/github.rs

@@ -8,7 +8,7 @@ use serde::de::DeserializeOwned;
 use std::str;
 
 use crate::app::App;
-use crate::util::errors::{cargo_err, internal, AppError, AppResult, NotFound};
+use crate::util::errors::{cargo_err, internal, not_found, AppError, AppResult};
 
 /// Does all the nonsense for sending a GET to Github. Doesn't handle parsing
 /// because custom error-code handling may be desirable. Use
@@ -46,7 +46,7 @@ fn handle_error_response(app: &App, error: &reqwest::Error) -> Box<dyn AppError>
              https://{}/login",
             app.config.domain_name,
         )),
-        Some(Status::NOT_FOUND) => Box::new(NotFound),
+        Some(Status::NOT_FOUND) => not_found(),
         _ => internal(&format_args!(
             "didn't get a 200 result from github: {}",
             error

src/router.rs

@@ -178,7 +178,7 @@ impl Handler for R404 {
                 req.mut_extensions().insert(m.params.clone());
                 m.handler.call(req)
             }
-            Err(..) => Ok(NotFound.response().unwrap()),
+            Err(..) => Ok(NotFound.into()),
         }
     }
 }
@@ -187,7 +187,7 @@ impl Handler for R404 {
 mod tests {
     use super::*;
     use crate::util::errors::{
-        bad_request, cargo_err, internal, AppError, ChainError, NotFound, Unauthorized,
+        bad_request, cargo_err, forbidden, internal, not_found, AppError, ChainError,
     };
     use crate::util::EndpointResult;
 
@@ -209,7 +209,7 @@ mod tests {
             StatusCode::BAD_REQUEST
         );
         assert_eq!(
-            C(|_| err(Unauthorized)).call(&mut req).unwrap().status(),
+            C(|_| Err(forbidden())).call(&mut req).unwrap().status(),
             StatusCode::FORBIDDEN
         );
         assert_eq!(
@@ -220,7 +220,7 @@ mod tests {
             StatusCode::NOT_FOUND
         );
         assert_eq!(
-            C(|_| err(NotFound)).call(&mut req).unwrap().status(),
+            C(|_| Err(not_found())).call(&mut req).unwrap().status(),
             StatusCode::NOT_FOUND
         );
 

src/util/errors.rs

@@ -16,22 +16,22 @@ use std::any::{Any, TypeId};
 use std::error::Error;
 use std::fmt;
 
-use chrono::NaiveDateTime;
-use conduit::{header, StatusCode};
 use diesel::result::Error as DieselError;
 
-use crate::util::{json_response, AppResponse};
+use crate::util::AppResponse;
 
 pub(super) mod concrete;
-mod http;
+mod json;
+
+pub(crate) use json::{NotFound, ReadOnlyMode, TooManyRequests};
 
 /// Returns an error with status 200 and the provided description as JSON
 ///
 /// This is for backwards compatibility with cargo endpoints.  For all other
 /// endpoints, use helpers like `bad_request` or `server_error` which set a
 /// correct status code.
 pub fn cargo_err<S: ToString + ?Sized>(error: &S) -> Box<dyn AppError> {
-    Box::new(http::Ok(error.to_string()))
+    Box::new(json::Ok(error.to_string()))
 }
 
 // The following are intended to be used for errors being sent back to the Ember
@@ -41,30 +41,20 @@ pub fn cargo_err<S: ToString + ?Sized>(error: &S) -> Box<dyn AppError> {
 
 /// Return an error with status 400 and the provided description as JSON
 pub fn bad_request<S: ToString + ?Sized>(error: &S) -> Box<dyn AppError> {
-    Box::new(http::BadRequest(error.to_string()))
+    Box::new(json::BadRequest(error.to_string()))
 }
 
-/// Returns an error with status 500 and the provided description as JSON
-pub fn server_error<S: ToString + ?Sized>(error: &S) -> Box<dyn AppError> {
-    Box::new(http::ServerError(error.to_string()))
+pub fn forbidden() -> Box<dyn AppError> {
+    Box::new(json::Forbidden)
 }
 
-#[derive(Serialize)]
-struct StringError<'a> {
-    detail: &'a str,
-}
-#[derive(Serialize)]
-struct Bad<'a> {
-    errors: Vec<StringError<'a>>,
+pub fn not_found() -> Box<dyn AppError> {
+    Box::new(json::NotFound)
 }
 
-/// Generates a response with the provided status and description as JSON
-fn json_error(detail: &str, status: StatusCode) -> AppResponse {
-    let mut response = json_response(&Bad {
-        errors: vec![StringError { detail }],
-    });
-    *response.status_mut() = status;
-    response
+/// Returns an error with status 500 and the provided description as JSON
+pub fn server_error<S: ToString + ?Sized>(error: &S) -> Box<dyn AppError> {
+    Box::new(json::ServerError(error.to_string()))
 }
 
 // =============================================================================
@@ -99,7 +89,7 @@ impl dyn AppError {
 
     fn try_convert(err: &(dyn Error + Send + 'static)) -> Option<Box<Self>> {
         match err.downcast_ref() {
-            Some(DieselError::NotFound) => Some(Box::new(NotFound)),
+            Some(DieselError::NotFound) => Some(not_found()),
             Some(DieselError::DatabaseError(_, info))
                 if info.message().ends_with("read-only transaction") =>
             {
@@ -217,45 +207,6 @@ impl AppError for InternalAppError {
     }
 }
 
-// TODO: The remaining can probably move under `http`
-
-#[derive(Debug, Clone, Copy)]
-pub struct NotFound;
-
-impl From<NotFound> for AppResponse {
-    fn from(_: NotFound) -> AppResponse {
-        json_error("Not Found", StatusCode::NOT_FOUND)
-    }
-}
-
-impl AppError for NotFound {
-    fn response(&self) -> Option<AppResponse> {
-        Some(NotFound.into())
-    }
-}
-
-impl fmt::Display for NotFound {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        "Not Found".fmt(f)
-    }
-}
-
-#[derive(Debug, Clone, Copy)]
-pub struct Unauthorized;
-
-impl AppError for Unauthorized {
-    fn response(&self) -> Option<AppResponse> {
-        let detail = "must be logged in to perform that action";
-        Some(json_error(detail, StatusCode::FORBIDDEN))
-    }
-}
-
-impl fmt::Display for Unauthorized {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        "must be logged in to perform that action".fmt(f)
-    }
-}
-
 pub fn internal<S: ToString + ?Sized>(error: &S) -> Box<dyn AppError> {
     Box::new(InternalAppError {
         description: error.to_string(),
@@ -277,59 +228,6 @@ pub(crate) fn std_error(e: Box<dyn AppError>) -> Box<dyn Error + Send> {
     Box::new(AppErrToStdErr(e))
 }
 
-#[derive(Debug, Clone, Copy)]
-pub struct ReadOnlyMode;
-
-impl AppError for ReadOnlyMode {
-    fn response(&self) -> Option<AppResponse> {
-        let detail = "Crates.io is currently in read-only mode for maintenance. \
-                      Please try again later.";
-        Some(json_error(detail, StatusCode::SERVICE_UNAVAILABLE))
-    }
-}
-
-impl fmt::Display for ReadOnlyMode {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        "Tried to write in read only mode".fmt(f)
-    }
-}
-
-#[derive(Debug, Clone, Copy)]
-pub struct TooManyRequests {
-    pub retry_after: NaiveDateTime,
-}
-
-impl AppError for TooManyRequests {
-    fn response(&self) -> Option<AppResponse> {
-        use std::convert::TryInto;
-
-        const HTTP_DATE_FORMAT: &str = "%a, %d %b %Y %H:%M:%S GMT";
-        let retry_after = self.retry_after.format(HTTP_DATE_FORMAT);
-
-        let detail = format!(
-            "You have published too many crates in a \
-             short period of time. Please try again after {} or email \
-             [email protected] to have your limit increased.",
-            retry_after
-        );
-        let mut response = json_error(&detail, StatusCode::TOO_MANY_REQUESTS);
-        response.headers_mut().insert(
-            header::RETRY_AFTER,
-            retry_after
-                .to_string()
-                .try_into()
-                .expect("HTTP_DATE_FORMAT contains invalid char"),
-        );
-        Some(response)
-    }
-}
-
-impl fmt::Display for TooManyRequests {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        "Too many requests".fmt(f)
-    }
-}
-
 #[test]
 fn chain_error_internal() {
     assert_eq!(
@@ -358,7 +256,7 @@ fn chain_error_internal() {
         "outer caused by inner"
     );
     assert_eq!(
-        Err::<(), _>(Unauthorized)
+        Err::<(), _>(forbidden())
             .chain_error(|| internal("outer"))
             .unwrap_err()
             .to_string(),