From 61a58783ea9efeeba25d8ef39f7c380cb18e440f Mon Sep 17 00:00:00 2001 From: Daniel Carl Jones Date: Tue, 17 Dec 2024 15:38:57 +0000 Subject: [PATCH] Update HeadersError::HeaderNotFound to include header name Signed-off-by: Daniel Carl Jones --- mountpoint-s3-crt/CHANGELOG.md | 3 + .../src/http/request_response.rs | 66 +++++++++++++------ 2 files changed, 49 insertions(+), 20 deletions(-) diff --git a/mountpoint-s3-crt/CHANGELOG.md b/mountpoint-s3-crt/CHANGELOG.md index e98ca69a7..5c384135f 100644 --- a/mountpoint-s3-crt/CHANGELOG.md +++ b/mountpoint-s3-crt/CHANGELOG.md @@ -4,6 +4,9 @@ * Checksum hashers no longer implement `std::hash::Hasher`. ([#1082](https://github.com/awslabs/mountpoint-s3/pull/1082)) * Add bindings to remaining checksum types CRC64, SHA1, and SHA256. ([#1082](https://github.com/awslabs/mountpoint-s3/pull/1082)) * Add wrapping type `ByteBuf` for `aws_byte_buf`. ([#1082](https://github.com/awslabs/mountpoint-s3/pull/1082)) +* `HeadersError::HeaderNotFound` variant now includes the name of the header. + Despite the new field being private, this may impact any code that was pattern matching on this variant. + ([#1205](https://github.com/awslabs/mountpoint-s3/pull/1205)) ## v0.10.0 (October 17, 2024) diff --git a/mountpoint-s3-crt/src/http/request_response.rs b/mountpoint-s3-crt/src/http/request_response.rs index 06cce753b..47a50dcb9 100644 --- a/mountpoint-s3-crt/src/http/request_response.rs +++ b/mountpoint-s3-crt/src/http/request_response.rs @@ -66,8 +66,8 @@ unsafe impl Sync for Headers {} #[derive(Debug, Error, PartialEq, Eq)] pub enum HeadersError { /// The header was not found - #[error("Header not found")] - HeaderNotFound, + #[error("Header {0:?} not found")] + HeaderNotFound(OsString), /// Internal CRT error #[error("CRT error: {0}")] @@ -78,14 +78,12 @@ pub enum HeadersError { Invalid(OsString), } -// Convert CRT error into HeadersError, mapping the HEADER_NOT_FOUND to HeadersError::HeaderNotFound. -impl From for HeadersError { - fn from(err: Error) -> Self { - if err == (aws_http_errors::AWS_ERROR_HTTP_HEADER_NOT_FOUND as i32).into() { - Self::HeaderNotFound - } else { - Self::CrtError(err) - } +/// Try to convert the CRT [Error] into [HeadersError::HeaderNotFound], or return [HeadersError::CrtError]. +fn try_as_header_not_found(err: Error, header_name: &OsStr) -> HeadersError { + if err.raw_error() == (aws_http_errors::AWS_ERROR_HTTP_HEADER_NOT_FOUND as i32) { + HeadersError::HeaderNotFound(header_name.to_owned()) + } else { + HeadersError::CrtError(err) } } @@ -105,7 +103,11 @@ impl Headers { /// Create a new [Headers] object in the given allocator. pub fn new(allocator: &Allocator) -> Result { // SAFETY: allocator is a valid aws_allocator, and we check the return is non-null. - let inner = unsafe { aws_http_headers_new(allocator.inner.as_ptr()).ok_or_last_error()? }; + let inner = unsafe { + aws_http_headers_new(allocator.inner.as_ptr()) + .ok_or_last_error() + .map_err(HeadersError::CrtError)? + }; Ok(Self { inner }) } @@ -118,12 +120,14 @@ impl Headers { } /// Get the header at the specified index. - pub fn get_index(&self, index: usize) -> Result, HeadersError> { + fn get_index(&self, index: usize) -> Result, HeadersError> { // SAFETY: `self.inner` is a valid aws_http_headers, and `aws_http_headers_get_index` // promises to initialize the output `struct aws_http_header *out_header` on success. let header = unsafe { let mut header: MaybeUninit = MaybeUninit::uninit(); - aws_http_headers_get_index(self.inner.as_ptr(), index, header.as_mut_ptr()).ok_or_last_error()?; + aws_http_headers_get_index(self.inner.as_ptr(), index, header.as_mut_ptr()) + .ok_or_last_error() + .map_err(HeadersError::CrtError)?; header.assume_init() }; @@ -153,7 +157,9 @@ impl Headers { // SAFETY: `aws_http_headers_add_header` makes a copy of the underlying strings. // Also, this function takes a mut reference to `self`, since this function modifies the headers. unsafe { - aws_http_headers_add_header(self.inner.as_ptr(), &header.inner).ok_or_last_error()?; + aws_http_headers_add_header(self.inner.as_ptr(), &header.inner) + .ok_or_last_error() + .map_err(HeadersError::CrtError)?; } Ok(()) @@ -171,7 +177,9 @@ impl Headers { // SAFETY: `aws_http_headers_erase` doesn't hold on to a copy of the name we pass in, so it's // okay to call with with an `aws_byte_cursor` that may not outlive this `Headers`. unsafe { - aws_http_headers_erase(self.inner.as_ptr(), name.as_ref().as_aws_byte_cursor()).ok_or_last_error()?; + aws_http_headers_erase(self.inner.as_ptr(), name.as_ref().as_aws_byte_cursor()) + .ok_or_last_error() + .map_err(|err| try_as_header_not_found(err, name.as_ref()))?; } Ok(()) @@ -183,12 +191,15 @@ impl Headers { // initialize the output `struct aws_byte_cursor *out_value` on success. let value = unsafe { let mut value: MaybeUninit = MaybeUninit::uninit(); + aws_http_headers_get( self.inner.as_ptr(), name.as_ref().as_aws_byte_cursor(), value.as_mut_ptr(), ) - .ok_or_last_error()?; + .ok_or_last_error() + .map_err(|err| try_as_header_not_found(err, name.as_ref()))?; + value.assume_init() }; @@ -263,7 +274,7 @@ impl Iterator for HeadersIterator<'_> { let header = self .headers .get_index(self.offset) - .expect("HeadersIterator: failed to get next header"); + .expect("headers at any offset smaller than original count should always exist given mut access"); self.offset += 1; Some((header.name, header.value)) @@ -417,14 +428,29 @@ mod test { #[test] fn test_header_not_present() { let headers = Headers::new(&Allocator::default()).expect("failed to create headers"); - assert!(!headers.has_header("a")); + + assert!(!headers.has_header("a"), "header should not be present"); + let error = headers.get("a").expect_err("should fail because header is not present"); - assert_eq!(error, HeadersError::HeaderNotFound, "should fail with HeaderNotFound"); + assert_eq!( + error.to_string(), + "Header \"a\" not found", + "header error display should match expected output", + ); + if let HeadersError::HeaderNotFound(name) = error { + assert_eq!(name, "a", "header name should match original argument"); + } else { + panic!("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"); + if let HeadersError::HeaderNotFound(name) = error { + assert_eq!(name, "a", "header name should match original argument"); + } else { + panic!("should fail with HeaderNotFound"); + } let header = headers .get_as_optional_string("a")