Skip to content

Commit 847cda0

Browse files
yenfryherrerafelizyenfryherrerafeliz
committed
chore: add unit tests
- Add test coverage for IfMatch used in every part or range get request in a multipart download operation. - Fixes the case of the algorithm. It was moved from capitalized case to upper case. - Adds validation after a multipart download operation to make sure the number of parts match the expected one.
1 parent e7842ed commit 847cda0

9 files changed

+2307
-10
lines changed

‎src/S3/S3Transfer/AbstractMultipartUploader.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -228,7 +228,7 @@ protected function completeMultipartOperation(): PromiseInterface
228228
if ($this->requestChecksum !== null) {
229229
$completeMultipartUploadArgs['ChecksumType'] = self::CHECKSUM_TYPE_FULL_OBJECT;
230230
$completeMultipartUploadArgs[
231-
'Checksum' . ucfirst($this->requestChecksumAlgorithm)
231+
'Checksum' . strtoupper($this->requestChecksumAlgorithm)
232232
] = $this->requestChecksum;
233233
}
234234

‎src/S3/S3Transfer/MultipartDownloader.php‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ abstract class MultipartDownloader implements PromisorInterface
6565
*/
6666
public function __construct(
6767
protected readonly S3ClientInterface $s3Client,
68-
array $downloadRequestArgs = [],
68+
array $downloadRequestArgs,
6969
array $config = [],
7070
?DownloadHandler $downloadHandler = null,
7171
int $currentPartNo = 0,
@@ -188,6 +188,12 @@ public function promise(): PromiseInterface
188188
});
189189
}
190190

191+
if ($this->currentPartNo !== $this->objectPartsCount) {
192+
throw new S3TransferException(
193+
"Expected number of parts `$this->objectPartsCount` to have been transferred but got `$this->currentPartNo`."
194+
);
195+
}
196+
191197
// Transfer completed
192198
$this->downloadComplete();
193199

@@ -222,7 +228,7 @@ protected function initialRequest(): PromiseInterface
222228
// Compute object dimensions such as parts count and object size
223229
$this->computeObjectDimensions($result);
224230

225-
// If a multipart is likely to happen then save the ETag
231+
// If there are more than one part then save the ETag
226232
if ($this->objectPartsCount > 1) {
227233
$this->eTag = $result['ETag'];
228234
}

