fix: Destination Service Error Handling

cloudplatform/connectivity-destination-service/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationServiceAdapter.java

@@ -22,7 +22,9 @@
 import org.apache.http.StatusLine;
 import org.apache.http.client.methods.HttpGet;
 import org.apache.http.client.methods.HttpUriRequest;
+import org.slf4j.event.Level;
 
+import com.google.common.base.Strings;
 import com.sap.cloud.environment.servicebinding.api.DefaultServiceBindingAccessor;
 import com.sap.cloud.environment.servicebinding.api.ServiceBinding;
 import com.sap.cloud.environment.servicebinding.api.ServiceIdentifier;
@@ -148,35 +150,37 @@ private static String handleResponse( final HttpUriRequest request, final HttpRe
         final int statusCode = status.getStatusCode();
         final String reasonPhrase = status.getReasonPhrase();
 
-        log.debug("Destination service returned HTTP status {} ({})", statusCode, reasonPhrase);
-
-        if( statusCode != HttpStatus.SC_OK ) {
-            final String requestUri = request.getURI().getPath();
-            if( statusCode == HttpStatus.SC_NOT_FOUND ) {
-                throw new DestinationNotFoundException(
-                    null,
-                    "Destination could not be found for path " + requestUri + ".");
-            } else {
-                throw new DestinationAccessException(
-                    String
-                        .format(
-                            "Failed to get destinations: destination service returned HTTP status %s (%S) at '%s'.,",
-                            statusCode,
-                            reasonPhrase,
-                            requestUri));
-            }
+        Try<String> maybeBody = Try.of(() -> HttpEntityUtil.getResponseBody(response));
+        String logMessage = "Destination service returned HTTP status %s (%s)";
+        if( maybeBody.isFailure() ) {
+            final var ex =
+                new DestinationAccessException("Failed to read body from HTTP response", maybeBody.getCause());
+            maybeBody = Try.failure(ex);
+            logMessage = String.format(logMessage, statusCode, reasonPhrase);
+        } else {
+            logMessage = String.format(logMessage + "and body '%s'", statusCode, reasonPhrase, maybeBody.get());
         }
 
-        try {
-            final String responseBody = HttpEntityUtil.getResponseBody(response);
-            if( responseBody == null ) {
-                throw new DestinationAccessException("Failed to get destinations: no body returned in response.");
-            }
-            return responseBody;
+        if( statusCode == HttpStatus.SC_OK ) {
+            final var ex = new DestinationAccessException("Failed to get destinations: no body returned in response.");
+            maybeBody = maybeBody.filter(it -> !Strings.isNullOrEmpty(it), () -> ex);
+            log.atLevel(maybeBody.isSuccess() ? Level.DEBUG : Level.ERROR).log(logMessage);
+            return maybeBody.get();
         }
-        catch( final IOException e ) {
-            throw new DestinationAccessException(e);
+
+        log.error(logMessage);
+        final String requestUri = request.getURI().getPath();
+        if( statusCode == HttpStatus.SC_NOT_FOUND ) {
+            throw new DestinationNotFoundException(null, "Destination could not be found for path " + requestUri + ".");
         }
+        throw new DestinationAccessException(
+            String
+                .format(
+                    "Failed to get destinations: destination service returned HTTP status %s (%S) at '%s'.,",
+                    statusCode,
+                    reasonPhrase,
+                    requestUri));
+
     }
 
     private HttpUriRequest prepareRequest( final String servicePath, final DestinationRetrievalStrategy strategy )

cloudplatform/connectivity-destination-service/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationServiceAdapterTest.java

@@ -14,6 +14,8 @@
 import static com.github.tomakehurst.wiremock.client.WireMock.stubFor;
 import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;
 import static com.github.tomakehurst.wiremock.client.WireMock.verify;
+import static com.github.tomakehurst.wiremock.client.WireMock.badRequest;
+import static com.github.tomakehurst.wiremock.client.WireMock.notFound;
 import static com.sap.cloud.sdk.cloudplatform.connectivity.OnBehalfOf.NAMED_USER_CURRENT_TENANT;
 import static com.sap.cloud.sdk.cloudplatform.connectivity.OnBehalfOf.TECHNICAL_USER_CURRENT_TENANT;
 import static com.sap.cloud.sdk.cloudplatform.connectivity.XsuaaTokenMocker.mockXsuaaToken;
@@ -32,6 +34,7 @@
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 
+import com.sap.cloud.sdk.cloudplatform.connectivity.exception.DestinationNotFoundException;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.BeforeEach;
@@ -370,6 +373,39 @@ void getDestinationServiceProviderTenantShouldThrowForMissingId()
                 """);
     }
 
+    @Test
+    void testErrorHandling()
+    {
+        final DestinationServiceAdapter adapterToTest = createSut(DEFAULT_SERVICE_BINDING);
+        stubFor(get(urlEqualTo(DESTINATION_SERVICE_URL)).willReturn(badRequest().withBody("bad, evil request")));
+        stubFor(get(urlEqualTo(DESTINATION_SERVICE_URL + "dest")).willReturn(notFound()));
+
+        assertThatThrownBy(
+            () -> adapterToTest
+                .getConfigurationAsJson("/", DestinationRetrievalStrategy.withoutToken(TECHNICAL_USER_CURRENT_TENANT)))
+            .isInstanceOf(DestinationAccessException.class)
+            .hasMessageContaining("status 400");
+        assertThatThrownBy(
+            () -> adapterToTest
+                .getConfigurationAsJson(
+                    "/dest",
+                    DestinationRetrievalStrategy.withoutToken(TECHNICAL_USER_CURRENT_TENANT)))
+            .describedAs("A 404 should produce a DestinationNotFoundException")
+            .isInstanceOf(DestinationNotFoundException.class)
+            .hasMessageContaining("Destination could not be found");
+
+        // invoke adapter 200 times, ensuring the response content is consumed
+        for( int i = 0; i < 200; i++ ) {
+            assertThatThrownBy(
+                () -> adapterToTest
+                    .getConfigurationAsJson(
+                        "/",
+                        DestinationRetrievalStrategy.withoutToken(TECHNICAL_USER_CURRENT_TENANT)))
+                .isInstanceOf(DestinationAccessException.class)
+                .hasMessageContaining("status 400");
+        }
+    }
+
     private static DestinationServiceAdapter createSut( @Nonnull final ServiceBinding... serviceBindings )
     {
         return new DestinationServiceAdapter(null, () -> {

release_notes.md

@@ -25,6 +25,8 @@
 - Upgrade  to version `1.66.0` of `gRPC` dependencies coming in transitively when using `connectivity-ztis`
 - Improve the error handling for OData batch requests.
   In case an OData error is given within a batch response it will now be parsed and returned as `ODataServiceErrorException`.
+- Improve the error handling for requests to the destination service.
+  In case of an error a potential response body will now be logged with the error message.
 
 ### 🐛 Fixed Issues