feat: strategy variants (#265)

* feat: strategy variants * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix linter * add unit tests * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * add unit tests * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * lint * cleanup * cleanup * cleanup * cleanup * fix linter complaints * fix linter complaints * fix linter complaints * fix logic * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * formatting * improvements * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * bug fixes * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * remove constraint noise from test * fix tests * fix tests * fix ruff complaining * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix PR comments * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Unleash · Aug 3, 2023 · 0e7b894 · 0e7b894
1 parent 15c9000
commit 0e7b894
Show file tree

Hide file tree

Showing 15 changed files with 221 additions and 67 deletions.
diff --git a/UnleashClient/__init__.py b/UnleashClient/__init__.py
@@ -469,7 +469,8 @@ def get_variant(self, feature_name: str, context: Optional[dict] = None) -> dict
                 self.features[feature_name] = new_feature
 
                 # Use the feature's get_variant method to count the call
-                return new_feature.get_variant(context)
+                variant_check = new_feature.get_variant(context)
+                return variant_check
         else:
             LOGGER.log(
                 self.unleash_verbose_log_level,

diff --git a/UnleashClient/constants.py b/UnleashClient/constants.py
@@ -6,7 +6,7 @@
 REQUEST_TIMEOUT = 30
 REQUEST_RETRIES = 3
 METRIC_LAST_SENT_TIME = "mlst"
-CLIENT_SPEC_VERSION = "4.2.2"
+CLIENT_SPEC_VERSION = "4.3.1"
 
 # =Unleash=
 APPLICATION_HEADERS = {

diff --git a/UnleashClient/constraints/Constraint.py b/UnleashClient/constraints/Constraint.py
@@ -226,10 +226,9 @@ def apply(self, context: dict = None) -> bool:
                     constraint_check = self.check_semver_operators(
                         context_value=context_value
                     )
-            else:
-                # This is a special case in the client spec - so it's getting it's own handler here
-                if self.operator is ConstraintOperators.NOT_IN:  # noqa: PLR5501
-                    constraint_check = True
+            # This is a special case in the client spec - so it's getting it's own handler here
+            elif self.operator is ConstraintOperators.NOT_IN:  # noqa: PLR5501
+                constraint_check = True
 
         except Exception as excep:  # pylint: disable=broad-except
             LOGGER.info(

diff --git a/UnleashClient/deprecation_warnings.py b/UnleashClient/deprecation_warnings.py
@@ -10,7 +10,7 @@ def strategy_v2xx_deprecation_check(strategies: list) -> None:
     for strategy in strategies:
         try:
             # Check if the __call__() method is overwritten (should only be true for custom strategies in v1.x or v2.x.
-            if strategy.__call__ != Strategy.__call__:
+            if strategy.__call__ != Strategy.__call__:  # type:ignore
                 warnings.warn(
                     f"unleash-client-python v3.x.x requires overriding the execute() method instead of the __call__() method. Error in: {strategy.__name__}",
                     DeprecationWarning,

diff --git a/UnleashClient/features/Feature.py b/UnleashClient/features/Feature.py
@@ -1,7 +1,9 @@
 # pylint: disable=invalid-name
+import copy
 from typing import Dict, Optional, cast
 
 from UnleashClient.constants import DISABLED_VARIATION
+from UnleashClient.strategies import EvaluationResult, Strategy
 from UnleashClient.utils import LOGGER
 from UnleashClient.variants import Variants
 
@@ -73,33 +75,16 @@ def _count_variant(self, variant_name: str) -> None:
         """
         self.variant_counts[variant_name] = self.variant_counts.get(variant_name, 0) + 1
 
-    def is_enabled(
-        self, context: dict = None, default_value: bool = False
-    ) -> bool:  # pylint: disable=unused-argument
+    def is_enabled(self, context: dict = None) -> bool:
         """
         Checks if feature is enabled.
 
         :param context: Context information
-        :param default_value: Deprecated!  Users should use the fallback_function on the main is_enabled() method.
         :return:
         """
-        flag_value = False
+        evaluation_result = self._get_evaluation_result(context)
 
-        if self.enabled:
-            try:
-                if self.strategies:
-                    strategy_result = any(x.execute(context) for x in self.strategies)
-                else:
-                    # If no strategies are present, should default to true. This isn't possible via UI.
-                    strategy_result = True
-
-                flag_value = strategy_result
-            except Exception as strategy_except:
-                LOGGER.warning("Error checking feature flag: %s", strategy_except)
-
-        self.increment_stats(flag_value)
-
-        LOGGER.info("Feature toggle status for feature %s: %s", self.name, flag_value)
+        flag_value = evaluation_result.enabled
 
         return flag_value
 
@@ -110,19 +95,46 @@ def get_variant(self, context: dict = None) -> dict:
         :param context: Context information
         :return:
         """
-        variant = DISABLED_VARIATION
-        is_feature_enabled = self.is_enabled(context)
-
-        if is_feature_enabled and self.variants is not None:
+        evaluation_result = self._get_evaluation_result(context)
+        is_feature_enabled = evaluation_result.enabled
+        variant = evaluation_result.variant
+        if variant is None or (is_feature_enabled and variant == DISABLED_VARIATION):
             try:
-                variant = self.variants.get_variant(context)
-                variant["enabled"] = is_feature_enabled
+                LOGGER.debug("Getting variant from feature: %s", self.name)
+                variant = (
+                    self.variants.get_variant(context, is_feature_enabled)
+                    if is_feature_enabled
+                    else copy.deepcopy(DISABLED_VARIATION)
+                )
+
             except Exception as variant_exception:
                 LOGGER.warning("Error selecting variant: %s", variant_exception)
 
         self._count_variant(cast(str, variant["name"]))
         return variant
 
+    def _get_evaluation_result(self, context: dict = None) -> EvaluationResult:
+        strategy_result = EvaluationResult(False, None)
+        if self.enabled:
+            try:
+                if self.strategies:
+                    enabled_strategy: Strategy = next(
+                        (x for x in self.strategies if x.execute(context)), None
+                    )
+                    if enabled_strategy is not None:
+                        strategy_result = enabled_strategy.get_result(context)
+
+                else:
+                    # If no strategies are present, should default to true. This isn't possible via UI.
+                    strategy_result = EvaluationResult(True, None)
+
+            except Exception as evaluation_except:
+                LOGGER.warning("Error getting evaluation result: %s", evaluation_except)
+
+        self.increment_stats(strategy_result.enabled)
+        LOGGER.info("%s evaluation result: %s", self.name, strategy_result)
+        return strategy_result
+
     @staticmethod
     def metrics_only_feature(feature_name: str):
         feature = Feature(feature_name, False, [])

diff --git a/UnleashClient/loader.py b/UnleashClient/loader.py
@@ -33,9 +33,15 @@ def _create_strategies(
             else:
                 segment_provisioning = []
 
+            if "variants" in strategy.keys():
+                variant_provisioning = strategy["variants"]
+            else:
+                variant_provisioning = []
+
             feature_strategies.append(
                 strategy_mapping[strategy["name"]](
                     constraints=constraint_provisioning,
+                    variants=variant_provisioning,
                     parameters=strategy_provisioning,
                     global_segments=global_segments,
                     segment_ids=segment_provisioning,

diff --git a/UnleashClient/strategies/RemoteAddress.py b/UnleashClient/strategies/RemoteAddress.py
@@ -61,9 +61,8 @@ def apply(self, context: dict = None) -> bool:
                     if context_ip == addr_or_range:
                         return_value = True
                         break
-                else:
-                    if context_ip in addr_or_range:  # noqa: PLR5501
-                        return_value = True
-                        break
+                elif context_ip in addr_or_range:  # noqa: PLR5501
+                    return_value = True
+                    break
 
         return return_value
diff --git a/UnleashClient/strategies/Strategy.py b/UnleashClient/strategies/Strategy.py
@@ -1,8 +1,16 @@
 # pylint: disable=invalid-name,dangerous-default-value
 import warnings
-from typing import Iterator
+from dataclasses import dataclass
+from typing import Iterator, Optional
 
 from UnleashClient.constraints import Constraint
+from UnleashClient.variants import Variants
+
+
+@dataclass
+class EvaluationResult:
+    enabled: bool
+    variant: Optional[dict]
 
 
 class Strategy:
@@ -15,6 +23,7 @@ class Strategy:
     - ``apply()`` - Your feature provisioning
 
     :param constraints: List of 'constraints' objects derived from strategy section (...from feature section) of `/api/clients/features` Unleash server response.
+    :param variants: List of 'variant' objects derived from strategy section (...from feature section) of `/api/clients/features` Unleash server response.
     :param parameters: The 'parameter' objects from the strategy section (...from feature section) of `/api/clients/features` Unleash server response.
     """
 
@@ -24,9 +33,11 @@ def __init__(
         parameters: dict = {},
         segment_ids: list = None,
         global_segments: dict = None,
+        variants: list = None,
     ) -> None:
         self.parameters = parameters
         self.constraints = constraints
+        self.variants = variants or []
         self.segment_ids = segment_ids or []
         self.global_segments = global_segments or {}
         self.parsed_provisioning = self.load_provisioning()
@@ -56,6 +67,15 @@ def execute(self, context: dict = None) -> bool:
 
         return flag_state
 
+    def get_result(self, context) -> EvaluationResult:
+        enabled = self.execute(context)
+        variant = None
+        if enabled:
+            variant = self.parsed_variants.get_variant(context, enabled)
+
+        result = EvaluationResult(enabled, variant)
+        return result
+
     @property
     def parsed_constraints(self) -> Iterator[Constraint]:
         for constraint_dict in self.constraints:
@@ -66,6 +86,14 @@ def parsed_constraints(self) -> Iterator[Constraint]:
             for constraint in segment["constraints"]:
                 yield Constraint(constraint_dict=constraint)
 
+    @property
+    def parsed_variants(self) -> Variants:
+        return Variants(
+            variants_list=self.variants,
+            group_id=self.parameters.get("groupId"),
+            is_feature_variants=False,
+        )
+
     def load_provisioning(self) -> list:  # pylint: disable=no-self-use
         """
         Loads strategy provisioning from Unleash feature flag configuration.

diff --git a/UnleashClient/strategies/__init__.py b/UnleashClient/strategies/__init__.py
@@ -6,5 +6,5 @@
 from .GradualRolloutSessionId import GradualRolloutSessionId
 from .GradualRolloutUserId import GradualRolloutUserId
 from .RemoteAddress import RemoteAddress
-from .Strategy import Strategy
+from .Strategy import EvaluationResult, Strategy
 from .UserWithId import UserWithId
diff --git a/UnleashClient/variants/Variants.py b/UnleashClient/variants/Variants.py
@@ -1,21 +1,24 @@
 # pylint: disable=invalid-name, too-few-public-methods
 import copy
 import random
-from typing import Dict  # noqa: F401
+from typing import Dict, Optional  # noqa: F401
 
 from UnleashClient import utils
 from UnleashClient.constants import DISABLED_VARIATION
 
 
 class Variants:
-    def __init__(self, variants_list: list, feature_name: str) -> None:
+    def __init__(
+        self, variants_list: list, group_id: str, is_feature_variants: bool = True
+    ) -> None:
         """
         Represents an A/B test
 
         variants_list = From the strategy document.
         """
         self.variants = variants_list
-        self.feature_name = feature_name
+        self.group_id = group_id
+        self.is_feature_variants = is_feature_variants
 
     def _apply_overrides(self, context: dict) -> dict:
         """
@@ -60,28 +63,31 @@ def _get_seed(context: dict, stickiness_selector: str = "default") -> str:
         return seed
 
     @staticmethod
-    def _format_variation(variation: dict) -> dict:
+    def _format_variation(variation: dict, flag_status: Optional[bool] = None) -> dict:
         formatted_variation = copy.deepcopy(variation)
         del formatted_variation["weight"]
         if "overrides" in formatted_variation:
             del formatted_variation["overrides"]
         if "stickiness" in formatted_variation:
             del formatted_variation["stickiness"]
+        if "enabled" not in formatted_variation and flag_status is not None:
+            formatted_variation["enabled"] = flag_status
         return formatted_variation
 
-    def get_variant(self, context: dict) -> dict:
+    def get_variant(self, context: dict, flag_status: Optional[bool] = None) -> dict:
         """
         Determines what variation a user is in.
 
         :param context:
+        :param flag_status:
         :return:
         """
         fallback_variant = copy.deepcopy(DISABLED_VARIATION)
 
         if self.variants:
             override_variant = self._apply_overrides(context)
             if override_variant:
-                return self._format_variation(override_variant)
+                return self._format_variation(override_variant, flag_status)
 
             total_weight = sum(x["weight"] for x in self.variants)
             if total_weight <= 0:
@@ -92,17 +98,18 @@ def get_variant(self, context: dict) -> dict:
                 if "stickiness" in self.variants[0].keys()
                 else "default"
             )
+
             target = utils.normalized_hash(
                 self._get_seed(context, stickiness_selector),
-                self.feature_name,
+                self.group_id,
                 total_weight,
             )
             counter = 0
             for variation in self.variants:
                 counter += variation["weight"]
 
                 if counter >= target:
-                    return self._format_variation(variation)
+                    return self._format_variation(variation, flag_status)
 
         # Catch all return.
         return fallback_variant
diff --git a/tests/specification_tests/test_client_specs.py b/tests/specification_tests/test_client_specs.py
@@ -27,29 +27,43 @@ def load_specs():
         return json.load(_f)
 
 
+def get_client(state, test_context=None):
+    cache = FileCache("MOCK_CACHE")
+    cache.bootstrap_from_dict(state)
+    env = "default"
+    if test_context is not None and "environment" in test_context:
+        env = test_context["environment"]
+
+    unleash_client = UnleashClient(
+        url=URL,
+        app_name=APP_NAME,
+        instance_id="pytest_%s" % uuid.uuid4(),
+        disable_metrics=True,
+        disable_registration=True,
+        cache=cache,
+        environment=env,
+    )
+
+    unleash_client.initialize_client(fetch_toggles=False)
+    return unleash_client
+
+
 def iter_spec():
     for spec in load_specs():
         name, state, tests, variant_tests = load_spec(spec)
 
-        cache = FileCache("MOCK_CACHE")
-        cache.bootstrap_from_dict(state)
-
-        unleash_client = UnleashClient(
-            url=URL,
-            app_name=APP_NAME,
-            instance_id="pytest_%s" % uuid.uuid4(),
-            disable_metrics=True,
-            disable_registration=True,
-            cache=cache,
-        )
-
-        unleash_client.initialize_client(fetch_toggles=False)
+        unleash_client = get_client(state=state)
 
         for test in tests:
             yield name, test["description"], unleash_client, test, False
 
         for variant_test in variant_tests:
-            yield name, test["description"], unleash_client, variant_test, True
+            test_context = {}
+            if "context" in variant_test:
+                test_context = variant_test["context"]
+
+            unleash_client = get_client(state, test_context)
+            yield name, variant_test["description"], unleash_client, variant_test, True
 
         unleash_client.destroy()
 
@@ -77,6 +91,5 @@ def test_spec(spec):
         toggle_name = test_data["toggleName"]
         expected = test_data["expectedResult"]
         context = test_data.get("context")
-
         variant = unleash_client.get_variant(toggle_name, context)
         assert variant == expected