Unleash / unleash-client-java

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

Strategy check return only the first enabled #210

Closed gabritgtg closed 1 year ago

gabritgtg commented 1 year ago

Describe the bug

In the [commit https://github.com/Unleash/unleash-client-java/commit/78a0d33358bb375d6abe8d08de60f704a12961a6](feat: strategy variants) has been introduced a bug. When retrieving the toggle enabled strategies it is returned only the first. In case of multiple strategy the the toggle will result disabled even when the context is valid.

See into class src/main/java/io/getunleash/DefaultUnleash.java

        } else {
            enabledStrategy =
                    featureToggle.getStrategies().stream()
                            .filter(
                                    strategy -> {
                                        Strategy configuredStrategy =
                                                getStrategy(strategy.getName());
    @@ -181,13 +184,33 @@ private boolean checkEnabled(
                                        }

                                        return configuredStrategy.isEnabled(
                                                strategy.getParameters(), context);
                                    })
                            .findFirst();

Steps to reproduce the bug

  1. Create a toggle with more then one strategy
  2. Check if the toggle is enabled having as context valued that meet second or further strategy in the list.
  3. Should expect to return enabled true but is false

Expected behavior

When calling the method isEnabled with context that meet one of the strategy should expect true as response

Logs, error output, etc.

No response

Screenshots

No response

Additional context

No response

Unleash version

Subscription type

Enterprise

Hosting type

Hosted by Unleash

SDK information (language and version)

java, 8.3.0 ( introduced in this commit-78a0d33358bb375d6abe8d08de60f704a12961a6)

gastonfournier commented 1 year ago

Thanks for the report! I currently managed to reproduce it locally, I'm working on a unit test and then trying to fix it. I believe you properly pointed at the issue with that findFirst, thanks!

sighphyre commented 1 year ago

Hey @gabritgtg, this has been fixed in 8.3.1, which has been released, thank you so much for reporting this!

gabritgtg commented 1 year ago

Thanks to everyone for the fast replay. You are a great team :)