Skip to content

Commit 0773b97

Browse files
author
Test User
committed
Fix incorrect part size in multipart copy
The bug was using size (remaining bytes) instead of length (actual part size) when constructing PartInfo in the multipart copy loop. This would record wrong sizes for each part - especially problematic for the last part.
1 parent 2e94a0e commit 0773b97

26 files changed

+259
-113
lines changed

.github/workflows/rust.yml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ jobs:
3636
export ACCESS_KEY=minioadmin
3737
export SECRET_KEY=minioadmin
3838
export ENABLE_HTTPS=1
39+
export SERVER_REGION=us-east-1
3940
export MINIO_SSL_CERT_FILE=./tests/public.crt
4041
MINIO_TEST_TOKIO_RUNTIME_FLAVOR="multi_thread" cargo test -- --nocapture
4142
test-current-thread:
@@ -49,6 +50,7 @@ jobs:
4950
export ACCESS_KEY=minioadmin
5051
export SECRET_KEY=minioadmin
5152
export ENABLE_HTTPS=1
53+
export SERVER_REGION=us-east-1
5254
export MINIO_SSL_CERT_FILE=./tests/public.crt
5355
MINIO_TEST_TOKIO_RUNTIME_FLAVOR="current_thread" cargo test -- --nocapture
5456

common/src/example.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ pub fn create_bucket_notification_config_example() -> NotificationConfig {
5252
suffix_filter_rule: Some(SuffixFilterRule {
5353
value: String::from("pg"),
5454
}),
55-
queue: String::from("arn:minio:sqs::miniojavatest:webhook"),
55+
queue: String::from("arn:minio:sqs:us-east-1:miniojavatest:webhook"),
5656
}]),
5757
..Default::default()
5858
}

src/s3/builders/append_object.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -61,8 +61,8 @@ pub struct AppendObject {
6161
#[builder(!default)] // force required
6262
data: Arc<SegmentedBytes>,
6363

64+
/// Value of `x-amz-write-offset-bytes`.
6465
#[builder(!default)] // force required
65-
/// value of x-amz-write-offset-bytes
6666
offset_bytes: u64,
6767
}
6868

@@ -141,7 +141,7 @@ pub struct AppendObjectContent {
141141
content_stream: ContentStream,
142142
#[builder(default)]
143143
part_count: Option<u16>,
144-
/// Value of x-amz-write-offset-bytes
144+
/// Value of `x-amz-write-offset-bytes`.
145145
#[builder(default)]
146146
offset_bytes: u64,
147147
}
@@ -243,7 +243,7 @@ impl AppendObjectContent {
243243
}
244244
}
245245

