Add additional validation to UploadPartRequests in Transfer Utility #4083
Conversation
sdk/src/Services/S3/Custom/Transfer/Internal/MultipartUploadCommand.cs
Outdated
Show resolved
Hide resolved
|
|
||
| filePosition += partSize; | ||
| } | ||
|
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| return await _s3Client.UploadPartAsync(uploadRequest, internalCts.Token) | ||
| var response = await _s3Client.UploadPartAsync(uploadRequest, internalCts.Token) | ||
| .ConfigureAwait(continueOnCapturedContext: false); | ||
|
|
There was a problem hiding this comment.
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
i dont think it matters to me where we do it too much. what do you think?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| && _s3Client is Amazon.S3.Internal.IAmazonS3Encryption) | ||
| { | ||
| uploadPartRequest.IsLastPart = true; | ||
| uploadPartRequest.PartSize = 0; |
There was a problem hiding this comment.
is this going to fail validation if we are using amazonencryption client? do we need to skip it in that case?
There was a problem hiding this comment.
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.
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
|
|
||
| namespace AWSSDK.UnitTests.S3.NetFramework.Custom |
There was a problem hiding this comment.
this namespace is not right. should be AWSSDK.UnitTests
There was a problem hiding this comment.
yeah ^^ also, that means the dry run might not have run these tests?
There was a problem hiding this comment.
Will rename and rerun the dry run
sdk/src/Services/S3/Custom/Transfer/Internal/_async/MultipartUploadCommand.async.cs
Show resolved
Hide resolved
sdk/src/Services/S3/Custom/Transfer/Internal/_async/MultipartUploadCommand.async.cs
Outdated
Show resolved
Hide resolved
sdk/src/Services/S3/Custom/Transfer/Internal/_async/MultipartUploadCommand.async.cs
Show resolved
Hide resolved
New dry run passed |
3 participants
DOTNET-8274
Description
Add additional validation to UploadPartRequests in Transfer Utility
Motivation and Context
Add additional validation to UploadPartRequests in Transfer Utility
Testing
Ran tests locally
Ran successful dry run
Screenshots (if appropriate)
Types of changes
Checklist
License