Merge pull request #810 from commercetools/fix-npe-invalid-response

fix NPE when server response has no body
commercetools · Jan 23, 2025 · 548f590 · 548f590
2 parents 3431113 + aac430e
commit 548f590
Show file tree

Hide file tree

Showing 6 changed files with 108 additions and 41 deletions.
diff --git a/.yarn/install-state.gz b/.yarn/install-state.gz
diff --git a/...dk-java-api/src/main/java/com/commercetools/api/client/error/ApiHttpExceptionFactory.java b/...dk-java-api/src/main/java/com/commercetools/api/client/error/ApiHttpExceptionFactory.java
@@ -1,6 +1,8 @@
 
 package com.commercetools.api.client.error;
 
+import java.util.Optional;
+
 import io.vrap.rmf.base.client.ApiHttpException;
 import io.vrap.rmf.base.client.ApiHttpRequest;
 import io.vrap.rmf.base.client.ApiHttpResponse;
@@ -28,25 +30,26 @@ public ApiHttpException createClientException(ApiHttpRequest request, ApiHttpRes
         String message = "Client error response [url] " + request.getUri().toString() + " [status code] "
                 + response.getStatusCode() + " [reason phrase] " + response.getMessage();
 
+        final String body = Optional.ofNullable(response.getBody()).map(String::new).orElse("");
         switch (response.getStatusCode()) {
             case 400:
-                return new BadRequestException(response.getStatusCode(), new String(response.getBody()),
-                    request.getHeaders(), message, response, request, serializer);
+                return new BadRequestException(response.getStatusCode(), body, request.getHeaders(), message, response,
+                    request, serializer);
             case 401:
-                return new UnauthorizedException(response.getStatusCode(), new String(response.getBody()),
-                    request.getHeaders(), message, response, request, serializer);
+                return new UnauthorizedException(response.getStatusCode(), body, request.getHeaders(), message,
+                    response, request, serializer);
             case 403:
-                return new ForbiddenException(response.getStatusCode(), new String(response.getBody()),
-                    request.getHeaders(), message, response, request, serializer);
+                return new ForbiddenException(response.getStatusCode(), body, request.getHeaders(), message, response,
+                    request, serializer);
             case 404:
-                return new NotFoundException(response.getStatusCode(), new String(response.getBody()),
-                    request.getHeaders(), message, response, request, serializer);
+                return new NotFoundException(response.getStatusCode(), body, request.getHeaders(), message, response,
+                    request, serializer);
             case 409:
-                return new ConcurrentModificationException(response.getStatusCode(), new String(response.getBody()),
-                    request.getHeaders(), message, response, request, serializer);
+                return new ConcurrentModificationException(response.getStatusCode(), body, request.getHeaders(),
+                    message, response, request, serializer);
         }
-        return new ApiClientException(response.getStatusCode(), new String(response.getBody()), response.getHeaders(),
-            message, response, request);
+        return new ApiClientException(response.getStatusCode(), body, response.getHeaders(), message, response,
+            request);
     }
 
     public static HttpExceptionFactory of(ResponseSerializer serializer) {

diff --git a/...api/src/main/java/com/commercetools/api/client/error/ConcurrentModificationException.java b/...api/src/main/java/com/commercetools/api/client/error/ConcurrentModificationException.java
@@ -1,6 +1,8 @@
 
 package com.commercetools.api.client.error;
 
+import java.util.Optional;
+
 import com.commercetools.api.models.error.ConcurrentModificationError;
 import com.commercetools.api.models.error.ErrorResponse;
 
@@ -57,11 +59,12 @@ public Long getCurrentVersion() {
     }
 
     public static Long retrieveCurrentVersion(final ErrorResponse errorResponse) {
-        return errorResponse.getErrors()
-                .stream()
-                .filter(errorObject -> errorObject instanceof ConcurrentModificationError)
-                .findFirst()
-                .map(errorObject -> ((ConcurrentModificationError) errorObject).getCurrentVersion())
+        return Optional.ofNullable(errorResponse)
+                .flatMap(response -> response.getErrors()
+                        .stream()
+                        .filter(errorObject -> errorObject instanceof ConcurrentModificationError)
+                        .findFirst()
+                        .map(errorObject -> ((ConcurrentModificationError) errorObject).getCurrentVersion()))
                 .orElse(null);
     }
 }
diff --git a/commercetools/commercetools-sdk-java-api/src/test/java/com/commercetools/ErrorTest.java b/commercetools/commercetools-sdk-java-api/src/test/java/com/commercetools/ErrorTest.java
@@ -2,19 +2,46 @@
 package com.commercetools;
 
 import static com.commercetools.TestUtils.stringFromResource;
+import static io.vrap.rmf.base.client.utils.ClientUtils.blockingWait;
 import static org.assertj.core.api.Assertions.assertThat;
 
 import java.io.IOException;
+import java.net.URI;
+import java.time.Duration;
+import java.util.concurrent.CompletableFuture;
 
