Skip to content

Commit 689acb8

Browse files
Jonas-IsrJonas-Isr
committed
reviews
1 parent a3b144f commit 689acb8

5 files changed

Lines changed: 45 additions & 70 deletions

File tree

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

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,8 @@
4141
import lombok.Getter;
4242
import lombok.NoArgsConstructor;
4343
import lombok.RequiredArgsConstructor;
44-
import lombok.Setter;
4544
import lombok.extern.slf4j.Slf4j;
45+
import lombok.val;
4646

4747
/**
4848
* Retrieves destination information from the SCP destination service on Cloud Foundry.
@@ -121,6 +121,10 @@ static ResilienceConfiguration createResilienceConfiguration(
121121
Try<Destination>
122122
tryGetDestination( @Nonnull final String destinationName, @Nonnull final DestinationOptions options )
123123
{
124+
if( Cache.preLookupCheckEnabled && !preLookupCheckSuccessful(destinationName) ) {
125+
final String msg = "Destination %s was not found among the destinations of the current tenant.";
126+
return Try.failure(new DestinationAccessException(String.format(msg, destinationName)));
127+
}
124128
return Cache.getOrComputeDestination(this, destinationName, options, this::loadAndParseDestination);
125129
}
126130

@@ -385,6 +389,17 @@ private static boolean hasCauseAssignableFrom( @Nonnull final Throwable t, @Nonn
385389
return ExceptionUtils.getThrowableList(t).stream().map(Throwable::getClass).anyMatch(cls::isAssignableFrom);
386390
}
387391

392+
private boolean preLookupCheckSuccessful( String destinationName )
393+
{
394+
val allDestinations = getAllDestinationProperties();
395+
if( !allDestinations.isEmpty() ) {
396+
return allDestinations
397+
.stream()
398+
.anyMatch(properties -> properties.get(DestinationProperty.NAME).contains(destinationName));
399+
}
400+
return false;
401+
}
402+
388403
/**
389404
* Helper class that encapsulates all caching related configuration options.
390405
*
@@ -427,8 +442,8 @@ public static final class Cache
427442

428443
private static boolean cacheEnabled = true;
429444
private static boolean changeDetectionEnabled = true;
430-
@Setter
431-
private static boolean getAllDocumentsPrepended = false;
445+
@Getter( AccessLevel.PACKAGE )
446+
private static boolean preLookupCheckEnabled = false;
432447

433448
static {
434449
recreateSingleCache();
@@ -446,9 +461,14 @@ static boolean isChangeDetectionEnabled()
446461
return changeDetectionEnabled;
447462
}
448463

449-
static boolean isGetAllDocumentsPrepended()
464+
public static void enablePreLookupCheck()
465+
{
466+
preLookupCheckEnabled = true;
467+
}
468+
469+
public static void disablePreLookupCheck()
450470
{
451-
return getAllDocumentsPrepended;
471+
preLookupCheckEnabled = false;
452472
}
453473

454474
@Nonnull
@@ -504,7 +524,7 @@ static void reset()
504524
cacheEnabled = true;
505525
}
506526
changeDetectionEnabled = true;
507-
getAllDocumentsPrepended = false;
527+
preLookupCheckEnabled = false;
508528

509529
sizeLimit = Option.some(DEFAULT_SIZE_LIMIT);
510530
expirationDuration = Option.some(DEFAULT_EXPIRATION_DURATION);
@@ -875,8 +895,7 @@ private static Try<Destination> getOrComputeDestination(
875895
instanceSingle(),
876896
isolationLocks(),
877897
destinationDownloader,
878-
getAllCommand,
879-
getAllDocumentsPrepended);
898+
getAllCommand);
880899
return command.flatMap(GetOrComputeSingleDestinationCommand::execute);
881900
}
882901

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

Lines changed: 2 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626
import lombok.Getter;
2727
import lombok.RequiredArgsConstructor;
2828
import lombok.extern.slf4j.Slf4j;
29-
import lombok.val;
3029

3130
@Slf4j
3231
@RequiredArgsConstructor( access = AccessLevel.PRIVATE )
@@ -53,8 +52,6 @@ class GetOrComputeSingleDestinationCommand
5352
private final DestinationServiceTokenExchangeStrategy exchangeStrategy;
5453
@Nullable
5554
private final GetOrComputeAllDestinationsCommand getAllCommand;
56-
@Nonnull
57-
private final Boolean prependGetAllDestinationCall;
5855

5956
@SuppressWarnings( "deprecation" )
6057
static Try<GetOrComputeSingleDestinationCommand> prepareCommand(
@@ -63,8 +60,7 @@ static Try<GetOrComputeSingleDestinationCommand> prepareCommand(
6360
@Nonnull final Cache<CacheKey, Destination> destinationCache,
6461
@Nonnull final Cache<CacheKey, ReentrantLock> isolationLocks,
6562
@Nonnull final BiFunction<String, DestinationOptions, Destination> destinationRetriever,
66-
@Nullable final GetOrComputeAllDestinationsCommand getAllCommand,
67-
@Nonnull final Boolean prependGetAllDestinationCall )
63+
@Nullable final GetOrComputeAllDestinationsCommand getAllCommand )
6864
{
6965
final Supplier<Destination> destinationSupplier =
7066
() -> destinationRetriever.apply(destinationName, destinationOptions);
@@ -113,8 +109,7 @@ static Try<GetOrComputeSingleDestinationCommand> prepareCommand(
113109
destinationCache,
114110
destinationSupplier,
115111
exchangeStrategy,
116-
getAllCommand,
117-
prependGetAllDestinationCall));
112+
getAllCommand));
118113
}
119114

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

162153
final Try<Destination> maybeResult = Try.ofSupplier(destinationSupplier);
163154
if( maybeResult.isFailure() ) {
@@ -272,20 +263,6 @@ private Destination getCachedDestination()
272263
return maybeDestination;
273264
}
274265

275-
private boolean destinationExists()
276-
{
277-
if( getAllCommand != null ) {
278-
val allDestinations = getAllCommand.execute();
279-
if( allDestinations != null && allDestinations.isSuccess() ) {
280-
return allDestinations
281-
.get()
282-
.stream()
283-
.anyMatch(properties -> properties.get(DestinationProperty.NAME).contains(destinationName));
284-
}
285-
}
286-
return false;
287-
}
288-
289266
/**
290267
* currently this effectively limits the cache duration to the change detection interval, because change detection
291268
* on certificates isn't possible in all cases See the note on {@link DestinationServiceFactory} where this property

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1930,7 +1930,7 @@ void testPrependGetAllDestinationsCall()
19301930
.when(destinationServiceAdapter)
19311931
.getConfigurationAsJson(eq("/v1/subaccountDestinations"), any());
19321932

1933-
DestinationService.Cache.setGetAllDocumentsPrepended(true);
1933+
DestinationService.Cache.enablePreLookupCheck();
19341934

19351935
final DestinationOptions options =
19361936
DestinationOptions.builder().augmentBuilder(augmenter().retrievalStrategy(ALWAYS_PROVIDER)).build();
@@ -1943,7 +1943,7 @@ void testPrependGetAllDestinationsCall()
19431943
.getConfigurationAsJson(eq("/v1/destinations/" + destinationName), any());
19441944
verifyNoMoreInteractions(destinationServiceAdapter);
19451945

1946-
DestinationService.Cache.setGetAllDocumentsPrepended(false);
1946+
DestinationService.Cache.disablePreLookupCheck();
19471947
}
19481948

19491949
@Test
@@ -1956,7 +1956,7 @@ void testPrependGetAllDestinationsCallWithMissingDestination()
19561956
.when(destinationServiceAdapter)
19571957
.getConfigurationAsJson(eq("/v1/subaccountDestinations"), any());
19581958

1959-
DestinationService.Cache.setGetAllDocumentsPrepended(true);
1959+
DestinationService.Cache.enablePreLookupCheck();
19601960

19611961
final DestinationOptions options =
19621962
DestinationOptions.builder().augmentBuilder(augmenter().retrievalStrategy(ALWAYS_PROVIDER)).build();
@@ -1969,6 +1969,6 @@ void testPrependGetAllDestinationsCallWithMissingDestination()
19691969
verify(destinationServiceAdapter, times(1)).getConfigurationAsJson(eq("/v1/subaccountDestinations"), any());
19701970
verifyNoMoreInteractions(destinationServiceAdapter);
19711971

1972-
DestinationService.Cache.setGetAllDocumentsPrepended(false);
1972+
DestinationService.Cache.disablePreLookupCheck();
19731973
}
19741974
}

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

Lines changed: 11 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -72,14 +72,7 @@ void testDefaultTokenExchangeStrategy()
7272

7373
final GetOrComputeSingleDestinationCommand sut =
7474
GetOrComputeSingleDestinationCommand
75-
.prepareCommand(
76-
"destination",
77-
options,
78-
destinationCache,
79-
isolationLocks,
80-
( foo, bar ) -> null,
81-
null,
82-
false)
75+
.prepareCommand("destination", options, destinationCache, isolationLocks, ( foo, bar ) -> null, null)
8376
.get();
8477

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

166159
final GetOrComputeSingleDestinationCommand sut =
167160
GetOrComputeSingleDestinationCommand
168-
.prepareCommand(DESTINATION_NAME, options, destinationCache, isolationLocks, function, null, false)
161+
.prepareCommand(DESTINATION_NAME, options, destinationCache, isolationLocks, function, null)
169162
.get();
170163

171164
assertThat(sut.getCacheKey()).isEqualTo(expectedCacheKey);
@@ -205,7 +198,7 @@ void testLookupOnlyOnUserTokenDestination()
205198

206199
final GetOrComputeSingleDestinationCommand sut =
207200
GetOrComputeSingleDestinationCommand
208-
.prepareCommand(DESTINATION_NAME, options, destinationCache, isolationLocks, function, null, false)
201+
.prepareCommand(DESTINATION_NAME, options, destinationCache, isolationLocks, function, null)
209202
.get();
210203

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

244237
final GetOrComputeSingleDestinationCommand sut =
245238
GetOrComputeSingleDestinationCommand
246-
.prepareCommand(DESTINATION_NAME, options, destinationCache, isolationLocks, function, null, false)
239+
.prepareCommand(DESTINATION_NAME, options, destinationCache, isolationLocks, function, null)
247240
.get();
248241

249242
final Try<Destination> fetchedDestination = sut.execute();
@@ -295,8 +288,7 @@ void testLookupThenExchangeStrategy()
295288
destinationCache,
296289
isolationLocks,
297290
function,
298-
null,
299-
false)
291+
null)
300292
.get()));
301293

302294
final Try<Destination> fetchedDestination = sut.execute();
@@ -350,8 +342,7 @@ void testForwardUserTokenStrategyForUserPropagationDestination()
350342
destinationCache,
351343
isolationLocks,
352344
function,
353-
null,
354-
false)
345+
null)
355346
.get()));
356347

357348
final Try<Destination> fetchedDestination =
@@ -402,8 +393,7 @@ void testForwardUserTokenStrategyForUserPropagationDestinationWithoutJwt()
402393
destinationCache,
403394
isolationLocks,
404395
function,
405-
null,
406-
false)
396+
null)
407397
.get()));
408398

409399
final Try<Destination> fetchedDestination = sut.execute();
@@ -447,14 +437,7 @@ void testForwardUserTokenStrategyForUserPropagationDestinationWithoutPrincipalUn
447437
.executeWithTenant(
448438
t1,
449439
() -> GetOrComputeSingleDestinationCommand
450-
.prepareCommand(
451-
DESTINATION_NAME,
452-
options,
453-
destinationCache,
454-
isolationLocks,
455-
function,
456-
null,
457-
false)
440+
.prepareCommand(DESTINATION_NAME, options, destinationCache, isolationLocks, function, null)
458441
.get());
459442

460443
final Try<Destination> shouldBeFailure = TenantAccessor.executeWithTenant(t1, sut::execute);
@@ -504,8 +487,7 @@ void testForwardUserTokenStrategyForClientCredentialsDestination()
504487
destinationCache,
505488
isolationLocks,
506489
function,
507-
null,
508-
false)
490+
null)
509491
.get()));
510492

511493
final Try<Destination> fetchedDestination = PrincipalAccessor.executeWithPrincipal(p1, () -> sut.execute());
@@ -560,8 +542,7 @@ void testChangeDetectionOnValidAuthToken()
560542
destinationCache,
561543
isolationLocks,
562544
destinationRetriever,
563-
getAllCommand,
564-
false)
545+
getAllCommand)
565546
.flatMap(GetOrComputeSingleDestinationCommand::execute);
566547

567548
assertThat(result.isSuccess()).isTrue();
@@ -646,8 +627,7 @@ void testFirstDestinationAccessDoesNotTriggerGetAllRequest()
646627
destinationCache,
647628
isolationLocks,
648629
destinationRetriever,
649-
getAllMock,
650-
false)
630+
getAllMock)
651631
.get();
652632
final Try<Destination> result = sut.execute();
653633

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -233,8 +233,7 @@ private void runTest( TestCase testCase )
233233
destinationCache,
234234
isolationLocks,
235235
destinationService::loadAndParseDestination,
236-
null,
237-
false);
236+
null);
238237

239238
if( maybeCommand.isFailure() ) {
240239
if( testCase.isCommandCreationExpectedToSucceed() ) {

0 commit comments

Comments
 (0)