From 59699c1144eb46d146f6805083eed1393beb8259 Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Wed, 1 Jul 2026 10:55:47 +0200 Subject: [PATCH] ref(service): Use static context strings and helper functions for errors Use static string references for error context where possible, avoiding unnecessary allocations for fixed messages. Introduce ergonomic helper functions so call sites can construct errors concisely without manually boxing causes or assembling struct literals. --- objectstore-service/src/backend/bigtable.rs | 37 +++--- objectstore-service/src/backend/gcs.rs | 108 +++++++----------- objectstore-service/src/backend/local_fs.rs | 52 +++------ .../src/backend/s3_compatible.rs | 27 ++--- objectstore-service/src/backend/tiered.rs | 2 +- objectstore-service/src/error.rs | 34 +++--- 6 files changed, 111 insertions(+), 149 deletions(-) diff --git a/objectstore-service/src/backend/bigtable.rs b/objectstore-service/src/backend/bigtable.rs index 7ab842e4..015e8ff9 100644 --- a/objectstore-service/src/backend/bigtable.rs +++ b/objectstore-service/src/backend/bigtable.rs @@ -1133,27 +1133,30 @@ impl HighVolumeBackend for BigTableBackend { /// required by BigTable, the resulting timestamp has millisecond precision, with the last digits at /// 0. fn ttl_to_micros(ttl: Duration, from: SystemTime) -> Result { - let deadline = from.checked_add(ttl).ok_or_else(|| Error::Generic { - context: format!( + let deadline = from.checked_add(ttl).ok_or_else(|| { + Error::generic(format!( "TTL duration overflow: {} plus {}s cannot be represented as SystemTime", humantime::format_rfc3339_seconds(from), ttl.as_secs() - ), - cause: None, + )) })?; let millis = deadline .duration_since(SystemTime::UNIX_EPOCH) - .map_err(|e| Error::Generic { - context: format!( - "unable to get duration since UNIX_EPOCH for SystemTime {}", - humantime::format_rfc3339_seconds(deadline) - ), - cause: Some(Box::new(e)), + .map_err(|e| { + Error::generic_cause( + format!( + "unable to get duration since UNIX_EPOCH for SystemTime {}", + humantime::format_rfc3339_seconds(deadline) + ), + e, + ) })? .as_millis(); - (millis * 1000).try_into().map_err(|e| Error::Generic { - context: format!("failed to convert {millis}ms to i64 microseconds"), - cause: Some(Box::new(e)), + (millis * 1000).try_into().map_err(|e| { + Error::generic_cause( + format!("failed to convert {millis}ms to i64 microseconds"), + e, + ) }) } @@ -1186,10 +1189,10 @@ where Ok(res) => return Ok(res), Err(e) if retry_count >= REQUEST_RETRY_COUNT || !is_retryable(&e) => { objectstore_metrics::count!("bigtable.failures", action = context); - return Err(Error::Generic { - context: format!("Bigtable: `{context}` failed"), - cause: Some(Box::new(e)), - }); + return Err(Error::generic_cause( + format!("Bigtable: `{context}` failed"), + e, + )); } Err(e) => { retry_count += 1; diff --git a/objectstore-service/src/backend/gcs.rs b/objectstore-service/src/backend/gcs.rs index d43f3eb8..927f2347 100644 --- a/objectstore-service/src/backend/gcs.rs +++ b/objectstore-service/src/backend/gcs.rs @@ -203,9 +203,8 @@ impl GcsObject { .size .map(|size| size.parse()) .transpose() - .map_err(|e| Error::Generic { - context: "GCS: failed to parse size from object metadata".to_string(), - cause: Some(Box::new(e)), + .map_err(|e| { + Error::generic_cause("GCS: failed to parse size from object metadata", e) })?; let time_created = self.time_created; @@ -215,12 +214,9 @@ impl GcsObject { if let GcsMetaKey::Custom(custom_key) = key { custom.insert(custom_key, value); } else { - return Err(Error::Generic { - context: format!( - "GCS: unexpected built-in metadata key in object metadata: {key}" - ), - cause: None, - }); + return Err(Error::generic(format!( + "GCS: unexpected built-in metadata key in object metadata: {key}" + ))); } } @@ -314,23 +310,19 @@ fn metadata_to_gcs_headers(metadata: &Metadata) -> Result { let formatted = humantime::format_rfc3339_seconds(custom_time); headers.insert( HeaderName::from_static("x-goog-custom-time"), - formatted.to_string().parse().map_err(|e| Error::Generic { - context: "GCS: invalid custom-time header value".into(), - cause: Some(Box::new(e)), - })?, + formatted + .to_string() + .parse() + .map_err(|e| Error::generic_cause("GCS: invalid custom-time header value", e))?, ); } if let Some(compression) = metadata.compression { headers.insert( header::CONTENT_ENCODING, - compression - .to_string() - .parse() - .map_err(|e| Error::Generic { - context: "GCS: invalid content-encoding header value".into(), - cause: Some(Box::new(e)), - })?, + compression.to_string().parse().map_err(|e| { + Error::generic_cause("GCS: invalid content-encoding header value", e) + })?, ); } @@ -364,13 +356,11 @@ fn insert_gcs_meta_header( ) -> Result<()> { let header_name = format!("x-goog-meta-{key}"); headers.insert( - HeaderName::try_from(&header_name).map_err(|e| Error::Generic { - context: format!("GCS: invalid header name: {header_name}"), - cause: Some(Box::new(e)), + HeaderName::try_from(&header_name).map_err(|e| { + Error::generic_cause(format!("GCS: invalid header name: {header_name}"), e) })?, - value.parse().map_err(|e| Error::Generic { - context: format!("GCS: invalid header value for {header_name}"), - cause: Some(Box::new(e)), + value.parse().map_err(|e| { + Error::generic_cause(format!("GCS: invalid header value for {header_name}"), e) })?, ); Ok(()) @@ -438,12 +428,11 @@ impl GcsBackend { let path = id.as_storage_path().to_string(); url.path_segments_mut() - .map_err(|()| Error::Generic { - context: format!( + .map_err(|()| { + Error::generic(format!( "GCS: invalid endpoint URL, {} cannot be a base", self.endpoint - ), - cause: None, + )) })? .extend(&["storage", "v1", "b", &self.bucket, "o", &path]); @@ -455,12 +444,11 @@ impl GcsBackend { let mut url = self.endpoint.clone(); url.path_segments_mut() - .map_err(|()| Error::Generic { - context: format!( + .map_err(|()| { + Error::generic(format!( "GCS: invalid endpoint URL, {} cannot be a base", self.endpoint - ), - cause: None, + )) })? .extend(&["upload", "storage", "v1", "b", &self.bucket, "o"]); @@ -480,12 +468,11 @@ impl GcsBackend { fn xml_object_url(&self, id: &ObjectId) -> Result { let mut url = self.endpoint.clone(); { - let mut segments = url.path_segments_mut().map_err(|()| Error::Generic { - context: format!( + let mut segments = url.path_segments_mut().map_err(|()| { + Error::generic(format!( "GCS: invalid endpoint URL, {} cannot be a base", self.endpoint - ), - cause: None, + )) })?; segments.push(&self.bucket); for part in id.as_storage_path().to_string().split('/') { @@ -636,10 +623,8 @@ impl Backend for GcsBackend { // NB: Ensure the order of these fields and that a content-type is attached to them. Both // are required by the GCS API. - let metadata_json = serde_json::to_string(&gcs_metadata).map_err(|cause| Error::Serde { - context: "failed to serialize metadata for GCS upload".to_string(), - cause, - })?; + let metadata_json = serde_json::to_string(&gcs_metadata) + .map_err(|cause| Error::serde("failed to serialize metadata for GCS upload", cause))?; let multipart = multipart::Form::new() .part( @@ -652,9 +637,11 @@ impl Backend for GcsBackend { "media", multipart::Part::stream(Body::wrap_stream(stream)) .mime_str(&metadata.content_type) - .map_err(|e| Error::Generic { - context: format!("invalid mime type: {}", &metadata.content_type), - cause: Some(Box::new(e)), + .map_err(|e| { + Error::generic_cause( + format!("invalid mime type: {}", &metadata.content_type), + e, + ) })?, ); @@ -707,12 +694,9 @@ impl Backend for GcsBackend { match total { Some(total) => return Err(Error::RangeNotSatisfiable { total }), None => { - return Err(Error::Generic { - context: format!( - "GCS: 416 response with invalid Content-Range: {raw:?}" - ), - cause: None, - }); + return Err(Error::generic(format!( + "GCS: 416 response with invalid Content-Range: {raw:?}" + ))); } } } @@ -728,9 +712,8 @@ impl Backend for GcsBackend { .get(header::CONTENT_RANGE) .and_then(|v| v.to_str().ok()) .and_then(|s| s.parse::().ok()) - .ok_or_else(|| Error::Generic { - context: "GCS: 206 response missing valid Content-Range header".to_owned(), - cause: None, + .ok_or_else(|| { + Error::generic("GCS: 206 response missing valid Content-Range header") })?, ) } else { @@ -920,9 +903,8 @@ impl MultipartUploadBackend for GcsBackend { .map_err(|e| Error::reqwest("GCS: read initiate multipart body", e))?; let xml: XmlInitiateMultipartUploadResponse = quick_xml::de::from_reader(body.as_ref()) - .map_err(|e| Error::Generic { - context: "GCS: failed to parse initiate multipart response".to_owned(), - cause: Some(Box::new(e)), + .map_err(|e| { + Error::generic_cause("GCS: failed to parse initiate multipart response", e) })?; Ok(xml.try_into()?) @@ -1000,11 +982,8 @@ impl MultipartUploadBackend for GcsBackend { .await .map_err(|e| Error::reqwest("GCS: read list parts body", e))?; - let xml: XmlListPartsResponse = - quick_xml::de::from_reader(body.as_ref()).map_err(|e| Error::Generic { - context: "GCS: failed to parse list parts response".to_owned(), - cause: Some(Box::new(e)), - })?; + let xml: XmlListPartsResponse = quick_xml::de::from_reader(body.as_ref()) + .map_err(|e| Error::generic_cause("GCS: failed to parse list parts response", e))?; Ok(xml.into()) } @@ -1041,9 +1020,8 @@ impl MultipartUploadBackend for GcsBackend { url.query_pairs_mut().append_pair("uploadId", upload_id); let body = XmlCompleteMultipartUpload::from(parts); - let xml = quick_xml::se::to_string(&body).map_err(|e| Error::Generic { - context: "GCS: failed to serialize complete multipart request".into(), - cause: Some(Box::new(e)), + let xml = quick_xml::se::to_string(&body).map_err(|e| { + Error::generic_cause("GCS: failed to serialize complete multipart request", e) })?; let resp = self diff --git a/objectstore-service/src/backend/local_fs.rs b/objectstore-service/src/backend/local_fs.rs index ae3f8c86..3c13f570 100644 --- a/objectstore-service/src/backend/local_fs.rs +++ b/objectstore-service/src/backend/local_fs.rs @@ -96,10 +96,8 @@ impl Backend for LocalFsBackend { let mut reader = pin!(StreamReader::new(stream)); let mut writer = BufWriter::new(file); - let metadata_json = serde_json::to_string(metadata).map_err(|cause| Error::Serde { - context: "failed to serialize metadata".to_string(), - cause, - })?; + let metadata_json = serde_json::to_string(metadata) + .map_err(|cause| Error::serde("failed to serialize metadata", cause))?; writer.write_all(metadata_json.as_bytes()).await?; writer.write_all(b"\n").await?; @@ -136,11 +134,8 @@ impl Backend for LocalFsBackend { let mut metadata_line = String::new(); reader.read_line(&mut metadata_line).await?; let file_len = reader.get_ref().metadata().await?.len(); - let mut metadata: Metadata = - serde_json::from_str(metadata_line.trim_end()).map_err(|cause| Error::Serde { - context: "failed to deserialize metadata".to_string(), - cause, - })?; + let mut metadata: Metadata = serde_json::from_str(metadata_line.trim_end()) + .map_err(|cause| Error::serde("failed to deserialize metadata", cause))?; let payload_size = file_len .checked_sub(metadata_line.len() as u64) .ok_or_else(|| Error::generic("local-fs file corrupted: shorter than header"))?; @@ -199,10 +194,8 @@ impl MultipartUploadBackend for LocalFsBackend { tokio::fs::create_dir_all(&dir).await?; let meta_path = dir.join("metadata.json"); - let metadata_json = serde_json::to_string(metadata).map_err(|cause| Error::Serde { - context: "failed to serialize multipart metadata".to_string(), - cause, - })?; + let metadata_json = serde_json::to_string(metadata) + .map_err(|cause| Error::serde("failed to serialize multipart metadata", cause))?; tokio::fs::write(meta_path, metadata_json).await?; Ok(upload_id) @@ -229,10 +222,8 @@ impl MultipartUploadBackend for LocalFsBackend { "uploaded_at": SystemTime::now(), "size": content_length, }); - let header_line = serde_json::to_string(&header).map_err(|cause| Error::Serde { - context: "failed to serialize part header".to_string(), - cause, - })?; + let header_line = serde_json::to_string(&header) + .map_err(|cause| Error::serde("failed to serialize part header", cause))?; let part_path = dir.join(format!("{part_number}.part")); let file = OpenOptions::new() @@ -299,11 +290,8 @@ impl MultipartUploadBackend for LocalFsBackend { let mut reader = BufReader::new(file); let mut header_line = String::new(); reader.read_line(&mut header_line).await?; - let header: serde_json::Value = - serde_json::from_str(header_line.trim_end()).map_err(|cause| Error::Serde { - context: "failed to deserialize part header".to_string(), - cause, - })?; + let header: serde_json::Value = serde_json::from_str(header_line.trim_end()) + .map_err(|cause| Error::serde("failed to deserialize part header", cause))?; parts.push(Part { part_number: pn, @@ -359,11 +347,8 @@ impl MultipartUploadBackend for LocalFsBackend { // Read metadata let meta_path = dir.join("metadata.json"); let meta_bytes = tokio::fs::read(&meta_path).await?; - let metadata: Metadata = - serde_json::from_slice(&meta_bytes).map_err(|cause| Error::Serde { - context: "failed to deserialize multipart metadata".to_string(), - cause, - })?; + let metadata: Metadata = serde_json::from_slice(&meta_bytes) + .map_err(|cause| Error::serde("failed to deserialize multipart metadata", cause))?; // TODO: validate that parts are in ascending part_number order and reject with // InvalidPartOrder if not (matches S3/GCS behavior). Needs a proper client error variant. @@ -382,11 +367,8 @@ impl MultipartUploadBackend for LocalFsBackend { let mut reader = BufReader::new(file); let mut header_line = String::new(); reader.read_line(&mut header_line).await?; - let header: serde_json::Value = - serde_json::from_str(header_line.trim_end()).map_err(|cause| Error::Serde { - context: "failed to deserialize part header".to_string(), - cause, - })?; + let header: serde_json::Value = serde_json::from_str(header_line.trim_end()) + .map_err(|cause| Error::serde("failed to deserialize part header", cause))?; let stored_etag = header["etag"].as_str().unwrap_or(""); if stored_etag != completed.etag { @@ -411,10 +393,8 @@ impl MultipartUploadBackend for LocalFsBackend { .await?; let mut writer = BufWriter::new(file); - let metadata_json = serde_json::to_string(&metadata).map_err(|cause| Error::Serde { - context: "failed to serialize metadata".to_string(), - cause, - })?; + let metadata_json = serde_json::to_string(&metadata) + .map_err(|cause| Error::serde("failed to serialize metadata", cause))?; writer.write_all(metadata_json.as_bytes()).await?; writer.write_all(b"\n").await?; diff --git a/objectstore-service/src/backend/s3_compatible.rs b/objectstore-service/src/backend/s3_compatible.rs index 6987cd3a..77eca3d9 100644 --- a/objectstore-service/src/backend/s3_compatible.rs +++ b/objectstore-service/src/backend/s3_compatible.rs @@ -148,7 +148,7 @@ where .get_token() .await .map_err(|err| Error::Generic { - context: "S3: failed to get authentication token".to_owned(), + context: "S3: failed to get authentication token".into(), cause: Some(err.into()), })? .as_str(), @@ -172,10 +172,10 @@ where if let Some(r) = range { builder = builder.header(reqwest::header::RANGE, r.to_header_value()); } - let response = builder.send().await.map_err(|cause| Error::Reqwest { - context: "S3: failed to send request".to_string(), - cause, - })?; + let response = builder + .send() + .await + .map_err(|cause| Error::reqwest("S3: failed to send request", cause))?; if response.status() == StatusCode::NOT_FOUND { objectstore_log::debug!("Object not found"); @@ -191,10 +191,9 @@ where match total { Some(total) => return Err(Error::RangeNotSatisfiable { total }), None => { - return Err(Error::Generic { - context: format!("S3: 416 response with invalid Content-Range: {raw:?}"), - cause: None, - }); + return Err(Error::generic(format!( + "S3: 416 response with invalid Content-Range: {raw:?}" + ))); } } } @@ -209,9 +208,8 @@ where .get(reqwest::header::CONTENT_RANGE) .and_then(|v| v.to_str().ok()) .and_then(|s| s.parse::().ok()) - .ok_or_else(|| Error::Generic { - context: "S3: 206 response missing valid Content-Range header".to_owned(), - cause: None, + .ok_or_else(|| { + Error::generic("S3: 206 response missing valid Content-Range header") })?; metadata.size = Some(range.total as usize); Some(range) @@ -341,10 +339,7 @@ impl Backend for S3CompatibleBackend { .await? .send() .await - .map_err(|cause| Error::Reqwest { - context: "S3: failed to send delete request".to_string(), - cause, - })?; + .map_err(|cause| Error::reqwest("S3: failed to send delete request", cause))?; // Do not error for objects that do not exist. if response.status() != StatusCode::NOT_FOUND { diff --git a/objectstore-service/src/backend/tiered.rs b/objectstore-service/src/backend/tiered.rs index 204613bb..878f1f2e 100644 --- a/objectstore-service/src/backend/tiered.rs +++ b/objectstore-service/src/backend/tiered.rs @@ -593,7 +593,7 @@ impl TryFrom<&UploadId> for TieredUploadId { fn try_from(value: &UploadId) -> Result { let json = base64::engine::general_purpose::URL_SAFE_NO_PAD .decode(value.as_bytes()) - .map_err(|e| Error::generic(format!("invalid multipart upload ID: {e}")))?; + .map_err(|e| Error::generic_cause("invalid multipart upload ID", e))?; serde_json::from_slice(&json).map_err(|e| Error::serde("decoding multipart token", e)) } } diff --git a/objectstore-service/src/error.rs b/objectstore-service/src/error.rs index 9643dd43..12b7c701 100644 --- a/objectstore-service/src/error.rs +++ b/objectstore-service/src/error.rs @@ -4,6 +4,7 @@ //! and backend-specific failures. [`Result`] is the corresponding alias. use std::any::Any; +use std::borrow::Cow; use std::fmt; use objectstore_log::Level; @@ -62,7 +63,7 @@ pub enum Error { #[error("serde error: {context}")] Serde { /// Context describing what was being serialized/deserialized. - context: String, + context: &'static str, /// The underlying serde error. #[source] cause: serde_json::Error, @@ -75,7 +76,7 @@ pub enum Error { #[error("reqwest error: {context}")] Reqwest { /// Context describing the request that failed. - context: String, + context: &'static str, /// The underlying reqwest error. #[source] cause: reqwest::Error, @@ -138,7 +139,7 @@ pub enum Error { #[error("storage backend error: {context}")] Generic { /// Context describing the operation that failed. - context: String, + context: Cow<'static, str>, /// The underlying error, if available. #[source] cause: Option>, @@ -167,29 +168,34 @@ impl Error { } /// Creates an [`Error::Reqwest`] from a reqwest error with context. - pub fn reqwest(context: impl Into, cause: reqwest::Error) -> Self { - Self::Reqwest { - context: context.into(), - cause, - } + pub fn reqwest(context: &'static str, cause: reqwest::Error) -> Self { + Self::Reqwest { context, cause } } /// Creates an [`Error::Serde`] from a serde error with context. - pub fn serde(context: impl Into, cause: serde_json::Error) -> Self { - Self::Serde { - context: context.into(), - cause, - } + pub fn serde(context: &'static str, cause: serde_json::Error) -> Self { + Self::Serde { context, cause } } /// Creates an [`Error::Generic`] with a context string and no cause. - pub fn generic(context: impl Into) -> Self { + pub fn generic(context: impl Into>) -> Self { Self::Generic { context: context.into(), cause: None, } } + /// Creates an [`Error::Generic`] with a context string and a cause. + pub fn generic_cause( + context: impl Into>, + cause: impl std::error::Error + Send + Sync + 'static, + ) -> Self { + Self::Generic { + context: context.into(), + cause: Some(Box::new(cause)), + } + } + /// Returns the appropriate log level for this error. pub fn level(&self) -> Level { match self {