Merge pull request #221 from mozilla/pb/tidy-app-errors

https://github.com/mozilla/fxa-email-service/pull/221 r=rfk
2018-11-01 21:13:34 +00:00 · 2018-11-01 21:13:34 +00:00 · f5023c2f23
--- a/src/api/send/test.rs
+++ b/src/api/send/test.rs
@ -218,7 +218,8 @@ fn missing_to_field() {

    let body = response.body().unwrap().into_string().unwrap();
    let error: AppError = AppErrorKind::MissingEmailParams(String::from("")).into();
-    assert_eq!(body, error.json().to_string());
+    let expected = serde_json::to_string(&error).unwrap();
+    assert_eq!(body, expected);
 }

 #[test]
@ -243,7 +244,8 @@ fn missing_subject_field() {

    let body = response.body().unwrap().into_string().unwrap();
    let error: AppError = AppErrorKind::MissingEmailParams(String::from("")).into();
-    assert_eq!(body, error.json().to_string());
+    let expected = serde_json::to_string(&error).unwrap();
+    assert_eq!(body, expected);
 }

 #[test]
@ -269,7 +271,8 @@ fn missing_body_text_field() {

    let body = response.body().unwrap().into_string().unwrap();
    let error: AppError = AppErrorKind::MissingEmailParams(String::from("")).into();
-    assert_eq!(body, error.json().to_string());
+    let expected = serde_json::to_string(&error).unwrap();
+    assert_eq!(body, expected);
 }

 #[test]
@ -295,7 +298,8 @@ fn invalid_to_field() {

    let body = response.body().unwrap().into_string().unwrap();
    let error: AppError = AppErrorKind::MissingEmailParams(String::from("")).into();
-    assert_eq!(body, error.json().to_string());
+    let expected = serde_json::to_string(&error).unwrap();
+    assert_eq!(body, expected);
 }

 #[test]
@ -322,5 +326,6 @@ fn invalid_cc_field() {

    let body = response.body().unwrap().into_string().unwrap();
    let error: AppError = AppErrorKind::MissingEmailParams(String::from("")).into();
-    assert_eq!(body, error.json().to_string());
+    let expected = serde_json::to_string(&error).unwrap();
+    assert_eq!(body, expected);
 }
--- a/src/db/delivery_problems/test.rs
+++ b/src/db/delivery_problems/test.rs
@ -108,25 +108,22 @@ fn check_soft_bounce() {
    let settings = create_settings(bounce_settings);
    let db = DbMockBounceSoft;
    let problems = DeliveryProblems::new(&settings, db);
-    match problems.check(&"foo@example.com".parse().unwrap()) {
-        Ok(_) => assert!(false, "DeliveryProblems::check should have failed"),
-        Err(error) => {
-            assert_eq!(format!("{}", error), "Email account soft bounced");
-            let err_data = error.kind().additional_fields();
-            let address = err_data.get("address");
-            if let Some(ref address) = address {
-                assert_eq!("foo@example.com", address.as_str().unwrap());
-            } else {
-                assert!(false, "Error::address should be set");
-            }
-            let problem = err_data.get("problem");
-            assert!(problem.is_some());
-            let record: Json = serde_json::from_str(problem.unwrap().as_str().unwrap()).unwrap();
-            assert_eq!(record["problem_type"], 2);
-            assert_eq!(&record["created_at"], err_data.get("time").unwrap());
-            assert_eq!(error.kind().http_status(), Status::TooManyRequests);
-        }
-    }
+    let result = problems.check(&"foo@example.com".parse().unwrap());
+    assert!(result.is_err());
+    let error = result.unwrap_err();
+    assert_eq!(error.code(), 429);
+    assert_eq!(error.errno().unwrap(), 107);
+    assert_eq!(error.error(), "Too Many Requests");
+    assert_eq!(error.to_string(), "Email account soft bounced");
+    let additional_fields = error.additional_fields();
+    assert_eq!(additional_fields.get("address").unwrap(), "foo@example.com");
+    let record: DeliveryProblem =
+        serde_json::from_str(additional_fields.get("problem").unwrap().as_str().unwrap()).unwrap();
+    assert_eq!(record.problem_type, ProblemType::SoftBounce);
+    assert_eq!(
+        record.created_at,
+        additional_fields.get("time").unwrap().as_u64().unwrap()
+    );
 }

 #[derive(Debug)]