‎src/S3/S3Transfer/MultipartUploader.php‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,9 @@ private function evaluateCustomChecksum(): void
119119
'',
120120
$checksumName
121121
);
122+
$this->requestChecksumAlgorithm = strtolower(
123+
$this->requestChecksumAlgorithm
124+
);
122125
} else {
123126
$this->requestChecksum = null;
124127
$this->requestChecksumAlgorithm = null;
@@ -139,7 +142,7 @@ protected function processMultipartOperation(): PromiseInterface
139142
if ($this->requestChecksum !== null) {
140143
// To avoid default calculation
141144
$uploadPartCommandArgs['@context']['request_checksum_calculation'] = 'when_required';
142-
unset($uploadPartCommandArgs['Checksum'. ucfirst($this->requestChecksumAlgorithm)]);
145+
unset($uploadPartCommandArgs['Checksum'. strtoupper($this->requestChecksumAlgorithm)]);
143146
} elseif ($this->requestChecksumAlgorithm !== null) {
144147
// Normalize algorithm name
145148
$algoName = strtolower($this->requestChecksumAlgorithm);
@@ -151,7 +154,7 @@ protected function processMultipartOperation(): PromiseInterface
151154
$this->hashContext = hash_init($algoName);
152155
// To avoid default calculation
153156
$uploadPartCommandArgs['@context']['request_checksum_calculation'] = 'when_required';
154-
unset($uploadPartCommandArgs['Checksum'. ucfirst($this->requestChecksumAlgorithm)]);
157+
unset($uploadPartCommandArgs['Checksum'. strtoupper($this->requestChecksumAlgorithm)]);
155158
}
156159

157160
while (!$this->body->eof()) {

‎tests/S3/S3Transfer/MultipartUploaderTest.php‎

Lines changed: 417 additions & 2 deletions
Large diffs are not rendered by default.

‎tests/S3/S3Transfer/PartGetMultipartDownloaderTest.php‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
use Aws\S3\S3Transfer\Models\DownloadResult;
99
use Aws\S3\S3Transfer\PartGetMultipartDownloader;
1010
use Aws\S3\S3Transfer\Utils\StreamDownloadHandler;
11+
use Generator;
1112
use GuzzleHttp\Promise\Create;
1213
use GuzzleHttp\Psr7\Utils;
1314
use PHPUnit\Framework\TestCase;
@@ -200,4 +201,111 @@ public function testComputeObjectDimensions(): void
200201
$this->assertEquals(5, $downloader->getObjectPartsCount());
201202
$this->assertEquals(2048, $downloader->getObjectSizeInBytes());
202203
}
204+
205+
/**
206+
* Test IfMatch is properly called in each part get operation.
207+
*
208+
* @param int $objectSizeInBytes
209+
* @param int $targetPartSize
210+
* @param string $eTag
211+
*
212+
* @dataProvider ifMatchIsPresentInEachPartRequestAfterFirstProvider
213+
*
214+
* @return void
215+
*/
216+
public function testIfMatchIsPresentInEachRangeRequestAfterFirst(
217+
int $objectSizeInBytes,
218+
int $targetPartSize,
219+
string $eTag
220+
): void
221+
{
222+
$firstRequestCalled = false;
223+
$ifMatchCalledTimes = 0;
224+
$partsCount = ceil($objectSizeInBytes / $targetPartSize);
225+
$remainingToTransfer = $objectSizeInBytes;
226+
$s3Client = $this->getMockBuilder(S3Client::class)
227+
->disableOriginalConstructor()
228+
->getMock();
229+
$s3Client->method('getCommand')
230+
->willReturnCallback(function ($commandName, $args)
231+
use ($eTag, &$ifMatchCalledTimes) {
232+
if (isset($args['IfMatch'])) {
233+
$ifMatchCalledTimes++;
234+
$this->assertEquals(
235+
$eTag,
236+
$args['IfMatch']
237+
);
238+
}
239+
240+
return new Command($commandName, $args);
241+
});
242+
$s3Client->method('executeAsync')
243+
-> willReturnCallback(function ($command)
244+
use (
245+
$eTag,
246+
$objectSizeInBytes,
247+
$partsCount,
248+
$targetPartSize,
249+
&$remainingToTransfer,
250+
&$firstRequestCalled
251+
) {
252+
$firstRequestCalled = true;
253+
$currentPartLength = min(
254+
$targetPartSize,
255+
$remainingToTransfer
256+
);
257+
$from = $objectSizeInBytes - $remainingToTransfer;
258+
$to = $from + $currentPartLength;
259+
$remainingToTransfer -= $currentPartLength;
260+
return Create::promiseFor(new Result([
261+
'Body' => Utils::streamFor('Foo'),
262+
'PartsCount' => $partsCount,
263+
'PartNumber' => $command['PartNumber'],
264+
'ContentRange' => "bytes $from-$to/$objectSizeInBytes",
265+
'ContentLength' => $currentPartLength,
266+
'ETag' => $eTag,
267+
]));
268+
});
269+
$requestArgs = [
270+
'Bucket' => 'TestBucket',
271+
'Key' => 'TestKey',
272+
];
273+
$partGetMultipartDownloader = new PartGetMultipartDownloader(
274+
$s3Client,
275+
$requestArgs,
276+
[
277+
'target_part_size_bytes' => $targetPartSize,
278+
]
279+
);
280+
$partGetMultipartDownloader->download();
281+
$this->assertTrue($firstRequestCalled);
282+
$this->assertEquals(
283+
$partsCount - 1,
284+
$ifMatchCalledTimes
285+
);
286+
}
287+
288+
/**
289+
* @return Generator
290+
*/
291+
public function ifMatchIsPresentInEachPartRequestAfterFirstProvider(): Generator
292+
{
293+
yield 'multipart_download_with_3_parts_1' => [
294+
'object_size_in_bytes' => 1024 * 1024 * 20,
295+
'target_part_size_bytes' => 8 * 1024 * 1024,
296+
'eTag' => 'ETag1234',
297+
];
298+
299+
yield 'multipart_download_with_2_parts_1' => [
300+
'object_size_in_bytes' => 1024 * 1024 * 16,
301+
'target_part_size_bytes' => 8 * 1024 * 1024,
302+
'eTag' => 'ETag12345678',
303+
];
304+
305+
yield 'multipart_download_with_5_parts_1' => [
306+
'object_size_in_bytes' => 1024 * 1024 * 40,
307+
'target_part_size_bytes' => 8 * 1024 * 1024,
308+
'eTag' => 'ETag12345678',
309+
];
310+
}
203311
}

‎tests/S3/S3Transfer/RangeGetMultipartDownloaderTest.php‎

Lines changed: 107 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
use Aws\S3\S3Transfer\Models\DownloadResult;
1010
use Aws\S3\S3Transfer\RangeGetMultipartDownloader;
1111
use Aws\S3\S3Transfer\Utils\StreamDownloadHandler;
12+
use Generator;
1213
use GuzzleHttp\Promise\Create;
1314
use GuzzleHttp\Psr7\Utils;
1415
use PHPUnit\Framework\TestCase;
@@ -157,7 +158,6 @@ public function testNextCommandGeneratesCorrectRangeHeaders(): void
157158
// Use reflection to test the protected nextCommand method
158159
$reflection = new \ReflectionClass($downloader);
159160
$nextCommandMethod = $reflection->getMethod('nextCommand');
160-
$nextCommandMethod->setAccessible(true);
161161

162162
// First call should create range 0-1023
163163
$command1 = $nextCommandMethod->invoke($downloader);
@@ -197,7 +197,6 @@ public function testComputeObjectDimensionsForSinglePart(): void
197197
// Use reflection to test the protected computeObjectDimensions method
198198
$reflection = new \ReflectionClass($downloader);
199199
$computeObjectDimensionsMethod = $reflection->getMethod('computeObjectDimensions');
200-
$computeObjectDimensionsMethod->setAccessible(true);
201200

