Skip to content

Commit e89007e

Browse files
millemsmillems
authored
Improve error messaging for S3 HEAD responses by including HTTP status text as the errorMessage. (#5492)
1 parent 737609c commit e89007e

File tree

7 files changed

+98
-29
lines changed

7 files changed

+98
-29
lines changed

‎.changes/next-release/feature-AWSSDKforJavav2-5190b56.json

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"type": "feature",
3+
"category": "AWS SDK for Java v2",
4+
"contributor": "",
5+
"description": "Update service exception messages to never include the string \"null\" in the message."
6+
}

‎.changes/next-release/feature-AmazonS3-9e9969c.json

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"type": "feature",
3+
"category": "Amazon S3",
4+
"contributor": "",
5+
"description": "When S3 returns a HEAD response, use the HTTP status text as the errorMessage."
6+
}

‎core/aws-core/src/main/java/software/amazon/awssdk/awscore/exception/AwsServiceException.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,9 @@ public String getMessage() {
7474
if (message == null) {
7575
message = awsErrorDetails().errorMessage();
7676
}
77+
if (message == null) {
78+
return details.toString();
79+
}
7780
return message + " " + details;
7881
}
7982

‎core/aws-core/src/test/java/software/amazon/awssdk/awscore/exception/AwsServiceExceptionTest.java

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,19 @@ public void exceptionMessage_withoutExtendedRequestId() {
8282
assertThat(e.getMessage()).isEqualTo("errorMessage (Service: serviceName, Status Code: 500, Request ID: requestId)");
8383
}
8484

