Error refactoring

author: Alex Auvolat <alex@adnab.me> 2022-05-13 15:04:53 +0200
committer: Alex Auvolat <alex@adnab.me> 2022-05-13 15:04:53 +0200
commit: 96b11524d53b3616a28f33e2b057655be1639f6f (patch)
tree: 324a8a9624c5b861fb5a09b0ae0e0c07f6b9614c
parent: c0fb9fd0fe553e5eda39dcb1a09f059bcd631b6c (diff)
download: garage-96b11524d53b3616a28f33e2b057655be1639f6f.tar.gz
garage-96b11524d53b3616a28f33e2b057655be1639f6f.zip
24 files changed, 135 insertions, 206 deletions
diff --git a/src/api/admin/api_server.rs b/src/api/admin/api_server.rs
index b344a51b..b2effa62 100644
--- a/src/api/admin/api_server.rs
+++ b/src/api/admin/api_server.rs
@@ -17,9 +17,9 @@ use garage_util::error::Error as GarageError;
 
 use crate::generic_server::*;
 
-use crate::admin::error::*;
 use crate::admin::bucket::*;
 use crate::admin::cluster::*;
+use crate::admin::error::*;
 use crate::admin::key::*;
 use crate::admin::router::{Authorization, Endpoint};
 
diff --git a/src/api/admin/bucket.rs b/src/api/admin/bucket.rs
index 1ecb66ab..db1fda0f 100644
--- a/src/api/admin/bucket.rs
+++ b/src/api/admin/bucket.rs
@@ -16,8 +16,8 @@ use garage_model::garage::Garage;
 use garage_model::permission::*;
 use garage_model::s3::object_table::ObjectFilter;
 
-use crate::admin::key::ApiBucketKeyPerm;
 use crate::admin::error::*;
+use crate::admin::key::ApiBucketKeyPerm;
 use crate::admin::parse_json_body;
 
 pub async fn handle_list_buckets(garage: &Arc<Garage>) -> Result<Response<Body>, Error> {
@@ -98,7 +98,7 @@ pub async fn handle_get_bucket_info(
 			.ok_or_bad_request("Bucket not found")?,
 		_ => {
 			return Err(Error::bad_request(
-				"Either id or globalAlias must be provided (but not both)"
+				"Either id or globalAlias must be provided (but not both)",
 			));
 		}
 	};
diff --git a/src/api/admin/error.rs b/src/api/admin/error.rs
index 3e488d8d..1f49fed5 100644
--- a/src/api/admin/error.rs
+++ b/src/api/admin/error.rs
@@ -3,10 +3,10 @@ use hyper::header::HeaderValue;
 use hyper::{Body, HeaderMap, StatusCode};
 
 use garage_model::helper::error::Error as HelperError;
-use garage_util::error::Error as GarageError;
 
 use crate::generic_server::ApiError;
-pub use crate::common_error::*;
+use crate::common_error::CommonError;
+pub use crate::common_error::{OkOrBadRequest, OkOrInternalError};
 
 /// Errors of this crate
 #[derive(Debug, Error)]
@@ -47,7 +47,9 @@ pub enum Error {
 }
 
 impl<T> From<T> for Error
-where CommonError: From<T> {
+where
+	CommonError: From<T>,
+{
 	fn from(err: T) -> Self {
 		Error::CommonError(CommonError::from(err))
 	}
@@ -83,7 +85,10 @@ impl ApiError for Error {
 	}
 
 	fn http_body(&self, garage_region: &str, path: &str) -> Body {
-		Body::from(format!("ERROR: {}\n\ngarage region: {}\npath: {}", self, garage_region, path))
+		Body::from(format!(
+			"ERROR: {}\n\ngarage region: {}\npath: {}",
+			self, garage_region, path
+		))
 	}
 }
 
diff --git a/src/api/admin/mod.rs b/src/api/admin/mod.rs
index 68839039..73700e6e 100644
--- a/src/api/admin/mod.rs
+++ b/src/api/admin/mod.rs
@@ -1,14 +1,13 @@
 pub mod api_server;
-mod router;
 mod error;
+mod router;
 
 mod bucket;
 mod cluster;
 mod key;
 
-
-use serde::{Deserialize};
-use hyper::{Request, Body};
+use hyper::{Body, Request};
+use serde::Deserialize;
 
 use error::*;
 
diff --git a/src/api/common_error.rs b/src/api/common_error.rs
index 8be85f97..eca27e6b 100644
--- a/src/api/common_error.rs
+++ b/src/api/common_error.rs
@@ -1,12 +1,8 @@
 use err_derive::Error;
-use hyper::header::HeaderValue;
-use hyper::{Body, HeaderMap, StatusCode};
+use hyper::StatusCode;
 
-use garage_model::helper::error::Error as HelperError;
 use garage_util::error::Error as GarageError;
 
-use crate::generic_server::ApiError;
-
 /// Errors of this crate
 #[derive(Debug, Error)]
 pub enum CommonError {
@@ -36,8 +32,9 @@ impl CommonError {
 				| GarageError::RemoteError(_)
 				| GarageError::Quorum(_, _, _, _),
 			) => StatusCode::SERVICE_UNAVAILABLE,
-			CommonError::InternalError(_) | CommonError::Hyper(_) | CommonError::Http(_) => 
-				StatusCode::INTERNAL_SERVER_ERROR,
+			CommonError::InternalError(_) | CommonError::Hyper(_) | CommonError::Http(_) => {
+				StatusCode::INTERNAL_SERVER_ERROR
+			}
 			CommonError::BadRequest(_) => StatusCode::BAD_REQUEST,
 		}
 	}
@@ -57,7 +54,11 @@ where
 	fn ok_or_bad_request<M: AsRef<str>>(self, reason: M) -> Result<T, CommonError> {
 		match self {
 			Ok(x) => Ok(x),
-			Err(e) => Err(CommonError::BadRequest(format!("{}: {}", reason.as_ref(), e))),
+			Err(e) => Err(CommonError::BadRequest(format!(
+				"{}: {}",
+				reason.as_ref(),
+				e
+			))),
 		}
 	}
 }
diff --git a/src/api/error.rs b/src/api/error.rs
index 90bfccef..3cb97019 100644
--- a/src/api/error.rs
+++ b/src/api/error.rs
@@ -7,24 +7,17 @@ use hyper::{Body, HeaderMap, StatusCode};
 use garage_model::helper::error::Error as HelperError;
 use garage_util::error::Error as GarageError;
 
+use crate::common_error::CommonError;
+pub use crate::common_error::{OkOrBadRequest, OkOrInternalError};
 use crate::generic_server::ApiError;
 use crate::s3::xml as s3_xml;
 
 /// Errors of this crate
 #[derive(Debug, Error)]
 pub enum Error {
-	// Category: internal error
-	/// Error related to deeper parts of Garage
-	#[error(display = "Internal error: {}", _0)]
-	InternalError(#[error(source)] GarageError),
-
-	/// Error related to Hyper
-	#[error(display = "Internal error (Hyper error): {}", _0)]
-	Hyper(#[error(source)] hyper::Error),
-
-	/// Error related to HTTP
-	#[error(display = "Internal error (HTTP error): {}", _0)]
-	Http(#[error(source)] http::Error),
+	#[error(display = "{}", _0)]
+	/// Error from common error
+	CommonError(CommonError),
 
 	// Category: cannot process
 	/// No proper api key was used, or the signature was invalid
@@ -101,10 +94,6 @@ pub enum Error {
 	#[error(display = "Invalid HTTP range: {:?}", _0)]
 	InvalidRange(#[error(from)] (http_range::HttpRangeParseError, u64)),
 
-	/// The client sent an invalid request
-	#[error(display = "Bad request: {}", _0)]
-	BadRequest(String),
-
 	/// The client asked for an invalid return format (invalid Accept header)
 	#[error(display = "Not acceptable: {}", _0)]
 	NotAcceptable(String),
@@ -114,6 +103,15 @@ pub enum Error {
 	NotImplemented(String),
 }
 
+impl<T> From<T> for Error
+where
+	CommonError: From<T>,
+{
+	fn from(err: T) -> Self {
+		Error::CommonError(CommonError::from(err))
+	}
+}
+
 impl From<roxmltree::Error> for Error {
 	fn from(err: roxmltree::Error) -> Self {
 		Self::InvalidXml(format!("{}", err))
@@ -129,16 +127,16 @@ impl From<quick_xml::de::DeError> for Error {
 impl From<HelperError> for Error {
 	fn from(err: HelperError) -> Self {
 		match err {
-			HelperError::Internal(i) => Self::InternalError(i),
-			HelperError::BadRequest(b) => Self::BadRequest(b),
-			e => Self::BadRequest(format!("{}", e)),
+			HelperError::Internal(i) => Self::CommonError(CommonError::InternalError(i)),
+			HelperError::BadRequest(b) => Self::CommonError(CommonError::BadRequest(b)),
+			e => Self::CommonError(CommonError::BadRequest(format!("{}", e))),
 		}
 	}
 }
 
 impl From<multer::Error> for Error {
 	fn from(err: multer::Error) -> Self {
-		Self::BadRequest(err.to_string())
+		Self::bad_request(err)
 	}
 }
 
@@ -157,18 +155,26 @@ impl Error {
 			Error::Forbidden(_) => "AccessDenied",
 			Error::AuthorizationHeaderMalformed(_) => "AuthorizationHeaderMalformed",
 			Error::NotImplemented(_) => "NotImplemented",
-			Error::InternalError(
+			Error::CommonError(CommonError::InternalError(
 				GarageError::Timeout
 				| GarageError::RemoteError(_)
 				| GarageError::Quorum(_, _, _, _),
-			) => "ServiceUnavailable",
-			Error::InternalError(_) | Error::Hyper(_) | Error::Http(_) => "InternalError",
+			)) => "ServiceUnavailable",
+			Error::CommonError(
+				CommonError::InternalError(_) | CommonError::Hyper(_) | CommonError::Http(_),
+			) => "InternalError",
 			_ => "InvalidRequest",
 		}
 	}
 
+	pub fn internal_error<M: ToString>(msg: M) -> Self {
+		Self::CommonError(CommonError::InternalError(GarageError::Message(
+			msg.to_string(),
+		)))
+	}
+
 	pub fn bad_request<M: ToString>(msg: M) -> Self {
-		Self::BadRequest(msg.to_string())
+		Self::CommonError(CommonError::BadRequest(msg.to_string()))
 	}
 }
 
@@ -176,19 +182,12 @@ impl ApiError for Error {
 	/// Get the HTTP status code that best represents the meaning of the error for the client
 	fn http_status_code(&self) -> StatusCode {
 		match self {
+			Error::CommonError(c) => c.http_status_code(),
 			Error::NoSuchKey | Error::NoSuchBucket | Error::NoSuchUpload => StatusCode::NOT_FOUND,
 			Error::BucketNotEmpty | Error::BucketAlreadyExists => StatusCode::CONFLICT,
 			Error::PreconditionFailed => StatusCode::PRECONDITION_FAILED,
 			Error::Forbidden(_) => StatusCode::FORBIDDEN,
 			Error::NotAcceptable(_) => StatusCode::NOT_ACCEPTABLE,
-			Error::InternalError(
-				GarageError::Timeout
-				| GarageError::RemoteError(_)
-				| GarageError::Quorum(_, _, _, _),
-			) => StatusCode::SERVICE_UNAVAILABLE,
-			Error::InternalError(_) | Error::Hyper(_) | Error::Http(_) => {
-				StatusCode::INTERNAL_SERVER_ERROR
-			}
 			Error::InvalidRange(_) => StatusCode::RANGE_NOT_SATISFIABLE,
 			Error::NotImplemented(_) => StatusCode::NOT_IMPLEMENTED,
 			_ => StatusCode::BAD_REQUEST,
@@ -230,67 +229,3 @@ impl ApiError for Error {
 		}))
 	}
 }
-
-/// Trait to map error to the Bad Request error code
-pub trait OkOrBadRequest {
-	type S;
-	fn ok_or_bad_request<M: AsRef<str>>(self, reason: M) -> Result<Self::S, Error>;
-}
-
-impl<T, E> OkOrBadRequest for Result<T, E>
-where
-	E: std::fmt::Display,
-{
-	type S = T;
-	fn ok_or_bad_request<M: AsRef<str>>(self, reason: M) -> Result<T, Error> {
-		match self {
-			Ok(x) => Ok(x),
-			Err(e) => Err(Error::BadRequest(format!("{}: {}", reason.as_ref(), e))),
-		}
-	}
-}
-
-impl<T> OkOrBadRequest for Option<T> {
-	type S = T;
-	fn ok_or_bad_request<M: AsRef<str>>(self, reason: M) -> Result<T, Error> {
-		match self {
-			Some(x) => Ok(x),
-			None => Err(Error::BadRequest(reason.as_ref().to_string())),
-		}
-	}
-}
-
-/// Trait to map an error to an Internal Error code
-pub trait OkOrInternalError {
-	type S;
-	fn ok_or_internal_error<M: AsRef<str>>(self, reason: M) -> Result<Self::S, Error>;
-}
-
-impl<T, E> OkOrInternalError for Result<T, E>
-where
-	E: std::fmt::Display,
-{
-	type S = T;
-	fn ok_or_internal_error<M: AsRef<str>>(self, reason: M) -> Result<T, Error> {
-		match self {
-			Ok(x) => Ok(x),
-			Err(e) => Err(Error::InternalError(GarageError::Message(format!(
-				"{}: {}",
-				reason.as_ref(),
-				e
-			)))),
-		}
-	}
-}
-
-impl<T> OkOrInternalError for Option<T> {
-	type S = T;
-	fn ok_or_internal_error<M: AsRef<str>>(self, reason: M) -> Result<T, Error> {
-		match self {
-			Some(x) => Ok(x),
-			None => Err(Error::InternalError(GarageError::Message(
-				reason.as_ref().to_string(),
-			))),
-		}
-	}
-}
diff --git a/src/api/helpers.rs b/src/api/helpers.rs
index 5e249dae..e94e8b00 100644
--- a/src/api/helpers.rs
+++ b/src/api/helpers.rs
@@ -52,7 +52,7 @@ pub fn authority_to_host(authority: &str) -> Result<String, Error> {
 	let mut iter = authority.chars().enumerate();
 	let (_, first_char) = iter
 		.next()
-		.ok_or_else(|| Error::BadRequest("Authority is empty".to_string()))?;
+		.ok_or_else(|| Error::bad_request("Authority is empty".to_string()))?;
 
 	let split = match first_char {
 		'[' => {
@@ -60,7 +60,7 @@ pub fn authority_to_host(authority: &str) -> Result<String, Error> {
 			match iter.next() {
 				Some((_, ']')) => iter.next(),
 				_ => {
-					return Err(Error::BadRequest(format!(
+					return Err(Error::bad_request(format!(
 						"Authority {} has an illegal format",
 						authority
 					)))
@@ -73,7 +73,7 @@ pub fn authority_to_host(authority: &str) -> Result<String, Error> {
 	let authority = match split {
 		Some((i, ':')) => Ok(&authority[..i]),
 		None => Ok(authority),
-		Some((_, _)) => Err(Error::BadRequest(format!(
+		Some((_, _)) => Err(Error::bad_request(format!(
 			"Authority {} has an illegal format",
 			authority
 		))),
@@ -134,7 +134,7 @@ pub fn parse_bucket_key<'a>(
 		None => (path, None),
 	};
 	if bucket.is_empty() {
-		return Err(Error::BadRequest("No bucket specified".to_string()));
+		return Err(Error::bad_request("No bucket specified"));
 	}
 	Ok((bucket, key))
 }
diff --git a/src/api/k2v/batch.rs b/src/api/k2v/batch.rs
index a97bd7f2..26d3cef0 100644
--- a/src/api/k2v/batch.rs
+++ b/src/api/k2v/batch.rs
@@ -88,7 +88,7 @@ async fn handle_read_batch_query(
 
 	let (items, more, next_start) = if query.single_item {
 		if query.prefix.is_some() || query.end.is_some() || query.limit.is_some() || query.reverse {
-			return Err(Error::BadRequest("Batch query parameters 'prefix', 'end', 'limit' and 'reverse' must not be set when singleItem is true.".into()));
+			return Err(Error::bad_request("Batch query parameters 'prefix', 'end', 'limit' and 'reverse' must not be set when singleItem is true."));
 		}
 		let sk = query
 			.start
@@ -183,7 +183,7 @@ async fn handle_delete_batch_query(
 
 	let deleted_items = if query.single_item {
 		if query.prefix.is_some() || query.end.is_some() {
-			return Err(Error::BadRequest("Batch query parameters 'prefix' and 'end' must not be set when singleItem is true.".into()));
+			return Err(Error::bad_request("Batch query parameters 'prefix' and 'end' must not be set when singleItem is true."));
 		}
 		let sk = query
 			.start
diff --git a/src/api/k2v/range.rs b/src/api/k2v/range.rs
index cd019723..8d4cefbc 100644
--- a/src/api/k2v/range.rs
+++ b/src/api/k2v/range.rs
@@ -31,7 +31,7 @@ where
 		(None, Some(s)) => (Some(s.clone()), false),
 		(Some(p), Some(s)) => {
 			if !s.starts_with(p) {
-				return Err(Error::BadRequest(format!(
+				return Err(Error::bad_request(format!(
 					"Start key '{}' does not start with prefix '{}'",
 					s, p
 				)));
diff --git a/src/api/k2v/router.rs b/src/api/k2v/router.rs
index f948ffce..611b6629 100644
--- a/src/api/k2v/router.rs
+++ b/src/api/k2v/router.rs
@@ -62,7 +62,7 @@ impl Endpoint {
 			.unwrap_or((path.to_owned(), ""));
 
 		if bucket.is_empty() {
-			return Err(Error::BadRequest("Missing bucket name".to_owned()));
+			return Err(Error::bad_request("Missing bucket name".to_owned()));
 		}
 
 		if *req.method() == Method::OPTIONS {
@@ -83,7 +83,7 @@ impl Endpoint {
 			Method::PUT => Self::from_put(partition_key, &mut query)?,
 			Method::DELETE => Self::from_delete(partition_key, &mut query)?,
 			_ if req.method() == method_search => Self::from_search(partition_key, &mut query)?,
-			_ => return Err(Error::BadRequest("Unknown method".to_owned())),
+			_ => return Err(Error::bad_request("Unknown method".to_owned())),
 		};
 
 		if let Some(message) = query.nonempty_message() {
diff --git a/src/api/s3/api_server.rs b/src/api/s3/api_server.rs
index 2bf4a1bc..af9f03e7 100644
--- a/src/api/s3/api_server.rs
+++ b/src/api/s3/api_server.rs
@@ -310,7 +310,7 @@ impl ApiHandler for S3ApiServer {
 					)
 					.await
 				} else {
-					Err(Error::BadRequest(format!(
+					Err(Error::bad_request(format!(
 						"Invalid endpoint: list-type={}",
 						list_type
 					)))
diff --git a/src/api/s3/bucket.rs b/src/api/s3/bucket.rs
index 93048a8c..6ecda2cd 100644
--- a/src/api/s3/bucket.rs
+++ b/src/api/s3/bucket.rs
@@ -130,7 +130,7 @@ pub async fn handle_create_bucket(
 
 	if let Some(location_constraint) = cmd {
 		if location_constraint != garage.config.s3_api.s3_region {
-			return Err(Error::BadRequest(format!(
+			return Err(Error::bad_request(format!(
 				"Cannot satisfy location constraint `{}`: buckets can only be created in region `{}`",
 				location_constraint,
 				garage.config.s3_api.s3_region
@@ -163,7 +163,7 @@ pub async fn handle_create_bucket(
 	} else {
 		// Create the bucket!
 		if !is_valid_bucket_name(&bucket_name) {
-			return Err(Error::BadRequest(format!(
+			return Err(Error::bad_request(format!(
 				"{}: {}",
 				bucket_name, INVALID_BUCKET_NAME_MESSAGE
 			)));
diff --git a/src/api/s3/copy.rs b/src/api/s3/copy.rs
index 4e94d887..825b8fc0 100644
--- a/src/api/s3/copy.rs
+++ b/src/api/s3/copy.rs
@@ -201,8 +201,8 @@ pub async fn handle_upload_part_copy(
 			let mut ranges = http_range::HttpRange::parse(range_str, source_version_meta.size)
 				.map_err(|e| (e, source_version_meta.size))?;
 			if ranges.len() != 1 {
-				return Err(Error::BadRequest(
-					"Invalid x-amz-copy-source-range header: exactly 1 range must be given".into(),
+				return Err(Error::bad_request(
+					"Invalid x-amz-copy-source-range header: exactly 1 range must be given",
 				));
 			} else {
 				ranges.pop().unwrap()
@@ -230,8 +230,8 @@ pub async fn handle_upload_part_copy(
 			// This is only for small files, we don't bother handling this.
 			// (in AWS UploadPartCopy works for parts at least 5MB which
 			// is never the case of an inline object)
-			return Err(Error::BadRequest(
-				"Source object is too small (minimum part size is 5Mb)".into(),
+			return Err(Error::bad_request(
+				"Source object is too small (minimum part size is 5Mb)",
 			));
 		}
 		ObjectVersionData::FirstBlock(_meta, _first_block_hash) => (),
@@ -250,7 +250,7 @@ pub async fn handle_upload_part_copy(
 	// Check this part number hasn't yet been uploaded
 	if let Some(dv) = dest_version {
 		if dv.has_part_number(part_number) {
-			return Err(Error::BadRequest(format!(
+			return Err(Error::bad_request(format!(
 				"Part number {} has already been uploaded",
 				part_number
 			)));
@@ -536,8 +536,8 @@ impl CopyPreconditionHeaders {
 			(None, None, None, Some(ims)) => v_date > *ims,
 			(None, None, None, None) => true,
 			_ => {
-				return Err(Error::BadRequest(
-					"Invalid combination of x-amz-copy-source-if-xxxxx headers".into(),
+				return Err(Error::bad_request(
+					"Invalid combination of x-amz-copy-source-if-xxxxx headers",
 				))
 			}
 		};
diff --git a/src/api/s3/get.rs b/src/api/s3/get.rs
index 3edf22a6..794bd4e9 100644
--- a/src/api/s3/get.rs
+++ b/src/api/s3/get.rs
@@ -210,8 +210,8 @@ pub async fn handle_get(
 
 	match (part_number, parse_range_header(req, last_v_meta.size)?) {
 		(Some(_), Some(_)) => {
-			return Err(Error::BadRequest(
-				"Cannot specify both partNumber and Range header".into(),
+			return Err(Error::bad_request(
+				"Cannot specify both partNumber and Range header",
 			));
 		}
 		(Some(pn), None) => {
@@ -302,9 +302,9 @@ async fn handle_get_range(
 				let body: Body = Body::from(bytes[begin as usize..end as usize].to_vec());
 				Ok(resp_builder.body(body)?)
 			} else {
-				None.ok_or_internal_error(
+				Err(Error::internal_error(
 					"Requested range not present in inline bytes when it should have been",
-				)
+				))
 			}
 		}
 		ObjectVersionData::FirstBlock(_meta, _first_block_hash) => {
diff --git a/src/api/s3/list.rs b/src/api/s3/list.rs
index e2848c57..d97aafe5 100644
--- a/src/api/s3/list.rs
+++ b/src/api/s3/list.rs
@@ -588,7 +588,7 @@ impl ListObjectsQuery {
 					"]" => Ok(RangeBegin::AfterKey {
 						key: String::from_utf8(base64::decode(token[1..].as_bytes())?)?,
 					}),
-					_ => Err(Error::BadRequest("Invalid continuation token".to_string())),
+					_ => Err(Error::bad_request("Invalid continuation token".to_string())),
 				},
 
 				// StartAfter has defined semantics in the spec:
diff --git a/src/api/s3/post_object.rs b/src/api/s3/post_object.rs
index 86fa7880..91648a19 100644
--- a/src/api/s3/post_object.rs
+++ b/src/api/s3/post_object.rs
@@ -48,7 +48,7 @@ pub async fn handle_post_object(
 		let field = if let Some(field) = multipart.next_field().await? {
 			field
 		} else {
-			return Err(Error::BadRequest(
+			return Err(Error::bad_request(
 				"Request did not contain a file".to_owned(),
 			));
 		};
@@ -66,14 +66,14 @@ pub async fn handle_post_object(
 				"tag" => (/* tag need to be reencoded, but we don't support them yet anyway */),
 				"acl" => {
 					if params.insert("x-amz-acl", content).is_some() {
-						return Err(Error::BadRequest(
+						return Err(Error::bad_request(
 							"Field 'acl' provided more than one time".to_string(),
 						));
 					}
 				}
 				_ => {
 					if params.insert(&name, content).is_some() {
-						return Err(Error::BadRequest(format!(
+						return Err(Error::bad_request(format!(
 							"Field '{}' provided more than one time",
 							name
 						)));
@@ -145,7 +145,7 @@ pub async fn handle_post_object(
 		.ok_or_bad_request("Invalid expiration date")?
 		.into();
 	if Utc::now() - expiration > Duration::zero() {
-		return Err(Error::BadRequest(
+		return Err(Error::bad_request(
 			"Expiration date is in the paste".to_string(),
 		));
 	}
@@ -159,7 +159,7 @@ pub async fn handle_post_object(
 			"policy" | "x-amz-signature" => (), // this is always accepted, as it's required to validate other fields
 			"content-type" => {
 				let conds = conditions.params.remove("content-type").ok_or_else(|| {
-					Error::BadRequest(format!("Key '{}' is not allowed in policy", param_key))
+					Error::bad_request(format!("Key '{}' is not allowed in policy", param_key))
 				})?;
 				for cond in conds {
 					let ok = match cond {
@@ -169,7 +169,7 @@ pub async fn handle_post_object(
 						}
 					};
 					if !ok {
-						return Err(Error::BadRequest(format!(
+						return Err(Error::bad_request(format!(
 							"Key '{}' has value not allowed in policy",
 							param_key
 						)));
@@ -178,7 +178,7 @@ pub async fn handle_post_object(
 			}
 			"key" => {
 				let conds = conditions.params.remove("key").ok_or_else(|| {
-					Error::BadRequest(format!("Key '{}' is not allowed in policy", param_key))
+					Error::bad_request(format!("Key '{}' is not allowed in policy", param_key))
 				})?;
 				for cond in conds {
 					let ok = match cond {
@@ -186,7 +186,7 @@ pub async fn handle_post_object(
 						Operation::StartsWith(s) => key.starts_with(&s),
 					};
 					if !ok {
-						return Err(Error::BadRequest(format!(
+						return Err(Error::bad_request(format!(
 							"Key '{}' has value not allowed in policy",
 							param_key
 						)));
@@ -201,7 +201,7 @@ pub async fn handle_post_object(
 					continue;
 				}
 				let conds = conditions.params.remove(&param_key).ok_or_else(|| {
-					Error::BadRequest(format!("Key '{}' is not allowed in policy", param_key))
+					Error::bad_request(format!("Key '{}' is not allowed in policy", param_key))
 				})?;
 				for cond in conds {
 					let ok = match cond {
@@ -209,7 +209,7 @@ pub async fn handle_post_object(
 						Operation::StartsWith(s) => value.to_str()?.starts_with(s.as_str()),
 					};
 					if !ok {
-						return Err(Error::BadRequest(format!(
+						return Err(Error::bad_request(format!(
 							"Key '{}' has value not allowed in policy",
 							param_key
 						)));
@@ -220,7 +220,7 @@ pub async fn handle_post_object(
 	}
 
 	if let Some((param_key, _)) = conditions.params.iter().next() {
-		return Err(Error::BadRequest(format!(
+		return Err(Error::bad_request(format!(
 			"Key '{}' is required in policy, but no value was provided",
 			param_key
 		)));
@@ -326,7 +326,7 @@ impl Policy {
 			match condition {
 				PolicyCondition::Equal(map) => {
 					if map.len() != 1 {
-						return Err(Error::BadRequest("Invalid policy item".to_owned()));
+						return Err(Error::bad_request("Invalid policy item".to_owned()));
 					}
 					let (mut k, v) = map.into_iter().next().expect("size was verified");
 					k.make_ascii_lowercase();
@@ -334,7 +334,7 @@ impl Policy {
 				}
 				PolicyCondition::OtherOp([cond, mut key, value]) => {
 					if key.remove(0) != '$' {
-						return Err(Error::BadRequest("Invalid policy item".to_owned()));
+						return Err(Error::bad_request("Invalid policy item".to_owned()));
 					}
 					key.make_ascii_lowercase();
 					match cond.as_str() {
@@ -347,7 +347,7 @@ impl Policy {
 								.or_default()
 								.push(Operation::StartsWith(value));
 						}
-						_ => return Err(Error::BadRequest("Invalid policy item".to_owned())),
+						_ => return Err(Error::bad_request("Invalid policy item".to_owned())),
 					}
 				}
 				PolicyCondition::SizeRange(key, min, max) => {
@@ -355,7 +355,7 @@ impl Policy {
 						length.0 = length.0.max(min);
 						length.1 = length.1.min(max);
 					} else {
-						return Err(Error::BadRequest("Invalid policy item".to_owned()));
+						return Err(Error::bad_request("Invalid policy item".to_owned()));
 					}
 				}
 			}
@@ -420,14 +420,14 @@ where
 				self.read += bytes.len() as u64;
 				// optimization to fail early when we know before the end it's too long
 				if self.length.end() < &self.read {
-					return Poll::Ready(Some(Err(Error::BadRequest(
+					return Poll::Ready(Some(Err(Error::bad_request(
 						"File size does not match policy".to_owned(),
 					))));
 				}
 			}
 			Poll::Ready(None) => {
 				if !self.length.contains(&self.read) {
-					return Poll::Ready(Some(Err(Error::BadRequest(
+					return Poll::Ready(Some(Err(Error::bad_request(
 						"File size does not match policy".to_owned(),
 					))));
 				}
diff --git a/src/api/s3/put.rs b/src/api/s3/put.rs
index 89aa8d84..d50e32b0 100644
--- a/src/api/s3/put.rs
+++ b/src/api/s3/put.rs
@@ -183,7 +183,7 @@ fn ensure_checksum_matches(
 ) -> Result<(), Error> {
 	if let Some(expected_sha256) = content_sha256 {
 		if expected_sha256 != data_sha256sum {
-			return Err(Error::BadRequest(
+			return Err(Error::bad_request(
 				"Unable to validate x-amz-content-sha256".to_string(),
 			));
 		} else {
@@ -192,7 +192,7 @@ fn ensure_checksum_matches(
 	}
 	if let Some(expected_md5) = content_md5 {
 		if expected_md5.trim_matches('"') != base64::encode(data_md5sum) {
-			return Err(Error::BadRequest(
+			return Err(Error::bad_request(
 				"Unable to validate content-md5".to_string(),
 			));
 		} else {
@@ -428,7 +428,7 @@ pub async fn handle_put_part(
 	// Check part hasn't already been uploaded
 	if let Some(v) = version {
 		if v.has_part_number(part_number) {
-			return Err(Error::BadRequest(format!(
+			return Err(Error::bad_request(format!(
 				"Part number {} has already been uploaded",
 				part_number
 			)));
@@ -513,7 +513,7 @@ pub async fn handle_complete_multipart_upload(
 
 	let version = version.ok_or(Error::NoSuchKey)?;
 	if version.blocks.is_empty() {
-		return Err(Error::BadRequest("No data was uploaded".to_string()));
+		return Err(Error::bad_request("No data was uploaded".to_string()));
 	}
 
 	let headers = match object_version.state {
@@ -574,8 +574,8 @@ pub async fn handle_complete_multipart_upload(
 		.map(|x| x.part_number)
 		.eq(block_parts.into_iter());
 	if !same_parts {
-		return Err(Error::BadRequest(
-			"Part numbers in block list and part list do not match. This can happen if a part was partially uploaded. Please abort the multipart upload and try again.".into(),
+		return Err(Error::bad_request(
+			"Part numbers in block list and part list do not match. This can happen if a part was partially uploaded. Please abort the multipart upload and try again."
 		));
 	}
 
diff --git a/src/api/s3/router.rs b/src/api/s3/router.rs
index 446ceb54..e920e162 100644
--- a/src/api/s3/router.rs
+++ b/src/api/s3/router.rs
@@ -342,7 +342,7 @@ impl Endpoint {
 			Method::POST => Self::from_post(key, &mut query)?,
 			Method::PUT => Self::from_put(key, &mut query, req.headers())?,
 			Method::DELETE => Self::from_delete(key, &mut query)?,
-			_ => return Err(Error::BadRequest("Unknown method".to_owned())),
+			_ => return Err(Error::bad_request("Unknown method".to_owned())),
 		};
 
 		if let Some(message) = query.nonempty_message() {
diff --git a/src/api/s3/website.rs b/src/api/s3/website.rs
index 561130dc..4fc7b7bb 100644
--- a/src/api/s3/website.rs
+++ b/src/api/s3/website.rs
@@ -176,7 +176,7 @@ impl WebsiteConfiguration {
 				|| self.index_document.is_some()
 				|| self.routing_rules.is_some())
 		{
-			return Err(Error::BadRequest(
+			return Err(Error::bad_request(
 				"Bad XML: can't have RedirectAllRequestsTo and other fields".to_owned(),
 			));
 		}
@@ -222,7 +222,7 @@ impl WebsiteConfiguration {
 impl Key {
 	pub fn validate(&self) -> Result<(), Error> {
 		if self.key.0.is_empty() {
-			Err(Error::BadRequest(
+			Err(Error::bad_request(
 				"Bad XML: error document specified but empty".to_owned(),
 			))
 		} else {
@@ -234,7 +234,7 @@ impl Key {
 impl Suffix {
 	pub fn validate(&self) -> Result<(), Error> {
 		if self.suffix.0.is_empty() | self.suffix.0.contains('/') {
-			Err(Error::BadRequest(
+			Err(Error::bad_request(
 				"Bad XML: index document is empty or contains /".to_owned(),
 			))
 		} else {
@@ -247,7 +247,7 @@ impl Target {
 	pub fn validate(&self) -> Result<(), Error> {
 		if let Some(ref protocol) = self.protocol {
 			if protocol.0 != "http" && protocol.0 != "https" {
-				return Err(Error::BadRequest("Bad XML: invalid protocol".to_owned()));
+				return Err(Error::bad_request("Bad XML: invalid protocol".to_owned()));
 			}
 		}
 		Ok(())
@@ -269,19 +269,19 @@ impl Redirect {
 	pub fn validate(&self, has_prefix: bool) -> Result<(), Error> {
 		if self.replace_prefix.is_some() {
 			if self.replace_full.is_some() {
-				return Err(Error::BadRequest(
+				return Err(Error::bad_request(
 					"Bad XML: both ReplaceKeyPrefixWith and ReplaceKeyWith are set".to_owned(),
 				));
 			}
 			if !has_prefix {
-				return Err(Error::BadRequest(
+				return Err(Error::bad_request(
 					"Bad XML: ReplaceKeyPrefixWith is set, but  KeyPrefixEquals isn't".to_owned(),
 				));
 			}
 		}
 		if let Some(ref protocol) = self.protocol {
 			if protocol.0 != "http" && protocol.0 != "https" {
-				return Err(Error::BadRequest("Bad XML: invalid protocol".to_owned()));
+				return Err(Error::bad_request("Bad XML: invalid protocol".to_owned()));
 			}
 		}
 		// TODO there are probably more invalide cases, but which ones?
diff --git a/src/api/signature/mod.rs b/src/api/signature/mod.rs
index 5646f4fa..e3554080 100644
--- a/src/api/signature/mod.rs
+++ b/src/api/signature/mod.rs
@@ -16,7 +16,7 @@ type HmacSha256 = Hmac<Sha256>;
 
 pub fn verify_signed_content(expected_sha256: Hash, body: &[u8]) -> Result<(), Error> {
 	if expected_sha256 != sha256sum(body) {
-		return Err(Error::BadRequest(
+		return Err(Error::bad_request(
 			"Request content hash does not match signed hash".to_string(),
 		));
 	}
diff --git a/src/api/signature/payload.rs b/src/api/signature/payload.rs
index 9137dd2d..52c4d401 100644
--- a/src/api/signature/payload.rs
+++ b/src/api/signature/payload.rs
@@ -105,7 +105,7 @@ fn parse_authorization(
 	let (auth_kind, rest) = authorization.split_at(first_space);
 
 	if auth_kind != "AWS4-HMAC-SHA256" {
-		return Err(Error::BadRequest("Unsupported authorization method".into()));
+		return Err(Error::bad_request("Unsupported authorization method"));
 	}
 
 	let mut auth_params = HashMap::new();
@@ -129,10 +129,11 @@ fn parse_authorization(
 	let date = headers
 		.get("x-amz-date")
 		.ok_or_bad_request("Missing X-Amz-Date field")
+		.map_err(Error::from)
 		.and_then(|d| parse_date(d))?;
 
 	if Utc::now() - date > Duration::hours(24) {
-		return Err(Error::BadRequest("Date is too old".to_string()));
+		return Err(Error::bad_request("Date is too old".to_string()));
 	}
 
 	let auth = Authorization {
@@ -156,7 +157,7 @@ fn parse_query_authorization(
 	headers: &HashMap<String, String>,
 ) -> Result<Authorization, Error> {
 	if algorithm != "AWS4-HMAC-SHA256" {
-		return Err(Error::BadRequest(
+		return Err(Error::bad_request(
 			"Unsupported authorization method".to_string(),
 		));
 	}
@@ -179,10 +180,10 @@ fn parse_query_authorization(
 		.get("x-amz-expires")
 		.ok_or_bad_request("X-Amz-Expires not found in query parameters")?
 		.parse()
-		.map_err(|_| Error::BadRequest("X-Amz-Expires is not a number".to_string()))?;
+		.map_err(|_| Error::bad_request("X-Amz-Expires is not a number".to_string()))?;
 
 	if duration > 7 * 24 * 3600 {
-		return Err(Error::BadRequest(
+		return Err(Error::bad_request(
 			"X-Amz-Exprires may not exceed a week".to_string(),
 		));
 	}
@@ -190,10 +191,11 @@ fn parse_query_authorization(
 	let date = headers
 		.get("x-amz-date")
 		.ok_or_bad_request("Missing X-Amz-Date field")
+		.map_err(Error::from)
 		.and_then(|d| parse_date(d))?;
 
 	if Utc::now() - date > Duration::seconds(duration) {
-		return Err(Error::BadRequest("Date is too old".to_string()));
+		return Err(Error::bad_request("Date is too old".to_string()));
 	}
 
 	Ok(Authorization {
diff --git a/src/api/signature/streaming.rs b/src/api/signature/streaming.rs
index ded9d993..6c326c54 100644
--- a/src/api/signature/streaming.rs
+++ b/src/api/signature/streaming.rs
@@ -87,7 +87,7 @@ fn compute_streaming_payload_signature(
 	let mut hmac = signing_hmac.clone();
 	hmac.update(string_to_sign.as_bytes());
 
-	Hash::try_from(&hmac.finalize().into_bytes()).ok_or_internal_error("Invalid signature")
+	Ok(Hash::try_from(&hmac.finalize().into_bytes()).ok_or_internal_error("Invalid signature")?)
 }
 
 mod payload {
@@ -163,10 +163,10 @@ impl From<SignedPayloadStreamError> for Error {
 		match err {
 			SignedPayloadStreamError::Stream(e) => e,
 			SignedPayloadStreamError::InvalidSignature => {
-				Error::BadRequest("Invalid payload signature".into())
+				Error::bad_request("Invalid payload signature")
 			}
 			SignedPayloadStreamError::Message(e) => {
-				Error::BadRequest(format!("Chunk format error: {}", e))
+				Error::bad_request(format!("Chunk format error: {}", e))
 			}
 		}
 	}
diff --git a/src/web/error.rs b/src/web/error.rs
index e9ed5314..478731b5 100644
--- a/src/web/error.rs
+++ b/src/web/error.rs
@@ -3,50 +3,39 @@ use hyper::header::HeaderValue;
 use hyper::{HeaderMap, StatusCode};
 
 use garage_api::generic_server::ApiError;
-use garage_util::error::Error as GarageError;
 
 /// Errors of this crate
 #[derive(Debug, Error)]
 pub enum Error {
 	/// An error received from the API crate
 	#[error(display = "API error: {}", _0)]
-	ApiError(#[error(source)] garage_api::Error),
-
-	// Category: internal error
-	/// Error internal to garage
-	#[error(display = "Internal error: {}", _0)]
-	InternalError(#[error(source)] GarageError),
+	ApiError(garage_api::Error),
 
 	/// The file does not exist
 	#[error(display = "Not found")]
 	NotFound,
 
-	/// The request contained an invalid UTF-8 sequence in its path or in other parameters
-	#[error(display = "Invalid UTF-8: {}", _0)]
-	InvalidUtf8(#[error(source)] std::str::Utf8Error),
-
-	/// The client send a header with invalid value
-	#[error(display = "Invalid header value: {}", _0)]
-	InvalidHeader(#[error(source)] hyper::header::ToStrError),
-
 	/// The client sent a request without host, or with unsupported method
 	#[error(display = "Bad request: {}", _0)]
 	BadRequest(String),
 }
 
+impl<T> From<T> for Error
+where
+	garage_api::Error: From<T>,
+{
+	fn from(err: T) -> Self {
+		Error::ApiError(garage_api::Error::from(err))
+	}
+}
+
 impl Error {
 	/// Transform errors into http status code
 	pub fn http_status_code(&self) -> StatusCode {
 		match self {
 			Error::NotFound => StatusCode::NOT_FOUND,
 			Error::ApiError(e) => e.http_status_code(),
-			Error::InternalError(
-				GarageError::Timeout
-				| GarageError::RemoteError(_)
-				| GarageError::Quorum(_, _, _, _),
-			) => StatusCode::SERVICE_UNAVAILABLE,
-			Error::InternalError(_) => StatusCode::INTERNAL_SERVER_ERROR,
-			_ => StatusCode::BAD_REQUEST,
+			Error::BadRequest(_) => StatusCode::BAD_REQUEST,
 		}
 	}
 
diff --git a/src/web/web_server.rs b/src/web/web_server.rs
index 867adc51..e83bc4cb 100644
--- a/src/web/web_server.rs
+++ b/src/web/web_server.rs
@@ -207,7 +207,7 @@ async fn serve_file(garage: Arc<Garage>, req: &Request<Body>) -> Result<Response
 		Method::OPTIONS => handle_options_for_bucket(req, &bucket),
 		Method::HEAD => handle_head(garage.clone(), req, bucket_id, &key, None).await,
 		Method::GET => handle_get(garage.clone(), req, bucket_id, &key, None).await,
-		_ => Err(ApiError::BadRequest("HTTP method not supported".into())),
+		_ => Err(ApiError::bad_request("HTTP method not supported")),
 	}
 	.map_err(Error::from);
 
@@ -290,9 +290,7 @@ fn path_to_key<'a>(path: &'a str, index: &str) -> Result<Cow<'a, str>, Error> {
 	let path_utf8 = percent_encoding::percent_decode_str(path).decode_utf8()?;
 
 	if !path_utf8.starts_with('/') {
-		return Err(Error::BadRequest(
-			"Path must start with a / (slash)".to_string(),
-		));
+		return Err(Error::BadRequest("Path must start with a / (slash)".into()));
 	}
 
 	match path_utf8.chars().last() {
author	Alex Auvolat <alex@adnab.me>	2022-05-13 15:04:53 +0200
committer	Alex Auvolat <alex@adnab.me>	2022-05-13 15:04:53 +0200
commit	96b11524d53b3616a28f33e2b057655be1639f6f (patch)
tree	324a8a9624c5b861fb5a09b0ae0e0c07f6b9614c
parent	c0fb9fd0fe553e5eda39dcb1a09f059bcd631b6c (diff)
download	garage-96b11524d53b3616a28f33e2b057655be1639f6f.tar.gz garage-96b11524d53b3616a28f33e2b057655be1639f6f.zip