Skip to content

fix: Fix caching in getOrComputeAllDestinationsCommand #1055

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
newtork merged 6 commits into from
Jan 9, 2026
Merged

fix: Fix caching in getOrComputeAllDestinationsCommand #1055

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
202f37e
apply fix
Jonas-Isr Jan 5, 2026
f04cf0b
adapt tests
Jonas-Isr Jan 5, 2026
bf81047
add warn log
Jonas-Isr Jan 7, 2026
da49da1
Merge branch 'main' into fix-caching-getOrComputeAllDestinations
Jonas-Isr Jan 8, 2026
87dd83d
improve warn messages
Jonas-Isr Jan 8, 2026
64dab3f
Minor fix
newtork Jan 9, 2026
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
20 changes: 20 additions & 0 deletions ...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 @@ -2,10 +2,12 @@

import java.time.Duration;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.locks.ReentrantLock;
import java.util.function.BiFunction;
import java.util.function.Function;
Expand Down Expand Up @@ -847,6 +849,11 @@ private static Try<Destination> getOrComputeDestination(
@Nullable
final GetOrComputeAllDestinationsCommand getAllCommand;
if( changeDetectionEnabled ) {
if( isUsingExperimentalFeatures(options) ) {
final String msg =
"Using change detection together with either fragments, cross-level options, or custom headers is discouraged and might lead to unexpected caching behaviour.";
log.warn(msg);
}
getAllCommand =
GetOrComputeAllDestinationsCommand
.prepareCommand(
Expand Down Expand Up @@ -878,11 +885,24 @@ private static Try<List<DestinationProperties>> getOrComputeAllDestinations(
return destinationDownloader.apply(options);
}

if( isUsingExperimentalFeatures(options) ) {
final String msg =
"Using caching together with either fragments, cross-level options, or custom headers is discouraged and might lead to unexpected caching behaviour.";
log.warn(msg);
}

return GetOrComputeAllDestinationsCommand
.prepareCommand(options, instanceAll(), isolationLocks(), destinationDownloader)
.execute();
}

private static boolean isUsingExperimentalFeatures( @Nonnull final DestinationOptions options )
{
final String[] featureNames = { "X-fragment-name", "crossLevelSetting", "customHeader" };
Copy link
Contributor

@newtork newtork Jan 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

(Minor/Comment)

Not an ideal solution in my opinion.
But effective enough to keep the discussion going.

final Set<String> keys = options.getOptionKeys();
return keys.stream().anyMatch(s -> Arrays.stream(featureNames).anyMatch(s::equalsIgnoreCase));
}

private Cache()
{
throw new IllegalStateException("This static class must never be instantiated.");
Expand Down
2 changes: 1 addition & 1 deletion ...java/com/sap/cloud/sdk/cloudplatform/connectivity/GetOrComputeAllDestinationsCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ static GetOrComputeAllDestinationsCommand prepareCommand(

final CacheKey cacheKey = CacheKey.ofTenantOptionalIsolation();

cacheKey.append(destinationOptions);
cacheKey.append(DestinationServiceOptionsAugmenter.getRetrievalStrategy(destinationOptions));
Copy link
Contributor

@newtork newtork Jan 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

(Minor/Suggestion)

Overall suggested change looks good! However I think we can improve code quality if we required retrieval-strategy instead of destination-options.

It looks like we could easily replace

- Function<DestinationOptions, Try<List<DestinationProperties>>> destinationRetriever
+ Function<DestinationServiceRetrievalStrategy, Try<List<DestinationProperties>>> destinationRetriever

Copy link
Contributor

@newtork newtork Jan 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

(Comment/Question)

Can we check implications regarding fragmentName(String) and crossLevelConsumption(CrossLevelScope)?

Copy link
Contributor

@newtork newtork Jan 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I see following problem example:

DestinationService loader;

final DestinationOptions optionsInstance =
  DestinationOptions
    .builder()
    .augmentBuilder(
      DestinationServiceOptionsAugmenter
        .augmenter()
        .crossLevelConsumption(DestinationServiceOptionsAugmenter.CrossLevelScope.PROVIDER_SUBACCOUNT))
    .build();

// get-all-destinations to CURRENT_TENANT | get-single-destination to ALWAYS_PROVIDER
Try<Destination> destination = loader.tryGetDestination(destinationName, optionsSubaccount);

get-all-destinations (for caching) may run on behalf of a different tenant than get-single-destination (lookup)

Copy link
Contributor

@newtork newtork Jan 6, 2026

Choose a reason for hiding this comment

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

Maybe we need some kind of indicator that disables destination caching for specific destination-option-parameters:

  • DESTINATION_RETRIEVAL_STRATEGY_KEY (obvious)
  • CROSS_LEVEL_SETTING_KEY (implied above)
  • X_FRAGMENT_KEY (rather not cache when fragments are involved?)
  • CUSTOM_HEADER_KEY (rather not cache when custom headers for destination retrieval are involved?)
See DestinationServiceOptionsAugmenter image

Copy link
Member

@MatKuhr MatKuhr Jan 6, 2026

Choose a reason for hiding this comment

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

Yeah, these beta features such as fragment and cross level will the break change detection and I think they have javadoc that instruct to turn off change detection for them. Similarly, if we add "not found" check we should probably actively check for these. While fragments shouldn't affect the not found check, the cross level scope likely will. In the custom headers, users can also explicitly pass tenant information, so for full safety we should check for those too (compare with API hub for the header details)

Copy link
Member Author

@Jonas-Isr Jonas-Isr Jan 7, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for catching this!

How should we go about this? We could
1) add a comment in javadoc of those features,
2) log a warning if preLookupCheck is enabled and any of the features are used,
3) throw if we detect preLookupCheck is enabled and any of the features are used?

(Discussion moved to Slack.)


final ReentrantLock isolationLock =
Objects.requireNonNull(isolationLocks.get(cacheKey, any -> new ReentrantLock()));
Expand Down
16 changes: 12 additions & 4 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 @@ -1255,9 +1255,13 @@ void testConcurrentFetchSameDestinationButDifferentTenant()
assertThat(DestinationService.Cache.isolationLocks().asMap())
.containsOnlyKeys(
CacheKey.fromIds("TenantA", null).append(destinationName, options),
CacheKey.fromIds("TenantA", null).append(options),
CacheKey
.fromIds("TenantA", null)
.append(DestinationServiceOptionsAugmenter.getRetrievalStrategy(options)),
CacheKey.fromIds("TenantB", null).append(destinationName, options),
CacheKey.fromIds("TenantB", null).append(options));
CacheKey
.fromIds("TenantB", null)
.append(DestinationServiceOptionsAugmenter.getRetrievalStrategy(options)));

