From 60b086d521dba0ec6fa9c8d65131a97aaeb9da16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20D=C3=BCmont?= Date: Wed, 11 Feb 2026 16:48:50 +0100 Subject: [PATCH 01/17] Add current behavior test --- .../DefaultHttpClientCacheTest.java | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java index 9d06adb885..c328ae7e05 100644 --- a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java +++ b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java @@ -1,6 +1,7 @@ package com.sap.cloud.sdk.cloudplatform.connectivity; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; import static org.assertj.core.api.Assertions.assertThatThrownBy; import java.util.ArrayList; @@ -12,6 +13,7 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; +import lombok.SneakyThrows; import org.apache.http.client.HttpClient; import org.apache.http.client.methods.HttpGet; import org.apache.http.client.methods.HttpUriRequest; @@ -357,6 +359,37 @@ void testInvalidatePrincipalCacheEntriesWithUserTokenExchangeDestination() assertThat(unclearedClientWithoutDestination).isSameAs(sut.tryGetHttpClient(FACTORY).get()); } + @SneakyThrows + @Test + void testHttpClientWrapperWithDestinationIsCalledWhenDestinationIsProvided() + { + final DefaultHttpDestination destination1 = DefaultHttpDestination.builder("http://foo.com").build(); + final HttpClient client1 = sut.tryGetHttpClient(destination1, FACTORY).get(); + assertThat(((HttpClientWrapper) client1).getDestination()).isSameAs(destination1); + + final DefaultHttpDestination destination2 = DefaultHttpDestination.builder("http://foo.com").build(); + final HttpClient client2 = sut.tryGetHttpClient(destination2, FACTORY).get(); + assertThat(((HttpClientWrapper) client2).getDestination()).isSameAs(destination2); + + // Verify the destinations are equal but not the same reference + assertThat(destination1).isEqualTo(destination2); + assertThat(destination1).isNotSameAs(destination2); + + // Http clients are distinct instances, since the cache key contains the destination reference and not its content + assertThat(client1).isNotSameAs(client2); + + // When using the exact same destination object, the same http-client wrapper should be returned + final HttpClient client1Again = sut.tryGetHttpClient(destination1, FACTORY).get(); + assertThat(client1Again).isSameAs(client1); + assertThat(((HttpClientWrapper) client1Again).getDestination()).isSameAs(destination1); + + // simulate garbage collection + ((HttpClientWrapper) client1).close(); + + // since client1 inherited client2 connection manager, client2 is shut down as well + assertThatCode(() -> client2.execute(new HttpGet())).hasMessage("Connection pool shut down"); + } + @Test void testPrincipalPropagationIsPrincipalIsolated() { From cfa4e6ef5406b9a0e5f1fbb7a3c0ccd7fddf1f59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20D=C3=BCmont?= Date: Wed, 11 Feb 2026 16:58:45 +0100 Subject: [PATCH 02/17] Minor test extension --- .../connectivity/DefaultHttpClientCacheTest.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java index c328ae7e05..85f6d26a3d 100644 --- a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java +++ b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java @@ -361,18 +361,22 @@ void testInvalidatePrincipalCacheEntriesWithUserTokenExchangeDestination() @SneakyThrows @Test - void testHttpClientWrapperWithDestinationIsCalledWhenDestinationIsProvided() + void testHttpClientWrapperOutlivesGarbageCollector() { final DefaultHttpDestination destination1 = DefaultHttpDestination.builder("http://foo.com").build(); final HttpClient client1 = sut.tryGetHttpClient(destination1, FACTORY).get(); assertThat(((HttpClientWrapper) client1).getDestination()).isSameAs(destination1); - final DefaultHttpDestination destination2 = DefaultHttpDestination.builder("http://foo.com").build(); + final DefaultHttpDestination destination2 = + DefaultHttpDestination + .builder("http://foo.com") + .headerProviders(c -> List.of(new Header("Authorization", "Bearer foo"))) + .build(); final HttpClient client2 = sut.tryGetHttpClient(destination2, FACTORY).get(); assertThat(((HttpClientWrapper) client2).getDestination()).isSameAs(destination2); // Verify the destinations are equal but not the same reference - assertThat(destination1).isEqualTo(destination2); + assertThat(destination1).isEqualTo(destination2); // header providers are not part of the equality check assertThat(destination1).isNotSameAs(destination2); // Http clients are distinct instances, since the cache key contains the destination reference and not its content @@ -383,7 +387,7 @@ void testHttpClientWrapperWithDestinationIsCalledWhenDestinationIsProvided() assertThat(client1Again).isSameAs(client1); assertThat(((HttpClientWrapper) client1Again).getDestination()).isSameAs(destination1); - // simulate garbage collection + // simulate garbage collection on client1 ((HttpClientWrapper) client1).close(); // since client1 inherited client2 connection manager, client2 is shut down as well From 3956d753dbc3dab20bd8787775490f1004de7a6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20D=C3=BCmont?= Date: Wed, 11 Feb 2026 17:08:35 +0100 Subject: [PATCH 03/17] Improve test method name --- .../cloudplatform/connectivity/DefaultHttpClientCacheTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java index 85f6d26a3d..6428e38082 100644 --- a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java +++ b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java @@ -361,7 +361,7 @@ void testInvalidatePrincipalCacheEntriesWithUserTokenExchangeDestination() @SneakyThrows @Test - void testHttpClientWrapperOutlivesGarbageCollector() + void testCachedEqualHttpClientsClosingBehavior() { final DefaultHttpDestination destination1 = DefaultHttpDestination.builder("http://foo.com").build(); final HttpClient client1 = sut.tryGetHttpClient(destination1, FACTORY).get(); From c1ca4ce404cc249ca5eac7db454ab78e7171b9fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20D=C3=BCmont?= Date: Wed, 11 Feb 2026 17:09:41 +0100 Subject: [PATCH 04/17] Format --- .../cloudplatform/connectivity/DefaultHttpClientCacheTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java index 6428e38082..5a1372f709 100644 --- a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java +++ b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java @@ -13,7 +13,6 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; -import lombok.SneakyThrows; import org.apache.http.client.HttpClient; import org.apache.http.client.methods.HttpGet; import org.apache.http.client.methods.HttpUriRequest; @@ -30,6 +29,8 @@ import com.sap.cloud.sdk.cloudplatform.tenant.Tenant; import com.sap.cloud.sdk.testutil.TestContext; +import lombok.SneakyThrows; + @Isolated class DefaultHttpClientCacheTest { From 02d94a5838f98f65cfb048aba48e5545c747fb99 Mon Sep 17 00:00:00 2001 From: I538344 Date: Thu, 16 Jul 2026 10:18:42 +0200 Subject: [PATCH 05/17] WiP --- .../connectivity/DefaultHttpDestination.java | 23 ++++++++++++++-- .../connectivity/HttpClientWrapper.java | 6 ++--- .../DefaultHttpClientCacheTest.java | 27 ++++++++++--------- .../connectivity/HttpClientWrapperTest.java | 6 ++++- .../ApacheHttpClient5Wrapper.java | 6 ++--- .../DefaultApacheHttpClient5CacheTest.java | 17 +++++++----- .../connectivity/HttpClientWrapperTest.java | 6 ++++- 7 files changed, 61 insertions(+), 30 deletions(-) diff --git a/cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java b/cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java index abf2160107..da0bf30b77 100644 --- a/cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java +++ b/cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java @@ -511,8 +511,7 @@ public static Builder fromDestination( @Nonnull final Destination destination ) .getPropertyNames() .forEach(propertyName -> builder.property(propertyName, destination.get(propertyName).get())); - if( destination instanceof DefaultHttpDestination ) { - final DefaultHttpDestination httpDestination = (DefaultHttpDestination) destination; + if( destination instanceof DefaultHttpDestination httpDestination ) { builder.headers(httpDestination.customHeaders); builder .headerProviders(httpDestination.getCustomHeaderProviders().toArray(new DestinationHeaderProvider[0])); @@ -543,6 +542,7 @@ public boolean equals( @Nullable final Object o ) resolveCertificatesOnly(keyStoreSupplier.get().getOrNull()), resolveCertificatesOnly(that.keyStoreSupplier.get().getOrNull())) .append(resolveCertificatesOnly(trustStore), resolveCertificatesOnly(that.trustStore)) + .append(customHeaderProviders, that.customHeaderProviders) .isEquals(); } @@ -554,9 +554,28 @@ public int hashCode() .append(customHeaders) .append(resolveKeyStoreHashCode(keyStoreSupplier.get().getOrNull())) .append(resolveKeyStoreHashCode(trustStore)) + .append(computeHeaderProvidersHashCode(customHeaderProviders)) .toHashCode(); } + /** + * Computes a hash code for the custom header providers list. Since header providers can be lambda functions that + * don't have meaningful equals/hashCode implementations, we use the identity hash code of each provider to uniquely + * identify them. + * + * @param providers + * the list of header providers + * @return a hash code based on the identity of each provider + */ + private static int computeHeaderProvidersHashCode( @Nonnull final List providers ) + { + int result = 0; + for( final DestinationHeaderProvider provider : providers ) { + result = 31 * result + System.identityHashCode(provider); + } + return result; + } + /** * Builder class to allow for easy creation of an immutable {@code DefaultHttpDestination} instance. */ diff --git a/cloudplatform/connectivity-apache-httpclient4/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapper.java b/cloudplatform/connectivity-apache-httpclient4/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapper.java index e1e3739f87..2f60e4d97c 100644 --- a/cloudplatform/connectivity-apache-httpclient4/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapper.java +++ b/cloudplatform/connectivity-apache-httpclient4/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapper.java @@ -93,9 +93,9 @@ public String toString() HttpClientWrapper withDestination( final HttpDestinationProperties destination ) { - // explicitly check the reference equality, since equals doesn't check header providers - // this is a slight improvement, avoiding unnecessary wrapper instantiation - // in cases where destination objects are reused / served from cache + // Since equals() now includes customHeaderProviders in the equality check, + // this method will throw an exception if the destination has different header providers. + // This is the expected behavior to ensure HTTP clients are properly isolated by their configuration. if( !destination.equals(this.destination) ) { throw new ShouldNotHappenException( "This method must not be used outside of updating an instance of HttpClientWrapper for http clients served from the HttpClientCache."); diff --git a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java index 5a1372f709..1976d0c0de 100644 --- a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java +++ b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java @@ -1,7 +1,6 @@ package com.sap.cloud.sdk.cloudplatform.connectivity; import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatCode; import static org.assertj.core.api.Assertions.assertThatThrownBy; import java.util.ArrayList; @@ -182,8 +181,7 @@ void testGetClientWithDestinationUsesTenantOptionalForIsolation() } @Test - //This is a known limitation of excluding header providers in the equality check of destinations - void testGetClientReturnsSameClientForDestinationsWithOnlyDifferentHeaderProviders() + void testGetClientReturnsDifferentClientForDestinationsWithDifferentHeaderProviders() { final Header header1 = new Header("foo", "bar"); final Header header2 = new Header("foo1", "bar1"); @@ -196,25 +194,28 @@ void testGetClientReturnsSameClientForDestinationsWithOnlyDifferentHeaderProvide final DefaultHttpDestination secondDestination = DefaultHttpDestination - .fromDestination(firstDestination) + .builder("http://some-uri") .headerProviders(( any ) -> Collections.singletonList(header2)) .build(); + // Verify that destinations with different header providers are not equal + assertThat(firstDestination).isNotEqualTo(secondDestination); + final HttpClientWrapper client1 = (HttpClientWrapper) sut.tryGetHttpClient(firstDestination, FACTORY).get(); final HttpClientWrapper client2 = (HttpClientWrapper) sut.tryGetHttpClient(secondDestination, FACTORY).get(); assertThat(client1.getDestination()).isSameAs(firstDestination); assertThat(client2.getDestination()).isSameAs(secondDestination); + // Each client should be a distinct instance now + assertThat(client1).isNotSameAs(client2); + final HttpUriRequest request1 = client1.wrapRequest(new HttpGet()); final HttpUriRequest request2 = client2.wrapRequest(new HttpGet()); - // This behavior is to be improved by https://github.com/SAP/cloud-sdk-java-backlog/issues/396 + // Each destination's header provider should only add its own headers assertThat(request1.getAllHeaders()).containsExactly(new HttpClientWrapper.ApacheHttpHeader(header1)); - assertThat(request2.getAllHeaders()) - .containsExactly( - new HttpClientWrapper.ApacheHttpHeader(header1), - new HttpClientWrapper.ApacheHttpHeader(header2)); + assertThat(request2.getAllHeaders()).containsExactly(new HttpClientWrapper.ApacheHttpHeader(header2)); } @Test @@ -376,8 +377,8 @@ void testCachedEqualHttpClientsClosingBehavior() final HttpClient client2 = sut.tryGetHttpClient(destination2, FACTORY).get(); assertThat(((HttpClientWrapper) client2).getDestination()).isSameAs(destination2); - // Verify the destinations are equal but not the same reference - assertThat(destination1).isEqualTo(destination2); // header providers are not part of the equality check + // Verify the destinations are not equal due to different header providers + assertThat(destination1).isNotEqualTo(destination2); // header providers are now included in the equality check assertThat(destination1).isNotSameAs(destination2); // Http clients are distinct instances, since the cache key contains the destination reference and not its content @@ -391,8 +392,8 @@ void testCachedEqualHttpClientsClosingBehavior() // simulate garbage collection on client1 ((HttpClientWrapper) client1).close(); - // since client1 inherited client2 connection manager, client2 is shut down as well - assertThatCode(() -> client2.execute(new HttpGet())).hasMessage("Connection pool shut down"); + // since client1 did not inherit client2 connection manager, client2 is not shut down + client2.execute(new HttpGet()); } @Test diff --git a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapperTest.java b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapperTest.java index 736578b8a3..2e0ce45f53 100644 --- a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapperTest.java +++ b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapperTest.java @@ -22,9 +22,13 @@ void testDestinationWrapping() final DefaultHttpDestination thirdDestination = DefaultHttpDestination.builder("http://bar.com").build(); final HttpClientWrapper sut = new HttpClientWrapper(mock(CloseableHttpClient.class), firstDestination); + // withDestination returns the same wrapper instance when the destination reference is identical assertThat(sut.withDestination(firstDestination)).isSameAs(sut); - assertThat(sut.withDestination(firstDestination)).isNotSameAs(sut.withDestination(secondDestination)); + // withDestination throws an exception when destinations are not equal (different header providers) + assertThatThrownBy(() -> sut.withDestination(secondDestination)).isInstanceOf(ShouldNotHappenException.class); + + // withDestination throws an exception when destinations have different URIs assertThatThrownBy(() -> sut.withDestination(thirdDestination)).isInstanceOf(ShouldNotHappenException.class); } } diff --git a/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/ApacheHttpClient5Wrapper.java b/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/ApacheHttpClient5Wrapper.java index e7e48783ee..e4feef9ab8 100644 --- a/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/ApacheHttpClient5Wrapper.java +++ b/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/ApacheHttpClient5Wrapper.java @@ -78,9 +78,9 @@ public void close( final CloseMode closeMode ) ApacheHttpClient5Wrapper withDestination( final HttpDestinationProperties destination ) { - // explicitly check the reference equality, since equals doesn't check header providers - // this is a slight improvement, avoiding unnecessary wrapper instantiation - // in cases where destination objects are reused / served from cache + // Since equals() now includes customHeaderProviders in the equality check, + // this method will throw an exception if the destination has different header providers. + // This is the expected behavior to ensure HTTP clients are properly isolated by their configuration. if( !destination.equals(this.destination) ) { throw new ShouldNotHappenException( "This method must not be used outside of updating an instance of ApacheHttpClient5Wrapper for http clients served from the ApacheHttpClient5Cache."); diff --git a/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5CacheTest.java b/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5CacheTest.java index 7295ec9d96..44d92f5bd6 100644 --- a/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5CacheTest.java +++ b/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5CacheTest.java @@ -348,8 +348,7 @@ void testInvalidatePrincipalCacheEntriesWithUserTokenExchangeDestination() } @Test - //This is a known limitation of excluding header providers in the equality check of destinations - void testGetClientReturnsSameClientForDestinationsWithOnlyDifferentHeaderProviders() + void testGetClientReturnsDifferentClientForDestinationsWithDifferentHeaderProviders() { final Header header1 = new Header("foo", "bar"); final Header header2 = new Header("foo1", "bar1"); @@ -362,10 +361,13 @@ void testGetClientReturnsSameClientForDestinationsWithOnlyDifferentHeaderProvide final DefaultHttpDestination secondDestination = DefaultHttpDestination - .fromDestination(firstDestination) + .builder("http://some-uri") .headerProviders(( any ) -> Collections.singletonList(header2)) .build(); + // Verify that destinations with different header providers are not equal + assertThat(firstDestination).isNotEqualTo(secondDestination); + final ApacheHttpClient5Wrapper client1 = (ApacheHttpClient5Wrapper) sut.tryGetHttpClient(firstDestination, FACTORY).get(); final ApacheHttpClient5Wrapper client2 = @@ -374,6 +376,9 @@ void testGetClientReturnsSameClientForDestinationsWithOnlyDifferentHeaderProvide assertThat(client1.getDestination()).isSameAs(firstDestination); assertThat(client2.getDestination()).isSameAs(secondDestination); + // Each client should be a distinct instance now + assertThat(client1).isNotSameAs(client2); + final ClassicHttpRequest request1 = client1.wrapRequest(new HttpGet("/")); final ClassicHttpRequest request2 = client2.wrapRequest(new HttpGet("/")); @@ -382,15 +387,13 @@ void testGetClientReturnsSameClientForDestinationsWithOnlyDifferentHeaderProvide request1.headerIterator().forEachRemaining(headersRequest1::add); request2.headerIterator().forEachRemaining(headersRequest2::add); - // recursive comparison because BasicHeader doesn't implement equals/hashCode + // Each destination's header provider should only add its own headers assertThat(headersRequest1) .usingRecursiveFieldByFieldElementComparator() .containsExactly(new BasicHeader(header1.getName(), header1.getValue())); assertThat(headersRequest2) .usingRecursiveFieldByFieldElementComparator() - .containsExactly( - new BasicHeader(header1.getName(), header1.getValue()), - new BasicHeader(header2.getName(), header2.getValue())); + .containsExactly(new BasicHeader(header2.getName(), header2.getValue())); } @Test diff --git a/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapperTest.java b/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapperTest.java index 154a584f4e..d07128acc7 100644 --- a/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapperTest.java +++ b/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapperTest.java @@ -24,9 +24,13 @@ void testDestinationWrapping() final ApacheHttpClient5Wrapper sut = new ApacheHttpClient5Wrapper(mock(CloseableHttpClient.class), firstDestination, mock(RequestConfig.class)); + // withDestination returns the same wrapper instance when the destination reference is identical assertThat(sut.withDestination(firstDestination)).isSameAs(sut); - assertThat(sut.withDestination(firstDestination)).isNotSameAs(sut.withDestination(secondDestination)); + // withDestination throws an exception when destinations are not equal (different header providers) + assertThatThrownBy(() -> sut.withDestination(secondDestination)).isInstanceOf(ShouldNotHappenException.class); + + // withDestination throws an exception when destinations have different URIs assertThatThrownBy(() -> sut.withDestination(thirdDestination)).isInstanceOf(ShouldNotHappenException.class); } } From 7197bb653d4fbac9cd66240aaf4daa08d085cee8 Mon Sep 17 00:00:00 2001 From: I538344 Date: Wed, 22 Jul 2026 11:17:31 +0200 Subject: [PATCH 06/17] WiP2 --- .../connectivity/DefaultHttpDestination.java | 30 ++++++++++++++++++- .../DefaultHttpClientCacheTest.java | 10 ++++--- 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java b/cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java index da0bf30b77..3be4a67ab1 100644 --- a/cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java +++ b/cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java @@ -542,7 +542,7 @@ public boolean equals( @Nullable final Object o ) resolveCertificatesOnly(keyStoreSupplier.get().getOrNull()), resolveCertificatesOnly(that.keyStoreSupplier.get().getOrNull())) .append(resolveCertificatesOnly(trustStore), resolveCertificatesOnly(that.trustStore)) - .append(customHeaderProviders, that.customHeaderProviders) + .append(areHeaderProvidersEqual(this.customHeaderProviders, that.customHeaderProviders), true) .isEquals(); } @@ -576,6 +576,34 @@ private static int computeHeaderProvidersHashCode( @Nonnull final List providers1, + @Nonnull final List providers2 ) + { + if( providers1.size() != providers2.size() ) { + return false; + } + + for( int i = 0; i < providers1.size(); i++ ) { + if( providers1.get(i) != providers2.get(i) ) { + return false; + } + } + + return true; + } + /** * Builder class to allow for easy creation of an immutable {@code DefaultHttpDestination} instance. */ diff --git a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java index 1976d0c0de..549fcc6d22 100644 --- a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java +++ b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java @@ -198,8 +198,9 @@ void testGetClientReturnsDifferentClientForDestinationsWithDifferentHeaderProvid .headerProviders(( any ) -> Collections.singletonList(header2)) .build(); - // Verify that destinations with different header providers are not equal - assertThat(firstDestination).isNotEqualTo(secondDestination); + // Note: Destinations with different header providers will be equal since header providers + // are not part of equality comparison. However, they are still different instances, + // so they will be handled separately by the HTTP client cache. final HttpClientWrapper client1 = (HttpClientWrapper) sut.tryGetHttpClient(firstDestination, FACTORY).get(); final HttpClientWrapper client2 = (HttpClientWrapper) sut.tryGetHttpClient(secondDestination, FACTORY).get(); @@ -377,8 +378,9 @@ void testCachedEqualHttpClientsClosingBehavior() final HttpClient client2 = sut.tryGetHttpClient(destination2, FACTORY).get(); assertThat(((HttpClientWrapper) client2).getDestination()).isSameAs(destination2); - // Verify the destinations are not equal due to different header providers - assertThat(destination1).isNotEqualTo(destination2); // header providers are now included in the equality check + // Note: Destinations are now equal even with different header providers, since header providers + // are not part of the equality check. However, they are different instances, so they result in + // different HTTP clients due to the cache key being based on instance identity. assertThat(destination1).isNotSameAs(destination2); // Http clients are distinct instances, since the cache key contains the destination reference and not its content From 7fdeed3a308e5c200543db17e98aa78fd24e9b30 Mon Sep 17 00:00:00 2001 From: I538344 Date: Wed, 22 Jul 2026 13:48:50 +0200 Subject: [PATCH 07/17] WiP 5 million --- .../connectivity/DefaultHttpDestination.java | 77 +++++-------------- .../connectivity/HttpClientWrapper.java | 6 +- .../DefaultHttpClientCacheTest.java | 19 ++--- .../connectivity/HttpClientWrapperTest.java | 6 +- .../ApacheHttpClient5Wrapper.java | 6 +- .../DefaultApacheHttpClient5CacheTest.java | 17 ++-- .../connectivity/HttpClientWrapperTest.java | 6 +- .../TransparentProxyDestination.java | 19 +++-- ...h2ServiceBindingDestinationLoaderTest.java | 58 +++++++++++++- 9 files changed, 111 insertions(+), 103 deletions(-) diff --git a/cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java b/cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java index 3be4a67ab1..4f41947527 100644 --- a/cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java +++ b/cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java @@ -50,6 +50,19 @@ @Slf4j public final class DefaultHttpDestination implements HttpDestination { + /** + * Cached header providers from class loading. These are loaded once at class initialization time and reused across + * all destination instances to ensure stable identities for cache key generation. This ensures that two + * destinations with identical configurations will have the same header provider instances, enabling proper HTTP + * client caching. + */ + @Nonnull + private static final ImmutableList CACHED_HEADER_PROVIDERS_FROM_CLASS_LOADING = + ImmutableList + . builder() + .addAll(FacadeLocator.getFacades(DestinationHeaderProvider.class)) + .build(); + @Delegate private final DestinationProperties baseProperties; @@ -66,9 +79,6 @@ public final class DefaultHttpDestination implements HttpDestination @Getter( AccessLevel.PACKAGE ) private final ImmutableList customHeaderProviders; - @Nonnull - private final ImmutableList headerProvidersFromClassLoading; - // the following 'cached' fields are ALWAYS derived from the baseProperties and stored in the corresponding fields // to avoid additional computation at runtime ONLY. // this is why we are calling them 'cached'. @@ -77,6 +87,12 @@ public final class DefaultHttpDestination implements HttpDestination // in other words: caching the values is safe and will not lead to any inconsistencies. // furthermore, it is safe to exclude these fields from the equals and hashCode methods because their values are // purely derived from the baseProperties, which are included in the equals and hashCode methods. + // + // NOTE: customHeaderProviders and headerProvidersFromClassLoading are intentionally excluded from equals() and hashCode() + // because header providers are called at request time to generate dynamic headers. Two destinations with identical + // configurations but different provider instances should still share the same HTTP client, as the providers generate + // headers when the client executes the request. By caching header providers globally (see CACHED_HEADER_PROVIDERS_FROM_CLASS_LOADING), + // we ensure that identical destinations will have the same provider instances anyway. @Nonnull private final Option cachedProxyConfiguration; @@ -114,11 +130,6 @@ private DefaultHttpDestination( this.customHeaders = customHeaders != null ? ImmutableList.
builder().addAll(customHeaders).build() : ImmutableList.of(); - final Collection headerProvidersFromClassLoading = - FacadeLocator.getFacades(DestinationHeaderProvider.class); - this.headerProvidersFromClassLoading = - ImmutableList. builder().addAll(headerProvidersFromClassLoading).build(); - this.customHeaderProviders = customHeaderProviders != null ? ImmutableList. builder().addAll(customHeaderProviders).build() @@ -176,7 +187,7 @@ public Collection
getHeaders( @Nonnull final URI requestUri ) this, requestUri, customHeaderProviders, - headerProvidersFromClassLoading)); + CACHED_HEADER_PROVIDERS_FROM_CLASS_LOADING)); allHeaders.addAll(cachedHeadersFromProperties); if( allHeaders.stream().noneMatch(header -> header.getName().equalsIgnoreCase(HttpHeaders.AUTHORIZATION)) ) { allHeaders.addAll(getHeadersForAuthType()); @@ -542,7 +553,6 @@ public boolean equals( @Nullable final Object o ) resolveCertificatesOnly(keyStoreSupplier.get().getOrNull()), resolveCertificatesOnly(that.keyStoreSupplier.get().getOrNull())) .append(resolveCertificatesOnly(trustStore), resolveCertificatesOnly(that.trustStore)) - .append(areHeaderProvidersEqual(this.customHeaderProviders, that.customHeaderProviders), true) .isEquals(); } @@ -554,56 +564,9 @@ public int hashCode() .append(customHeaders) .append(resolveKeyStoreHashCode(keyStoreSupplier.get().getOrNull())) .append(resolveKeyStoreHashCode(trustStore)) - .append(computeHeaderProvidersHashCode(customHeaderProviders)) .toHashCode(); } - /** - * Computes a hash code for the custom header providers list. Since header providers can be lambda functions that - * don't have meaningful equals/hashCode implementations, we use the identity hash code of each provider to uniquely - * identify them. - * - * @param providers - * the list of header providers - * @return a hash code based on the identity of each provider - */ - private static int computeHeaderProvidersHashCode( @Nonnull final List providers ) - { - int result = 0; - for( final DestinationHeaderProvider provider : providers ) { - result = 31 * result + System.identityHashCode(provider); - } - return result; - } - - /** - * Compares two lists of header providers using identity-based equality. Since header providers can be lambda - * functions that don't have meaningful equals/hashCode implementations, we compare them by identity. - * - * @param providers1 - * the first list of header providers - * @param providers2 - * the second list of header providers - * @return true if both lists have the same size and all providers are the same instance (by identity); false - * otherwise - */ - private static boolean areHeaderProvidersEqual( - @Nonnull final List providers1, - @Nonnull final List providers2 ) - { - if( providers1.size() != providers2.size() ) { - return false; - } - - for( int i = 0; i < providers1.size(); i++ ) { - if( providers1.get(i) != providers2.get(i) ) { - return false; - } - } - - return true; - } - /** * Builder class to allow for easy creation of an immutable {@code DefaultHttpDestination} instance. */ diff --git a/cloudplatform/connectivity-apache-httpclient4/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapper.java b/cloudplatform/connectivity-apache-httpclient4/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapper.java index 2f60e4d97c..e1e3739f87 100644 --- a/cloudplatform/connectivity-apache-httpclient4/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapper.java +++ b/cloudplatform/connectivity-apache-httpclient4/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapper.java @@ -93,9 +93,9 @@ public String toString() HttpClientWrapper withDestination( final HttpDestinationProperties destination ) { - // Since equals() now includes customHeaderProviders in the equality check, - // this method will throw an exception if the destination has different header providers. - // This is the expected behavior to ensure HTTP clients are properly isolated by their configuration. + // explicitly check the reference equality, since equals doesn't check header providers + // this is a slight improvement, avoiding unnecessary wrapper instantiation + // in cases where destination objects are reused / served from cache if( !destination.equals(this.destination) ) { throw new ShouldNotHappenException( "This method must not be used outside of updating an instance of HttpClientWrapper for http clients served from the HttpClientCache."); diff --git a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java index 549fcc6d22..7705311ddf 100644 --- a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java +++ b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java @@ -181,7 +181,8 @@ void testGetClientWithDestinationUsesTenantOptionalForIsolation() } @Test - void testGetClientReturnsDifferentClientForDestinationsWithDifferentHeaderProviders() + //This is a known limitation of excluding header providers in the equality check of destinations + void testGetClientReturnsSameClientForDestinationsWithOnlyDifferentHeaderProviders() { final Header header1 = new Header("foo", "bar"); final Header header2 = new Header("foo1", "bar1"); @@ -194,29 +195,25 @@ void testGetClientReturnsDifferentClientForDestinationsWithDifferentHeaderProvid final DefaultHttpDestination secondDestination = DefaultHttpDestination - .builder("http://some-uri") + .fromDestination(firstDestination) .headerProviders(( any ) -> Collections.singletonList(header2)) .build(); - // Note: Destinations with different header providers will be equal since header providers - // are not part of equality comparison. However, they are still different instances, - // so they will be handled separately by the HTTP client cache. - final HttpClientWrapper client1 = (HttpClientWrapper) sut.tryGetHttpClient(firstDestination, FACTORY).get(); final HttpClientWrapper client2 = (HttpClientWrapper) sut.tryGetHttpClient(secondDestination, FACTORY).get(); assertThat(client1.getDestination()).isSameAs(firstDestination); assertThat(client2.getDestination()).isSameAs(secondDestination); - // Each client should be a distinct instance now - assertThat(client1).isNotSameAs(client2); - final HttpUriRequest request1 = client1.wrapRequest(new HttpGet()); final HttpUriRequest request2 = client2.wrapRequest(new HttpGet()); - // Each destination's header provider should only add its own headers + // This behavior is to be improved by https://github.com/SAP/cloud-sdk-java-backlog/issues/396 assertThat(request1.getAllHeaders()).containsExactly(new HttpClientWrapper.ApacheHttpHeader(header1)); - assertThat(request2.getAllHeaders()).containsExactly(new HttpClientWrapper.ApacheHttpHeader(header2)); + assertThat(request2.getAllHeaders()) + .containsExactly( + new HttpClientWrapper.ApacheHttpHeader(header1), + new HttpClientWrapper.ApacheHttpHeader(header2)); } @Test diff --git a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapperTest.java b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapperTest.java index 2e0ce45f53..736578b8a3 100644 --- a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapperTest.java +++ b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapperTest.java @@ -22,13 +22,9 @@ void testDestinationWrapping() final DefaultHttpDestination thirdDestination = DefaultHttpDestination.builder("http://bar.com").build(); final HttpClientWrapper sut = new HttpClientWrapper(mock(CloseableHttpClient.class), firstDestination); - // withDestination returns the same wrapper instance when the destination reference is identical assertThat(sut.withDestination(firstDestination)).isSameAs(sut); + assertThat(sut.withDestination(firstDestination)).isNotSameAs(sut.withDestination(secondDestination)); - // withDestination throws an exception when destinations are not equal (different header providers) - assertThatThrownBy(() -> sut.withDestination(secondDestination)).isInstanceOf(ShouldNotHappenException.class); - - // withDestination throws an exception when destinations have different URIs assertThatThrownBy(() -> sut.withDestination(thirdDestination)).isInstanceOf(ShouldNotHappenException.class); } } diff --git a/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/ApacheHttpClient5Wrapper.java b/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/ApacheHttpClient5Wrapper.java index e4feef9ab8..e7e48783ee 100644 --- a/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/ApacheHttpClient5Wrapper.java +++ b/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/ApacheHttpClient5Wrapper.java @@ -78,9 +78,9 @@ public void close( final CloseMode closeMode ) ApacheHttpClient5Wrapper withDestination( final HttpDestinationProperties destination ) { - // Since equals() now includes customHeaderProviders in the equality check, - // this method will throw an exception if the destination has different header providers. - // This is the expected behavior to ensure HTTP clients are properly isolated by their configuration. + // explicitly check the reference equality, since equals doesn't check header providers + // this is a slight improvement, avoiding unnecessary wrapper instantiation + // in cases where destination objects are reused / served from cache if( !destination.equals(this.destination) ) { throw new ShouldNotHappenException( "This method must not be used outside of updating an instance of ApacheHttpClient5Wrapper for http clients served from the ApacheHttpClient5Cache."); diff --git a/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5CacheTest.java b/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5CacheTest.java index 44d92f5bd6..7295ec9d96 100644 --- a/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5CacheTest.java +++ b/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5CacheTest.java @@ -348,7 +348,8 @@ void testInvalidatePrincipalCacheEntriesWithUserTokenExchangeDestination() } @Test - void testGetClientReturnsDifferentClientForDestinationsWithDifferentHeaderProviders() + //This is a known limitation of excluding header providers in the equality check of destinations + void testGetClientReturnsSameClientForDestinationsWithOnlyDifferentHeaderProviders() { final Header header1 = new Header("foo", "bar"); final Header header2 = new Header("foo1", "bar1"); @@ -361,13 +362,10 @@ void testGetClientReturnsDifferentClientForDestinationsWithDifferentHeaderProvid final DefaultHttpDestination secondDestination = DefaultHttpDestination - .builder("http://some-uri") + .fromDestination(firstDestination) .headerProviders(( any ) -> Collections.singletonList(header2)) .build(); - // Verify that destinations with different header providers are not equal - assertThat(firstDestination).isNotEqualTo(secondDestination); - final ApacheHttpClient5Wrapper client1 = (ApacheHttpClient5Wrapper) sut.tryGetHttpClient(firstDestination, FACTORY).get(); final ApacheHttpClient5Wrapper client2 = @@ -376,9 +374,6 @@ void testGetClientReturnsDifferentClientForDestinationsWithDifferentHeaderProvid assertThat(client1.getDestination()).isSameAs(firstDestination); assertThat(client2.getDestination()).isSameAs(secondDestination); - // Each client should be a distinct instance now - assertThat(client1).isNotSameAs(client2); - final ClassicHttpRequest request1 = client1.wrapRequest(new HttpGet("/")); final ClassicHttpRequest request2 = client2.wrapRequest(new HttpGet("/")); @@ -387,13 +382,15 @@ void testGetClientReturnsDifferentClientForDestinationsWithDifferentHeaderProvid request1.headerIterator().forEachRemaining(headersRequest1::add); request2.headerIterator().forEachRemaining(headersRequest2::add); - // Each destination's header provider should only add its own headers + // recursive comparison because BasicHeader doesn't implement equals/hashCode assertThat(headersRequest1) .usingRecursiveFieldByFieldElementComparator() .containsExactly(new BasicHeader(header1.getName(), header1.getValue())); assertThat(headersRequest2) .usingRecursiveFieldByFieldElementComparator() - .containsExactly(new BasicHeader(header2.getName(), header2.getValue())); + .containsExactly( + new BasicHeader(header1.getName(), header1.getValue()), + new BasicHeader(header2.getName(), header2.getValue())); } @Test diff --git a/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapperTest.java b/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapperTest.java index d07128acc7..154a584f4e 100644 --- a/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapperTest.java +++ b/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapperTest.java @@ -24,13 +24,9 @@ void testDestinationWrapping() final ApacheHttpClient5Wrapper sut = new ApacheHttpClient5Wrapper(mock(CloseableHttpClient.class), firstDestination, mock(RequestConfig.class)); - // withDestination returns the same wrapper instance when the destination reference is identical assertThat(sut.withDestination(firstDestination)).isSameAs(sut); + assertThat(sut.withDestination(firstDestination)).isNotSameAs(sut.withDestination(secondDestination)); - // withDestination throws an exception when destinations are not equal (different header providers) - assertThatThrownBy(() -> sut.withDestination(secondDestination)).isInstanceOf(ShouldNotHappenException.class); - - // withDestination throws an exception when destinations have different URIs assertThatThrownBy(() -> sut.withDestination(thirdDestination)).isInstanceOf(ShouldNotHappenException.class); } } diff --git a/cloudplatform/connectivity-destination-service/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/TransparentProxyDestination.java b/cloudplatform/connectivity-destination-service/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/TransparentProxyDestination.java index 085c6829c8..2a2f5889c6 100644 --- a/cloudplatform/connectivity-destination-service/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/TransparentProxyDestination.java +++ b/cloudplatform/connectivity-destination-service/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/TransparentProxyDestination.java @@ -56,6 +56,17 @@ public class TransparentProxyDestination implements HttpDestination static final String CHAIN_VAR_SAML_PROVIDER_DESTINATION_NAME_HEADER_KEY = "x-chain-var-samlProviderDestinationName"; static final String TENANT_ID_AND_TENANT_SUBDOMAIN_BOTH_PASSED_ERROR_MESSAGE = "Tenant id and tenant subdomain cannot be passed at the same time."; + + /** + * Cached header providers from class loading. These are loaded once at class initialization time + * and reused across all destination instances to ensure stable identities for cache key generation. + */ + @Nonnull + private static final ImmutableList CACHED_HEADER_PROVIDERS_FROM_CLASS_LOADING = + ImmutableList. builder() + .addAll(FacadeLocator.getFacades(DestinationHeaderProvider.class)) + .build(); + @Nonnull final ImmutableList
customHeaders; @Delegate @@ -64,8 +75,6 @@ public class TransparentProxyDestination implements HttpDestination @Getter( AccessLevel.PACKAGE ) private final ImmutableList customHeaderProviders; - @Nonnull - private final ImmutableList headerProvidersFromClassLoading; private TransparentProxyDestination( @Nonnull final DestinationProperties baseProperties, @@ -76,10 +85,6 @@ private TransparentProxyDestination( this.customHeaders = customHeaders != null ? ImmutableList.
builder().addAll(customHeaders).build() : ImmutableList.of(); - final Collection headerProvidersFromClassLoading = - FacadeLocator.getFacades(DestinationHeaderProvider.class); - this.headerProvidersFromClassLoading = - ImmutableList. builder().addAll(headerProvidersFromClassLoading).build(); this.customHeaderProviders = customHeaderProviders != null @@ -144,7 +149,7 @@ public Collection
getHeaders( @Nonnull final URI requestUri ) this, requestUri, customHeaderProviders, - headerProvidersFromClassLoading)); + CACHED_HEADER_PROVIDERS_FROM_CLASS_LOADING)); // Automatically add tenant id if not already present TenantAccessor.tryGetCurrentTenant().onSuccess(tenant -> { diff --git a/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/OAuth2ServiceBindingDestinationLoaderTest.java b/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/OAuth2ServiceBindingDestinationLoaderTest.java index 2b05514fbd..c1bbcc063f 100644 --- a/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/OAuth2ServiceBindingDestinationLoaderTest.java +++ b/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/OAuth2ServiceBindingDestinationLoaderTest.java @@ -24,6 +24,8 @@ import java.util.function.Predicate; import org.apache.http.HttpHeaders; +import org.apache.http.client.HttpClient; +import org.apache.http.client.methods.HttpGet; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; @@ -43,11 +45,12 @@ import com.sap.cloud.security.config.ClientIdentity; import io.vavr.control.Try; +import lombok.SneakyThrows; class OAuth2ServiceBindingDestinationLoaderTest { - private static final URI baseUrl = URI.create("baseUrl"); - private static final URI tokenUrl = URI.create("tokenUrl"); + private static final URI baseUrl = URI.create("http://baseUrl"); + private static final URI tokenUrl = URI.create("http://tokenUrl"); public static final ClientIdentity credentials = new ClientCredentials("id", "sec"); private static final ServiceIdentifier TEST_SERVICE = ServiceIdentifier.of("TEST_SERVICE_IDENTIFIER"); @@ -351,6 +354,57 @@ void testErrorHandling() } } + @SneakyThrows + @Test + void testEqualProxiedDestinationsShareHttpClient() + { + final URI proxyUrl = URI.create("http://proxyUrl:1234"); + final DefaultHttpDestination baseDestination = + DefaultHttpDestination.builder(baseUrl).proxyType(ProxyType.ON_PREMISE).buildInternal(); + + final DestinationHeaderProvider headerProviderMock = mock(DestinationHeaderProvider.class); + when(headerProviderMock.getHeaders(any())).thenReturn(Collections.emptyList()); + + sut = spy(new OAuth2ServiceBindingDestinationLoader()); + doReturn(headerProviderMock).when(sut).createHeaderProvider(any(), any(), any(), any(), any(), any()); + + // Create two equal destinations through separate invocations + final HttpDestination dest1 = + sut + .toProxiedDestination( + baseDestination, + proxyUrl, + tokenUrl, + credentials, + OnBehalfOf.TECHNICAL_USER_CURRENT_TENANT, + OAuth2Options.DEFAULT, + TEST_SERVICE); + final HttpDestination dest2 = + sut + .toProxiedDestination( + baseDestination, + proxyUrl, + tokenUrl, + credentials, + OnBehalfOf.TECHNICAL_USER_CURRENT_TENANT, + OAuth2Options.DEFAULT, + TEST_SERVICE); + + // Destinations are equal but different instances + assertThat(dest1).isNotSameAs(dest2).isEqualTo(dest2); + + // Get HTTP clients from cache - they should be the same instance + final DefaultHttpClientCache cache = new DefaultHttpClientCache(5, java.util.concurrent.TimeUnit.MINUTES); + final HttpClient client1 = cache.tryGetHttpClient(dest1, new DefaultHttpClientFactory()).get(); + final HttpClient client2 = cache.tryGetHttpClient(dest2, new DefaultHttpClientFactory()).get(); + + assertThat(client1).isNotSameAs(client2); + + // Closing client1 closes the shared pool, so client2 fails + ((org.apache.http.impl.client.CloseableHttpClient) client1).close(); + client2.execute(new HttpGet()); + } + @Test void testProxiedDestination() { From 134d655fa16864397e89c06e079207af481e6803 Mon Sep 17 00:00:00 2001 From: I538344 Date: Mon, 3 Aug 2026 15:27:46 +0200 Subject: [PATCH 08/17] Wip pair programming --- .../connectivity/DefaultHttpDestination.java | 33 ++++++++----------- .../connectivity/HttpClientWrapper.java | 2 +- .../DefaultHttpClientCacheTest.java | 14 +++++--- .../ApacheHttpClient5Wrapper.java | 2 +- .../TransparentProxyDestination.java | 19 ++++------- 5 files changed, 31 insertions(+), 39 deletions(-) diff --git a/cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java b/cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java index 4f41947527..195e2c7a15 100644 --- a/cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java +++ b/cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java @@ -50,19 +50,6 @@ @Slf4j public final class DefaultHttpDestination implements HttpDestination { - /** - * Cached header providers from class loading. These are loaded once at class initialization time and reused across - * all destination instances to ensure stable identities for cache key generation. This ensures that two - * destinations with identical configurations will have the same header provider instances, enabling proper HTTP - * client caching. - */ - @Nonnull - private static final ImmutableList CACHED_HEADER_PROVIDERS_FROM_CLASS_LOADING = - ImmutableList - . builder() - .addAll(FacadeLocator.getFacades(DestinationHeaderProvider.class)) - .build(); - @Delegate private final DestinationProperties baseProperties; @@ -79,6 +66,9 @@ public final class DefaultHttpDestination implements HttpDestination @Getter( AccessLevel.PACKAGE ) private final ImmutableList customHeaderProviders; + @Nonnull + private final ImmutableList headerProvidersFromClassLoading; + // the following 'cached' fields are ALWAYS derived from the baseProperties and stored in the corresponding fields // to avoid additional computation at runtime ONLY. // this is why we are calling them 'cached'. @@ -87,12 +77,6 @@ public final class DefaultHttpDestination implements HttpDestination // in other words: caching the values is safe and will not lead to any inconsistencies. // furthermore, it is safe to exclude these fields from the equals and hashCode methods because their values are // purely derived from the baseProperties, which are included in the equals and hashCode methods. - // - // NOTE: customHeaderProviders and headerProvidersFromClassLoading are intentionally excluded from equals() and hashCode() - // because header providers are called at request time to generate dynamic headers. Two destinations with identical - // configurations but different provider instances should still share the same HTTP client, as the providers generate - // headers when the client executes the request. By caching header providers globally (see CACHED_HEADER_PROVIDERS_FROM_CLASS_LOADING), - // we ensure that identical destinations will have the same provider instances anyway. @Nonnull private final Option cachedProxyConfiguration; @@ -130,6 +114,11 @@ private DefaultHttpDestination( this.customHeaders = customHeaders != null ? ImmutableList.
builder().addAll(customHeaders).build() : ImmutableList.of(); + final Collection headerProvidersFromClassLoading = + FacadeLocator.getFacades(DestinationHeaderProvider.class); + this.headerProvidersFromClassLoading = + ImmutableList. builder().addAll(headerProvidersFromClassLoading).build(); + this.customHeaderProviders = customHeaderProviders != null ? ImmutableList. builder().addAll(customHeaderProviders).build() @@ -187,7 +176,7 @@ public Collection
getHeaders( @Nonnull final URI requestUri ) this, requestUri, customHeaderProviders, - CACHED_HEADER_PROVIDERS_FROM_CLASS_LOADING)); + headerProvidersFromClassLoading)); allHeaders.addAll(cachedHeadersFromProperties); if( allHeaders.stream().noneMatch(header -> header.getName().equalsIgnoreCase(HttpHeaders.AUTHORIZATION)) ) { allHeaders.addAll(getHeadersForAuthType()); @@ -549,6 +538,8 @@ public boolean equals( @Nullable final Object o ) return new EqualsBuilder() .append(baseProperties, that.baseProperties) .append(customHeaders, that.customHeaders) + .append(customHeaderProviders, that.customHeaderProviders) + .append(headerProvidersFromClassLoading, that.headerProvidersFromClassLoading) .append( resolveCertificatesOnly(keyStoreSupplier.get().getOrNull()), resolveCertificatesOnly(that.keyStoreSupplier.get().getOrNull())) @@ -562,6 +553,8 @@ public int hashCode() return new HashCodeBuilder(17, 37) .append(baseProperties) .append(customHeaders) + .append(customHeaderProviders) + .append(headerProvidersFromClassLoading) .append(resolveKeyStoreHashCode(keyStoreSupplier.get().getOrNull())) .append(resolveKeyStoreHashCode(trustStore)) .toHashCode(); diff --git a/cloudplatform/connectivity-apache-httpclient4/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapper.java b/cloudplatform/connectivity-apache-httpclient4/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapper.java index e1e3739f87..c9c17eedb7 100644 --- a/cloudplatform/connectivity-apache-httpclient4/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapper.java +++ b/cloudplatform/connectivity-apache-httpclient4/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapper.java @@ -96,7 +96,7 @@ HttpClientWrapper withDestination( final HttpDestinationProperties destination ) // explicitly check the reference equality, since equals doesn't check header providers // this is a slight improvement, avoiding unnecessary wrapper instantiation // in cases where destination objects are reused / served from cache - if( !destination.equals(this.destination) ) { + if( !destination.getUri().equals(this.destination.getUri()) ) { throw new ShouldNotHappenException( "This method must not be used outside of updating an instance of HttpClientWrapper for http clients served from the HttpClientCache."); } diff --git a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java index 7705311ddf..37234bd28d 100644 --- a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java +++ b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java @@ -363,15 +363,19 @@ void testInvalidatePrincipalCacheEntriesWithUserTokenExchangeDestination() @Test void testCachedEqualHttpClientsClosingBehavior() { - final DefaultHttpDestination destination1 = DefaultHttpDestination.builder("http://foo.com").build(); - final HttpClient client1 = sut.tryGetHttpClient(destination1, FACTORY).get(); - assertThat(((HttpClientWrapper) client1).getDestination()).isSameAs(destination1); - + final DefaultHttpDestination destination1 = + DefaultHttpDestination + .builder("http://foo.com") + .headerProviders(c -> List.of(new Header("Authorization", "Bearer old"))) + .build(); final DefaultHttpDestination destination2 = DefaultHttpDestination .builder("http://foo.com") - .headerProviders(c -> List.of(new Header("Authorization", "Bearer foo"))) + .headerProviders(c -> List.of(new Header("Authorization", "Bearer new"))) .build(); + + final HttpClient client1 = sut.tryGetHttpClient(destination1, FACTORY).get(); + assertThat(((HttpClientWrapper) client1).getDestination()).isSameAs(destination1); final HttpClient client2 = sut.tryGetHttpClient(destination2, FACTORY).get(); assertThat(((HttpClientWrapper) client2).getDestination()).isSameAs(destination2); diff --git a/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/ApacheHttpClient5Wrapper.java b/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/ApacheHttpClient5Wrapper.java index e7e48783ee..d6a5018dcc 100644 --- a/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/ApacheHttpClient5Wrapper.java +++ b/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/ApacheHttpClient5Wrapper.java @@ -81,7 +81,7 @@ ApacheHttpClient5Wrapper withDestination( final HttpDestinationProperties destin // explicitly check the reference equality, since equals doesn't check header providers // this is a slight improvement, avoiding unnecessary wrapper instantiation // in cases where destination objects are reused / served from cache - if( !destination.equals(this.destination) ) { + if( !destination.getUri().equals(this.destination.getUri()) ) { throw new ShouldNotHappenException( "This method must not be used outside of updating an instance of ApacheHttpClient5Wrapper for http clients served from the ApacheHttpClient5Cache."); } diff --git a/cloudplatform/connectivity-destination-service/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/TransparentProxyDestination.java b/cloudplatform/connectivity-destination-service/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/TransparentProxyDestination.java index 2a2f5889c6..085c6829c8 100644 --- a/cloudplatform/connectivity-destination-service/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/TransparentProxyDestination.java +++ b/cloudplatform/connectivity-destination-service/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/TransparentProxyDestination.java @@ -56,17 +56,6 @@ public class TransparentProxyDestination implements HttpDestination static final String CHAIN_VAR_SAML_PROVIDER_DESTINATION_NAME_HEADER_KEY = "x-chain-var-samlProviderDestinationName"; static final String TENANT_ID_AND_TENANT_SUBDOMAIN_BOTH_PASSED_ERROR_MESSAGE = "Tenant id and tenant subdomain cannot be passed at the same time."; - - /** - * Cached header providers from class loading. These are loaded once at class initialization time - * and reused across all destination instances to ensure stable identities for cache key generation. - */ - @Nonnull - private static final ImmutableList CACHED_HEADER_PROVIDERS_FROM_CLASS_LOADING = - ImmutableList. builder() - .addAll(FacadeLocator.getFacades(DestinationHeaderProvider.class)) - .build(); - @Nonnull final ImmutableList
customHeaders; @Delegate @@ -75,6 +64,8 @@ ImmutableList. builder() @Getter( AccessLevel.PACKAGE ) private final ImmutableList customHeaderProviders; + @Nonnull + private final ImmutableList headerProvidersFromClassLoading; private TransparentProxyDestination( @Nonnull final DestinationProperties baseProperties, @@ -85,6 +76,10 @@ private TransparentProxyDestination( this.customHeaders = customHeaders != null ? ImmutableList.
builder().addAll(customHeaders).build() : ImmutableList.of(); + final Collection headerProvidersFromClassLoading = + FacadeLocator.getFacades(DestinationHeaderProvider.class); + this.headerProvidersFromClassLoading = + ImmutableList. builder().addAll(headerProvidersFromClassLoading).build(); this.customHeaderProviders = customHeaderProviders != null @@ -149,7 +144,7 @@ public Collection
getHeaders( @Nonnull final URI requestUri ) this, requestUri, customHeaderProviders, - CACHED_HEADER_PROVIDERS_FROM_CLASS_LOADING)); + headerProvidersFromClassLoading)); // Automatically add tenant id if not already present TenantAccessor.tryGetCurrentTenant().onSuccess(tenant -> { From 0feaf9cc0215c22494ff25cb2e4a5a7ebc015a19 Mon Sep 17 00:00:00 2001 From: I538344 Date: Mon, 3 Aug 2026 15:54:08 +0200 Subject: [PATCH 09/17] Remove equals assertions from OAuth2ServiceBindingDestinationLoaderTest --- ...h2ServiceBindingDestinationLoaderTest.java | 69 ++----------------- 1 file changed, 4 insertions(+), 65 deletions(-) diff --git a/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/OAuth2ServiceBindingDestinationLoaderTest.java b/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/OAuth2ServiceBindingDestinationLoaderTest.java index c1bbcc063f..117291b703 100644 --- a/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/OAuth2ServiceBindingDestinationLoaderTest.java +++ b/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/OAuth2ServiceBindingDestinationLoaderTest.java @@ -16,7 +16,6 @@ import java.net.URI; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.List; @@ -24,8 +23,6 @@ import java.util.function.Predicate; import org.apache.http.HttpHeaders; -import org.apache.http.client.HttpClient; -import org.apache.http.client.methods.HttpGet; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; @@ -45,12 +42,11 @@ import com.sap.cloud.security.config.ClientIdentity; import io.vavr.control.Try; -import lombok.SneakyThrows; class OAuth2ServiceBindingDestinationLoaderTest { - private static final URI baseUrl = URI.create("http://baseUrl"); - private static final URI tokenUrl = URI.create("http://tokenUrl"); + private static final URI baseUrl = URI.create("baseUrl"); + private static final URI tokenUrl = URI.create("tokenUrl"); public static final ClientIdentity credentials = new ClientCredentials("id", "sec"); private static final ServiceIdentifier TEST_SERVICE = ServiceIdentifier.of("TEST_SERVICE_IDENTIFIER"); @@ -121,7 +117,7 @@ void testOptionsMatcher() .builder() .copy(Collections.emptyMap()) .withServiceIdentifier(TEST_SERVICE) - .withTags(Arrays.asList("test")) + .withTags(List.of("test")) .build(); final ServiceBindingDestinationOptions options = ServiceBindingDestinationOptions.forService(binding).build(); @@ -212,11 +208,8 @@ void testClientSecretBasedBinding() assertThat(sut.tryGetDestination(OPTIONS_WITH_EMPTY_BINDING).get()) .as("The destination should not be cached.") .isNotSameAs(result.get()); - assertThat(sut.tryGetDestination(OPTIONS_WITH_EMPTY_BINDING).get()) - .as("The destination objects should be equal so that they use the same HTTP client.") - .isEqualTo(result.get()); - verify(sut, times(3)) + verify(sut, times(2)) .toDestination( eq(baseUrl), eq(tokenUrl), @@ -354,57 +347,6 @@ void testErrorHandling() } } - @SneakyThrows - @Test - void testEqualProxiedDestinationsShareHttpClient() - { - final URI proxyUrl = URI.create("http://proxyUrl:1234"); - final DefaultHttpDestination baseDestination = - DefaultHttpDestination.builder(baseUrl).proxyType(ProxyType.ON_PREMISE).buildInternal(); - - final DestinationHeaderProvider headerProviderMock = mock(DestinationHeaderProvider.class); - when(headerProviderMock.getHeaders(any())).thenReturn(Collections.emptyList()); - - sut = spy(new OAuth2ServiceBindingDestinationLoader()); - doReturn(headerProviderMock).when(sut).createHeaderProvider(any(), any(), any(), any(), any(), any()); - - // Create two equal destinations through separate invocations - final HttpDestination dest1 = - sut - .toProxiedDestination( - baseDestination, - proxyUrl, - tokenUrl, - credentials, - OnBehalfOf.TECHNICAL_USER_CURRENT_TENANT, - OAuth2Options.DEFAULT, - TEST_SERVICE); - final HttpDestination dest2 = - sut - .toProxiedDestination( - baseDestination, - proxyUrl, - tokenUrl, - credentials, - OnBehalfOf.TECHNICAL_USER_CURRENT_TENANT, - OAuth2Options.DEFAULT, - TEST_SERVICE); - - // Destinations are equal but different instances - assertThat(dest1).isNotSameAs(dest2).isEqualTo(dest2); - - // Get HTTP clients from cache - they should be the same instance - final DefaultHttpClientCache cache = new DefaultHttpClientCache(5, java.util.concurrent.TimeUnit.MINUTES); - final HttpClient client1 = cache.tryGetHttpClient(dest1, new DefaultHttpClientFactory()).get(); - final HttpClient client2 = cache.tryGetHttpClient(dest2, new DefaultHttpClientFactory()).get(); - - assertThat(client1).isNotSameAs(client2); - - // Closing client1 closes the shared pool, so client2 fails - ((org.apache.http.impl.client.CloseableHttpClient) client1).close(); - client2.execute(new HttpGet()); - } - @Test void testProxiedDestination() { @@ -450,9 +392,6 @@ void testProxiedDestination() assertThat(secondInvocationResult) .as("There should not be a cache in place for proxied destinations.") .isNotSameAs(result); - assertThat(secondInvocationResult) - .as("The destination objects should be equal so that they use the same HTTP client.") - .isEqualTo(result); verify(sut, times(2)) .createHeaderProvider( From 93ee4da04250137f5746ddeef5f0be9799815841 Mon Sep 17 00:00:00 2001 From: I538344 Date: Tue, 4 Aug 2026 09:39:43 +0200 Subject: [PATCH 10/17] =?UTF-8?q?=E2=9C=85?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ...omputeSingleDestinationCommandWithoutAllDestinationsTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/cloudplatform/connectivity-destination-service/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/GetOrComputeSingleDestinationCommandWithoutAllDestinationsTest.java b/cloudplatform/connectivity-destination-service/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/GetOrComputeSingleDestinationCommandWithoutAllDestinationsTest.java index 0ea3bfa147..31e4342e64 100644 --- a/cloudplatform/connectivity-destination-service/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/GetOrComputeSingleDestinationCommandWithoutAllDestinationsTest.java +++ b/cloudplatform/connectivity-destination-service/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/GetOrComputeSingleDestinationCommandWithoutAllDestinationsTest.java @@ -261,7 +261,6 @@ private void runTest( TestCase testCase ) softly.fail("Expected command execution to fail, but it succeeded."); } - softly.assertThat(maybeDestination.get()).isEqualTo(testCase.getExpectedDestination()); // sanity checks no cache was hit if( testCase.getTokenExchangeStrategy() == DestinationServiceTokenExchangeStrategy.LOOKUP_THEN_EXCHANGE && DestinationUtility.requiresUserTokenExchange(testCase.getExpectedDestination()) ) { From cdb02560d21194e9293417c7e7b48f0344aff903 Mon Sep 17 00:00:00 2001 From: I538344 Date: Tue, 4 Aug 2026 11:32:05 +0200 Subject: [PATCH 11/17] verify fails --- .../connectivity/HttpClientWrapper.java | 3 - .../DefaultHttpClientCacheTest.java | 64 ++++++++++++++++++- .../ApacheHttpClient5Wrapper.java | 3 - 3 files changed, 61 insertions(+), 9 deletions(-) diff --git a/cloudplatform/connectivity-apache-httpclient4/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapper.java b/cloudplatform/connectivity-apache-httpclient4/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapper.java index c9c17eedb7..e4b074087c 100644 --- a/cloudplatform/connectivity-apache-httpclient4/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapper.java +++ b/cloudplatform/connectivity-apache-httpclient4/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapper.java @@ -93,9 +93,6 @@ public String toString() HttpClientWrapper withDestination( final HttpDestinationProperties destination ) { - // explicitly check the reference equality, since equals doesn't check header providers - // this is a slight improvement, avoiding unnecessary wrapper instantiation - // in cases where destination objects are reused / served from cache if( !destination.getUri().equals(this.destination.getUri()) ) { throw new ShouldNotHappenException( "This method must not be used outside of updating an instance of HttpClientWrapper for http clients served from the HttpClientCache."); diff --git a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java index 37234bd28d..8fbe73e9cd 100644 --- a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java +++ b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java @@ -1,5 +1,13 @@ package com.sap.cloud.sdk.cloudplatform.connectivity; +import static com.github.tomakehurst.wiremock.client.WireMock.anyUrl; +import static com.github.tomakehurst.wiremock.client.WireMock.equalTo; +import static com.github.tomakehurst.wiremock.client.WireMock.get; +import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor; +import static com.github.tomakehurst.wiremock.client.WireMock.ok; +import static com.github.tomakehurst.wiremock.client.WireMock.stubFor; +import static com.github.tomakehurst.wiremock.client.WireMock.verify; +import static com.github.tomakehurst.wiremock.stubbing.Scenario.STARTED; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -12,9 +20,11 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; +import com.github.tomakehurst.wiremock.junit5.WireMockTest; import org.apache.http.client.HttpClient; import org.apache.http.client.methods.HttpGet; import org.apache.http.client.methods.HttpUriRequest; +import org.jspecify.annotations.NonNull; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; @@ -31,6 +41,7 @@ import lombok.SneakyThrows; @Isolated +@WireMockTest class DefaultHttpClientCacheTest { private static final HttpDestination DESTINATION = DefaultHttpDestination.builder("https://url1").build(); @@ -359,18 +370,22 @@ void testInvalidatePrincipalCacheEntriesWithUserTokenExchangeDestination() assertThat(unclearedClientWithoutDestination).isSameAs(sut.tryGetHttpClient(FACTORY).get()); } - @SneakyThrows @Test + @SneakyThrows void testCachedEqualHttpClientsClosingBehavior() { + String url = "https://url1"; + + stubFor(get(url).willReturn(ok())); + final DefaultHttpDestination destination1 = DefaultHttpDestination - .builder("http://foo.com") + .builder(url) .headerProviders(c -> List.of(new Header("Authorization", "Bearer old"))) .build(); final DefaultHttpDestination destination2 = DefaultHttpDestination - .builder("http://foo.com") + .builder(url) .headerProviders(c -> List.of(new Header("Authorization", "Bearer new"))) .build(); @@ -397,6 +412,49 @@ void testCachedEqualHttpClientsClosingBehavior() // since client1 did not inherit client2 connection manager, client2 is not shut down client2.execute(new HttpGet()); + verify(1, getRequestedFor(anyUrl())); + } + + @Test + @SneakyThrows + void testCachedDestinationIsReused() + { + stubFor( + get(anyUrl()) + .withHeader("Authorization", equalTo("Bearer token1")) + .inScenario("Refreshing token") + .whenScenarioStateIs(STARTED) + .willReturn(ok()) + .willSetStateTo("First token sent")); + stubFor( + get(anyUrl()) + .withHeader("Authorization", equalTo("Bearer token2")) + .inScenario("Refreshing token") + .whenScenarioStateIs("First token sent") + .willReturn(ok())); + + final DefaultHttpDestination destination = + DefaultHttpDestination.builder("https://url1").headerProviders(c -> getHeaders()).build(); + + // token1 is sent + final HttpClient client1 = sut.tryGetHttpClient(destination, FACTORY).get(); + client1.execute(new HttpGet()); + + // token2 is sent, and, since the destination is cached, the same client is reused + final HttpClient client2 = sut.tryGetHttpClient(destination, FACTORY).get(); + client2.execute(new HttpGet()); + + assertThat(client1).isSameAs(client2); + + verify(1, getRequestedFor(anyUrl()).withHeader("Authorization", equalTo("Bearer token1"))); + verify(1, getRequestedFor(anyUrl()).withHeader("Authorization", equalTo("Bearer token2"))); + } + + private int count = 1; + + private @NonNull List
getHeaders() + { + return List.of(new Header("Authorization", "Bearer token" + count++)); } @Test diff --git a/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/ApacheHttpClient5Wrapper.java b/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/ApacheHttpClient5Wrapper.java index d6a5018dcc..5a16c1ac39 100644 --- a/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/ApacheHttpClient5Wrapper.java +++ b/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/ApacheHttpClient5Wrapper.java @@ -78,9 +78,6 @@ public void close( final CloseMode closeMode ) ApacheHttpClient5Wrapper withDestination( final HttpDestinationProperties destination ) { - // explicitly check the reference equality, since equals doesn't check header providers - // this is a slight improvement, avoiding unnecessary wrapper instantiation - // in cases where destination objects are reused / served from cache if( !destination.getUri().equals(this.destination.getUri()) ) { throw new ShouldNotHappenException( "This method must not be used outside of updating an instance of ApacheHttpClient5Wrapper for http clients served from the ApacheHttpClient5Cache."); From 0d4f3a2be67edaac9301f579ef55f0404a76d49b Mon Sep 17 00:00:00 2001 From: I538344 Date: Tue, 4 Aug 2026 11:37:40 +0200 Subject: [PATCH 12/17] verify --- .../DefaultHttpClientCacheTest.java | 54 ++++++++++--------- 1 file changed, 28 insertions(+), 26 deletions(-) diff --git a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java index 8fbe73e9cd..966951299d 100644 --- a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java +++ b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java @@ -5,8 +5,7 @@ import static com.github.tomakehurst.wiremock.client.WireMock.get; import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor; import static com.github.tomakehurst.wiremock.client.WireMock.ok; -import static com.github.tomakehurst.wiremock.client.WireMock.stubFor; -import static com.github.tomakehurst.wiremock.client.WireMock.verify; +import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig; import static com.github.tomakehurst.wiremock.stubbing.Scenario.STARTED; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -20,7 +19,7 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; -import com.github.tomakehurst.wiremock.junit5.WireMockTest; +import com.github.tomakehurst.wiremock.junit5.WireMockExtension; import org.apache.http.client.HttpClient; import org.apache.http.client.methods.HttpGet; import org.apache.http.client.methods.HttpUriRequest; @@ -41,9 +40,12 @@ import lombok.SneakyThrows; @Isolated -@WireMockTest class DefaultHttpClientCacheTest { + @RegisterExtension + static final WireMockExtension WIRE_MOCK_SERVER = + WireMockExtension.newInstance().options(wireMockConfig().dynamicPort()).build(); + private static final HttpDestination DESTINATION = DefaultHttpDestination.builder("https://url1").build(); private static final DefaultHttpDestination USER_TOKEN_EXCHANGE_DESTINATION = DefaultHttpDestination @@ -374,18 +376,16 @@ void testInvalidatePrincipalCacheEntriesWithUserTokenExchangeDestination() @SneakyThrows void testCachedEqualHttpClientsClosingBehavior() { - String url = "https://url1"; - - stubFor(get(url).willReturn(ok())); + WIRE_MOCK_SERVER.stubFor(get(anyUrl()).willReturn(ok())); final DefaultHttpDestination destination1 = DefaultHttpDestination - .builder(url) + .builder(WIRE_MOCK_SERVER.baseUrl()) .headerProviders(c -> List.of(new Header("Authorization", "Bearer old"))) .build(); final DefaultHttpDestination destination2 = DefaultHttpDestination - .builder(url) + .builder(WIRE_MOCK_SERVER.baseUrl()) .headerProviders(c -> List.of(new Header("Authorization", "Bearer new"))) .build(); @@ -412,29 +412,31 @@ void testCachedEqualHttpClientsClosingBehavior() // since client1 did not inherit client2 connection manager, client2 is not shut down client2.execute(new HttpGet()); - verify(1, getRequestedFor(anyUrl())); + WIRE_MOCK_SERVER.verify(1, getRequestedFor(anyUrl())); } @Test @SneakyThrows void testCachedDestinationIsReused() { - stubFor( - get(anyUrl()) - .withHeader("Authorization", equalTo("Bearer token1")) - .inScenario("Refreshing token") - .whenScenarioStateIs(STARTED) - .willReturn(ok()) - .willSetStateTo("First token sent")); - stubFor( - get(anyUrl()) - .withHeader("Authorization", equalTo("Bearer token2")) - .inScenario("Refreshing token") - .whenScenarioStateIs("First token sent") - .willReturn(ok())); + WIRE_MOCK_SERVER + .stubFor( + get(anyUrl()) + .withHeader("Authorization", equalTo("Bearer token1")) + .inScenario("Refreshing token") + .whenScenarioStateIs(STARTED) + .willReturn(ok()) + .willSetStateTo("First token sent")); + WIRE_MOCK_SERVER + .stubFor( + get(anyUrl()) + .withHeader("Authorization", equalTo("Bearer token2")) + .inScenario("Refreshing token") + .whenScenarioStateIs("First token sent") + .willReturn(ok())); final DefaultHttpDestination destination = - DefaultHttpDestination.builder("https://url1").headerProviders(c -> getHeaders()).build(); + DefaultHttpDestination.builder(WIRE_MOCK_SERVER.baseUrl()).headerProviders(c -> getHeaders()).build(); // token1 is sent final HttpClient client1 = sut.tryGetHttpClient(destination, FACTORY).get(); @@ -446,8 +448,8 @@ void testCachedDestinationIsReused() assertThat(client1).isSameAs(client2); - verify(1, getRequestedFor(anyUrl()).withHeader("Authorization", equalTo("Bearer token1"))); - verify(1, getRequestedFor(anyUrl()).withHeader("Authorization", equalTo("Bearer token2"))); + WIRE_MOCK_SERVER.verify(1, getRequestedFor(anyUrl()).withHeader("Authorization", equalTo("Bearer token1"))); + WIRE_MOCK_SERVER.verify(1, getRequestedFor(anyUrl()).withHeader("Authorization", equalTo("Bearer token2"))); } private int count = 1; From c7ee5d8699ecfdd482421dd3891c3f8db8ca45a7 Mon Sep 17 00:00:00 2001 From: I538344 Date: Tue, 4 Aug 2026 12:44:45 +0200 Subject: [PATCH 13/17] duplicate tests for apache 5 --- .../DefaultHttpClientCacheTest.java | 8 +- .../DefaultApacheHttpClient5CacheTest.java | 131 ++++++++++++------ release_notes.md | 4 +- 3 files changed, 98 insertions(+), 45 deletions(-) diff --git a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java index 966951299d..797de531c9 100644 --- a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java +++ b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java @@ -441,15 +441,15 @@ void testCachedDestinationIsReused() // token1 is sent final HttpClient client1 = sut.tryGetHttpClient(destination, FACTORY).get(); client1.execute(new HttpGet()); + WIRE_MOCK_SERVER.verify(1, getRequestedFor(anyUrl()).withHeader("Authorization", equalTo("Bearer token1"))); - // token2 is sent, and, since the destination is cached, the same client is reused + // token2 is sent final HttpClient client2 = sut.tryGetHttpClient(destination, FACTORY).get(); client2.execute(new HttpGet()); + WIRE_MOCK_SERVER.verify(1, getRequestedFor(anyUrl()).withHeader("Authorization", equalTo("Bearer token2"))); + // Because the destination is cached, the same client is reused assertThat(client1).isSameAs(client2); - - WIRE_MOCK_SERVER.verify(1, getRequestedFor(anyUrl()).withHeader("Authorization", equalTo("Bearer token1"))); - WIRE_MOCK_SERVER.verify(1, getRequestedFor(anyUrl()).withHeader("Authorization", equalTo("Bearer token2"))); } private int count = 1; diff --git a/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5CacheTest.java b/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5CacheTest.java index 7295ec9d96..170d043a14 100644 --- a/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5CacheTest.java +++ b/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5CacheTest.java @@ -1,21 +1,29 @@ package com.sap.cloud.sdk.cloudplatform.connectivity; +import static com.github.tomakehurst.wiremock.client.WireMock.anyUrl; +import static com.github.tomakehurst.wiremock.client.WireMock.equalTo; +import static com.github.tomakehurst.wiremock.client.WireMock.get; +import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor; +import static com.github.tomakehurst.wiremock.client.WireMock.ok; +import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig; +import static com.github.tomakehurst.wiremock.stubbing.Scenario.STARTED; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import java.time.Duration; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Set; import java.util.concurrent.atomic.AtomicLong; +import com.github.tomakehurst.wiremock.junit5.WireMockExtension; +import lombok.SneakyThrows; import org.apache.hc.client5.http.classic.HttpClient; import org.apache.hc.client5.http.classic.methods.HttpGet; -import org.apache.hc.core5.http.ClassicHttpRequest; -import org.apache.hc.core5.http.message.BasicHeader; +import org.apache.hc.client5.http.impl.classic.BasicHttpClientResponseHandler; +import org.jspecify.annotations.NonNull; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; @@ -30,6 +38,10 @@ class DefaultApacheHttpClient5CacheTest { + @RegisterExtension + static final WireMockExtension WIRE_MOCK_SERVER = + WireMockExtension.newInstance().options(wireMockConfig().dynamicPort()).build(); + private static final List TENANTS = Arrays.asList(new DefaultTenant("tenant#1"), new DefaultTenant("tenant#2"), null); private static final List PRINCIPALS = @@ -348,49 +360,90 @@ void testInvalidatePrincipalCacheEntriesWithUserTokenExchangeDestination() } @Test - //This is a known limitation of excluding header providers in the equality check of destinations - void testGetClientReturnsSameClientForDestinationsWithOnlyDifferentHeaderProviders() + @SneakyThrows + void testCachedEqualHttpClientsClosingBehavior() { - final Header header1 = new Header("foo", "bar"); - final Header header2 = new Header("foo1", "bar1"); + WIRE_MOCK_SERVER.stubFor(get(anyUrl()).willReturn(ok())); - final DefaultHttpDestination firstDestination = + final DefaultHttpDestination destination1 = DefaultHttpDestination - .builder("http://some-uri") - .headerProviders(( any ) -> Collections.singletonList(header1)) + .builder(WIRE_MOCK_SERVER.baseUrl()) + .headerProviders(c -> List.of(new Header("Authorization", "Bearer old"))) .build(); - - final DefaultHttpDestination secondDestination = + final DefaultHttpDestination destination2 = DefaultHttpDestination - .fromDestination(firstDestination) - .headerProviders(( any ) -> Collections.singletonList(header2)) + .builder(WIRE_MOCK_SERVER.baseUrl()) + .headerProviders(c -> List.of(new Header("Authorization", "Bearer new"))) .build(); - final ApacheHttpClient5Wrapper client1 = - (ApacheHttpClient5Wrapper) sut.tryGetHttpClient(firstDestination, FACTORY).get(); - final ApacheHttpClient5Wrapper client2 = - (ApacheHttpClient5Wrapper) sut.tryGetHttpClient(secondDestination, FACTORY).get(); - - assertThat(client1.getDestination()).isSameAs(firstDestination); - assertThat(client2.getDestination()).isSameAs(secondDestination); - - final ClassicHttpRequest request1 = client1.wrapRequest(new HttpGet("/")); - final ClassicHttpRequest request2 = client2.wrapRequest(new HttpGet("/")); - - final List headersRequest1 = new ArrayList<>(); - final List headersRequest2 = new ArrayList<>(); - request1.headerIterator().forEachRemaining(headersRequest1::add); - request2.headerIterator().forEachRemaining(headersRequest2::add); - - // recursive comparison because BasicHeader doesn't implement equals/hashCode - assertThat(headersRequest1) - .usingRecursiveFieldByFieldElementComparator() - .containsExactly(new BasicHeader(header1.getName(), header1.getValue())); - assertThat(headersRequest2) - .usingRecursiveFieldByFieldElementComparator() - .containsExactly( - new BasicHeader(header1.getName(), header1.getValue()), - new BasicHeader(header2.getName(), header2.getValue())); + final HttpClient client1 = sut.tryGetHttpClient(destination1, FACTORY).get(); + assertThat(((ApacheHttpClient5Wrapper) client1).getDestination()).isSameAs(destination1); + final HttpClient client2 = sut.tryGetHttpClient(destination2, FACTORY).get(); + assertThat(((ApacheHttpClient5Wrapper) client2).getDestination()).isSameAs(destination2); + + // Note: Destinations are now equal even with different header providers, since header providers + // are not part of the equality check. However, they are different instances, so they result in + // different HTTP clients due to the cache key being based on instance identity. + assertThat(destination1).isNotSameAs(destination2); + + // Http clients are distinct instances, since the cache key contains the destination reference and not its content + assertThat(client1).isNotSameAs(client2); + + // When using the exact same destination object, the same http-client wrapper should be returned + final HttpClient client1Again = sut.tryGetHttpClient(destination1, FACTORY).get(); + assertThat(client1Again).isSameAs(client1); + assertThat(((ApacheHttpClient5Wrapper) client1Again).getDestination()).isSameAs(destination1); + + // simulate garbage collection on client1 + ((ApacheHttpClient5Wrapper) client1).close(); + + // since client1 did not inherit client2 connection manager, client2 is not shut down + client2.execute(new HttpGet("/"), new BasicHttpClientResponseHandler()); + WIRE_MOCK_SERVER.verify(1, getRequestedFor(anyUrl())); + } + + @Test + @SneakyThrows + void testCachedDestinationIsReused() + { + WIRE_MOCK_SERVER + .stubFor( + get(anyUrl()) + .withHeader("Authorization", equalTo("Bearer token1")) + .inScenario("Refreshing token") + .whenScenarioStateIs(STARTED) + .willReturn(ok()) + .willSetStateTo("First token sent")); + WIRE_MOCK_SERVER + .stubFor( + get(anyUrl()) + .withHeader("Authorization", equalTo("Bearer token2")) + .inScenario("Refreshing token") + .whenScenarioStateIs("First token sent") + .willReturn(ok())); + + final DefaultHttpDestination destination = + DefaultHttpDestination.builder(WIRE_MOCK_SERVER.baseUrl()).headerProviders(c -> getHeaders()).build(); + + // token1 is sent + final HttpClient client1 = sut.tryGetHttpClient(destination, FACTORY).get(); + client1.execute(new HttpGet("/"), new BasicHttpClientResponseHandler()); + WIRE_MOCK_SERVER.verify(1, getRequestedFor(anyUrl()).withHeader("Authorization", equalTo("Bearer token1"))); + + // token2 is sent + final HttpClient client2 = sut.tryGetHttpClient(destination, FACTORY).get(); + client2.execute(new HttpGet("/"), new BasicHttpClientResponseHandler()); + WIRE_MOCK_SERVER.verify(1, getRequestedFor(anyUrl()).withHeader("Authorization", equalTo("Bearer token2"))); + + // Because the destination is cached, the same client is reused + assertThat(client1).isSameAs(client2); + } + + private int count = 1; + + private @NonNull List
getHeaders() + { + return List.of(new Header("Authorization", "Bearer token" + count++)); } @Test diff --git a/release_notes.md b/release_notes.md index 8fb3414093..e5b00e5e55 100644 --- a/release_notes.md +++ b/release_notes.md @@ -8,7 +8,7 @@ ### 🔧 Compatibility Notes -- +- [Connectivity] Destination objects with different header providers are now considered not equal. ### ✨ New Functionality @@ -16,7 +16,7 @@ ### 📈 Improvements -- +- [Connectivity] Fixed `Connection pool shut down` edge-case when closing a client with a destination that has a custom header provider. ### 🐛 Fixed Issues From 68051dcebb7435298ed702678cd41caf5328bd69 Mon Sep 17 00:00:00 2001 From: I538344 Date: Tue, 4 Aug 2026 13:08:07 +0200 Subject: [PATCH 14/17] more precise equals --- .../connectivity/DefaultHttpDestination.java | 55 +++++++++++++------ 1 file changed, 37 insertions(+), 18 deletions(-) diff --git a/cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java b/cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java index 195e2c7a15..784533c26f 100644 --- a/cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java +++ b/cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java @@ -19,6 +19,7 @@ import javax.annotation.Nullable; import javax.net.ssl.SSLContext; +import lombok.val; import org.apache.commons.lang3.builder.EqualsBuilder; import org.apache.commons.lang3.builder.HashCodeBuilder; @@ -535,29 +536,47 @@ public boolean equals( @Nullable final Object o ) } final DefaultHttpDestination that = (DefaultHttpDestination) o; - return new EqualsBuilder() - .append(baseProperties, that.baseProperties) - .append(customHeaders, that.customHeaders) - .append(customHeaderProviders, that.customHeaderProviders) - .append(headerProvidersFromClassLoading, that.headerProvidersFromClassLoading) - .append( - resolveCertificatesOnly(keyStoreSupplier.get().getOrNull()), - resolveCertificatesOnly(that.keyStoreSupplier.get().getOrNull())) - .append(resolveCertificatesOnly(trustStore), resolveCertificatesOnly(that.trustStore)) - .isEquals(); + + if( headerProvidersFromClassLoading.size() != that.headerProvidersFromClassLoading.size() + || customHeaderProviders.size() != that.customHeaderProviders.size() ) { + return false; + } + + val builder = + new EqualsBuilder() + .append(baseProperties, that.baseProperties) + .append(customHeaders, that.customHeaders) + .append( + resolveCertificatesOnly(keyStoreSupplier.get().getOrNull()), + resolveCertificatesOnly(that.keyStoreSupplier.get().getOrNull())) + .append(resolveCertificatesOnly(trustStore), resolveCertificatesOnly(that.trustStore)); + + customHeaderProviders + .forEach( + provider -> builder + .append(provider, that.customHeaderProviders.get(customHeaderProviders.indexOf(provider)))); + headerProvidersFromClassLoading + .forEach( + provider -> builder + .append( + provider, + that.headerProvidersFromClassLoading.get(headerProvidersFromClassLoading.indexOf(provider)))); + return builder.isEquals(); } @Override public int hashCode() { - return new HashCodeBuilder(17, 37) - .append(baseProperties) - .append(customHeaders) - .append(customHeaderProviders) - .append(headerProvidersFromClassLoading) - .append(resolveKeyStoreHashCode(keyStoreSupplier.get().getOrNull())) - .append(resolveKeyStoreHashCode(trustStore)) - .toHashCode(); + val builder = + new HashCodeBuilder(17, 37) + .append(baseProperties) + .append(customHeaders) + .append(resolveKeyStoreHashCode(keyStoreSupplier.get().getOrNull())) + .append(resolveKeyStoreHashCode(trustStore)); + + customHeaderProviders.forEach(builder::append); + headerProvidersFromClassLoading.forEach(builder::append); + return builder.toHashCode(); } /** From 6e96667e5fd9c476312aacd6bd37a5d866c92aba Mon Sep 17 00:00:00 2001 From: I538344 Date: Tue, 4 Aug 2026 14:42:38 +0200 Subject: [PATCH 15/17] cache HeaderProvidersFromClassLoading --- .../connectivity/DefaultHttpDestination.java | 38 +++++++++++++++++-- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java b/cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java index 784533c26f..465529cf6d 100644 --- a/cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java +++ b/cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java @@ -67,6 +67,14 @@ public final class DefaultHttpDestination implements HttpDestination @Getter( AccessLevel.PACKAGE ) private final ImmutableList customHeaderProviders; + /** + * Lazily initialized and cached header providers loaded via FacadeLocator. + * This ensures the same instances are shared across all DefaultHttpDestination instances. + * Uses volatile to ensure visibility of changes across threads. + */ + @Nullable + private static volatile ImmutableList cachedHeaderProvidersFromClassLoading; + @Nonnull private final ImmutableList headerProvidersFromClassLoading; @@ -115,10 +123,7 @@ private DefaultHttpDestination( this.customHeaders = customHeaders != null ? ImmutableList.
builder().addAll(customHeaders).build() : ImmutableList.of(); - final Collection headerProvidersFromClassLoading = - FacadeLocator.getFacades(DestinationHeaderProvider.class); - this.headerProvidersFromClassLoading = - ImmutableList. builder().addAll(headerProvidersFromClassLoading).build(); + this.headerProvidersFromClassLoading = getCachedHeaderProvidersFromClassLoading(); this.customHeaderProviders = customHeaderProviders != null @@ -145,6 +150,31 @@ private DefaultHttpDestination( .build(); } + /** + * Lazily initializes and returns the cached header providers from class loading. + * Uses double-checked locking to ensure thread-safe lazy initialization while minimizing synchronization overhead. + * + * @return The immutable list of header providers loaded via FacadeLocator. + */ + @Nonnull + private static ImmutableList getCachedHeaderProvidersFromClassLoading() + { + ImmutableList cached = cachedHeaderProvidersFromClassLoading; + if( cached == null ) { + synchronized (DefaultHttpDestination.class) { + cached = cachedHeaderProvidersFromClassLoading; + if( cached == null ) { + cached = ImmutableList + . builder() + .addAll(FacadeLocator.getFacades(DestinationHeaderProvider.class)) + .build(); + cachedHeaderProvidersFromClassLoading = cached; + } + } + } + return cached; + } + /** * Verifies that the given "generic" destination might be decorated into a {@code DefaultHttpDestination}. * From c4ac0c6e1da3b002dd7b9b0a5c43659f970a0bc7 Mon Sep 17 00:00:00 2001 From: I538344 Date: Tue, 4 Aug 2026 14:48:32 +0200 Subject: [PATCH 16/17] formatting --- .../connectivity/DefaultHttpDestination.java | 22 +++++++++---------- .../DefaultHttpClientCacheTest.java | 5 ++--- .../DefaultApacheHttpClient5CacheTest.java | 8 +++---- 3 files changed, 17 insertions(+), 18 deletions(-) diff --git a/cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java b/cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java index 465529cf6d..3d68e80ae6 100644 --- a/cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java +++ b/cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java @@ -19,7 +19,6 @@ import javax.annotation.Nullable; import javax.net.ssl.SSLContext; -import lombok.val; import org.apache.commons.lang3.builder.EqualsBuilder; import org.apache.commons.lang3.builder.HashCodeBuilder; @@ -44,6 +43,7 @@ import lombok.experimental.Accessors; import lombok.experimental.Delegate; import lombok.extern.slf4j.Slf4j; +import lombok.val; /** * Immutable default implementation of the {@link HttpDestination} interface. @@ -68,9 +68,8 @@ public final class DefaultHttpDestination implements HttpDestination private final ImmutableList customHeaderProviders; /** - * Lazily initialized and cached header providers loaded via FacadeLocator. - * This ensures the same instances are shared across all DefaultHttpDestination instances. - * Uses volatile to ensure visibility of changes across threads. + * Lazily initialized and cached header providers loaded via FacadeLocator. This ensures the same instances are + * shared across all DefaultHttpDestination instances. Uses volatile to ensure visibility of changes across threads. */ @Nullable private static volatile ImmutableList cachedHeaderProvidersFromClassLoading; @@ -151,8 +150,8 @@ private DefaultHttpDestination( } /** - * Lazily initializes and returns the cached header providers from class loading. - * Uses double-checked locking to ensure thread-safe lazy initialization while minimizing synchronization overhead. + * Lazily initializes and returns the cached header providers from class loading. Uses double-checked locking to + * ensure thread-safe lazy initialization while minimizing synchronization overhead. * * @return The immutable list of header providers loaded via FacadeLocator. */ @@ -161,13 +160,14 @@ private static ImmutableList getCachedHeaderProviders { ImmutableList cached = cachedHeaderProvidersFromClassLoading; if( cached == null ) { - synchronized (DefaultHttpDestination.class) { + synchronized( DefaultHttpDestination.class ) { cached = cachedHeaderProvidersFromClassLoading; if( cached == null ) { - cached = ImmutableList - . builder() - .addAll(FacadeLocator.getFacades(DestinationHeaderProvider.class)) - .build(); + cached = + ImmutableList + . builder() + .addAll(FacadeLocator.getFacades(DestinationHeaderProvider.class)) + .build(); cachedHeaderProvidersFromClassLoading = cached; } } diff --git a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java index 797de531c9..88b01ccb4b 100644 --- a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java +++ b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java @@ -19,16 +19,15 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; -import com.github.tomakehurst.wiremock.junit5.WireMockExtension; import org.apache.http.client.HttpClient; import org.apache.http.client.methods.HttpGet; import org.apache.http.client.methods.HttpUriRequest; -import org.jspecify.annotations.NonNull; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; import org.junit.jupiter.api.parallel.Isolated; +import com.github.tomakehurst.wiremock.junit5.WireMockExtension; import com.sap.cloud.sdk.cloudplatform.cache.CacheManager; import com.sap.cloud.sdk.cloudplatform.connectivity.exception.HttpClientInstantiationException; import com.sap.cloud.sdk.cloudplatform.security.principal.DefaultPrincipal; @@ -454,7 +453,7 @@ void testCachedDestinationIsReused() private int count = 1; - private @NonNull List
getHeaders() + private List
getHeaders() { return List.of(new Header("Authorization", "Bearer token" + count++)); } diff --git a/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5CacheTest.java b/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5CacheTest.java index 170d043a14..d2f2327c6c 100644 --- a/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5CacheTest.java +++ b/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5CacheTest.java @@ -18,16 +18,14 @@ import java.util.Set; import java.util.concurrent.atomic.AtomicLong; -import com.github.tomakehurst.wiremock.junit5.WireMockExtension; -import lombok.SneakyThrows; import org.apache.hc.client5.http.classic.HttpClient; import org.apache.hc.client5.http.classic.methods.HttpGet; import org.apache.hc.client5.http.impl.classic.BasicHttpClientResponseHandler; -import org.jspecify.annotations.NonNull; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; +import com.github.tomakehurst.wiremock.junit5.WireMockExtension; import com.sap.cloud.sdk.cloudplatform.cache.CacheManager; import com.sap.cloud.sdk.cloudplatform.connectivity.exception.HttpClientInstantiationException; import com.sap.cloud.sdk.cloudplatform.security.principal.DefaultPrincipal; @@ -36,6 +34,8 @@ import com.sap.cloud.sdk.cloudplatform.tenant.Tenant; import com.sap.cloud.sdk.testutil.TestContext; +import lombok.SneakyThrows; + class DefaultApacheHttpClient5CacheTest { @RegisterExtension @@ -441,7 +441,7 @@ void testCachedDestinationIsReused() private int count = 1; - private @NonNull List
getHeaders() + private List
getHeaders() { return List.of(new Header("Authorization", "Bearer token" + count++)); } From acadead23eb86afca7053ef00f0e4c8973b1db44 Mon Sep 17 00:00:00 2001 From: I538344 Date: Tue, 4 Aug 2026 14:54:56 +0200 Subject: [PATCH 17/17] comments --- .../connectivity/DefaultHttpClientCacheTest.java | 8 +------- .../connectivity/DefaultApacheHttpClient5CacheTest.java | 8 +------- 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java index 88b01ccb4b..ecfd747d03 100644 --- a/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java +++ b/cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java @@ -393,18 +393,12 @@ void testCachedEqualHttpClientsClosingBehavior() final HttpClient client2 = sut.tryGetHttpClient(destination2, FACTORY).get(); assertThat(((HttpClientWrapper) client2).getDestination()).isSameAs(destination2); - // Note: Destinations are now equal even with different header providers, since header providers - // are not part of the equality check. However, they are different instances, so they result in - // different HTTP clients due to the cache key being based on instance identity. - assertThat(destination1).isNotSameAs(destination2); - - // Http clients are distinct instances, since the cache key contains the destination reference and not its content assertThat(client1).isNotSameAs(client2); // When using the exact same destination object, the same http-client wrapper should be returned final HttpClient client1Again = sut.tryGetHttpClient(destination1, FACTORY).get(); - assertThat(client1Again).isSameAs(client1); assertThat(((HttpClientWrapper) client1Again).getDestination()).isSameAs(destination1); + assertThat(client1Again).isSameAs(client1); // simulate garbage collection on client1 ((HttpClientWrapper) client1).close(); diff --git a/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5CacheTest.java b/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5CacheTest.java index d2f2327c6c..7c4d09f369 100644 --- a/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5CacheTest.java +++ b/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5CacheTest.java @@ -381,18 +381,12 @@ void testCachedEqualHttpClientsClosingBehavior() final HttpClient client2 = sut.tryGetHttpClient(destination2, FACTORY).get(); assertThat(((ApacheHttpClient5Wrapper) client2).getDestination()).isSameAs(destination2); - // Note: Destinations are now equal even with different header providers, since header providers - // are not part of the equality check. However, they are different instances, so they result in - // different HTTP clients due to the cache key being based on instance identity. - assertThat(destination1).isNotSameAs(destination2); - - // Http clients are distinct instances, since the cache key contains the destination reference and not its content assertThat(client1).isNotSameAs(client2); // When using the exact same destination object, the same http-client wrapper should be returned final HttpClient client1Again = sut.tryGetHttpClient(destination1, FACTORY).get(); - assertThat(client1Again).isSameAs(client1); assertThat(((ApacheHttpClient5Wrapper) client1Again).getDestination()).isSameAs(destination1); + assertThat(client1Again).isSameAs(client1); // simulate garbage collection on client1 ((ApacheHttpClient5Wrapper) client1).close();