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 <[email protected]> * Mark ChecksummedBytes as must_use Signed-off-by: Alessandro Passaro <[email protected]> --------- Signed-off-by: Alessandro Passaro <[email protected]>
awslabs · Nov 16, 2023 · 68e36c9 · 68e36c9
1 parent 3264f2b
commit 68e36c9
Show file tree

Hide file tree

Showing 2 changed files with 48 additions and 6 deletions.
diff --git 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
@@ -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());