Feature context and Async Filters #43435
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mrm9084
requested review from
rujche,
Netyyyy,
saragluna and
moarychan
as code owners
There was a problem hiding this comment.
Copilot reviewed 5 out of 10 changed files in this pull request and generated 1 comment.
Files not reviewed (5)
- sdk/spring/spring-cloud-azure-feature-management/src/test/java/com/azure/spring/cloud/feature/management/filters/TargetingFilterTest.java: Evaluated as low risk
- sdk/spring/spring-cloud-azure-feature-management/src/test/java/com/azure/spring/cloud/feature/management/FeatureManagerTest.java: Evaluated as low risk
- sdk/spring/spring-cloud-azure-feature-management/src/main/java/com/azure/spring/cloud/feature/management/implementation/FeatureManagementConstants.java: Evaluated as low risk
- sdk/spring/spring-cloud-azure-feature-management/src/main/java/com/azure/spring/cloud/feature/management/FeatureManager.java: Evaluated as low risk
- sdk/spring/spring-cloud-azure-feature-management/src/main/java/com/azure/spring/cloud/feature/management/implementation/FeatureFilterUtils.java: Evaluated as low risk
...ment/src/main/java/com/azure/spring/cloud/feature/management/filters/FeatureFilterAsync.java
Show resolved
Hide resolved
rossgrambo
requested changes
Jan 8, 2025
| } | ||
|
|
||
| /** | ||
| * Checks to see if the feature is enabled. If enabled it check each filter, once a single filter returns true it |
There was a problem hiding this comment.
Suggested change
| * Checks to see if the feature is enabled. If enabled it check each filter, once a single filter returns true it | |
| * Checks to see if the feature is enabled. If enabled it checks each filter, once a single filter returns true it |
Same comment on multiple functions
| @@ -59,7 +67,7 @@ public class FeatureManager { | |||
| * @throws FilterNotFoundException file not found | |||
| */ | |||
| public Mono<Boolean> isEnabledAsync(String feature) { | |||
| return Mono.just(checkFeature(feature)); | |||
| return checkFeature(feature, null); | |||
There was a problem hiding this comment.
How do async methods throw the FilterNotFoundException?
| * @throws FilterNotFoundException file not found | ||
| */ | ||
| public Boolean isEnabled(String feature, Object featureContext) throws FilterNotFoundException { | ||
| return checkFeature(feature, featureContext).block(Duration.ofSeconds(100)); |
There was a problem hiding this comment.
I want context for why we chose 100. Could you maybe pull this out into a constant and name it AzureSDKDefaultBlockDuration or something like that
| ReflectionUtils.rethrowRuntimeException(new FilterNotFoundException(message, e, filter)); | ||
| private Mono<Boolean> checkFeatureFilters(Feature featureFlag, Object featureContext) { | ||
| Conditions conditions = featureFlag.getConditions(); | ||
| List<FeatureFilterEvaluationContext> featureFilters = conditions.getClientFilters(); |
| String filterName = featureFilter.getName(); | ||
|
|
||
| try { | ||
|
|
|
|
||
| /** | ||
| * A Filter for Feature Management that is attached to Features. The filter needs to have @Component set to be found by | ||
| * feature management. |
There was a problem hiding this comment.
Should probably call out that these filters have context
| contextAccessor.configureTargetingContext(targetingContext); | ||
| if (contextualAccessor != null && (appContext != null || contextAccessor == null)) { | ||
| // Use this if, there is an appContext + the contextualAccessor, or there is no contextAccessor. | ||
| contextualAccessor.configureTargetingContext(targetingContext, appContext); |
There was a problem hiding this comment.
I'm not convinced the contextualAccessor is providing value here. Couldn't this just be
targetingContext = appContext (with a type check)
It's unclear to me why we want a ContextualTargetingContextAccessor at all. I think it could be removed if we make the above change.
| private boolean targetUser(String userId, List<String> users) { | ||
| return userId != null && users != null && users.stream().anyMatch(user -> equals(userId, user)); | ||
| private boolean targetUser(String userId, Collection<String> collection) { | ||
| return userId != null && collection != null && collection.stream().anyMatch(user -> equals(userId, user)); |
There was a problem hiding this comment.
I prefer the variable name still be users
| /** | ||
| * Looks at the given key in the parameters and coverts it to a list if it is currently a map. | ||
| * | ||
| * @param parameters map of generic objects | ||
| * @param key key of object int the parameters map | ||
| */ | ||
| @SuppressWarnings("unchecked") | ||
| public static void updateValueFromMapToList(Map<String, Object> parameters, String key) { | ||
| public static void updateValueFromMapToList(Map<String, Object> parameters, String key, boolean fixNull) { |
There was a problem hiding this comment.
I think you can remove the fixNull here- as you only ever call it with true as far as I can tell. Could be removed from the if below as well. Also means we remove the overload method.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Adds Async filters and Feature Context.
Trying to minimize the change in here #42853