diff --git a/api/features/feature_segments/serializers.py b/api/features/feature_segments/serializers.py index c3b566005ba0..f435ffb844e2 100644 --- a/api/features/feature_segments/serializers.py +++ b/api/features/feature_segments/serializers.py @@ -60,11 +60,6 @@ def save(self, **kwargs: typing.Any) -> FeatureSegment: priority: int | None = self.validated_data.get("priority", None) - if kwargs["environment"].use_v2_feature_versioning: # pragma: no cover - assert kwargs["environment_feature_version"] is not None, ( - "Must provide environment_feature_version for environment using v2 versioning" - ) - if priority is not None: collision_qs = FeatureSegment.objects.filter( environment=kwargs["environment"], diff --git a/api/features/versioning/exceptions.py b/api/features/versioning/exceptions.py index 3fb5817a2748..5d34b9d2f6f9 100644 --- a/api/features/versioning/exceptions.py +++ b/api/features/versioning/exceptions.py @@ -12,3 +12,9 @@ class FeatureVersionDeleteError(FeatureVersioningError): class CannotModifyLiveVersionError(FeatureVersioningError): status_code = status.HTTP_400_BAD_REQUEST + + +class DirectFeatureStateWriteNotAllowedError(FeatureVersioningError): + status_code = status.HTTP_400_BAD_REQUEST + default_code = "direct_feature_state_write_not_allowed" + default_detail = "This environment uses v2 feature versioning. Use the environment feature version endpoint instead." diff --git a/api/features/versioning/versioning_service.py b/api/features/versioning/versioning_service.py index e027692d384c..531cc9eba6d9 100644 --- a/api/features/versioning/versioning_service.py +++ b/api/features/versioning/versioning_service.py @@ -13,9 +13,30 @@ FlagChangeSet, FlagChangeSetV2, ) +from features.versioning.exceptions import DirectFeatureStateWriteNotAllowedError from features.versioning.models import EnvironmentFeatureVersion +def require_direct_state_write( + environment: Environment, *, is_identity_override: bool +) -> None: + if is_identity_override or not environment.use_v2_feature_versioning: + return + raise DirectFeatureStateWriteNotAllowedError() + + +def require_direct_state_write_for_state(feature_state: FeatureState) -> None: + # FS rows attached to an unpublished EFV are a draft, so direct mutation is + # part of the versioning flow rather than a bypass of it. + efv = feature_state.environment_feature_version + if efv is not None and not efv.published: + return + require_direct_state_write( + environment=feature_state.environment, # type: ignore[arg-type] + is_identity_override=feature_state.identity_id is not None, + ) + + def get_environment_flags_queryset( environment: Environment, feature_name: str = None, # type: ignore[assignment] diff --git a/api/features/views.py b/api/features/views.py index 965f01bdf79b..345849738428 100644 --- a/api/features/views.py +++ b/api/features/views.py @@ -108,6 +108,8 @@ from .versioning.versioning_service import ( get_environment_flags_list, get_environment_flags_queryset, + require_direct_state_write, + require_direct_state_write_for_state, ) logger = logging.getLogger(__name__) @@ -740,6 +742,11 @@ def create(self, request, *args, **kwargs): # type: ignore[no-untyped-def] if identity_pk: data["identity"] = identity_pk + require_direct_state_write( + environment=environment, + is_identity_override=bool(identity_pk), + ) + serializer = self.get_serializer(data=data) if serializer.is_valid(raise_exception=True): @@ -763,6 +770,7 @@ def update(self, request, *args, **kwargs): # type: ignore[no-untyped-def] feature state value. """ feature_state_to_update = self.get_object() + require_direct_state_write_for_state(feature_state_to_update) feature_state_data = request.data # Check if feature state value was provided with request data. If so, create / update @@ -797,6 +805,10 @@ def partial_update(self, request, *args, **kwargs): # type: ignore[no-untyped-d """ return self.update(request, *args, **kwargs) # type: ignore[no-untyped-call] + def destroy(self, request, *args, **kwargs): # type: ignore[no-untyped-def] + require_direct_state_write_for_state(self.get_object()) + return super().destroy(request, *args, **kwargs) + def update_feature_state_value(self, value, feature_state): # type: ignore[no-untyped-def] feature_state_value_dict = feature_state.generate_feature_state_value_data( value @@ -930,6 +942,21 @@ class SimpleFeatureStateViewSet( permission_classes = [FeatureStatePermissions] filterset_fields = ["environment", "feature", "feature_segment"] + def create(self, request, *args, **kwargs): # type: ignore[no-untyped-def] + environment_id = request.data.get("environment") + targets_version = bool(request.data.get("environment_feature_version")) + if environment_id and not targets_version: + environment = get_object_or_404(Environment, id=environment_id) + require_direct_state_write( + environment=environment, + is_identity_override=bool(request.data.get("identity")), + ) + return super().create(request, *args, **kwargs) + + def update(self, request, *args, **kwargs): # type: ignore[no-untyped-def] + require_direct_state_write_for_state(self.get_object()) + return super().update(request, *args, **kwargs) + def get_queryset(self): # type: ignore[no-untyped-def] if getattr(self, "swagger_fake_view", False): return FeatureState.objects.none() @@ -1166,6 +1193,8 @@ def create_segment_override( # type: ignore[no-untyped-def] environment = get_object_or_404(Environment, api_key=environment_api_key) feature = get_object_or_404(Feature, project=environment.project, pk=feature_pk) + require_direct_state_write(environment=environment, is_identity_override=False) + serializer = CustomCreateSegmentOverrideFeatureStateSerializer( data=request.data, context={"environment": environment, "feature": feature} ) diff --git a/api/tests/unit/features/test_unit_features_views.py b/api/tests/unit/features/test_unit_features_views.py index 042735aea104..c524a8e8b010 100644 --- a/api/tests/unit/features/test_unit_features_views.py +++ b/api/tests/unit/features/test_unit_features_views.py @@ -1364,6 +1364,38 @@ def test_create_segment_override__admin_user__creates_override( assert created_override.get_feature_state_value() == string_value +def test_create_segment_override__v2_versioning_environment__returns_400( + admin_client_new: APIClient, + feature: Feature, + segment: Segment, + environment_v2_versioning: Environment, +) -> None: + # Given + url = reverse( + "api-v1:environments:create-segment-override", + args=[environment_v2_versioning.api_key, feature.id], + ) + data = { + "feature_state_value": {"string_value": "foo"}, + "enabled": True, + "feature_segment": {"segment": segment.id}, + } + + # When + response = admin_client_new.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "v2 feature versioning" in response.json()["detail"] + assert not FeatureState.objects.filter( + feature=feature, + environment=environment_v2_versioning, + feature_segment__segment=segment, + ).exists() + + def test_get_flags__user_throttle_set__is_not_throttled( # type: ignore[no-untyped-def] api_client: APIClient, environment: Environment, @@ -2856,6 +2888,305 @@ def test_update_feature_state__no_fsv_history__returns_200( assert response.status_code == status.HTTP_200_OK +def test_update_feature_state__v2_env_via_simple_endpoint__returns_400( + admin_client_new: APIClient, + environment_v2_versioning: Environment, + feature: Feature, +) -> None: + # Given + live_fs = FeatureState.objects.get( + environment=environment_v2_versioning, + feature=feature, + identity=None, + feature_segment=None, + ) + url = reverse("api-v1:features:featurestates-detail", args=[live_fs.id]) + data = { + "id": live_fs.id, + "enabled": True, + "feature_state_value": {"type": "unicode", "string_value": "should-be-blocked"}, + "environment": environment_v2_versioning.id, + "feature": feature.id, + } + + # When + response = admin_client_new.put( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "v2 feature versioning" in response.json()["detail"] + + +def test_update_feature_state__v2_env_via_nested_endpoint__returns_400( + admin_client_new: APIClient, + environment_v2_versioning: Environment, + feature: Feature, +) -> None: + # Given + live_fs = FeatureState.objects.get( + environment=environment_v2_versioning, + feature=feature, + identity=None, + feature_segment=None, + ) + url = reverse( + "api-v1:environments:environment-featurestates-detail", + args=[environment_v2_versioning.api_key, live_fs.id], + ) + data = { + "id": live_fs.id, + "enabled": True, + "feature_state_value": "should-be-blocked", + "environment": environment_v2_versioning.id, + "feature": feature.id, + "identity": None, + "feature_segment": None, + } + + # When + response = admin_client_new.put( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "v2 feature versioning" in response.json()["detail"] + + +def test_update_feature_state__v2_env_identity_override__returns_200( + admin_client_new: APIClient, + environment_v2_versioning: Environment, + feature: Feature, + identity: Identity, +) -> None: + # Given - identity overrides live outside the version graph in v2 + identity_fs = FeatureState.objects.create( + feature=feature, + environment=environment_v2_versioning, + identity=identity, + enabled=False, + ) + url = reverse("api-v1:features:featurestates-detail", args=[identity_fs.id]) + data = { + "id": identity_fs.id, + "enabled": True, + "feature_state_value": {"type": "unicode", "string_value": "identity-override"}, + "environment": environment_v2_versioning.id, + "feature": feature.id, + "identity": identity.id, + } + + # When + response = admin_client_new.put( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_200_OK + + +def test_create_feature_state__v2_env_via_simple_endpoint__returns_400( + admin_client_new: APIClient, + environment_v2_versioning: Environment, + feature: Feature, +) -> None: + # Given + url = reverse("api-v1:features:featurestates-list") + data = { + "enabled": True, + "feature_state_value": {"type": "unicode", "string_value": "blocked"}, + "environment": environment_v2_versioning.id, + "feature": feature.id, + } + + # When + response = admin_client_new.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "v2 feature versioning" in response.json()["detail"] + + +def test_create_feature_state__v2_env_via_nested_endpoint__returns_400( + admin_client_new: APIClient, + environment_v2_versioning: Environment, + feature: Feature, + project: Project, +) -> None: + # Given - create a feature_segment so the create has somewhere to go + segment = Segment.objects.create(name="new_override_segment", project=project) + url = reverse( + "api-v1:environments:environment-featurestates-list", + args=[environment_v2_versioning.api_key], + ) + data = { + "enabled": True, + "feature_state_value": "blocked", + "feature": feature.id, + "feature_segment": {"segment": segment.id, "priority": 1}, + } + + # When + response = admin_client_new.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "v2 feature versioning" in response.json()["detail"] + + +def test_create_feature_state__v2_env_identity_override__returns_201( + admin_client_new: APIClient, + environment_v2_versioning: Environment, + feature: Feature, + identity: Identity, +) -> None: + # Given - identity FS creates are not part of the version graph + url = reverse( + "api-v1:environments:identity-featurestates-list", + args=[environment_v2_versioning.api_key, identity.id], + ) + data = { + "enabled": True, + "feature_state_value": "identity-create", + "feature": feature.id, + } + + # When + response = admin_client_new.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_201_CREATED + + +def test_create_feature_state__v2_env_targets_version__returns_201( + admin_client_new: APIClient, + environment_v2_versioning: Environment, + feature: Feature, + project: Project, +) -> None: + # Given - a draft EFV plus a feature_segment attached to it + draft_version = EnvironmentFeatureVersion.objects.create( + feature=feature, environment=environment_v2_versioning + ) + segment = Segment.objects.create(name="seg_targets_version", project=project) + feature_segment = FeatureSegment.objects.create( + feature=feature, + segment=segment, + environment=environment_v2_versioning, + environment_feature_version=draft_version, + priority=1, + ) + url = reverse("api-v1:features:featurestates-list") + data = { + "enabled": True, + "feature_state_value": {"type": "unicode", "string_value": "override"}, + "environment": environment_v2_versioning.id, + "feature": feature.id, + "feature_segment": feature_segment.id, + "environment_feature_version": str(draft_version.uuid), + } + + # When + response = admin_client_new.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_201_CREATED + + +def test_update_feature_state__v2_env_draft_version__returns_200( + admin_client_new: APIClient, + environment_v2_versioning: Environment, + feature: Feature, +) -> None: + # Given - a new draft EFV (post_save clones the previous version's FSes) + draft_version = EnvironmentFeatureVersion.objects.create( + feature=feature, environment=environment_v2_versioning + ) + draft_fs = FeatureState.objects.get( + environment_feature_version=draft_version, + identity=None, + feature_segment=None, + ) + url = reverse("api-v1:features:featurestates-detail", args=[draft_fs.id]) + data = { + "id": draft_fs.id, + "enabled": True, + "feature_state_value": {"type": "unicode", "string_value": "draft-update"}, + "environment": environment_v2_versioning.id, + "feature": feature.id, + } + + # When + response = admin_client_new.put( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_200_OK + + +def test_delete_feature_state__v2_env_via_nested_endpoint__returns_400( + admin_client_new: APIClient, + environment_v2_versioning: Environment, + feature: Feature, +) -> None: + # Given - the live env-default FS attached to the current EFV + live_fs = FeatureState.objects.get( + environment=environment_v2_versioning, + feature=feature, + identity=None, + feature_segment=None, + ) + url = reverse( + "api-v1:environments:environment-featurestates-detail", + args=[environment_v2_versioning.api_key, live_fs.id], + ) + + # When + response = admin_client_new.delete(url) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "v2 feature versioning" in response.json()["detail"] + assert FeatureState.objects.filter(id=live_fs.id).exists() + + +def test_delete_feature_state__v2_env_identity_override__returns_204( + admin_client_new: APIClient, + environment_v2_versioning: Environment, + feature: Feature, + identity: Identity, +) -> None: + # Given - identity overrides live outside the version graph + identity_fs = FeatureState.objects.create( + feature=feature, + environment=environment_v2_versioning, + identity=identity, + enabled=False, + ) + url = reverse( + "api-v1:environments:identity-featurestates-detail", + args=[environment_v2_versioning.api_key, identity.id, identity_fs.id], + ) + + # When + response = admin_client_new.delete(url) + + # Then + assert response.status_code == status.HTTP_204_NO_CONTENT + assert not FeatureState.objects.filter(id=identity_fs.id).exists() + + def test_update_feature_state__change_feature__returns_400( admin_client_new: APIClient, environment: Environment,