Add Headers.get_as_optional_string and get_as_string (#1114)

<!--
The title and description of pull requests will be used when creating a
squash commit to the base branch (usually `main`).
Please keep them both up-to-date as the code change evolves, to ensure
that the commit message is useful for future readers.
-->

## Description of change

Refactors `Headers` to have two new public methods:
`get_as_optional_string` and `get_as_string`.

Refactor `head_object` and `put_object` to use new header methods rather
than custom implementations

<!--
    Please describe your contribution here.
    What is the change and why are you making it?
-->

Relevant issues: N/A

## Does this change impact existing behavior?

Changes log format slightly by making "Header string was not valid" text
part of HeadersError.

<!-- Please confirm there's no breaking change, or call our any behavior
changes you think are necessary. -->

## Does this change need a changelog entry in any of the crates?

No

<!--
    Please confirm yes or no.
    If no, add justification. If unsure, ask a reviewer.

    You can find the changelog for each crate here:
-
https://github.com/awslabs/mountpoint-s3/blob/main/mountpoint-s3/CHANGELOG.md
-
https://github.com/awslabs/mountpoint-s3/blob/main/mountpoint-s3-client/CHANGELOG.md
-
https://github.com/awslabs/mountpoint-s3/blob/main/mountpoint-s3-crt/CHANGELOG.md
-
https://github.com/awslabs/mountpoint-s3/blob/main/mountpoint-s3-crt-sys/CHANGELOG.md
-->

---

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and I agree to the terms of
the [Developer Certificate of Origin
(DCO)](https://developercertificate.org/).

---------

Signed-off-by: Simon Beal <[email protected]>

mountpoint-s3-client/src/s3_crt_client/head_object.rs

@@ -1,4 +1,3 @@
-use std::ffi::OsString;
 use std::str::FromStr;
 use std::sync::{Arc, Mutex};
 
@@ -24,9 +23,6 @@ pub enum ParseError {
     #[error("Header response error: {0}")]
     Header(#[from] HeadersError),
 
-    #[error("Header string was not valid: {0:?}")]
-    Invalid(OsString),
-
     #[error("Failed to parse field {1} as OffsetDateTime: {0:?}")]
     OffsetDateTime(#[source] time::error::Parse, String),
 
@@ -37,24 +33,6 @@ pub enum ParseError {
     InvalidRestore(String),
 }
 
-fn get_field(headers: &Headers, name: &str) -> Result<String, ParseError> {
-    let header = headers.get(name)?;
-    let value = header.value();
-    if let Some(s) = value.to_str() {
-        Ok(s.to_string())
-    } else {
-        Err(ParseError::Invalid(value.clone()))
-    }
-}
-
-fn get_optional_field(headers: &Headers, name: &str) -> Result<Option<String>, ParseError> {
-    Ok(if headers.has_header(name) {
-        Some(get_field(headers, name)?)
-    } else {
-        None
-    })
-}
-
 lazy_static! {
     // Example: ongoing-request="true"
     static ref RESTORE_IN_PROGRESS_RE: Regex = Regex::new(r#"^ongoing-request="(?<ongoing>[^"]*)"$"#).unwrap();
@@ -66,7 +44,7 @@ lazy_static! {
 
 impl HeadObjectResult {
     fn parse_restore_status(headers: &Headers) -> Result<Option<RestoreStatus>, ParseError> {
-        let Some(header) = get_optional_field(headers, "x-amz-restore")? else {
+        let Some(header) = headers.get_as_optional_string("x-amz-restore")? else {
             return Ok(None);
         };
 
@@ -88,10 +66,10 @@ impl HeadObjectResult {
     }
 
     fn parse_checksum(headers: &Headers) -> Result<Checksum, ParseError> {
-        let checksum_crc32 = get_optional_field(headers, "x-amz-checksum-crc32")?;
-        let checksum_crc32c = get_optional_field(headers, "x-amz-checksum-crc32c")?;
-        let checksum_sha1 = get_optional_field(headers, "x-amz-checksum-sha1")?;
-        let checksum_sha256 = get_optional_field(headers, "x-amz-checksum-sha256")?;
+        let checksum_crc32 = headers.get_as_optional_string("x-amz-checksum-crc32")?;
+        let checksum_crc32c = headers.get_as_optional_string("x-amz-checksum-crc32c")?;
+        let checksum_sha1 = headers.get_as_optional_string("x-amz-checksum-sha1")?;
+        let checksum_sha256 = headers.get_as_optional_string("x-amz-checksum-sha256")?;
 
         Ok(Checksum {
             checksum_crc32,
@@ -103,12 +81,12 @@ impl HeadObjectResult {
 
     /// Parse from HeadObject headers
     fn parse_from_hdr(headers: &Headers) -> Result<Self, ParseError> {
-        let last_modified = OffsetDateTime::parse(&get_field(headers, "Last-Modified")?, &Rfc2822)
+        let last_modified = OffsetDateTime::parse(&headers.get_as_string("Last-Modified")?, &Rfc2822)
             .map_err(|e| ParseError::OffsetDateTime(e, "LastModified".into()))?;
-        let size = u64::from_str(&get_field(headers, "Content-Length")?)
+        let size = u64::from_str(&headers.get_as_string("Content-Length")?)
             .map_err(|e| ParseError::Int(e, "ContentLength".into()))?;
-        let etag = get_field(headers, "Etag")?;
-        let storage_class = get_optional_field(headers, "x-amz-storage-class")?;
+        let etag = headers.get_as_string("Etag")?;
+        let storage_class = headers.get_as_optional_string("x-amz-storage-class")?;
         let restore_status = Self::parse_restore_status(headers)?;
         let checksum = Self::parse_checksum(headers)?;
         let result = HeadObjectResult {
@@ -196,6 +174,8 @@ fn parse_head_object_error(result: &MetaRequestResult) -> Option<HeadObjectError
 
 #[cfg(test)]
 mod tests {
+    use std::ffi::OsString;
+
     use mountpoint_s3_crt::common::allocator::Allocator;
     use mountpoint_s3_crt::http::request_response::Header;
 

mountpoint-s3-client/src/s3_crt_client/put_object.rs

@@ -1,4 +1,3 @@
-use std::ffi::OsString;
 use std::sync::{Arc, Mutex};
 use std::time::Instant;
 
@@ -10,7 +9,6 @@ use futures::channel::oneshot::{self, Receiver};
 use mountpoint_s3_crt::http::request_response::{Header, Headers, HeadersError};
 use mountpoint_s3_crt::io::stream::InputStream;
 use mountpoint_s3_crt::s3::client::{ChecksumConfig, RequestType, UploadReview};
-use thiserror::Error;
 use tracing::error;
 
 use super::{
@@ -267,38 +265,19 @@ enum S3PutObjectRequestState {
     Idle,
 }
 
-fn try_get_header_value(headers: &Headers, key: &str) -> Option<String> {
-    headers.get(key).ok()?.value().clone().into_string().ok()
-}
-
-fn get_etag(response_headers: &Headers) -> Result<ETag, ParseError> {
-    Ok(response_headers
-        .get(ETAG_HEADER_NAME)?
-        .value()
-        .clone()
-        .into_string()
-        .map_err(ParseError::Invalid)?
-        .into())
-}
-
-#[derive(Error, Debug)]
-#[non_exhaustive]
-pub enum ParseError {
-    #[error("Header response error: {0}")]
-    Header(#[from] HeadersError),
-
-    #[error("Header string was not valid: {0:?}")]
-    Invalid(OsString),
+fn get_etag(response_headers: &Headers) -> Result<ETag, HeadersError> {
+    Ok(response_headers.get_as_string(ETAG_HEADER_NAME)?.into())
 }
 
 fn extract_result(response_headers: Headers) -> Result<PutObjectResult, S3RequestError> {
-    let etag = get_etag(&response_headers).map_err(|e| S3RequestError::InternalError(Box::new(e)))?;
-
-    Ok(PutObjectResult {
-        etag,
-        sse_type: try_get_header_value(&response_headers, SSE_TYPE_HEADER_NAME),
-        sse_kms_key_id: try_get_header_value(&response_headers, SSE_KEY_ID_HEADER_NAME),
-    })
+    fn extract_result_headers_err(response_headers: Headers) -> Result<PutObjectResult, HeadersError> {
+        Ok(PutObjectResult {
+            etag: get_etag(&response_headers)?,
+            sse_type: response_headers.get_as_optional_string(SSE_TYPE_HEADER_NAME)?,
+            sse_kms_key_id: response_headers.get_as_optional_string(SSE_KEY_ID_HEADER_NAME)?,
+        })
+    }
+    extract_result_headers_err(response_headers).map_err(|e| S3RequestError::InternalError(Box::new(e)))
 }
 
 /// Creates `on_headers` callback that will send the response headers to the matching `Receiver`.

mountpoint-s3-crt/src/http/request_response.rs

@@ -72,6 +72,10 @@ pub enum HeadersError {
     /// Internal CRT error
     #[error("CRT error: {0}")]
     CrtError(#[source] Error),
+
+    /// Header value could not be converted to String
+    #[error("Header string was not valid: {0:?}")]
+    Invalid(OsString),
 }
 
 // Convert CRT error into HeadersError, mapping the HEADER_NOT_FOUND to HeadersError::HeaderNotFound.
@@ -197,6 +201,26 @@ impl Headers {
         Ok(Header::new(name, value))
     }
 
+    /// Get a single header by name as a [String].
+    pub fn get_as_string<H: AsRef<OsStr>>(&self, name: H) -> Result<String, HeadersError> {
+        let header = self.get(name)?;
+        let value = header.value();
+        if let Some(s) = value.to_str() {
+            Ok(s.to_string())
+        } else {
+            Err(HeadersError::Invalid(value.clone()))
+        }
+    }
+
+    /// Get an optional header by name as a [String].
+    pub fn get_as_optional_string<H: AsRef<OsStr>>(&self, name: H) -> Result<Option<String>, HeadersError> {
+        Ok(if self.has_header(&name) {
+            Some(self.get_as_string(name)?)
+        } else {
+            None
+        })
+    }
+
     /// Iterate over the headers as (name, value) pairs.
     pub fn iter(&self) -> impl Iterator<Item = (OsString, OsString)> + '_ {
         HeadersIterator {
@@ -380,6 +404,9 @@ mod test {
         assert_eq!(headers.get("a").unwrap().name(), "a");
         assert_eq!(headers.get("a").unwrap().value(), "1");
 
+        assert_eq!(headers.get_as_string("a"), Ok("1".to_string()));
+        assert_eq!(headers.get_as_optional_string("a"), Ok(Some("1".to_string())));
+
         let map: HashMap<OsString, OsString> = headers.iter().collect();
 
         assert_eq!(map.len(), 3);
@@ -393,6 +420,16 @@ mod test {
         assert!(!headers.has_header("a"));
         let error = headers.get("a").expect_err("should fail because header is not present");
         assert_eq!(error, HeadersError::HeaderNotFound, "should fail with HeaderNotFound");
+
+        let error = headers
+            .get_as_string("a")
+            .expect_err("should fail because header is not present");
+        assert_eq!(error, HeadersError::HeaderNotFound, "should fail with HeaderNotFound");
+
+        let header = headers
+            .get_as_optional_string("a")
+            .expect("Should not fail as optional is expected here");
+        assert_eq!(header, None, "should return None");
     }
 
     /// Test setting the same header twice, which should overwrite with the second value.