Skip to content

chore: Migrate connectivity-destination-service to HttpClient5 #1028

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
CharlesDuboisSAP wants to merge 2 commits into
base: main
Choose a base branch
from
+135 −125
Open

chore: Migrate connectivity-destination-service to HttpClient5 #1028

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5355c37
chore: Migrate `connectivity-destination-service` to HttpClient5
CharlesDuboisSAP Dec 10, 2025
40d74d7
formatting
CharlesDuboisSAP Dec 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 12 additions & 24 deletions cloudplatform/connectivity-destination-service/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,10 @@
<groupId>com.sap.cloud.sdk.cloudplatform</groupId>
<artifactId>cloudplatform-connectivity</artifactId>
</dependency>
<dependency>
<groupId>com.sap.cloud.sdk.cloudplatform</groupId>
<artifactId>connectivity-apache-httpclient4</artifactId>
</dependency>
<dependency>
<groupId>com.sap.cloud.sdk.cloudplatform</groupId>
<artifactId>connectivity-apache-httpclient5</artifactId>
</dependency>
<!-- Included by default to serve as the default implementation -->
<dependency>
<groupId>com.sap.cloud.sdk.cloudplatform</groupId>
Expand Down Expand Up @@ -97,26 +97,14 @@
<groupId>commons-io</groupId>
<artifactId>commons-io</artifactId>
</dependency>
<dependency>
<groupId>org.apache.httpcomponents</groupId>
<artifactId>httpcore</artifactId>
<exclusions>
<exclusion>
<artifactId>commons-logging</artifactId>
<groupId>commons-logging</groupId>
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>org.apache.httpcomponents</groupId>
<artifactId>httpclient</artifactId>
<exclusions>
<exclusion>
<artifactId>commons-logging</artifactId>
<groupId>commons-logging</groupId>
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>org.apache.httpcomponents.core5</groupId>
<artifactId>httpcore5</artifactId>
</dependency>
<dependency>
<groupId>org.apache.httpcomponents.client5</groupId>
<artifactId>httpclient5</artifactId>
</dependency>
<dependency>
<groupId>io.vavr</groupId>
<artifactId>vavr</artifactId>
Expand Down
14 changes: 5 additions & 9 deletions ...e/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/AuthTokenHeaderProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

import javax.annotation.Nonnull;

import org.apache.http.HttpHeaders;
import org.apache.hc.core5.http.HttpHeaders;

import com.google.common.base.Strings;
import com.sap.cloud.sdk.cloudplatform.connectivity.exception.DestinationAccessException;
Expand Down Expand Up @@ -96,13 +96,9 @@ private static List<Header> getDestinationHeaders( @Nonnull final List<Destinati

private static boolean isAuthTokenExpected( @Nonnull final AuthenticationType authType )
{
switch( authType ) {
case NO_AUTHENTICATION:
case PRINCIPAL_PROPAGATION:
case CLIENT_CERTIFICATE_AUTHENTICATION:
return false;
default:
return true;
}
return switch( authType ) {
case NO_AUTHENTICATION, PRINCIPAL_PROPAGATION, CLIENT_CERTIFICATE_AUTHENTICATION -> false;
default -> true;
};
}
}
84 changes: 47 additions & 37 deletions ...src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationServiceAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@
import javax.annotation.Nonnull;
import javax.annotation.Nullable;

import org.apache.http.HttpResponse;
import org.apache.http.HttpStatus;
import org.apache.http.StatusLine;
import org.apache.http.client.methods.HttpGet;
import org.apache.http.client.methods.HttpUriRequest;
import org.apache.hc.client5.http.classic.methods.HttpGet;
import org.apache.hc.client5.http.impl.classic.BasicHttpClientResponseHandler;
import org.apache.hc.core5.http.ClassicHttpRequest;
import org.apache.hc.core5.http.ClassicHttpResponse;
import org.apache.hc.core5.http.HttpStatus;

import com.google.common.base.Strings;
import com.sap.cloud.environment.servicebinding.api.DefaultServiceBindingAccessor;
Expand All @@ -32,6 +32,7 @@
import io.vavr.control.Try;
import lombok.AccessLevel;
import lombok.Getter;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;

@Slf4j
Expand Down Expand Up @@ -126,53 +127,62 @@ String getConfigurationAsJson(
serviceDestinationLoader.apply(strategy.behalf()),
() -> "Destination for Destination Service on behalf of " + strategy.behalf() + " not found.");

