Skip to content

fix: Improve Circuit breaker behaviour #1017

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
Jonas-Isr merged 23 commits into from
Dec 16, 2025
Merged

fix: Improve Circuit breaker behaviour #1017

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
b43e388
feat: Circuit Breaker thing
CharlesDuboisSAP Nov 26, 2025
87aab24
tests green
Jonas-Isr Nov 26, 2025
0ec0870
add feature switch
Jonas-Isr Nov 27, 2025
7b0e572
add tests for feature
Jonas-Isr Nov 27, 2025
077b784
remove other changes from tests
Jonas-Isr Nov 27, 2025
75e300b
code style
Jonas-Isr Nov 27, 2025
5475777
PMD
Jonas-Isr Nov 27, 2025
2501904
move feature switch
Jonas-Isr Nov 28, 2025
31924b3
codestyle
Jonas-Isr Nov 28, 2025
4ab435e
codestyle
Jonas-Isr Dec 1, 2025
a3b144f
formatting
Jonas-Isr Dec 2, 2025
689acb8
reviews
Jonas-Isr Dec 2, 2025
624c10a
javadoc
Jonas-Isr Dec 2, 2025
3e05498
Merge branch 'main' into circuit-breaker
Jonas-Isr Dec 3, 2025
d573e51
codestyle
Jonas-Isr Dec 3, 2025
6cf6d0f
release notes
Jonas-Isr Dec 3, 2025
d8f9139
PMD
Jonas-Isr Dec 3, 2025
d62b970
reviews + enable by default
Jonas-Isr Dec 8, 2025
5ba5226
reviews
Jonas-Isr Dec 8, 2025
0bf62f6
reviews
Jonas-Isr Dec 9, 2025
7b6e46e
reviews
Jonas-Isr Dec 10, 2025
1bc4f01
Merge branch 'main' into circuit-breaker
Jonas-Isr Dec 10, 2025
2aef26b
codestyle
Jonas-Isr Dec 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion ...ervice/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import lombok.Getter;
import lombok.NoArgsConstructor;
import lombok.RequiredArgsConstructor;
import lombok.Setter;
import lombok.extern.slf4j.Slf4j;

