From ffd2b0b2254ad12cc9422bb3dfd5d3d88dcfa0e3 Mon Sep 17 00:00:00 2001 From: TejasLamba2006 Date: Mon, 9 Nov 2026 22:10:46 +0530 Subject: [PATCH 1/3] fix(api): block multivariate option changes during live experiments Multivariate options were only restricted in the frontend. The API accepted create, update and delete on the MV options endpoint, and changed nested multivariate_options on feature update, even when the feature was in a running experiment. Reject these writes with a 400 while any environment has a running experiment for the feature. On feature update, only reject when the submitted options differ from the stored ones, so other feature fields can still be saved. Closes #8766 --- api/experimentation/feature_state_metadata.py | 9 + api/features/constants.py | 3 + api/features/multivariate/views.py | 19 +++ api/features/serializers.py | 31 ++++ .../test_unit_multivariate_live_experiment.py | 158 ++++++++++++++++++ 5 files changed, 220 insertions(+) create mode 100644 api/tests/unit/features/multivariate/test_unit_multivariate_live_experiment.py diff --git a/api/experimentation/feature_state_metadata.py b/api/experimentation/feature_state_metadata.py index b0af6cddfcbd..1459903017fd 100644 --- a/api/experimentation/feature_state_metadata.py +++ b/api/experimentation/feature_state_metadata.py @@ -9,6 +9,15 @@ from features.models import FeatureState +def has_live_experiment(feature_id: int) -> bool: + # Multivariate options belong to the feature, not an environment, so an + # experiment running in any environment locks them. + return Experiment.objects.filter( + feature_id=feature_id, + status=ExperimentStatus.RUNNING, + ).exists() + + def get_feature_state_metadata_builder( environment: "Environment", ) -> Callable[["FeatureState"], dict[str, Any] | None]: diff --git a/api/features/constants.py b/api/features/constants.py index ad2008ce00fa..37626469dfec 100644 --- a/api/features/constants.py +++ b/api/features/constants.py @@ -10,6 +10,9 @@ # Multivariate variant reported when an identity falls through to the control value CONTROL_VARIANT_KEY = "control" RESERVED_VARIANT_KEY_MESSAGE = f'"{CONTROL_VARIANT_KEY}" is a reserved variant key.' +LIVE_EXPERIMENT_MV_OPTIONS_MESSAGE = ( + "Multivariate options cannot be changed while the feature is in a live experiment." +) # Tag filtering strategy UNION = "UNION" diff --git a/api/features/multivariate/views.py b/api/features/multivariate/views.py index aecb0dee4a6d..ffb3a9c5201a 100644 --- a/api/features/multivariate/views.py +++ b/api/features/multivariate/views.py @@ -6,9 +6,12 @@ from drf_spectacular.utils import extend_schema from rest_framework import viewsets from rest_framework.decorators import api_view +from rest_framework.exceptions import ValidationError from rest_framework.generics import get_object_or_404 from rest_framework.response import Response +from experimentation.feature_state_metadata import has_live_experiment +from features.constants import LIVE_EXPERIMENT_MV_OPTIONS_MESSAGE from features.models import Feature from projects.permissions import NestedProjectPermissions @@ -72,6 +75,22 @@ def create(self, request, *args, **kwargs): # type: ignore[no-untyped-def] get_object_or_404(Feature.objects.filter(project__id=project_pk), pk=feature_pk) return super().create(request, *args, **kwargs) + def perform_create(self, serializer): # type: ignore[no-untyped-def] + self._check_no_live_experiment(serializer.validated_data["feature"]) + super().perform_create(serializer) + + def perform_update(self, serializer): # type: ignore[no-untyped-def] + self._check_no_live_experiment(serializer.instance.feature) + super().perform_update(serializer) + + def perform_destroy(self, instance): # type: ignore[no-untyped-def] + self._check_no_live_experiment(instance.feature) + super().perform_destroy(instance) + + def _check_no_live_experiment(self, feature: Feature) -> None: + if has_live_experiment(feature.id): + raise ValidationError(LIVE_EXPERIMENT_MV_OPTIONS_MESSAGE) + def get_queryset(self): # type: ignore[no-untyped-def] if getattr(self, "swagger_fake_view", False): return MultivariateFeatureOption.objects.none() diff --git a/api/features/serializers.py b/api/features/serializers.py index 19e067b9bbba..40fd5ecf1cf8 100644 --- a/api/features/serializers.py +++ b/api/features/serializers.py @@ -31,7 +31,9 @@ from evaluation.types import EvaluatedFeatureState from experimentation.feature_state_metadata import ( get_feature_state_metadata_builder, + has_live_experiment, ) +from features.constants import LIVE_EXPERIMENT_MV_OPTIONS_MESSAGE from integrations.vcs.services import dispatch_vcs_on_feature_state_changed from metadata.serializers import MetadataSerializer, MetadataSerializerMixin from projects.code_references.serializers import ( @@ -324,8 +326,37 @@ def validate_multivariate_options(self, multivariate_options): # type: ignore[n ) if total_percentage_allocation > 100: raise serializers.ValidationError("Invalid percentage allocation") + if ( + self.instance + and self._changes_multivariate_options(multivariate_options) + and has_live_experiment(self.instance.id) + ): + raise serializers.ValidationError(LIVE_EXPERIMENT_MV_OPTIONS_MESSAGE) return multivariate_options + def _changes_multivariate_options(self, multivariate_options) -> bool: # type: ignore[no-untyped-def] + # The frontend resubmits the whole feature, so only a real change to + # the options counts. Compare on content, not ids. + fields = ( + "key", + "type", + "integer_value", + "string_value", + "boolean_value", + "default_percentage_allocation", + ) + current = NestedMultivariateFeatureOptionSerializer( + self.instance.multivariate_options.all(), many=True + ).data + + def signature(options) -> list[tuple[Any, ...]]: # type: ignore[no-untyped-def] + return sorted( + (tuple(option.get(field) for field in fields) for option in options), + key=repr, + ) + + return signature(current) != signature(multivariate_options or []) + def validate_owners(self, owners: list[FFAdminUser]) -> list[FFAdminUser]: project: Project = self.context["project"] invalid_users = [ diff --git a/api/tests/unit/features/multivariate/test_unit_multivariate_live_experiment.py b/api/tests/unit/features/multivariate/test_unit_multivariate_live_experiment.py new file mode 100644 index 000000000000..dd8430839d16 --- /dev/null +++ b/api/tests/unit/features/multivariate/test_unit_multivariate_live_experiment.py @@ -0,0 +1,158 @@ +import pytest +from rest_framework import status +from rest_framework.test import APIClient + +from experimentation.models import Experiment, ExperimentStatus +from features.constants import LIVE_EXPERIMENT_MV_OPTIONS_MESSAGE +from features.models import Feature +from features.multivariate.models import MultivariateFeatureOption +from projects.models import Project + + +@pytest.fixture() +def live_experiment(environment, multivariate_feature): # type: ignore[no-untyped-def] + return Experiment.objects.create( + environment=environment, + feature=multivariate_feature, + name="live experiment", + hypothesis="new option wins", + status=ExperimentStatus.RUNNING, + ) + + +def _options_url(project: Project, feature: Feature) -> str: + return f"/api/v1/projects/{project.id}/features/{feature.id}/mv-options/" + + +def test_create_mv_option__live_experiment__returns_400( # type: ignore[no-untyped-def] + admin_client: APIClient, project, multivariate_feature, live_experiment +): + # When + response = admin_client.post( + _options_url(project, multivariate_feature), + data={ + "feature": multivariate_feature.id, + "type": "unicode", + "string_value": "new option", + "default_percentage_allocation": 0, + "key": "new", + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert LIVE_EXPERIMENT_MV_OPTIONS_MESSAGE in str(response.json()) + assert multivariate_feature.multivariate_options.count() == 3 + + +def test_update_mv_option__live_experiment__returns_400( # type: ignore[no-untyped-def] + admin_client: APIClient, project, multivariate_feature, live_experiment +): + # Given + option = multivariate_feature.multivariate_options.first() + + # When + response = admin_client.put( + f"{_options_url(project, multivariate_feature)}{option.id}/", + data={ + "feature": multivariate_feature.id, + "type": "unicode", + "string_value": "edited", + "default_percentage_allocation": option.default_percentage_allocation, + "key": option.key, + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + option.refresh_from_db() + assert option.string_value != "edited" + + +def test_delete_mv_option__live_experiment__returns_400( # type: ignore[no-untyped-def] + admin_client: APIClient, project, multivariate_feature, live_experiment +): + # Given + option = multivariate_feature.multivariate_options.first() + + # When + response = admin_client.delete( + f"{_options_url(project, multivariate_feature)}{option.id}/" + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert MultivariateFeatureOption.objects.filter(id=option.id).exists() + + +def test_create_mv_option__no_live_experiment__not_blocked_by_experiment_check( # type: ignore[no-untyped-def] + admin_client: APIClient, project, multivariate_feature +): + # When + response = admin_client.post( + _options_url(project, multivariate_feature), + data={ + "feature": multivariate_feature.id, + "type": "unicode", + "string_value": "new option", + "default_percentage_allocation": 0, + "key": "new", + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_201_CREATED + + +def test_update_feature__live_experiment__changed_mv_options_returns_400( # type: ignore[no-untyped-def] + admin_client: APIClient, project, multivariate_feature, live_experiment +): + # Given + url = f"/api/v1/projects/{project.id}/features/{multivariate_feature.id}/" + options = [ + { + "type": "unicode", + "string_value": "changed", + "default_percentage_allocation": 100, + }, + ] + + # When + response = admin_client.patch( + url, data={"multivariate_options": options}, format="json" + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert LIVE_EXPERIMENT_MV_OPTIONS_MESSAGE in str(response.json()) + + +def test_update_feature__live_experiment__unchanged_mv_options_allowed( # type: ignore[no-untyped-def] + admin_client: APIClient, project, multivariate_feature, live_experiment +): + # Given + url = f"/api/v1/projects/{project.id}/features/{multivariate_feature.id}/" + unchanged = [ + { + "type": option.type, + "string_value": option.string_value, + "default_percentage_allocation": option.default_percentage_allocation, + } + for option in multivariate_feature.multivariate_options.all() + ] + + # When + response = admin_client.patch( + url, + data={ + "description": "only metadata changed", + "multivariate_options": unchanged, + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_200_OK From 5b4a677359af2cbd8248f9f60465509bb92357e1 Mon Sep 17 00:00:00 2001 From: TejasLamba2006 Date: Mon, 9 Nov 2026 23:07:25 +0530 Subject: [PATCH 2/3] fix(api): guard multivariate options during experiments --- api/experimentation/feature_state_metadata.py | 10 ++- api/features/import_export/services.py | 14 ++++ api/features/multivariate/views.py | 10 ++- api/features/serializers.py | 34 ++++++-- .../test_unit_features_import_export_tasks.py | 53 ++++++++++++ .../test_unit_multivariate_live_experiment.py | 83 ++++++++++++++++++- 6 files changed, 190 insertions(+), 14 deletions(-) diff --git a/api/experimentation/feature_state_metadata.py b/api/experimentation/feature_state_metadata.py index 1459903017fd..2799a386dd84 100644 --- a/api/experimentation/feature_state_metadata.py +++ b/api/experimentation/feature_state_metadata.py @@ -12,10 +12,12 @@ def has_live_experiment(feature_id: int) -> bool: # Multivariate options belong to the feature, not an environment, so an # experiment running in any environment locks them. - return Experiment.objects.filter( - feature_id=feature_id, - status=ExperimentStatus.RUNNING, - ).exists() + return bool( + Experiment.objects.filter( + feature_id=feature_id, + status=ExperimentStatus.RUNNING, + ).exists() + ) def get_feature_state_metadata_builder( diff --git a/api/features/import_export/services.py b/api/features/import_export/services.py index 4e31b6f35ae6..e6ec645ef5db 100644 --- a/api/features/import_export/services.py +++ b/api/features/import_export/services.py @@ -1,6 +1,9 @@ +from django.core.exceptions import ValidationError from django.db.models import Q from environments.models import Environment +from experimentation.feature_state_metadata import has_live_experiment +from features.constants import LIVE_EXPERIMENT_MV_OPTIONS_MESSAGE from features.import_export.types import FeatureExportData from features.models import Feature, FeatureSegment, FeatureState from features.multivariate.models import MultivariateFeatureOption @@ -15,6 +18,17 @@ def overwrite_feature_for_environment( Apply a destructive feature import to a single environment without affecting other environments' feature states or the Feature definition. """ + imported_option_values = { + (mv_data["type"], mv_data["value"]) for mv_data in feature_data["multivariate"] + } + existing_option_values = { + (option.type, option.value) + for option in existing_feature.multivariate_options.all() + } + creates_option = not imported_option_values.issubset(existing_option_values) + if creates_option and has_live_experiment(existing_feature.id): + raise ValidationError(LIVE_EXPERIMENT_MV_OPTIONS_MESSAGE) + FeatureSegment.objects.filter( feature=existing_feature, environment=environment ).delete() diff --git a/api/features/multivariate/views.py b/api/features/multivariate/views.py index ffb3a9c5201a..4182fb6f0310 100644 --- a/api/features/multivariate/views.py +++ b/api/features/multivariate/views.py @@ -76,11 +76,19 @@ def create(self, request, *args, **kwargs): # type: ignore[no-untyped-def] return super().create(request, *args, **kwargs) def perform_create(self, serializer): # type: ignore[no-untyped-def] - self._check_no_live_experiment(serializer.validated_data["feature"]) + url_feature = get_object_or_404( + Feature.objects.filter(project__id=self.kwargs["project_pk"]), + pk=self.kwargs["feature_pk"], + ) + if serializer.validated_data["feature"] != url_feature: + raise ValidationError({"feature": "Feature must match the URL."}) + self._check_no_live_experiment(url_feature) super().perform_create(serializer) def perform_update(self, serializer): # type: ignore[no-untyped-def] self._check_no_live_experiment(serializer.instance.feature) + if "feature" in serializer.validated_data: + self._check_no_live_experiment(serializer.validated_data["feature"]) super().perform_update(serializer) def perform_destroy(self, instance): # type: ignore[no-untyped-def] diff --git a/api/features/serializers.py b/api/features/serializers.py index 40fd5ecf1cf8..65f949e5cffe 100644 --- a/api/features/serializers.py +++ b/api/features/serializers.py @@ -1,6 +1,6 @@ from datetime import datetime from functools import cached_property -from typing import Any, Callable +from typing import Any, Callable, cast from uuid import UUID import django.core.exceptions @@ -309,6 +309,17 @@ def validate_project_features_limit(self, project: Project) -> None: } ) + def update(self, instance: Feature, validated_data: dict[str, Any]) -> Feature: + if ( + "multivariate_options" in validated_data + and has_live_experiment(instance.id) + and not self._changes_multivariate_options( + validated_data["multivariate_options"], instance + ) + ): + validated_data.pop("multivariate_options") + return super().update(instance, validated_data) # type: ignore[no-any-return] + def validate_multivariate_options(self, multivariate_options): # type: ignore[no-untyped-def] if multivariate_options: user = self.context["request"].user @@ -326,19 +337,26 @@ def validate_multivariate_options(self, multivariate_options): # type: ignore[n ) if total_percentage_allocation > 100: raise serializers.ValidationError("Invalid percentage allocation") + instance = cast(Feature | None, self.instance) if ( - self.instance - and self._changes_multivariate_options(multivariate_options) - and has_live_experiment(self.instance.id) + instance + and self._changes_multivariate_options(multivariate_options, instance) + and has_live_experiment(instance.id) ): raise serializers.ValidationError(LIVE_EXPERIMENT_MV_OPTIONS_MESSAGE) return multivariate_options - def _changes_multivariate_options(self, multivariate_options) -> bool: # type: ignore[no-untyped-def] + def _changes_multivariate_options( + self, + multivariate_options: list[dict[str, Any]] | None, + instance: Feature | None = None, + ) -> bool: + instance = instance or cast(Feature | None, self.instance) + if instance is None: + return False # The frontend resubmits the whole feature, so only a real change to - # the options counts. Compare on content, not ids. + # writable option fields counts. Compare on content, not ids. fields = ( - "key", "type", "integer_value", "string_value", @@ -346,7 +364,7 @@ def _changes_multivariate_options(self, multivariate_options) -> bool: # type: "default_percentage_allocation", ) current = NestedMultivariateFeatureOptionSerializer( - self.instance.multivariate_options.all(), many=True + instance.multivariate_options.all(), many=True ).data def signature(options) -> list[tuple[Any, ...]]: # type: ignore[no-untyped-def] diff --git a/api/tests/unit/features/import_export/test_unit_features_import_export_tasks.py b/api/tests/unit/features/import_export/test_unit_features_import_export_tasks.py index 702f9a20b841..7a229e66db4d 100644 --- a/api/tests/unit/features/import_export/test_unit_features_import_export_tasks.py +++ b/api/tests/unit/features/import_export/test_unit_features_import_export_tasks.py @@ -2,6 +2,7 @@ from datetime import timedelta import pytest +from django.core.exceptions import ValidationError from django.db.models import Q from django.utils import timezone from freezegun.api import FrozenDateTimeFactory @@ -9,6 +10,7 @@ from environments.identities.models import Identity from environments.models import Environment +from experimentation.models import Experiment, ExperimentStatus from features.feature_types import MULTIVARIATE, STANDARD from features.import_export.constants import ( FAILED, @@ -22,6 +24,7 @@ FeatureImport, FlagsmithOnFlagsmithFeatureExport, ) +from features.import_export.services import overwrite_feature_for_environment from features.import_export.tasks import ( _create_flagsmith_on_flagsmith_feature_export, clear_stale_feature_imports_and_exports, @@ -29,6 +32,7 @@ import_features_for_environment, retire_stalled_feature_imports_and_exports, ) +from features.import_export.types import FeatureExportData from features.models import Feature, FeatureSegment, FeatureState from features.multivariate.models import MultivariateFeatureOption from features.value_types import STRING @@ -39,6 +43,55 @@ from segments.models import Segment +def test_overwrite_feature_for_environment__live_experiment_requires_new_option__raises_validation_error( + db: None, + environment: Environment, + feature: Feature, +) -> None: + # Given + MultivariateFeatureOption.objects.create( + feature=feature, + type=STRING, + string_value="existing", + default_percentage_allocation=100, + ) + Experiment.objects.create( + environment=environment, + feature=feature, + name="live experiment", + hypothesis="new option wins", + status=ExperimentStatus.RUNNING, + ) + feature_state = feature.feature_states.get(environment=environment) + imported_feature: FeatureExportData = { + "name": feature.name, + "default_enabled": feature.default_enabled, + "is_server_key_only": feature.is_server_key_only, + "initial_value": feature.initial_value, + "value": feature.initial_value, + "type": STRING, + "enabled": feature_state.enabled, + "multivariate": [ + { + "percentage_allocation": 100, + "default_percentage_allocation": 100, + "value": "new", + "type": STRING, + } + ], + } + + # When / Then + with pytest.raises(ValidationError): + overwrite_feature_for_environment(imported_feature, feature, environment) + + assert feature.multivariate_options.count() == 1 + assert ( + feature.feature_states.get(environment=environment).feature_state_value.value + == feature.initial_value + ) + + def test_clear_stale_feature_imports_and_exports__stale_records__deletes_old_keeps_new( # type: ignore[no-untyped-def] db: None, environment: Environment, freezer: FrozenDateTimeFactory ): diff --git a/api/tests/unit/features/multivariate/test_unit_multivariate_live_experiment.py b/api/tests/unit/features/multivariate/test_unit_multivariate_live_experiment.py index dd8430839d16..fd5d3ba7eb36 100644 --- a/api/tests/unit/features/multivariate/test_unit_multivariate_live_experiment.py +++ b/api/tests/unit/features/multivariate/test_unit_multivariate_live_experiment.py @@ -27,6 +27,7 @@ def _options_url(project: Project, feature: Feature) -> str: def test_create_mv_option__live_experiment__returns_400( # type: ignore[no-untyped-def] admin_client: APIClient, project, multivariate_feature, live_experiment ): + # Given # When response = admin_client.post( _options_url(project, multivariate_feature), @@ -71,6 +72,73 @@ def test_update_mv_option__live_experiment__returns_400( # type: ignore[no-unty assert option.string_value != "edited" +def test_update_mv_option__moving_to_live_experiment__returns_400( # type: ignore[no-untyped-def] + admin_client: APIClient, project, environment, multivariate_feature +): + # Given + other_feature = Feature.objects.create(name="other feature", project=project) + option = MultivariateFeatureOption.objects.create( + feature=other_feature, + type="unicode", + string_value="source option", + default_percentage_allocation=0, + ) + for existing_option in multivariate_feature.multivariate_options.all(): + existing_option.default_percentage_allocation = 0 + existing_option.save() + + Experiment.objects.create( + environment=environment, + feature=multivariate_feature, + name="live experiment", + hypothesis="new option wins", + status=ExperimentStatus.RUNNING, + ) + + # When + response = admin_client.put( + f"{_options_url(project, other_feature)}{option.id}/", + data={ + "feature": multivariate_feature.id, + "type": option.type, + "string_value": option.string_value, + "default_percentage_allocation": option.default_percentage_allocation, + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + option.refresh_from_db() + assert option.feature == other_feature + assert multivariate_feature.multivariate_options.count() == 3 + + +def test_create_mv_option__body_feature_mismatches_url__returns_400( # type: ignore[no-untyped-def] + admin_client: APIClient, project, multivariate_feature +): + # Given + other_feature = Feature.objects.create(name="other feature", project=project) + + # When + response = admin_client.post( + _options_url(project, multivariate_feature), + data={ + "feature": other_feature.id, + "type": "unicode", + "string_value": "new option", + "default_percentage_allocation": 0, + "key": "new", + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "Feature must match the URL." in str(response.json()) + assert other_feature.multivariate_options.count() == 0 + + def test_delete_mv_option__live_experiment__returns_400( # type: ignore[no-untyped-def] admin_client: APIClient, project, multivariate_feature, live_experiment ): @@ -90,6 +158,7 @@ def test_delete_mv_option__live_experiment__returns_400( # type: ignore[no-unty def test_create_mv_option__no_live_experiment__not_blocked_by_experiment_check( # type: ignore[no-untyped-def] admin_client: APIClient, project, multivariate_feature ): + # Given # When response = admin_client.post( _options_url(project, multivariate_feature), @@ -130,11 +199,17 @@ def test_update_feature__live_experiment__changed_mv_options_returns_400( # typ assert LIVE_EXPERIMENT_MV_OPTIONS_MESSAGE in str(response.json()) -def test_update_feature__live_experiment__unchanged_mv_options_allowed( # type: ignore[no-untyped-def] +def test_update_feature__live_experiment__unchanged_keyed_options_preserved( # type: ignore[no-untyped-def] admin_client: APIClient, project, multivariate_feature, live_experiment ): # Given + keyed_option = multivariate_feature.multivariate_options.first() + keyed_option.key = "stable-option" + keyed_option.save() url = f"/api/v1/projects/{project.id}/features/{multivariate_feature.id}/" + stored_option_ids = list( + multivariate_feature.multivariate_options.values_list("id", flat=True) + ) unchanged = [ { "type": option.type, @@ -156,3 +231,9 @@ def test_update_feature__live_experiment__unchanged_mv_options_allowed( # type: # Then assert response.status_code == status.HTTP_200_OK + keyed_option.refresh_from_db() + assert keyed_option.key == "stable-option" + assert ( + list(multivariate_feature.multivariate_options.values_list("id", flat=True)) + == stored_option_ids + ) From d564c8d053d14408261767f8c48432435b3adc51 Mon Sep 17 00:00:00 2001 From: TejasLamba2006 Date: Mon, 9 Nov 2026 23:44:32 +0530 Subject: [PATCH 3/3] fix: serialize experiment option changes and imports --- api/experimentation/services.py | 5 +- api/features/import_export/services.py | 11 +--- api/features/import_export/tasks.py | 48 +++++++------- api/features/multivariate/views.py | 37 ++++++++--- api/features/serializers.py | 25 +++++--- .../test_unit_features_import_export_tasks.py | 63 ++++++++++++++++++- .../test_unit_multivariate_live_experiment.py | 8 +-- 7 files changed, 138 insertions(+), 59 deletions(-) diff --git a/api/experimentation/services.py b/api/experimentation/services.py index 9434e0885f93..7f5b8800a06f 100644 --- a/api/experimentation/services.py +++ b/api/experimentation/services.py @@ -79,7 +79,7 @@ from features.feature_states.models import API_VALUE_TYPES from features.future.services import delete_segment_override, update_flag from features.future.types import SegmentOverrideRequest -from features.models import FeatureState +from features.models import Feature, FeatureState from features.value_types import BOOLEAN, STRING from features.versioning.versioning_service import get_environment_flags_list from integrations.flagsmith.client import get_openfeature_client @@ -617,6 +617,9 @@ def transition_experiment_status( experiment.status = target_status if target_status == ExperimentStatus.RUNNING and not experiment.started_at: + Feature.objects.select_for_update().filter( + pk=experiment.feature_id + ).exists() experiment.started_at = timezone.now() _remove_superseded_experiment_rollouts( experiment, AuthorData.from_user(user) diff --git a/api/features/import_export/services.py b/api/features/import_export/services.py index e6ec645ef5db..044ae1b3313a 100644 --- a/api/features/import_export/services.py +++ b/api/features/import_export/services.py @@ -18,17 +18,8 @@ def overwrite_feature_for_environment( Apply a destructive feature import to a single environment without affecting other environments' feature states or the Feature definition. """ - imported_option_values = { - (mv_data["type"], mv_data["value"]) for mv_data in feature_data["multivariate"] - } - existing_option_values = { - (option.type, option.value) - for option in existing_feature.multivariate_options.all() - } - creates_option = not imported_option_values.issubset(existing_option_values) - if creates_option and has_live_experiment(existing_feature.id): + if has_live_experiment(existing_feature.id): raise ValidationError(LIVE_EXPERIMENT_MV_OPTIONS_MESSAGE) - FeatureSegment.objects.filter( feature=existing_feature, environment=environment ).delete() diff --git a/api/features/import_export/tasks.py b/api/features/import_export/tasks.py index cdfe7cfa6fef..bc848d9c9c21 100644 --- a/api/features/import_export/tasks.py +++ b/api/features/import_export/tasks.py @@ -3,6 +3,7 @@ from typing import Optional from django.conf import settings +from django.db import transaction from django.db.models import Q from django.utils import timezone from task_processor.decorators import ( @@ -143,27 +144,32 @@ def import_features_for_environment(feature_import_id: int) -> None: def _import_features_for_environment(feature_import: FeatureImport) -> None: - environment = feature_import.environment - input_data: list[FeatureExportData] = json.loads(feature_import.data) - project = environment.project - - for feature_data in input_data: - existing_feature = Feature.objects.filter( - name=feature_data["name"], - project=project, - ).first() - - if existing_feature and feature_import.strategy == SKIP: - continue - - if existing_feature is None: - existing_feature = map_feature_export_data_to_feature(feature_data, project) - existing_feature.save() - - overwrite_feature_for_environment(feature_data, existing_feature, environment) - - feature_import.status = SUCCESS - feature_import.save() + with transaction.atomic(): + environment = feature_import.environment + input_data: list[FeatureExportData] = json.loads(feature_import.data) + project = environment.project + + for feature_data in input_data: + existing_feature = Feature.objects.filter( + name=feature_data["name"], + project=project, + ).first() + + if existing_feature and feature_import.strategy == SKIP: + continue + + if existing_feature is None: + existing_feature = map_feature_export_data_to_feature( + feature_data, project + ) + existing_feature.save() + + overwrite_feature_for_environment( + feature_data, existing_feature, environment + ) + + feature_import.status = SUCCESS + feature_import.save() # Should only run on official flagsmith instance. diff --git a/api/features/multivariate/views.py b/api/features/multivariate/views.py index 4182fb6f0310..0f3f1fa14363 100644 --- a/api/features/multivariate/views.py +++ b/api/features/multivariate/views.py @@ -2,6 +2,7 @@ CREATE_FEATURE, VIEW_PROJECT, ) +from django.db import transaction from django.utils.decorators import method_decorator from drf_spectacular.utils import extend_schema from rest_framework import viewsets @@ -72,7 +73,13 @@ def get_permissions(self): # type: ignore[no-untyped-def] def create(self, request, *args, **kwargs): # type: ignore[no-untyped-def] feature_pk = self.kwargs.get("feature_pk") project_pk = self.kwargs.get("project_pk") - get_object_or_404(Feature.objects.filter(project__id=project_pk), pk=feature_pk) + url_feature = get_object_or_404( + Feature.objects.filter(project__id=project_pk), pk=feature_pk + ) + if "feature" in request.data and str(request.data.get("feature")) != str( + url_feature.id + ): + raise ValidationError({"feature": "Feature must match the URL."}) return super().create(request, *args, **kwargs) def perform_create(self, serializer): # type: ignore[no-untyped-def] @@ -82,18 +89,32 @@ def perform_create(self, serializer): # type: ignore[no-untyped-def] ) if serializer.validated_data["feature"] != url_feature: raise ValidationError({"feature": "Feature must match the URL."}) - self._check_no_live_experiment(url_feature) - super().perform_create(serializer) + with transaction.atomic(): + Feature.objects.select_for_update().filter(pk=url_feature.pk).exists() + self._check_no_live_experiment(url_feature) + super().perform_create(serializer) def perform_update(self, serializer): # type: ignore[no-untyped-def] - self._check_no_live_experiment(serializer.instance.feature) + features = [serializer.instance.feature] if "feature" in serializer.validated_data: - self._check_no_live_experiment(serializer.validated_data["feature"]) - super().perform_update(serializer) + features.append(serializer.validated_data["feature"]) + feature_ids = sorted({feature.id for feature in features}) + with transaction.atomic(): + list( + Feature.objects.select_for_update() + .filter(pk__in=feature_ids) + .order_by("pk") + .values_list("pk", flat=True) + ) + for feature in features: + self._check_no_live_experiment(feature) + super().perform_update(serializer) def perform_destroy(self, instance): # type: ignore[no-untyped-def] - self._check_no_live_experiment(instance.feature) - super().perform_destroy(instance) + with transaction.atomic(): + Feature.objects.select_for_update().filter(pk=instance.feature_id).exists() + self._check_no_live_experiment(instance.feature) + super().perform_destroy(instance) def _check_no_live_experiment(self, feature: Feature) -> None: if has_live_experiment(feature.id): diff --git a/api/features/serializers.py b/api/features/serializers.py index 65f949e5cffe..c20a81119490 100644 --- a/api/features/serializers.py +++ b/api/features/serializers.py @@ -12,7 +12,7 @@ FeatureStateValueSerializer, ) from common.projects.permissions import VIEW_PROJECT -from django.db import models +from django.db import models, transaction from drf_spectacular.types import OpenApiTypes from drf_spectacular.utils import extend_schema_field from drf_writable_nested import ( # type: ignore[attr-defined] @@ -310,15 +310,20 @@ def validate_project_features_limit(self, project: Project) -> None: ) def update(self, instance: Feature, validated_data: dict[str, Any]) -> Feature: - if ( - "multivariate_options" in validated_data - and has_live_experiment(instance.id) - and not self._changes_multivariate_options( - validated_data["multivariate_options"], instance - ) - ): - validated_data.pop("multivariate_options") - return super().update(instance, validated_data) # type: ignore[no-any-return] + with transaction.atomic(): + Feature.objects.select_for_update().filter(pk=instance.pk).exists() + if ( + has_live_experiment(instance.id) + and "multivariate_options" in validated_data + ): + if self._changes_multivariate_options( + validated_data["multivariate_options"], instance + ): + raise serializers.ValidationError( + {"multivariate_options": LIVE_EXPERIMENT_MV_OPTIONS_MESSAGE} + ) + validated_data.pop("multivariate_options") + return super().update(instance, validated_data) # type: ignore[no-any-return] def validate_multivariate_options(self, multivariate_options): # type: ignore[no-untyped-def] if multivariate_options: diff --git a/api/tests/unit/features/import_export/test_unit_features_import_export_tasks.py b/api/tests/unit/features/import_export/test_unit_features_import_export_tasks.py index 7a229e66db4d..1e3b576dd9e4 100644 --- a/api/tests/unit/features/import_export/test_unit_features_import_export_tasks.py +++ b/api/tests/unit/features/import_export/test_unit_features_import_export_tasks.py @@ -43,10 +43,11 @@ from segments.models import Segment -def test_overwrite_feature_for_environment__live_experiment_requires_new_option__raises_validation_error( +def test_overwrite_feature_for_environment__live_experiment__raises_validation_error( db: None, environment: Environment, feature: Feature, + feature_segment: FeatureSegment, ) -> None: # Given MultivariateFeatureOption.objects.create( @@ -75,7 +76,7 @@ def test_overwrite_feature_for_environment__live_experiment_requires_new_option_ { "percentage_allocation": 100, "default_percentage_allocation": 100, - "value": "new", + "value": "existing", "type": STRING, } ], @@ -85,6 +86,7 @@ def test_overwrite_feature_for_environment__live_experiment_requires_new_option_ with pytest.raises(ValidationError): overwrite_feature_for_environment(imported_feature, feature, environment) + assert FeatureSegment.objects.filter(pk=feature_segment.pk).exists() assert feature.multivariate_options.count() == 1 assert ( feature.feature_states.get(environment=environment).feature_state_value.value @@ -92,6 +94,63 @@ def test_overwrite_feature_for_environment__live_experiment_requires_new_option_ ) +def test_import_features__live_experiment_on_later_feature__rolls_back_batch( + db: None, + environment: Environment, + project: Project, + feature: Feature, + feature_segment: FeatureSegment, +) -> None: + # Given + Experiment.objects.create( + environment=environment, + feature=feature, + name="live experiment", + hypothesis="new option wins", + status=ExperimentStatus.RUNNING, + ) + feature_import = FeatureImport.objects.create( + environment=environment, + strategy=OVERWRITE_DESTRUCTIVE, + data=json.dumps( + [ + { + "name": "created before failure", + "default_enabled": False, + "is_server_key_only": False, + "initial_value": "initial", + "value": "imported", + "type": STRING, + "enabled": True, + "multivariate": [], + }, + { + "name": feature.name, + "default_enabled": feature.default_enabled, + "is_server_key_only": feature.is_server_key_only, + "initial_value": feature.initial_value, + "value": feature.initial_value, + "type": STRING, + "enabled": False, + "multivariate": [], + }, + ] + ), + ) + + # When / Then + with pytest.raises(ValidationError): + import_features_for_environment(feature_import.id) + + feature_import.refresh_from_db() + assert feature_import.status == FAILED + assert not Feature.objects.filter( + project=project, name="created before failure" + ).exists() + assert FeatureSegment.objects.filter(pk=feature_segment.pk).exists() + assert feature.feature_states.get(environment=environment).enabled is False + + def test_clear_stale_feature_imports_and_exports__stale_records__deletes_old_keeps_new( # type: ignore[no-untyped-def] db: None, environment: Environment, freezer: FrozenDateTimeFactory ): diff --git a/api/tests/unit/features/multivariate/test_unit_multivariate_live_experiment.py b/api/tests/unit/features/multivariate/test_unit_multivariate_live_experiment.py index fd5d3ba7eb36..1b942a9a052d 100644 --- a/api/tests/unit/features/multivariate/test_unit_multivariate_live_experiment.py +++ b/api/tests/unit/features/multivariate/test_unit_multivariate_live_experiment.py @@ -123,13 +123,7 @@ def test_create_mv_option__body_feature_mismatches_url__returns_400( # type: ig # When response = admin_client.post( _options_url(project, multivariate_feature), - data={ - "feature": other_feature.id, - "type": "unicode", - "string_value": "new option", - "default_percentage_allocation": 0, - "key": "new", - }, + data={"feature": other_feature.id}, format="json", )