Unleash / unleash-client-java

Unleash client SDK for Java
https://docs.getunleash.io
Apache License 2.0
121 stars 70 forks source link

Wrong toggle evaluation with dependencies #246

Closed DAGRSAG closed 4 months ago

DAGRSAG commented 4 months ago

Describe the bug

Hi, we have the following config defined in our bootstrap file

{
    "version": 2,
    "features": [
        {
            "name": "SR26_Pack",
            "enabled": false
        },
        {
            "name": "featureOne",
            "enabled": true,
            "dependencies": [
                {
                    "feature": "SR26_Pack"
                }
            ]
        }
    ]
}

My purpose here is, that we have toggle featureOne which should only be true, when the parent SR26_Pack is true.

If your code evaluates the toggle now, the return value is true, which is not expected.

In my opinion the problem is in this DefaultUnleash code here

UnleashContext enhancedContext = context.applyStaticFields(config);
if (featureToggle == null) {
    return new FeatureEvaluationResult(
            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));
}

this code means, that if no strategy is defined (featureToggle.getStrategies().isEmpty() returns true) always true is returned as evaluation value for the toggle. The parent dependencies are only looked at for the first time in the else block afterwards.

Why not checking first if the isParentDependencySatisfied after !featureToggle.isEnabled()? Because if the parent dependencies not satisfied to toggle can not be true.

The behavior is correct, if e.g. the default strategy is added to the toggle def

Steps to reproduce the bug

No response

Expected behavior

Parent dependencies gets considered also if no strategy is defined

Logs, error output, etc.

No response

Screenshots

No response

Additional context

No response

Unleash version

No response

Subscription type

Open source

Hosting type

Self-hosted

SDK information (language and version)

Java in version 9.2.0

DAGRSAG commented 4 months ago

I should add, that we use the sdk in an offline mode. Means only the bootstrap file is loaded and decides about the toggle state

chriswk commented 4 months ago

Hi @DAGRSAG . Thanks for your report. I think you're right; I'll see if I can't release a fix this week.

DAGRSAG commented 4 months ago

Hi @chriswk, thanks for the update and working on a fix.

chriswk commented 4 months ago

I've released 9.2.4 just now, which includes this fix. Should be live within the next hour. Thanks for reporting this.

DAGRSAG commented 4 months ago

Awesome. Thanks. I will test it on Thursday 👍