fix: Fix caching in getOrComputeAllDestinationsCommand (#1055)

Co-authored-by: Alexander Dümont <[email protected]>

Author: Jonas-Isr

cloudplatform/connectivity-destination-service/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationService.java

@@ -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;
@@ -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(
@@ -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" };
+            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.");

cloudplatform/connectivity-destination-service/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/GetOrComputeAllDestinationsCommand.java

@@ -41,7 +41,7 @@ static GetOrComputeAllDestinationsCommand prepareCommand(
 
         final CacheKey cacheKey = CacheKey.ofTenantOptionalIsolation();
 
-        cacheKey.append(destinationOptions);
+        cacheKey.append(DestinationServiceOptionsAugmenter.getRetrievalStrategy(destinationOptions));
 
         final ReentrantLock isolationLock =
             Objects.requireNonNull(isolationLocks.get(cacheKey, any -> new ReentrantLock()));

cloudplatform/connectivity-destination-service/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationServiceTest.java

@@ -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())
@@ -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(
@@ -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(

cloudplatform/connectivity-destination-service/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/GetOrComputeAllDestinationsCommandTest.java

@@ -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();
     }
@@ -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 -> {