From 619c3f52a821d8eaace2035c5e34b51b16040deb Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Thu, 12 Oct 2023 09:44:26 +0100 Subject: [PATCH] fix: incorrect default_percentage_allocation on import, binary flags imported as multivariate (#2841) --- api/features/feature_types.py | 8 +- api/integrations/launch_darkly/services.py | 96 ++++++++++++++----- .../launch_darkly/test_services.py | 50 +++++++--- 3 files changed, 117 insertions(+), 37 deletions(-) diff --git a/api/features/feature_types.py b/api/features/feature_types.py index a6e54654a18a..602a40ab4fd5 100644 --- a/api/features/feature_types.py +++ b/api/features/feature_types.py @@ -1,5 +1,9 @@ -MULTIVARIATE = "MULTIVARIATE" -STANDARD = "STANDARD" +from typing import Literal + +FeatureType = Literal["STANDARD", "MULTIVARIATE"] + +MULTIVARIATE: FeatureType = "MULTIVARIATE" +STANDARD: FeatureType = "STANDARD" # the following two types have been merged in terms of functionality # but kept for now until the FE is updated diff --git a/api/integrations/launch_darkly/services.py b/api/integrations/launch_darkly/services.py index 7f655a006104..0820d98b607a 100644 --- a/api/integrations/launch_darkly/services.py +++ b/api/integrations/launch_darkly/services.py @@ -1,12 +1,12 @@ from contextlib import contextmanager -from typing import TYPE_CHECKING, Optional +from typing import TYPE_CHECKING, Callable, Tuple from django.core import signing from django.utils import timezone from requests.exceptions import HTTPError, RequestException from environments.models import Environment -from features.feature_types import MULTIVARIATE, STANDARD +from features.feature_types import MULTIVARIATE, STANDARD, FeatureType from features.models import Feature, FeatureState, FeatureStateValue from features.multivariate.models import ( MultivariateFeatureOption, @@ -102,7 +102,7 @@ def _create_tags_from_ld( return tags_by_ld_tag -def _create_standard_feature_states( +def _create_boolean_feature_states( ld_flag: ld_types.FeatureFlag, feature: Feature, environments_by_ld_environment_key: dict[str, Environment], @@ -114,9 +114,47 @@ def _create_standard_feature_states( environment=environment, defaults={"enabled": ld_flag_config["on"]}, ) + FeatureStateValue.objects.update_or_create( feature_state=feature_state, - defaults={"feature_state": feature_state}, + ) + + +def _create_string_feature_states( + ld_flag: ld_types.FeatureFlag, + feature: Feature, + environments_by_ld_environment_key: dict[str, Environment], +) -> None: + variations_by_idx = { + str(idx): variation for idx, variation in enumerate(ld_flag["variations"]) + } + + for ld_environment_key, environment in environments_by_ld_environment_key.items(): + ld_flag_config = ld_flag["environments"][ld_environment_key] + + is_flag_on = ld_flag_config["on"] + if is_flag_on: + variation_config_key = "isFallthrough" + else: + variation_config_key = "isOff" + + string_value = "" + + if ld_flag_config_summary := ld_flag_config.get("_summary"): + enabled_variations = ld_flag_config_summary.get("variations") or {} + for idx, variation_config in enabled_variations.items(): + if variation_config.get(variation_config_key): + string_value = variations_by_idx[idx]["value"] + break + + feature_state, _ = FeatureState.objects.update_or_create( + feature=feature, + environment=environment, + defaults={"enabled": is_flag_on}, + ) + FeatureStateValue.objects.update_or_create( + feature_state=feature_state, + defaults={"type": STRING, "string_value": string_value}, ) @@ -129,16 +167,15 @@ def _create_mv_feature_states( mv_feature_options_by_variation: dict[str, MultivariateFeatureOption] = {} for idx, variation in enumerate(variations): - mv_feature_options_by_variation[str(idx)] = MultivariateFeatureOption( + ( + mv_feature_options_by_variation[str(idx)], + _, + ) = MultivariateFeatureOption.objects.update_or_create( feature=feature, - type=STRING, string_value=variation["value"], + defaults={"default_percentage_allocation": 0, "type": STRING}, ) - MultivariateFeatureOption.objects.bulk_create( - mv_feature_options_by_variation.values(), - ) - for ld_environment_key, environment in environments_by_ld_environment_key.items(): ld_flag_config = ld_flag["environments"][ld_environment_key] feature_state, _ = FeatureState.objects.update_or_create( @@ -178,16 +215,38 @@ def _create_mv_feature_states( ) +def _get_feature_type_and_feature_state_factory( + ld_flag: ld_types.FeatureFlag, +) -> Tuple[ + FeatureType, + Callable[[ld_types.FeatureFlag, Feature, dict[str, Environment]], None], +]: + match ld_flag["kind"]: + case "multivariate" if len(ld_flag["variations"]) > 2: + feature_type = MULTIVARIATE + feature_state_factory = _create_mv_feature_states + case "multivariate": + feature_type = STANDARD + feature_state_factory = _create_string_feature_states + case _: # assume boolean + feature_type = STANDARD + feature_state_factory = _create_boolean_feature_states + + return feature_type, feature_state_factory + + def _create_feature_from_ld( ld_flag: ld_types.FeatureFlag, environments_by_ld_environment_key: dict[str, Environment], tags_by_ld_tag: dict[str, Tag], project_id: int, ) -> Feature: - feature_type, feature_state_factory = { - "boolean": (STANDARD, _create_standard_feature_states), - "multivariate": (MULTIVARIATE, _create_mv_feature_states), - }[ld_flag["kind"]] + ( + feature_type, + feature_state_factory, + ) = _get_feature_type_and_feature_state_factory( + ld_flag, + ) tags = [ tags_by_ld_tag[LAUNCH_DARKLY_IMPORTED_DEFAULT_TAG_LABEL], @@ -232,15 +291,6 @@ def _create_features_from_ld( ] -def get_import_request( - project: "Project", ld_project_key: str -) -> Optional[LaunchDarklyImportRequest]: - return LaunchDarklyImportRequest.objects.get( - project=project, - ld_project_key=ld_project_key, - ) - - def create_import_request( project: "Project", user: "FFAdminUser", diff --git a/api/tests/unit/integrations/launch_darkly/test_services.py b/api/tests/unit/integrations/launch_darkly/test_services.py index 96173622e998..46d4fd8ebb06 100644 --- a/api/tests/unit/integrations/launch_darkly/test_services.py +++ b/api/tests/unit/integrations/launch_darkly/test_services.py @@ -137,19 +137,45 @@ def test_process_import_request__success__expected_status( boolean_standard_feature_states_by_env_name["Test"].enabled is True boolean_standard_feature_states_by_env_name["Production"].enabled is False - value_standard_feature = Feature.objects.get(project=project, name="flag2_value") - value_standard_feature_states_by_env_name = { + string_standard_feature = Feature.objects.get(project=project, name="flag2_value") + string_standard_feature_states_by_env_name = { fs.environment.name: fs - for fs in FeatureState.objects.filter(feature=value_standard_feature) + for fs in FeatureState.objects.filter(feature=string_standard_feature) } - value_standard_feature_states_by_env_name["Test"].enabled is True - value_standard_feature_states_by_env_name[ - "Test" - ].get_feature_state_value() == "123123" - value_standard_feature_states_by_env_name["Production"].enabled is False - value_standard_feature_states_by_env_name[ - "Production" - ].get_feature_state_value() == "" + assert string_standard_feature_states_by_env_name["Test"].enabled is True + assert ( + string_standard_feature_states_by_env_name["Test"].get_feature_state_value() + == "123123" + ) + assert ( + string_standard_feature_states_by_env_name["Test"].feature_state_value.type + == "unicode" + ) + assert ( + string_standard_feature_states_by_env_name[ + "Test" + ].feature_state_value.string_value + == "123123" + ) + assert string_standard_feature_states_by_env_name["Production"].enabled is False + assert ( + string_standard_feature_states_by_env_name[ + "Production" + ].get_feature_state_value() + == "" + ) + assert ( + string_standard_feature_states_by_env_name[ + "Production" + ].feature_state_value.type + == "unicode" + ) + assert ( + string_standard_feature_states_by_env_name[ + "Production" + ].feature_state_value.string_value + == "" + ) # Multivariate feature states with percentage rollout have expected values. percentage_mv_feature = Feature.objects.get( @@ -168,7 +194,7 @@ def test_process_import_request__success__expected_status( "multivariate_feature_option__string_value", "percentage_allocation", ) - ) == [("variation1", 100), ("variation2", 0)] + ) == [("variation1", 100), ("variation2", 0), ("variation3", 0)] assert percentage_mv_feature_states_by_env_name["Production"].enabled is True assert list(