Skip to content

Fix DeleteObjects to throw DeleteObjectsException instead of AmazonS3Exception #3839

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: aws-sdk-net-v3.7-development
Choose a base branch
from
Open
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
11 changes: 11 additions & 0 deletions generator/.DevConfigs/1f197260-2f36-43db-8312-41be9543531c.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"services": [
{
"serviceName": "S3",
"type": "patch",
"changeLogMessages": [
"Fixed DeleteObjects to throw DeleteObjectsException instead of AmazonS3Exception for consistent error handling"
]
}
]
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@
* permissions and limitations under the License.
*/
using System;
using System.IO;
using System.Net;
using System.Xml;
using System.Collections.Generic;
using Amazon.S3.Model;
using Amazon.S3.Util;
Expand Down Expand Up @@ -96,6 +98,77 @@ private static void UnmarshallResult(XmlUnmarshallerContext context,DeleteObject
return;
}

/// <summary>
/// Unmarshaller error response to exception.
/// For DeleteObjects operations, we always return DeleteObjectsException instead of AmazonS3Exception
/// to maintain consistency in exception types regardless of the specific error scenario.
/// </summary>
/// <param name="context"></param>
/// <param name="innerException"></param>
/// <param name="statusCode"></param>
/// <returns></returns>
public override AmazonServiceException UnmarshallException(XmlUnmarshallerContext context, Exception innerException, HttpStatusCode statusCode)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add an integration test that would trigger this code path and asserts that a DeleteObjectsException is thrown?

{
var responseBodyBytes = context.GetResponseBodyBytes();

// First, try to parse as a standard DeleteObjects response (for cases where S3 returns mixed results)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you explain what you mean by "mixed results"?

if (responseBodyBytes != null && responseBodyBytes.Length > 0)
{
using (var streamCopy = new MemoryStream(responseBodyBytes))
using (var contextCopy = new XmlUnmarshallerContext(streamCopy, false, null))
{
try
{
var deleteResponse = this.Unmarshall(contextCopy) as DeleteObjectsResponse;

// If we successfully parsed a DeleteObjects response, use it directly
if (deleteResponse != null)
{
return new DeleteObjectsException(deleteResponse);
}
}
catch (XmlException)
{
// Response is not a valid DeleteObjects XML, fall through to error handling
}
catch (InvalidOperationException)
{
// Response is not a valid DeleteObjects XML, fall through to error handling
}
}
}

// For standard S3 error responses (like KeyTooLongError), create a DeleteObjectsException
// with the error information wrapped in a DeleteObjectsResponse
var errorResponse = S3ErrorResponseUnmarshaller.Instance.Unmarshall(context);
Copy link

@grsw92 grsw92 May 23, 2025

Choose a reason for hiding this comment

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

@AlexDaines does this solution guarantee that object under valid key gets deleted when sent along with invalid key in a single DeleteObjectsRequest?


// Create a DeleteObjectsResponse containing the error information
var deleteObjectsResponse = new DeleteObjectsResponse();
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are now implementing unmarshallException properly can we remove this logic here as part of this PR?

https://github.com/aws/aws-sdk-net/blob/main/sdk/src/Services/S3/Custom/Internal/AmazonS3ResponseHandler.cs#L100-L107

It seems useless since UnmarshallException will be called first and the sdk will throw an exception.

deleteObjectsResponse.DeleteErrors = new List<DeleteError>
{
new DeleteError
{
Code = errorResponse?.Code ?? "UnknownError",
Message = errorResponse?.Message ?? "An error occurred during the delete operation",
Key = "" // We don't know which specific key caused the error in batch validation scenarios
}
};

// Create the DeleteObjectsException with our constructed response
var deleteException = new DeleteObjectsException(deleteObjectsResponse);

// Set additional properties from the error response
if (errorResponse != null)
{
deleteException.ErrorCode = errorResponse.Code;
deleteException.RequestId = errorResponse.RequestId;
deleteException.StatusCode = statusCode;
// Note: ResponseBody will be set by the base exception handling if available
}

return deleteException;
}

private static DeleteObjectsResponseUnmarshaller _instance;

/// <summary>
Expand Down