Wrap CrtS3RuntimeException to SdkServiceException #2558
Conversation
| ? crtS3RuntimeExceptionOptional | ||
| .filter(item -> StringUtils.isNotBlank(item.getAwsErrorCode()) | ||
| || StringUtils.isNotBlank(item.getAwsErrorMessage())).map(e -> SdkServiceException.builder() | ||
| .statusCode(crtS3RuntimeExceptionOptional.get().getStatusCode()) |
There was a problem hiding this comment.
Can we create specific S3Exception based on the error code?
For example:
NoSuchBucket -> NoSuchBucketException.
That way, customers can handle specific cases without checking the error code by themselves.
If the error code can't be mapped, I think we should just return a generic S3ServiceException
If there's no error code, we should wrap it with SdkClientException.
WDUT?
There was a problem hiding this comment.
Implemented to get Service side exception.This uses reflection.
We might need to update the CrtS3RuntimeException to send requestId so that service exception is built with this.
There was a problem hiding this comment.
Can we write down each exception type rather than using reflection? I don't think there are many specific exceptions for get and put. https://github.com/aws/aws-sdk-java-v2/blob/master/services/s3/src/main/resources/codegen-resources/service-2.json#L509 In the future, we will generate the code. I think we should be very careful about using reflection.
| public Exception transformException(Exception crtRuntimeException) { | ||
| Optional<CrtS3RuntimeException> crtS3RuntimeExceptionOptional = getCrtS3RuntimeException(crtRuntimeException); | ||
|
|
||
| return crtS3RuntimeExceptionOptional.isPresent() |
There was a problem hiding this comment.
Minor: instead of checking if it's present and doing crtS3RuntimeExceptionOptional.get(), we can do the following:
return crtS3RuntimeExceptionOptional.filter(xxxx).orElse(crtRuntimeException)
Can we break up the code block within filter(xx)? It's a bit hard to read
| assertThatThrownBy(() -> s3Crt.getObject(GetObjectRequest.builder().bucket(BUCKET).key("randomKey").build(), | ||
| Paths.get(randomBaseDirectory).resolve("testFile")).get()) | ||
| .hasCauseInstanceOf(SdkServiceException.class) | ||
| .hasMessageContaining("Failed with Aws Error code \"NoSuchKey\" and Aws Error Message \"The specified key does not exist.\""); |
There was a problem hiding this comment.
Minor: we probably don't need to verify the whole message because services can change them, which will make the tests flaky.
Same as other tests
...r-manager/src/test/java/software/amazon/awssdk/transfer/s3/internal/CrtErrorHandlerTest.java
Outdated
Show resolved
Hide resolved
|
Kudos, SonarCloud Quality Gate passed! |
…3c5e5ce2e Pull request: release <- staging/242be335-3287-4272-b0ff-0433c5e5ce2e
Description
Motivation and Context
Testing
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsLicense