Skip to content

Commit

Permalink
fix: Destination Service Error Handling (#555)
Browse files Browse the repository at this point in the history 
Co-authored-by: Alexander Dümont <[email protected]>
  • Loading branch information
@MatKuhr @newtork
MatKuhr and newtork authored Aug 26, 2024
1 parent e9d308f commit 3168bbb
Show file tree
Hide file tree
Showing 3 changed files with 95 additions and 42 deletions.
54 changes: 29 additions & 25 deletions ...src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationServiceAdapter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 )
Expand Down
81 changes: 64 additions & 17 deletions ...test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationServiceAdapterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -56,6 +69,8 @@
import com.sap.cloud.security.config.Service;
import com.sap.cloud.security.test.JwtGenerator;

import lombok.SneakyThrows;

@WireMockTest
class DestinationServiceAdapterTest
{
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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, () -> {
Expand Down
2 changes: 2 additions & 0 deletions release_notes.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down

0 comments on commit 3168bbb

Please sign in to comment.