Skip to content

Conversation

normj
Copy link
Member

@normj normj commented Oct 14, 2025

PR in draft mode till full dry run is successul

Description

The PutObject operation does work with an unseekable stream if you set both DisablePayloadSigning is set to true but the UploadPart which also fail. That is because it is always wrapping the input stream in a PartialWrapperStream which requires the stream to be seekable. The PR switches the code to use the GetStreamWithLength method like PutObject which returns back a PartialReadOnlyWrapperStream instance which does not require the wrapped stream to be seekable.

The content-length will be determine by either the stream's length if available and then fallback to PartSize property of the UploadPartRequest`.

Motivation and Context

#4010

Testing

Added new integ tests
Dry run: pending

@normj normj marked this pull request as draft October 14, 2025 21:39
@GarrettBeatty GarrettBeatty requested a review from Copilot October 14, 2025 23:28
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Fixes an issue where the UploadPart operation fails when using an unseekable stream with DisablePayloadSigning set to true. The fix replaces the use of PartialWrapperStream (which requires seekable streams) with the GetStreamWithLength method that returns a PartialReadOnlyWrapperStream compatible with unseekable streams.

  • Updates stream handling in UploadPart to use GetStreamWithLength method for unseekable stream compatibility
  • Adds comprehensive integration tests for both PutObject and UploadPart operations with unseekable streams
  • Includes proper dev config file with patch version bump

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
sdk/test/Services/S3/IntegrationTests/PutUnseekableStreamTests.cs New integration test file with tests for PutObject and UploadPart using unseekable streams
sdk/src/Services/S3/Custom/Internal/AmazonS3PostMarshallHandler.cs Updates UploadPart stream handling to use GetStreamWithLength method instead of PartialWrapperStream
generator/.DevConfigs/32a12d7c-afc6-4bcf-a2d8-9c49b335b935.json Dev config file with patch version bump and changelog entry

Comment on lines +20 to +23
StreamWriter writer = File.CreateText("PutObjectFile.txt");
writer.Write("This is some sample text.!!");
writer.Close();

Copy link

Copilot AI Oct 14, 2025

Choose a reason for hiding this comment

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

The created file 'PutObjectFile.txt' is never used in the test methods and should be removed to avoid creating unnecessary files during test execution.

Suggested change
StreamWriter writer = File.CreateText("PutObjectFile.txt");
writer.Write("This is some sample text.!!");
writer.Close();

Copilot uses AI. Check for mistakes.

Comment on lines +69 to +115

var initiateMultipartUploadRequest = new InitiateMultipartUploadRequest
{
BucketName = bucketName,
Key = "upload-part-unseekable-test.txt"
};

var initiateMultipartUploadResponse = await Client.InitiateMultipartUploadAsync(initiateMultipartUploadRequest);

var uploadPartRequest = new UploadPartRequest
{
BucketName = bucketName,
Key = "upload-part-unseekable-test.txt",
UploadId = initiateMultipartUploadResponse.UploadId,
PartNumber = 1,
PartSize = stream.Length,
InputStream = stream,
DisablePayloadSigning = true,
IsLastPart = true,
};


var uploadPartResponse = await Client.UploadPartAsync(uploadPartRequest);

var completeMultipartUploadRequest = new CompleteMultipartUploadRequest
{
BucketName = bucketName,
Key = "upload-part-unseekable-test.txt",
UploadId = initiateMultipartUploadResponse.UploadId
};

completeMultipartUploadRequest.AddPartETags(uploadPartResponse);

await Client.CompleteMultipartUploadAsync(completeMultipartUploadRequest);

