-
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?
Fix DeleteObjects to throw DeleteObjectsException instead of AmazonS3Exception #3839
Conversation
|
||
// 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 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
?
Discussed offline, this should target V4 instead as it's not a blocking issue in V3. |
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.
Functionally, looks good. Just needs an integ test and needs to target the development
branch.
/// <param name="innerException"></param> | ||
/// <param name="statusCode"></param> | ||
/// <returns></returns> | ||
public override AmazonServiceException UnmarshallException(XmlUnmarshallerContext context, Exception innerException, HttpStatusCode statusCode) |
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?
var errorResponse = S3ErrorResponseUnmarshaller.Instance.Unmarshall(context); | ||
|
||
// Create a DeleteObjectsResponse containing the error information | ||
var deleteObjectsResponse = new DeleteObjectsResponse(); |
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.
Since we are now implementing unmarshallException
properly can we remove this logic here as part of this PR?
It seems useless since UnmarshallException
will be called first and the sdk will throw an exception.
{ | ||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
can you explain what you mean by "mixed results"?
Description
Added UnmarshallException method override to
DeleteObjectsResponseUnmarshaller
to ensure that allDeleteObjects
operation failures consistently throwDeleteObjectsException
instead of the genericAmazonS3Exception
.The fix creates a
DeleteObjectsResponse
containing structured error information and wraps it in aDeleteObjectsException
, preserving all error details (ErrorCode
,RequestId
,StatusCode
) while providing the expected exception type.Motivation and Context
Fixes issue #3828 reported inconsistent exception handling for
DeleteObjects
operations.Testing
DeleteObjectsException
is now thrownResponse.DeleteErrors
collectionErrorCode
,RequestId
, andStatusCode
are properly preservedTypes of changes
Checklist
License