diff --git a/api/evaluation/mappers.py b/api/evaluation/mappers.py index 1dd7424d4ecb..f80aa0856c6c 100644 --- a/api/evaluation/mappers.py +++ b/api/evaluation/mappers.py @@ -22,6 +22,7 @@ IdentityContext, SegmentContext, ) +from features.dependencies.mappers import map_override_to_value_source from features.types import FeatureEngineMetadata from segments.types import SegmentEngineMetadata @@ -137,19 +138,33 @@ def to_feature_context( feature_state: "FeatureState", *, priority: float | None = None, + value_source: "FeatureState | None" = None, ) -> FeatureContext: return map_feature_state_to_feature_context( feature_state, - mv_fs_values=mv_fs_values_by_feature_state_id.get(feature_state.pk), + mv_fs_values=mv_fs_values_by_feature_state_id.get( + (value_source or feature_state).pk + ), priority=priority, + value_source=value_source, ) if segments is not None: + environment_defaults = { + feature_state.feature_id: feature_state for feature_state in feature_states + } context["segments"] = { str(segment.pk): map_segment_to_segment_context( segment, overrides=[ - to_feature_context(feature_state) + to_feature_context( + feature_state, + value_source=map_override_to_value_source( + feature_state, + segment=segment, + environment_defaults=environment_defaults, + ), + ) for feature_state in segment_overrides.get(segment.pk) or () ], ) @@ -301,20 +316,27 @@ def map_feature_state_to_feature_context( *, mv_fs_values: "Iterable[MultivariateFeatureStateValue] | None" = None, priority: float | None = None, + value_source: "FeatureState | None" = None, ) -> FeatureContext: - """Map a Django ORM FeatureState to a flag-engine FeatureContext TypedDict.""" + """Map a Django ORM FeatureState to a flag-engine FeatureContext TypedDict. + + :param mv_fs_values: the variants of `value_source`, if given. + :param value_source: the feature state to serve the value and variants + from, if not `feature_state` itself. + """ feature = feature_state.feature + value_source = value_source or feature_state feature_context: FeatureContext = { # The engine seeds multivariate variant allocation on the feature # context key, so it has to be the bucketing seed rather than the # feature state id, or recreating a feature state would move every # enrolled identity to a different variant. See issue #7913. - "key": str(feature_state.mv_hashing_seed), + "key": str(value_source.mv_hashing_seed), "name": feature.name, "enabled": feature_state.enabled, # Deliberately unparameterised by identity: picking a multivariate # value is the engine's job now. - "value": feature_state.get_feature_state_value(), + "value": value_source.get_feature_state_value(), "metadata": FeatureEngineMetadata(feature_state=feature_state), } diff --git a/api/features/dependencies/mappers.py b/api/features/dependencies/mappers.py index 4d79436bfab2..82f6a498333b 100644 --- a/api/features/dependencies/mappers.py +++ b/api/features/dependencies/mappers.py @@ -1,3 +1,5 @@ +from collections.abc import Mapping + import jsonpath_rfc9535 from jsonpath_rfc9535.exceptions import JSONPathError from jsonpath_rfc9535.segments import JSONPathChildSegment, JSONPathSegment @@ -9,12 +11,35 @@ FeatureName, ReferencingSegment, ) -from features.models import Feature +from features.models import Feature, FeatureState +from segments.models import Segment, SegmentManagedBy from segments.types import SegmentRule _JSONPathStr = str +def map_override_to_value_source( + override: FeatureState, + *, + segment: Segment, + environment_defaults: Mapping[int, FeatureState], +) -> FeatureState: + """Return the feature state to serve the segment override's value from. + + A dependency override only disables its feature, so its value, variants + and variant bucketing follow the feature's environment default. Every + other override serves its own. + + :param environment_defaults: the environment defaults, by feature id. + """ + if ( + segment.managed_by == SegmentManagedBy.DEPENDENCY + and segment.feature_id == override.feature_id + ): + return environment_defaults.get(override.feature_id, override) + return override + + def map_rules_to_prerequisite_feature_names( rules: list[SegmentRule], ) -> dict[_JSONPathStr, FeatureName]: diff --git a/api/tests/integration/environments/identities/test_integration_identities.py b/api/tests/integration/environments/identities/test_integration_identities.py index 216f64018a15..9706c8c17c91 100644 --- a/api/tests/integration/environments/identities/test_integration_identities.py +++ b/api/tests/integration/environments/identities/test_integration_identities.py @@ -10,6 +10,7 @@ from rest_framework.test import APIClient from features.feature_types import MULTIVARIATE +from features.future.types import UpdateFlagRequest from tests.integration.helpers import ( create_feature_with_api, create_mv_option_with_api, @@ -732,3 +733,58 @@ def test_identify_user__prerequisite__serves_dependent_accordingly( assert { flag["feature"]["name"]: flag["enabled"] for flag in response.json()["flags"] } == {"payments": payments_enabled, "checkout": payments_enabled} + + +def test_identify_user__dependent_default_value_changed__serves_current_value( + admin_client: APIClient, + sdk_client: APIClient, + project: int, + environment: int, + environment_api_key: str, + identity_identifier: str, +) -> None: + # Given + chicken_id = create_feature_with_api( + client=admin_client, + project_id=project, + feature_name="chicken", + initial_value="", + ) + chicken_response = admin_client.patch( + f"/api/__future__/environments/{environment_api_key}/features/{chicken_id}/", + data=UpdateFlagRequest({"environment_default": {"enabled": False}}), + format="json", + ) + assert chicken_response.status_code == status.HTTP_200_OK + egg_id = create_feature_with_api( + client=admin_client, + project_id=project, + feature_name="egg", + initial_value="wild egg", + ) + dependency_response = admin_client.post( + f"/api/v1/environments/{environment_api_key}/features/{egg_id}/dependencies/{chicken_id}/", + ) + assert dependency_response.status_code == status.HTTP_201_CREATED + egg_response = admin_client.patch( + f"/api/__future__/environments/{environment_api_key}/features/{egg_id}/", + data=UpdateFlagRequest( + {"environment_default": {"value": {"type": "string", "value": "farm egg"}}} + ), + format="json", + ) + assert egg_response.status_code == status.HTTP_200_OK + + # When + response = sdk_client.post( + "/api/v1/identities/", + data={"identifier": identity_identifier, "traits": []}, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_200_OK + assert { + flag["feature"]["name"]: (flag["enabled"], flag["feature_state_value"]) + for flag in response.json()["flags"] + } == {"chicken": (False, ""), "egg": (False, "farm egg")} diff --git a/api/tests/unit/evaluation/test_unit_evaluation_mappers.py b/api/tests/unit/evaluation/test_unit_evaluation_mappers.py index aca4da998323..0a6bd0a8d810 100644 --- a/api/tests/unit/evaluation/test_unit_evaluation_mappers.py +++ b/api/tests/unit/evaluation/test_unit_evaluation_mappers.py @@ -17,7 +17,7 @@ from features.models import Feature, FeatureSegment, FeatureState from features.multivariate.models import MultivariateFeatureStateValue from projects.models import Project -from segments.models import Condition, Segment, SegmentRule +from segments.models import Condition, Segment, SegmentManagedBy, SegmentRule from util.engine_models.features.models import ( FeatureModel, FeatureStateModel, @@ -541,3 +541,60 @@ def test_map_engine_feature_state_to_feature_context__multivariate_override__ret ], "metadata": {"edge_feature_state": feature_state}, } + + +def test_map_environment_to_evaluation_context__dependency_override__serves_environment_default( + environment: Environment, + identity: Identity, + multivariate_feature: Feature, +) -> None: + # Given + environment_default = FeatureState.objects.get( + environment=environment, + feature=multivariate_feature, + identity=None, + feature_segment=None, + ) + environment_default.mv_hashing_salt = 999 + environment_default.save() + segment = Segment.objects.create( + name="feature-depends-on-prerequisite", + project=multivariate_feature.project, + feature=multivariate_feature, + is_system_segment=True, + managed_by=SegmentManagedBy.DEPENDENCY, + ) + override = FeatureState.objects.create( + feature=multivariate_feature, + environment=environment, + enabled=False, + feature_segment=FeatureSegment.objects.create( + feature=multivariate_feature, segment=segment, environment=environment + ), + ) + override.feature_state_value.string_value = "stale" + override.feature_state_value.save() + MultivariateFeatureStateValue.objects.filter(feature_state=override).update( + percentage_allocation=0 + ) + + # When + context = map_environment_to_evaluation_context( + environment=environment, + identity=identity, + segments=[segment], + ) + + # Then + environment_default_context = context["features"][multivariate_feature.name] + assert context["segments"][str(segment.pk)]["overrides"] == [ + { + "key": "999", + "name": multivariate_feature.name, + "enabled": False, + "value": "control", + "variants": environment_default_context["variants"], + "priority": override.feature_segment.priority, + "metadata": {"feature_state": override}, + } + ] diff --git a/api/tests/unit/features/dependencies/test_mappers.py b/api/tests/unit/features/dependencies/test_mappers.py index 1ff222affbfc..86653e4be05c 100644 --- a/api/tests/unit/features/dependencies/test_mappers.py +++ b/api/tests/unit/features/dependencies/test_mappers.py @@ -1,6 +1,11 @@ import pytest -from features.dependencies.mappers import map_rules_to_prerequisite_feature_names +from features.dependencies.mappers import ( + map_override_to_value_source, + map_rules_to_prerequisite_feature_names, +) +from features.models import FeatureState +from segments.models import Segment, SegmentManagedBy from segments.types import SegmentRule @@ -38,3 +43,54 @@ def test_map_rules_to_prerequisite_feature_names__invalid_prerequisite_jsonpath_ # Then assert feature_names_by_json_path == {} + + +@pytest.mark.parametrize( + "segment, environment_defaults, expected_value_source", + [ + pytest.param( + Segment(feature_id=1, managed_by=SegmentManagedBy.DEPENDENCY), + {1: FeatureState(id=1, feature_id=1)}, + FeatureState(id=1, feature_id=1), + id="dependency_segment__environment_default", + ), + pytest.param( + Segment(feature_id=1, managed_by=SegmentManagedBy.DEPENDENCY), + {}, + FeatureState(id=2, feature_id=1), + id="dependency_segment_without_environment_default__override", + ), + pytest.param( + Segment(feature_id=None), + {1: FeatureState(id=1, feature_id=1)}, + FeatureState(id=2, feature_id=1), + id="user_segment__override", + ), + pytest.param( + Segment(feature_id=1, managed_by=SegmentManagedBy.EXPERIMENT), + {1: FeatureState(id=1, feature_id=1)}, + FeatureState(id=2, feature_id=1), + id="experiment_segment__override", + ), + pytest.param( + Segment(feature_id=1, managed_by=SegmentManagedBy.RELEASE_PIPELINE), + {1: FeatureState(id=1, feature_id=1)}, + FeatureState(id=2, feature_id=1), + id="release_pipeline_segment__override", + ), + ], +) +def test_map_override_to_value_source__segment_override__returns_expected_value_source( + segment: Segment, + environment_defaults: dict[int, FeatureState], + expected_value_source: FeatureState, +) -> None: + # Given / When + value_source = map_override_to_value_source( + FeatureState(id=2, feature_id=1), + segment=segment, + environment_defaults=environment_defaults, + ) + + # Then + assert value_source == expected_value_source diff --git a/api/tests/unit/util/mappers/test_unit_mappers_engine.py b/api/tests/unit/util/mappers/test_unit_mappers_engine.py index 5f1f0a96d0ed..e87bbc9e90a4 100644 --- a/api/tests/unit/util/mappers/test_unit_mappers_engine.py +++ b/api/tests/unit/util/mappers/test_unit_mappers_engine.py @@ -11,6 +11,7 @@ from environments.models import Environment from experimentation.models import Experiment, ExperimentStatus from features.models import Feature, FeatureSegment, FeatureState +from features.multivariate.models import MultivariateFeatureStateValue from features.versioning.models import EnvironmentFeatureVersion from features.versioning.tasks import enable_v2_versioning from integrations.common.models import IntegrationsModel @@ -18,7 +19,7 @@ from integrations.mixpanel.models import MixpanelConfiguration from integrations.segment.models import SegmentConfiguration from integrations.webhook.models import WebhookConfiguration -from segments.models import Segment, SegmentRule +from segments.models import Segment, SegmentManagedBy, SegmentRule from users.models import FFAdminUser from util.engine_models.environments.integrations.models import IntegrationModel from util.engine_models.environments.models import ( @@ -912,3 +913,54 @@ def test_map_environment_to_engine__experiment_without_rollout_segment__no_enrol for segment in result.project.segments for fs in segment.feature_states ) + + +def test_map_environment_to_engine__dependency_override__serves_environment_default( + environment: Environment, + multivariate_feature: "Feature", +) -> None: + # Given + environment_default = FeatureState.objects.get( + environment=environment, + feature=multivariate_feature, + identity=None, + feature_segment=None, + ) + environment_default.mv_hashing_salt = 999 + environment_default.save() + segment = Segment.objects.create( + name="feature-depends-on-prerequisite", + project=multivariate_feature.project, + feature=multivariate_feature, + is_system_segment=True, + managed_by=SegmentManagedBy.DEPENDENCY, + ) + override = FeatureState.objects.create( + feature=multivariate_feature, + environment=environment, + enabled=False, + feature_segment=FeatureSegment.objects.create( + feature=multivariate_feature, segment=segment, environment=environment + ), + ) + override.feature_state_value.string_value = "stale" + override.feature_state_value.save() + MultivariateFeatureStateValue.objects.filter(feature_state=override).update( + percentage_allocation=0 + ) + + # When + result = engine.map_environment_to_engine(environment) + + # Then + [environment_default_model] = result.feature_states + [segment_model] = result.project.segments + [override_model] = segment_model.feature_states + assert override_model.featurestate_uuid == override.uuid + assert override_model.enabled is False + assert override_model.django_id == 999 + assert override_model.feature_state_value == "control" + assert ( + override_model.multivariate_feature_state_values + == environment_default_model.multivariate_feature_state_values + ) diff --git a/api/util/mappers/engine.py b/api/util/mappers/engine.py index 77ffc7a6bb02..d8715523bb7e 100644 --- a/api/util/mappers/engine.py +++ b/api/util/mappers/engine.py @@ -4,6 +4,7 @@ from uuid import UUID from environments.constants import IDENTITY_INTEGRATIONS_RELATION_NAMES +from features.dependencies.mappers import map_override_to_value_source from features.versioning.models import EnvironmentFeatureVersion from util.engine_models.environments.integrations.models import IntegrationModel from util.engine_models.environments.models import ( @@ -131,8 +132,15 @@ def map_feature_state_to_engine( *, mv_fs_values: Optional[Iterable["MultivariateFeatureStateValue"]] = None, metadata: Optional[dict[str, object]] = None, + value_source: Optional["FeatureState"] = None, ) -> FeatureStateModel: + """ + :param mv_fs_values: the variants of `value_source`, if given. + :param value_source: the feature state to serve the value and variants + from, if not `feature_state` itself. + """ feature = feature_state.feature + value_source = value_source or feature_state feature_segment: Optional["FeatureSegment"] = feature_state.feature_segment if feature_segment: @@ -149,8 +157,8 @@ def map_feature_state_to_engine( # so feeding it the bucketing seed keeps variant assignment stable when # a feature state is recreated, without changing the engine model or # environment document schema. See issue #7913. - django_id=feature_state.mv_hashing_seed, - feature_state_value=feature_state.get_feature_state_value(), + django_id=value_source.mv_hashing_seed, + feature_state_value=value_source.get_feature_state_value(), featurestate_uuid=feature_state.uuid, feature_segment=feature_segment_model, feature=map_feature_to_engine(feature), @@ -260,6 +268,11 @@ def map_environment_to_engine( for feature_state in all_environment_feature_states } + environment_defaults = { + feature_state.feature_id: feature_state + for feature_state in environment_feature_states + } + # Read integrations. integration_configs: dict[ str, "EnvironmentIntegrationModel | WebhookConfiguration | None" @@ -273,6 +286,25 @@ def map_environment_to_engine( # No reading from ORM past this point! # Prepare relationships. + def map_segment_override_to_engine( + feature_state: "FeatureState", + *, + segment: "Segment", + ) -> FeatureStateModel: + value_source = map_override_to_value_source( + feature_state, + segment=segment, + environment_defaults=environment_defaults, + ) + return map_feature_state_to_engine( + feature_state, + mv_fs_values=multivariate_feature_state_values_by_feature_state_id[ + value_source.pk + ], + metadata=get_feature_state_metadata(feature_state), + value_source=value_source, + ) + organisation_model = OrganisationModel( id=organisation.pk, name=organisation.name, @@ -289,13 +321,7 @@ def map_environment_to_engine( for segment_rule in project_segment_rules_by_segment_id.pop(segment.pk) ], feature_states=[ - map_feature_state_to_engine( - feature_state, - mv_fs_values=multivariate_feature_state_values_by_feature_state_id.pop( - feature_state.pk, - ), - metadata=get_feature_state_metadata(feature_state), - ) + map_segment_override_to_engine(feature_state, segment=segment) for feature_state in project_segment_feature_states_by_segment_id.pop( segment.pk )