feat: Add pre lookup check to DestinationService.tryGetDestination
#1056
Conversation
...n-service/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationService.java
Outdated
Show resolved
Hide resolved
...n-service/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationService.java
Outdated
Show resolved
Hide resolved
|
Regarding a test showing the failure we saw in the e2e tests in December: I added an additional test that is green in the current implementation but will fail if the retrieval strategy is not correctly forwarded in the pre-lookup check of the To see this, simply delete |
…o add-preLookup-Check
…o add-preLookup-Check
# Conflicts: # release_notes.md
# Conflicts: # cloudplatform/connectivity-destination-service/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationService.java
| final String msg = "Destination %s was not found among the destinations of the current tenant."; | ||
| return Try.failure(new DestinationNotFoundException(destinationName, String.format(msg, destinationName))); |
There was a problem hiding this comment.
(Minor)
I found the hard-coded "destinations of current tenant" misleading for cases like "always-provider". Therefore I moved the code around a little and introduced the notion of "validating destination lookup". Maybe "intercept" would've been a better word(?)
| final String[] featureNames = | ||
| { | ||
| DestinationServiceOptionsAugmenter.X_FRAGMENT_KEY, | ||
| DestinationServiceOptionsAugmenter.CROSS_LEVEL_SETTING_KEY, | ||
| DestinationServiceOptionsAugmenter.CUSTOM_HEADER_KEY }; | ||
| final Set<String> keys = options.getOptionKeys(); | ||
| return keys.stream().anyMatch(s -> Arrays.stream(featureNames).anyMatch(s::equalsIgnoreCase)); | ||
| return keys.stream().anyMatch(s -> Arrays.stream(featureNames).anyMatch(s::startsWith)); |
There was a problem hiding this comment.
(Comment)
I replaced my incorrectly changed "equalsIgnoreCase" back to your original "startsWith". Unit tests now cover this difference.
Also referenced the options-augmenter directly, in case we'll refine it later.
BLI:
Prerequisite:
getOrComputeAllDestinationsCommand#1055Context
This is a feature that we already merged at one point and then later removed again because we found problems that we were not able to fix and test before release. (The underlying problem we found and wanted to fix first is in the PR linked above.)
This PR prepends an additional check to
tryGetDestination: when a single destination is called, it is first checked whether this destination appears in the result of a get all destination call. If the destination is not present, no call to the destination is made. The goal of this feature is to improve the circuit breaker, as the circuit breaker now does not open unnecessarily when non-existing destination(s) are called multiple times.Feature scope:
DestinationService.tryGetDestinationDefinition of Done