@ -160,25 +157,22 @@ fn check_hard_bounce() {
    let settings = create_settings(bounce_settings);
    let db = DbMockBounceHard;
    let problems = DeliveryProblems::new(&settings, db);
-    match problems.check(&"bar@example.com".parse().unwrap()) {
-        Ok(_) => assert!(false, "DeliveryProblems::check should have failed"),
-        Err(error) => {
-            assert_eq!(format!("{}", error), "Email account hard bounced");
-            let err_data = error.kind().additional_fields();
-            let address = err_data.get("address");
-            if let Some(ref address) = address {
-                assert_eq!("bar@example.com", address.as_str().unwrap());
-            } else {
-                assert!(false, "Error::address should be set");
-            }
-            let problem = err_data.get("problem");
-            assert!(problem.is_some());
-            let record: Json = serde_json::from_str(problem.unwrap().as_str().unwrap()).unwrap();
-            assert_eq!(record["problem_type"], 1);
-            assert_eq!(&record["created_at"], err_data.get("time").unwrap());
-            assert_eq!(error.kind().http_status(), Status::TooManyRequests);
-        }
-    }
+    let result = problems.check(&"bar@example.com".parse().unwrap());
+    assert!(result.is_err());
+    let error = result.unwrap_err();
+    assert_eq!(error.code(), 429);
+    assert_eq!(error.errno().unwrap(), 108);
+    assert_eq!(error.error(), "Too Many Requests");
+    assert_eq!(error.to_string(), "Email account hard bounced");
+    let additional_fields = error.additional_fields();
+    assert_eq!(additional_fields.get("address").unwrap(), "bar@example.com");
+    let record: DeliveryProblem =
+        serde_json::from_str(additional_fields.get("problem").unwrap().as_str().unwrap()).unwrap();
+    assert_eq!(record.problem_type, ProblemType::HardBounce);
+    assert_eq!(
+        record.created_at,
+        additional_fields.get("time").unwrap().as_u64().unwrap()
+    );
 }

 #[derive(Debug)]
@ -212,25 +206,22 @@ fn check_complaint() {
    let settings = create_settings(bounce_settings);
    let db = DbMockComplaint;
    let problems = DeliveryProblems::new(&settings, db);
-    match problems.check(&"baz@example.com".parse().unwrap()) {
-        Ok(_) => assert!(false, "DeliveryProblems::check should have failed"),
-        Err(error) => {
-            assert_eq!(format!("{}", error), "Email account sent complaint");
-            let err_data = error.kind().additional_fields();
-            let address = err_data.get("address");
-            if let Some(ref address) = address {
-                assert_eq!("baz@example.com", address.as_str().unwrap());
-            } else {
-                assert!(false, "Error::address should be set");
-            }
-            let problem = err_data.get("problem");
-            assert!(problem.is_some());
-            let record: Json = serde_json::from_str(problem.unwrap().as_str().unwrap()).unwrap();
-            assert_eq!(record["problem_type"], 3);
-            assert_eq!(&record["created_at"], err_data.get("time").unwrap());
-            assert_eq!(error.kind().http_status(), Status::TooManyRequests);
-        }
-    }
+    let result = problems.check(&"baz@example.com".parse().unwrap());
+    assert!(result.is_err());
+    let error = result.unwrap_err();
+    assert_eq!(error.code(), 429);
+    assert_eq!(error.errno().unwrap(), 106);
+    assert_eq!(error.error(), "Too Many Requests");
+    assert_eq!(error.to_string(), "Email account sent complaint");
+    let additional_fields = error.additional_fields();
+    assert_eq!(additional_fields.get("address").unwrap(), "baz@example.com");
+    let record: DeliveryProblem =
+        serde_json::from_str(additional_fields.get("problem").unwrap().as_str().unwrap()).unwrap();
+    assert_eq!(record.problem_type, ProblemType::Complaint);
+    assert_eq!(
+        record.created_at,
+        additional_fields.get("time").unwrap().as_u64().unwrap()
+    );
 }

 #[derive(Debug)]
@ -268,13 +259,14 @@ fn check_db_error() {
    let settings = create_settings(bounce_settings);
    let db = DbMockError;
    let problems = DeliveryProblems::new(&settings, db);
-    match problems.check(&"foo@example.com".parse().unwrap()) {
-        Ok(_) => assert!(false, "DeliveryProblems::check should have failed"),
-        Err(error) => {
-            assert_eq!(format!("{}", error), "wibble blee");
-            assert_eq!(error.kind().http_status(), Status::InternalServerError);
-        }
-    }
+    let result = problems.check(&"foo@example.com".parse().unwrap());
+    assert!(result.is_err());
+    let error = result.unwrap_err();
+    assert_eq!(error.code(), 500);
+    assert_eq!(error.errno().unwrap(), 109);
+    assert_eq!(error.error(), "Internal Server Error");
+    assert_eq!(error.to_string(), "wibble blee");
+    assert_eq!(error.additional_fields().len(), 0);
 }

 #[derive(Debug)]
