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.

Sign up for GitHub

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

fix: Always check parent dependency if present #248

Merged
merged 1 commit into from
Jul 29, 2024
Merged

fix: Always check parent dependency if present #248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
136 changes: 65 additions & 71 deletions src/main/java/io/getunleash/DefaultUnleash.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,56 +179,53 @@ private FeatureEvaluationResult getFeatureEvaluationResult(
fallbackAction.test(toggleName, enhancedContext), defaultVariant);
} else if (!featureToggle.isEnabled()) {
return new FeatureEvaluationResult(false, defaultVariant);
} else if (featureToggle.getStrategies().isEmpty()) {
return new FeatureEvaluationResult(
true, VariantUtil.selectVariant(featureToggle, context, defaultVariant));
} else {
} else if (isParentDependencySatisfied(featureToggle, context, fallbackAction)) {
// Dependent toggles, no point in evaluating child strategies if our dependencies are
// not satisfied
if (isParentDependencySatisfied(featureToggle, context, fallbackAction)) {
for (ActivationStrategy strategy : featureToggle.getStrategies()) {
Strategy configuredStrategy = getStrategy(strategy.getName());
if (configuredStrategy == UNKNOWN_STRATEGY) {
LOGGER.warn(
"Unable to find matching strategy for toggle:{} strategy:{}",
toggleName,
strategy.getName());
}
if (featureToggle.getStrategies().isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we do something like:

processedStrategies = featureToggle.getStrategies().map(this::getStrategy).filter(s -> s != UNKNOWN_STRATEGY);
if (processedStrategies.isEmpty()) {
...

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Rethinking this is probably a good idea, but it doesn't relate to this PR, so I'd suggest leaving it for a separate sparring + PR session

return new FeatureEvaluationResult(
true, VariantUtil.selectVariant(featureToggle, context, defaultVariant));
}
for (ActivationStrategy strategy : featureToggle.getStrategies()) {
Strategy configuredStrategy = getStrategy(strategy.getName());
if (configuredStrategy == UNKNOWN_STRATEGY) {
LOGGER.warn(
"Unable to find matching strategy for toggle:{} strategy:{}",
toggleName,
strategy.getName());
}

FeatureEvaluationResult result =
configuredStrategy.getResult(
strategy.getParameters(),
enhancedContext,
ConstraintMerger.mergeConstraints(featureRepository, strategy),
strategy.getVariants());

if (result.isEnabled()) {
Variant variant = result.getVariant();
// If strategy variant is null, look for a variant in the featureToggle
if (variant == null) {
variant =
VariantUtil.selectVariant(
featureToggle, context, defaultVariant);
}
result.setVariant(variant);
return result;
FeatureEvaluationResult result =
configuredStrategy.getResult(
strategy.getParameters(),
enhancedContext,
ConstraintMerger.mergeConstraints(featureRepository, strategy),
strategy.getVariants());
Comment on lines +198 to +203
Copy link
Contributor

Choose a reason for hiding this comment

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

I find it awkward that if we don't find strategies we return enabled=true but if we do find strategies but we can't map the strategy to a known strategy we (probably) return enabled=false at line 216

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 don't disagree, it's not because of this PR though.

Maybe a rethink here; as a separate thing. I'm open to some sparring

Copy link
Member

Choose a reason for hiding this comment

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

I find it awkward that if we don't find strategies we return enabled=true but if we do find strategies but we can't map the strategy to a known strategy we (probably) return enabled=false at line 216

I think you have to take this up with v0.0.1 of the SDK spec. That's the correct behavior. An empty list of strategies is valid. A custom strategy that you defined server side but didn't implement client side is a broken and is expected to short circuit to false

Copy link

Choose a reason for hiding this comment

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

A custom strategy that you defined server side but didn't implement client side is a broken and is expected to short circuit to false

I am completely with @sighphyre here. If a strategy is not found, the safe and correct way is to return false for the toggle if the strategy is not found


if (result.isEnabled()) {
Variant variant = result.getVariant();
// If strategy variant is null, look for a variant in the featureToggle
if (variant == null) {
variant = VariantUtil.selectVariant(featureToggle, context, defaultVariant);
}
result.setVariant(variant);
return result;
}
}
return new FeatureEvaluationResult(false, defaultVariant);
}
return new FeatureEvaluationResult(false, defaultVariant);
Copy link
Member

Choose a reason for hiding this comment

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

Something feels very subtly off here but it's not breaking tests sooo... it might be okay?

What's going through my head? Previously, we'd fall through to true, now we fall through to false. In theory, a toggle that has no strategies should fallback to the 'enabled' property, not true or false. That's distinct from a toggle that doesn't have strategies defined, which should never happen server side but when it does we turn off the toggle

So really we're into undefined territory here. It's probably okay but there's nothing in the client spec that enforces this behavior, so it's possible we break this again subtly

Copy link
Member Author

Choose a reason for hiding this comment

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

But we've already checked the enabled property. If that evaluates to false, we short-circuit the entire evaluation. The one thing this does, that it didn't do previously, is the case where strategies was empty, now we have to check parent as well

Copy link
Member Author

Choose a reason for hiding this comment

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

But, since we now have 3 people saying that we struggle with following the flow; I'm all for taking the time to make a new PR (Separate from this one) trying to clean up the logic, still making sure we pass all the tests, but so we don't need clarification talks every time we touch this.

}

/**
* Uses the old, statistically broken Variant seed for finding the correct variant
*
* @deprecated
* @param toggleName Name of the toggle
* @param context The UnleashContext
* @param fallbackAction What to do if we fail to find the toggle
* @param defaultVariant If we can't resolve a variant, what are we returning
* @return A wrapper containing whether the feature was enabled as well which Variant was
* selected
* @deprecated
*/
private FeatureEvaluationResult deprecatedGetFeatureEvaluationResult(
String toggleName,
Expand All @@ -244,46 +241,43 @@ private FeatureEvaluationResult deprecatedGetFeatureEvaluationResult(
fallbackAction.test(toggleName, enhancedContext), defaultVariant);
} else if (!featureToggle.isEnabled()) {
return new FeatureEvaluationResult(false, defaultVariant);
} else if (featureToggle.getStrategies().isEmpty()) {
return new FeatureEvaluationResult(
true,
VariantUtil.selectDeprecatedVariantHashingAlgo(
featureToggle, context, defaultVariant));
} else {
// Dependent toggles, no point in evaluating child strategies if our dependencies are
// not satisfied
if (isParentDependencySatisfied(featureToggle, context, fallbackAction)) {
for (ActivationStrategy strategy : featureToggle.getStrategies()) {
Strategy configuredStrategy = getStrategy(strategy.getName());
if (configuredStrategy == UNKNOWN_STRATEGY) {
LOGGER.warn(
"Unable to find matching strategy for toggle:{} strategy:{}",
toggleName,
strategy.getName());
}
} else if (isParentDependencySatisfied(featureToggle, context, fallbackAction)) {
if (featureToggle.getStrategies().isEmpty()) {
return new FeatureEvaluationResult(
true,
VariantUtil.selectDeprecatedVariantHashingAlgo(
featureToggle, context, defaultVariant));
}
for (ActivationStrategy strategy : featureToggle.getStrategies()) {
Strategy configuredStrategy = getStrategy(strategy.getName());
if (configuredStrategy == UNKNOWN_STRATEGY) {
LOGGER.warn(
"Unable to find matching strategy for toggle:{} strategy:{}",
toggleName,
strategy.getName());
}

FeatureEvaluationResult result =
configuredStrategy.getDeprecatedHashingAlgoResult(
strategy.getParameters(),
enhancedContext,
ConstraintMerger.mergeConstraints(featureRepository, strategy),
strategy.getVariants());

if (result.isEnabled()) {
Variant variant = result.getVariant();
// If strategy variant is null, look for a variant in the featureToggle
if (variant == null) {
variant =
VariantUtil.selectDeprecatedVariantHashingAlgo(
featureToggle, context, defaultVariant);
}
result.setVariant(variant);
return result;
FeatureEvaluationResult result =
configuredStrategy.getDeprecatedHashingAlgoResult(
strategy.getParameters(),
enhancedContext,
ConstraintMerger.mergeConstraints(featureRepository, strategy),
strategy.getVariants());

if (result.isEnabled()) {
Variant variant = result.getVariant();
// If strategy variant is null, look for a variant in the featureToggle
if (variant == null) {
variant =
VariantUtil.selectDeprecatedVariantHashingAlgo(
featureToggle, context, defaultVariant);
}
result.setVariant(variant);
return result;
}
}
return new FeatureEvaluationResult(false, defaultVariant);
}
return new FeatureEvaluationResult(false, defaultVariant);
}

private boolean isParentDependencySatisfied(
Expand Down Expand Up @@ -393,10 +387,10 @@ public Variant getVariant(String toggleName, Variant defaultValue) {
/**
* Uses the old, statistically broken Variant seed for finding the correct variant
*
* @deprecated
* @param toggleName
* @param context
* @return
* @deprecated
*/
@Override
public Variant deprecatedGetVariant(String toggleName, UnleashContext context) {
Expand All @@ -406,11 +400,11 @@ public Variant deprecatedGetVariant(String toggleName, UnleashContext context) {
/**
* Uses the old, statistically broken Variant seed for finding the correct variant
*
* @deprecated
* @param toggleName
* @param context
* @param defaultValue
* @return
* @deprecated
*/
@Override
public Variant deprecatedGetVariant(
Expand All @@ -437,9 +431,9 @@ private Variant deprecatedGetVariant(
/**
* Uses the old, statistically broken Variant seed for finding the correct variant
*
* @deprecated
* @param toggleName
* @return
* @deprecated
*/
@Override
public Variant deprecatedGetVariant(String toggleName) {
Expand All @@ -449,10 +443,10 @@ public Variant deprecatedGetVariant(String toggleName) {
/**
* Uses the old, statistically broken Variant seed for finding the correct variant
*
* @deprecated
* @param toggleName
* @param defaultValue
* @return
* @deprecated
*/
@Override
public Variant deprecatedGetVariant(String toggleName, Variant defaultValue) {
Expand Down
26 changes: 26 additions & 0 deletions src/test/java/io/getunleash/DependentFeatureToggleTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,4 +176,30 @@ public void should_trigger_impression_event_for_parent_variant_when_checking_chi
when(featureRepository.getToggle(parentName)).thenReturn(parent);
assertThat(sut.isEnabled(childName, UnleashContext.builder().build())).isFalse();
}

@Test
public void childIsDisabledWhenChildDoesNotHaveStrategiesAndParentIsDisabled() {
FeatureToggle parent =
new FeatureToggle(
"parent", false, singletonList(new ActivationStrategy("default", null)));
FeatureDependency childDependsOnParent = new FeatureDependency("parant", true, emptyList());
FeatureToggle child =
new FeatureToggle(
"child",
true,
emptyList(),
emptyList(),
true,
singletonList(childDependsOnParent));
when(featureRepository.getToggle("child")).thenReturn(child);
when(featureRepository.getToggle("parent")).thenReturn(parent);
assertThat(sut.isEnabled("child", UnleashContext.builder().build())).isFalse();
}

@Test
public void shouldBeEnabledWhenMissingStrategies() {
FeatureToggle c = new FeatureToggle("c", true, emptyList());
when(featureRepository.getToggle("c")).thenReturn(c);
assertThat(sut.isEnabled("c", UnleashContext.builder().build())).isTrue();
}
}
Loading