var getRequest = new GetObjectRequest
{
BucketName = bucketName,
Key = "upload-part-unseekable-test.txt"
};
using (var getResponse = await Client.GetObjectAsync(getRequest))
{
using (var reader = new StreamReader(getResponse.ResponseStream))
{
var content = reader.ReadToEnd();
Assert.AreEqual("Hello, S3!", content);
}
Copy link

Copilot AI Oct 14, 2025

Choose a reason for hiding this comment

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

Add error handling for the multipart upload scenario. If the UploadPart or CompleteMultipartUpload operations fail, the multipart upload should be aborted to avoid leaving incomplete uploads in S3.

Suggested change
var initiateMultipartUploadRequest = new InitiateMultipartUploadRequest
{
BucketName = bucketName,
Key = "upload-part-unseekable-test.txt"
};
var initiateMultipartUploadResponse = await Client.InitiateMultipartUploadAsync(initiateMultipartUploadRequest);
var uploadPartRequest = new UploadPartRequest
{
BucketName = bucketName,
Key = "upload-part-unseekable-test.txt",
UploadId = initiateMultipartUploadResponse.UploadId,
PartNumber = 1,
PartSize = stream.Length,
InputStream = stream,
DisablePayloadSigning = true,
IsLastPart = true,
};
var uploadPartResponse = await Client.UploadPartAsync(uploadPartRequest);
var completeMultipartUploadRequest = new CompleteMultipartUploadRequest
{
BucketName = bucketName,
Key = "upload-part-unseekable-test.txt",
UploadId = initiateMultipartUploadResponse.UploadId
};
completeMultipartUploadRequest.AddPartETags(uploadPartResponse);
await Client.CompleteMultipartUploadAsync(completeMultipartUploadRequest);
var getRequest = new GetObjectRequest
{
BucketName = bucketName,
Key = "upload-part-unseekable-test.txt"
};
using (var getResponse = await Client.GetObjectAsync(getRequest))
{
using (var reader = new StreamReader(getResponse.ResponseStream))
{
var content = reader.ReadToEnd();
Assert.AreEqual("Hello, S3!", content);
}
string uploadId = null;
try
{
var initiateMultipartUploadRequest = new InitiateMultipartUploadRequest
{
BucketName = bucketName,
Key = "upload-part-unseekable-test.txt"
};
var initiateMultipartUploadResponse = await Client.InitiateMultipartUploadAsync(initiateMultipartUploadRequest);
uploadId = initiateMultipartUploadResponse.UploadId;
var uploadPartRequest = new UploadPartRequest
{
BucketName = bucketName,
Key = "upload-part-unseekable-test.txt",
UploadId = uploadId,
PartNumber = 1,
PartSize = stream.Length,
InputStream = stream,
DisablePayloadSigning = true,
IsLastPart = true,
};
var uploadPartResponse = await Client.UploadPartAsync(uploadPartRequest);
var completeMultipartUploadRequest = new CompleteMultipartUploadRequest
{
BucketName = bucketName,
Key = "upload-part-unseekable-test.txt",
UploadId = uploadId
};
completeMultipartUploadRequest.AddPartETags(uploadPartResponse);
await Client.CompleteMultipartUploadAsync(completeMultipartUploadRequest);
var getRequest = new GetObjectRequest
{
BucketName = bucketName,
Key = "upload-part-unseekable-test.txt"
};
using (var getResponse = await Client.GetObjectAsync(getRequest))
{
using (var reader = new StreamReader(getResponse.ResponseStream))
{
var content = reader.ReadToEnd();
Assert.AreEqual("Hello, S3!", content);
}
}
}
catch
{
if (uploadId != null)
{
await Client.AbortMultipartUploadAsync(new AbortMultipartUploadRequest
{
BucketName = bucketName,
Key = "upload-part-unseekable-test.txt",
UploadId = uploadId
});
}
throw;

Copilot uses AI. Check for mistakes.

@normj normj force-pushed the normj/fix-unseekable branch from fb6fe9d to 5bd7587 Compare October 15, 2025 02:04
@normj
Copy link
Member Author

normj commented Oct 15, 2025

The PR is turning into a bit of a whack a mole experience. We'll keep plugging away but there are currently some tests failing making this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant