fix: Fix caching in getOrComputeAllDestinationsCommand

[Jonas-Isr]

This PR fixes an issue in the caching logic of the GetOrComputeAllDestinationsCommand class:

Before this fix, in GetOrComputeAllDestinationsCommand, cache keys are build from the entire DestinationOptions object. These options might very well contain information that is irrelevant for the call to get all destinations. This will lead to unnecessary HTTP calls due to over-eager cache isolation.
More precisely, when change detection was enabled and DestinationService.tryGetDestination was called, we invoke GetOrComputeAllDestinationsCommand. This uses the above mentioned unnecessarily strict cache, leading to an unnecessary cache-miss.

This problem was first found while working on https://github.com/orgs/SAP/projects/62/views/7?pane=issue&itemId=140717964&issue=SAP%7Ccloud-sdk-java-backlog%7C495.

Feature scope:

• Fix cache in GetOrComputeAllDestinationsCommand by using cacheKey.append(DestinationServiceOptionsAugmenter.getRetrievalStrategy(destinationOptions))
• adapt tests where necessary

Definition of Done

• Functionality scope stated & covered
• Tests cover the scope above
• Error handling created / updated & covered by the tests above
• Documentation updated
• Release notes updated

[newtork]

(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

[newtork]

(Comment/Question)

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

[newtork]

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)

[newtork]

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

[MatKuhr]

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)

[Jonas-Isr]

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.)

[newtork]

(Minor/Comment)

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