85+
@Test
86+
public void exceptionMessage_withoutErrorMessage() {
87+
AwsServiceException e = AwsServiceException.builder()
88+
.awsErrorDetails(AwsErrorDetails.builder()
89+
.serviceName("serviceName")
90+
.errorCode("errorCode")
91+
.build())
92+
.statusCode(500)
93+
.requestId("requestId")
94+
.build();
95+
assertThat(e.getMessage()).isEqualTo("(Service: serviceName, Status Code: 500, Request ID: requestId)");
96+
}
97+
8598
public void assertSkewed(int clientSideTimeOffset,
8699
String errorCode,
87100
int statusCode,

‎services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/handlers/ExceptionTranslationInterceptor.java

Lines changed: 39 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -36,41 +36,56 @@ public final class ExceptionTranslationInterceptor implements ExecutionIntercept
3636

3737
@Override
3838
public Throwable modifyException(Context.FailedExecution context, ExecutionAttributes executionAttributes) {
39-
40-
if (!isS3Exception404(context.exception()) || !isHeadRequest(context.request())) {
39+
if (!isS3ExceptionWithDetails(context.exception()) || !isHeadRequest(context.request())) {
4140
return context.exception();
4241
}
4342

4443
String message = context.exception().getMessage();
45-
S3Exception exception = (S3Exception) (context.exception());
44+
S3Exception exception = (S3Exception) context.exception();
4645

4746
String requestIdFromHeader = exception.awsErrorDetails()
4847
.sdkHttpResponse()
4948
.firstMatchingHeader("x-amz-request-id")
5049
.orElse(null);
5150

5251
String requestId = Optional.ofNullable(exception.requestId()).orElse(requestIdFromHeader);
52+
String extendedRequestId = exception.extendedRequestId();
5353

5454
AwsErrorDetails errorDetails = exception.awsErrorDetails();
5555

56-
if (context.request() instanceof HeadObjectRequest) {
57-
return NoSuchKeyException.builder()
58-
.awsErrorDetails(fillErrorDetails(errorDetails, "NoSuchKey",
59-
"The specified key does not exist."))
60-
.statusCode(404)
61-
.requestId(requestId)
62-
.message(message)
63-
.build();
64-
}
65-
66-
if (context.request() instanceof HeadBucketRequest) {
67-
return NoSuchBucketException.builder()
68-
.awsErrorDetails(fillErrorDetails(errorDetails, "NoSuchBucket",
69-
"The specified bucket does not exist."))
70-
.statusCode(404)
71-
.requestId(requestId)
72-
.message(message)
73-
.build();
56+
if (exception.statusCode() == 404) {
57+
if (context.request() instanceof HeadObjectRequest) {
58+
return NoSuchKeyException.builder()
59+
.awsErrorDetails(fillErrorDetails(errorDetails, "NoSuchKey",
60+
"The specified key does not exist."))
61+
.statusCode(404)
62+
.requestId(requestId)
63+
.extendedRequestId(extendedRequestId)
64+
.message(message)
65+
.build();
66+
}
67+
68+
if (context.request() instanceof HeadBucketRequest) {
69+
return NoSuchBucketException.builder()
70+
.awsErrorDetails(fillErrorDetails(errorDetails, "NoSuchBucket",
71+
"The specified bucket does not exist."))
72+
.statusCode(404)
73+
.requestId(requestId)
74+
.extendedRequestId(extendedRequestId)
75+
.message(message)
76+
.build();
77+
}
78+
} else if (errorDetails.errorMessage() == null) {
79+
// Populate the error message using the HTTP response status text. Usually that's just the value from the
80+
// HTTP spec (e.g. "Forbidden"), but sometimes S3 throws some more useful things in there, like "Slow Down".
81+
String errorMessage = errorDetails.sdkHttpResponse().statusText().orElse(null);
82+
return S3Exception.builder()
83+
.awsErrorDetails(fillErrorDetails(errorDetails, null, errorMessage))
84+
.statusCode(exception.statusCode())
85+
.requestId(requestId)
86+
.extendedRequestId(extendedRequestId)
87+
.message(errorMessage)
88+
.build();
7489
}
7590

7691
return context.exception();
@@ -84,11 +99,8 @@ private boolean isHeadRequest(SdkRequest request) {
8499
return (request instanceof HeadObjectRequest || request instanceof HeadBucketRequest);
85100
}
86101

87-
private boolean isS3Exception404(Throwable thrown) {
88-
if (!(thrown instanceof S3Exception)) {
89-
return false;
90-
}
91-
92-
return ((S3Exception) thrown).statusCode() == 404;
102+
private boolean isS3ExceptionWithDetails(Throwable thrown) {
103+
return thrown instanceof S3Exception &&
104+
((S3Exception) thrown).awsErrorDetails() != null;
93105
}
94106
}

‎services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/crossregion/S3CrossRegionAsyncClientTest.java

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,8 @@
4343
import org.mockito.ArgumentCaptor;
4444
import org.mockito.ArgumentMatchers;
4545
import org.mockito.Mockito;
46+
import software.amazon.awssdk.awscore.AwsResponse;
47+
import software.amazon.awssdk.awscore.exception.AwsServiceException;
4648
import software.amazon.awssdk.core.ResponseBytes;
4749
import software.amazon.awssdk.core.async.AsyncResponseTransformer;
4850
import software.amazon.awssdk.core.interceptor.Context;
@@ -60,6 +62,7 @@
6062
import software.amazon.awssdk.regions.Region;
6163
import software.amazon.awssdk.services.s3.S3AsyncClient;
6264
import software.amazon.awssdk.services.s3.S3AsyncClientBuilder;
65+
import software.amazon.awssdk.services.s3.S3Client;
6366
import software.amazon.awssdk.services.s3.endpoints.S3EndpointParams;
6467
import software.amazon.awssdk.services.s3.endpoints.S3EndpointProvider;
6568
import software.amazon.awssdk.services.s3.endpoints.internal.DefaultS3EndpointProvider;
@@ -68,7 +71,9 @@
6871
import software.amazon.awssdk.services.s3.model.GetObjectResponse;
6972
import software.amazon.awssdk.services.s3.model.ListObjectsV2Response;
7073
import software.amazon.awssdk.services.s3.model.S3Exception;
74+
import software.amazon.awssdk.services.s3.model.S3Response;
7175
import software.amazon.awssdk.services.s3.paginators.ListObjectsV2Publisher;
76+
import software.amazon.awssdk.services.sts.StsClient;
7277
import software.amazon.awssdk.testutils.service.http.MockAsyncHttpClient;
7378
import software.amazon.awssdk.utils.StringInputStream;
7479
import software.amazon.awssdk.utils.StringUtils;
@@ -257,7 +262,7 @@ void given_CrossRegionClient_when_CallsHeadObjectErrors_then_ShouldTerminateTheA
257262
clientBuilder().endpointOverride(null).region(OVERRIDE_CONFIGURED_REGION)
258263
.crossRegionAccessEnabled(true).build();
259264

260-
String errorMessage = String.format("software.amazon.awssdk.services.s3.model.S3Exception: null "
265+
String errorMessage = String.format("software.amazon.awssdk.services.s3.model.S3Exception: "
261266
+ "(Service: S3, Status Code: %d, Request ID: null)"
262267
, statusCode);
263268
assertThatExceptionOfType(CompletionException.class)
@@ -285,7 +290,7 @@ void given_CrossRegionClient_when_CallsHeadObjectWithNoRegion_then_ShouldTermina
285290

286291
assertThatExceptionOfType(CompletionException.class)
287292
.isThrownBy(() -> crossRegionClient.getObject(r -> r.bucket(BUCKET).key(KEY), AsyncResponseTransformer.toBytes()).join())
288-
.withMessageContaining("software.amazon.awssdk.services.s3.model.S3Exception: null (Service: S3, Status Code: 301, "
293+
.withMessageContaining("software.amazon.awssdk.services.s3.model.S3Exception: (Service: S3, Status Code: 301, "
289294
+ "Request ID: null)")
290295
.withCauseInstanceOf(S3Exception.class).withRootCauseExactlyInstanceOf(S3Exception.class);
291296

@@ -470,6 +475,7 @@ void given_globalRegion_Client_Updates_region_to_useast1_and_useGlobalEndpointFl
470475
assertThat(resolvedParams.region()).isEqualTo(Region.US_EAST_1);
471476
assertThat(resolvedParams.useGlobalEndpoint()).isFalse();
472477
});
478+
473479
}
474480

475481
private S3AsyncClientBuilder clientBuilder() {

‎services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/handlers/ExceptionTranslationInterceptorTest.java

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,18 @@ public void headObject404_shouldTranslateException() {
6363
.isExactlyInstanceOf(NoSuchKeyException.class);
6464
}
6565

66+
@Test
67+
public void headObject403_shouldTranslateException() {
68+
S3Exception s3Exception = create403S3Exception();
69+
Context.FailedExecution failedExecution = getFailedExecution(s3Exception,
70+
HeadObjectRequest.builder().build());
71+
72+
assertThat(interceptor.modifyException(failedExecution, new ExecutionAttributes()))
73+
.isExactlyInstanceOf(S3Exception.class)
74+
.hasMessageStartingWith("No can do")
75+
.satisfies(e -> assertThat(((S3Exception) e).awsErrorDetails().errorMessage()).isEqualTo("No can do"));
76+
}
77+
6678
@Test
6779
public void headObjectOtherException_shouldNotThrowException() {
6880
S3Exception s3Exception = (S3Exception) S3Exception.builder().statusCode(500).build();
@@ -95,4 +107,15 @@ private S3Exception create404S3Exception() {
95107
.statusCode(404)
96108
.build();
97109
}
110+
111+
private S3Exception create403S3Exception() {
112+
return (S3Exception) S3Exception.builder()
113+
.awsErrorDetails(AwsErrorDetails.builder()
114+
.sdkHttpResponse(SdkHttpFullResponse.builder()
115+
.statusText("No can do")
116+
.build())
117+
.build())
118+
.statusCode(403)
119+
.build();
120+
}
98121
}

0 commit comments

Comments
 (0)