-
Notifications
You must be signed in to change notification settings - Fork 867
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
base: aws-sdk-net-v3.7-development
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 |
---|---|---|
|
@@ -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; | ||
|
@@ -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) | ||
{ | ||
var responseBodyBytes = context.GetResponseBodyBytes(); | ||
|
||
// First, try to parse as a standard DeleteObjects response (for cases where S3 returns mixed results) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
// Create a DeleteObjectsResponse containing the error information | ||
var deleteObjectsResponse = new DeleteObjectsResponse(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we are now implementing It seems useless since |
||
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> | ||
|
There was a problem hiding this comment.
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?