feat: [Destinations] Support Fragments #491
Conversation
| if( fragmentName.isBlank() ) { | ||
| throw new IllegalArgumentException("Fragment name must not be empty"); | ||
| } | ||
| // sanity check to enforce this is only ever set once |
There was a problem hiding this comment.
not ideal, but making it final leads to more ugly code elsewhere. I felt this is safe enough..
There was a problem hiding this comment.
Hmm.. all other methods create a new immutable object, leveraging the Wither API design pattern.
I would want to see and compare the "ugly code" first before going with this compromise.
There was a problem hiding this comment.
making it immutable would mean we need to change every instantiation of this object. That would be quite some more places, you can find them via usages of the current with.. methods.
| */ | ||
| @Beta | ||
| @Nonnull | ||
| public DestinationServiceOptionsAugmenter fragmentName( @Nonnull final String fragmentName ) |
There was a problem hiding this comment.
Only one fragment can be given per request, thus String. Adding this here is the natural place IMO, but it also has the advantage that the fragment name is automatically part of the cache key 😉
There was a problem hiding this comment.
the advantage that the fragment name is automatically part of the cache key
... How? is it the DestinationOptions part of the cache key?
Or do you mean the resulting destination properties will have mixed in values, that qualify as cache-key anyway?
There was a problem hiding this comment.
is it the DestinationOptions part of the cache key?
yes 👍🏻
| @@ -28,6 +28,8 @@ final class DestinationRetrievalStrategy | |||
| @Nullable | |||
| @ToString.Exclude | |||
| private final String token; | |||
| @Nullable | |||
There was a problem hiding this comment.
maybe a bit questionable to use this class for holding the fragment, but the alternative I saw would be to change DestinationServiceAdapter.java:
String getConfigurationAsJson(
@Nonnull final String servicePath,
@Nonnull final DestinationRetrievalStrategy strategy,
+ @Nullable final String fragment )which would lead to > 100 test changes. Considering that fragment is somewhat similar to refresh token I felt it was fair enough to put it here 😄
There was a problem hiding this comment.
Let's hope the cardinality remains 0..1 :)
| @Nullable final String refreshToken ) | ||
| @Nullable final String refreshToken, | ||
| @Nullable final String fragmentName ) |
There was a problem hiding this comment.
(Comment/Question)
At what point will we consider a header list to wrap these optional features? 😁
There was a problem hiding this comment.
I guess one or two more 😄
With the tokens once class makes sense, because the three options are mutually exclusive.
But in theory, we might want to support e.g. $filter in the future, then it's probably worth to create a new, dedicated object
...in/java/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationServiceOptionsAugmenter.java
Show resolved
Hide resolved
...rvice/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationServiceTest.java
Outdated
Show resolved
Hide resolved
| .warn( | ||
| """ | ||
| A fragment was requested while change detection caching is enabled.\ | ||
| This is not recommended, as fragment-based destinations will effectively not be cached with this strategy.\ |
There was a problem hiding this comment.
(Question)
Fragment-based destinations will effectively not be cached with this strategy
I don't really understand this. Isn't "fragment-based" destination just a regular destination?
There was a problem hiding this comment.
no, the destination + fragment != destination. Thus, if you do getAll you will get the properties without the fragment properties, so when performing change detection the destination will always appear to be changed.
There was a problem hiding this comment.
Please take over my WiP pull request: Fragment Destinations are compatible with Change Detection
Co-authored-by: Alexander Dümont <[email protected]>
Context
This PR enhances the Cloud SDK to support Destination Fragments.
Feature scope:
Definition of Done
Error handling created / updated & covered by the tests above