final HttpUriRequest request = prepareRequest(servicePath, strategy);
final ClassicHttpRequest request = prepareRequest(servicePath, strategy);

final HttpResponse response;
try {
response = HttpClientAccessor.getHttpClient(serviceDestination).execute(request);
return ApacheHttpClient5Accessor
.getHttpClient(serviceDestination)
.execute(request, new DestinationHttpClientResponseHandler(request));
}
catch( final IOException e ) {
throw new DestinationAccessException(e);
}
return handleResponse(request, response);
}

@Nonnull
private static String handleResponse( final HttpUriRequest request, final HttpResponse response )
@RequiredArgsConstructor( access = AccessLevel.PRIVATE )
static class DestinationHttpClientResponseHandler extends BasicHttpClientResponseHandler
{
final StatusLine status = response.getStatusLine();
final int statusCode = status.getStatusCode();
final String reasonPhrase = status.getReasonPhrase();
final ClassicHttpRequest request;

log.debug("Destination service returned HTTP status {} ({})", statusCode, reasonPhrase);
@Override
public String handleResponse( final ClassicHttpResponse response )
{
final int statusCode = response.getCode();
final String reasonPhrase = response.getReasonPhrase();

Try<String> maybeBody = Try.of(() -> HttpEntityUtil.getResponseBody(response));
if( maybeBody.isFailure() ) {
final var ex =
new DestinationAccessException("Failed to read body from HTTP response", maybeBody.getCause());
maybeBody = Try.failure(ex);
}
log.debug("Destination service returned HTTP status {} ({})", statusCode, reasonPhrase);

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);
return maybeBody.get();
}
Try<String> maybeBody = Try.of(() -> handleEntity(response.getEntity()));
if( maybeBody.isFailure() ) {
final var ex =
new DestinationAccessException("Failed to read body from HTTP response", maybeBody.getCause());
maybeBody = Try.failure(ex);
}

final String requestUri = request.getURI().getPath();
if( statusCode == HttpStatus.SC_NOT_FOUND ) {
throw new DestinationNotFoundException(null, "Destination could not be found for path " + requestUri + ".");
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);
return maybeBody.get();
}

final String requestUri = request.getRequestUri();
if( statusCode == HttpStatus.SC_NOT_FOUND ) {
throw new DestinationNotFoundException(
null,
"Destination could not be found for path " + requestUri + ".");
}
final String message =
"Failed to get destinations: destination service responded with HTTP status %s (%S) at '%s'."
.formatted(statusCode, reasonPhrase, requestUri);
final String messageWithBody =
message + " Body: %s".formatted(maybeBody.getOrElseGet(Throwable::getMessage));
log.error(messageWithBody);
throw new DestinationAccessException(message);
}
final String message =
"Failed to get destinations: destination service responded with HTTP status %s (%S) at '%s'."
.formatted(statusCode, reasonPhrase, requestUri);
final String messageWithBody = message + " Body: %s".formatted(maybeBody.getOrElseGet(Throwable::getMessage));
log.error(messageWithBody);
throw new DestinationAccessException(message);
}

