Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: [Destinations] Support Fragments #491

Merged
merged 5 commits into from
Jul 29, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ final class DestinationRetrievalStrategy
@Nullable
@ToString.Exclude
private final String token;
@Nullable
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's hope the cardinality remains 0..1 :)

private String fragment;

static DestinationRetrievalStrategy withoutToken( @Nonnull final OnBehalfOf behalf )
{
Expand All @@ -53,6 +55,19 @@ static DestinationRetrievalStrategy withUserToken( @Nonnull final OnBehalfOf beh
return new DestinationRetrievalStrategy(behalf, REFRESH_TOKEN, token);
}

DestinationRetrievalStrategy withFragmentName( @Nonnull final String fragmentName )
{
if( fragmentName.isBlank() ) {
throw new IllegalArgumentException("Fragment name must not be empty");
}
// sanity check to enforce this is only ever set once
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not ideal, but making it final leads to more ugly code elsewhere. I felt this is safe enough..

Copy link
Contributor

@newtork newtork Jul 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if( fragment != null ) {
throw new IllegalStateException("Attempted to change an already set fragment name");
}
fragment = fragmentName;
return this;
}

enum TokenForwarding
{
USER_TOKEN,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,13 +115,18 @@ DestinationRetrieval prepareSupplier( @Nonnull final DestinationOptions options
.getRefreshToken(options)
.peek(any -> log.debug("Refresh token given, applying refresh token flow."))
.getOrNull();
final String fragmentName =
DestinationServiceOptionsAugmenter
.getFragmentName(options)
.peek(it -> log.debug("Found fragment name '{}'.", it))
.getOrNull();

log
.debug(
"Loading destination from reuse-destination-service with retrieval strategy {} and token exchange strategy {}.",
retrievalStrategy,
tokenExchangeStrategy);
return prepareSupplier(retrievalStrategy, tokenExchangeStrategy, refreshToken);
return prepareSupplier(retrievalStrategy, tokenExchangeStrategy, refreshToken, fragmentName);
}

/**
Expand Down Expand Up @@ -159,7 +164,8 @@ private DestinationServiceTokenExchangeStrategy getDefaultTokenExchangeStrategy(
DestinationRetrieval prepareSupplier(
@Nonnull final DestinationServiceRetrievalStrategy retrievalStrategy,
@Nonnull final DestinationServiceTokenExchangeStrategy tokenExchangeStrategy,
@Nullable final String refreshToken )
@Nullable final String refreshToken,
@Nullable final String fragmentName )
Comment on lines -162 to +168
Copy link
Contributor

@newtork newtork Jul 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Comment/Question)

At what point will we consider a header list to wrap these optional features? 😁

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

throws DestinationAccessException
{
log
Expand All @@ -175,6 +181,9 @@ DestinationRetrieval prepareSupplier(
retrievalStrategy,
DestinationServiceTokenExchangeStrategy.LOOKUP_ONLY,
refreshToken);
if( fragmentName != null ) {
strategy.withFragmentName(fragmentName);
}
return new DestinationRetrieval(() -> {
final DestinationServiceV1Response result = destinationRetriever.apply(strategy);
if( !doesDestinationConfigurationRequireUserTokenExchange(result) ) {
Expand All @@ -190,6 +199,9 @@ DestinationRetrieval prepareSupplier(

final DestinationRetrievalStrategy strategy =
resolveSingleRequestStrategy(retrievalStrategy, tokenExchangeStrategy, refreshToken);
if( fragmentName != null ) {
strategy.withFragmentName(fragmentName);
}
return new DestinationRetrieval(() -> destinationRetriever.apply(strategy), strategy.behalf());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,9 @@ private HttpUriRequest prepareRequest( final String servicePath, final Destinati
if( headerName != null ) {
request.addHeader(headerName, strategy.token());
}
if( strategy.fragment() != null ) {
request.addHeader("x-fragment-name", strategy.fragment());
}
return request;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ public class DestinationServiceOptionsAugmenter implements DestinationOptionsAug
static final String DESTINATION_RETRIEVAL_STRATEGY_KEY = "scp.cf.destinationRetrievalStrategy";
static final String DESTINATION_TOKEN_EXCHANGE_STRATEGY_KEY = "scp.cf.destinationTokenExchangeStrategy";
static final String X_REFRESH_TOKEN_KEY = "x-refresh-token";
static final String X_FRAGMENT_KEY = "X-fragment-name";
newtork marked this conversation as resolved.
Show resolved Hide resolved

private final Map<String, Object> parameters = new HashMap<>();

Expand Down Expand Up @@ -86,6 +87,31 @@ public DestinationServiceOptionsAugmenter refreshToken( @Nonnull final String re
return this;
}

/**
* Fragment that should enhance the destination to be fetched.
*
* @param fragmentName
* The fragment name.
* @return The same augmenter that called this method.
* @since 5.11.0
*/
@Beta
@Nonnull
public DestinationServiceOptionsAugmenter fragmentName( @Nonnull final String fragmentName )
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😉

Copy link
Contributor

@newtork newtork Jul 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it the DestinationOptions part of the cache key?

yes 👍🏻

{
parameters.put(X_FRAGMENT_KEY, fragmentName);
if( DestinationService.Cache.isEnabled() && DestinationService.Cache.isChangeDetectionEnabled() ) {
log
.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.\
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Consider disabling change detection, if you frequently use destination fragments.
""");
}
return this;
}

@Override
public void augmentBuilder( @Nonnull final DestinationOptions.Builder builder )
{
Expand Down Expand Up @@ -145,4 +171,10 @@ static Option<String> getRefreshToken( @Nonnull final DestinationOptions options
{
return options.get(X_REFRESH_TOKEN_KEY).filter(String.class::isInstance).map(String.class::cast);
}

@Nonnull
static Option<String> getFragmentName( @Nonnull final DestinationOptions options )
{
return options.get(X_FRAGMENT_KEY).filter(String.class::isInstance).map(String.class::cast);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ void testExceptionsAreThrownOnIllegalCombinations()
.executeWithTenant(
c._3(),
() -> softly
.assertThatThrownBy(() -> sut.prepareSupplier(c._1(), c._2(), null))
.assertThatThrownBy(() -> sut.prepareSupplier(c._1(), c._2(), null, null))
.as("Expecting '%s' with '%s' and '%s' to throw.", c._1(), c._2(), c._3())
.isInstanceOf(DestinationAccessException.class)));

Expand All @@ -234,7 +234,12 @@ void testExceptionsAreThrownForImpossibleTokenExchanges()
{
doAnswer(( any ) -> true).when(sut).doesDestinationConfigurationRequireUserTokenExchange(any());
final DestinationRetrieval supplier =
sut.prepareSupplier(ALWAYS_PROVIDER, DestinationServiceTokenExchangeStrategy.LOOKUP_THEN_EXCHANGE, null);
sut
.prepareSupplier(
ALWAYS_PROVIDER,
DestinationServiceTokenExchangeStrategy.LOOKUP_THEN_EXCHANGE,
null,
null);

TenantAccessor
.executeWithTenant(
Expand All @@ -259,7 +264,7 @@ void testDefaultStrategies()
sut.prepareSupplier(DestinationOptions.builder().build());
sut.prepareSupplierAllDestinations(DestinationOptions.builder().build());

verify(sut).prepareSupplier(CURRENT_TENANT, FORWARD_USER_TOKEN, null);
verify(sut).prepareSupplier(CURRENT_TENANT, FORWARD_USER_TOKEN, null, null);
verify(sut).prepareSupplierAllDestinations(CURRENT_TENANT);
}

Expand All @@ -272,7 +277,8 @@ void testDefaultNonXsuaaTokenStrategy()
sut.prepareSupplier(DestinationOptions.builder().build());
sut.prepareSupplierAllDestinations(DestinationOptions.builder().build());

verify(sut).prepareSupplier(CURRENT_TENANT, DestinationServiceTokenExchangeStrategy.LOOKUP_THEN_EXCHANGE, null);
verify(sut)
.prepareSupplier(CURRENT_TENANT, DestinationServiceTokenExchangeStrategy.LOOKUP_THEN_EXCHANGE, null, null);
verify(sut).prepareSupplierAllDestinations(CURRENT_TENANT);
}

Expand All @@ -290,4 +296,19 @@ void testRefreshToken()

verify(sut).resolveSingleRequestStrategy(eq(CURRENT_TENANT), eq(LOOKUP_ONLY), eq(refreshToken));
}

@Test
void testFragmentName()
{
final String fragmentName = "my-fragment";
final DestinationOptions opts =
DestinationOptions
.builder()
.augmentBuilder(DestinationServiceOptionsAugmenter.augmenter().fragmentName(fragmentName))
.build();

sut.prepareSupplier(opts);

verify(sut).prepareSupplier(any(), any(), eq(null), eq(fragmentName));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,31 @@ void testRefreshTokenFlow()
.withoutHeader("x-user-token"));
}

@Test
void testFragmentName()
{
final DestinationServiceAdapter adapterToTest = createSut(DEFAULT_SERVICE_BINDING);

final String fragment = "my-fragment";

final String destinationResponse =
adapterToTest
.getConfigurationAsJson(
"/",
DestinationRetrievalStrategy
.withoutToken(TECHNICAL_USER_CURRENT_TENANT)
.withFragmentName(fragment));

assertThat(destinationResponse).isEqualTo(DESTINATION_RESPONSE);

verify(
1,
getRequestedFor(urlEqualTo(DESTINATION_SERVICE_URL))
.withHeader("Authorization", equalTo("Bearer " + xsuaaToken))
.withHeader("x-fragment-name", equalTo(fragment))
.withoutHeader("x-user-token"));
}

@Test
void getDestinationServiceProviderTenantShouldReturnProviderTenantFromServiceBinding()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
import java.util.concurrent.TimeoutException;
import java.util.concurrent.atomic.AtomicReference;
import java.util.concurrent.locks.ReentrantLock;
import java.util.function.Function;

import org.apache.http.HttpVersion;
import org.apache.http.client.HttpClient;
Expand Down Expand Up @@ -1598,6 +1599,60 @@ void testAuthTokenFailureIsNotCached()
withUserToken(TECHNICAL_USER_CURRENT_TENANT, userToken));
}

@Test
void testFragmentDestinationsAreCacheIsolated()
{
DestinationService.Cache.disableChangeDetection();
final String destinationTemplate = """
{
"owner": {
"SubaccountId": "00000000-0000-0000-0000-000000000000",
"InstanceId": null
},
"destinationConfiguration": {
"Name": "destination",
%s
"Type": "HTTP",
"URL": "https://%s.com/",
"Authentication": "NoAuthentication",
"ProxyType": "Internet"
}
}
""";

doReturn(destinationTemplate.formatted("\"FragmentName\": \"a-fragment\",", "a.fragment"))
.when(destinationServiceAdapter)
.getConfigurationAsJson(any(), argThat(it -> "a-fragment".equals(it.fragment())));
doReturn(destinationTemplate.formatted("\"FragmentName\": \"b-fragment\",", "b.fragment"))
.when(destinationServiceAdapter)
.getConfigurationAsJson(any(), argThat(it -> "b-fragment".equals(it.fragment())));
doReturn(destinationTemplate.formatted("", "destination"))
.when(destinationServiceAdapter)
.getConfigurationAsJson(any(), argThat(it -> it.fragment() == null));

final Function<String, DestinationOptions> optsBuilder =
frag -> DestinationOptions
.builder()
.augmentBuilder(DestinationServiceOptionsAugmenter.augmenter().fragmentName(frag))
.build();

final Destination dA = loader.tryGetDestination("destination", optsBuilder.apply("a-fragment")).get();
final Destination dB = loader.tryGetDestination("destination", optsBuilder.apply("b-fragment")).get();
final Destination d = loader.tryGetDestination("destination").get();

assertThat(dA).isNotEqualTo(dB).isNotEqualTo(d);
assertThat(dA.get("FragmentName")).contains("a-fragment");
assertThat(dB).isNotEqualTo(d);
assertThat(dB.get("FragmentName")).contains("b-fragment");

assertThat(d.get("FragmentName")).isEmpty();

assertThat(dA)
.describedAs("Destinations with fragments should be cached")
.isSameAs(loader.tryGetDestination("destination", optsBuilder.apply("a-fragment")).get());
verify(destinationServiceAdapter, times(3)).getConfigurationAsJson(any(), any());
}

// @Test
// Performance test is unreliable on Jenkins
void runLoadTest()
Expand Down
4 changes: 3 additions & 1 deletion release_notes.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@

### ✨ New Functionality

-
- Add support for [_Destination Fragments_](https://help.sap.com/docs/connectivity/sap-btp-connectivity-cf/extending-destinations-with-fragments).
MatKuhr marked this conversation as resolved.
Show resolved Hide resolved
Fragment names can be passed upon requesting destinations via `DestinationServiceOptionsAugmenter.fragmentName("my-fragment-name")`.
For further details refer to [the documentation]().
MatKuhr marked this conversation as resolved.
Show resolved Hide resolved

### 📈 Improvements

Expand Down
Loading