From 52b978023ed10a36652ad41369c003dc1c31f4bd Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Thu, 12 Sep 2024 16:16:25 +0100 Subject: [PATCH] fix: ignore old versions when validating segment override limit (#4618) --- api/features/feature_segments/limits.py | 49 +++ api/features/feature_segments/serializers.py | 14 +- api/features/serializers.py | 23 +- api/features/versioning/serializers.py | 40 +++ .../test_unit_feature_segments_limits.py | 27 ++ .../versioning/test_unit_versioning_views.py | 311 ++++++++++++++++++ 6 files changed, 442 insertions(+), 22 deletions(-) create mode 100644 api/features/feature_segments/limits.py create mode 100644 api/tests/unit/features/feature_segments/test_unit_feature_segments_limits.py diff --git a/api/features/feature_segments/limits.py b/api/features/feature_segments/limits.py new file mode 100644 index 000000000000..e8fde2e52447 --- /dev/null +++ b/api/features/feature_segments/limits.py @@ -0,0 +1,49 @@ +from django.db.models import Q + +from environments.models import Environment +from features.versioning.models import EnvironmentFeatureVersion + +SEGMENT_OVERRIDE_LIMIT_EXCEEDED_MESSAGE = ( + "The environment has reached the maximum allowed segments overrides limit." +) + + +def exceeds_segment_override_limit( + environment: Environment, + segment_ids_to_create_overrides: list[int] | None = None, + segment_ids_to_delete_overrides: list[int] | None = None, + exclusive: bool = False, +) -> bool: + q = Q() + + segment_ids_to_create_overrides = segment_ids_to_create_overrides or [] + segment_ids_to_delete_overrides = segment_ids_to_delete_overrides or [] + + def _check(left: int, right: int) -> bool: + if exclusive: + return left > right + return left >= right + + if environment.use_v2_feature_versioning: + latest_versions = ( + EnvironmentFeatureVersion.objects.get_latest_versions_by_environment_id( + environment_id=environment.id + ) + ) + q = q & Q(environment_feature_version__in=latest_versions) + + existing_overridden_segment_ids = set( + environment.feature_segments.filter(q).values_list("segment_id", flat=True) + ) + segment_override_count = len(existing_overridden_segment_ids) + + extra = len(segment_ids_to_create_overrides) - len( + set(segment_ids_to_delete_overrides).intersection( + existing_overridden_segment_ids + ) + ) + + return _check( + segment_override_count + extra, + environment.project.max_segment_overrides_allowed, + ) diff --git a/api/features/feature_segments/serializers.py b/api/features/feature_segments/serializers.py index 9d9ccddb89d1..1064d807dad1 100644 --- a/api/features/feature_segments/serializers.py +++ b/api/features/feature_segments/serializers.py @@ -6,6 +6,10 @@ from rest_framework.exceptions import PermissionDenied from environments.permissions.constants import MANAGE_SEGMENT_OVERRIDES +from features.feature_segments.limits import ( + SEGMENT_OVERRIDE_LIMIT_EXCEEDED_MESSAGE, + exceeds_segment_override_limit, +) from features.models import FeatureSegment @@ -17,15 +21,9 @@ class Meta: def validate(self, data): data = super().validate(data) - environment = data["environment"] - if ( - environment.feature_segments.count() - >= environment.project.max_segment_overrides_allowed - ): + if exceeds_segment_override_limit(data["environment"]): raise serializers.ValidationError( - { - "environment": "The environment has reached the maximum allowed segments overrides limit." - } + {"environment": SEGMENT_OVERRIDE_LIMIT_EXCEEDED_MESSAGE} ) segment = data["segment"] diff --git a/api/features/serializers.py b/api/features/serializers.py index 5d4de0005517..f0226642d94c 100644 --- a/api/features/serializers.py +++ b/api/features/serializers.py @@ -15,7 +15,6 @@ from rest_framework.exceptions import PermissionDenied from environments.identities.models import Identity -from environments.models import Environment from environments.sdk.serializers_mixins import ( HideSensitiveFieldsSerializerMixin, ) @@ -33,6 +32,10 @@ ) from .constants import INTERSECTION, UNION +from .feature_segments.limits import ( + SEGMENT_OVERRIDE_LIMIT_EXCEEDED_MESSAGE, + exceeds_segment_override_limit, +) from .feature_segments.serializers import ( CustomCreateSegmentOverrideFeatureSegmentSerializer, ) @@ -599,6 +602,8 @@ class SDKFeatureStatesQuerySerializer(serializers.Serializer): class CustomCreateSegmentOverrideFeatureStateSerializer( CreateSegmentOverrideFeatureStateSerializer ): + validate_override_limit = True + feature_segment = CustomCreateSegmentOverrideFeatureSegmentSerializer( required=False, allow_null=True ) @@ -615,18 +620,8 @@ def _get_save_kwargs(self, field_name): def create(self, validated_data: dict) -> FeatureState: environment = validated_data["environment"] - self.validate_environment_segment_override_limit(environment) - return super().create(validated_data) - - def validate_environment_segment_override_limit( - self, environment: Environment - ) -> None: - if ( - environment.feature_segments.count() - >= environment.project.max_segment_overrides_allowed - ): + if self.validate_override_limit and exceeds_segment_override_limit(environment): raise serializers.ValidationError( - { - "environment": "The environment has reached the maximum allowed segments overrides limit." - } + {"environment": SEGMENT_OVERRIDE_LIMIT_EXCEEDED_MESSAGE} ) + return super().create(validated_data) diff --git a/api/features/versioning/serializers.py b/api/features/versioning/serializers.py index 32e96b420147..724c7ef767fa 100644 --- a/api/features/versioning/serializers.py +++ b/api/features/versioning/serializers.py @@ -5,6 +5,11 @@ from rest_framework import serializers from api_keys.user import APIKeyUser +from environments.models import Environment +from features.feature_segments.limits import ( + SEGMENT_OVERRIDE_LIMIT_EXCEEDED_MESSAGE, + exceeds_segment_override_limit, +) from features.serializers import ( CustomCreateSegmentOverrideFeatureStateSerializer, ) @@ -18,6 +23,8 @@ class CustomEnvironmentFeatureVersionFeatureStateSerializer( CustomCreateSegmentOverrideFeatureStateSerializer ): + validate_override_limit = False + class Meta(CustomCreateSegmentOverrideFeatureStateSerializer.Meta): read_only_fields = ( CustomCreateSegmentOverrideFeatureStateSerializer.Meta.read_only_fields @@ -141,10 +148,24 @@ class Meta(EnvironmentFeatureVersionSerializer.Meta): "publish_immediately", ) + @property + def segment_ids_to_create_overrides(self) -> list[int]: + return [ + fs["feature_segment"]["segment"] + for fs in self.initial_data.get("feature_states_to_create", []) + if fs["feature_segment"] is not None + ] + @transaction.atomic def create( self, validated_data: dict[str, typing.Any] ) -> EnvironmentFeatureVersion: + # Since the environment is passed as a keyword argument to save, we have + # to perform this validation here instead of in the `validate` method. + environment = validated_data["environment"] + self._validate_segment_override_limit(environment) + self._validate_v2_versioning(environment) + # Note that we use self.initial_data below for handling the feature states # since we want the raw data (rather than the serialized ORM objects) to pass # into the serializers in the separate private methods used for modifying the @@ -284,6 +305,25 @@ def _delete_feature_states( def _is_segment_override(self, feature_state: dict) -> bool: return feature_state.get("feature_segment") is not None + def _validate_segment_override_limit(self, environment: Environment) -> None: + if exceeds_segment_override_limit( + environment=environment, + segment_ids_to_create_overrides=self.segment_ids_to_create_overrides, + segment_ids_to_delete_overrides=self.initial_data.get( + "segment_ids_to_delete_overrides", [] + ), + exclusive=True, + ): + raise serializers.ValidationError( + {"environment": SEGMENT_OVERRIDE_LIMIT_EXCEEDED_MESSAGE} + ) + + def _validate_v2_versioning(self, environment: Environment) -> None: + if not environment.use_v2_feature_versioning: + raise serializers.ValidationError( + {"environment": "Environment must use v2 feature versioning."} + ) + class EnvironmentFeatureVersionPublishSerializer(serializers.Serializer): live_from = serializers.DateTimeField(required=False) diff --git a/api/tests/unit/features/feature_segments/test_unit_feature_segments_limits.py b/api/tests/unit/features/feature_segments/test_unit_feature_segments_limits.py new file mode 100644 index 000000000000..d5fc41249dc3 --- /dev/null +++ b/api/tests/unit/features/feature_segments/test_unit_feature_segments_limits.py @@ -0,0 +1,27 @@ +from environments.models import Environment +from features.feature_segments.limits import exceeds_segment_override_limit +from features.models import Feature +from projects.models import Project +from segments.models import Segment + + +def test_segment_override_limit_does_not_exclude_invalid_overrides_being_deleted( + feature: Feature, + segment: Segment, + another_segment: Segment, + environment_v2_versioning: Environment, + project: Project, +) -> None: + # Given + project.max_segment_overrides_allowed = 0 + project.save() + + # When + result = exceeds_segment_override_limit( + environment=environment_v2_versioning, + segment_ids_to_create_overrides=[another_segment.id], + segment_ids_to_delete_overrides=[segment.id], + ) + + # Then + assert result is True diff --git a/api/tests/unit/features/versioning/test_unit_versioning_views.py b/api/tests/unit/features/versioning/test_unit_versioning_views.py index 22d03618617d..3690328cffba 100644 --- a/api/tests/unit/features/versioning/test_unit_versioning_views.py +++ b/api/tests/unit/features/versioning/test_unit_versioning_views.py @@ -19,9 +19,13 @@ UPDATE_FEATURE_STATE, VIEW_ENVIRONMENT, ) +from features.feature_segments.limits import ( + SEGMENT_OVERRIDE_LIMIT_EXCEEDED_MESSAGE, +) from features.models import Feature, FeatureSegment, FeatureState from features.multivariate.models import MultivariateFeatureOption from features.versioning.models import EnvironmentFeatureVersion +from projects.models import Project from projects.permissions import VIEW_PROJECT from segments.models import Segment from tests.types import ( @@ -1198,3 +1202,310 @@ def generate_segment_override_fs_payload( ).priority == 1 ) + + +def test_create_new_version_fails_when_breaching_segment_override_limit( + feature: Feature, + segment: Segment, + another_segment: Segment, + environment_v2_versioning: Environment, + project: Project, + staff_user: FFAdminUser, + staff_client: APIClient, + with_environment_permissions: WithEnvironmentPermissionsCallable, + with_project_permissions: WithProjectPermissionsCallable, +) -> None: + # Given + with_environment_permissions([VIEW_ENVIRONMENT, UPDATE_FEATURE_STATE]) + with_project_permissions([VIEW_PROJECT]) + + # We update the limit of segment overrides on the project + project.max_segment_overrides_allowed = 1 + project.save() + + # And we create an existing version with a segment override in it + version_2 = EnvironmentFeatureVersion.objects.create( + environment=environment_v2_versioning, feature=feature + ) + FeatureState.objects.create( + feature=feature, + environment=environment_v2_versioning, + environment_feature_version=version_2, + feature_segment=FeatureSegment.objects.create( + feature=feature, + segment=segment, + environment=environment_v2_versioning, + environment_feature_version=version_2, + ), + ) + version_2.publish() + + data = { + "publish_immediately": True, + "feature_states_to_create": [ + { + "feature_segment": {"segment": segment.id}, + "enabled": True, + "feature_state_value": { + "type": "unicode", + "string_value": "some new value", + }, + } + ], + } + create_version_url = reverse( + "api-v1:versioning:environment-feature-versions-list", + args=[environment_v2_versioning.id, feature.id], + ) + + # When + create_version_response = staff_client.post( + create_version_url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert create_version_response.status_code == status.HTTP_400_BAD_REQUEST + assert create_version_response.json() == { + "environment": SEGMENT_OVERRIDE_LIMIT_EXCEEDED_MESSAGE + } + assert ( + EnvironmentFeatureVersion.objects.filter( + environment=environment_v2_versioning, feature=feature + ).count() + == 2 + ) + + +def test_segment_override_limit_excludes_older_versions__when_not_creating_any_new_overrides( + feature: Feature, + segment: Segment, + environment_v2_versioning: Environment, + project: Project, + staff_user: FFAdminUser, + staff_client: APIClient, + with_environment_permissions: WithEnvironmentPermissionsCallable, + with_project_permissions: WithProjectPermissionsCallable, +) -> None: + # Given + with_environment_permissions([VIEW_ENVIRONMENT, UPDATE_FEATURE_STATE]) + with_project_permissions([VIEW_PROJECT]) + + # We update the limit of segment overrides on the project + project.max_segment_overrides_allowed = 1 + project.save() + + # And we create an existing version with a segment override in it + version_2 = EnvironmentFeatureVersion.objects.create( + environment=environment_v2_versioning, feature=feature + ) + FeatureState.objects.create( + feature=feature, + environment=environment_v2_versioning, + environment_feature_version=version_2, + feature_segment=FeatureSegment.objects.create( + feature=feature, + segment=segment, + environment=environment_v2_versioning, + environment_feature_version=version_2, + ), + ) + version_2.publish() + + data = {"publish_immediately": True} + create_version_url = reverse( + "api-v1:versioning:environment-feature-versions-list", + args=[environment_v2_versioning.id, feature.id], + ) + + # When + create_version_response = staff_client.post( + create_version_url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert create_version_response.status_code == status.HTTP_201_CREATED + + version_3_uuid = create_version_response.json()["uuid"] + assert FeatureState.objects.filter( + feature=feature, + environment=environment_v2_versioning, + environment_feature_version__uuid=version_3_uuid, + ).exists() + + +def test_segment_override_limit_excludes_older_versions__when_creating_new_override( + feature: Feature, + segment: Segment, + another_segment: Segment, + environment_v2_versioning: Environment, + project: Project, + staff_user: FFAdminUser, + staff_client: APIClient, + with_environment_permissions: WithEnvironmentPermissionsCallable, + with_project_permissions: WithProjectPermissionsCallable, +) -> None: + # Given + with_environment_permissions([VIEW_ENVIRONMENT, UPDATE_FEATURE_STATE]) + with_project_permissions([VIEW_PROJECT]) + + # We update the limit of segment overrides on the project + project.max_segment_overrides_allowed = 2 + project.save() + + # And we create an existing version with a segment override in it + version_2 = EnvironmentFeatureVersion.objects.create( + environment=environment_v2_versioning, feature=feature + ) + FeatureState.objects.create( + feature=feature, + environment=environment_v2_versioning, + environment_feature_version=version_2, + feature_segment=FeatureSegment.objects.create( + feature=feature, + segment=segment, + environment=environment_v2_versioning, + environment_feature_version=version_2, + ), + ) + version_2.publish() + + data = { + "publish_immediately": True, + "feature_states_to_create": [ + { + "enabled": True, + "feature_state_value": { + "type": "unicode", + "string_value": "some new value", + }, + "feature_segment": { + "segment": another_segment.id, + }, + } + ], + } + + create_version_url = reverse( + "api-v1:versioning:environment-feature-versions-list", + args=[environment_v2_versioning.id, feature.id], + ) + + # When + create_version_response = staff_client.post( + create_version_url, + data=json.dumps(data), + content_type="application/json", + ) + + # Then + assert create_version_response.status_code == status.HTTP_201_CREATED + + version_3_uuid = create_version_response.json()["uuid"] + assert FeatureState.objects.filter( + feature=feature, + environment=environment_v2_versioning, + environment_feature_version__uuid=version_3_uuid, + ).exists() + + +def test_segment_override_limit_excludes_overrides_being_deleted_when_creating_new_override( + feature: Feature, + segment: Segment, + another_segment: Segment, + environment_v2_versioning: Environment, + project: Project, + staff_user: FFAdminUser, + staff_client: APIClient, + with_environment_permissions: WithEnvironmentPermissionsCallable, + with_project_permissions: WithProjectPermissionsCallable, +) -> None: + # Given + with_environment_permissions([VIEW_ENVIRONMENT, UPDATE_FEATURE_STATE]) + with_project_permissions([VIEW_PROJECT]) + + # We update the limit of segment overrides on the project + project.max_segment_overrides_allowed = 1 + project.save() + + # And we create an existing version with a segment override in it + version_2 = EnvironmentFeatureVersion.objects.create( + environment=environment_v2_versioning, feature=feature + ) + FeatureState.objects.create( + feature=feature, + environment=environment_v2_versioning, + environment_feature_version=version_2, + feature_segment=FeatureSegment.objects.create( + feature=feature, + segment=segment, + environment=environment_v2_versioning, + environment_feature_version=version_2, + ), + ) + version_2.publish() + + data = { + "publish_immediately": True, + "feature_states_to_create": [ + { + "enabled": True, + "feature_state_value": { + "type": "unicode", + "string_value": "some new value", + }, + "feature_segment": { + "segment": another_segment.id, + }, + } + ], + "segment_ids_to_delete_overrides": [segment.id], + } + + create_version_url = reverse( + "api-v1:versioning:environment-feature-versions-list", + args=[environment_v2_versioning.id, feature.id], + ) + + # When + create_version_response = staff_client.post( + create_version_url, + data=json.dumps(data), + content_type="application/json", + ) + + # Then + assert create_version_response.status_code == status.HTTP_201_CREATED + + version_3_uuid = create_version_response.json()["uuid"] + assert FeatureState.objects.filter( + feature=feature, + environment=environment_v2_versioning, + environment_feature_version__uuid=version_3_uuid, + ).exists() + + +def test_cannot_create_new_version_for_environment_not_enabled_for_versioning_v2( + environment: Environment, + feature: Feature, + staff_client: APIClient, + with_environment_permissions: WithEnvironmentPermissionsCallable, + with_project_permissions: WithProjectPermissionsCallable, +) -> None: + # Given + with_environment_permissions([VIEW_ENVIRONMENT, UPDATE_FEATURE_STATE]) + with_project_permissions([VIEW_PROJECT]) + + url = reverse( + "api-v1:versioning:environment-feature-versions-list", + args=[environment.id, feature.id], + ) + + # When + response = staff_client.post(url) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + + assert response.json() == { + "environment": "Environment must use v2 feature versioning." + }