246-
/// multipart append
246+
/// Performs multipart append.
247247
async fn send_mpa(
248248
&mut self,
249249
part_size: u64,

src/s3/builders/bucket_common.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ use crate::s3::multimap_ext::Multimap;
1818
use std::marker::PhantomData;
1919
use typed_builder::TypedBuilder;
2020

21-
/// Common parameters for bucket operations
21+
/// Common parameters for bucket operations.
2222
#[derive(Clone, Debug, TypedBuilder)]
2323
pub struct BucketCommon<T> {
2424
#[builder(!default)] // force required

src/s3/builders/bucket_exists.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,10 @@ use crate::s3::types::{S3Api, S3Request, ToS3Request};
2121
use crate::s3::utils::check_bucket_name;
2222
use http::Method;
2323

24-
/// This struct constructs the parameters required for the [`Client::bucket_exists`](crate::s3::client::MinioClient::bucket_exists) method.
24+
/// Constructs the parameters for the [`Client::bucket_exists`](crate::s3::client::MinioClient::bucket_exists) method.
2525
///
2626
/// See [Amazon S3: Working with Buckets](https://docs.aws.amazon.com/AmazonS3/latest/userguide/UsingBucket.html)
27-
/// for more information about checking if a bucket exists.
27+
/// for more information.
2828
pub type BucketExists = BucketCommon<BucketExistsPhantomData>;
2929

3030
#[doc(hidden)]

src/s3/builders/copy_object.rs

Lines changed: 127 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -587,7 +587,7 @@ impl ComposeObjectInternal {
587587
size -= o;
588588
}
589589

590-
let mut offset = source.offset.unwrap_or_default();
590+
let offset = source.offset.unwrap_or_default();
591591

592592
let mut headers = source.get_headers();
593593
headers.add_multimap(ssec_headers.clone());
@@ -631,19 +631,15 @@ impl ComposeObjectInternal {
631631
size,
632632
});
633633
} else {
634-
while size > 0 {
634+
let part_ranges = calculate_part_ranges(offset, size, MAX_PART_SIZE);
635+
for (part_offset, length) in part_ranges {
635636
part_number += 1;
636-
637-
let mut length = size;
638-
if length > MAX_PART_SIZE {
639-
length = MAX_PART_SIZE;
640-
}
641-
let end_bytes = offset + length - 1;
637+
let end_bytes = part_offset + length - 1;
642638

643639
let mut headers_copy = headers.clone();
644640
headers_copy.add(
645641
X_AMZ_COPY_SOURCE_RANGE,
646-
format!("bytes={offset}-{end_bytes}"),
642+
format!("bytes={part_offset}-{end_bytes}"),
647643
);
648644

649645
let resp: UploadPartCopyResponse = match self
@@ -668,11 +664,8 @@ impl ComposeObjectInternal {
668664
parts.push(PartInfo {
669665
number: part_number,
670666
etag,
671-
size,
667+
size: length,
672668
});
673-
674-
offset += length;
675-
size -= length;
676669
}
677670
}
678671
}
@@ -796,8 +789,8 @@ impl ComposeObject {
796789

797790
// region: misc
798791

792+
/// Source object information for [`compose_object`](MinioClient::compose_object).
799793
#[derive(Clone, Debug, Default)]
800-
/// Source object information for [compose_object](MinioClient::compose_object)
801794
pub struct ComposeSource {
802795
pub extra_headers: Option<Multimap>,
803796
pub extra_query_params: Option<Multimap>,
@@ -818,7 +811,7 @@ pub struct ComposeSource {
818811
}
819812

820813
impl ComposeSource {
821-
/// Returns a compose source with given bucket name and object name
814+
/// Returns a compose source with given bucket name and object name.
822815
///
823816
/// # Examples
824817
///
@@ -927,8 +920,8 @@ impl ComposeSource {
927920
}
928921
}
929922

923+
/// Base argument for object conditional read APIs.
930924
#[derive(Clone, Debug, TypedBuilder)]
931-
/// Base argument for object conditional read APIs
932925
pub struct CopySource {
933926
#[builder(default, setter(into))]
934927
pub extra_headers: Option<Multimap>,
@@ -1036,4 +1029,122 @@ fn into_headers_copy_object(
10361029

10371030
map
10381031
}
1032+
1033+
/// Calculates part ranges (offset, length) for multipart copy operations.
1034+
///
1035+
/// Given a starting offset, total size, and maximum part size, returns a vector of
1036+
/// (offset, length) tuples for each part. This is extracted as a separate function
1037+
/// to enable unit testing without requiring actual S3 operations or multi-gigabyte files.
1038+
///
1039+
/// # Arguments
1040+
/// * `start_offset` - Starting byte offset
1041+
/// * `total_size` - Total bytes to copy
1042+
/// * `max_part_size` - Maximum size per part (typically MAX_PART_SIZE = 5GB)
1043+
///
1044+
/// # Returns
1045+
/// Vector of (offset, length) tuples for each part
1046+
fn calculate_part_ranges(
1047+
start_offset: u64,
1048+
total_size: u64,
1049+
max_part_size: u64,
1050+
) -> Vec<(u64, u64)> {
1051+
let mut ranges = Vec::new();
1052+
let mut offset = start_offset;
1053+
let mut remaining = total_size;
1054+
1055+
while remaining > 0 {
1056+
let length = remaining.min(max_part_size);
1057+
ranges.push((offset, length));
1058+
offset += length;
1059+
remaining -= length;
1060+
}
1061+
1062+
ranges
1063+
}
1064+
10391065
// endregion: misc
1066+
1067+
#[cfg(test)]
1068+
mod tests {
1069+
use super::*;
1070+
1071+
#[test]
1072+
fn test_calculate_part_ranges_single_part() {
1073+
// Size <= max_part_size should return single part
1074+
let ranges = calculate_part_ranges(0, 1000, 5000);
1075+
assert_eq!(ranges, vec![(0, 1000)]);
1076+
}
1077+
1078+
#[test]
1079+
fn test_calculate_part_ranges_exact_multiple() {
1080+
// Size exactly divisible by max_part_size
1081+
let ranges = calculate_part_ranges(0, 10000, 5000);
1082+
assert_eq!(ranges, vec![(0, 5000), (5000, 5000)]);
1083+
}
1084+
1085+
#[test]
1086+
fn test_calculate_part_ranges_with_remainder() {
1087+
// Size with remainder
1088+
let ranges = calculate_part_ranges(0, 12000, 5000);
1089+
assert_eq!(ranges, vec![(0, 5000), (5000, 5000), (10000, 2000)]);
1090+
}
1091+
1092+
#[test]
1093+
fn test_calculate_part_ranges_with_start_offset() {
1094+
// Starting from non-zero offset
1095+
let ranges = calculate_part_ranges(1000, 12000, 5000);
1096+
assert_eq!(ranges, vec![(1000, 5000), (6000, 5000), (11000, 2000)]);
1097+
}
1098+
1099+
#[test]
1100+
fn test_calculate_part_ranges_zero_size() {
1101+
// Zero size edge case - returns empty
1102+
let ranges = calculate_part_ranges(0, 0, 5000);
1103+
assert!(ranges.is_empty());
1104+
}
1105+
1106+
#[test]
1107+
fn test_calculate_part_ranges_realistic() {
1108+
// Simulate 12GB file with 5GB max part size
1109+
let total_size: u64 = 12 * 1024 * 1024 * 1024; // 12 GB
1110+
let max_part_size: u64 = 5 * 1024 * 1024 * 1024; // 5 GB
1111+
1112+
let ranges = calculate_part_ranges(0, total_size, max_part_size);
1113+
1114+
assert_eq!(ranges.len(), 3);
1115+
assert_eq!(ranges[0], (0, max_part_size)); // 0-5GB
1116+
assert_eq!(ranges[1], (max_part_size, max_part_size)); // 5GB-10GB
1117+
assert_eq!(ranges[2], (2 * max_part_size, 2 * 1024 * 1024 * 1024)); // 10GB-12GB
1118+
1119+
// Verify total size matches
1120+
let total: u64 = ranges.iter().map(|(_, len)| len).sum();
1121+
assert_eq!(total, total_size);
1122+
1123+
// Verify offsets are contiguous
1124+
let mut expected_offset = 0;
1125+
for (offset, length) in &ranges {
1126+
assert_eq!(*offset, expected_offset);
1127+
expected_offset += length;
1128+
}
1129+
}
1130+
1131+
#[test]
1132+
fn test_calculate_part_ranges_each_part_correct_length() {
1133+
// This test catches the bug where `size` (remaining) was used instead of `length`
1134+
let ranges = calculate_part_ranges(0, 17000, 5000);
1135+
1136+
// Should be [(0,5000), (5000,5000), (10000,5000), (15000,2000)]
1137+
// NOT [(0,17000), (17000,12000), ...] which the buggy code would produce
1138+
assert_eq!(
1139+
ranges,
1140+
vec![(0, 5000), (5000, 5000), (10000, 5000), (15000, 2000)]
1141+
);
1142+
1143+
// Each non-final part should be exactly max_part_size
1144+
for (i, (_, length)) in ranges.iter().enumerate() {
1145+
if i < ranges.len() - 1 {
1146+
assert_eq!(*length, 5000, "Part {} should be max_part_size", i);
1147+
}
1148+
}
1149+
}
1150+
}

src/s3/builders/delete_objects.rs

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -37,15 +37,16 @@ impl ValidKey for String {}
3737
impl ValidKey for &str {}
3838
impl ValidKey for &String {}
3939

40-
/// Specify an object to be deleted. The object can be specified by key or by
41-
/// key and version_id via the From trait.
40+
/// Specifies an object to be deleted.
41+
///
42+
/// The object can be specified by key or by key and version_id via the `From` trait.
4243
#[derive(Debug, Clone, Default)]
4344
pub struct ObjectToDelete {
4445
key: String,
4546
version_id: Option<String>,
4647
}
4748

48-
/// A key can be converted into a DeleteObject. The version_id is set to None.
49+
/// A key can be converted into a `DeleteObject` with `version_id` set to `None`.
4950
impl<K: ValidKey> From<K> for ObjectToDelete {
5051
fn from(key: K) -> Self {
5152
Self {
@@ -55,7 +56,7 @@ impl<K: ValidKey> From<K> for ObjectToDelete {
5556
}
5657
}
5758

58-
/// A tuple of key and version_id can be converted into a DeleteObject.
59+
/// A tuple of key and version_id can be converted into a `DeleteObject`.
5960
impl<K: ValidKey> From<(K, &str)> for ObjectToDelete {
6061
fn from((key, version_id): (K, &str)) -> Self {
6162
Self {
@@ -65,7 +66,7 @@ impl<K: ValidKey> From<(K, &str)> for ObjectToDelete {
6566
}
6667
}
6768

68-
/// A tuple of key and option version_id can be converted into a DeleteObject.
69+
/// A tuple of key and optional version_id can be converted into a `DeleteObject`.
6970
impl<K: ValidKey> From<(K, Option<&str>)> for ObjectToDelete {
7071
fn from((key, version_id): (K, Option<&str>)) -> Self {
7172
Self {
@@ -178,9 +179,10 @@ pub struct DeleteObjects {
178179
#[builder(default)]
179180
bypass_governance_mode: bool,
180181

181-
/// Enable verbose mode (defaults to false). If enabled, the response will
182-
/// include the keys of objects that were successfully deleted. Otherwise,
183-
/// only objects that encountered an error are returned.
182+
/// Enables verbose mode (defaults to false).
183+
///
184+
/// If enabled, the response will include the keys of objects that were successfully
185+
/// deleted. Otherwise, only objects that encountered an error are returned.
184186
#[builder(default)]
185187
verbose_mode: bool,
186188
}
@@ -320,9 +322,10 @@ impl DeleteObjectsStreaming {
320322
self
321323
}
322324

323-
/// Enable verbose mode (defaults to false). If enabled, the response will
324-
/// include the keys of objects that were successfully deleted. Otherwise
325-
/// only objects that encountered an error are returned.
325+
/// Enables verbose mode (defaults to false).
326+
///
327+
/// If enabled, the response will include the keys of objects that were successfully
328+
/// deleted. Otherwise, only objects that encountered an error are returned.
326329
pub fn verbose_mode(mut self, verbose_mode: bool) -> Self {
327330
self.verbose_mode = verbose_mode;
328331
self
@@ -338,7 +341,7 @@ impl DeleteObjectsStreaming {
338341
self
339342
}
340343

341-
/// Sets the region for the request
344+
/// Sets the region for the request.
342345
pub fn region(mut self, region: Option<String>) -> Self {
343346
self.region = region;
344347
self

src/s3/builders/get_presigned_policy_form_data.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -61,10 +61,10 @@ impl GetPresignedPolicyFormData {
6161
pub type GetPresignedPolicyFormDataBldr =
6262
GetPresignedPolicyFormDataBuilder<((MinioClient,), (PostPolicy,))>;
6363

64-
/// Post policy information for presigned post policy form-data
64+
/// Post policy information for presigned POST policy form-data.
6565
///
66-
/// Condition elements and respective condition for Post policy is available <a
67-
/// href="https://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-HTTPPOSTConstructPolicy.html#sigv4-PolicyConditions">here</a>.
66+
/// See [Post Policy Conditions](https://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-HTTPPOSTConstructPolicy.html#sigv4-PolicyConditions)
67+
/// for condition elements and their usage.
6868
#[derive(Clone, Debug)]
6969
pub struct PostPolicy {
7070
pub region: Option<String>,
@@ -82,7 +82,7 @@ impl PostPolicy {
8282
const STARTS_WITH: &'static str = "starts-with";
8383
const ALGORITHM: &'static str = "AWS4-HMAC-SHA256";
8484

85-
/// Returns post policy with given bucket name and expiration
85+
/// Returns a post policy with given bucket name and expiration.
8686
///
8787
/// # Examples
8888
///

0 commit comments

Comments
 (0)