Permalink
Browse files

refactor(api): reduce the number of different error kinds we return

The error-handling in this repo grew pretty organically without us
giving much thought to an over-arching strategy. The end result of that
was a proliferation of different error types that weren't really adding
much value.

The API here is simple and the 4xx responses are really limited to bad
requests and bounce/complaint limit violations. Everything else is an
internal server error. As such, the `error` module could be simplified
by employing a blanket `Internal` error type to cover the multitude of
niche errors that should never occur during normal conditions. The
detail for those errors is still available in the `message` property and
Sentry will still log the backtrace of course.

I also noticed that the Rocket catcher stuff wasn't really being used,
so pulled that out too. We have a standard JSON error format like the
rest of the FxA ecosystem, and we always want the response payload to be
the serialisation of that structure.

One side-effect of all these changes is that the errno value has changed
in most cases. However, I took care to preserve the errno for the bounce
and complaint violations, because it's hard-coded in the auth server. It
wouldn't have been that onerous to open a PR for commensurate changes
over there, but the number of errors left here worked out in such a way
that made sense not to bother.
  • Loading branch information...
philbooth committed Nov 8, 2018
1 parent 1ef7392 commit 664e5958a01e4df43239b38f24ffd47389e64d69
@@ -31,12 +31,9 @@ fn lbheartbeat() -> Json {
#[get("/__heartbeat__")]
fn heartbeat(settings: State<Settings>) -> AppResult<Json> {
let db = RequestClient::new()
RequestClient::new()
.get(&format!("{}__heartbeat__", settings.authdb.baseuri))
.send();
match db {
Ok(_) => Ok(Json(json!({}))),
Err(err) => Err(AppErrorKind::AuthDbError(format!("{}", err)).into()),
}
.send()
.map(|_| Ok(Json(json!({}))))
.map_err(|error| AppErrorKind::Internal(format!("{:?}", error)))?
}
@@ -66,7 +66,7 @@ fn unsuccessful_heartbeat() {
let body: Value =
serde_json::from_str(&response.body().unwrap().into_string().unwrap()).unwrap();
assert_eq!(body["code"], 500);
assert_eq!(body["errno"], 109);
assert_eq!(body["errno"], 100);
assert_eq!(body["error"], "Internal Server Error");
assert_eq!(response.status(), Status::InternalServerError);
}
@@ -49,7 +49,7 @@ impl FromData for Email {
Outcome::Success(json) => Outcome::Success(json.into_inner()),
Outcome::Failure((_status, error)) => Outcome::Failure((
Status::BadRequest,
AppErrorKind::MissingEmailParams(error.to_string()).into(),
AppErrorKind::InvalidPayload(error.to_string()).into(),
)),
Outcome::Forward(forward) => Outcome::Forward(forward),
}
@@ -3,7 +3,6 @@
// file, you can obtain one at https://mozilla.org/MPL/2.0/.
use rocket::{
self,
http::{ContentType, Status},
local::Client,
};
@@ -12,7 +11,7 @@ use db::{auth_db::DbClient, delivery_problems::DeliveryProblems, message_data::M
use logging::MozlogLogger;
use providers::Providers;
use settings::Settings;
use types::error::{self, AppError, AppErrorKind};
use types::error::{AppError, AppErrorKind};
fn setup() -> Client {
let mut settings = Settings::new().unwrap();
@@ -28,15 +27,7 @@ fn setup() -> Client {
.manage(logger)
.manage(message_data)
.manage(providers)
.mount("/", routes![super::handler])
.catch(catchers![
error::bad_request,
error::not_found,
error::method_not_allowed,
error::unprocessable_entity,
error::too_many_requests,
error::internal_server_error
]);
.mount("/", routes![super::handler]);
Client::new(server).unwrap()
}
@@ -217,7 +208,8 @@ fn missing_to_field() {
assert_eq!(response.status(), Status::BadRequest);
let body = response.body().unwrap().into_string().unwrap();
let error: AppError = AppErrorKind::MissingEmailParams(String::from("")).into();
let error: AppError =
AppErrorKind::InvalidPayload(String::from("missing field `to` at line 7 column 5")).into();
let expected = serde_json::to_string(&error).unwrap();
assert_eq!(body, expected);
}
@@ -231,7 +223,7 @@ fn missing_subject_field() {
.header(ContentType::JSON)
.body(
r#"{
"to": [ "foo@example.com" ],
"to": "foo@example.com",
"body": {
"text": "baz"
},
@@ -243,7 +235,9 @@ fn missing_subject_field() {
assert_eq!(response.status(), Status::BadRequest);
let body = response.body().unwrap().into_string().unwrap();
let error: AppError = AppErrorKind::MissingEmailParams(String::from("")).into();
let error: AppError =
AppErrorKind::InvalidPayload(String::from("missing field `subject` at line 7 column 5"))
.into();
let expected = serde_json::to_string(&error).unwrap();
assert_eq!(body, expected);
}
@@ -257,7 +251,7 @@ fn missing_body_text_field() {
.header(ContentType::JSON)
.body(
r#"{
"to": [ "foo@example.com" ],
"to": "foo@example.com",
"subject": "bar",
"body": {
"html": "<a>qux</a>"
@@ -270,7 +264,9 @@ fn missing_body_text_field() {
assert_eq!(response.status(), Status::BadRequest);
let body = response.body().unwrap().into_string().unwrap();
let error: AppError = AppErrorKind::MissingEmailParams(String::from("")).into();
let error: AppError =
AppErrorKind::InvalidPayload(String::from("missing field `text` at line 6 column 7"))
.into();
let expected = serde_json::to_string(&error).unwrap();
assert_eq!(body, expected);
}
@@ -297,7 +293,10 @@ fn invalid_to_field() {
assert_eq!(response.status(), Status::BadRequest);
let body = response.body().unwrap().into_string().unwrap();
let error: AppError = AppErrorKind::MissingEmailParams(String::from("")).into();
let error: AppError = AppErrorKind::InvalidPayload(String::from(
"invalid type: sequence, expected a string at line 2 column 12",
))
.into();
let expected = serde_json::to_string(&error).unwrap();
assert_eq!(body, expected);
}
@@ -311,7 +310,7 @@ fn invalid_cc_field() {
.header(ContentType::JSON)
.body(
r#"{
"to": [ "foo@example.com" ],
"to": "foo@example.com",
"cc": [ "bar" ],
"subject": "baz",
"body": {
@@ -325,7 +324,10 @@ fn invalid_cc_field() {
assert_eq!(response.status(), Status::BadRequest);
let body = response.body().unwrap().into_string().unwrap();
let error: AppError = AppErrorKind::MissingEmailParams(String::from("")).into();
let error: AppError = AppErrorKind::InvalidPayload(String::from(
"invalid value: string \"bar\", expected email address at line 3 column 21",
))
.into();
let expected = serde_json::to_string(&error).unwrap();
assert_eq!(body, expected);
}
@@ -29,7 +29,6 @@ use fxa_email_service::{
logging::MozlogLogger,
providers::Providers,
settings::Settings,
types::error,
};
fn main() {
@@ -74,14 +73,6 @@ fn main() {
healthcheck::version
],
)
.catch(catchers![
error::bad_request,
error::not_found,
error::method_not_allowed,
error::unprocessable_entity,
error::too_many_requests,
error::internal_server_error
])
.attach(rocket::fairing::AdHoc::on_request(|request, _| {
let log =
MozlogLogger::with_request(request).expect("MozlogLogger::with_request error");
@@ -19,15 +19,15 @@
use std::fmt::Debug;
use hex;
use reqwest::{Client as RequestClient, Error as RequestError, StatusCode, Url, UrlError};
use reqwest::{Client as RequestClient, StatusCode, Url, UrlError};
use super::delivery_problems::{
LegacyDeliveryProblem as DeliveryProblem, ProblemSubtype, ProblemType,
};
use settings::Settings;
use types::{
email_address::EmailAddress,
error::{AppError, AppErrorKind, AppResult},
error::{AppErrorKind, AppResult},
};
#[cfg(test)]
@@ -42,7 +42,9 @@ pub trait Db: Debug + Sync {
_problem_type: ProblemType,
_problem_subtype: ProblemSubtype,
) -> AppResult<()> {
Err(AppErrorKind::NotImplemented.into())
Err(AppErrorKind::NotImplemented(
"db::auth_db::Db::create_bounce".to_owned(),
))?
}
}
@@ -65,11 +67,21 @@ impl Db for DbClient {
fn get_bounces(&self, address: &EmailAddress) -> AppResult<Vec<DeliveryProblem>> {
let mut response = self
.request_client
.get(self.urls.get_bounces(address)?)
.send()?;
.get(
self.urls
.get_bounces(address)
.map_err(|error| AppErrorKind::Internal(format!("{:?}", error)))?,
)
.send()
.map_err(|error| AppErrorKind::Internal(format!("{:?}", error)))?;
match response.status() {
StatusCode::Ok => response.json::<Vec<DeliveryProblem>>().map_err(From::from),
status => Err(AppErrorKind::AuthDbError(format!("{}", status)).into()),
StatusCode::Ok => response
.json::<Vec<DeliveryProblem>>()
.map_err(|error| AppErrorKind::Internal(format!("{:?}", error)).into()),
status => Err(AppErrorKind::Internal(format!(
"Auth db get_bounces response: {}",
status
)))?,
}
}
@@ -88,10 +100,14 @@ impl Db for DbClient {
problem_subtype,
created_at: 0,
})
.send()?;
.send()
.map_err(|error| AppErrorKind::Internal(format!("{:?}", error)))?;
match response.status() {
StatusCode::Ok => Ok(()),
status => Err(AppErrorKind::AuthDbError(format!("{}", status)).into()),
status => Err(AppErrorKind::Internal(format!(
"Auth db create_bounce response: {}",
status
)))?,
}
}
}
@@ -124,15 +140,3 @@ impl DbUrls {
self.create_bounce.clone()
}
}
impl From<UrlError> for AppError {
fn from(error: UrlError) -> AppError {
AppErrorKind::AuthDbError(format!("{}", error)).into()
}
}
impl From<RequestError> for AppError {
fn from(error: RequestError) -> AppError {
AppErrorKind::AuthDbError(format!("{}", error)).into()
}
}
@@ -61,7 +61,8 @@ impl Client {
.map_err(From::from)
.and_then(|value: Option<String>| {
value.map_or(Ok(None), |value| {
serde_json::from_str(&value).map_err(From::from)
serde_json::from_str(&value)
.map_err(|error| AppErrorKind::Internal(format!("{:?}", error)).into())
})
})
}
@@ -79,7 +80,8 @@ impl Client {
.and_then(|value: Option<String>| {
value.map_or(Ok(None), |value| {
self.client.del::<&str, u8>(key_str).ok();
serde_json::from_str(&value).map_err(From::from)
serde_json::from_str(&value)
.map_err(|error| AppErrorKind::Internal(format!("{:?}", error)).into())
})
})
}
@@ -93,9 +95,9 @@ impl Client {
D: Serialize,
{
let key = self.generate_key(key, data_type)?;
self.client
.set(key.as_str(), serde_json::to_string(data)?)
.map_err(From::from)
let value = serde_json::to_string(data)
.map_err(|error| AppErrorKind::Internal(format!("{:?}", error)).into(): AppError)?;
self.client.set(key.as_str(), value).map_err(From::from)
}
fn generate_key(&self, key: &str, data_type: DataType) -> AppResult<String> {
@@ -131,12 +133,12 @@ impl Display for DataType {
impl From<RedisError> for AppError {
fn from(error: RedisError) -> AppError {
AppErrorKind::DbError(format!("{:?}", error)).into()
AppErrorKind::Internal(format!("{:?}", error)).into()
}
}
impl From<InvalidKeyLength> for AppError {
fn from(_error: InvalidKeyLength) -> AppError {
AppErrorKind::HmacError("invalid key length".to_string()).into()
fn from(error: InvalidKeyLength) -> AppError {
AppErrorKind::Internal(format!("{:?}", error)).into()
}
}
@@ -80,24 +80,21 @@ where
};
if is_limit_violation(*count, problem.created_at, timestamp, limits) {
return match problem.problem_type {
ProblemType::HardBounce => Err(AppErrorKind::BounceHardError {
ProblemType::HardBounce => Err(AppErrorKind::HardBounce {
address: address.clone(),
time: problem.created_at,
problem: From::from(problem.clone()),
}
.into()),
ProblemType::SoftBounce => Err(AppErrorKind::BounceSoftError {
})?,
ProblemType::SoftBounce => Err(AppErrorKind::SoftBounce {
address: address.clone(),
time: problem.created_at,
problem: From::from(problem.clone()),
}
.into()),
ProblemType::Complaint => Err(AppErrorKind::ComplaintError {
})?,
ProblemType::Complaint => Err(AppErrorKind::Complaint {
address: address.clone(),
time: problem.created_at,
problem: From::from(problem.clone()),
}
.into()),
})?,
};
}
}
Oops, something went wrong.

0 comments on commit 664e595

Please sign in to comment.