@ -394,24 +386,18 @@ fn check_bounce_with_multiple_limits() {
    let settings = create_settings(bounce_settings);
    let db = DbMockBounceWithMultipleLimits;
    let problems = DeliveryProblems::new(&settings, db);
-    match problems.check(&"foo@example.com".parse().unwrap()) {
-        Ok(_) => assert!(false, "DeliveryProblems::check should have failed"),
-        Err(error) => {
-            assert_eq!(format!("{}", error), "Email account soft bounced");
-            let err_data = error.kind().additional_fields();
-            let address = err_data.get("address");
-            if let Some(ref address) = address {
-                assert_eq!("foo@example.com", address.as_str().unwrap());
-            } else {
-                assert!(false, "Error::address should be set");
-            }
-            let problem = err_data.get("problem");
-            assert!(problem.is_some());
-            let record: Json = serde_json::from_str(problem.unwrap().as_str().unwrap()).unwrap();
-            assert_eq!(record["problem_type"], 2);
-            assert_eq!(&record["created_at"], err_data.get("time").unwrap());
-        }
-    }
+    let result = problems.check(&"foo@example.com".parse().unwrap());
+    assert!(result.is_err());
+    let error = result.unwrap_err();
+    assert_eq!(error.code(), 429);
+    assert_eq!(error.errno().unwrap(), 107);
+    assert_eq!(error.error(), "Too Many Requests");
+    assert_eq!(error.to_string(), "Email account soft bounced");
+    let additional_fields = error.additional_fields();
+    assert_eq!(additional_fields.get("address").unwrap(), "foo@example.com");
+    let record: DeliveryProblem =
+        serde_json::from_str(additional_fields.get("problem").unwrap().as_str().unwrap()).unwrap();
+    assert_eq!(record.problem_type, ProblemType::SoftBounce);
 }

 #[derive(Debug)]
--- a/src/logging/mod.rs
+++ b/src/logging/mod.rs
@ -64,7 +64,7 @@ impl KV for RequestMozlogFields {
 #[derive(Clone)]
 struct AppErrorFields {
    code: u16,
-    error: String,
+    error: &'static str,
    errno: Option<u16>,
    message: String,
    additional_fields: Map<String, JsonValue>,
@ -72,15 +72,12 @@ struct AppErrorFields {

 impl AppErrorFields {
    pub fn from_app_error(error: &AppError) -> AppErrorFields {
-        let kind = error.kind();
-        let status = kind.http_status();
-
        AppErrorFields {
-            code: status.code,
-            error: status.reason.to_string(),
-            errno: kind.errno(),
-            message: format!("{}", error),
-            additional_fields: kind.additional_fields(),
+            code: error.code(),
+            error: error.error(),
+            errno: error.errno(),
+            message: error.to_string(),
+            additional_fields: error.additional_fields(),
        }
    }
 }
--- a/src/providers/ses/test.rs
+++ b/src/providers/ses/test.rs
@ -83,25 +83,12 @@ fn ses_send_handles_error_response() {
        )),
        sender: "Foo Bar <a@a.com>".to_string(),
    };
-    match mock_ses.send("b@b.com", &[], None, "subject", "body", None) {
-        Ok(_) => assert!(false, "Request should have failed"),
-        Err(error) => {
-            let error = error.json();
-            assert_eq!(
-                500,
-                error["code"]
-                    .as_u64()
-                    .expect("Error code should be a number")
-            );
-            assert_eq!(
-                104,
-                error["errno"]
-                    .as_u64()
-                    .expect("Error errno should be a number")
-            );
-            assert_eq!("Internal Server Error", &error["error"]);
-            assert_eq!("Unknown(\"FREAKOUT\")", &error["message"]);
-            assert_eq!("SES", &error["name"]);
-        }
-    }
+    let result = mock_ses.send("b@b.com", &[], None, "subject", "body", None);
+    assert!(result.is_err());
+    let error = result.unwrap_err();
+    assert_eq!(error.code(), 500);
+    assert_eq!(error.errno().unwrap(), 104);
+    assert_eq!(error.error(), "Internal Server Error");
+    assert_eq!(error.to_string(), "Unknown(\"FREAKOUT\")");
+    assert_eq!(error.additional_fields().get("name").unwrap(), "SES");
 }
