diff --git a/cloudplatform/connectivity-destination-service/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationServiceAdapter.java b/cloudplatform/connectivity-destination-service/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationServiceAdapter.java index 1ea1f20db..c4cb0afa2 100644 --- a/cloudplatform/connectivity-destination-service/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationServiceAdapter.java +++ b/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 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 ) diff --git a/cloudplatform/connectivity-destination-service/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationServiceAdapterTest.java b/cloudplatform/connectivity-destination-service/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationServiceAdapterTest.java index bb2870c5d..8b62e2188 100644 --- a/cloudplatform/connectivity-destination-service/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationServiceAdapterTest.java +++ b/cloudplatform/connectivity-destination-service/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationServiceAdapterTest.java @@ -14,15 +14,23 @@ 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.sap.cloud.sdk.cloudplatform.connectivity.DestinationRetrievalStrategy.withUserToken; +import static com.sap.cloud.sdk.cloudplatform.connectivity.DestinationRetrievalStrategy.withoutToken; 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; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.atLeastOnce; +import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; import static org.mockito.Mockito.when; +import java.io.ByteArrayInputStream; +import java.nio.charset.StandardCharsets; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; @@ -32,6 +40,10 @@ import javax.annotation.Nonnull; import javax.annotation.Nullable; +import org.apache.http.HttpVersion; +import org.apache.http.client.HttpClient; +import org.apache.http.entity.InputStreamEntity; +import org.apache.http.message.BasicHttpResponse; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; @@ -48,6 +60,7 @@ import com.sap.cloud.environment.servicebinding.api.ServiceBindingAccessor; import com.sap.cloud.environment.servicebinding.api.ServiceIdentifier; import com.sap.cloud.sdk.cloudplatform.connectivity.exception.DestinationAccessException; +import com.sap.cloud.sdk.cloudplatform.connectivity.exception.DestinationNotFoundException; import com.sap.cloud.sdk.cloudplatform.exception.MultipleServiceBindingsException; import com.sap.cloud.sdk.cloudplatform.exception.NoServiceBindingException; import com.sap.cloud.sdk.cloudplatform.security.ClientCredentials; @@ -56,6 +69,8 @@ import com.sap.cloud.security.config.Service; import com.sap.cloud.security.test.JwtGenerator; +import lombok.SneakyThrows; + @WireMockTest class DestinationServiceAdapterTest { @@ -118,10 +133,7 @@ void testWithoutUserTokenExchange() final DestinationServiceAdapter adapterToTest = createSut(DEFAULT_SERVICE_BINDING); final String response = - adapterToTest - .getConfigurationAsJson( - "/", - DestinationRetrievalStrategy.withoutToken(OnBehalfOf.TECHNICAL_USER_PROVIDER)); + adapterToTest.getConfigurationAsJson("/", withoutToken(OnBehalfOf.TECHNICAL_USER_PROVIDER)); assertThat(response).isEqualTo(DESTINATION_RESPONSE); verify( @@ -159,10 +171,7 @@ void testWithUserTokenExchange() TenantAccessor .executeWithTenant( () -> "tenant-id", - () -> adapterToTest - .getConfigurationAsJson( - "/", - DestinationRetrievalStrategy.withoutToken(NAMED_USER_CURRENT_TENANT))); + () -> adapterToTest.getConfigurationAsJson("/", withoutToken(NAMED_USER_CURRENT_TENANT))); assertThat(destinationResponse).isEqualTo(DESTINATION_RESPONSE); verify( @@ -192,10 +201,7 @@ void testWithUserTokenForwarding() // actual request, ensure that the tenant matches the one in the User JWT final String destinationResponse = - adapterToTest - .getConfigurationAsJson( - "/", - DestinationRetrievalStrategy.withUserToken(TECHNICAL_USER_CURRENT_TENANT, token)); + adapterToTest.getConfigurationAsJson("/", withUserToken(TECHNICAL_USER_CURRENT_TENANT, token)); assertThat(destinationResponse).isEqualTo(DESTINATION_RESPONSE); verify( @@ -253,11 +259,7 @@ void testFragmentName() final String destinationResponse = adapterToTest - .getConfigurationAsJson( - "/", - DestinationRetrievalStrategy - .withoutToken(TECHNICAL_USER_CURRENT_TENANT) - .withFragmentName(fragment)); + .getConfigurationAsJson("/", withoutToken(TECHNICAL_USER_CURRENT_TENANT).withFragmentName(fragment)); assertThat(destinationResponse).isEqualTo(DESTINATION_RESPONSE); @@ -370,6 +372,51 @@ void getDestinationServiceProviderTenantShouldThrowForMissingId() """); } + @SneakyThrows + @Test + void testErrorHandling() + { + final var httpClient = mock(HttpClient.class); + final var destination = DefaultHttpDestination.builder("http://foo").build(); + HttpClientAccessor.setHttpClientFactory(( dest ) -> dest == destination ? httpClient : null); + + final var destinations = Collections.singletonMap(OnBehalfOf.TECHNICAL_USER_PROVIDER, destination); + final var SUT = new DestinationServiceAdapter(destinations::get, () -> null, null); + + // setup 400 response + var stream400 = spy(new ByteArrayInputStream("bad, evil request".getBytes(StandardCharsets.UTF_8))); + var response400 = new BasicHttpResponse(HttpVersion.HTTP_1_1, 400, "Bad Request"); + response400.setEntity(new InputStreamEntity(stream400)); + doReturn(response400).when(httpClient).execute(any()); + + // test + assertThatThrownBy( + () -> SUT.getConfigurationAsJson("/service-path", withoutToken(OnBehalfOf.TECHNICAL_USER_PROVIDER))) + .isInstanceOf(DestinationAccessException.class) + .hasMessageContaining("status 400"); + + // verify closed stream + Mockito.verify(stream400, atLeastOnce()).close(); + + // setup 404 response + var stream404 = spy(new ByteArrayInputStream("Nothing here.".getBytes(StandardCharsets.UTF_8))); + var response404 = new BasicHttpResponse(HttpVersion.HTTP_1_1, 404, "Not Found"); + response404.setEntity(new InputStreamEntity(stream404)); + doReturn(response404).when(httpClient).execute(any()); + + // test + assertThatThrownBy( + () -> SUT.getConfigurationAsJson("/service-path", withoutToken(OnBehalfOf.TECHNICAL_USER_PROVIDER))) + .describedAs("A 404 should produce a DestinationNotFoundException") + .isInstanceOf(DestinationNotFoundException.class) + .hasMessageContaining("Destination could not be found"); + + // verify closed stream + Mockito.verify(stream404, atLeastOnce()).close(); + + HttpClientAccessor.setHttpClientFactory(null); + } + private static DestinationServiceAdapter createSut( @Nonnull final ServiceBinding... serviceBindings ) { return new DestinationServiceAdapter(null, () -> { diff --git a/release_notes.md b/release_notes.md index d31b23dae..021b91d38 100644 --- a/release_notes.md +++ b/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