/**
Expand Down Expand Up @@ -426,6 +427,8 @@ public static final class Cache

private static boolean cacheEnabled = true;
private static boolean changeDetectionEnabled = true;
@Setter
private static boolean getAllDocumentsPrepended = false;

static {
recreateSingleCache();
Expand All @@ -443,6 +446,11 @@ static boolean isChangeDetectionEnabled()
return changeDetectionEnabled;
}

static boolean isGetAllDocumentsPrepended()
Comment thread
Jonas-Isr marked this conversation as resolved.
Outdated
{
return getAllDocumentsPrepended;
}

@Nonnull
static com.github.benmanes.caffeine.cache.Cache<CacheKey, Destination> instanceSingle()
{
Expand Down Expand Up @@ -496,6 +504,7 @@ static void reset()
cacheEnabled = true;
}
changeDetectionEnabled = true;
getAllDocumentsPrepended = false;

sizeLimit = Option.some(DEFAULT_SIZE_LIMIT);
expirationDuration = Option.some(DEFAULT_EXPIRATION_DURATION);
Expand Down Expand Up @@ -866,7 +875,8 @@ private static Try<Destination> getOrComputeDestination(
instanceSingle(),
isolationLocks(),
destinationDownloader,
getAllCommand);
getAllCommand,
getAllDocumentsPrepended);
return command.flatMap(GetOrComputeSingleDestinationCommand::execute);
}

Expand Down
27 changes: 25 additions & 2 deletions ...va/com/sap/cloud/sdk/cloudplatform/connectivity/GetOrComputeSingleDestinationCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import lombok.Getter;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import lombok.val;

@Slf4j
@RequiredArgsConstructor( access = AccessLevel.PRIVATE )
Expand All @@ -52,6 +53,8 @@ class GetOrComputeSingleDestinationCommand
private final DestinationServiceTokenExchangeStrategy exchangeStrategy;
@Nullable
private final GetOrComputeAllDestinationsCommand getAllCommand;
@Nonnull
private final Boolean prependGetAllDestinationCall;

@SuppressWarnings( "deprecation" )
static Try<GetOrComputeSingleDestinationCommand> prepareCommand(
Expand All @@ -60,7 +63,8 @@ static Try<GetOrComputeSingleDestinationCommand> prepareCommand(
@Nonnull final Cache<CacheKey, Destination> destinationCache,
@Nonnull final Cache<CacheKey, ReentrantLock> isolationLocks,
@Nonnull final BiFunction<String, DestinationOptions, Destination> destinationRetriever,
@Nullable final GetOrComputeAllDestinationsCommand getAllCommand )
@Nullable final GetOrComputeAllDestinationsCommand getAllCommand,
@Nonnull final Boolean prependGetAllDestinationCall )
{
final Supplier<Destination> destinationSupplier =
() -> destinationRetriever.apply(destinationName, destinationOptions);
Expand Down Expand Up @@ -109,7 +113,8 @@ static Try<GetOrComputeSingleDestinationCommand> prepareCommand(
destinationCache,
destinationSupplier,
exchangeStrategy,
getAllCommand));
getAllCommand,
prependGetAllDestinationCall));
}

/**
Expand Down Expand Up @@ -149,6 +154,10 @@ Try<Destination> execute()
if( result != null ) {
return Try.success(result);
}
if( prependGetAllDestinationCall && !destinationExists() ) {
final String msg = "Destination %s was not found among the destinations of the current tenant.";
return Try.failure(new DestinationAccessException(String.format(msg, destinationName)));
}

@newtork newtork Dec 2, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Question/Minor)

Not a huge fan of Boolean (sic) prependGetAllDestinationCall additional 7th argument.
Have you considered wrapping this check into destinationRetriever argument before calling the method? If it's not too complex/too much additional code (likely).

@Jonas-Isr Jonas-Isr Dec 2, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Boolean was a typo, should of course have been boolean 😬

I struggled a bit with placing this into destinationRetriever but found another solution by putting it directly into DestinationService.tryGetDestination. Is there anything that speaks against this?


final Try<Destination> maybeResult = Try.ofSupplier(destinationSupplier);
if( maybeResult.isFailure() ) {
Expand Down Expand Up @@ -263,6 +272,20 @@ private Destination getCachedDestination()
return maybeDestination;
}

private boolean destinationExists()
{
if( getAllCommand != null ) {
val allDestinations = getAllCommand.execute();
if( allDestinations != null && allDestinations.isSuccess() ) {
return allDestinations
.get()
.stream()
.anyMatch(properties -> properties.get(DestinationProperty.NAME).contains(destinationName));
}
}
return false;
}

/**
* currently this effectively limits the cache duration to the change detection interval, because change detection
* on certificates isn't possible in all cases See the note on {@link DestinationServiceFactory} where this property
Expand Down
4 changes: 2 additions & 2 deletions ...va/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationServiceAuthenticationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ void testOAuth2PasswordWithoutUserToken()
assertThat(dest.asHttp().getHeaders())
.containsExactlyInAnyOrder(new Header("Authorization", "Bearer " + oAuthToken));

verify(mockAdapter, times(1)).getConfigurationAsJson(anyString(), eq(expectedStrategy));
verify(mockAdapter, times(1)).getConfigurationAsJson(eq(SERVICE_PATH_DESTINATION), eq(expectedStrategy));
verifyNoMoreInteractions(mockAdapter);
}

Expand Down Expand Up @@ -382,7 +382,7 @@ void testSAPAssertionSSO()
assertThat(dest.asHttp().getAuthenticationType()).isEqualTo(AuthenticationType.SAP_ASSERTION_SSO);
assertThat(dest.asHttp().getHeaders()).containsExactlyInAnyOrder(new Header("Cookie", assertionCookie));

verify(mockAdapter, times(1)).getConfigurationAsJson(anyString(), eq(expectedStrategy));
verify(mockAdapter, times(1)).getConfigurationAsJson(eq(SERVICE_PATH_DESTINATION), eq(expectedStrategy));
verifyNoMoreInteractions(mockAdapter);
}

Expand Down
71 changes: 66 additions & 5 deletions ...ce/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,15 @@ class DestinationServiceTest
"ProxyType": "Internet",
"KeyStorePassword": "password",
"KeyStoreLocation": "aaa"
},
{
"Name": "SomeDestinationName",
"Type": "HTTP",
"URL": "https://a.s4hana.ondemand.com",
"Authentication": "ClientCertificateAuthentication",
"ProxyType": "Internet",
"KeyStorePassword": "password",
"KeyStoreLocation": "aaa"
}]
""";

Expand Down Expand Up @@ -442,7 +451,7 @@ void testGettingDestinationProperties()

assertThat(destinationList)
.extracting(d -> d.get(DestinationProperty.NAME).get())
.containsExactly("CC8-HTTP-BASIC", "CC8-HTTP-CERT1", "CC8-HTTP-CERT");
.containsExactly("CC8-HTTP-BASIC", "CC8-HTTP-CERT1", "CC8-HTTP-CERT", destinationName);

final DestinationProperties destination =
destinationList
Expand Down Expand Up @@ -478,7 +487,7 @@ void testGettingDestinationPropertiesProvider()
final Collection<DestinationProperties> destinationList = loader.getAllDestinationProperties(ALWAYS_PROVIDER);
assertThat(destinationList)
.extracting(d -> d.get(DestinationProperty.NAME).get())
.containsExactly("CC8-HTTP-BASIC", "CC8-HTTP-CERT1", "CC8-HTTP-CERT");
.containsExactly("CC8-HTTP-BASIC", "CC8-HTTP-CERT1", "CC8-HTTP-CERT", destinationName);
}

@Test
Expand All @@ -494,7 +503,7 @@ void testGettingDestinationPropertiesSubscriber()
final Collection<DestinationProperties> destinationList = loader.getAllDestinationProperties(ONLY_SUBSCRIBER);
assertThat(destinationList)
.extracting(d -> d.get(DestinationProperty.NAME).get())
.containsExactly("CC8-HTTP-BASIC", "CC8-HTTP-CERT1", "CC8-HTTP-CERT");
.containsExactly("CC8-HTTP-BASIC", "CC8-HTTP-CERT1", "CC8-HTTP-CERT", destinationName);
}

@Test
Expand Down Expand Up @@ -589,10 +598,10 @@ void testGetAllDestinationsForProvider()
final List<Destination> destinationList = new ArrayList<>();
destinations.get().forEach(destinationList::add);

assertThat(destinationList.size()).isEqualTo(3);
assertThat(destinationList.size()).isEqualTo(4);
assertThat(destinationList)
.extracting(d -> d.get(DestinationProperty.NAME).get())
.containsOnly("CC8-HTTP-BASIC", "CC8-HTTP-CERT", "CC8-HTTP-CERT1");
.containsOnly("CC8-HTTP-BASIC", "CC8-HTTP-CERT", "CC8-HTTP-CERT1", destinationName);
}

@SuppressWarnings( "deprecation" )
Expand Down Expand Up @@ -1910,4 +1919,56 @@ private String createHttpDestinationServiceResponse( final String name, final St
}
""", name, url);
}

@Test
void testPrependGetAllDestinationsCall()
{
doReturn(responseServiceInstanceDestination)
.when(destinationServiceAdapter)
.getConfigurationAsJson(eq("/v1/instanceDestinations"), any());
doReturn(responseSubaccountDestination)
.when(destinationServiceAdapter)
.getConfigurationAsJson(eq("/v1/subaccountDestinations"), any());

DestinationService.Cache.setGetAllDocumentsPrepended(true);

@newtork newtork Dec 2, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Major)

Please consider different API options, to reflect on existing available methods.

- DestinationService.Cache.setGetAllDocumentsPrepended(true);

+ DestinationService.Cache.enableGetAllDocumentsPrepended();
+ DestinationService.Cache.enableGetAllDestinationsPrepended();
+ DestinationService.Cache.enablePreLookupCheck();
+ DestinationService.Cache.enableExistenceCheck();

Also your feature would be enabled by default - right?

@Jonas-Isr Jonas-Isr Dec 2, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed it to DestinationService.Cache.enablePreLookupCheck().

Right now I have so that the behaviour is disabled by default. I did that to not introduce breaking changes (and also we talked about that this might increase response times for the first call). Do you think it should be enabled by default?


final DestinationOptions options =
DestinationOptions.builder().augmentBuilder(augmenter().retrievalStrategy(ALWAYS_PROVIDER)).build();
Destination result = loader.tryGetDestination(destinationName).get();

// verify all results are cached
verify(destinationServiceAdapter, times(1)).getConfigurationAsJson(eq("/v1/instanceDestinations"), any());
verify(destinationServiceAdapter, times(1)).getConfigurationAsJson(eq("/v1/subaccountDestinations"), any());
verify(destinationServiceAdapter, times(1))
.getConfigurationAsJson(eq("/v1/destinations/" + destinationName), any());
verifyNoMoreInteractions(destinationServiceAdapter);

DestinationService.Cache.setGetAllDocumentsPrepended(false);
}

@Test
void testPrependGetAllDestinationsCallWithMissingDestination()
{
doReturn(responseServiceInstanceDestination)
.when(destinationServiceAdapter)
.getConfigurationAsJson(eq("/v1/instanceDestinations"), any());
doReturn(responseSubaccountDestination)
.when(destinationServiceAdapter)
.getConfigurationAsJson(eq("/v1/subaccountDestinations"), any());

DestinationService.Cache.setGetAllDocumentsPrepended(true);

final DestinationOptions options =
DestinationOptions.builder().augmentBuilder(augmenter().retrievalStrategy(ALWAYS_PROVIDER)).build();
assertThatThrownBy(() -> loader.tryGetDestination("thisDestinationDoesNotExist").get())
.isInstanceOf(DestinationAccessException.class)
.hasMessageContaining("was not found among the destinations of the current tenant.");

// verify all results are cached
verify(destinationServiceAdapter, times(1)).getConfigurationAsJson(eq("/v1/instanceDestinations"), any());
verify(destinationServiceAdapter, times(1)).getConfigurationAsJson(eq("/v1/subaccountDestinations"), any());
verifyNoMoreInteractions(destinationServiceAdapter);

DestinationService.Cache.setGetAllDocumentsPrepended(false);
}
}
42 changes: 31 additions & 11 deletions ...java/com/sap/cloud/sdk/cloudplatform/connectivity/GetOrComputeDestinationCommandTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,14 @@ void testDefaultTokenExchangeStrategy()

final GetOrComputeSingleDestinationCommand sut =
GetOrComputeSingleDestinationCommand
.prepareCommand("destination", options, destinationCache, isolationLocks, ( foo, bar ) -> null, null)
.prepareCommand(
"destination",
options,
destinationCache,
isolationLocks,
( foo, bar ) -> null,
null,
false)
.get();

assertThat(sut.getExchangeStrategy()).isEqualTo(DestinationServiceTokenExchangeStrategy.LOOKUP_THEN_EXCHANGE);
Expand Down Expand Up @@ -158,7 +165,7 @@ private void assertCacheKeysMatchExchangeStrategy(

final GetOrComputeSingleDestinationCommand sut =
GetOrComputeSingleDestinationCommand
.prepareCommand(DESTINATION_NAME, options, destinationCache, isolationLocks, function, null)
.prepareCommand(DESTINATION_NAME, options, destinationCache, isolationLocks, function, null, false)
.get();

assertThat(sut.getCacheKey()).isEqualTo(expectedCacheKey);
Expand Down Expand Up @@ -198,7 +205,7 @@ void testLookupOnlyOnUserTokenDestination()

final GetOrComputeSingleDestinationCommand sut =
GetOrComputeSingleDestinationCommand
.prepareCommand(DESTINATION_NAME, options, destinationCache, isolationLocks, function, null)
.prepareCommand(DESTINATION_NAME, options, destinationCache, isolationLocks, function, null, false)
.get();

final Try<Destination> fetchedDestination = sut.execute();
Expand Down Expand Up @@ -236,7 +243,7 @@ void testLookupOnlyExchangeStrategy()

final GetOrComputeSingleDestinationCommand sut =
GetOrComputeSingleDestinationCommand
.prepareCommand(DESTINATION_NAME, options, destinationCache, isolationLocks, function, null)
.prepareCommand(DESTINATION_NAME, options, destinationCache, isolationLocks, function, null, false)
.get();

final Try<Destination> fetchedDestination = sut.execute();
Expand Down Expand Up @@ -288,7 +295,8 @@ void testLookupThenExchangeStrategy()
destinationCache,
isolationLocks,
function,
null)
null,
false)
.get()));

final Try<Destination> fetchedDestination = sut.execute();
Expand Down Expand Up @@ -342,7 +350,8 @@ void testForwardUserTokenStrategyForUserPropagationDestination()
destinationCache,
isolationLocks,
function,
null)
null,
false)
.get()));

final Try<Destination> fetchedDestination =
Expand Down Expand Up @@ -393,7 +402,8 @@ void testForwardUserTokenStrategyForUserPropagationDestinationWithoutJwt()
destinationCache,
isolationLocks,
function,
null)
null,
false)
.get()));

final Try<Destination> fetchedDestination = sut.execute();
Expand Down Expand Up @@ -437,7 +447,14 @@ void testForwardUserTokenStrategyForUserPropagationDestinationWithoutPrincipalUn
.executeWithTenant(
t1,
() -> GetOrComputeSingleDestinationCommand
.prepareCommand(DESTINATION_NAME, options, destinationCache, isolationLocks, function, null)
.prepareCommand(
DESTINATION_NAME,
options,
destinationCache,
isolationLocks,
function,
null,
false)
.get());

final Try<Destination> shouldBeFailure = TenantAccessor.executeWithTenant(t1, sut::execute);
Expand Down Expand Up @@ -487,7 +504,8 @@ void testForwardUserTokenStrategyForClientCredentialsDestination()
destinationCache,
isolationLocks,
function,
null)
null,
false)
.get()));

final Try<Destination> fetchedDestination = PrincipalAccessor.executeWithPrincipal(p1, () -> sut.execute());
Expand Down Expand Up @@ -542,7 +560,8 @@ void testChangeDetectionOnValidAuthToken()
destinationCache,
isolationLocks,
destinationRetriever,
getAllCommand)
getAllCommand,
false)
.flatMap(GetOrComputeSingleDestinationCommand::execute);

assertThat(result.isSuccess()).isTrue();
Expand Down Expand Up @@ -627,7 +646,8 @@ void testFirstDestinationAccessDoesNotTriggerGetAllRequest()
destinationCache,
isolationLocks,
destinationRetriever,
getAllMock)
getAllMock,
false)
.get();
final Try<Destination> result = sut.execute();

Expand Down
3 changes: 2 additions & 1 deletion ...platform/connectivity/GetOrComputeSingleDestinationCommandWithoutAllDestinationsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,8 @@ private void runTest( TestCase testCase )
destinationCache,
isolationLocks,
destinationService::loadAndParseDestination,
null);
null,
false);

if( maybeCommand.isFailure() ) {
if( testCase.isCommandCreationExpectedToSucceed() ) {
Expand Down