private HttpUriRequest prepareRequest( final String servicePath, final DestinationRetrievalStrategy strategy )
private ClassicHttpRequest prepareRequest( final String servicePath, final DestinationRetrievalStrategy strategy )
{
final URI requestUri;
try {
Expand All @@ -183,7 +193,7 @@ private HttpUriRequest prepareRequest( final String servicePath, final Destinati
}

log.debug("Querying Destination Service via URI {}.", requestUri);
final HttpUriRequest request = new HttpGet(requestUri);
final ClassicHttpRequest request = new HttpGet(requestUri);

if( !servicePath.startsWith(DestinationService.PATH_DEFAULT)
&& !servicePath.startsWith(DestinationService.PATH_V2) ) {
Expand Down
2 changes: 1 addition & 1 deletion ...c/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/AuthTokenHeaderProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import java.util.Arrays;
import java.util.Collection;

import org.apache.http.HttpHeaders;
import org.apache.hc.core5.http.HttpHeaders;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.DisplayName;
Expand Down
61 changes: 38 additions & 23 deletions ...test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationServiceAdapterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
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.doAnswer;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.times;
Expand All @@ -40,10 +40,13 @@
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.apache.hc.client5.http.classic.HttpClient;
import org.apache.hc.core5.http.ClassicHttpRequest;
import org.apache.hc.core5.http.ContentType;
import org.apache.hc.core5.http.HttpStatus;
import org.apache.hc.core5.http.io.HttpClientResponseHandler;
import org.apache.hc.core5.http.io.entity.InputStreamEntity;
import org.apache.hc.core5.http.message.BasicClassicHttpResponse;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.BeforeEach;
Expand All @@ -52,13 +55,15 @@
import org.mockito.Mockito;

import com.auth0.jwt.JWT;
import com.github.tomakehurst.wiremock.junit5.WireMockExtension;
import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo;
import com.github.tomakehurst.wiremock.junit5.WireMockTest;
import com.sap.cloud.environment.servicebinding.api.DefaultServiceBinding;
import com.sap.cloud.environment.servicebinding.api.DefaultServiceBindingAccessor;
import com.sap.cloud.environment.servicebinding.api.ServiceBinding;
import com.sap.cloud.environment.servicebinding.api.ServiceBindingAccessor;
import com.sap.cloud.environment.servicebinding.api.ServiceIdentifier;
import com.sap.cloud.sdk.cloudplatform.connectivity.DestinationServiceAdapter.DestinationHttpClientResponseHandler;
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;
Expand Down Expand Up @@ -93,6 +98,9 @@ class DestinationServiceAdapterTest

private String xsuaaToken;

@RegisterExtension
static final WireMockExtension server = WireMockExtension.newInstance().build();

@BeforeAll
static void setupSession()
{
Expand Down Expand Up @@ -434,20 +442,33 @@ void getDestinationServiceProviderTenantShouldThrowForMissingId()
@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 HttpClient httpClient = mock(HttpClient.class);
final HttpDestination destination = DefaultHttpDestination.builder("http://foo").build();
ApacheHttpClient5Accessor.setHttpClientFactory(dest -> httpClient);

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
// prepare 400 response with a spied input stream so we can verify it is closed
final var stream400 = spy(new ByteArrayInputStream("bad, evil request".getBytes(StandardCharsets.UTF_8)));
final var response400 = new BasicClassicHttpResponse(HttpStatus.SC_BAD_REQUEST, "Bad Request");
response400.setEntity(new InputStreamEntity(stream400, -1, ContentType.TEXT_PLAIN));

// prepare 404 response with a spied input stream
final var stream404 = spy(new ByteArrayInputStream("Nothing here.".getBytes(StandardCharsets.UTF_8)));
final var response404 = new BasicClassicHttpResponse(HttpStatus.SC_NOT_FOUND, "Not Found");
response404.setEntity(new InputStreamEntity(stream404, -1, ContentType.TEXT_PLAIN));

// make the mocked httpClient call the provided response handler with our prepared responses
doAnswer(invocation -> {
final HttpClientResponseHandler<?> handler = invocation.getArgument(1);
return handler.handleResponse(response400);
}).doAnswer(invocation -> {
final HttpClientResponseHandler<?> handler = invocation.getArgument(1);
return handler.handleResponse(response404);
}).when(httpClient).execute(any(ClassicHttpRequest.class), any(DestinationHttpClientResponseHandler.class));

// first invocation -> 400 -> DestinationAccessException
assertThatThrownBy(
() -> SUT.getConfigurationAsJson("/service-path", withoutToken(OnBehalfOf.TECHNICAL_USER_PROVIDER)))
.isInstanceOf(DestinationAccessException.class)
Expand All @@ -456,13 +477,7 @@ void testErrorHandling()
// 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
// second invocation -> 404 -> DestinationNotFoundException
assertThatThrownBy(
() -> SUT.getConfigurationAsJson("/service-path", withoutToken(OnBehalfOf.TECHNICAL_USER_PROVIDER)))
.describedAs("A 404 should produce a DestinationNotFoundException")
Expand All @@ -472,7 +487,7 @@ void testErrorHandling()
// verify closed stream
Mockito.verify(stream404, atLeastOnce()).close();

HttpClientAccessor.setHttpClientFactory(null);
ApacheHttpClient5Accessor.setHttpClientFactory(null);
}

private static DestinationServiceAdapter createSut( @Nonnull final ServiceBinding... serviceBindings )
Expand Down
2 changes: 1 addition & 1 deletion ...va/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationServiceAuthenticationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
import javax.annotation.Nonnull;
import javax.annotation.Nullable;

import org.apache.http.HttpHeaders;
import org.apache.hc.core5.http.HttpHeaders;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;
Expand Down
Loading