diff --git a/api/experimentation/feature_state_metadata.py b/api/experimentation/feature_state_metadata.py index b0af6cddfcbd..2799a386dd84 100644 --- a/api/experimentation/feature_state_metadata.py +++ b/api/experimentation/feature_state_metadata.py @@ -9,6 +9,17 @@ 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 bool( + 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/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/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/import_export/services.py b/api/features/import_export/services.py index 4e31b6f35ae6..044ae1b3313a 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,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. """ + 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 aecb0dee4a6d..0f3f1fa14363 100644 --- a/api/features/multivariate/views.py +++ b/api/features/multivariate/views.py @@ -2,13 +2,17 @@ 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 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 @@ -69,9 +73,53 @@ 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] + 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."}) + 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] + features = [serializer.instance.feature] + if "feature" in serializer.validated_data: + 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] + 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): + 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..c20a81119490 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 @@ -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] @@ -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 ( @@ -307,6 +309,22 @@ def validate_project_features_limit(self, project: Project) -> None: } ) + def update(self, instance: Feature, validated_data: dict[str, Any]) -> Feature: + 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: user = self.context["request"].user @@ -324,8 +342,44 @@ 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 ( + 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: 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 + # writable option fields counts. Compare on content, not ids. + fields = ( + "type", + "integer_value", + "string_value", + "boolean_value", + "default_percentage_allocation", + ) + current = NestedMultivariateFeatureOptionSerializer( + 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/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..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 @@ -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,114 @@ from segments.models import Segment +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( + 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": "existing", + "type": STRING, + } + ], + } + + # When / Then + 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 + == feature.initial_value + ) + + +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 new file mode 100644 index 000000000000..1b942a9a052d --- /dev/null +++ b/api/tests/unit/features/multivariate/test_unit_multivariate_live_experiment.py @@ -0,0 +1,233 @@ +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 +): + # Given + # 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_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}, + 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 +): + # 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 +): + # Given + # 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_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, + "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 + 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 + )