chore: switch failure crate with thiserror (#1122)

* switch fail crate to thiserror

Co-authored-by: JR Conlin <jconlin+git@mozilla.com>
This commit is contained in:
Tif Tran 2021-07-29 17:00:23 -07:00 committed by GitHub
parent 503d1aa81b
commit 5369f1aef4
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 187 additions and 133 deletions

36
Cargo.lock generated
View File

@ -288,9 +288,9 @@ dependencies = [
[[package]]
name = "addr2line"
version = "0.15.2"
version = "0.16.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "e7a2e47a1fbe209ee101dd6d61285226744c6c8d3c21c8dc878ba6cb9f467f3a"
checksum = "3e61f2b7f93d2c7d2b08263acaa4a363b3e276806c68af6134c44f523bf1aacd"
dependencies = [
"gimli",
]
@ -382,9 +382,9 @@ dependencies = [
[[package]]
name = "backtrace"
version = "0.3.60"
version = "0.3.61"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "b7815ea54e4d821e791162e078acbebfd6d8c8939cd559c9335dceb1c8ca7282"
checksum = "e7a905d892734eea339e896738c14b9afce22b5318f64b951e70bf3844419b01"
dependencies = [
"addr2line",
"cc",
@ -768,9 +768,9 @@ dependencies = [
[[package]]
name = "curl-sys"
version = "0.4.44+curl-7.77.0"
version = "0.4.45+curl-7.78.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "4b6d85e9322b193f117c966e79c2d6929ec08c02f339f950044aba12e20bbaf1"
checksum = "de9e5a72b1c744eb5dd20b2be4d7eb84625070bb5c4ab9b347b70464ab1e62eb"
dependencies = [
"cc",
"libc",
@ -1193,9 +1193,9 @@ dependencies = [
[[package]]
name = "gimli"
version = "0.24.0"
version = "0.25.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "0e4075386626662786ddb0ec9081e7c7eeb1ba31951f447ca780ef9f5d568189"
checksum = "f0a01e0497841a3b2db4f8afa483cce65f7e96a3498bd6c541734792aeac8fe7"
[[package]]
name = "glob"
@ -1867,9 +1867,9 @@ dependencies = [
[[package]]
name = "object"
version = "0.25.3"
version = "0.26.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "a38f2be3697a57b4060074ff41b44c16870d916ad7877c17696e063257482bc7"
checksum = "c55827317fb4c08822499848a14237d2874d6f139828893017237e7ab93eb386"
dependencies = [
"memchr",
]
@ -2903,6 +2903,7 @@ dependencies = [
"actix-web",
"actix-web-httpauth",
"async-trait",
"backtrace",
"base64 0.13.0",
"bb8",
"bytes 1.0.1",
@ -2915,7 +2916,6 @@ dependencies = [
"diesel_migrations",
"docopt",
"env_logger",
"failure",
"futures 0.3.15",
"googleapis-raw",
"grpcio",
@ -2934,6 +2934,7 @@ dependencies = [
"regex",
"scheduled-thread-pool",
"sentry",
"sentry-backtrace",
"serde 1.0.126",
"serde_derive",
"serde_json",
@ -2945,6 +2946,7 @@ dependencies = [
"slog-scope",
"slog-stdlog",
"slog-term",
"thiserror",
"time 0.2.27",
"tokio",
"url 2.2.2",
@ -2957,9 +2959,9 @@ dependencies = [
[[package]]
name = "synstructure"
version = "0.12.4"
version = "0.12.5"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "b834f2d66f734cb897113e34aaff2f1ab4719ca946f9a7358dba8f8064148701"
checksum = "474aaa926faa1603c40b7885a9eaea29b444d1cb2850cb7c0e37bb1a4182f4fa"
dependencies = [
"proc-macro2",
"quote",
@ -3009,18 +3011,18 @@ dependencies = [
[[package]]
name = "thiserror"
version = "1.0.25"
version = "1.0.26"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "fa6f76457f59514c7eeb4e59d891395fab0b2fd1d40723ae737d64153392e9c6"
checksum = "93119e4feac1cbe6c798c34d3a53ea0026b0b1de6a120deef895137c0529bfe2"
dependencies = [
"thiserror-impl",
]
[[package]]
name = "thiserror-impl"
version = "1.0.25"
version = "1.0.26"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "8a36768c0fbf1bb15eca10defa29526bda730a2376c2ab4393ccfa16fb1a318d"
checksum = "060d69a0afe7796bf42e9e2ff91f5ee691fb15c53d38b4b62a9a53eb23164745"
dependencies = [
"proc-macro2",
"quote",

View File

@ -21,6 +21,7 @@ actix-web-httpauth = "0.5"
actix-rt = "1" # Pin to 1.0, due to dependencies on Tokio
actix-cors = "0.5"
async-trait = "0.1.40"
backtrace = "0.3.61"
base64 = "0.13"
bb8 = "0.4.1" # pin to 0.4 due to dependencies on Tokio
bytes = "1.0"
@ -36,7 +37,6 @@ diesel_logger = "0.1.1"
diesel_migrations = { version = "1.4.0", features = ["mysql"] }
docopt = "1.1.0"
env_logger = "0.8"
failure = "0.1.8"
futures = { version = "0.3", features = ["compat"] }
googleapis-raw = { version = "0", path = "vendor/mozilla-rust-sdk/googleapis-raw" }
# Some versions of OpenSSL 1.1.1 conflict with grpcio's built-in boringssl which can cause
@ -58,7 +58,8 @@ protobuf = "2.20.0"
rand = "0.8"
regex = "1.4"
# pin to 0.19: https://github.com/getsentry/sentry-rust/issues/277
sentry = { version = "0.19", features = ["with_curl_transport"] } # pin to 0.19 due to curl transport, failure
sentry = { version = "0.19", features = ["with_curl_transport"] }# pin to 0.19 due to curl transport, failure
sentry-backtrace = "0.19"
serde = "1.0"
serde_derive = "1.0"
serde_json = { version = "1.0", features = ["arbitrary_precision"] }
@ -71,6 +72,7 @@ slog-mozlog-json = "0.1"
slog-scope = "4.3"
slog-stdlog = "4.1"
slog-term = "2.6"
thiserror = "1.0.26"
time = "^0.2.25"
# pinning to 0.2.4 due to high number of dependencies (actix, bb8, deadpool, etc.)
tokio = { version = "0.2.4", features = ["macros", "sync"] }

View File

@ -1,71 +1,68 @@
use std::fmt;
use actix_web::http::StatusCode;
use failure::{Backtrace, Context, Fail};
use thiserror::Error;
#[derive(Debug)]
#[derive(Error, Debug)]
pub struct DbError {
inner: Context<DbErrorKind>,
kind: DbErrorKind,
pub status: StatusCode,
}
#[derive(Debug, Fail)]
#[derive(Debug, Error)]
pub enum DbErrorKind {
#[fail(display = "A database error occurred: {}", _0)]
DieselQuery(#[cause] diesel::result::Error),
#[error("A database error occurred: {}", _0)]
DieselQuery(#[from] diesel::result::Error),
#[fail(
display = "An error occurred while establishing a db connection: {}",
_0
)]
DieselConnection(#[cause] diesel::result::ConnectionError),
#[error("An error occurred while establishing a db connection: {}", _0)]
DieselConnection(#[from] diesel::result::ConnectionError),
#[fail(display = "A database error occurred: {}", _0)]
SpannerGrpc(#[cause] grpcio::Error),
#[error("A database error occurred: {}", _0)]
SpannerGrpc(#[from] grpcio::Error),
#[fail(display = "Spanner data load too large: {}", _0)]
#[error("Spanner data load too large: {}", _0)]
SpannerTooLarge(String),
#[fail(display = "A database pool error occurred: {}", _0)]
#[error("A database pool error occurred: {}", _0)]
Pool(diesel::r2d2::PoolError),
#[fail(display = "Error migrating the database: {}", _0)]
#[error("Error migrating the database: {}", _0)]
Migration(diesel_migrations::RunMigrationsError),
#[fail(display = "Specified collection does not exist")]
#[error("Specified collection does not exist")]
CollectionNotFound,
#[fail(display = "Specified bso does not exist")]
#[error("Specified bso does not exist")]
BsoNotFound,
#[fail(display = "Specified batch does not exist")]
#[error("Specified batch does not exist")]
BatchNotFound,
#[fail(display = "Tokenserver user not found")]
#[error("Tokenserver user not found")]
TokenserverUserNotFound,
#[fail(display = "An attempt at a conflicting write")]
#[error("An attempt at a conflicting write")]
Conflict,
#[fail(display = "Database integrity error: {}", _0)]
#[error("Database integrity error: {}", _0)]
Integrity(String),
#[fail(display = "Invalid SYNC_DATABASE_URL: {}", _0)]
#[error("Invalid SYNC_DATABASE_URL: {}", _0)]
InvalidUrl(String),
#[fail(display = "Unexpected error: {}", _0)]
#[error("Unexpected error: {}", _0)]
Internal(String),
#[fail(display = "User over quota")]
#[error("User over quota")]
Quota,
#[fail(display = "Connection expired")]
#[error("Connection expired")]
Expired,
}
impl DbError {
pub fn kind(&self) -> &DbErrorKind {
self.inner.get_context()
&self.kind
}
pub fn internal(msg: &str) -> Self {
@ -73,20 +70,20 @@ impl DbError {
}
pub fn is_reportable(&self) -> bool {
!matches!(self.inner.get_context(), DbErrorKind::Conflict)
!matches!(&self.kind, DbErrorKind::Conflict)
}
pub fn metric_label(&self) -> Option<String> {
match self.inner.get_context() {
match &self.kind {
DbErrorKind::Conflict => Some("storage.conflict".to_owned()),
_ => None,
}
}
}
impl From<Context<DbErrorKind>> for DbError {
fn from(inner: Context<DbErrorKind>) -> Self {
let status = match inner.get_context() {
impl From<DbErrorKind> for DbError {
fn from(kind: DbErrorKind) -> Self {
let status = match kind {
DbErrorKind::TokenserverUserNotFound
| DbErrorKind::CollectionNotFound
| DbErrorKind::BsoNotFound => StatusCode::NOT_FOUND,
@ -102,11 +99,11 @@ impl From<Context<DbErrorKind>> for DbError {
_ => StatusCode::INTERNAL_SERVER_ERROR,
};
Self { inner, status }
Self { kind, status }
}
}
failure_boilerplate!(DbError, DbErrorKind);
impl_fmt_display!(DbError, DbErrorKind);
from_error!(diesel::result::Error, DbError, DbErrorKind::DieselQuery);
from_error!(

View File

@ -192,10 +192,10 @@ impl Default for CollectionCache {
#[derive(Debug, Clone, Copy)]
pub struct LoggingErrorSink;
impl<E: failure::Fail> ErrorSink<E> for LoggingErrorSink {
impl<E: std::error::Error> ErrorSink<E> for LoggingErrorSink {
fn sink(&self, e: E) {
error!("bb8 Error: {}", e);
let event = sentry::integrations::failure::event_from_fail(&e);
let event = sentry::event_from_error(&e);
sentry::capture_event(event);
}

View File

@ -4,7 +4,7 @@
// this cascades into Failure requiring std::error::Error being implemented
// which is out of scope.
#![allow(clippy::single_match, clippy::large_enum_variant)]
use backtrace::Backtrace;
use std::convert::From;
use std::fmt;
@ -15,15 +15,18 @@ use actix_web::{
middleware::errhandlers::ErrorHandlerResponse,
HttpResponse, Result,
};
use failure::{Backtrace, Context, Fail};
use serde::{
ser::{SerializeMap, SerializeSeq, Serializer},
Serialize,
};
use thiserror::Error;
use crate::db::error::{DbError, DbErrorKind};
use crate::web::error::{HawkError, ValidationError, ValidationErrorKind};
use crate::web::extractors::RequestErrorLocation;
use std::error::Error;
/// Legacy Sync 1.1 error codes, which Sync 1.5 also returns by replacing the descriptive JSON
/// information and replacing it with one of these error codes.
@ -53,27 +56,32 @@ pub const RETRY_AFTER: u8 = 10;
/// Top-level error type.
#[derive(Debug)]
pub struct ApiError {
inner: Context<ApiErrorKind>,
kind: ApiErrorKind,
pub(crate) backtrace: Backtrace,
status: StatusCode,
}
/// Top-level ErrorKind.
#[derive(Debug, Fail)]
#[derive(Error, Debug)]
pub enum ApiErrorKind {
#[fail(display = "{}", _0)]
Db(#[cause] DbError),
// Note, `#[from]` applies some derivation to the target error, but can fail
// if the target has any complexity associated with it. It's best to add
// #[derive(thiserror::Error,...)] to the target error to ensure that various
// traits are defined.
#[error("{}", _0)]
Db(DbError),
#[fail(display = "HAWK authentication error: {}", _0)]
Hawk(#[cause] HawkError),
#[error("HAWK authentication error: {}", _0)]
Hawk(HawkError),
#[fail(display = "No app_data ServerState")]
#[error("No app_data ServerState")]
NoServerState,
#[fail(display = "{}", _0)]
#[error("{}", _0)]
Internal(String),
#[fail(display = "{}", _0)]
Validation(#[cause] ValidationError),
#[error("{}", _0)]
Validation(ValidationError),
}
impl ApiErrorKind {
@ -89,7 +97,7 @@ impl ApiErrorKind {
impl ApiError {
pub fn kind(&self) -> &ApiErrorKind {
self.inner.get_context()
&self.kind
}
pub fn is_collection_not_found(&self) -> bool {
@ -199,6 +207,11 @@ impl From<actix_web::error::BlockingError<ApiError>> for ApiError {
}
}
}
impl Error for ApiError {
fn source(&self) -> Option<&(dyn Error + 'static)> {
self.kind.source()
}
}
impl From<ApiError> for HttpResponse {
fn from(inner: ApiError) -> Self {
@ -218,9 +231,9 @@ impl From<std::io::Error> for ApiError {
}
}
impl From<Context<ApiErrorKind>> for ApiError {
fn from(inner: Context<ApiErrorKind>) -> Self {
let status = match inner.get_context() {
impl From<ApiErrorKind> for ApiError {
fn from(kind: ApiErrorKind) -> Self {
let status = match &kind {
ApiErrorKind::Db(error) => error.status,
ApiErrorKind::Hawk(_) => StatusCode::UNAUTHORIZED,
ApiErrorKind::NoServerState | ApiErrorKind::Internal(_) => {
@ -229,7 +242,11 @@ impl From<Context<ApiErrorKind>> for ApiError {
ApiErrorKind::Validation(error) => error.status,
};
Self { inner, status }
Self {
kind,
backtrace: Backtrace::new(),
status,
}
}
}
@ -265,7 +282,7 @@ impl Serialize for ApiError {
map.serialize_entry("reason", self.status.canonical_reason().unwrap_or(""))?;
if self.status != StatusCode::UNAUTHORIZED {
map.serialize_entry("errors", &self.inner.get_context())?;
map.serialize_entry("errors", &self.kind)?;
}
map.end()
@ -301,33 +318,17 @@ where
seq.end()
}
macro_rules! failure_boilerplate {
macro_rules! impl_fmt_display {
($error:ty, $kind:ty) => {
impl Fail for $error {
fn cause(&self) -> Option<&dyn Fail> {
self.inner.cause()
}
fn backtrace(&self) -> Option<&Backtrace> {
self.inner.backtrace()
}
}
impl fmt::Display for $error {
fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
fmt::Display::fmt(&self.inner, formatter)
}
}
impl From<$kind> for $error {
fn from(kind: $kind) -> Self {
Context::new(kind).into()
fmt::Display::fmt(&self.kind, formatter)
}
}
};
}
failure_boilerplate!(ApiError, ApiErrorKind);
impl_fmt_display!(ApiError, ApiErrorKind);
macro_rules! from_error {
($from:ty, $to:ty, $to_kind:expr) => {

View File

@ -5,7 +5,7 @@ use std::fmt;
use actix_web::http::{header::ToStrError, StatusCode};
use actix_web::Error as ActixError;
use base64::DecodeError;
use failure::{Backtrace, Context, Fail};
use hawk::Error as ParseError;
use hmac::crypto_mac::{InvalidKeyLength, MacError};
use serde::{
@ -17,15 +17,17 @@ use serde_json::{Error as JsonError, Value};
use super::extractors::RequestErrorLocation;
use crate::error::ApiError;
use thiserror::Error;
/// An error occurred during HAWK authentication.
#[derive(Debug)]
pub struct HawkError {
inner: Context<HawkErrorKind>,
kind: HawkErrorKind,
}
impl HawkError {
pub fn kind(&self) -> &HawkErrorKind {
self.inner.get_context()
&self.kind
}
pub fn is_reportable(&self) -> bool {
@ -52,58 +54,58 @@ impl HawkError {
}
/// Causes of HAWK errors.
#[derive(Debug, Fail)]
#[derive(Debug, Error)]
pub enum HawkErrorKind {
#[fail(display = "{}", _0)]
Base64(#[cause] DecodeError),
#[error("{}", _0)]
Base64(DecodeError),
#[fail(display = "expired payload")]
#[error("expired payload")]
Expired,
#[fail(display = "{}", _0)]
Header(#[cause] ToStrError),
#[error("{}", _0)]
Header(ToStrError),
#[fail(display = "{}", _0)]
#[error("{}", _0)]
Hmac(MacError),
#[fail(display = "validation failed")]
#[error("validation failed")]
InvalidHeader,
#[fail(display = "{}", _0)]
#[error("{}", _0)]
InvalidKeyLength(InvalidKeyLength),
#[fail(display = "{}", _0)]
Json(#[cause] JsonError),
#[error("{}", _0)]
Json(JsonError),
#[fail(display = "missing header")]
#[error("missing header")]
MissingHeader,
#[fail(display = "missing id property")]
#[error("missing id property")]
MissingId,
#[fail(display = "missing path")]
#[error("missing path")]
MissingPath,
#[fail(display = "missing \"Hawk \" prefix")]
#[error("missing \"Hawk \" prefix")]
MissingPrefix,
#[fail(display = "{}", _0)]
#[error("{}", _0)]
Parse(ParseError),
#[fail(display = "id property is too short")]
#[error("id property is too short")]
TruncatedId,
}
/// An error occurred in an Actix extractor.
#[derive(Debug)]
#[derive(Error, Debug)]
pub struct ValidationError {
pub status: StatusCode,
inner: Context<ValidationErrorKind>,
kind: ValidationErrorKind,
}
impl ValidationError {
pub fn kind(&self) -> &ValidationErrorKind {
self.inner.get_context()
&self.kind
}
pub fn metric_label(&self) -> Option<String> {
@ -123,21 +125,21 @@ impl ValidationError {
}
/// Causes of extractor errors.
#[derive(Debug, Fail)]
#[derive(Debug, Error)]
pub enum ValidationErrorKind {
#[fail(display = "{}", _0)]
#[error("{}", _0)]
FromDetails(String, RequestErrorLocation, Option<String>, Option<String>),
#[fail(display = "{}", _0)]
#[error("{}", _0)]
FromValidationErrors(
#[cause] validator::ValidationErrors,
validator::ValidationErrors,
RequestErrorLocation,
Option<String>,
),
}
failure_boilerplate!(HawkError, HawkErrorKind);
failure_boilerplate!(ValidationError, ValidationErrorKind);
impl_fmt_display!(HawkError, HawkErrorKind);
impl_fmt_display!(ValidationError, ValidationErrorKind);
from_error!(DecodeError, ApiError, HawkErrorKind::Base64);
from_error!(InvalidKeyLength, ApiError, HawkErrorKind::InvalidKeyLength);
@ -145,16 +147,16 @@ from_error!(JsonError, ApiError, HawkErrorKind::Json);
from_error!(MacError, ApiError, HawkErrorKind::Hmac);
from_error!(ToStrError, ApiError, HawkErrorKind::Header);
impl From<Context<HawkErrorKind>> for HawkError {
fn from(inner: Context<HawkErrorKind>) -> Self {
Self { inner }
impl From<HawkErrorKind> for HawkError {
fn from(kind: HawkErrorKind) -> Self {
Self { kind }
}
}
impl From<Context<ValidationErrorKind>> for ValidationError {
fn from(inner: Context<ValidationErrorKind>) -> Self {
trace!("Validation Error: {:?}", inner.get_context());
let status = match inner.get_context() {
impl From<ValidationErrorKind> for ValidationError {
fn from(kind: ValidationErrorKind) -> Self {
trace!("Validation Error: {:?}", kind);
let status = match kind {
ValidationErrorKind::FromDetails(ref _description, ref location, Some(ref name), _)
if *location == RequestErrorLocation::Header =>
{
@ -173,13 +175,13 @@ impl From<Context<ValidationErrorKind>> for ValidationError {
_ => StatusCode::BAD_REQUEST,
};
Self { status, inner }
Self { status, kind }
}
}
impl From<HawkErrorKind> for ApiError {
fn from(kind: HawkErrorKind) -> Self {
let hawk_error: HawkError = Context::new(kind).into();
let hawk_error: HawkError = kind.into();
hawk_error.into()
}
}
@ -192,7 +194,7 @@ impl From<ParseError> for ApiError {
impl From<ValidationErrorKind> for ApiError {
fn from(kind: ValidationErrorKind) -> Self {
let validation_error: ValidationError = Context::new(kind).into();
let validation_error: ValidationError = kind.into();
validation_error.into()
}
}
@ -209,7 +211,7 @@ impl Serialize for ValidationError {
where
S: Serializer,
{
Serialize::serialize(&self.inner.get_context(), serializer)
Serialize::serialize(&self.kind, serializer)
}
}

View File

@ -1,3 +1,4 @@
use std::error::Error as StdError;
use std::task::Context;
use std::{cell::RefCell, rc::Rc};
@ -13,6 +14,7 @@ use std::task::Poll;
use crate::error::ApiError;
use crate::server::{metrics::Metrics, ServerState};
use crate::web::tags::Tags;
use sentry_backtrace::parse_stacktrace;
pub struct SentryWrapper;
@ -142,7 +144,7 @@ where
trace!("Sentry: Not reporting error: {:?}", apie);
return future::ok(sresp);
}
report(&tags, sentry::integrations::failure::event_from_fail(apie));
report(&tags, event_from_error(apie));
}
}
}
@ -150,3 +152,51 @@ where
}))
}
}
/// Custom `sentry::event_from_error` for `ApiError`
///
/// `sentry::event_from_error` can't access `std::Error` backtraces as its
/// `backtrace()` method is currently Rust nightly only. This function works
/// against `HandlerError` instead to access its backtrace.
pub fn event_from_error(err: &ApiError) -> Event<'static> {
let mut exceptions = vec![exception_from_error_with_backtrace(err)];
let mut source = err.source();
while let Some(err) = source {
let exception = if let Some(err) = err.downcast_ref() {
exception_from_error_with_backtrace(err)
} else {
exception_from_error(err)
};
exceptions.push(exception);
source = err.source();
}
exceptions.reverse();
Event {
exception: exceptions.into(),
level: sentry::protocol::Level::Error,
..Default::default()
}
}
/// Custom `exception_from_error` support function for `ApiError`
///
/// Based moreso on sentry_failure's `exception_from_single_fail`.
fn exception_from_error_with_backtrace(err: &ApiError) -> sentry::protocol::Exception {
let mut exception = exception_from_error(err);
// format the stack trace with alternate debug to get addresses
let bt = format!("{:#?}", err.backtrace);
exception.stacktrace = parse_stacktrace(&bt);
exception
}
/// Exact copy of sentry's unfortunately private `exception_from_error`
fn exception_from_error<E: StdError + ?Sized>(err: &E) -> sentry::protocol::Exception {
let dbg = format!("{:?}", err);
sentry::protocol::Exception {
ty: sentry::parse_type_from_debug(&dbg).to_owned(),
value: Some(err.to_string()),
..Default::default()
}
}