202201
// Simulate object smaller than part size
203202
$result = new Result([
@@ -215,6 +214,7 @@ public function testComputeObjectDimensionsForSinglePart(): void
215214
* Tests nextCommand method includes IfMatch header when ETag is present.
216215
*
217216
* @return void
217+
* @throws \ReflectionException
218218
*/
219219
public function testNextCommandIncludesIfMatchWhenETagPresent(): void
220220
{
@@ -247,9 +247,113 @@ public function testNextCommandIncludesIfMatchWhenETagPresent(): void
247247
// Use reflection to test the protected nextCommand method
248248
$reflection = new \ReflectionClass($downloader);
249249
$nextCommandMethod = $reflection->getMethod('nextCommand');
250-
$nextCommandMethod->setAccessible(true);
251250

252251
$command = $nextCommandMethod->invoke($downloader);
253252
$this->assertEquals($eTag, $command['IfMatch']);
254253
}
254+
255+
/**
256+
* Test IfMatch is properly called in each part get operation.
257+
*
258+
* @param int $objectSizeInBytes
259+
* @param int $targetPartSize
260+
* @param string $eTag
261+
*
262+
* @dataProvider ifMatchIsPresentInEachRangeRequestAfterFirstProvider
263+
*
264+
* @return void
265+
*/
266+
public function testIfMatchIsPresentInEachRangeRequestAfterFirst(
267+
int $objectSizeInBytes,
268+
int $targetPartSize,
269+
string $eTag
270+
): void
271+
{
272+
$firstRequestCalled = false;
273+
$ifMatchCalledTimes = 0;
274+
$partsCount = ceil($objectSizeInBytes / $targetPartSize);
275+
$remainingToTransfer = $objectSizeInBytes;
276+
$s3Client = $this->getMockBuilder(S3Client::class)
277+
->disableOriginalConstructor()
278+
->getMock();
279+
$s3Client->method('getCommand')
280+
->willReturnCallback(function ($commandName, $args)
281+
use ($eTag, &$ifMatchCalledTimes) {
282+
if (isset($args['IfMatch'])) {
283+
$ifMatchCalledTimes++;
284+
$this->assertEquals(
285+
$eTag,
286+
$args['IfMatch']
287+
);
288+
}
289+
290+
return new Command($commandName, $args);
291+
});
292+
$s3Client->method('executeAsync')
293+
-> willReturnCallback(function ($command)
294+
use (
295+
$eTag,
296+
$objectSizeInBytes,
297+
$partsCount,
298+
$targetPartSize,
299+
&$remainingToTransfer,
300+
&$firstRequestCalled
301+
) {
302+
$firstRequestCalled = true;
303+
$currentPartLength = min(
304+
$targetPartSize,
305+
$remainingToTransfer
306+
);
307+
$from = $objectSizeInBytes - $remainingToTransfer;
308+
$to = $from + $currentPartLength;
309+
$remainingToTransfer -= $currentPartLength;
310+
return Create::promiseFor(new Result([
311+
'Body' => Utils::streamFor('Foo'),
312+
'ContentRange' => "bytes $from-$to/$objectSizeInBytes",
313+
'ContentLength' => $currentPartLength,
314+
'ETag' => $eTag,
315+
]));
316+
});
317+
$requestArgs = [
318+
'Bucket' => 'TestBucket',
319+
'Key' => 'TestKey',
320+
];
321+
$rangeGetMultipartDownloader = new RangeGetMultipartDownloader(
322+
$s3Client,
323+
$requestArgs,
324+
[
325+
'target_part_size_bytes' => $targetPartSize,
326+
]
327+
);
328+
$rangeGetMultipartDownloader->download();
329+
$this->assertTrue($firstRequestCalled);
330+
$this->assertEquals(
331+
$partsCount - 1,
332+
$ifMatchCalledTimes
333+
);
334+
}
335+
336+
/**
337+
* @return Generator
338+
*/
339+
public function ifMatchIsPresentInEachRangeRequestAfterFirstProvider(): Generator
340+
{
341+
yield 'multipart_download_with_3_parts_1' => [
342+
'object_size_in_bytes' => 1024 * 1024 * 20,
343+
'target_part_size_bytes' => 8 * 1024 * 1024,
344+
'eTag' => 'ETag1234',
345+
];
346+
347+
yield 'multipart_download_with_2_parts_1' => [
348+
'object_size_in_bytes' => 1024 * 1024 * 16,
349+
'target_part_size_bytes' => 8 * 1024 * 1024,
350+
'eTag' => 'ETag12345678',
351+
];
352+
353+
yield 'multipart_download_with_5_parts_1' => [
354+
'object_size_in_bytes' => 1024 * 1024 * 40,
355+
'target_part_size_bytes' => 8 * 1024 * 1024,
356+
'eTag' => 'ETag12345678',
357+
];
358+
}
255359
}

0 commit comments

Comments
 (0)