--- a/src/types/error/mod.rs
+++ b/src/types/error/mod.rs
@ -4,7 +4,10 @@

 //! Error definitions.

-use std::{fmt, result};
+use std::{
+    fmt::{self, Display, Formatter},
+    result,
+};

 use failure::{Backtrace, Context, Fail};
 use rocket::{
@ -14,6 +17,7 @@ use rocket::{
    Outcome, Request, State,
 };
 use rocket_contrib::Json;
+use serde::ser::{Serialize, SerializeMap, Serializer};
 use serde_json::{map::Map, ser::to_string, Value};

 use super::email_address::EmailAddress;
@ -39,37 +43,26 @@ pub struct AppError {
 }

 impl AppError {
-    pub fn json(&self) -> Value {
-        let kind = self.kind();
-        let status = kind.http_status();
-
-        let mut fields = Map::new();
-        fields.insert(
-            String::from("code"),
-            Value::Number(status.code.to_owned().into()),
-        );
-        fields.insert(
-            String::from("error"),
-            Value::String(format!("{}", status.reason)),
-        );
-        let errno = kind.errno();
-        if let Some(ref errno) = errno {
-            fields.insert(
-                String::from("errno"),
-                Value::Number(errno.to_owned().into()),
-            );
-            fields.insert(String::from("message"), Value::String(format!("{}", self)));
-        };
-        let additional_fields = kind.additional_fields();
-        for (field, value) in additional_fields.iter() {
-            fields.insert(field.clone(), value.clone());
-        }
-
-        json!(fields)
+    pub fn code(&self) -> u16 {
+        self.inner.get_context().http_status().code
    }

-    pub fn kind(&self) -> &AppErrorKind {
-        self.inner.get_context()
+    pub fn error(&self) -> &'static str {
+        self.inner.get_context().http_status().reason
+    }
+
+    pub fn errno(&self) -> Option<u16> {
+        self.inner.get_context().errno()
+    }
+
+    pub fn additional_fields(&self) -> Map<String, Value> {
+        self.inner.get_context().additional_fields()
+    }
+}
+
+impl Display for AppError {
+    fn fmt(&self, formatter: &mut Formatter) -> fmt::Result {
+        Display::fmt(&self.inner, formatter)
    }
 }

@ -83,9 +76,61 @@ impl Fail for AppError {
    }
 }

-impl fmt::Display for AppError {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        self.inner.fmt(f)
+impl From<AppErrorKind> for AppError {
+    fn from(kind: AppErrorKind) -> AppError {
+        Context::new(kind).into()
+    }
+}
+
+impl From<Context<AppErrorKind>> for AppError {
+    fn from(inner: Context<AppErrorKind>) -> AppError {
+        AppError { inner }
+    }
+}
+
+impl Serialize for AppError {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: Serializer,
+    {
+        let kind = self.inner.get_context();
+
+        let mut map = serializer.serialize_map(None)?;
+
+        let status = kind.http_status();
+        map.serialize_entry("code", &status.code)?;
+        map.serialize_entry("error", status.reason)?;
+
+        if let Some(ref errno) = kind.errno() {
+            map.serialize_entry("errno", errno)?;
+        }
+
+        map.serialize_entry("message", &self.to_string())?;
+
+        for (field, value) in kind.additional_fields().iter() {
+            map.serialize_entry(field, value)?;
+        }
+
+        map.end()
+    }
+}
+
+/// Generate HTTP error responses for AppErrors
+impl<'r> Responder<'r> for AppError {
+    fn respond_to(self, request: &Request) -> response::Result<'r> {
+        match request.guard::<State<MozlogLogger>>() {
+            Outcome::Success(logger) => {
+                let log = MozlogLogger::with_app_error(&logger, &self)
+                    .map_err(|_| Status::InternalServerError)?;
+                slog_error!(log, "{}", "Request errored");
+            }
+            _ => panic!("Internal error: No managed MozlogLogger"),
+        }
+
+        let status = self.inner.get_context().http_status();
+        let json = Json(self);
+        let mut builder = Response::build_from(json.respond_to(request)?);
+        builder.status(status).ok()
    }
 }

@ -220,7 +265,7 @@ impl AppErrorKind {
            | AppErrorKind::BounceHardError { .. } => Status::TooManyRequests,
            AppErrorKind::BadRequest | AppErrorKind::InvalidEmailParams => Status::BadRequest,
            AppErrorKind::MissingEmailParams(_) => Status::BadRequest,
-            AppErrorKind::InternalServerError | _ => Status::InternalServerError,
+            _ => Status::InternalServerError,
        }
    }