// assert cache entries for one get-single command for each tenant
assertThat(DestinationService.Cache.instanceSingle().asMap())
Expand Down Expand Up @@ -1327,7 +1331,9 @@ void testPrincipalIsolationForDestinationWithUserPropagationWithExchangeOnlyStra
.containsOnlyKeys(
CacheKey.of(subscriberTenant, principal1).append(destinationName, options),
CacheKey.of(subscriberTenant, principal2).append(destinationName, options),
CacheKey.of(subscriberTenant, null).append(options));
CacheKey
.of(subscriberTenant, null)
.append(DestinationServiceOptionsAugmenter.getRetrievalStrategy(options)));

assertThat(DestinationService.Cache.instanceSingle().asMap())
.containsOnlyKeys(
Expand Down Expand Up @@ -1371,7 +1377,9 @@ void testPrincipalIsolationForDestinationWithUserPropagationWithDefaultExchangeS
assertThat(DestinationService.Cache.isolationLocks().asMap())
.containsOnlyKeys(
CacheKey.of(subscriberTenant, null).append(destinationName, options),
CacheKey.of(subscriberTenant, null).append(options));
CacheKey
.of(subscriberTenant, null)
.append(DestinationServiceOptionsAugmenter.getRetrievalStrategy(options)));

assertThat(DestinationService.Cache.instanceSingle().asMap())
.containsOnlyKeys(
Expand Down
13 changes: 10 additions & 3 deletions .../com/sap/cloud/sdk/cloudplatform/connectivity/GetOrComputeAllDestinationsCommandTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,12 @@ void testCommandIsIdempotent()
assertThat(result.get()).containsExactly(destination);

assertThat(allDestinationsCache.estimatedSize()).isEqualTo(1);
assertThat(allDestinationsCache.getIfPresent(CacheKey.ofNoIsolation().append(EMPTY_OPTIONS)))
assertThat(
allDestinationsCache
.getIfPresent(
CacheKey
.ofNoIsolation()
.append(DestinationServiceOptionsAugmenter.getRetrievalStrategy(EMPTY_OPTIONS))))
.containsExactly(destination);
verify(tryGetAllDestinations, times(1)).get();
}
Expand All @@ -104,8 +109,10 @@ void testIsolationAndAtomicityPerTenant()
final CountDownLatch mainThreadLatch = new CountDownLatch(1);
final CountDownLatch getAllLatch = new CountDownLatch(1);
final AtomicInteger lockInvocations = new AtomicInteger();
final CacheKey t1Key = CacheKey.of(t1, null).append(EMPTY_OPTIONS);
final CacheKey t2Key = CacheKey.of(t2, null).append(EMPTY_OPTIONS);
final CacheKey t1Key =
CacheKey.of(t1, null).append(DestinationServiceOptionsAugmenter.getRetrievalStrategy(EMPTY_OPTIONS));
final CacheKey t2Key =
CacheKey.of(t2, null).append(DestinationServiceOptionsAugmenter.getRetrievalStrategy(EMPTY_OPTIONS));
final ReentrantLock tenantIsolationLock = spy(ReentrantLock.class);

doAnswer(invocation -> {
Expand Down