Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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,27 @@ 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);

var remainingBytes = this._contentLength - filePosition;
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;
Copy link
Contributor

Choose a reason for hiding this comment

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

is this going to fail validation if we are using amazonencryption client? do we need to skip it in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know why the Part Size gets set to 0, but i updated our validation to match the same behavior so that we don't cause unexpected issues.

}

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,24 @@ public override async Task ExecuteAsync(CancellationToken cancellationToken)
cancellationToken.ThrowIfCancellationRequested();

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

var expectedFileOffset = (i - 1) * this._partSize;
var remainingBytes = this._contentLength - expectedFileOffset;
var isLastPart = calculateIsLastPart(remainingBytes);
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 +170,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 +388,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.S3.NetFramework.Custom
Copy link
Contributor

Choose a reason for hiding this comment

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

this namespace is not right. should be AWSSDK.UnitTests

Copy link
Contributor

@peterrsongg peterrsongg Oct 28, 2025

Choose a reason for hiding this comment

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

yeah ^^ also, that means the dry run might not have run these tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will rename and rerun the dry run

{
[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.

}
}
}