@ -331,37 +376,6 @@ impl AppErrorKind {
    }
 }

-impl From<AppErrorKind> for AppError {
-    fn from(kind: AppErrorKind) -> AppError {
-        Context::new(kind).into()
-    }
-}
-
-impl From<Context<AppErrorKind>> for AppError {
-    fn from(inner: Context<AppErrorKind>) -> AppError {
-        AppError { inner }
-    }
-}
-
-/// Generate HTTP error responses for AppErrors
-impl<'r> Responder<'r> for AppError {
-    fn respond_to(self, request: &Request) -> response::Result<'r> {
-        match request.guard::<State<MozlogLogger>>() {
-            Outcome::Success(logger) => {
-                let log = MozlogLogger::with_app_error(&logger, &self)
-                    .map_err(|_| Status::InternalServerError)?;
-                slog_error!(log, "{}", "Request errored");
-            }
-            _ => panic!("Internal error: No managed MozlogLogger"),
-        }
-
-        let status = self.kind().http_status();
-        let json = Json(self.json());
-        let mut builder = Response::build_from(json.respond_to(request)?);
-        builder.status(status).ok()
-    }
-}
-
 #[catch(400)]
 pub fn bad_request() -> AppResult<()> {
    Err(AppErrorKind::BadRequest)?
--- a/src/types/error/test.rs
+++ b/src/types/error/test.rs
@ -2,52 +2,58 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, you can obtain one at https://mozilla.org/MPL/2.0/.

-use super::AppErrorKind;
+use super::*;

 #[test]
 fn bad_request() {
-    assert_eq!(
-        format!("{}", super::bad_request().unwrap_err().kind()),
-        format!("{}", AppErrorKind::BadRequest)
-    );
+    let error: AppError = AppErrorKind::BadRequest.into();
+    assert_eq!(error.code(), 400);
+    assert_eq!(error.error(), "Bad Request");
+    assert!(error.errno().is_none());
+    assert_eq!(error.additional_fields().len(), 0);
 }

 #[test]
 fn not_found() {
-    assert_eq!(
-        format!("{}", super::not_found().unwrap_err().kind()),
-        format!("{}", AppErrorKind::NotFound)
-    );
+    let error: AppError = AppErrorKind::NotFound.into();
+    assert_eq!(error.code(), 404);
+    assert_eq!(error.error(), "Not Found");
+    assert!(error.errno().is_none());
+    assert_eq!(error.additional_fields().len(), 0);
 }

 #[test]
 fn method_not_allowed() {
-    assert_eq!(
-        format!("{}", super::method_not_allowed().unwrap_err().kind()),
-        format!("{}", AppErrorKind::MethodNotAllowed)
-    );
+    let error: AppError = AppErrorKind::MethodNotAllowed.into();
+    assert_eq!(error.code(), 405);
+    assert_eq!(error.error(), "Method Not Allowed");
+    assert!(error.errno().is_none());
+    assert_eq!(error.additional_fields().len(), 0);
 }

 #[test]
 fn unprocessable_entity() {
-    assert_eq!(
-        format!("{}", super::unprocessable_entity().unwrap_err().kind()),
-        format!("{}", AppErrorKind::UnprocessableEntity)
-    );
+    let error: AppError = AppErrorKind::UnprocessableEntity.into();
+    assert_eq!(error.code(), 422);
+    assert_eq!(error.error(), "Unprocessable Entity");
+    assert!(error.errno().is_none());
+    assert_eq!(error.additional_fields().len(), 0);
 }

 #[test]
 fn too_many_requests() {
-    assert_eq!(
-        format!("{}", super::too_many_requests().unwrap_err().kind()),
-        format!("{}", AppErrorKind::TooManyRequests)
-    );
+    let error: AppError = AppErrorKind::TooManyRequests.into();
+    assert_eq!(error.code(), 429);
+    assert_eq!(error.error(), "Too Many Requests");
+    assert!(error.errno().is_none());
+    assert_eq!(error.additional_fields().len(), 0);
 }

 #[test]
 fn internal_server_error() {
-    assert_eq!(
-        format!("{}", super::internal_server_error().unwrap_err().kind()),
-        format!("{}", AppErrorKind::InternalServerError)
-    );
+    let error: AppError = AppErrorKind::InternalServerError.into();
+    assert_eq!(error.code(), 500);
+    assert_eq!(error.error(), "Internal Server Error");
+    assert!(error.errno().is_none());
+    assert_eq!(error.additional_fields().len(), 0);
 }