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.

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

Dependent features #274

Merged
merged 15 commits into from
Oct 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
59 changes: 58 additions & 1 deletion UnleashClient/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,9 @@ def is_enabled(
if self.unleash_bootstrapped or self.is_initialized:
try:
feature = self.features[feature_name]
feature_check = feature.is_enabled(context)
feature_check = feature.is_enabled(
context
) and self._dependencies_are_satisfied(feature_name, context)

if feature.only_for_metrics:
return self._get_fallback_value(
Expand Down Expand Up @@ -429,6 +431,10 @@ def get_variant(self, feature_name: str, context: Optional[dict] = None) -> dict
if self.unleash_bootstrapped or self.is_initialized:
try:
feature = self.features[feature_name]

if not self._dependencies_are_satisfied(feature_name, context):
return DISABLED_VARIATION

variant_check = feature.get_variant(context)

if self.unleash_event_callback and feature.impression_data:
Expand Down Expand Up @@ -484,6 +490,57 @@ def get_variant(self, feature_name: str, context: Optional[dict] = None) -> dict
)
return DISABLED_VARIATION

def _is_dependency_satified(self, dependency: dict, context: dict) -> bool:
"""
Checks a single feature dependency.
"""

dependency_name = dependency["feature"]

dependency_feature = self.features[dependency_name]

if not dependency_feature:
LOGGER.warning("Feature dependency not found. %s", dependency_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

in Node we have a mechanism to only log this message once. I'm assuming we don't do the same thing in Python SDK

Copy link
Member

Choose a reason for hiding this comment

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

Not a thing in the Python SDK at the moment, unfortunately. Not against adding it, but that should probably be a different PR

return False

if dependency_feature.dependencies:
LOGGER.warning(
"Feature dependency cannot have it's own dependencies. %s",
dependency_name,
)
return False

should_be_enabled = dependency.get("enabled", True)
is_enabled = dependency_feature.is_enabled(context, skip_stats=True)

if is_enabled != should_be_enabled:
return False

variants = dependency.get("variants")
if variants:
variant = dependency_feature.get_variant(context, skip_stats=True)
if variant["name"] not in variants:
return False

return True

def _dependencies_are_satisfied(self, feature_name: str, context: dict) -> bool:
"""
If feature dependencies are satisfied (or non-existent).
"""

feature = self.features[feature_name]
dependencies = feature.dependencies

if not dependencies:
return True

for dependency in dependencies:
if not self._is_dependency_satified(dependency, context):
return False

return True

def _do_instance_check(self, multiple_instance_mode):
identifier = self.__get_identifier()
if identifier in INSTANCES:
Expand Down
2 changes: 1 addition & 1 deletion UnleashClient/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
REQUEST_TIMEOUT = 30
REQUEST_RETRIES = 3
METRIC_LAST_SENT_TIME = "mlst"
CLIENT_SPEC_VERSION = "4.3.1"
CLIENT_SPEC_VERSION = "4.5.0"

# =Unleash=
APPLICATION_HEADERS = {
Expand Down
24 changes: 17 additions & 7 deletions UnleashClient/features/Feature.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ def __init__(
strategies: list,
variants: Optional[Variants] = None,
impression_data: bool = False,
dependencies: list = None,
) -> None:
"""
A representation of a feature object
Expand Down Expand Up @@ -44,6 +45,12 @@ def __init__(
# Whether the feature exists only for tracking metrics or not.
self.only_for_metrics = False

# Prerequisite state of other features that this feature depends on
self.dependencies = [
dict(dependency, enabled=dependency.get("enabled", True))
for dependency in dependencies or []
]

def reset_stats(self) -> None:
"""
Resets stats after metrics reporting
Expand Down Expand Up @@ -75,20 +82,20 @@ 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) -> bool:
def is_enabled(self, context: dict = None, skip_stats: bool = False) -> bool:
Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a more elegant way to do it? Without passing skip_stats around.

Copy link
Member

Choose a reason for hiding this comment

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

Can't think of a way without rearchitecting this a little. It should be possible to move the count metrics methods to public ones and only call those in the is_enabled in the UnleashClient but... I'm not sure that's worth it. This is all internal at the moment and when Yggdrasil arrives here this will go away anyway.

I'm okay with this as it stands. I think we can do better but I also don't think it's worth it to do so. I think this is a reasonable tradeoff

"""
Checks if feature is enabled.

:param context: Context information
:return:
"""
evaluation_result = self._get_evaluation_result(context)
evaluation_result = self._get_evaluation_result(context, skip_stats)

flag_value = evaluation_result.enabled

return flag_value

def get_variant(self, context: dict = None) -> dict:
def get_variant(self, context: dict = None, skip_stats: bool = False) -> dict:
"""
Checks if feature is enabled and, if so, get the variant.

Expand All @@ -109,11 +116,13 @@ def get_variant(self, context: dict = None) -> dict:

except Exception as variant_exception:
LOGGER.warning("Error selecting variant: %s", variant_exception)

self._count_variant(cast(str, variant["name"]))
if not skip_stats:
self._count_variant(cast(str, variant["name"]))
return variant

def _get_evaluation_result(self, context: dict = None) -> EvaluationResult:
def _get_evaluation_result(
self, context: dict = None, skip_stats: bool = False
) -> EvaluationResult:
strategy_result = EvaluationResult(False, None)
if self.enabled:
try:
Expand All @@ -131,7 +140,8 @@ def _get_evaluation_result(self, context: dict = None) -> EvaluationResult:
except Exception as evaluation_except:
LOGGER.warning("Error getting evaluation result: %s", evaluation_except)

self.increment_stats(strategy_result.enabled)
if not skip_stats:
self.increment_stats(strategy_result.enabled)
LOGGER.info("%s evaluation result: %s", self.name, strategy_result)
return strategy_result

Expand Down
5 changes: 5 additions & 0 deletions UnleashClient/loader.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ def _create_feature(
strategies=parsed_strategies,
variants=variant,
impression_data=provisioning.get("impressionData", False),
dependencies=provisioning.get("dependencies", []),
)


Expand Down Expand Up @@ -151,6 +152,10 @@ def load_features(
"impressionData", False
)

feature_for_update.dependencies = parsed_features[feature].get(
"dependencies", []
)

# If the feature had previously been added to the features list only for
# tracking, indicate that it is now a real feature that should be
# evaluated properly.
Expand Down
47 changes: 47 additions & 0 deletions tests/unit_tests/test_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from tests.utilities.mocks.mock_features import (
MOCK_FEATURE_RESPONSE,
MOCK_FEATURE_RESPONSE_PROJECT,
MOCK_FEATURE_WITH_DEPENDENCIES_RESPONSE,
)
from tests.utilities.testing_constants import (
APP_NAME,
Expand Down Expand Up @@ -121,6 +122,22 @@ def unleash_client_toggle_only(cache):
unleash_client.destroy()


@pytest.fixture()
def unleash_client_bootstrap_dependencies():
cache = FileCache("MOCK_CACHE")
cache.bootstrap_from_dict(MOCK_FEATURE_WITH_DEPENDENCIES_RESPONSE)
unleash_client = UnleashClient(
url=URL,
app_name=APP_NAME,
disable_metrics=True,
disable_registration=True,
cache=cache,
environment="default",
)
unleash_client.initialize_client(fetch_toggles=False)
yield unleash_client


def test_UC_initialize_default():
client = UnleashClient(URL, APP_NAME)
assert client.unleash_url == URL
Expand Down Expand Up @@ -355,6 +372,15 @@ def test_uc_not_initialized_isenabled():
)


def test_uc_dependency(unleash_client_bootstrap_dependencies):
unleash_client = unleash_client_bootstrap_dependencies
assert unleash_client.is_enabled("Child")
assert not unleash_client.is_enabled("WithDisabledDependency")
assert unleash_client.is_enabled("ComplexExample")
assert not unleash_client.is_enabled("UnlistedDependency")
assert not unleash_client.is_enabled("TransitiveDependency")


@responses.activate
def test_uc_get_variant():
# Set up API
Expand Down Expand Up @@ -431,6 +457,27 @@ def test_uc_registers_metrics_for_nonexistent_features(unleash_client):
assert request["bucket"]["toggles"]["nonexistent-flag"]["no"] == 1


@responses.activate
def test_uc_metrics_dependencies(unleash_client):
responses.add(responses.POST, URL + REGISTER_URL, json={}, status=202)
responses.add(
responses.GET,
URL + FEATURES_URL,
json=MOCK_FEATURE_WITH_DEPENDENCIES_RESPONSE,
status=200,
)
responses.add(responses.POST, URL + METRICS_URL, json={}, status=202)

unleash_client.initialize_client()
time.sleep(1)
assert unleash_client.is_enabled("Child")

time.sleep(12)
request = json.loads(responses.calls[-1].request.body)
assert request["bucket"]["toggles"]["Child"]["yes"] == 1
assert "Parent" not in request["bucket"]["toggles"]


@responses.activate
def test_uc_registers_variant_metrics_for_nonexistent_features(unleash_client):
# Set up API
Expand Down
23 changes: 23 additions & 0 deletions tests/unit_tests/test_features.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,18 @@ def test_feature_strategy_variants():
yield Feature("My Feature", True, strategies, variants)


@pytest.fixture()
def test_feature_dependencies():
strategies = [Default()]
variants = Variants(VARIANTS, "My Feature")
dependencies = [
{"feature": "prerequisite"},
{"feature": "disabledDependency", "enabled": False},
{"feature": "withVariants", "variants": ["VarA", "VarB"]},
]
yield Feature("My Feature", True, strategies, variants, dependencies=dependencies)


def test_create_feature_true(test_feature):
my_feature = test_feature

Expand Down Expand Up @@ -140,3 +152,14 @@ def test_strategy_variant_is_returned(test_feature_strategy_variants):
"name": "VarB",
"payload": {"type": "string", "value": "Test 2"},
}


def test_dependencies(test_feature_dependencies):
assert isinstance(test_feature_dependencies.dependencies, list)
assert all(
isinstance(item, dict) for item in test_feature_dependencies.dependencies
)
assert all("feature" in item for item in test_feature_dependencies.dependencies)
assert all("enabled" in item for item in test_feature_dependencies.dependencies)
# if no enabled key is provided, it should default to True
assert test_feature_dependencies.dependencies[0]["enabled"]
94 changes: 94 additions & 0 deletions tests/utilities/mocks/mock_features.py
Original file line number Diff line number Diff line change
Expand Up @@ -158,3 +158,97 @@
}
],
}

MOCK_FEATURE_WITH_DEPENDENCIES_RESPONSE = {
"version": 1,
"features": [
{
"name": "Parent",
"description": "Dependency on Child feature toggle",
"enabled": True,
"strategies": [
{
"name": "default",
"parameters": {},
"variants": [
{
"name": "variant1",
"weight": 1000,
"stickiness": "default",
"weightType": "variable",
}
],
}
],
"createdAt": "2018-10-09T06:04:05.667Z",
"impressionData": False,
},
{
"name": "Child",
"description": "Feature toggle that depends on Parent feature toggle",
"enabled": True,
"strategies": [{"name": "default", "parameters": {}}],
"createdAt": "2018-10-09T06:04:05.667Z",
"impressionData": False,
"dependencies": [
{
"feature": "Parent",
}
],
},
{
"name": "Disabled",
"description": "Disabled feature toggle",
"enabled": False,
"strategies": [{"name": "default", "parameters": {}}],
"createdAt": "2023-10-06T11:53:02.161Z",
"impressionData": False,
},
{
"name": "WithDisabledDependency",
"description": "Feature toggle that depends on Parent feature toggle",
"enabled": True,
"strategies": [{"name": "default", "parameters": {}}],
"createdAt": "2023-10-06T12:04:05.667Z",
"impressionData": False,
"dependencies": [
{
"feature": "Disabled",
}
],
},
{
"name": "ComplexExample",
"description": "Feature toggle that depends on multiple feature toggles",
"enabled": True,
"strategies": [{"name": "default", "parameters": {}}],
"createdAt": "2023-10-06T12:04:05.667Z",
"impressionData": False,
"dependencies": [
{"feature": "Parent", "variants": ["variant1"]},
{
"feature": "Disabled",
"enabled": False,
},
],
},
{
"name": "UnlistedDependency",
"description": "Feature toggle that depends on a feature toggle that does not exist",
"enabled": True,
"strategies": [{"name": "default", "parameters": {}}],
"createdAt": "2023-10-06T12:04:05.667Z",
"impressionData": False,
"dependencies": [{"feature": "DoesNotExist"}],
},
{
"name": "TransitiveDependency",
"description": "Feature toggle that depends on a feature toggle that has a dependency",
"enabled": True,
"strategies": [{"name": "default", "parameters": {}}],
"createdAt": "2023-10-06T12:04:05.667Z",
"impressionData": False,
"dependencies": [{"feature": "Child"}],
},
],
}