Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions generator/.DevConfigs/49ef8a70-bb30-4cc4-a8b5-92de4f6068c1.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"services": [
{
"serviceName": "S3",
"type": "patch",
"changeLogMessages": [
"Fixed issue where PartSize and IsLastPart fields were not properly set on Transfer Utility Upload Part Request.",
"Add additional validations for Transfer Utility requests to ensure Upload Parts have the proper Content Length and File Offsets."
]
}
]
}
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ internal partial class MultipartUploadCommand : BaseCommand
long _totalTransferredBytes;
Queue<UploadPartRequest> _partsToUpload = new Queue<UploadPartRequest>();


long _contentLength;
private static Logger Logger
{
Expand Down Expand Up @@ -211,17 +210,30 @@ internal CompleteMultipartUploadRequest ConstructCompleteMultipartUploadRequest(
return compRequest;
}

private bool calculateIsLastPart(long remainingBytes)
{
var isLastPart = false;
if (remainingBytes <= this._partSize)
isLastPart = true;
return isLastPart;
}

internal UploadPartRequest ConstructUploadPartRequest(int partNumber, long filePosition, InitiateMultipartUploadResponse initiateResponse)
{
UploadPartRequest uploadPartRequest = ConstructGenericUploadPartRequest(initiateResponse);

// Calculating how many bytes are remaining to be uploaded from the current part.
// This is mainly used for the last part scenario.
var remainingBytes = this._contentLength - filePosition;
// We then check based on the remaining bytes and the content length if this is the last part.
var isLastPart = calculateIsLastPart(remainingBytes);
uploadPartRequest.PartNumber = partNumber;
uploadPartRequest.PartSize = this._partSize;
uploadPartRequest.PartSize = isLastPart ? remainingBytes : this._partSize;
uploadPartRequest.IsLastPart = isLastPart;

if ((filePosition + this._partSize >= this._contentLength)
if (isLastPart
&& _s3Client is Amazon.S3.Internal.IAmazonS3Encryption)
{
uploadPartRequest.IsLastPart = true;
uploadPartRequest.PartSize = 0;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ internal partial class MultipartUploadCommand : BaseCommand
{
public SemaphoreSlim AsyncThrottler { get; set; }

Dictionary<int, ExpectedUploadPart> _expectedUploadParts = new Dictionary<int, ExpectedUploadPart>();

public override async Task ExecuteAsync(CancellationToken cancellationToken)
{
// Fire transfer initiated event FIRST, before choosing path
Expand Down Expand Up @@ -69,6 +71,29 @@ public override async Task ExecuteAsync(CancellationToken cancellationToken)
cancellationToken.ThrowIfCancellationRequested();

var uploadRequest = ConstructUploadPartRequest(i, filePosition, initResponse);

var expectedFileOffset = (i - 1) * this._partSize;
// Calculating how many bytes are remaining to be uploaded from the current part.
// This is mainly used for the last part scenario.
var remainingBytes = this._contentLength - expectedFileOffset;
// We then check based on the remaining bytes and the content length if this is the last part.
var isLastPart = calculateIsLastPart(remainingBytes);
// To maintain the same behavior as the ConstructUploadPartRequest.
// We are setting the remainingBytes/partSize when using the IAmazonS3Encryption client to 0.
if (isLastPart
&& _s3Client is Amazon.S3.Internal.IAmazonS3Encryption)
{
remainingBytes = 0;
}
this._expectedUploadParts.Add(i, new ExpectedUploadPart {
PartNumber = i,
ExpectedContentLength =
isLastPart ?
remainingBytes :
this._partSize,
ExpectedFileOffset = expectedFileOffset,
IsLastPart = isLastPart
});
this._partsToUpload.Enqueue(uploadRequest);
filePosition += this._partSize;
}
Expand Down Expand Up @@ -150,8 +175,50 @@ private async Task<UploadPartResponse> UploadPartAsync(UploadPartRequest uploadR
{
try
{
return await _s3Client.UploadPartAsync(uploadRequest, internalCts.Token)
var response = await _s3Client.UploadPartAsync(uploadRequest, internalCts.Token)
.ConfigureAwait(continueOnCapturedContext: false);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think all of the validation looks good. just a general question. it looks like we do the other validation for total number of parts = expected here

if (this._uploadResponses.Count != this._totalNumberOfParts)
(right before we do complete multi part upload).

i dont think it matters to me where we do it too much. what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also will having the logic here fail/work for unseekable streams? i think the reason the other code is in the other place is because for unseekable streams they skip validation. wondering if this code path gets executed for unseekable stream or we need to skip it for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The validation if (this._uploadResponses.Count != this._totalNumberOfParts) is being skipped for unseekable streams. The new validation logic we added will and should be skipped. The code path of the UploadPart function where i added the new validation is different from the unseekable stream code path.

if (response.PartNumber is null)
{
throw new ArgumentNullException(nameof(response.PartNumber));
}
else
{
if (this._expectedUploadParts.TryGetValue((int) response.PartNumber, out var expectedUploadPart))
{
var actualContentLength = uploadRequest.PartSize;
if (actualContentLength != expectedUploadPart.ExpectedContentLength)
{
throw new InvalidOperationException($"Cannot complete multipart upload request. The expected content length of part {expectedUploadPart.PartNumber} " +
$"does not equal the actual content length.");
}

if (expectedUploadPart.IsLastPart)
{
if (actualContentLength < 0 ||
actualContentLength > expectedUploadPart.ExpectedContentLength)
{
throw new InvalidOperationException($"Cannot complete multipart upload request. The last part " +
$"has an invalid content length.");
}
}

var actualFileOsset = uploadRequest.FilePosition;
if (uploadRequest.IsSetFilePath() &&
actualFileOsset != expectedUploadPart.ExpectedFileOffset)
{
throw new InvalidOperationException($"Cannot complete multipart upload request. The expected file offset of part {expectedUploadPart.PartNumber} " +
$"does not equal the actual file offset.");
}
}
else
{
throw new InvalidOperationException("Multipart upload request part was unexpected.");
}
}


return response;
}
catch (Exception exception)
{
Expand Down Expand Up @@ -326,5 +393,13 @@ await _s3Client.AbortMultipartUploadAsync(new AbortMultipartUploadRequest()
throw;
}
}

private class ExpectedUploadPart
{
public int PartNumber { get; set; }
public long? ExpectedContentLength { get; set; }
public long? ExpectedFileOffset { get; set; }
public bool IsLastPart { get; set; }
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
using Amazon.S3;
using Amazon.S3.Model;
using Amazon.S3.Transfer;
using Amazon.S3.Transfer.Internal;
using Amazon.S3.Util;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using Moq;
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Text;
using System.Threading;
using System.Threading.Tasks;

namespace AWSSDK.UnitTests
{
[TestClass]
public class MultipartUploadValidationTests
{
private static string _tempFilePath;
private const long fileSizeInBytes = 40 * 1024 * 1024;

[ClassInitialize]
public static void ClassInitialize(TestContext context)
{
_tempFilePath = Path.GetTempFileName();

CreateFileWithSpecificSize(_tempFilePath, fileSizeInBytes);
}

[ClassCleanup]
public static void ClassCleanup()
{
if (File.Exists(_tempFilePath))
{
File.Delete(_tempFilePath);
}
}

private static void CreateFileWithSpecificSize(string path, long size)
{
using (var fileStream = new FileStream(path, FileMode.Create, FileAccess.Write))
{
fileStream.SetLength(size);
}
}

[TestMethod]
[TestCategory("S3")]
public async Task Validation_HappyPath()
{
var initiateMultipartUploadResponse = new InitiateMultipartUploadResponse
{
UploadId = "test"
};

var s3Client = new Mock<AmazonS3Client>();
s3Client
.Setup(x => x.InitiateMultipartUploadAsync(
It.IsAny<InitiateMultipartUploadRequest>(),
It.IsAny<CancellationToken>()))
.ReturnsAsync(initiateMultipartUploadResponse);

s3Client
.Setup(x => x.UploadPartAsync(It.IsAny<UploadPartRequest>(), It.IsAny<CancellationToken>()))
.ReturnsAsync((UploadPartRequest request, CancellationToken cancellationToken) =>
{
return new UploadPartResponse { PartNumber = request.PartNumber };
});

var uploadRequest = new TransferUtilityUploadRequest
{
FilePath = _tempFilePath,
BucketName = "test-bucket",
Key = "test"
};
var multipartUpload = new MultipartUploadCommand(s3Client.Object, new TransferUtilityConfig(), uploadRequest);
await multipartUpload.ExecuteAsync(new CancellationToken());
}

[TestMethod]
[TestCategory("S3")]
public void Validation_ConstructUploadPartRequest()
{
var initiateMultipartUploadResponse = new InitiateMultipartUploadResponse
{
UploadId = "test"
};

var s3Client = new Mock<AmazonS3Client>();

s3Client
.Setup(x => x.InitiateMultipartUploadAsync(
It.IsAny<InitiateMultipartUploadRequest>(),
It.IsAny<CancellationToken>()))
.ReturnsAsync(initiateMultipartUploadResponse);

var uploadRequest = new TransferUtilityUploadRequest
{
FilePath = _tempFilePath,
BucketName = "test-bucket",
Key = "test"
};

var multipartUpload = new MultipartUploadCommand(s3Client.Object, new TransferUtilityConfig(), uploadRequest);

var partSize = Math.Max(S3Constants.DefaultPartSize, uploadRequest.ContentLength / S3Constants.MaxNumberOfParts);

long filePosition = 0;
for (int i = 1; filePosition < uploadRequest.ContentLength; i++)
{
var constructUploadPartRequest = multipartUpload.ConstructUploadPartRequest(i, filePosition, initiateMultipartUploadResponse);

var expectedFileOffset = (i - 1) * partSize;
var remainingBytes = uploadRequest.ContentLength - expectedFileOffset;
var isLastPart = false;
if (remainingBytes <= partSize)
isLastPart = true;

Assert.AreEqual(i, constructUploadPartRequest.PartNumber);
Assert.AreEqual(isLastPart, constructUploadPartRequest.IsLastPart);
Assert.AreEqual(
isLastPart ? remainingBytes : partSize,
constructUploadPartRequest.PartSize);
Assert.AreEqual(expectedFileOffset, constructUploadPartRequest.FilePosition);

filePosition += partSize;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any easy way to right a unit test/integration tests which would reach the code path that throws exception? somehow mock the ConstructUploadPartRequest call to put in incorrect values and then when the response is sent and validation is reached we assert validation error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not without restructuring a lot of existing code. Most of the stuff is created within the functions and not really mockable. Let me know if you have thoughts on how to do it.

}
}
}