+import com.commercetools.api.client.error.ApiHttpExceptionFactory;
 import com.commercetools.api.models.error.ErrorObject;
 import com.commercetools.api.models.error.ErrorResponse;
 import com.commercetools.api.models.error.InvalidJsonInputError;
+import com.tngtech.junit.dataprovider.DataProvider;
+import com.tngtech.junit.dataprovider.DataProviderExtension;
+import com.tngtech.junit.dataprovider.UseDataProvider;
+import com.tngtech.junit.dataprovider.UseDataProviderExtension;
 
+import io.vrap.rmf.base.client.*;
+import io.vrap.rmf.base.client.error.*;
+import io.vrap.rmf.base.client.http.ErrorMiddleware;
 import io.vrap.rmf.base.client.utils.json.JsonUtils;
 
+import org.assertj.core.api.Assertions;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
 
+@ExtendWith(DataProviderExtension.class)
+@ExtendWith(UseDataProviderExtension.class)
 public class ErrorTest {
+    @DataProvider
+    public static Object[][] exceptions() {
+        return new Object[][] { { 400, BadRequestException.class }, { 401, UnauthorizedException.class },
+                { 403, ForbiddenException.class }, { 404, NotFoundException.class },
+                { 409, com.commercetools.api.client.error.ConcurrentModificationException.class },
+                { 499, ApiClientException.class }, { 500, InternalServerErrorException.class },
+                { 502, BadGatewayException.class }, { 503, ServiceUnavailableException.class },
+                { 504, GatewayTimeoutException.class }, { 599, ApiServerException.class }, };
+    }
+
     @Test
     public void deprecatedAttributesAccessor() throws IOException {
         ErrorResponse errorResponse = JsonUtils.fromJsonString(stringFromResource("errors.json"), ErrorResponse.class);
@@ -25,4 +52,19 @@ public void deprecatedAttributesAccessor() throws IOException {
         final InvalidJsonInputError error = (InvalidJsonInputError) errorObject;
         assertThat(error.getDetailedErrorMessage()).isEqualTo("actions -> name: Missing required value");
     }
+
+    @TestTemplate
+    @UseDataProvider("exceptions")
+    public void testErrorInvalidResponse(int statusCode, Class<ApiHttpException> exceptionClass) {
+        ErrorMiddleware middleware = ErrorMiddleware.of(ApiHttpExceptionFactory.of(ResponseSerializer.of()));
+
+        final ApiHttpRequest request = new ApiHttpRequest(ApiHttpMethod.GET, URI.create("/"), new ApiHttpHeaders(),
+            "".getBytes());
+        final ApiHttpResponse<byte[]> response = new ApiHttpResponse<>(statusCode, null, null);
+
+        Assertions.assertThatExceptionOfType(exceptionClass).isThrownBy(() -> {
+            blockingWait(middleware.invoke(request, request1 -> CompletableFuture.completedFuture(response)),
+                Duration.ofSeconds(1));
+        }).matches(e -> e.getStatusCode() == statusCode);
+    }
 }
diff --git a/rmf/rmf-java-base/src/main/java/io/vrap/rmf/base/client/error/HttpExceptionFactory.java b/rmf/rmf-java-base/src/main/java/io/vrap/rmf/base/client/error/HttpExceptionFactory.java
@@ -1,6 +1,8 @@
 
 package io.vrap.rmf.base.client.error;
 
+import java.util.Optional;
+
 import io.vrap.rmf.base.client.ApiHttpException;
 import io.vrap.rmf.base.client.ApiHttpRequest;
 import io.vrap.rmf.base.client.ApiHttpResponse;
@@ -25,51 +27,53 @@ public default ApiHttpException create(final ApiHttpRequest request, final ApiHt
     public default ApiHttpException createServerException(final ApiHttpRequest request,
             final ApiHttpResponse<byte[]> response) {
         final ResponseSerializer serializer = getResponseSerializer();
-        String message = "Server error response [url] " + request.getUri().toString() + " [status code] "
+        final String message = "Server error response [url] " + request.getUri().toString() + " [status code] "
                 + response.getStatusCode() + " [reason phrase] " + response.getMessage();
 
+        final String body = Optional.ofNullable(response.getBody()).map(String::new).orElse("");
         switch (response.getStatusCode()) {
             case HttpStatusCode.INTERNAL_SERVER_ERROR_500:
-                return new InternalServerErrorException(response.getStatusCode(), new String(response.getBody()),
-                    request.getHeaders(), message, response, request, serializer);
+                return new InternalServerErrorException(response.getStatusCode(), body, request.getHeaders(), message,
+                    response, request, serializer);
             case HttpStatusCode.BAD_GATEWAY_502:
-                return new BadGatewayException(response.getStatusCode(), new String(response.getBody()),
-                    request.getHeaders(), message, response, request, serializer);
+                return new BadGatewayException(response.getStatusCode(), body, request.getHeaders(), message, response,
+                    request, serializer);
             case HttpStatusCode.SERVICE_UNAVAILABLE_503:
-                return new ServiceUnavailableException(response.getStatusCode(), new String(response.getBody()),
-                    request.getHeaders(), message, response, request, serializer);
+                return new ServiceUnavailableException(response.getStatusCode(), body, request.getHeaders(), message,
+                    response, request, serializer);
             case HttpStatusCode.GATEWAY_TIMEOUT_504:
-                return new GatewayTimeoutException(response.getStatusCode(), new String(response.getBody()),
-                    request.getHeaders(), message, response, request, serializer);
+                return new GatewayTimeoutException(response.getStatusCode(), body, request.getHeaders(), message,
+                    response, request, serializer);
         }
-        return new ApiServerException(response.getStatusCode(), new String(response.getBody()), response.getHeaders(),
-            message, response, request);
+        return new ApiServerException(response.getStatusCode(), body, response.getHeaders(), message, response,
+            request);
     }
 
     public default ApiHttpException createClientException(ApiHttpRequest request, ApiHttpResponse<byte[]> response) {
         final ResponseSerializer serializer = getResponseSerializer();
-        String message = "Client error response [url] " + request.getUri().toString() + " [status code] "
+        final String message = "Client error response [url] " + request.getUri().toString() + " [status code] "
                 + response.getStatusCode() + " [reason phrase] " + response.getMessage();
 
+        final String body = Optional.ofNullable(response.getBody()).map(String::new).orElse("");
         switch (response.getStatusCode()) {
             case HttpStatusCode.BAD_REQUEST_400:
-                return new BadRequestException(response.getStatusCode(), new String(response.getBody()),
-                    request.getHeaders(), message, response, request, serializer);
+                return new BadRequestException(response.getStatusCode(), body, request.getHeaders(), message, response,
+                    request, serializer);
             case HttpStatusCode.UNAUTHORIZED_401:
-                return new UnauthorizedException(response.getStatusCode(), new String(response.getBody()),
-                    request.getHeaders(), message, response, request, serializer);
+                return new UnauthorizedException(response.getStatusCode(), body, request.getHeaders(), message,
+                    response, request, serializer);
             case HttpStatusCode.FORBIDDEN_403:
-                return new ForbiddenException(response.getStatusCode(), new String(response.getBody()),
-                    request.getHeaders(), message, response, request, serializer);
+                return new ForbiddenException(response.getStatusCode(), body, request.getHeaders(), message, response,
+                    request, serializer);
             case HttpStatusCode.NOT_FOUND_404:
-                return new NotFoundException(response.getStatusCode(), new String(response.getBody()),
-                    request.getHeaders(), message, response, request, serializer);
+                return new NotFoundException(response.getStatusCode(), body, request.getHeaders(), message, response,
+                    request, serializer);
             case HttpStatusCode.CONFLICT_409:
-                return new ConcurrentModificationException(response.getStatusCode(), new String(response.getBody()),
-                    request.getHeaders(), message, response, request, serializer);
+                return new ConcurrentModificationException(response.getStatusCode(), body, request.getHeaders(),
+                    message, response, request, serializer);
         }
-        return new ApiClientException(response.getStatusCode(), new String(response.getBody()), response.getHeaders(),
-            message, response, request);
+        return new ApiClientException(response.getStatusCode(), body, response.getHeaders(), message, response,
+            request);
     }
 
     public static HttpExceptionFactory of(final ResponseSerializer serializer) {

diff --git a/rmf/rmf-java-base/src/test/java/io/vrap/rmf/base/client/http/ErrorMiddlewareTest.java b/rmf/rmf-java-base/src/test/java/io/vrap/rmf/base/client/http/ErrorMiddlewareTest.java
@@ -49,4 +49,19 @@ public void testError(int statusCode, Class<ApiHttpException> exceptionClass) {
                 Duration.ofSeconds(1));
         }).matches(e -> e.getStatusCode() == statusCode);
     }
+
+    @TestTemplate
+    @UseDataProvider("exceptions")
+    public void testErrorInvalidResponse(int statusCode, Class<ApiHttpException> exceptionClass) {
+        ErrorMiddleware middleware = ErrorMiddleware.of(HttpExceptionFactory.of(ResponseSerializer.of()));
+
+        final ApiHttpRequest request = new ApiHttpRequest(ApiHttpMethod.GET, URI.create("/"), new ApiHttpHeaders(),
+            "".getBytes());
+        final ApiHttpResponse<byte[]> response = new ApiHttpResponse<>(statusCode, null, null);
+
+        Assertions.assertThatExceptionOfType(exceptionClass).isThrownBy(() -> {
+            blockingWait(middleware.invoke(request, request1 -> CompletableFuture.completedFuture(response)),
+                Duration.ofSeconds(1));
+        }).matches(e -> e.getStatusCode() == statusCode);
+    }
 }