From 68e36c90f36c7debebfc0df3b9ab25b8113674ba Mon Sep 17 00:00:00 2001 From: Alessandro Passaro Date: Thu, 16 Nov 2023 11:10:09 +0000 Subject: [PATCH] Fix bug in ChecksummedBytes::into_inner (#609) * Fix bug in ChecksummedBytes::into_inner `ChecksummedBytes::into_inner()` was returning data from `self` rather than from the `shrink_to_fit` result. Added regression tests for `ChecksummedBytes` and for `DiskDataCache` (only caller of the `into_inner()`). Signed-off-by: Alessandro Passaro * Mark ChecksummedBytes as must_use Signed-off-by: Alessandro Passaro --------- Signed-off-by: Alessandro Passaro --- mountpoint-s3/src/checksums.rs | 28 +++++++++++++++---- .../src/data_cache/disk_data_cache.rs | 26 +++++++++++++++++ 2 files changed, 48 insertions(+), 6 deletions(-) diff --git a/mountpoint-s3/src/checksums.rs b/mountpoint-s3/src/checksums.rs index 70f2547ac..087153064 100644 --- a/mountpoint-s3/src/checksums.rs +++ b/mountpoint-s3/src/checksums.rs @@ -10,6 +10,7 @@ use thiserror::Error; /// Data transformations will either fail returning an [IntegrityError], or propagate the checksum /// so that it can be validated on access. #[derive(Clone, Debug)] +#[must_use] pub struct ChecksummedBytes { orig_bytes: Bytes, /// Always a subslice of `orig_bytes` @@ -158,8 +159,8 @@ impl ChecksummedBytes { /// /// If you are only interested in the underlying bytes, **you should use `into_bytes()`**. pub fn into_inner(self) -> Result<(Bytes, Crc32c), IntegrityError> { - self.shrink_to_fit()?; - Ok((self.curr_slice, self.checksum)) + let fit = self.shrink_to_fit()?; + Ok((fit.curr_slice, fit.checksum)) } } @@ -322,6 +323,21 @@ mod tests { assert!(matches!(result, Err(IntegrityError::ChecksumMismatch(_, _)))); } + #[test] + fn test_into_inner() { + let original = ChecksummedBytes::from_bytes(Bytes::from_static(b"some bytes")); + let (unchanged_bytes, unchanged_checksum) = original.clone().into_inner().unwrap(); + assert_eq!(original.curr_slice, unchanged_bytes); + assert_eq!(original.orig_bytes, unchanged_bytes); + assert_eq!(original.checksum, unchanged_checksum); + + let slice = original.clone().split_off(5); + let (shrunken_bytes, shrunken_checksum) = slice.clone().into_inner().unwrap(); + assert_eq!(slice.curr_slice, shrunken_bytes); + assert_ne!(slice.orig_bytes, shrunken_bytes); + assert_ne!(slice.checksum, shrunken_checksum); + } + #[test] fn test_extend() { let expected = Bytes::from_static(b"some bytes extended"); @@ -339,8 +355,8 @@ mod tests { let expected = Bytes::from_static(b"some ext"); let mut checksummed_bytes = ChecksummedBytes::from_bytes(Bytes::from_static(b"some bytes")); let mut extend = ChecksummedBytes::from_bytes(Bytes::from_static(b" extended")); - checksummed_bytes.split_off(split_off_at); - extend.split_off(split_off_at); + _ = checksummed_bytes.split_off(split_off_at); + _ = extend.split_off(split_off_at); checksummed_bytes.extend(extend).unwrap(); let actual = checksummed_bytes.curr_slice; assert_eq!(expected, actual); @@ -375,7 +391,7 @@ mod tests { checksummed_bytes.validate(), Err(IntegrityError::ChecksumMismatch(_, _)) )); - checksummed_bytes.split_off(4); + _ = checksummed_bytes.split_off(4); let extend = ChecksummedBytes::from_bytes(Bytes::from_static(b" extended")); assert!(matches!(extend.validate(), Ok(()))); @@ -426,7 +442,7 @@ mod tests { let corrupted_bytes = Bytes::from_static(b"corrupted data"); let extend_checksum = crc32c::checksum(b" extended"); let mut extend = ChecksummedBytes::new(corrupted_bytes, extend_checksum); - extend.split_off(4); + _ = extend.split_off(4); assert!(matches!(extend.validate(), Err(IntegrityError::ChecksumMismatch(_, _)))); let result = checksummed_bytes.extend(extend); diff --git a/mountpoint-s3/src/data_cache/disk_data_cache.rs b/mountpoint-s3/src/data_cache/disk_data_cache.rs index 87c6dfad1..c69c82957 100644 --- a/mountpoint-s3/src/data_cache/disk_data_cache.rs +++ b/mountpoint-s3/src/data_cache/disk_data_cache.rs @@ -420,6 +420,32 @@ mod tests { ); } + #[test] + fn test_checksummed_bytes_slice() { + let data = ChecksummedBytes::from_bytes("0123456789".into()); + let slice = data.slice(1..5); + + let cache_directory = tempfile::tempdir().unwrap(); + let cache = DiskDataCache::new(cache_directory.into_path(), 8 * 1024 * 1024); + let cache_key = CacheKey { + s3_key: "a".into(), + etag: ETag::for_tests(), + }; + + cache + .put_block(cache_key.clone(), 0, slice.clone()) + .expect("cache should be accessible"); + let entry = cache + .get_block(&cache_key, 0) + .expect("cache should be accessible") + .expect("cache entry should be returned"); + assert_eq!( + slice.into_bytes().expect("original slice should be valid"), + entry.into_bytes().expect("returned entry should be valid"), + "cache entry returned should match original slice after put" + ); + } + #[test] fn data_block_extract_checks() { let data_1 = ChecksummedBytes::from_bytes("Foo".into());