-
Notifications
You must be signed in to change notification settings - Fork 13
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
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,8 @@ final class DestinationRetrievalStrategy | |
@Nullable | ||
@ToString.Exclude | ||
private final String token; | ||
@Nullable | ||
private String fragment; | ||
|
||
static DestinationRetrievalStrategy withoutToken( @Nonnull final OnBehalfOf behalf ) | ||
{ | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not ideal, but making it There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
if( fragment != null ) { | ||
throw new IllegalStateException("Attempted to change an already set fragment name"); | ||
} | ||
fragment = fragmentName; | ||
return this; | ||
} | ||
|
||
enum TokenForwarding | ||
{ | ||
USER_TOKEN, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
} | ||
|
||
/** | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? 😁 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess one or two more 😄 But in theory, we might want to support e.g. |
||
throws DestinationAccessException | ||
{ | ||
log | ||
|
@@ -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) ) { | ||
|
@@ -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()); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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<>(); | ||
|
||
|
@@ -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 ) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only one fragment can be given per request, thus There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
... How? is it the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.\ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Question)
I don't really understand this. Isn't "fragment-based" destination just a regular destination? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no, the destination + fragment != destination. Thus, if you do |
||
Consider disabling change detection, if you frequently use destination fragments. | ||
"""); | ||
} | ||
return this; | ||
} | ||
|
||
@Override | ||
public void augmentBuilder( @Nonnull final DestinationOptions.Builder builder ) | ||
{ | ||
|
@@ -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); | ||
} | ||
} |
There was a problem hiding this comment.
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 😄
There was a problem hiding this comment.
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
:)