From 4ea79aadd297b5121b93fa9ea78be6d87d11ae82 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Wed, 29 Oct 2025 10:23:01 +0000 Subject: [PATCH 01/26] Initial PoC for new endpoint --- api/environments/urls.py | 6 ++ api/features/feature_states/views.py | 22 ++++++ api/features/serializers.py | 47 +++++++++++ api/features/versioning/dataclasses.py | 18 +++++ api/features/versioning/versioning_service.py | 78 ++++++++++++++++++- .../unit/features/feature_states/__init__.py | 0 .../test_unit_feature_states_views.py | 50 ++++++++++++ 7 files changed, 220 insertions(+), 1 deletion(-) create mode 100644 api/features/feature_states/views.py create mode 100644 api/tests/unit/features/feature_states/__init__.py create mode 100644 api/tests/unit/features/feature_states/test_unit_feature_states_views.py diff --git a/api/environments/urls.py b/api/environments/urls.py index d5cb0ceab33d..ad2c5ede484a 100644 --- a/api/environments/urls.py +++ b/api/environments/urls.py @@ -7,6 +7,7 @@ EdgeIdentityWithIdentifierFeatureStateView, get_edge_identity_overrides, ) +from features.feature_states.views import update_flag from features.views import ( EnvironmentFeatureStateViewSet, IdentityFeatureStateViewSet, @@ -167,4 +168,9 @@ get_edge_identity_overrides, name="edge-identity-overrides", ), + path( + "/features//update-flag/", + update_flag, + name="update-flag", + ), ] diff --git a/api/features/feature_states/views.py b/api/features/feature_states/views.py new file mode 100644 index 000000000000..f8bb0a28aa7a --- /dev/null +++ b/api/features/feature_states/views.py @@ -0,0 +1,22 @@ +from rest_framework import status +from rest_framework.decorators import api_view +from rest_framework.request import Request +from rest_framework.response import Response + +from environments.models import Environment +from features.models import Feature +from features.serializers import UpdateFlagSerializer + + +@api_view(http_method_names=["POST"]) +def update_flag(request: Request, environment_id: int, feature_name: str) -> Response: + environment = Environment.objects.get(id=environment_id) + feature = Feature.objects.get(name=feature_name, project_id=environment.project_id) + + serializer = UpdateFlagSerializer( + data=request.data, context={"request": request, "view": update_flag} + ) + serializer.is_valid(raise_exception=True) + serializer.save(environment=environment, feature=feature) + + return Response(serializer.data, status=status.HTTP_200_OK) diff --git a/api/features/serializers.py b/api/features/serializers.py index 55785e1bda5a..a4011e97daa7 100644 --- a/api/features/serializers.py +++ b/api/features/serializers.py @@ -1,3 +1,4 @@ +import typing from datetime import datetime from typing import Any @@ -18,6 +19,7 @@ from app_analytics.serializers import LabelsQuerySerializerMixin, LabelsSerializer from environments.identities.models import Identity +from environments.models import Environment from environments.sdk.serializers_mixins import ( HideSensitiveFieldsSerializerMixin, ) @@ -28,6 +30,7 @@ FeatureFlagCodeReferencesRepositoryCountSerializer, ) from projects.models import Project +from users.models import FFAdminUser from users.serializers import ( UserIdsSerializer, UserListSerializer, @@ -47,6 +50,8 @@ ) from .models import Feature, FeatureState from .multivariate.serializers import NestedMultivariateFeatureOptionSerializer +from .versioning.dataclasses import FlagChangeSet +from .versioning.versioning_service import update_flag class FeatureStateSerializerSmall(serializers.ModelSerializer): # type: ignore[type-arg] @@ -671,3 +676,45 @@ def create(self, validated_data: dict) -> FeatureState: # type: ignore[type-arg {"environment": SEGMENT_OVERRIDE_LIMIT_EXCEEDED_MESSAGE} ) return super().create(validated_data) # type: ignore[no-any-return,no-untyped-call] + + +class UpdateFlagSerializer(serializers.Serializer): # type: ignore[type-arg] + enabled = serializers.BooleanField(required=False) + + # TODO: this is introducing _another_ way of handling typing, but it feels closer + # to what we should have done from the start. This might be out of scope for this + # work though. + feature_state_value = serializers.CharField(required=False) + type = serializers.ChoiceField( + choices=["int", "string", "bool", "float"], + required=False, + default="string", + ) + + segment_id = serializers.IntegerField(required=False) + + # TODO: multivariate? + + @property + def flag_change_set(self) -> FlagChangeSet: + validated_data = self.validated_data + change_set = FlagChangeSet( + enabled=validated_data.get("enabled"), + feature_state_value=validated_data.get("feature_state_value"), + type_=validated_data.get("type"), + segment_id=validated_data.get("segment_id"), + ) + + request = self.context["request"] + if type(request.user) is FFAdminUser: + change_set.user = request.user + else: + change_set.api_key = request.user.key + + return change_set + + def save(self, **kwargs: typing.Any) -> FeatureState: + environment: Environment = kwargs["environment"] + feature: Feature = kwargs["feature"] + + return update_flag(environment, feature, self.flag_change_set) diff --git a/api/features/versioning/dataclasses.py b/api/features/versioning/dataclasses.py index 0c4476a4f51a..edb26243fb83 100644 --- a/api/features/versioning/dataclasses.py +++ b/api/features/versioning/dataclasses.py @@ -1,7 +1,13 @@ +import typing +from dataclasses import dataclass from datetime import datetime from pydantic import BaseModel, computed_field +if typing.TYPE_CHECKING: + from api_keys.models import MasterAPIKey + from users.models import FFAdminUser + class Conflict(BaseModel): segment_id: int | None = None @@ -12,3 +18,15 @@ class Conflict(BaseModel): @property def is_environment_default(self) -> bool: return self.segment_id is None + + +@dataclass +class FlagChangeSet: + enabled: bool + feature_state_value: str + type_: str + + user: typing.Optional["FFAdminUser"] = None + api_key: typing.Optional["MasterAPIKey"] = None + + segment_id: str | None = None diff --git a/api/features/versioning/versioning_service.py b/api/features/versioning/versioning_service.py index 41e30e4432a0..a83b81b9de81 100644 --- a/api/features/versioning/versioning_service.py +++ b/api/features/versioning/versioning_service.py @@ -4,8 +4,10 @@ from django.db.models import Prefetch, Q, QuerySet from django.utils import timezone +from core.constants import BOOLEAN, FLOAT, INTEGER, STRING from environments.models import Environment -from features.models import FeatureState +from features.models import Feature, FeatureState, FeatureStateValue +from features.versioning.dataclasses import FlagChangeSet from features.versioning.models import EnvironmentFeatureVersion @@ -101,6 +103,80 @@ def get_current_live_environment_feature_version( ) +def update_flag( + environment: Environment, feature: Feature, change_set: FlagChangeSet +) -> FeatureState: + if environment.use_v2_feature_versioning: + return _update_flag_for_versioning_v2(environment, feature, change_set) + else: + return _update_flag_for_versioning_v1(environment, feature, change_set) + + +def _update_flag_for_versioning_v2( + environment: Environment, feature: Feature, change_set: FlagChangeSet +) -> FeatureState: + new_version = EnvironmentFeatureVersion.objects.create( + environment=environment, + feature=feature, + created_by=change_set.user, + created_by_api_key=change_set.api_key, + ) + + target_feature_state: FeatureState = new_version.feature_states.get( + feature_segment__segment_id=change_set.segment_id, + ) + + target_feature_state.enabled = change_set.enabled + target_feature_state.save() + + _update_feature_state_value(target_feature_state.feature_state_value, change_set) + + new_version.publish( + published_by=change_set.user, published_by_api_key=change_set.api_key + ) + + return target_feature_state + + +def _update_flag_for_versioning_v1( + environment: Environment, feature: Feature, change_set: FlagChangeSet +) -> FeatureState: + latest_feature_states = get_environment_flags_dict( + environment=environment, + feature_name=feature.name, + additional_filters=Q(feature_segment__segment_id=change_set.segment_id), + ) + assert len(latest_feature_states) == 1 + + target_feature_state = list(latest_feature_states.values())[0] + target_feature_state.enabled = change_set.enabled + target_feature_state.save() + + _update_feature_state_value(target_feature_state.feature_state_value, change_set) + + return target_feature_state + + +def _update_feature_state_value( + fsv: FeatureStateValue, change_set: FlagChangeSet +) -> None: + match change_set.type_: + case "string": + fsv.string_value = change_set.feature_state_value + fsv.type = STRING + case "int": + fsv.integer_value = int(change_set.feature_state_value) + fsv.type = INTEGER + case "bool": + fsv.boolean_value = change_set.feature_state_value in ("True", "true", "1") + fsv.type = BOOLEAN + case "float": + fsv.float_value = float(change_set.feature_state_value) + fsv.type = FLOAT + + fsv.save() + + def _get_feature_states_queryset( environment: "Environment", feature_name: str | None = None, diff --git a/api/tests/unit/features/feature_states/__init__.py b/api/tests/unit/features/feature_states/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/api/tests/unit/features/feature_states/test_unit_feature_states_views.py b/api/tests/unit/features/feature_states/test_unit_feature_states_views.py new file mode 100644 index 000000000000..9674034f8946 --- /dev/null +++ b/api/tests/unit/features/feature_states/test_unit_feature_states_views.py @@ -0,0 +1,50 @@ +import json + +import pytest +from common.environments.permissions import UPDATE_FEATURE_STATE +from django.urls import reverse +from pytest_lazyfixture import lazy_fixture # type: ignore[import-untyped] +from rest_framework import status +from rest_framework.test import APIClient + +from environments.models import Environment +from features.models import Feature +from features.versioning.versioning_service import ( + get_environment_flags_list, +) +from tests.types import WithEnvironmentPermissionsCallable + + +@pytest.mark.parametrize( + "environment_", + (lazy_fixture("environment"), lazy_fixture("environment_v2_versioning")), +) +def test_update_flag( + staff_client: APIClient, + feature: Feature, + environment_: Environment, + with_environment_permissions: WithEnvironmentPermissionsCallable, +) -> None: + # Given + with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] + url = reverse( + "api-v1:environments:update-flag", + kwargs={"environment_id": environment_.id, "feature_name": feature.name}, + ) + + data = {"enabled": True, "feature_state_value": "42", "type": "int"} + + # When + response = staff_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_200_OK + + latest_flags = get_environment_flags_list( + environment=environment_, feature_name=feature.name + ) + + assert latest_flags[0].enabled is True + assert latest_flags[0].get_feature_state_value() == 42 From 891598ef63554fec5b1d79852a11e87cc415b11e Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Wed, 19 Nov 2025 19:24:56 +0530 Subject: [PATCH 02/26] feat(api): add experimental feature flag update endpoints Adds two new experimental endpoints under /api/experiments/: - update-flag-v1: Update single feature state (environment default or segment override) - update-flag-v2: Update multiple feature states in a single request (environment default + segment overrides) --- api/api/urls/experiments.py | 25 + api/app/urls.py | 4 + api/environments/urls.py | 6 - api/features/feature_states/serializers.py | 198 ++++++ api/features/feature_states/views.py | 131 +++- api/features/serializers.py | 47 -- api/features/versioning/dataclasses.py | 33 +- api/features/versioning/versioning_service.py | 310 +++++++++- .../test_unit_feature_states_views.py | 581 +++++++++++++++++- 9 files changed, 1253 insertions(+), 82 deletions(-) create mode 100644 api/api/urls/experiments.py create mode 100644 api/features/feature_states/serializers.py diff --git a/api/api/urls/experiments.py b/api/api/urls/experiments.py new file mode 100644 index 000000000000..8675d682af68 --- /dev/null +++ b/api/api/urls/experiments.py @@ -0,0 +1,25 @@ +""" +Experimental API endpoints. + +These endpoints are subject to change and should not be considered stable. +Use at your own risk - breaking changes may occur without prior notice. +""" + +from django.urls import path + +from features.feature_states.views import update_flag_v1, update_flag_v2 + +app_name = "experiments" + +urlpatterns = [ + path( + "environments//update-flag-v1/", + update_flag_v1, + name="update-flag-v1", + ), + path( + "environments//update-flag-v2/", + update_flag_v2, + name="update-flag-v2", + ), +] diff --git a/api/app/urls.py b/api/app/urls.py index f936b8d70953..69fd350419eb 100644 --- a/api/app/urls.py +++ b/api/app/urls.py @@ -22,6 +22,10 @@ ), re_path(r"^api/v1/", include("api.urls.v1", namespace="api-v1")), re_path(r"^api/v2/", include("api.urls.v2", namespace="api-v2")), + re_path( + r"^api/experiments/", + include("api.urls.experiments", namespace="api-experiments"), + ), re_path(r"^admin/", admin.site.urls), re_path( r"^sales-dashboard/", diff --git a/api/environments/urls.py b/api/environments/urls.py index ad2c5ede484a..d5cb0ceab33d 100644 --- a/api/environments/urls.py +++ b/api/environments/urls.py @@ -7,7 +7,6 @@ EdgeIdentityWithIdentifierFeatureStateView, get_edge_identity_overrides, ) -from features.feature_states.views import update_flag from features.views import ( EnvironmentFeatureStateViewSet, IdentityFeatureStateViewSet, @@ -168,9 +167,4 @@ get_edge_identity_overrides, name="edge-identity-overrides", ), - path( - "/features//update-flag/", - update_flag, - name="update-flag", - ), ] diff --git a/api/features/feature_states/serializers.py b/api/features/feature_states/serializers.py new file mode 100644 index 000000000000..aeb2f987927e --- /dev/null +++ b/api/features/feature_states/serializers.py @@ -0,0 +1,198 @@ +import typing + +from rest_framework import serializers + +from environments.models import Environment +from features.models import Feature, FeatureState +from features.versioning.dataclasses import ( + FlagChangeSet, + FlagChangeSetV2, + SegmentOverrideChangeSet, +) +from features.versioning.versioning_service import update_flag, update_flag_v2 +from users.models import FFAdminUser + + +class BaseFeatureUpdateSerializer(serializers.Serializer): # type: ignore[type-arg] + def get_feature(self) -> Feature: + feature_data = self.validated_data["feature"] + environment: Environment = self.context.get("environment") + + if not environment: + raise serializers.ValidationError("Environment context is required") + + try: + if "id" in feature_data: + return Feature.objects.get( + id=feature_data["id"], project_id=environment.project_id + ) + else: + return Feature.objects.get( + name=feature_data["name"], project_id=environment.project_id + ) + except Feature.DoesNotExist: + identifier = feature_data.get("id") or feature_data.get("name") + raise serializers.ValidationError( + f"Feature with identifier '{identifier}' not found in project" + ) + + def _add_audit_fields( + self, change_set: typing.Union[FlagChangeSet, FlagChangeSetV2] + ) -> None: + request = self.context["request"] + if type(request.user) is FFAdminUser: + change_set.user = request.user + else: + change_set.api_key = request.user.key + + def _parse_feature_value(self, value_data: dict) -> typing.Any: # type: ignore[type-arg] + value_serializer = FeatureValueSerializer(data=value_data) + value_serializer.is_valid() + return value_serializer.get_typed_value() + + +class FeatureIdentifierSerializer(serializers.Serializer): # type: ignore[type-arg] + """Accepts either name OR id (mutually exclusive).""" + + name = serializers.CharField(required=False, allow_blank=False) + id = serializers.IntegerField(required=False) + + def validate(self, data: dict) -> dict: # type: ignore[type-arg] + if not data.get("name") and not data.get("id"): + raise serializers.ValidationError( + "Either 'name' or 'id' is required for feature identification" + ) + if data.get("name") and data.get("id"): + raise serializers.ValidationError("Provide either 'name' or 'id', not both") + return data + + +class SegmentSerializer(serializers.Serializer): # type: ignore[type-arg] + id = serializers.IntegerField(required=True) + priority = serializers.IntegerField(required=False, allow_null=True) + + +class FeatureValueSerializer(serializers.Serializer): # type: ignore[type-arg] + """Value is always a string; type field indicates how to parse it.""" + + type = serializers.ChoiceField( + choices=["integer", "string", "boolean", "float"], required=True + ) + string_value = serializers.CharField(required=True, allow_blank=True) + + def get_typed_value(self) -> typing.Any: + value_type = self.validated_data["type"] + string_val = self.validated_data["string_value"] + + match value_type: + case "string": + return string_val + case "integer": + return int(string_val) + case "boolean": + return string_val.lower() == "true" + case "float": + return float(string_val) + + return string_val + + +class UpdateFlagSerializer(BaseFeatureUpdateSerializer): + feature = FeatureIdentifierSerializer(required=True) + segment = SegmentSerializer(required=False) + enabled = serializers.BooleanField(required=True) + value = FeatureValueSerializer(required=True) + + @property + def flag_change_set(self) -> FlagChangeSet: + validated_data = self.validated_data + value_data = validated_data["value"] + segment_data = validated_data.get("segment") + + change_set = FlagChangeSet( + enabled=validated_data["enabled"], + feature_state_value=self._parse_feature_value(value_data), + type_=value_data["type"], + segment_id=segment_data.get("id") if segment_data else None, + segment_priority=segment_data.get("priority") if segment_data else None, + ) + + self._add_audit_fields(change_set) + return change_set + + def save(self, **kwargs: typing.Any) -> FeatureState: + environment: Environment = kwargs["environment"] + feature: Feature = self.get_feature() + + return update_flag(environment, feature, self.flag_change_set) + + +class EnvironmentDefaultSerializer(serializers.Serializer): # type: ignore[type-arg] + enabled = serializers.BooleanField(required=True) + value = FeatureValueSerializer(required=True) + + +class SegmentOverrideSerializer(serializers.Serializer): # type: ignore[type-arg] + segment_id = serializers.IntegerField(required=True) + priority = serializers.IntegerField(required=False, allow_null=True) + enabled = serializers.BooleanField(required=True) + value = FeatureValueSerializer(required=True) + + +class UpdateFlagV2Serializer(BaseFeatureUpdateSerializer): + feature = FeatureIdentifierSerializer(required=True) + environment_default = EnvironmentDefaultSerializer(required=True) + segment_overrides = SegmentOverrideSerializer(many=True, required=False) + + def validate_segment_overrides( + self, + value: list[dict], # type: ignore[type-arg] + ) -> list[dict]: # type: ignore[type-arg] + if not value: + return value + + segment_ids = [override["segment_id"] for override in value] + if len(segment_ids) != len(set(segment_ids)): + raise serializers.ValidationError( + "Duplicate segment_id values are not allowed" + ) + + return value + + @property + def change_set_v2(self) -> FlagChangeSetV2: + validated_data = self.validated_data + + env_default = validated_data["environment_default"] + env_value_data = env_default["value"] + + segment_overrides_data = validated_data.get("segment_overrides", []) + segment_overrides = [] + + for override_data in segment_overrides_data: + value_data = override_data["value"] + + segment_override = SegmentOverrideChangeSet( + segment_id=override_data["segment_id"], + enabled=override_data["enabled"], + feature_state_value=self._parse_feature_value(value_data), + type_=value_data["type"], + priority=override_data.get("priority"), + ) + segment_overrides.append(segment_override) + + change_set = FlagChangeSetV2( + environment_default_enabled=env_default["enabled"], + environment_default_value=self._parse_feature_value(env_value_data), + environment_default_type=env_value_data["type"], + segment_overrides=segment_overrides, + ) + + self._add_audit_fields(change_set) + return change_set + + def save(self, **kwargs: typing.Any) -> dict: # type: ignore[type-arg] + environment: Environment = kwargs["environment"] + feature: Feature = self.get_feature() + + return update_flag_v2(environment, feature, self.change_set_v2) diff --git a/api/features/feature_states/views.py b/api/features/feature_states/views.py index f8bb0a28aa7a..0d941107ba24 100644 --- a/api/features/feature_states/views.py +++ b/api/features/feature_states/views.py @@ -1,22 +1,139 @@ +from drf_yasg import openapi # type: ignore[import-untyped] +from drf_yasg.utils import swagger_auto_schema # type: ignore[import-untyped] from rest_framework import status from rest_framework.decorators import api_view from rest_framework.request import Request from rest_framework.response import Response from environments.models import Environment -from features.models import Feature -from features.serializers import UpdateFlagSerializer +from .serializers import UpdateFlagSerializer, UpdateFlagV2Serializer + +@swagger_auto_schema( + method="post", + operation_summary="Update single feature state (V1)", + operation_description=""" + **EXPERIMENTAL ENDPOINT** - Subject to change without notice. + + Updates a single feature state within an environment. You can update either: + - The environment default state (when no segment is specified) + - A specific segment override (when segment.id is provided) + + **Feature Identification:** + - Use `feature.name` OR `feature.id` (mutually exclusive) + - Feature must belong to the environment's project + + **Value Format:** + - Always use `string_value` field (value is always a string) + - The `type` field tells the server how to parse it + - Available types: integer, string, boolean, float + - Examples: + - `{"type": "integer", "string_value": "42"}` + - `{"type": "boolean", "string_value": "true"}` + - `{"type": "float", "string_value": "3.14"}` + - `{"type": "string", "string_value": "hello"}` + + **Segment Priority:** + - Optional `segment.priority` field controls ordering + - If null/omitted, segment is appended to end + - Use specific priority value to reorder + """, + request_body=UpdateFlagSerializer, + responses={ + 204: openapi.Response( + description="Feature state updated successfully (no content returned)" + ) + }, + tags=["Experimental - Feature States"], +) @api_view(http_method_names=["POST"]) -def update_flag(request: Request, environment_id: int, feature_name: str) -> Response: +def update_flag_v1(request: Request, environment_id: int) -> Response: + """ + V1: Single feature state update endpoint (issue #6279). + + Updates a single feature state (environment default OR one segment override). + Feature identification is provided in the request payload. + """ environment = Environment.objects.get(id=environment_id) - feature = Feature.objects.get(name=feature_name, project_id=environment.project_id) serializer = UpdateFlagSerializer( - data=request.data, context={"request": request, "view": update_flag} + data=request.data, + context={ + "request": request, + "view": update_flag_v1, + "environment": environment, + }, + ) + serializer.is_valid(raise_exception=True) + serializer.save(environment=environment) + + return Response(status=status.HTTP_204_NO_CONTENT) + + +@swagger_auto_schema( + method="post", + operation_summary="Update multiple feature states", + operation_description=""" + **EXPERIMENTAL ENDPOINT** - Subject to change without notice. + + Updates multiple feature states in a single request. This endpoint allows + you to configure an entire feature (environment default + all segment overrides) + in one operation. + + **What You Can Update:** + - Environment default state (required) + - Multiple segment overrides (optional) + - Priority ordering for each segment + + **Feature Identification:** + - Use `feature.name` OR `feature.id` (mutually exclusive) + - Feature must belong to the environment's project + + **Value Format:** + - Always use `string_value` field (value is always a string) + - The `type` field tells the server how to parse it + - Available types: integer, string, boolean, float + - Examples: + - `{"type": "string", "string_value": "production"}` + - `{"type": "integer", "string_value": "100"}` + - `{"type": "boolean", "string_value": "false"}` + + **Segment Overrides:** + - Provide array of segment override configurations + - Each override must specify: segment_id, enabled, value + - Optional priority field controls ordering + - Duplicate segment_id values are not allowed + + """, + request_body=UpdateFlagV2Serializer, + responses={ + 204: openapi.Response( + description="Feature states updated successfully (no content returned)" + ) + }, + tags=["Experimental - Feature States"], +) +@api_view(http_method_names=["POST"]) +def update_flag_v2(request: Request, environment_id: int) -> Response: + """ + Update multiple feature states endpoint (issue #6233). + + Updates multiple feature states in a single request: + - Environment default state + - Multiple segment overrides with priority ordering + """ + environment = Environment.objects.get(id=environment_id) + + serializer = UpdateFlagV2Serializer( + data=request.data, + context={ + "request": request, + "view": update_flag_v2, + "environment": environment, + }, ) serializer.is_valid(raise_exception=True) - serializer.save(environment=environment, feature=feature) + serializer.save(environment=environment) - return Response(serializer.data, status=status.HTTP_200_OK) + return Response(status=status.HTTP_204_NO_CONTENT) diff --git a/api/features/serializers.py b/api/features/serializers.py index a4011e97daa7..55785e1bda5a 100644 --- a/api/features/serializers.py +++ b/api/features/serializers.py @@ -1,4 +1,3 @@ -import typing from datetime import datetime from typing import Any @@ -19,7 +18,6 @@ from app_analytics.serializers import LabelsQuerySerializerMixin, LabelsSerializer from environments.identities.models import Identity -from environments.models import Environment from environments.sdk.serializers_mixins import ( HideSensitiveFieldsSerializerMixin, ) @@ -30,7 +28,6 @@ FeatureFlagCodeReferencesRepositoryCountSerializer, ) from projects.models import Project -from users.models import FFAdminUser from users.serializers import ( UserIdsSerializer, UserListSerializer, @@ -50,8 +47,6 @@ ) from .models import Feature, FeatureState from .multivariate.serializers import NestedMultivariateFeatureOptionSerializer -from .versioning.dataclasses import FlagChangeSet -from .versioning.versioning_service import update_flag class FeatureStateSerializerSmall(serializers.ModelSerializer): # type: ignore[type-arg] @@ -676,45 +671,3 @@ def create(self, validated_data: dict) -> FeatureState: # type: ignore[type-arg {"environment": SEGMENT_OVERRIDE_LIMIT_EXCEEDED_MESSAGE} ) return super().create(validated_data) # type: ignore[no-any-return,no-untyped-call] - - -class UpdateFlagSerializer(serializers.Serializer): # type: ignore[type-arg] - enabled = serializers.BooleanField(required=False) - - # TODO: this is introducing _another_ way of handling typing, but it feels closer - # to what we should have done from the start. This might be out of scope for this - # work though. - feature_state_value = serializers.CharField(required=False) - type = serializers.ChoiceField( - choices=["int", "string", "bool", "float"], - required=False, - default="string", - ) - - segment_id = serializers.IntegerField(required=False) - - # TODO: multivariate? - - @property - def flag_change_set(self) -> FlagChangeSet: - validated_data = self.validated_data - change_set = FlagChangeSet( - enabled=validated_data.get("enabled"), - feature_state_value=validated_data.get("feature_state_value"), - type_=validated_data.get("type"), - segment_id=validated_data.get("segment_id"), - ) - - request = self.context["request"] - if type(request.user) is FFAdminUser: - change_set.user = request.user - else: - change_set.api_key = request.user.key - - return change_set - - def save(self, **kwargs: typing.Any) -> FeatureState: - environment: Environment = kwargs["environment"] - feature: Feature = kwargs["feature"] - - return update_flag(environment, feature, self.flag_change_set) diff --git a/api/features/versioning/dataclasses.py b/api/features/versioning/dataclasses.py index edb26243fb83..ba30a71f01bf 100644 --- a/api/features/versioning/dataclasses.py +++ b/api/features/versioning/dataclasses.py @@ -23,10 +23,39 @@ def is_environment_default(self) -> bool: @dataclass class FlagChangeSet: enabled: bool - feature_state_value: str + feature_state_value: typing.Any type_: str user: typing.Optional["FFAdminUser"] = None api_key: typing.Optional["MasterAPIKey"] = None - segment_id: str | None = None + segment_id: int | None = None + segment_priority: int | None = None + + +@dataclass +class SegmentOverrideChangeSet: + """Represents a single segment override change.""" + + segment_id: int + enabled: bool + feature_state_value: typing.Any + type_: str + priority: int | None = None + + +@dataclass +class FlagChangeSetV2: + """Represents V2 feature state changes (environment default + segment overrides).""" + + # Environment default state + environment_default_enabled: bool + environment_default_value: typing.Any + environment_default_type: str + + # Segment overrides (list) + segment_overrides: list[SegmentOverrideChangeSet] + + # Audit fields + user: typing.Optional["FFAdminUser"] = None + api_key: typing.Optional["MasterAPIKey"] = None diff --git a/api/features/versioning/versioning_service.py b/api/features/versioning/versioning_service.py index a83b81b9de81..e58a76810901 100644 --- a/api/features/versioning/versioning_service.py +++ b/api/features/versioning/versioning_service.py @@ -7,7 +7,10 @@ from core.constants import BOOLEAN, FLOAT, INTEGER, STRING from environments.models import Environment from features.models import Feature, FeatureState, FeatureStateValue -from features.versioning.dataclasses import FlagChangeSet +from features.versioning.dataclasses import ( + FlagChangeSet, + FlagChangeSetV2, +) from features.versioning.models import EnvironmentFeatureVersion @@ -115,6 +118,9 @@ def update_flag( def _update_flag_for_versioning_v2( environment: Environment, feature: Feature, change_set: FlagChangeSet ) -> FeatureState: + from features.models import FeatureSegment, FeatureState + from segments.models import Segment + new_version = EnvironmentFeatureVersion.objects.create( environment=environment, feature=feature, @@ -122,14 +128,47 @@ def _update_flag_for_versioning_v2( created_by_api_key=change_set.api_key, ) - target_feature_state: FeatureState = new_version.feature_states.get( - feature_segment__segment_id=change_set.segment_id, - ) + try: + target_feature_state: FeatureState = new_version.feature_states.get( + feature_segment__segment_id=change_set.segment_id, + ) + except FeatureState.DoesNotExist: + if change_set.segment_id: + segment = Segment.objects.get(id=change_set.segment_id) + + feature_segment = FeatureSegment.objects.create( + feature=feature, + segment=segment, + environment=environment, + priority=change_set.segment_priority + if change_set.segment_priority is not None + else 0, + ) + + # FeatureStateValue is automatically created via signal + target_feature_state = FeatureState.objects.create( + feature=feature, + environment=environment, + feature_segment=feature_segment, + environment_feature_version=new_version, + enabled=change_set.enabled, + ) + else: + raise target_feature_state.enabled = change_set.enabled target_feature_state.save() - _update_feature_state_value(target_feature_state.feature_state_value, change_set) + _update_feature_state_value( + target_feature_state.feature_state_value, + change_set.feature_state_value, + change_set.type_, + ) + + if change_set.segment_id and change_set.segment_priority is not None: + feature_segment = target_feature_state.feature_segment + if feature_segment: + feature_segment.to(change_set.segment_priority) new_version.publish( published_by=change_set.user, published_by_api_key=change_set.api_key @@ -141,42 +180,285 @@ def _update_flag_for_versioning_v2( def _update_flag_for_versioning_v1( environment: Environment, feature: Feature, change_set: FlagChangeSet ) -> FeatureState: + from features.models import FeatureSegment, FeatureState + from segments.models import Segment + latest_feature_states = get_environment_flags_dict( environment=environment, feature_name=feature.name, additional_filters=Q(feature_segment__segment_id=change_set.segment_id), ) + + if len(latest_feature_states) == 0 and change_set.segment_id: + segment = Segment.objects.get(id=change_set.segment_id) + + feature_segment = FeatureSegment.objects.create( + feature=feature, + segment=segment, + environment=environment, + priority=change_set.segment_priority + if change_set.segment_priority is not None + else 0, + ) + + # FeatureStateValue is automatically created via signal + target_feature_state = FeatureState.objects.create( + feature=feature, + environment=environment, + feature_segment=feature_segment, + enabled=change_set.enabled, + ) + + _update_feature_state_value( + target_feature_state.feature_state_value, + change_set.feature_state_value, + change_set.type_, + ) + + return target_feature_state + assert len(latest_feature_states) == 1 target_feature_state = list(latest_feature_states.values())[0] target_feature_state.enabled = change_set.enabled target_feature_state.save() - _update_feature_state_value(target_feature_state.feature_state_value, change_set) + _update_feature_state_value( + target_feature_state.feature_state_value, + change_set.feature_state_value, + change_set.type_, + ) + + if change_set.segment_id and change_set.segment_priority is not None: + feature_segment = target_feature_state.feature_segment + if feature_segment: + feature_segment.to(change_set.segment_priority) return target_feature_state def _update_feature_state_value( - fsv: FeatureStateValue, change_set: FlagChangeSet + fsv: FeatureStateValue, value: typing.Any, type_: str ) -> None: - match change_set.type_: + """Works for both V1 and V2 changesets.""" + match type_: case "string": - fsv.string_value = change_set.feature_state_value + fsv.string_value = str(value) fsv.type = STRING - case "int": - fsv.integer_value = int(change_set.feature_state_value) + case "integer": + fsv.integer_value = int(value) fsv.type = INTEGER - case "bool": - fsv.boolean_value = change_set.feature_state_value in ("True", "true", "1") + case "boolean": + if isinstance(value, bool): + fsv.boolean_value = value + else: + fsv.boolean_value = str(value).lower() == "true" fsv.type = BOOLEAN case "float": - fsv.float_value = float(change_set.feature_state_value) + fsv.float_value = float(value) fsv.type = FLOAT fsv.save() +def _create_segment_override( + feature: Feature, + environment: Environment, + segment_id: int, + enabled: bool, + priority: int | None, + version: EnvironmentFeatureVersion | None = None, +) -> FeatureState: + from features.models import FeatureSegment + from segments.models import Segment + + segment = Segment.objects.get(id=segment_id) + + feature_segment = FeatureSegment.objects.create( + feature=feature, + segment=segment, + environment=environment, + priority=priority if priority is not None else 0, + ) + + segment_state = FeatureState.objects.create( + feature=feature, + environment=environment, + feature_segment=feature_segment, + environment_feature_version=version, + enabled=enabled, + ) + + return segment_state + + +def _update_segment_priority(feature_state: FeatureState, priority: int) -> None: + feature_segment = feature_state.feature_segment + if feature_segment: + feature_segment.to(priority) + + +# V2 Update Functions (FlagChangeSetV2) + + +def update_flag_v2( + environment: Environment, feature: Feature, change_set: FlagChangeSetV2 +) -> dict: # type: ignore[type-arg] + """ + Update multiple feature states (V2 changeset). + Returns a dict with the results. + """ + if environment.use_v2_feature_versioning: + return _update_flag_v2_for_versioning_v2(environment, feature, change_set) + else: + return _update_flag_v2_for_versioning_v1(environment, feature, change_set) + + +def _update_flag_v2_for_versioning_v2( + environment: Environment, feature: Feature, change_set: FlagChangeSetV2 +) -> dict: # type: ignore[type-arg] + """ + V2 changeset update for V2 versioning. + Creates a new version with all changes applied. + """ + new_version = EnvironmentFeatureVersion.objects.create( + environment=environment, + feature=feature, + created_by=change_set.user, + created_by_api_key=change_set.api_key, + ) + + env_default_state = new_version.feature_states.get(feature_segment__isnull=True) + env_default_state.enabled = change_set.environment_default_enabled + env_default_state.save() + + _update_feature_state_value( + env_default_state.feature_state_value, + change_set.environment_default_value, + change_set.environment_default_type, + ) + + updated_segments = [] + for override in change_set.segment_overrides: + try: + segment_state = new_version.feature_states.get( + feature_segment__segment_id=override.segment_id + ) + segment_state.enabled = override.enabled + segment_state.save() + + _update_feature_state_value( + segment_state.feature_state_value, + override.feature_state_value, + override.type_, + ) + + if override.priority is not None: + _update_segment_priority(segment_state, override.priority) + except FeatureState.DoesNotExist: + segment_state = _create_segment_override( + feature=feature, + environment=environment, + segment_id=override.segment_id, + enabled=override.enabled, + priority=override.priority, + version=new_version, + ) + + _update_feature_state_value( + segment_state.feature_state_value, + override.feature_state_value, + override.type_, + ) + + updated_segments.append( + {"segment_id": override.segment_id, "enabled": override.enabled} + ) + + new_version.publish( + published_by=change_set.user, + published_by_api_key=change_set.api_key, + ) + + return { + "environment_default": {"enabled": change_set.environment_default_enabled}, + "segment_overrides": updated_segments, + "version": new_version.uuid, + } + + +def _update_flag_v2_for_versioning_v1( + environment: Environment, feature: Feature, change_set: FlagChangeSetV2 +) -> dict: # type: ignore[type-arg] + """ + V2 changeset update for V1 versioning. + Updates all feature states in place. + """ + env_default_states = get_environment_flags_dict( + environment=environment, + feature_name=feature.name, + additional_filters=Q(feature_segment__isnull=True), + ) + assert len(env_default_states) == 1 + + env_default_state = list(env_default_states.values())[0] + env_default_state.enabled = change_set.environment_default_enabled + env_default_state.save() + + _update_feature_state_value( + env_default_state.feature_state_value, + change_set.environment_default_value, + change_set.environment_default_type, + ) + + updated_segments = [] + for override in change_set.segment_overrides: + segment_states = get_environment_flags_dict( + environment=environment, + feature_name=feature.name, + additional_filters=Q(feature_segment__segment_id=override.segment_id), + ) + + if len(segment_states) == 0: + segment_state = _create_segment_override( + feature=feature, + environment=environment, + segment_id=override.segment_id, + enabled=override.enabled, + priority=override.priority, + version=None, # V1 versioning doesn't use versions + ) + + _update_feature_state_value( + segment_state.feature_state_value, + override.feature_state_value, + override.type_, + ) + else: + assert len(segment_states) == 1 + segment_state = list(segment_states.values())[0] + segment_state.enabled = override.enabled + segment_state.save() + + _update_feature_state_value( + segment_state.feature_state_value, + override.feature_state_value, + override.type_, + ) + + if override.priority is not None: + _update_segment_priority(segment_state, override.priority) + + updated_segments.append( + {"segment_id": override.segment_id, "enabled": override.enabled} + ) + + return { + "environment_default": {"enabled": change_set.environment_default_enabled}, + "segment_overrides": updated_segments, + } + + def _get_feature_states_queryset( environment: "Environment", feature_name: str | None = None, diff --git a/api/tests/unit/features/feature_states/test_unit_feature_states_views.py b/api/tests/unit/features/feature_states/test_unit_feature_states_views.py index 9674034f8946..4c347cb14113 100644 --- a/api/tests/unit/features/feature_states/test_unit_feature_states_views.py +++ b/api/tests/unit/features/feature_states/test_unit_feature_states_views.py @@ -1,4 +1,5 @@ import json +import typing import pytest from common.environments.permissions import UPDATE_FEATURE_STATE @@ -7,19 +8,64 @@ from rest_framework import status from rest_framework.test import APIClient +from environments.identities.models import Identity from environments.models import Environment -from features.models import Feature +from features.models import Feature, FeatureSegment from features.versioning.versioning_service import ( get_environment_flags_list, ) +from projects.models import Project +from segments.models import Condition, Segment, SegmentRule from tests.types import WithEnvironmentPermissionsCallable +@pytest.fixture() +def sdk_client_factory() -> typing.Callable[[Environment], APIClient]: + """Factory to create SDK clients for different environments.""" + + def _create_sdk_client(environment: Environment) -> APIClient: + client = APIClient() + client.credentials(HTTP_X_ENVIRONMENT_KEY=environment.api_key) + return client + + return _create_sdk_client + + +@pytest.fixture() +def sdk_client( + environment: Environment, + sdk_client_factory: typing.Callable[[Environment], APIClient], +) -> APIClient: + """API client configured with SDK authentication for the default environment.""" + return sdk_client_factory(environment) + + +def get_identity_feature_state_from_sdk( + sdk_client: APIClient, identifier: str, feature_name: str +) -> typing.Dict[str, typing.Any]: + """Helper to get a specific feature's state for an identity via SDK endpoint.""" + identities_url = reverse("api-v1:sdk-identities") + data = {"identifier": identifier} + + response = sdk_client.post( + identities_url, data=json.dumps(data), content_type="application/json" + ) + assert response.status_code == status.HTTP_200_OK + response_json = response.json() + + # Find the specific feature in the flags list + for flag in response_json["flags"]: + if flag["feature"]["name"] == feature_name: + return flag # type: ignore[no-any-return] + + raise ValueError(f"Feature {feature_name} not found in identity flags response") + + @pytest.mark.parametrize( "environment_", (lazy_fixture("environment"), lazy_fixture("environment_v2_versioning")), ) -def test_update_flag( +def test_update_flag_by_name( staff_client: APIClient, feature: Feature, environment_: Environment, @@ -28,11 +74,15 @@ def test_update_flag( # Given with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] url = reverse( - "api-v1:environments:update-flag", - kwargs={"environment_id": environment_.id, "feature_name": feature.name}, + "api-experiments:update-flag-v1", + kwargs={"environment_id": environment_.id}, ) - data = {"enabled": True, "feature_state_value": "42", "type": "int"} + data = { + "feature": {"name": feature.name}, + "enabled": True, + "value": {"type": "integer", "string_value": "42"}, + } # When response = staff_client.post( @@ -40,7 +90,7 @@ def test_update_flag( ) # Then - assert response.status_code == status.HTTP_200_OK + assert response.status_code == status.HTTP_204_NO_CONTENT latest_flags = get_environment_flags_list( environment=environment_, feature_name=feature.name @@ -48,3 +98,522 @@ def test_update_flag( assert latest_flags[0].enabled is True assert latest_flags[0].get_feature_state_value() == 42 + + +@pytest.mark.parametrize( + "environment_", + (lazy_fixture("environment"), lazy_fixture("environment_v2_versioning")), +) +def test_update_flag_by_id( + staff_client: APIClient, + feature: Feature, + environment_: Environment, + with_environment_permissions: WithEnvironmentPermissionsCallable, +) -> None: + # Given + with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] + url = reverse( + "api-experiments:update-flag-v1", + kwargs={"environment_id": environment_.id}, + ) + + data = { + "feature": {"id": feature.id}, + "enabled": False, + "value": {"type": "string", "string_value": "test_value"}, + } + + # When + response = staff_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_204_NO_CONTENT + + latest_flags = get_environment_flags_list( + environment=environment_, feature_name=feature.name + ) + + assert latest_flags[0].enabled is False + assert latest_flags[0].get_feature_state_value() == "test_value" + + +@pytest.mark.parametrize( + "environment_", + (lazy_fixture("environment"), lazy_fixture("environment_v2_versioning")), +) +def test_update_flag_error_when_both_name_and_id_provided( + staff_client: APIClient, + feature: Feature, + environment_: Environment, + with_environment_permissions: WithEnvironmentPermissionsCallable, +) -> None: + # Given + with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] + url = reverse( + "api-experiments:update-flag-v1", + kwargs={"environment_id": environment_.id}, + ) + + # Providing both name and ID (even if they match the same feature) + data = { + "feature": {"name": feature.name, "id": feature.id}, + "enabled": True, + "value": {"type": "integer", "string_value": "42"}, + } + + # When + response = staff_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "feature" in response.json() + assert "Provide either 'name' or 'id', not both" in str(response.json()["feature"]) + + +@pytest.mark.parametrize( + "environment_", + (lazy_fixture("environment"), lazy_fixture("environment_v2_versioning")), +) +def test_update_flag_error_when_both_name_and_id_provided_for_different_features( + staff_client: APIClient, + feature: Feature, + environment_: Environment, + project: Project, + with_environment_permissions: WithEnvironmentPermissionsCallable, +) -> None: + # Given + with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] + + # Create another feature in the same project + another_feature = Feature.objects.create( + name="another_feature", project=project, type="STANDARD" + ) + + url = reverse( + "api-experiments:update-flag-v1", + kwargs={"environment_id": environment_.id}, + ) + + # Providing both name and ID for DIFFERENT features (worst case) + data = { + "feature": {"name": feature.name, "id": another_feature.id}, + "enabled": True, + "value": {"type": "integer", "string_value": "42"}, + } + + # When + response = staff_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then - Should fail at validation level (mutual exclusion) + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "feature" in response.json() + assert "Provide either 'name' or 'id', not both" in str(response.json()["feature"]) + + +@pytest.mark.parametrize( + "environment_", + (lazy_fixture("environment"), lazy_fixture("environment_v2_versioning")), +) +def test_update_flag_error_when_neither_name_nor_id_provided( + staff_client: APIClient, + feature: Feature, + environment_: Environment, + with_environment_permissions: WithEnvironmentPermissionsCallable, +) -> None: + # Given + with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] + url = reverse( + "api-experiments:update-flag-v1", + kwargs={"environment_id": environment_.id}, + ) + + # Providing empty feature object + data = { + "feature": {}, + "enabled": True, + "value": {"type": "integer", "string_value": "42"}, + } + + # When + response = staff_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "feature" in response.json() + assert "Either 'name' or 'id' is required" in str(response.json()["feature"]) + + +@pytest.mark.parametrize( + "environment_", + (lazy_fixture("environment"), lazy_fixture("environment_v2_versioning")), +) +def test_update_flag_error_when_feature_not_found_by_name( + staff_client: APIClient, + environment_: Environment, + with_environment_permissions: WithEnvironmentPermissionsCallable, +) -> None: + # Given + with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] + url = reverse( + "api-experiments:update-flag-v1", + kwargs={"environment_id": environment_.id}, + ) + + data = { + "feature": {"name": "non_existent_feature"}, + "enabled": True, + "value": {"type": "integer", "string_value": "42"}, + } + + # When + response = staff_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "Feature with identifier 'non_existent_feature' not found" in str( + response.json() + ) + + +@pytest.mark.parametrize( + "environment_", + (lazy_fixture("environment"), lazy_fixture("environment_v2_versioning")), +) +def test_update_flag_error_when_feature_not_found_by_id( + staff_client: APIClient, + environment_: Environment, + with_environment_permissions: WithEnvironmentPermissionsCallable, +) -> None: + # Given + with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] + url = reverse( + "api-experiments:update-flag-v1", + kwargs={"environment_id": environment_.id}, + ) + + data = { + "feature": {"id": 999999}, # Non-existent ID + "enabled": True, + "value": {"type": "integer", "string_value": "42"}, + } + + # When + response = staff_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "Feature with identifier '999999' not found" in str(response.json()) + + +# V1 Segment Override Tests + + +@pytest.mark.parametrize( + "environment_", + (lazy_fixture("environment"), lazy_fixture("environment_v2_versioning")), +) +def test_update_flag_segment_override_by_name( + staff_client: APIClient, + feature: Feature, + environment_: Environment, + project: Project, + with_environment_permissions: WithEnvironmentPermissionsCallable, +) -> None: + # Given + with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] + + # Create SDK client for this environment + sdk_client = APIClient() + sdk_client.credentials(HTTP_X_ENVIRONMENT_KEY=environment_.api_key) + + # Create a segment with a rule that matches identities with email containing "test" + segment = Segment.objects.create( + name="test_segment", project=project, description="Test segment" + ) + segment_rule = SegmentRule.objects.create( + segment=segment, type=SegmentRule.ALL_RULE + ) + Condition.objects.create( + rule=segment_rule, + property="email", + operator="CONTAINS", + value="test", + ) + + # Create an identity that matches the segment + identity = Identity.objects.create(identifier="test_user", environment=environment_) + identity.update_traits([{"trait_key": "email", "trait_value": "test@example.com"}]) + + url = reverse( + "api-experiments:update-flag-v1", + kwargs={"environment_id": environment_.id}, + ) + + data = { + "feature": {"name": feature.name}, + "segment": {"id": segment.id, "priority": 1}, + "enabled": False, + "value": {"type": "integer", "string_value": "999"}, + } + + # When + response = staff_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_204_NO_CONTENT + + # Verify the segment override is applied to the identity via SDK + flag_state = get_identity_feature_state_from_sdk( + sdk_client, identity.identifier, feature.name + ) + assert flag_state["enabled"] is False + assert flag_state["feature_state_value"] == 999 + + +@pytest.mark.parametrize( + "environment_", + (lazy_fixture("environment"), lazy_fixture("environment_v2_versioning")), +) +def test_update_flag_segment_override_creates_feature_segment_if_not_exists( + staff_client: APIClient, + feature: Feature, + environment_: Environment, + project: Project, + with_environment_permissions: WithEnvironmentPermissionsCallable, +) -> None: + # Given + with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] + + # Create SDK client for this environment + sdk_client = APIClient() + sdk_client.credentials(HTTP_X_ENVIRONMENT_KEY=environment_.api_key) + + # Create a segment with a rule (but no existing feature segment override) + segment = Segment.objects.create( + name="new_segment", + project=project, + description="New segment for testing creation", + ) + segment_rule = SegmentRule.objects.create( + segment=segment, type=SegmentRule.ALL_RULE + ) + Condition.objects.create( + rule=segment_rule, + property="user_type", + operator="EQUAL", + value="premium", + ) + + # Verify FeatureSegment doesn't exist yet + assert not FeatureSegment.objects.filter( + feature=feature, segment=segment, environment=environment_ + ).exists() + + # Create an identity that matches the segment + identity = Identity.objects.create( + identifier="premium_user", environment=environment_ + ) + identity.update_traits([{"trait_key": "user_type", "trait_value": "premium"}]) + + url = reverse( + "api-experiments:update-flag-v1", + kwargs={"environment_id": environment_.id}, + ) + + data = { + "feature": {"name": feature.name}, + "segment": {"id": segment.id, "priority": 10}, + "enabled": True, + "value": {"type": "string", "string_value": "premium_feature"}, + } + + # When + response = staff_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then - Should succeed and CREATE the FeatureSegment + assert response.status_code == status.HTTP_204_NO_CONTENT + + # Verify the segment override was created and is applied via SDK + flag_state = get_identity_feature_state_from_sdk( + sdk_client, identity.identifier, feature.name + ) + assert flag_state["enabled"] is True + assert flag_state["feature_state_value"] == "premium_feature" + + +# Update Multiple Feature States Tests + + +@pytest.mark.parametrize( + "environment_", + (lazy_fixture("environment"), lazy_fixture("environment_v2_versioning")), +) +def test_update_feature_states_creates_new_segment_overrides( + staff_client: APIClient, + feature: Feature, + environment_: Environment, + project: Project, + with_environment_permissions: WithEnvironmentPermissionsCallable, +) -> None: + # Given + with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] + + # Create SDK client for this environment + sdk_client = APIClient() + sdk_client.credentials(HTTP_X_ENVIRONMENT_KEY=environment_.api_key) + + # Create two segments that don't have feature overrides yet + segment1 = Segment.objects.create( + name="vip_segment", project=project, description="VIP users" + ) + segment_rule1 = SegmentRule.objects.create( + segment=segment1, type=SegmentRule.ALL_RULE + ) + Condition.objects.create( + rule=segment_rule1, + property="tier", + operator="EQUAL", + value="vip", + ) + + segment2 = Segment.objects.create( + name="beta_segment", project=project, description="Beta testers" + ) + segment_rule2 = SegmentRule.objects.create( + segment=segment2, type=SegmentRule.ALL_RULE + ) + Condition.objects.create( + rule=segment_rule2, + property="beta", + operator="EQUAL", + value="true", + ) + + # Verify neither FeatureSegment exists yet + assert not FeatureSegment.objects.filter( + feature=feature, segment=segment1, environment=environment_ + ).exists() + assert not FeatureSegment.objects.filter( + feature=feature, segment=segment2, environment=environment_ + ).exists() + + # Create identities that match the segments + vip_identity = Identity.objects.create( + identifier="vip_user", environment=environment_ + ) + vip_identity.update_traits([{"trait_key": "tier", "trait_value": "vip"}]) + + beta_identity = Identity.objects.create( + identifier="beta_user", environment=environment_ + ) + beta_identity.update_traits([{"trait_key": "beta", "trait_value": "true"}]) + + url = reverse( + "api-experiments:update-flag-v2", + kwargs={"environment_id": environment_.id}, + ) + + # Batch update with environment default + 2 new segment overrides + data = { + "feature": {"name": feature.name}, + "environment_default": { + "enabled": False, + "value": {"type": "string", "string_value": "default"}, + }, + "segment_overrides": [ + { + "segment_id": segment1.id, + "priority": 1, + "enabled": True, + "value": {"type": "string", "string_value": "vip_feature"}, + }, + { + "segment_id": segment2.id, + "priority": 2, + "enabled": True, + "value": {"type": "string", "string_value": "beta_feature"}, + }, + ], + } + + # When + response = staff_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_204_NO_CONTENT + + # Verify both segment overrides were created and are applied via SDK + vip_flag = get_identity_feature_state_from_sdk( + sdk_client, vip_identity.identifier, feature.name + ) + assert vip_flag["enabled"] is True + assert vip_flag["feature_state_value"] == "vip_feature" + + beta_flag = get_identity_feature_state_from_sdk( + sdk_client, beta_identity.identifier, feature.name + ) + assert beta_flag["enabled"] is True + assert beta_flag["feature_state_value"] == "beta_feature" + + +@pytest.mark.parametrize( + "environment_", + (lazy_fixture("environment"), lazy_fixture("environment_v2_versioning")), +) +def test_update_feature_states_environment_default_only( + staff_client: APIClient, + feature: Feature, + environment_: Environment, + with_environment_permissions: WithEnvironmentPermissionsCallable, +) -> None: + # Given + with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] + url = reverse( + "api-experiments:update-flag-v2", + kwargs={"environment_id": environment_.id}, + ) + + # Test with just environment default (no segment overrides) + data = { + "feature": {"name": feature.name}, + "environment_default": { + "enabled": True, + "value": {"type": "integer", "string_value": "100"}, + }, + } + + # When + response = staff_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_204_NO_CONTENT + + # Verify the actual state in the database + latest_flags = get_environment_flags_list( + environment=environment_, feature_name=feature.name + ) + + # Find environment default + env_default = [fs for fs in latest_flags if fs.feature_segment is None][0] + assert env_default.enabled is True + assert env_default.get_feature_state_value() == 100 From d1288621e071f30cbd3fe75453b8d18ffe64fde2 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 19 Nov 2025 14:13:03 +0000 Subject: [PATCH 03/26] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- api/features/versioning/versioning_service.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/features/versioning/versioning_service.py b/api/features/versioning/versioning_service.py index f9b2c78267a0..0eaa3f8e7714 100644 --- a/api/features/versioning/versioning_service.py +++ b/api/features/versioning/versioning_service.py @@ -458,6 +458,7 @@ def _update_flag_v2_for_versioning_v1( "segment_overrides": updated_segments, } + def get_updated_feature_states_for_version( version: EnvironmentFeatureVersion, ) -> list[FeatureState]: @@ -504,7 +505,6 @@ def multivariate_values_changed( return changed_feature_states - def _get_feature_states_queryset( environment: "Environment", feature_name: str | None = None, From 4e6cec8fe55cf1daf53951a8d82a72d5e60f3032 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Wed, 26 Nov 2025 10:58:32 +0000 Subject: [PATCH 04/26] Typing fixes --- api/environments/identities/models.py | 1 + api/features/feature_states/serializers.py | 17 ++++++++--------- api/features/feature_states/views.py | 4 ++-- api/features/versioning/versioning_service.py | 4 ++-- .../test_unit_feature_states_views.py | 8 ++++---- 5 files changed, 17 insertions(+), 17 deletions(-) diff --git a/api/environments/identities/models.py b/api/environments/identities/models.py index 73b374f619ac..7cb1ea74214f 100644 --- a/api/environments/identities/models.py +++ b/api/environments/identities/models.py @@ -233,6 +233,7 @@ def generate_traits( def update_traits( self, + # TODO: this typing isn't true as it also supports regular dicts like {"trait_key": "foo", "trait_value": "bar"} trait_data_items: list[SDKTraitData], ) -> list[Trait]: """ diff --git a/api/features/feature_states/serializers.py b/api/features/feature_states/serializers.py index aeb2f987927e..8da1c62c1824 100644 --- a/api/features/feature_states/serializers.py +++ b/api/features/feature_states/serializers.py @@ -1,5 +1,6 @@ import typing +from django.db.models import Q from rest_framework import serializers from environments.models import Environment @@ -16,20 +17,18 @@ class BaseFeatureUpdateSerializer(serializers.Serializer): # type: ignore[type-arg] def get_feature(self) -> Feature: feature_data = self.validated_data["feature"] - environment: Environment = self.context.get("environment") + environment: Environment = self.context.get("environment") # type: ignore[assignment] if not environment: raise serializers.ValidationError("Environment context is required") try: - if "id" in feature_data: - return Feature.objects.get( - id=feature_data["id"], project_id=environment.project_id - ) - else: - return Feature.objects.get( - name=feature_data["name"], project_id=environment.project_id - ) + # TODO: strip out the id vs name piece as this is ugly as heck. + query = Q(project_id=environment.project_id) & ( + Q(id=feature_data.get("id")) | Q(name=feature_data.get("name")) + ) + feature: Feature = Feature.objects.get(query) + return feature except Feature.DoesNotExist: identifier = feature_data.get("id") or feature_data.get("name") raise serializers.ValidationError( diff --git a/api/features/feature_states/views.py b/api/features/feature_states/views.py index 0d941107ba24..153881c53f68 100644 --- a/api/features/feature_states/views.py +++ b/api/features/feature_states/views.py @@ -46,7 +46,7 @@ ) }, tags=["Experimental - Feature States"], -) +) # type: ignore[misc] @api_view(http_method_names=["POST"]) def update_flag_v1(request: Request, environment_id: int) -> Response: """ @@ -113,7 +113,7 @@ def update_flag_v1(request: Request, environment_id: int) -> Response: ) }, tags=["Experimental - Feature States"], -) +) # type: ignore[misc] @api_view(http_method_names=["POST"]) def update_flag_v2(request: Request, environment_id: int) -> Response: """ diff --git a/api/features/versioning/versioning_service.py b/api/features/versioning/versioning_service.py index 0eaa3f8e7714..cae7eff25720 100644 --- a/api/features/versioning/versioning_service.py +++ b/api/features/versioning/versioning_service.py @@ -202,7 +202,7 @@ def _update_flag_for_versioning_v1( ) # FeatureStateValue is automatically created via signal - target_feature_state = FeatureState.objects.create( + target_feature_state: FeatureState = FeatureState.objects.create( feature=feature, environment=environment, feature_segment=feature_segment, @@ -281,7 +281,7 @@ def _create_segment_override( priority=priority if priority is not None else 0, ) - segment_state = FeatureState.objects.create( + segment_state: FeatureState = FeatureState.objects.create( feature=feature, environment=environment, feature_segment=feature_segment, diff --git a/api/tests/unit/features/feature_states/test_unit_feature_states_views.py b/api/tests/unit/features/feature_states/test_unit_feature_states_views.py index 4c347cb14113..75a5d5f04171 100644 --- a/api/tests/unit/features/feature_states/test_unit_feature_states_views.py +++ b/api/tests/unit/features/feature_states/test_unit_feature_states_views.py @@ -354,7 +354,7 @@ def test_update_flag_segment_override_by_name( # Create an identity that matches the segment identity = Identity.objects.create(identifier="test_user", environment=environment_) - identity.update_traits([{"trait_key": "email", "trait_value": "test@example.com"}]) + identity.update_traits([{"trait_key": "email", "trait_value": "test@example.com"}]) # type: ignore[typeddict-item] url = reverse( "api-experiments:update-flag-v1", @@ -427,7 +427,7 @@ def test_update_flag_segment_override_creates_feature_segment_if_not_exists( identity = Identity.objects.create( identifier="premium_user", environment=environment_ ) - identity.update_traits([{"trait_key": "user_type", "trait_value": "premium"}]) + identity.update_traits([{"trait_key": "user_type", "trait_value": "premium"}]) # type: ignore[typeddict-item] url = reverse( "api-experiments:update-flag-v1", @@ -517,12 +517,12 @@ def test_update_feature_states_creates_new_segment_overrides( vip_identity = Identity.objects.create( identifier="vip_user", environment=environment_ ) - vip_identity.update_traits([{"trait_key": "tier", "trait_value": "vip"}]) + vip_identity.update_traits([{"trait_key": "tier", "trait_value": "vip"}]) # type: ignore[typeddict-item] beta_identity = Identity.objects.create( identifier="beta_user", environment=environment_ ) - beta_identity.update_traits([{"trait_key": "beta", "trait_value": "true"}]) + beta_identity.update_traits([{"trait_key": "beta", "trait_value": "true"}]) # type: ignore[typeddict-item] url = reverse( "api-experiments:update-flag-v2", From b0c0a6a1c477c2a30de71bb2e9d6360487f65c43 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Wed, 26 Nov 2025 11:17:26 +0000 Subject: [PATCH 05/26] Update SegmentSerializer name --- api/features/feature_states/serializers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/features/feature_states/serializers.py b/api/features/feature_states/serializers.py index 8da1c62c1824..2da21f4274b8 100644 --- a/api/features/feature_states/serializers.py +++ b/api/features/feature_states/serializers.py @@ -66,7 +66,7 @@ def validate(self, data: dict) -> dict: # type: ignore[type-arg] return data -class SegmentSerializer(serializers.Serializer): # type: ignore[type-arg] +class FeatureUpdateSegmentDataSerializer(serializers.Serializer): # type: ignore[type-arg] id = serializers.IntegerField(required=True) priority = serializers.IntegerField(required=False, allow_null=True) @@ -98,7 +98,7 @@ def get_typed_value(self) -> typing.Any: class UpdateFlagSerializer(BaseFeatureUpdateSerializer): feature = FeatureIdentifierSerializer(required=True) - segment = SegmentSerializer(required=False) + segment = FeatureUpdateSegmentDataSerializer(required=False) enabled = serializers.BooleanField(required=True) value = FeatureValueSerializer(required=True) From 2c03d5e6b1defa121c3ecb67f84594d74657d6c4 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Fri, 28 Nov 2025 13:22:49 +0530 Subject: [PATCH 06/26] test: add comprehensive test coverage for experimental flag endpoints - Parameterize test_update_flag_by_name for all value types (integer, string, boolean) - Remove unsupported float type from serializer (model doesn't support it) - Add test_serializers.py with unit tests for edge cases - Add test for duplicate segment_id validation - Add tests for updating existing segment overrides with priority - Add unit tests for boolean string conversion and replica path - Refactor _update_flag_for_versioning_v2 to separate segment vs environment default cases for cleaner code and full coverage --- api/features/feature_states/serializers.py | 8 +- api/features/versioning/versioning_service.py | 25 +-- .../feature_states/test_serializers.py | 80 +++++++ .../test_unit_feature_states_views.py | 195 +++++++++++++++++- ...test_unit_versioning_versioning_service.py | 35 ++++ 5 files changed, 324 insertions(+), 19 deletions(-) create mode 100644 api/tests/unit/features/feature_states/test_serializers.py diff --git a/api/features/feature_states/serializers.py b/api/features/feature_states/serializers.py index 2da21f4274b8..1368c856b327 100644 --- a/api/features/feature_states/serializers.py +++ b/api/features/feature_states/serializers.py @@ -75,7 +75,7 @@ class FeatureValueSerializer(serializers.Serializer): # type: ignore[type-arg] """Value is always a string; type field indicates how to parse it.""" type = serializers.ChoiceField( - choices=["integer", "string", "boolean", "float"], required=True + choices=["integer", "string", "boolean"], required=True ) string_value = serializers.CharField(required=True, allow_blank=True) @@ -90,10 +90,8 @@ def get_typed_value(self) -> typing.Any: return int(string_val) case "boolean": return string_val.lower() == "true" - case "float": - return float(string_val) - - return string_val + case _: + raise ValueError(f"Unsupported value type: {value_type}") class UpdateFlagSerializer(BaseFeatureUpdateSerializer): diff --git a/api/features/versioning/versioning_service.py b/api/features/versioning/versioning_service.py index cae7eff25720..792cc8c53449 100644 --- a/api/features/versioning/versioning_service.py +++ b/api/features/versioning/versioning_service.py @@ -4,7 +4,7 @@ from django.db.models import Prefetch, Q, QuerySet from django.utils import timezone -from core.constants import BOOLEAN, FLOAT, INTEGER, STRING +from core.constants import BOOLEAN, INTEGER, STRING from environments.models import Environment from features.models import Feature, FeatureState, FeatureStateValue from features.versioning.dataclasses import ( @@ -128,12 +128,13 @@ def _update_flag_for_versioning_v2( created_by_api_key=change_set.api_key, ) - try: - target_feature_state: FeatureState = new_version.feature_states.get( - feature_segment__segment_id=change_set.segment_id, - ) - except FeatureState.DoesNotExist: - if change_set.segment_id: + if change_set.segment_id: + # Segment override - may or may not exist + try: + target_feature_state: FeatureState = new_version.feature_states.get( + feature_segment__segment_id=change_set.segment_id, + ) + except FeatureState.DoesNotExist: segment = Segment.objects.get(id=change_set.segment_id) feature_segment = FeatureSegment.objects.create( @@ -153,8 +154,11 @@ def _update_flag_for_versioning_v2( environment_feature_version=new_version, enabled=change_set.enabled, ) - else: - raise + else: + # Environment default - always exists + target_feature_state = new_version.feature_states.get( + feature_segment__isnull=True, + ) target_feature_state.enabled = change_set.enabled target_feature_state.save() @@ -254,9 +258,6 @@ def _update_feature_state_value( else: fsv.boolean_value = str(value).lower() == "true" fsv.type = BOOLEAN - case "float": - fsv.float_value = float(value) - fsv.type = FLOAT fsv.save() diff --git a/api/tests/unit/features/feature_states/test_serializers.py b/api/tests/unit/features/feature_states/test_serializers.py new file mode 100644 index 000000000000..bc48f9fa77f8 --- /dev/null +++ b/api/tests/unit/features/feature_states/test_serializers.py @@ -0,0 +1,80 @@ +from unittest.mock import Mock + +import pytest + +from environments.models import Environment +from features.feature_states.serializers import ( + FeatureValueSerializer, + UpdateFlagSerializer, + UpdateFlagV2Serializer, +) +from features.models import Feature +from features.versioning.dataclasses import FlagChangeSet + + +def test_get_feature_raises_error_when_environment_not_in_context( + feature: Feature, +) -> None: + serializer = UpdateFlagSerializer( + data={ + "feature": {"name": feature.name}, + "enabled": True, + "value": {"type": "string", "string_value": "test"}, + }, + context={}, # No environment + ) + serializer.is_valid() + + with pytest.raises(Exception) as exc_info: + serializer.get_feature() + + assert "Environment context is required" in str(exc_info.value) + + +def test_add_audit_fields_sets_api_key_for_non_user( + environment: Environment, + feature: Feature, +) -> None: + # Create a mock API key user (not FFAdminUser) + mock_api_key = Mock() + mock_api_key.key = "test_api_key" + mock_request = Mock() + mock_request.user = mock_api_key + + serializer = UpdateFlagSerializer( + data={ + "feature": {"name": feature.name}, + "enabled": True, + "value": {"type": "string", "string_value": "test"}, + }, + context={"environment": environment, "request": mock_request}, + ) + serializer.is_valid() + + change_set = FlagChangeSet( + enabled=True, + feature_state_value="test", + type_="string", + ) + serializer._add_audit_fields(change_set) + + assert change_set.api_key == "test_api_key" + assert change_set.user is None + + +def test_validate_segment_overrides_returns_empty_list() -> None: + serializer = UpdateFlagV2Serializer() + result = serializer.validate_segment_overrides([]) + + assert result == [] + + +def test_get_typed_value_raises_error_for_unsupported_type() -> None: + serializer = FeatureValueSerializer() + # Bypass validation by setting validated_data directly + serializer._validated_data = {"type": "unsupported", "string_value": "test"} # type: ignore[attr-defined] + + with pytest.raises(ValueError) as exc_info: + serializer.get_typed_value() + + assert "Unsupported value type: unsupported" in str(exc_info.value) diff --git a/api/tests/unit/features/feature_states/test_unit_feature_states_views.py b/api/tests/unit/features/feature_states/test_unit_feature_states_views.py index 75a5d5f04171..e42ef0560e2a 100644 --- a/api/tests/unit/features/feature_states/test_unit_feature_states_views.py +++ b/api/tests/unit/features/feature_states/test_unit_feature_states_views.py @@ -61,6 +61,14 @@ def get_identity_feature_state_from_sdk( raise ValueError(f"Feature {feature_name} not found in identity flags response") +@pytest.mark.parametrize( + "value_type,string_value,expected_value", + [ + ("integer", "42", 42), + ("string", "hello", "hello"), + ("boolean", "true", True), + ], +) @pytest.mark.parametrize( "environment_", (lazy_fixture("environment"), lazy_fixture("environment_v2_versioning")), @@ -70,6 +78,9 @@ def test_update_flag_by_name( feature: Feature, environment_: Environment, with_environment_permissions: WithEnvironmentPermissionsCallable, + value_type: str, + string_value: str, + expected_value: typing.Any, ) -> None: # Given with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] @@ -81,7 +92,7 @@ def test_update_flag_by_name( data = { "feature": {"name": feature.name}, "enabled": True, - "value": {"type": "integer", "string_value": "42"}, + "value": {"type": value_type, "string_value": string_value}, } # When @@ -97,7 +108,7 @@ def test_update_flag_by_name( ) assert latest_flags[0].enabled is True - assert latest_flags[0].get_feature_state_value() == 42 + assert latest_flags[0].get_feature_state_value() == expected_value @pytest.mark.parametrize( @@ -617,3 +628,183 @@ def test_update_feature_states_environment_default_only( env_default = [fs for fs in latest_flags if fs.feature_segment is None][0] assert env_default.enabled is True assert env_default.get_feature_state_value() == 100 + + +def test_update_feature_states_rejects_duplicate_segment_ids( + staff_client: APIClient, + feature: Feature, + environment: Environment, + project: Project, + with_environment_permissions: WithEnvironmentPermissionsCallable, +) -> None: + # Given + with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] + + segment1 = Segment.objects.create(name="segment1", project=project) + + url = reverse( + "api-experiments:update-flag-v2", + kwargs={"environment_id": environment.id}, + ) + + # Request with duplicate segment_id + data = { + "feature": {"name": feature.name}, + "environment_default": { + "enabled": True, + "value": {"type": "string", "string_value": "default"}, + }, + "segment_overrides": [ + { + "segment_id": segment1.id, + "enabled": True, + "value": {"type": "string", "string_value": "value1"}, + }, + { + "segment_id": segment1.id, # Duplicate! + "enabled": False, + "value": {"type": "string", "string_value": "value2"}, + }, + ], + } + + # When + response = staff_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "Duplicate segment_id values are not allowed" in str(response.json()) + + +@pytest.mark.parametrize( + "environment_", + (lazy_fixture("environment"), lazy_fixture("environment_v2_versioning")), +) +def test_update_existing_segment_override_with_priority_v1( + staff_client: APIClient, + feature: Feature, + environment_: Environment, + project: Project, + with_environment_permissions: WithEnvironmentPermissionsCallable, +) -> None: + """Test updating an existing segment override with a new priority using V1 endpoint.""" + # Given + with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] + + segment = Segment.objects.create(name="priority_segment", project=project) + + url = reverse( + "api-experiments:update-flag-v1", + kwargs={"environment_id": environment_.id}, + ) + + # First, create the segment override + create_data = { + "feature": {"name": feature.name}, + "segment": {"id": segment.id, "priority": 5}, + "enabled": True, + "value": {"type": "integer", "string_value": "100"}, + } + response = staff_client.post( + url, data=json.dumps(create_data), content_type="application/json" + ) + assert response.status_code == status.HTTP_204_NO_CONTENT + + # When - Update the same segment override with new priority + update_data = { + "feature": {"name": feature.name}, + "segment": {"id": segment.id, "priority": 1}, # Changed priority + "enabled": False, # Changed enabled + "value": {"type": "integer", "string_value": "200"}, # Changed value + } + response = staff_client.post( + url, data=json.dumps(update_data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_204_NO_CONTENT + + # Verify the update - get the latest feature segment (V2 versioning creates new ones) + feature_segment = ( + FeatureSegment.objects.filter( + feature=feature, segment=segment, environment=environment_ + ) + .order_by("-id") + .first() + ) + assert feature_segment is not None + assert feature_segment.priority == 1 + + +@pytest.mark.parametrize( + "environment_", + (lazy_fixture("environment"), lazy_fixture("environment_v2_versioning")), +) +def test_update_existing_segment_override_with_priority_v2( + staff_client: APIClient, + feature: Feature, + environment_: Environment, + project: Project, + with_environment_permissions: WithEnvironmentPermissionsCallable, +) -> None: + """Test updating an existing segment override with a new priority using V2 endpoint.""" + # Given + with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] + + segment = Segment.objects.create(name="priority_segment_v2", project=project) + + # First, create the segment override using V1 endpoint + v1_url = reverse( + "api-experiments:update-flag-v1", + kwargs={"environment_id": environment_.id}, + ) + create_data = { + "feature": {"name": feature.name}, + "segment": {"id": segment.id, "priority": 5}, + "enabled": True, + "value": {"type": "string", "string_value": "initial"}, + } + response = staff_client.post( + v1_url, data=json.dumps(create_data), content_type="application/json" + ) + assert response.status_code == status.HTTP_204_NO_CONTENT + + # When - Update the existing segment override using V2 endpoint + v2_url = reverse( + "api-experiments:update-flag-v2", + kwargs={"environment_id": environment_.id}, + ) + update_data = { + "feature": {"name": feature.name}, + "environment_default": { + "enabled": True, + "value": {"type": "string", "string_value": "default"}, + }, + "segment_overrides": [ + { + "segment_id": segment.id, + "priority": 2, # Changed priority + "enabled": False, # Changed enabled + "value": {"type": "string", "string_value": "updated"}, + }, + ], + } + response = staff_client.post( + v2_url, data=json.dumps(update_data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_204_NO_CONTENT + + # Verify the priority was updated - get the latest feature segment + feature_segment = ( + FeatureSegment.objects.filter( + feature=feature, segment=segment, environment=environment_ + ) + .order_by("-id") + .first() + ) + assert feature_segment is not None + assert feature_segment.priority == 2 diff --git a/api/tests/unit/features/versioning/test_unit_versioning_versioning_service.py b/api/tests/unit/features/versioning/test_unit_versioning_versioning_service.py index 62b591a1c95f..0ef5a431369a 100644 --- a/api/tests/unit/features/versioning/test_unit_versioning_versioning_service.py +++ b/api/tests/unit/features/versioning/test_unit_versioning_versioning_service.py @@ -600,3 +600,38 @@ def test_get_updated_feature_states_for_version_detects_segment_override_multiva assert updated_feature_states[0].feature_segment is not None assert updated_feature_states[0].feature_segment.segment == segment assert updated_feature_states[0].get_feature_state_value() == "new_control_value" + + +def test_update_feature_state_value_with_boolean_string( + feature: Feature, + environment: Environment, +) -> None: + """Test _update_feature_state_value handles boolean passed as string.""" + from features.versioning.versioning_service import _update_feature_state_value + + feature_state = FeatureState.objects.get( + feature=feature, environment=environment, feature_segment=None + ) + fsv = feature_state.feature_state_value + + # Call with string "true" instead of bool True + _update_feature_state_value(fsv, "true", "boolean") + + fsv.refresh_from_db() + assert fsv.boolean_value is True + + +def test_get_environment_flags_list_with_replica( + feature: Feature, + environment: Environment, +) -> None: + """Test get_environment_flags_list with from_replica=True.""" + # This just verifies the code path works - actual replica behavior + # depends on database configuration + result = get_environment_flags_list( + environment=environment, + from_replica=True, + ) + + assert len(result) >= 1 + assert result[0].feature == feature From fc21bc09b289c93cdf024bde7db0a49371883116 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Fri, 28 Nov 2025 14:40:06 +0530 Subject: [PATCH 07/26] refactor: remove SDK integration patterns from unit tests Replace SDK endpoint calls with direct database verification using get_environment_flags_list. This keeps the tests as proper unit tests rather than integration tests. - Remove sdk_client and sdk_client_factory fixtures - Remove get_identity_feature_state_from_sdk helper function - Remove Identity, Condition, SegmentRule imports and usage - Simplify segment override tests to verify database state directly --- .../test_unit_feature_states_views.py | 175 ++++-------------- 1 file changed, 39 insertions(+), 136 deletions(-) diff --git a/api/tests/unit/features/feature_states/test_unit_feature_states_views.py b/api/tests/unit/features/feature_states/test_unit_feature_states_views.py index e42ef0560e2a..d4ca10e444af 100644 --- a/api/tests/unit/features/feature_states/test_unit_feature_states_views.py +++ b/api/tests/unit/features/feature_states/test_unit_feature_states_views.py @@ -8,59 +8,16 @@ from rest_framework import status from rest_framework.test import APIClient -from environments.identities.models import Identity from environments.models import Environment from features.models import Feature, FeatureSegment from features.versioning.versioning_service import ( get_environment_flags_list, ) from projects.models import Project -from segments.models import Condition, Segment, SegmentRule +from segments.models import Segment from tests.types import WithEnvironmentPermissionsCallable -@pytest.fixture() -def sdk_client_factory() -> typing.Callable[[Environment], APIClient]: - """Factory to create SDK clients for different environments.""" - - def _create_sdk_client(environment: Environment) -> APIClient: - client = APIClient() - client.credentials(HTTP_X_ENVIRONMENT_KEY=environment.api_key) - return client - - return _create_sdk_client - - -@pytest.fixture() -def sdk_client( - environment: Environment, - sdk_client_factory: typing.Callable[[Environment], APIClient], -) -> APIClient: - """API client configured with SDK authentication for the default environment.""" - return sdk_client_factory(environment) - - -def get_identity_feature_state_from_sdk( - sdk_client: APIClient, identifier: str, feature_name: str -) -> typing.Dict[str, typing.Any]: - """Helper to get a specific feature's state for an identity via SDK endpoint.""" - identities_url = reverse("api-v1:sdk-identities") - data = {"identifier": identifier} - - response = sdk_client.post( - identities_url, data=json.dumps(data), content_type="application/json" - ) - assert response.status_code == status.HTTP_200_OK - response_json = response.json() - - # Find the specific feature in the flags list - for flag in response_json["flags"]: - if flag["feature"]["name"] == feature_name: - return flag # type: ignore[no-any-return] - - raise ValueError(f"Feature {feature_name} not found in identity flags response") - - @pytest.mark.parametrize( "value_type,string_value,expected_value", [ @@ -345,27 +302,9 @@ def test_update_flag_segment_override_by_name( # Given with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] - # Create SDK client for this environment - sdk_client = APIClient() - sdk_client.credentials(HTTP_X_ENVIRONMENT_KEY=environment_.api_key) - - # Create a segment with a rule that matches identities with email containing "test" segment = Segment.objects.create( name="test_segment", project=project, description="Test segment" ) - segment_rule = SegmentRule.objects.create( - segment=segment, type=SegmentRule.ALL_RULE - ) - Condition.objects.create( - rule=segment_rule, - property="email", - operator="CONTAINS", - value="test", - ) - - # Create an identity that matches the segment - identity = Identity.objects.create(identifier="test_user", environment=environment_) - identity.update_traits([{"trait_key": "email", "trait_value": "test@example.com"}]) # type: ignore[typeddict-item] url = reverse( "api-experiments:update-flag-v1", @@ -387,12 +326,17 @@ def test_update_flag_segment_override_by_name( # Then assert response.status_code == status.HTTP_204_NO_CONTENT - # Verify the segment override is applied to the identity via SDK - flag_state = get_identity_feature_state_from_sdk( - sdk_client, identity.identifier, feature.name + # Verify the segment override was created + latest_flags = get_environment_flags_list( + environment=environment_, feature_name=feature.name ) - assert flag_state["enabled"] is False - assert flag_state["feature_state_value"] == 999 + segment_override = [ + fs + for fs in latest_flags + if fs.feature_segment and fs.feature_segment.segment_id == segment.id + ][0] + assert segment_override.enabled is False + assert segment_override.get_feature_state_value() == 999 @pytest.mark.parametrize( @@ -409,37 +353,17 @@ def test_update_flag_segment_override_creates_feature_segment_if_not_exists( # Given with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] - # Create SDK client for this environment - sdk_client = APIClient() - sdk_client.credentials(HTTP_X_ENVIRONMENT_KEY=environment_.api_key) - - # Create a segment with a rule (but no existing feature segment override) segment = Segment.objects.create( name="new_segment", project=project, description="New segment for testing creation", ) - segment_rule = SegmentRule.objects.create( - segment=segment, type=SegmentRule.ALL_RULE - ) - Condition.objects.create( - rule=segment_rule, - property="user_type", - operator="EQUAL", - value="premium", - ) # Verify FeatureSegment doesn't exist yet assert not FeatureSegment.objects.filter( feature=feature, segment=segment, environment=environment_ ).exists() - # Create an identity that matches the segment - identity = Identity.objects.create( - identifier="premium_user", environment=environment_ - ) - identity.update_traits([{"trait_key": "user_type", "trait_value": "premium"}]) # type: ignore[typeddict-item] - url = reverse( "api-experiments:update-flag-v1", kwargs={"environment_id": environment_.id}, @@ -460,12 +384,17 @@ def test_update_flag_segment_override_creates_feature_segment_if_not_exists( # Then - Should succeed and CREATE the FeatureSegment assert response.status_code == status.HTTP_204_NO_CONTENT - # Verify the segment override was created and is applied via SDK - flag_state = get_identity_feature_state_from_sdk( - sdk_client, identity.identifier, feature.name + # Verify the segment override was created + latest_flags = get_environment_flags_list( + environment=environment_, feature_name=feature.name ) - assert flag_state["enabled"] is True - assert flag_state["feature_state_value"] == "premium_feature" + segment_override = [ + fs + for fs in latest_flags + if fs.feature_segment and fs.feature_segment.segment_id == segment.id + ][0] + assert segment_override.enabled is True + assert segment_override.get_feature_state_value() == "premium_feature" # Update Multiple Feature States Tests @@ -485,36 +414,13 @@ def test_update_feature_states_creates_new_segment_overrides( # Given with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] - # Create SDK client for this environment - sdk_client = APIClient() - sdk_client.credentials(HTTP_X_ENVIRONMENT_KEY=environment_.api_key) - # Create two segments that don't have feature overrides yet segment1 = Segment.objects.create( name="vip_segment", project=project, description="VIP users" ) - segment_rule1 = SegmentRule.objects.create( - segment=segment1, type=SegmentRule.ALL_RULE - ) - Condition.objects.create( - rule=segment_rule1, - property="tier", - operator="EQUAL", - value="vip", - ) - segment2 = Segment.objects.create( name="beta_segment", project=project, description="Beta testers" ) - segment_rule2 = SegmentRule.objects.create( - segment=segment2, type=SegmentRule.ALL_RULE - ) - Condition.objects.create( - rule=segment_rule2, - property="beta", - operator="EQUAL", - value="true", - ) # Verify neither FeatureSegment exists yet assert not FeatureSegment.objects.filter( @@ -524,17 +430,6 @@ def test_update_feature_states_creates_new_segment_overrides( feature=feature, segment=segment2, environment=environment_ ).exists() - # Create identities that match the segments - vip_identity = Identity.objects.create( - identifier="vip_user", environment=environment_ - ) - vip_identity.update_traits([{"trait_key": "tier", "trait_value": "vip"}]) # type: ignore[typeddict-item] - - beta_identity = Identity.objects.create( - identifier="beta_user", environment=environment_ - ) - beta_identity.update_traits([{"trait_key": "beta", "trait_value": "true"}]) # type: ignore[typeddict-item] - url = reverse( "api-experiments:update-flag-v2", kwargs={"environment_id": environment_.id}, @@ -571,18 +466,26 @@ def test_update_feature_states_creates_new_segment_overrides( # Then assert response.status_code == status.HTTP_204_NO_CONTENT - # Verify both segment overrides were created and are applied via SDK - vip_flag = get_identity_feature_state_from_sdk( - sdk_client, vip_identity.identifier, feature.name + # Verify both segment overrides were created + latest_flags = get_environment_flags_list( + environment=environment_, feature_name=feature.name ) - assert vip_flag["enabled"] is True - assert vip_flag["feature_state_value"] == "vip_feature" - beta_flag = get_identity_feature_state_from_sdk( - sdk_client, beta_identity.identifier, feature.name - ) - assert beta_flag["enabled"] is True - assert beta_flag["feature_state_value"] == "beta_feature" + vip_override = [ + fs + for fs in latest_flags + if fs.feature_segment and fs.feature_segment.segment_id == segment1.id + ][0] + assert vip_override.enabled is True + assert vip_override.get_feature_state_value() == "vip_feature" + + beta_override = [ + fs + for fs in latest_flags + if fs.feature_segment and fs.feature_segment.segment_id == segment2.id + ][0] + assert beta_override.enabled is True + assert beta_override.get_feature_state_value() == "beta_feature" @pytest.mark.parametrize( From bf7bd7b3a7e8ae992ac499ab4a788805e24cc8b3 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Fri, 28 Nov 2025 15:13:35 +0530 Subject: [PATCH 08/26] misc --- api/features/feature_states/serializers.py | 2 -- api/features/versioning/dataclasses.py | 27 +++++++------------ api/features/versioning/versioning_service.py | 23 +--------------- ...test_unit_versioning_versioning_service.py | 20 -------------- 4 files changed, 11 insertions(+), 61 deletions(-) diff --git a/api/features/feature_states/serializers.py b/api/features/feature_states/serializers.py index 1368c856b327..c8aa67145878 100644 --- a/api/features/feature_states/serializers.py +++ b/api/features/feature_states/serializers.py @@ -72,8 +72,6 @@ class FeatureUpdateSegmentDataSerializer(serializers.Serializer): # type: ignor class FeatureValueSerializer(serializers.Serializer): # type: ignore[type-arg] - """Value is always a string; type field indicates how to parse it.""" - type = serializers.ChoiceField( choices=["integer", "string", "boolean"], required=True ) diff --git a/api/features/versioning/dataclasses.py b/api/features/versioning/dataclasses.py index ba30a71f01bf..ba4485dc10f4 100644 --- a/api/features/versioning/dataclasses.py +++ b/api/features/versioning/dataclasses.py @@ -1,5 +1,5 @@ import typing -from dataclasses import dataclass +from dataclasses import dataclass, field from datetime import datetime from pydantic import BaseModel, computed_field @@ -21,22 +21,23 @@ def is_environment_default(self) -> bool: @dataclass -class FlagChangeSet: +class AuditFieldsMixin: + user: typing.Optional["FFAdminUser"] = field(default=None, kw_only=True) + api_key: typing.Optional["MasterAPIKey"] = field(default=None, kw_only=True) + + +@dataclass +class FlagChangeSet(AuditFieldsMixin): enabled: bool feature_state_value: typing.Any type_: str - user: typing.Optional["FFAdminUser"] = None - api_key: typing.Optional["MasterAPIKey"] = None - segment_id: int | None = None segment_priority: int | None = None @dataclass class SegmentOverrideChangeSet: - """Represents a single segment override change.""" - segment_id: int enabled: bool feature_state_value: typing.Any @@ -45,17 +46,9 @@ class SegmentOverrideChangeSet: @dataclass -class FlagChangeSetV2: - """Represents V2 feature state changes (environment default + segment overrides).""" - - # Environment default state +class FlagChangeSetV2(AuditFieldsMixin): environment_default_enabled: bool environment_default_value: typing.Any environment_default_type: str - # Segment overrides (list) - segment_overrides: list[SegmentOverrideChangeSet] - - # Audit fields - user: typing.Optional["FFAdminUser"] = None - api_key: typing.Optional["MasterAPIKey"] = None + segment_overrides: list[SegmentOverrideChangeSet] = field(default_factory=list) diff --git a/api/features/versioning/versioning_service.py b/api/features/versioning/versioning_service.py index 792cc8c53449..02f12bf7de01 100644 --- a/api/features/versioning/versioning_service.py +++ b/api/features/versioning/versioning_service.py @@ -146,7 +146,6 @@ def _update_flag_for_versioning_v2( else 0, ) - # FeatureStateValue is automatically created via signal target_feature_state = FeatureState.objects.create( feature=feature, environment=environment, @@ -205,7 +204,6 @@ def _update_flag_for_versioning_v1( else 0, ) - # FeatureStateValue is automatically created via signal target_feature_state: FeatureState = FeatureState.objects.create( feature=feature, environment=environment, @@ -244,7 +242,6 @@ def _update_flag_for_versioning_v1( def _update_feature_state_value( fsv: FeatureStateValue, value: typing.Any, type_: str ) -> None: - """Works for both V1 and V2 changesets.""" match type_: case "string": fsv.string_value = str(value) @@ -253,10 +250,7 @@ def _update_feature_state_value( fsv.integer_value = int(value) fsv.type = INTEGER case "boolean": - if isinstance(value, bool): - fsv.boolean_value = value - else: - fsv.boolean_value = str(value).lower() == "true" + fsv.boolean_value = value fsv.type = BOOLEAN fsv.save() @@ -299,16 +293,9 @@ def _update_segment_priority(feature_state: FeatureState, priority: int) -> None feature_segment.to(priority) -# V2 Update Functions (FlagChangeSetV2) - - def update_flag_v2( environment: Environment, feature: Feature, change_set: FlagChangeSetV2 ) -> dict: # type: ignore[type-arg] - """ - Update multiple feature states (V2 changeset). - Returns a dict with the results. - """ if environment.use_v2_feature_versioning: return _update_flag_v2_for_versioning_v2(environment, feature, change_set) else: @@ -318,10 +305,6 @@ def update_flag_v2( def _update_flag_v2_for_versioning_v2( environment: Environment, feature: Feature, change_set: FlagChangeSetV2 ) -> dict: # type: ignore[type-arg] - """ - V2 changeset update for V2 versioning. - Creates a new version with all changes applied. - """ new_version = EnvironmentFeatureVersion.objects.create( environment=environment, feature=feature, @@ -391,10 +374,6 @@ def _update_flag_v2_for_versioning_v2( def _update_flag_v2_for_versioning_v1( environment: Environment, feature: Feature, change_set: FlagChangeSetV2 ) -> dict: # type: ignore[type-arg] - """ - V2 changeset update for V1 versioning. - Updates all feature states in place. - """ env_default_states = get_environment_flags_dict( environment=environment, feature_name=feature.name, diff --git a/api/tests/unit/features/versioning/test_unit_versioning_versioning_service.py b/api/tests/unit/features/versioning/test_unit_versioning_versioning_service.py index 0ef5a431369a..f6adb5238f62 100644 --- a/api/tests/unit/features/versioning/test_unit_versioning_versioning_service.py +++ b/api/tests/unit/features/versioning/test_unit_versioning_versioning_service.py @@ -602,30 +602,10 @@ def test_get_updated_feature_states_for_version_detects_segment_override_multiva assert updated_feature_states[0].get_feature_state_value() == "new_control_value" -def test_update_feature_state_value_with_boolean_string( - feature: Feature, - environment: Environment, -) -> None: - """Test _update_feature_state_value handles boolean passed as string.""" - from features.versioning.versioning_service import _update_feature_state_value - - feature_state = FeatureState.objects.get( - feature=feature, environment=environment, feature_segment=None - ) - fsv = feature_state.feature_state_value - - # Call with string "true" instead of bool True - _update_feature_state_value(fsv, "true", "boolean") - - fsv.refresh_from_db() - assert fsv.boolean_value is True - - def test_get_environment_flags_list_with_replica( feature: Feature, environment: Environment, ) -> None: - """Test get_environment_flags_list with from_replica=True.""" # This just verifies the code path works - actual replica behavior # depends on database configuration result = get_environment_flags_list( From db2477ebc0bf79920713cfb6a4c35f373849a54a Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Fri, 28 Nov 2025 15:48:13 +0530 Subject: [PATCH 09/26] return 403 if change request is enabled --- api/features/feature_states/views.py | 11 +++ .../test_unit_feature_states_views.py | 69 +++++++++++++++++++ 2 files changed, 80 insertions(+) diff --git a/api/features/feature_states/views.py b/api/features/feature_states/views.py index 153881c53f68..893cbaf1c554 100644 --- a/api/features/feature_states/views.py +++ b/api/features/feature_states/views.py @@ -2,6 +2,7 @@ from drf_yasg.utils import swagger_auto_schema # type: ignore[import-untyped] from rest_framework import status from rest_framework.decorators import api_view +from rest_framework.exceptions import PermissionDenied from rest_framework.request import Request from rest_framework.response import Response @@ -10,6 +11,14 @@ from .serializers import UpdateFlagSerializer, UpdateFlagV2Serializer +def _check_workflow_not_enabled(environment: Environment) -> None: + if environment.is_workflow_enabled: + raise PermissionDenied( + "This endpoint cannot be used when change requests are enabled. " + "Use the change request workflow instead." + ) + + @swagger_auto_schema( method="post", operation_summary="Update single feature state (V1)", @@ -56,6 +65,7 @@ def update_flag_v1(request: Request, environment_id: int) -> Response: Feature identification is provided in the request payload. """ environment = Environment.objects.get(id=environment_id) + _check_workflow_not_enabled(environment) serializer = UpdateFlagSerializer( data=request.data, @@ -124,6 +134,7 @@ def update_flag_v2(request: Request, environment_id: int) -> Response: - Multiple segment overrides with priority ordering """ environment = Environment.objects.get(id=environment_id) + _check_workflow_not_enabled(environment) serializer = UpdateFlagV2Serializer( data=request.data, diff --git a/api/tests/unit/features/feature_states/test_unit_feature_states_views.py b/api/tests/unit/features/feature_states/test_unit_feature_states_views.py index d4ca10e444af..72d7c65058c2 100644 --- a/api/tests/unit/features/feature_states/test_unit_feature_states_views.py +++ b/api/tests/unit/features/feature_states/test_unit_feature_states_views.py @@ -711,3 +711,72 @@ def test_update_existing_segment_override_with_priority_v2( ) assert feature_segment is not None assert feature_segment.priority == 2 + + +# Workflow enabled tests + + +def test_update_flag_v1_returns_403_when_workflow_enabled( + staff_client: APIClient, + feature: Feature, + environment: Environment, + with_environment_permissions: WithEnvironmentPermissionsCallable, +) -> None: + # Given + with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] + environment.minimum_change_request_approvals = 1 + environment.save() + + url = reverse( + "api-experiments:update-flag-v1", + kwargs={"environment_id": environment.id}, + ) + + data = { + "feature": {"name": feature.name}, + "enabled": True, + "value": {"type": "string", "string_value": "test"}, + } + + # When + response = staff_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_403_FORBIDDEN + assert "change requests are enabled" in str(response.json()) + + +def test_update_flag_v2_returns_403_when_workflow_enabled( + staff_client: APIClient, + feature: Feature, + environment: Environment, + with_environment_permissions: WithEnvironmentPermissionsCallable, +) -> None: + # Given + with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] + environment.minimum_change_request_approvals = 1 + environment.save() + + url = reverse( + "api-experiments:update-flag-v2", + kwargs={"environment_id": environment.id}, + ) + + data = { + "feature": {"name": feature.name}, + "environment_default": { + "enabled": True, + "value": {"type": "string", "string_value": "test"}, + }, + } + + # When + response = staff_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_403_FORBIDDEN + assert "change requests are enabled" in str(response.json()) From 03d0aa62ceef8b80851cc678a28d05b1cd4edb19 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Mon, 1 Dec 2025 10:42:01 +0530 Subject: [PATCH 10/26] cleanup --- api/features/feature_states/serializers.py | 85 +++++++------------ api/features/feature_states/views.py | 21 ++--- api/features/versioning/dataclasses.py | 29 +++++-- api/features/versioning/versioning_service.py | 8 +- .../feature_states/test_serializers.py | 54 +++--------- .../test_unit_feature_states_views.py | 64 ++++++-------- .../test_unit_versioning_dataclasses.py | 37 +++++++- 7 files changed, 137 insertions(+), 161 deletions(-) diff --git a/api/features/feature_states/serializers.py b/api/features/feature_states/serializers.py index c8aa67145878..7b11b72fb338 100644 --- a/api/features/feature_states/serializers.py +++ b/api/features/feature_states/serializers.py @@ -1,6 +1,3 @@ -import typing - -from django.db.models import Q from rest_framework import serializers from environments.models import Environment @@ -11,7 +8,6 @@ SegmentOverrideChangeSet, ) from features.versioning.versioning_service import update_flag, update_flag_v2 -from users.models import FFAdminUser class BaseFeatureUpdateSerializer(serializers.Serializer): # type: ignore[type-arg] @@ -21,38 +17,18 @@ def get_feature(self) -> Feature: if not environment: raise serializers.ValidationError("Environment context is required") - try: - # TODO: strip out the id vs name piece as this is ugly as heck. - query = Q(project_id=environment.project_id) & ( - Q(id=feature_data.get("id")) | Q(name=feature_data.get("name")) + feature: Feature = Feature.objects.get( + project_id=environment.project_id, **feature_data ) - feature: Feature = Feature.objects.get(query) return feature except Feature.DoesNotExist: - identifier = feature_data.get("id") or feature_data.get("name") raise serializers.ValidationError( - f"Feature with identifier '{identifier}' not found in project" + f"Feature '{feature_data}' not found in project" ) - def _add_audit_fields( - self, change_set: typing.Union[FlagChangeSet, FlagChangeSetV2] - ) -> None: - request = self.context["request"] - if type(request.user) is FFAdminUser: - change_set.user = request.user - else: - change_set.api_key = request.user.key - - def _parse_feature_value(self, value_data: dict) -> typing.Any: # type: ignore[type-arg] - value_serializer = FeatureValueSerializer(data=value_data) - value_serializer.is_valid() - return value_serializer.get_typed_value() - class FeatureIdentifierSerializer(serializers.Serializer): # type: ignore[type-arg] - """Accepts either name OR id (mutually exclusive).""" - name = serializers.CharField(required=False, allow_blank=False) id = serializers.IntegerField(required=False) @@ -77,19 +53,24 @@ class FeatureValueSerializer(serializers.Serializer): # type: ignore[type-arg] ) string_value = serializers.CharField(required=True, allow_blank=True) - def get_typed_value(self) -> typing.Any: - value_type = self.validated_data["type"] - string_val = self.validated_data["string_value"] + def validate(self, data: dict) -> dict: # type: ignore[type-arg] + value_type = data["type"] + string_val = data["string_value"] + + if value_type == "integer": + try: + int(string_val) + except ValueError: + raise serializers.ValidationError( + f"'{string_val}' is not a valid integer" + ) + elif value_type == "boolean": + if string_val.lower() not in ("true", "false"): + raise serializers.ValidationError( + f"'{string_val}' is not a valid boolean (use 'true' or 'false')" + ) - match value_type: - case "string": - return string_val - case "integer": - return int(string_val) - case "boolean": - return string_val.lower() == "true" - case _: - raise ValueError(f"Unsupported value type: {value_type}") + return data class UpdateFlagSerializer(BaseFeatureUpdateSerializer): @@ -106,20 +87,18 @@ def flag_change_set(self) -> FlagChangeSet: change_set = FlagChangeSet( enabled=validated_data["enabled"], - feature_state_value=self._parse_feature_value(value_data), + feature_state_value=value_data["string_value"], type_=value_data["type"], segment_id=segment_data.get("id") if segment_data else None, segment_priority=segment_data.get("priority") if segment_data else None, ) - self._add_audit_fields(change_set) + change_set.set_audit_fields_from_request(self.context["request"]) return change_set - def save(self, **kwargs: typing.Any) -> FeatureState: - environment: Environment = kwargs["environment"] - feature: Feature = self.get_feature() - - return update_flag(environment, feature, self.flag_change_set) + def save(self, **kwargs: object) -> FeatureState: + feature = self.get_feature() + return update_flag(self.context["environment"], feature, self.flag_change_set) class EnvironmentDefaultSerializer(serializers.Serializer): # type: ignore[type-arg] @@ -170,7 +149,7 @@ def change_set_v2(self) -> FlagChangeSetV2: segment_override = SegmentOverrideChangeSet( segment_id=override_data["segment_id"], enabled=override_data["enabled"], - feature_state_value=self._parse_feature_value(value_data), + feature_state_value=value_data["string_value"], type_=value_data["type"], priority=override_data.get("priority"), ) @@ -178,16 +157,14 @@ def change_set_v2(self) -> FlagChangeSetV2: change_set = FlagChangeSetV2( environment_default_enabled=env_default["enabled"], - environment_default_value=self._parse_feature_value(env_value_data), + environment_default_value=env_value_data["string_value"], environment_default_type=env_value_data["type"], segment_overrides=segment_overrides, ) - self._add_audit_fields(change_set) + change_set.set_audit_fields_from_request(self.context["request"]) return change_set - def save(self, **kwargs: typing.Any) -> dict: # type: ignore[type-arg] - environment: Environment = kwargs["environment"] - feature: Feature = self.get_feature() - - return update_flag_v2(environment, feature, self.change_set_v2) + def save(self, **kwargs: object) -> dict: # type: ignore[type-arg] + feature = self.get_feature() + return update_flag_v2(self.context["environment"], feature, self.change_set_v2) diff --git a/api/features/feature_states/views.py b/api/features/feature_states/views.py index 893cbaf1c554..bbee9de6282b 100644 --- a/api/features/feature_states/views.py +++ b/api/features/feature_states/views.py @@ -36,11 +36,10 @@ def _check_workflow_not_enabled(environment: Environment) -> None: **Value Format:** - Always use `string_value` field (value is always a string) - The `type` field tells the server how to parse it - - Available types: integer, string, boolean, float + - Available types: integer, string, boolean - Examples: - `{"type": "integer", "string_value": "42"}` - `{"type": "boolean", "string_value": "true"}` - - `{"type": "float", "string_value": "3.14"}` - `{"type": "string", "string_value": "hello"}` **Segment Priority:** @@ -69,14 +68,10 @@ def update_flag_v1(request: Request, environment_id: int) -> Response: serializer = UpdateFlagSerializer( data=request.data, - context={ - "request": request, - "view": update_flag_v1, - "environment": environment, - }, + context={"request": request, "environment": environment}, ) serializer.is_valid(raise_exception=True) - serializer.save(environment=environment) + serializer.save() return Response(status=status.HTTP_204_NO_CONTENT) @@ -103,7 +98,7 @@ def update_flag_v1(request: Request, environment_id: int) -> Response: **Value Format:** - Always use `string_value` field (value is always a string) - The `type` field tells the server how to parse it - - Available types: integer, string, boolean, float + - Available types: integer, string, boolean - Examples: - `{"type": "string", "string_value": "production"}` - `{"type": "integer", "string_value": "100"}` @@ -138,13 +133,9 @@ def update_flag_v2(request: Request, environment_id: int) -> Response: serializer = UpdateFlagV2Serializer( data=request.data, - context={ - "request": request, - "view": update_flag_v2, - "environment": environment, - }, + context={"request": request, "environment": environment}, ) serializer.is_valid(raise_exception=True) - serializer.save(environment=environment) + serializer.save() return Response(status=status.HTTP_204_NO_CONTENT) diff --git a/api/features/versioning/dataclasses.py b/api/features/versioning/dataclasses.py index ba4485dc10f4..48719bed1f41 100644 --- a/api/features/versioning/dataclasses.py +++ b/api/features/versioning/dataclasses.py @@ -1,11 +1,14 @@ -import typing +from __future__ import annotations + from dataclasses import dataclass, field from datetime import datetime +from typing import TYPE_CHECKING from pydantic import BaseModel, computed_field -if typing.TYPE_CHECKING: - from api_keys.models import MasterAPIKey +if TYPE_CHECKING: + from rest_framework.request import Request + from users.models import FFAdminUser @@ -22,14 +25,24 @@ def is_environment_default(self) -> bool: @dataclass class AuditFieldsMixin: - user: typing.Optional["FFAdminUser"] = field(default=None, kw_only=True) - api_key: typing.Optional["MasterAPIKey"] = field(default=None, kw_only=True) + user: FFAdminUser | None = field(default=None, kw_only=True) + api_key: str | None = field(default=None, kw_only=True) + + def set_audit_fields_from_request(self, request: Request) -> None: + from users.models import FFAdminUser + + if type(request.user) is FFAdminUser: + self.user = request.user + elif hasattr(request.user, "key"): + self.api_key = request.user.key + else: + raise ValueError("Request user must be FFAdminUser or have an API key") @dataclass class FlagChangeSet(AuditFieldsMixin): enabled: bool - feature_state_value: typing.Any + feature_state_value: str type_: str segment_id: int | None = None @@ -40,7 +53,7 @@ class FlagChangeSet(AuditFieldsMixin): class SegmentOverrideChangeSet: segment_id: int enabled: bool - feature_state_value: typing.Any + feature_state_value: str type_: str priority: int | None = None @@ -48,7 +61,7 @@ class SegmentOverrideChangeSet: @dataclass class FlagChangeSetV2(AuditFieldsMixin): environment_default_enabled: bool - environment_default_value: typing.Any + environment_default_value: str environment_default_type: str segment_overrides: list[SegmentOverrideChangeSet] = field(default_factory=list) diff --git a/api/features/versioning/versioning_service.py b/api/features/versioning/versioning_service.py index 02f12bf7de01..5a9e7fdec979 100644 --- a/api/features/versioning/versioning_service.py +++ b/api/features/versioning/versioning_service.py @@ -239,18 +239,16 @@ def _update_flag_for_versioning_v1( return target_feature_state -def _update_feature_state_value( - fsv: FeatureStateValue, value: typing.Any, type_: str -) -> None: +def _update_feature_state_value(fsv: FeatureStateValue, value: str, type_: str) -> None: match type_: case "string": - fsv.string_value = str(value) + fsv.string_value = value fsv.type = STRING case "integer": fsv.integer_value = int(value) fsv.type = INTEGER case "boolean": - fsv.boolean_value = value + fsv.boolean_value = value.lower() == "true" fsv.type = BOOLEAN fsv.save() diff --git a/api/tests/unit/features/feature_states/test_serializers.py b/api/tests/unit/features/feature_states/test_serializers.py index bc48f9fa77f8..5e9ab4f41160 100644 --- a/api/tests/unit/features/feature_states/test_serializers.py +++ b/api/tests/unit/features/feature_states/test_serializers.py @@ -1,15 +1,11 @@ -from unittest.mock import Mock - import pytest -from environments.models import Environment from features.feature_states.serializers import ( FeatureValueSerializer, UpdateFlagSerializer, UpdateFlagV2Serializer, ) from features.models import Feature -from features.versioning.dataclasses import FlagChangeSet def test_get_feature_raises_error_when_environment_not_in_context( @@ -31,37 +27,6 @@ def test_get_feature_raises_error_when_environment_not_in_context( assert "Environment context is required" in str(exc_info.value) -def test_add_audit_fields_sets_api_key_for_non_user( - environment: Environment, - feature: Feature, -) -> None: - # Create a mock API key user (not FFAdminUser) - mock_api_key = Mock() - mock_api_key.key = "test_api_key" - mock_request = Mock() - mock_request.user = mock_api_key - - serializer = UpdateFlagSerializer( - data={ - "feature": {"name": feature.name}, - "enabled": True, - "value": {"type": "string", "string_value": "test"}, - }, - context={"environment": environment, "request": mock_request}, - ) - serializer.is_valid() - - change_set = FlagChangeSet( - enabled=True, - feature_state_value="test", - type_="string", - ) - serializer._add_audit_fields(change_set) - - assert change_set.api_key == "test_api_key" - assert change_set.user is None - - def test_validate_segment_overrides_returns_empty_list() -> None: serializer = UpdateFlagV2Serializer() result = serializer.validate_segment_overrides([]) @@ -69,12 +34,17 @@ def test_validate_segment_overrides_returns_empty_list() -> None: assert result == [] -def test_get_typed_value_raises_error_for_unsupported_type() -> None: - serializer = FeatureValueSerializer() - # Bypass validation by setting validated_data directly - serializer._validated_data = {"type": "unsupported", "string_value": "test"} # type: ignore[attr-defined] +def test_feature_value_serializer_rejects_invalid_integer() -> None: + serializer = FeatureValueSerializer( + data={"type": "integer", "string_value": "not_a_number"} + ) + + assert serializer.is_valid() is False + assert "not a valid integer" in str(serializer.errors) + - with pytest.raises(ValueError) as exc_info: - serializer.get_typed_value() +def test_feature_value_serializer_rejects_invalid_boolean() -> None: + serializer = FeatureValueSerializer(data={"type": "boolean", "string_value": "yes"}) - assert "Unsupported value type: unsupported" in str(exc_info.value) + assert serializer.is_valid() is False + assert "not a valid boolean" in str(serializer.errors) diff --git a/api/tests/unit/features/feature_states/test_unit_feature_states_views.py b/api/tests/unit/features/feature_states/test_unit_feature_states_views.py index 72d7c65058c2..4371c211dd6a 100644 --- a/api/tests/unit/features/feature_states/test_unit_feature_states_views.py +++ b/api/tests/unit/features/feature_states/test_unit_feature_states_views.py @@ -9,7 +9,7 @@ from rest_framework.test import APIClient from environments.models import Environment -from features.models import Feature, FeatureSegment +from features.models import Feature, FeatureSegment, FeatureState from features.versioning.versioning_service import ( get_environment_flags_list, ) @@ -248,9 +248,7 @@ def test_update_flag_error_when_feature_not_found_by_name( # Then assert response.status_code == status.HTTP_400_BAD_REQUEST - assert "Feature with identifier 'non_existent_feature' not found" in str( - response.json() - ) + assert "not found in project" in str(response.json()) @pytest.mark.parametrize( @@ -282,10 +280,7 @@ def test_update_flag_error_when_feature_not_found_by_id( # Then assert response.status_code == status.HTTP_400_BAD_REQUEST - assert "Feature with identifier '999999' not found" in str(response.json()) - - -# V1 Segment Override Tests + assert "not found in project" in str(response.json()) @pytest.mark.parametrize( @@ -598,23 +593,25 @@ def test_update_existing_segment_override_with_priority_v1( segment = Segment.objects.create(name="priority_segment", project=project) + # Create the segment override manually + feature_segment = FeatureSegment.objects.create( + feature=feature, + segment=segment, + environment=environment_, + priority=5, + ) + FeatureState.objects.create( + feature=feature, + environment=environment_, + feature_segment=feature_segment, + enabled=True, + ) + url = reverse( "api-experiments:update-flag-v1", kwargs={"environment_id": environment_.id}, ) - # First, create the segment override - create_data = { - "feature": {"name": feature.name}, - "segment": {"id": segment.id, "priority": 5}, - "enabled": True, - "value": {"type": "integer", "string_value": "100"}, - } - response = staff_client.post( - url, data=json.dumps(create_data), content_type="application/json" - ) - assert response.status_code == status.HTTP_204_NO_CONTENT - # When - Update the same segment override with new priority update_data = { "feature": {"name": feature.name}, @@ -658,21 +655,19 @@ def test_update_existing_segment_override_with_priority_v2( segment = Segment.objects.create(name="priority_segment_v2", project=project) - # First, create the segment override using V1 endpoint - v1_url = reverse( - "api-experiments:update-flag-v1", - kwargs={"environment_id": environment_.id}, + # Create the segment override manually + feature_segment = FeatureSegment.objects.create( + feature=feature, + segment=segment, + environment=environment_, + priority=5, ) - create_data = { - "feature": {"name": feature.name}, - "segment": {"id": segment.id, "priority": 5}, - "enabled": True, - "value": {"type": "string", "string_value": "initial"}, - } - response = staff_client.post( - v1_url, data=json.dumps(create_data), content_type="application/json" + FeatureState.objects.create( + feature=feature, + environment=environment_, + feature_segment=feature_segment, + enabled=True, ) - assert response.status_code == status.HTTP_204_NO_CONTENT # When - Update the existing segment override using V2 endpoint v2_url = reverse( @@ -713,9 +708,6 @@ def test_update_existing_segment_override_with_priority_v2( assert feature_segment.priority == 2 -# Workflow enabled tests - - def test_update_flag_v1_returns_403_when_workflow_enabled( staff_client: APIClient, feature: Feature, diff --git a/api/tests/unit/features/versioning/test_unit_versioning_dataclasses.py b/api/tests/unit/features/versioning/test_unit_versioning_dataclasses.py index f7c89fd88efc..ab689f9d4f3c 100644 --- a/api/tests/unit/features/versioning/test_unit_versioning_dataclasses.py +++ b/api/tests/unit/features/versioning/test_unit_versioning_dataclasses.py @@ -1,6 +1,8 @@ +from unittest.mock import Mock + import pytest -from features.versioning.dataclasses import Conflict +from features.versioning.dataclasses import Conflict, FlagChangeSet @pytest.mark.parametrize("segment_id, expected_result", ((None, True), (1, False))) @@ -8,3 +10,36 @@ def test_conflict_is_environment_default( segment_id: int | None, expected_result: bool ) -> None: assert Conflict(segment_id=segment_id).is_environment_default is expected_result + + +def test_set_audit_fields_from_request_sets_api_key_for_non_user() -> None: + mock_api_key = Mock() + mock_api_key.key = "test_api_key" + mock_request = Mock() + mock_request.user = mock_api_key + + change_set = FlagChangeSet( + enabled=True, + feature_state_value="test", + type_="string", + ) + change_set.set_audit_fields_from_request(mock_request) + + assert change_set.api_key == "test_api_key" + assert change_set.user is None + + +def test_set_audit_fields_from_request_raises_for_invalid_user() -> None: + mock_request = Mock() + mock_request.user = object() # No .key attribute + + change_set = FlagChangeSet( + enabled=True, + feature_state_value="test", + type_="string", + ) + + with pytest.raises(ValueError) as exc_info: + change_set.set_audit_fields_from_request(mock_request) + + assert "must be FFAdminUser or have an API key" in str(exc_info.value) From cf91675a31117543bc0518078f28dff290341d45 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Mon, 1 Dec 2025 13:06:51 +0530 Subject: [PATCH 11/26] fixup! cleanup --- api/features/versioning/versioning_service.py | 20 ++--- .../test_unit_feature_states_views.py | 75 +++++++++++++++++++ 2 files changed, 82 insertions(+), 13 deletions(-) diff --git a/api/features/versioning/versioning_service.py b/api/features/versioning/versioning_service.py index 5a9e7fdec979..8609666af34d 100644 --- a/api/features/versioning/versioning_service.py +++ b/api/features/versioning/versioning_service.py @@ -119,7 +119,6 @@ def _update_flag_for_versioning_v2( environment: Environment, feature: Feature, change_set: FlagChangeSet ) -> FeatureState: from features.models import FeatureSegment, FeatureState - from segments.models import Segment new_version = EnvironmentFeatureVersion.objects.create( environment=environment, @@ -135,11 +134,9 @@ def _update_flag_for_versioning_v2( feature_segment__segment_id=change_set.segment_id, ) except FeatureState.DoesNotExist: - segment = Segment.objects.get(id=change_set.segment_id) - feature_segment = FeatureSegment.objects.create( feature=feature, - segment=segment, + segment_id=change_set.segment_id, environment=environment, priority=change_set.segment_priority if change_set.segment_priority is not None @@ -157,6 +154,7 @@ def _update_flag_for_versioning_v2( # Environment default - always exists target_feature_state = new_version.feature_states.get( feature_segment__isnull=True, + identity_id=None, ) target_feature_state.enabled = change_set.enabled @@ -184,7 +182,6 @@ def _update_flag_for_versioning_v1( environment: Environment, feature: Feature, change_set: FlagChangeSet ) -> FeatureState: from features.models import FeatureSegment, FeatureState - from segments.models import Segment latest_feature_states = get_environment_flags_dict( environment=environment, @@ -193,11 +190,9 @@ def _update_flag_for_versioning_v1( ) if len(latest_feature_states) == 0 and change_set.segment_id: - segment = Segment.objects.get(id=change_set.segment_id) - feature_segment = FeatureSegment.objects.create( feature=feature, - segment=segment, + segment_id=change_set.segment_id, environment=environment, priority=change_set.segment_priority if change_set.segment_priority is not None @@ -263,13 +258,10 @@ def _create_segment_override( version: EnvironmentFeatureVersion | None = None, ) -> FeatureState: from features.models import FeatureSegment - from segments.models import Segment - - segment = Segment.objects.get(id=segment_id) feature_segment = FeatureSegment.objects.create( feature=feature, - segment=segment, + segment_id=segment_id, environment=environment, priority=priority if priority is not None else 0, ) @@ -310,7 +302,9 @@ def _update_flag_v2_for_versioning_v2( created_by_api_key=change_set.api_key, ) - env_default_state = new_version.feature_states.get(feature_segment__isnull=True) + env_default_state = new_version.feature_states.get( + feature_segment__isnull=True, identity_id=None + ) env_default_state.enabled = change_set.environment_default_enabled env_default_state.save() diff --git a/api/tests/unit/features/feature_states/test_unit_feature_states_views.py b/api/tests/unit/features/feature_states/test_unit_feature_states_views.py index 4371c211dd6a..16cf4bcf7bff 100644 --- a/api/tests/unit/features/feature_states/test_unit_feature_states_views.py +++ b/api/tests/unit/features/feature_states/test_unit_feature_states_views.py @@ -11,6 +11,7 @@ from environments.models import Environment from features.models import Feature, FeatureSegment, FeatureState from features.versioning.versioning_service import ( + get_current_live_environment_feature_version, get_environment_flags_list, ) from projects.models import Project @@ -772,3 +773,77 @@ def test_update_flag_v2_returns_403_when_workflow_enabled( # Then assert response.status_code == status.HTTP_403_FORBIDDEN assert "change requests are enabled" in str(response.json()) + + +def test_update_existing_segment_override_v2_versioning( + staff_client: APIClient, + feature: Feature, + environment_v2_versioning: Environment, + project: Project, + with_environment_permissions: WithEnvironmentPermissionsCallable, +) -> None: + # Given + with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] + + segment = Segment.objects.create(name="existing_segment", project=project) + + # Get the current live version and create segment override associated with it + current_version = get_current_live_environment_feature_version( + environment_v2_versioning.id, feature.id + ) + feature_segment = FeatureSegment.objects.create( + feature=feature, + segment=segment, + environment=environment_v2_versioning, + priority=5, + ) + FeatureState.objects.create( + feature=feature, + environment=environment_v2_versioning, + feature_segment=feature_segment, + environment_feature_version=current_version, + enabled=True, + ) + + url = reverse( + "api-experiments:update-flag-v2", + kwargs={"environment_id": environment_v2_versioning.id}, + ) + + # When - Update the existing segment override + data = { + "feature": {"name": feature.name}, + "environment_default": { + "enabled": True, + "value": {"type": "string", "string_value": "default"}, + }, + "segment_overrides": [ + { + "segment_id": segment.id, + "priority": 1, # Changed priority from 5 to 1 + "enabled": False, # Changed enabled from True to False + "value": {"type": "integer", "string_value": "999"}, + }, + ], + } + response = staff_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_204_NO_CONTENT + + # Verify the segment override was updated + latest_flags = get_environment_flags_list( + environment=environment_v2_versioning, feature_name=feature.name + ) + segment_override = [ + fs + for fs in latest_flags + if fs.feature_segment and fs.feature_segment.segment_id == segment.id + ][0] + + assert segment_override.enabled is False + assert segment_override.get_feature_state_value() == 999 + assert segment_override.feature_segment is not None + assert segment_override.feature_segment.priority == 1 From 6ecba23d8d859ed62d32188ee5e208e4507521d0 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Mon, 1 Dec 2025 13:20:25 +0530 Subject: [PATCH 12/26] feat: add UPDATE_FEATURE_STATE permission check to experimental endpoints Add EnvironmentUpdateFeatureStatePermission class and apply it to update_flag_v1 and update_flag_v2 views using @permission_classes decorator. Returns 403 if user lacks UPDATE_FEATURE_STATE permission. --- api/features/feature_states/permissions.py | 19 +++++ api/features/feature_states/views.py | 19 ++--- .../test_unit_feature_states_views.py | 78 +++++++++++++++++++ 3 files changed, 102 insertions(+), 14 deletions(-) create mode 100644 api/features/feature_states/permissions.py diff --git a/api/features/feature_states/permissions.py b/api/features/feature_states/permissions.py new file mode 100644 index 000000000000..c0a5366eebc1 --- /dev/null +++ b/api/features/feature_states/permissions.py @@ -0,0 +1,19 @@ +from common.environments.permissions import UPDATE_FEATURE_STATE +from rest_framework.permissions import BasePermission +from rest_framework.request import Request +from rest_framework.views import APIView + +from environments.models import Environment + + +class EnvironmentUpdateFeatureStatePermission(BasePermission): + def has_permission(self, request: Request, view: APIView) -> bool: + environment_id = view.kwargs.get("environment_id") + try: + environment = Environment.objects.get(id=environment_id) + except Environment.DoesNotExist: + return False + + return request.user.has_environment_permission( # type: ignore[union-attr] + UPDATE_FEATURE_STATE, environment + ) diff --git a/api/features/feature_states/views.py b/api/features/feature_states/views.py index bbee9de6282b..56db34fd5fc8 100644 --- a/api/features/feature_states/views.py +++ b/api/features/feature_states/views.py @@ -1,13 +1,15 @@ from drf_yasg import openapi # type: ignore[import-untyped] from drf_yasg.utils import swagger_auto_schema # type: ignore[import-untyped] from rest_framework import status -from rest_framework.decorators import api_view +from rest_framework.decorators import api_view, permission_classes from rest_framework.exceptions import PermissionDenied +from rest_framework.permissions import IsAuthenticated from rest_framework.request import Request from rest_framework.response import Response from environments.models import Environment +from .permissions import EnvironmentUpdateFeatureStatePermission from .serializers import UpdateFlagSerializer, UpdateFlagV2Serializer @@ -56,13 +58,8 @@ def _check_workflow_not_enabled(environment: Environment) -> None: tags=["Experimental - Feature States"], ) # type: ignore[misc] @api_view(http_method_names=["POST"]) +@permission_classes([IsAuthenticated, EnvironmentUpdateFeatureStatePermission]) def update_flag_v1(request: Request, environment_id: int) -> Response: - """ - V1: Single feature state update endpoint (issue #6279). - - Updates a single feature state (environment default OR one segment override). - Feature identification is provided in the request payload. - """ environment = Environment.objects.get(id=environment_id) _check_workflow_not_enabled(environment) @@ -120,14 +117,8 @@ def update_flag_v1(request: Request, environment_id: int) -> Response: tags=["Experimental - Feature States"], ) # type: ignore[misc] @api_view(http_method_names=["POST"]) +@permission_classes([IsAuthenticated, EnvironmentUpdateFeatureStatePermission]) def update_flag_v2(request: Request, environment_id: int) -> Response: - """ - Update multiple feature states endpoint (issue #6233). - - Updates multiple feature states in a single request: - - Environment default state - - Multiple segment overrides with priority ordering - """ environment = Environment.objects.get(id=environment_id) _check_workflow_not_enabled(environment) diff --git a/api/tests/unit/features/feature_states/test_unit_feature_states_views.py b/api/tests/unit/features/feature_states/test_unit_feature_states_views.py index 16cf4bcf7bff..1d7015b5dd94 100644 --- a/api/tests/unit/features/feature_states/test_unit_feature_states_views.py +++ b/api/tests/unit/features/feature_states/test_unit_feature_states_views.py @@ -847,3 +847,81 @@ def test_update_existing_segment_override_v2_versioning( assert segment_override.get_feature_state_value() == 999 assert segment_override.feature_segment is not None assert segment_override.feature_segment.priority == 1 + + +def test_update_flag_v1_returns_403_without_permission( + staff_client: APIClient, + feature: Feature, + environment: Environment, +) -> None: + # Given - no permissions granted + url = reverse( + "api-experiments:update-flag-v1", + kwargs={"environment_id": environment.id}, + ) + + data = { + "feature": {"name": feature.name}, + "enabled": True, + "value": {"type": "string", "string_value": "test"}, + } + + # When + response = staff_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_403_FORBIDDEN + + +def test_update_flag_v2_returns_403_without_permission( + staff_client: APIClient, + feature: Feature, + environment: Environment, +) -> None: + # Given - no permissions granted + url = reverse( + "api-experiments:update-flag-v2", + kwargs={"environment_id": environment.id}, + ) + + data = { + "feature": {"name": feature.name}, + "environment_default": { + "enabled": True, + "value": {"type": "string", "string_value": "test"}, + }, + } + + # When + response = staff_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_403_FORBIDDEN + + +def test_update_flag_v1_returns_403_for_nonexistent_environment( + staff_client: APIClient, +) -> None: + # Given + url = reverse( + "api-experiments:update-flag-v1", + kwargs={"environment_id": 999999}, + ) + + data = { + "feature": {"name": "any_feature"}, + "enabled": True, + "value": {"type": "string", "string_value": "test"}, + } + + # When + response = staff_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_403_FORBIDDEN From 8a34a26e74d6b2a3af522ac964bb07dde5342a50 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Wed, 3 Dec 2025 15:16:44 +0530 Subject: [PATCH 13/26] refactor: use environment_key instead of environment_id in experimental endpoints Change URL parameter from int:environment_id to str:environment_key for consistency with other Flagsmith endpoints that use API keys for environment identification. --- api/api/urls/experiments.py | 4 +- api/features/feature_states/permissions.py | 4 +- api/features/feature_states/views.py | 26 ++++++++++-- .../test_unit_feature_states_views.py | 40 +++++++++---------- 4 files changed, 46 insertions(+), 28 deletions(-) diff --git a/api/api/urls/experiments.py b/api/api/urls/experiments.py index 8675d682af68..2fbb500ee4f0 100644 --- a/api/api/urls/experiments.py +++ b/api/api/urls/experiments.py @@ -13,12 +13,12 @@ urlpatterns = [ path( - "environments//update-flag-v1/", + "environments//update-flag-v1/", update_flag_v1, name="update-flag-v1", ), path( - "environments//update-flag-v2/", + "environments//update-flag-v2/", update_flag_v2, name="update-flag-v2", ), diff --git a/api/features/feature_states/permissions.py b/api/features/feature_states/permissions.py index c0a5366eebc1..d7bf46e80d9f 100644 --- a/api/features/feature_states/permissions.py +++ b/api/features/feature_states/permissions.py @@ -8,9 +8,9 @@ class EnvironmentUpdateFeatureStatePermission(BasePermission): def has_permission(self, request: Request, view: APIView) -> bool: - environment_id = view.kwargs.get("environment_id") + environment_key = view.kwargs.get("environment_key") try: - environment = Environment.objects.get(id=environment_id) + environment = Environment.objects.get(api_key=environment_key) except Environment.DoesNotExist: return False diff --git a/api/features/feature_states/views.py b/api/features/feature_states/views.py index 56db34fd5fc8..80d5610b9d7f 100644 --- a/api/features/feature_states/views.py +++ b/api/features/feature_states/views.py @@ -49,6 +49,15 @@ def _check_workflow_not_enabled(environment: Environment) -> None: - If null/omitted, segment is appended to end - Use specific priority value to reorder """, + manual_parameters=[ + openapi.Parameter( + "environment_key", + openapi.IN_PATH, + description="The environment API key", + type=openapi.TYPE_STRING, + required=True, + ) + ], request_body=UpdateFlagSerializer, responses={ 204: openapi.Response( @@ -59,8 +68,8 @@ def _check_workflow_not_enabled(environment: Environment) -> None: ) # type: ignore[misc] @api_view(http_method_names=["POST"]) @permission_classes([IsAuthenticated, EnvironmentUpdateFeatureStatePermission]) -def update_flag_v1(request: Request, environment_id: int) -> Response: - environment = Environment.objects.get(id=environment_id) +def update_flag_v1(request: Request, environment_key: str) -> Response: + environment = Environment.objects.get(api_key=environment_key) _check_workflow_not_enabled(environment) serializer = UpdateFlagSerializer( @@ -108,6 +117,15 @@ def update_flag_v1(request: Request, environment_id: int) -> Response: - Duplicate segment_id values are not allowed """, + manual_parameters=[ + openapi.Parameter( + "environment_key", + openapi.IN_PATH, + description="The environment API key", + type=openapi.TYPE_STRING, + required=True, + ) + ], request_body=UpdateFlagV2Serializer, responses={ 204: openapi.Response( @@ -118,8 +136,8 @@ def update_flag_v1(request: Request, environment_id: int) -> Response: ) # type: ignore[misc] @api_view(http_method_names=["POST"]) @permission_classes([IsAuthenticated, EnvironmentUpdateFeatureStatePermission]) -def update_flag_v2(request: Request, environment_id: int) -> Response: - environment = Environment.objects.get(id=environment_id) +def update_flag_v2(request: Request, environment_key: str) -> Response: + environment = Environment.objects.get(api_key=environment_key) _check_workflow_not_enabled(environment) serializer = UpdateFlagV2Serializer( diff --git a/api/tests/unit/features/feature_states/test_unit_feature_states_views.py b/api/tests/unit/features/feature_states/test_unit_feature_states_views.py index 1d7015b5dd94..9fc38227a02e 100644 --- a/api/tests/unit/features/feature_states/test_unit_feature_states_views.py +++ b/api/tests/unit/features/feature_states/test_unit_feature_states_views.py @@ -44,7 +44,7 @@ def test_update_flag_by_name( with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] url = reverse( "api-experiments:update-flag-v1", - kwargs={"environment_id": environment_.id}, + kwargs={"environment_key": environment_.api_key}, ) data = { @@ -83,7 +83,7 @@ def test_update_flag_by_id( with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] url = reverse( "api-experiments:update-flag-v1", - kwargs={"environment_id": environment_.id}, + kwargs={"environment_key": environment_.api_key}, ) data = { @@ -122,7 +122,7 @@ def test_update_flag_error_when_both_name_and_id_provided( with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] url = reverse( "api-experiments:update-flag-v1", - kwargs={"environment_id": environment_.id}, + kwargs={"environment_key": environment_.api_key}, ) # Providing both name and ID (even if they match the same feature) @@ -164,7 +164,7 @@ def test_update_flag_error_when_both_name_and_id_provided_for_different_features url = reverse( "api-experiments:update-flag-v1", - kwargs={"environment_id": environment_.id}, + kwargs={"environment_key": environment_.api_key}, ) # Providing both name and ID for DIFFERENT features (worst case) @@ -199,7 +199,7 @@ def test_update_flag_error_when_neither_name_nor_id_provided( with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] url = reverse( "api-experiments:update-flag-v1", - kwargs={"environment_id": environment_.id}, + kwargs={"environment_key": environment_.api_key}, ) # Providing empty feature object @@ -233,7 +233,7 @@ def test_update_flag_error_when_feature_not_found_by_name( with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] url = reverse( "api-experiments:update-flag-v1", - kwargs={"environment_id": environment_.id}, + kwargs={"environment_key": environment_.api_key}, ) data = { @@ -265,7 +265,7 @@ def test_update_flag_error_when_feature_not_found_by_id( with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] url = reverse( "api-experiments:update-flag-v1", - kwargs={"environment_id": environment_.id}, + kwargs={"environment_key": environment_.api_key}, ) data = { @@ -304,7 +304,7 @@ def test_update_flag_segment_override_by_name( url = reverse( "api-experiments:update-flag-v1", - kwargs={"environment_id": environment_.id}, + kwargs={"environment_key": environment_.api_key}, ) data = { @@ -362,7 +362,7 @@ def test_update_flag_segment_override_creates_feature_segment_if_not_exists( url = reverse( "api-experiments:update-flag-v1", - kwargs={"environment_id": environment_.id}, + kwargs={"environment_key": environment_.api_key}, ) data = { @@ -428,7 +428,7 @@ def test_update_feature_states_creates_new_segment_overrides( url = reverse( "api-experiments:update-flag-v2", - kwargs={"environment_id": environment_.id}, + kwargs={"environment_key": environment_.api_key}, ) # Batch update with environment default + 2 new segment overrides @@ -498,7 +498,7 @@ def test_update_feature_states_environment_default_only( with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] url = reverse( "api-experiments:update-flag-v2", - kwargs={"environment_id": environment_.id}, + kwargs={"environment_key": environment_.api_key}, ) # Test with just environment default (no segment overrides) @@ -543,7 +543,7 @@ def test_update_feature_states_rejects_duplicate_segment_ids( url = reverse( "api-experiments:update-flag-v2", - kwargs={"environment_id": environment.id}, + kwargs={"environment_key": environment.api_key}, ) # Request with duplicate segment_id @@ -610,7 +610,7 @@ def test_update_existing_segment_override_with_priority_v1( url = reverse( "api-experiments:update-flag-v1", - kwargs={"environment_id": environment_.id}, + kwargs={"environment_key": environment_.api_key}, ) # When - Update the same segment override with new priority @@ -673,7 +673,7 @@ def test_update_existing_segment_override_with_priority_v2( # When - Update the existing segment override using V2 endpoint v2_url = reverse( "api-experiments:update-flag-v2", - kwargs={"environment_id": environment_.id}, + kwargs={"environment_key": environment_.api_key}, ) update_data = { "feature": {"name": feature.name}, @@ -722,7 +722,7 @@ def test_update_flag_v1_returns_403_when_workflow_enabled( url = reverse( "api-experiments:update-flag-v1", - kwargs={"environment_id": environment.id}, + kwargs={"environment_key": environment.api_key}, ) data = { @@ -754,7 +754,7 @@ def test_update_flag_v2_returns_403_when_workflow_enabled( url = reverse( "api-experiments:update-flag-v2", - kwargs={"environment_id": environment.id}, + kwargs={"environment_key": environment.api_key}, ) data = { @@ -807,7 +807,7 @@ def test_update_existing_segment_override_v2_versioning( url = reverse( "api-experiments:update-flag-v2", - kwargs={"environment_id": environment_v2_versioning.id}, + kwargs={"environment_key": environment_v2_versioning.api_key}, ) # When - Update the existing segment override @@ -857,7 +857,7 @@ def test_update_flag_v1_returns_403_without_permission( # Given - no permissions granted url = reverse( "api-experiments:update-flag-v1", - kwargs={"environment_id": environment.id}, + kwargs={"environment_key": environment.api_key}, ) data = { @@ -883,7 +883,7 @@ def test_update_flag_v2_returns_403_without_permission( # Given - no permissions granted url = reverse( "api-experiments:update-flag-v2", - kwargs={"environment_id": environment.id}, + kwargs={"environment_key": environment.api_key}, ) data = { @@ -909,7 +909,7 @@ def test_update_flag_v1_returns_403_for_nonexistent_environment( # Given url = reverse( "api-experiments:update-flag-v1", - kwargs={"environment_id": 999999}, + kwargs={"environment_key": "nonexistent_key"}, ) data = { From 56276249b268d68f776be55e4c437f944b9e91e8 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Wed, 3 Dec 2025 16:36:17 +0530 Subject: [PATCH 14/26] fix: use explicit isnull filter for environment defaults in V1 versioning The previous filter Q(feature_segment__segment_id=None) relies on LEFT OUTER JOIN behavior to match NULL FKs. Using Q(feature_segment__isnull=True) is more explicit and robust as it directly checks the FK column. --- api/features/versioning/versioning_service.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/api/features/versioning/versioning_service.py b/api/features/versioning/versioning_service.py index 8609666af34d..f354ef3a10da 100644 --- a/api/features/versioning/versioning_service.py +++ b/api/features/versioning/versioning_service.py @@ -183,10 +183,15 @@ def _update_flag_for_versioning_v1( ) -> FeatureState: from features.models import FeatureSegment, FeatureState + if change_set.segment_id: + additional_filters = Q(feature_segment__segment_id=change_set.segment_id) + else: + additional_filters = Q(feature_segment__isnull=True) + latest_feature_states = get_environment_flags_dict( environment=environment, feature_name=feature.name, - additional_filters=Q(feature_segment__segment_id=change_set.segment_id), + additional_filters=additional_filters, ) if len(latest_feature_states) == 0 and change_set.segment_id: From 6a3fa629c27680764b62fa8710f238301994eca2 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Thu, 4 Dec 2025 12:17:03 +0530 Subject: [PATCH 15/26] refactor: add set_value method to clear stale fields The _update_feature_state_value function was setting new typed values without clearing old value fields when changing types. This left stale data in the database. Added set_value() method to AbstractBaseFeatureValueModel that centralizes validation, type conversion, and ensures unused fields are cleared. --- api/features/feature_states/models.py | 33 ++++++ api/features/versioning/versioning_service.py | 13 +-- .../features/feature_states/test_models.py | 107 ++++++++++++++++++ .../unit/features/test_unit_features_views.py | 42 +++---- 4 files changed, 154 insertions(+), 41 deletions(-) create mode 100644 api/tests/unit/features/feature_states/test_models.py diff --git a/api/features/feature_states/models.py b/api/features/feature_states/models.py index 9cbf1bb65e5e..3f06d592333f 100644 --- a/api/features/feature_states/models.py +++ b/api/features/feature_states/models.py @@ -38,3 +38,36 @@ def value(self) -> typing.Union[str, int, bool]: self.type, # type: ignore[arg-type] self.string_value, ) + + def set_value(self, value: str, type_: str) -> None: + typed_value: str | int | bool + match type_: + case "string": + typed_value = value + field = "string_value" + type_const = STRING + case "integer": + try: + typed_value = int(value) + except ValueError: + raise ValueError(f"'{value}' is not a valid integer") + field = "integer_value" + type_const = INTEGER + case "boolean": + if value.lower() not in ("true", "false"): + raise ValueError( + f"'{value}' is not a valid boolean (use 'true' or 'false')" + ) + typed_value = value.lower() == "true" + field = "boolean_value" + type_const = BOOLEAN + case _: + raise ValueError( + f"'{type_}' is not a valid type (use 'string', 'integer', or 'boolean')" + ) + + self.string_value = None + self.integer_value = None + self.boolean_value = None + setattr(self, field, typed_value) + self.type = type_const diff --git a/api/features/versioning/versioning_service.py b/api/features/versioning/versioning_service.py index f354ef3a10da..58dea7d68d59 100644 --- a/api/features/versioning/versioning_service.py +++ b/api/features/versioning/versioning_service.py @@ -4,7 +4,6 @@ from django.db.models import Prefetch, Q, QuerySet from django.utils import timezone -from core.constants import BOOLEAN, INTEGER, STRING from environments.models import Environment from features.models import Feature, FeatureState, FeatureStateValue from features.versioning.dataclasses import ( @@ -240,17 +239,7 @@ def _update_flag_for_versioning_v1( def _update_feature_state_value(fsv: FeatureStateValue, value: str, type_: str) -> None: - match type_: - case "string": - fsv.string_value = value - fsv.type = STRING - case "integer": - fsv.integer_value = int(value) - fsv.type = INTEGER - case "boolean": - fsv.boolean_value = value.lower() == "true" - fsv.type = BOOLEAN - + fsv.set_value(value, type_) fsv.save() diff --git a/api/tests/unit/features/feature_states/test_models.py b/api/tests/unit/features/feature_states/test_models.py new file mode 100644 index 000000000000..528242b8acd6 --- /dev/null +++ b/api/tests/unit/features/feature_states/test_models.py @@ -0,0 +1,107 @@ +import pytest + +from features.models import FeatureState +from features.value_types import BOOLEAN, INTEGER, STRING + + +@pytest.mark.parametrize( + "value,type_,expected_value,expected_type", + [ + ("hello", "string", "hello", STRING), + ("42", "integer", 42, INTEGER), + ("true", "boolean", True, BOOLEAN), + ("false", "boolean", False, BOOLEAN), + ("TRUE", "boolean", True, BOOLEAN), + ], +) +def test_set_value( + feature_state: FeatureState, + value: str, + type_: str, + expected_value: str | int | bool, + expected_type: str, +) -> None: + # Given + fsv = feature_state.feature_state_value + + # When + fsv.set_value(value, type_) + + # Then + assert fsv.value == expected_value + assert fsv.type == expected_type + + +@pytest.mark.parametrize( + "value,type_,expected_error", + [ + ("not_a_number", "integer", "'not_a_number' is not a valid integer"), + ("yes", "boolean", "'yes' is not a valid boolean"), + ("hello", "invalid", "'invalid' is not a valid type"), + ], +) +def test_set_value_invalid_raises_error( + feature_state: FeatureState, + value: str, + type_: str, + expected_error: str, +) -> None: + # Given + fsv = feature_state.feature_state_value + + # When / Then + with pytest.raises(ValueError) as exc_info: + fsv.set_value(value, type_) + + assert expected_error in str(exc_info.value) + + +def test_set_value_clears_old_fields_when_changing_type( + feature_state: FeatureState, +) -> None: + # Given + fsv = feature_state.feature_state_value + fsv.set_value("42", "integer") + fsv.save() + + assert fsv.integer_value == 42 + + # When - change from integer to string + fsv.set_value("hello", "string") + + # Then - integer_value should be cleared + assert fsv.string_value == "hello" + assert fsv.integer_value is None + assert fsv.boolean_value is None + assert fsv.type == STRING + + +@pytest.mark.parametrize( + "invalid_value,invalid_type", + [ + ("not_a_number", "integer"), + ("yes", "boolean"), + ("hello", "invalid_type"), + ], +) +def test_set_value_preserves_original_value_on_error( + feature_state: FeatureState, + invalid_value: str, + invalid_type: str, +) -> None: + # Given + fsv = feature_state.feature_state_value + fsv.set_value("42", "integer") + fsv.save() + + assert fsv.integer_value == 42 + assert fsv.type == INTEGER + + # When - try to set invalid value + with pytest.raises(ValueError): + fsv.set_value(invalid_value, invalid_type) + + # Then - original value should be preserved + assert fsv.integer_value == 42 + assert fsv.type == INTEGER + assert fsv.value == 42 diff --git a/api/tests/unit/features/test_unit_features_views.py b/api/tests/unit/features/test_unit_features_views.py index c53a59e1234a..23b79dc59565 100644 --- a/api/tests/unit/features/test_unit_features_views.py +++ b/api/tests/unit/features/test_unit_features_views.py @@ -47,7 +47,7 @@ from features.feature_types import MULTIVARIATE from features.models import Feature, FeatureSegment, FeatureState from features.multivariate.models import MultivariateFeatureOption -from features.value_types import BOOLEAN, INTEGER, STRING +from features.value_types import STRING from features.versioning.models import EnvironmentFeatureVersion from metadata.models import MetadataModelField from organisations.models import Organisation, OrganisationRole @@ -3341,9 +3341,7 @@ def test_list_features_with_feature_state( version=100, ) feature_state_value_versioned = feature_state_versioned.feature_state_value - feature_state_value_versioned.string_value = None - feature_state_value_versioned.integer_value = 2005 - feature_state_value_versioned.type = INTEGER + feature_state_value_versioned.set_value("2005", "integer") feature_state_value_versioned.save() feature_state2 = feature2.feature_states.filter(environment=environment).first() @@ -3351,9 +3349,7 @@ def test_list_features_with_feature_state( feature_state2.save() feature_state_value2 = feature_state2.feature_state_value - feature_state_value2.string_value = None - feature_state_value2.boolean_value = True - feature_state_value2.type = BOOLEAN + feature_state_value2.set_value("true", "boolean") feature_state_value2.save() feature_state_value3 = ( @@ -3361,7 +3357,7 @@ def test_list_features_with_feature_state( .first() .feature_state_value ) - feature_state_value3.string_value = "present" + feature_state_value3.set_value("present", "string") feature_state_value3.save() # This should be ignored due to identity being set. @@ -3470,9 +3466,7 @@ def test_list_features_with_filter_by_value_search_string_and_int( environment_feature_version1.publish(staff_user) feature_state_value1 = feature_state1.feature_state_value - feature_state_value1.string_value = None - feature_state_value1.integer_value = 1945 - feature_state_value1.type = INTEGER + feature_state_value1.set_value("1945", "integer") feature_state_value1.save() feature_state2 = feature2.feature_states.filter(environment=environment).first() @@ -3480,9 +3474,7 @@ def test_list_features_with_filter_by_value_search_string_and_int( feature_state2.save() feature_state_value2 = feature_state2.feature_state_value - feature_state_value2.string_value = None - feature_state_value2.boolean_value = True - feature_state_value2.type = BOOLEAN + feature_state_value2.set_value("true", "boolean") feature_state_value2.save() feature_state_value3 = ( @@ -3490,8 +3482,7 @@ def test_list_features_with_filter_by_value_search_string_and_int( .first() .feature_state_value ) - feature_state_value3.string_value = "present" - feature_state_value3.type = STRING + feature_state_value3.set_value("present", "string") feature_state_value3.save() feature_state4 = feature4.feature_states.filter(environment=environment).first() @@ -3499,8 +3490,7 @@ def test_list_features_with_filter_by_value_search_string_and_int( feature_state4.save() feature_state_value4 = feature_state4.feature_state_value - feature_state_value4.string_value = "year 1945" - feature_state_value4.type = STRING + feature_state_value4.set_value("year 1945", "string") feature_state_value4.save() base_url = reverse("api-v1:projects:project-features-list", args=[project.id]) @@ -3548,9 +3538,7 @@ def test_list_features_with_filter_by_search_value_boolean( feature_state1.save() # type: ignore[union-attr] feature_state_value1 = feature_state1.feature_state_value # type: ignore[union-attr] - feature_state_value1.string_value = None - feature_state_value1.integer_value = 1945 - feature_state_value1.type = INTEGER + feature_state_value1.set_value("1945", "integer") feature_state_value1.save() feature_state2 = feature2.feature_states.filter(environment=environment).first() @@ -3558,9 +3546,7 @@ def test_list_features_with_filter_by_search_value_boolean( feature_state2.save() feature_state_value2 = feature_state2.feature_state_value - feature_state_value2.string_value = None - feature_state_value2.boolean_value = True - feature_state_value2.type = BOOLEAN + feature_state_value2.set_value("true", "boolean") feature_state_value2.save() feature_state_value3 = ( @@ -3568,8 +3554,7 @@ def test_list_features_with_filter_by_search_value_boolean( .first() .feature_state_value ) - feature_state_value3.string_value = "present" - feature_state_value3.type = STRING + feature_state_value3.set_value("present", "string") feature_state_value3.save() feature_state4 = feature4.feature_states.filter(environment=environment).first() @@ -3577,8 +3562,7 @@ def test_list_features_with_filter_by_search_value_boolean( feature_state4.save() feature_state_value4 = feature_state4.feature_state_value - feature_state_value4.string_value = "year 1945" - feature_state_value4.type = STRING + feature_state_value4.set_value("year 1945", "string") feature_state_value4.save() base_url = reverse("api-v1:projects:project-features-list", args=[project.id]) @@ -4094,7 +4078,7 @@ def test_list_features_segment_query_param_with_valid_segment( environment=environment, enabled=True, ) - segment_override.feature_state_value.string_value = "segment_value" + segment_override.feature_state_value.set_value("segment_value", "string") segment_override.feature_state_value.save() base_url = reverse("api-v1:projects:project-features-list", args=[project.id]) From 39c4b44838c9ca6cd5f59b1c3d1db8da75887b5e Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Thu, 4 Dec 2025 13:15:26 +0530 Subject: [PATCH 16/26] fix: add identity filter to V1 versioning environment default queries The filter Q(feature_segment__isnull=True) doesn't exclude identity overrides, which also have feature_segment=None. Added identity_id__isnull=True to correctly retrieve only the environment default state. --- api/features/versioning/versioning_service.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/features/versioning/versioning_service.py b/api/features/versioning/versioning_service.py index 58dea7d68d59..3a343f3abe17 100644 --- a/api/features/versioning/versioning_service.py +++ b/api/features/versioning/versioning_service.py @@ -185,7 +185,7 @@ def _update_flag_for_versioning_v1( if change_set.segment_id: additional_filters = Q(feature_segment__segment_id=change_set.segment_id) else: - additional_filters = Q(feature_segment__isnull=True) + additional_filters = Q(feature_segment__isnull=True, identity_id__isnull=True) latest_feature_states = get_environment_flags_dict( environment=environment, @@ -363,7 +363,7 @@ def _update_flag_v2_for_versioning_v1( env_default_states = get_environment_flags_dict( environment=environment, feature_name=feature.name, - additional_filters=Q(feature_segment__isnull=True), + additional_filters=Q(feature_segment__isnull=True, identity_id__isnull=True), ) assert len(env_default_states) == 1 From 8d43e11f9f10863a7cfe638569a11b85c3ad8071 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Thu, 4 Dec 2025 13:32:05 +0530 Subject: [PATCH 17/26] fix: use .to() for priority reordering when creating new segment overrides Creating a FeatureSegment with a priority value doesn't trigger reordering of existing segments. Use .to() after creation to properly reorder. Fixes both V1 and V2 endpoints. --- api/features/versioning/versioning_service.py | 16 ++- .../test_unit_feature_states_views.py | 125 ++++++++++++++++++ 2 files changed, 134 insertions(+), 7 deletions(-) diff --git a/api/features/versioning/versioning_service.py b/api/features/versioning/versioning_service.py index 3a343f3abe17..8c3a8ea14851 100644 --- a/api/features/versioning/versioning_service.py +++ b/api/features/versioning/versioning_service.py @@ -137,11 +137,11 @@ def _update_flag_for_versioning_v2( feature=feature, segment_id=change_set.segment_id, environment=environment, - priority=change_set.segment_priority - if change_set.segment_priority is not None - else 0, ) + if change_set.segment_priority is not None: + feature_segment.to(change_set.segment_priority) + target_feature_state = FeatureState.objects.create( feature=feature, environment=environment, @@ -198,11 +198,11 @@ def _update_flag_for_versioning_v1( feature=feature, segment_id=change_set.segment_id, environment=environment, - priority=change_set.segment_priority - if change_set.segment_priority is not None - else 0, ) + if change_set.segment_priority is not None: + feature_segment.to(change_set.segment_priority) + target_feature_state: FeatureState = FeatureState.objects.create( feature=feature, environment=environment, @@ -257,9 +257,11 @@ def _create_segment_override( feature=feature, segment_id=segment_id, environment=environment, - priority=priority if priority is not None else 0, ) + if priority is not None: + feature_segment.to(priority) + segment_state: FeatureState = FeatureState.objects.create( feature=feature, environment=environment, diff --git a/api/tests/unit/features/feature_states/test_unit_feature_states_views.py b/api/tests/unit/features/feature_states/test_unit_feature_states_views.py index 9fc38227a02e..044c4e7d08c9 100644 --- a/api/tests/unit/features/feature_states/test_unit_feature_states_views.py +++ b/api/tests/unit/features/feature_states/test_unit_feature_states_views.py @@ -391,6 +391,65 @@ def test_update_flag_segment_override_creates_feature_segment_if_not_exists( ][0] assert segment_override.enabled is True assert segment_override.get_feature_state_value() == "premium_feature" + assert segment_override.feature_segment is not None + assert segment_override.feature_segment.priority == 10 + + +def test_create_new_segment_override_reorders_priorities_v1( + staff_client: APIClient, + feature: Feature, + environment: Environment, + project: Project, + with_environment_permissions: WithEnvironmentPermissionsCallable, +) -> None: + # Given + with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] + + segment1 = Segment.objects.create(name="segment1", project=project) + segment2 = Segment.objects.create(name="segment2", project=project) + + # Create existing segment override at priority 0 + feature_segment1 = FeatureSegment.objects.create( + feature=feature, + segment=segment1, + environment=environment, + priority=0, + ) + FeatureState.objects.create( + feature=feature, + environment=environment, + feature_segment=feature_segment1, + enabled=True, + ) + + url = reverse( + "api-experiments:update-flag-v1", + kwargs={"environment_key": environment.api_key}, + ) + + # When - Create new segment override at priority 0 (should push segment1 to priority 1) + data = { + "feature": {"name": feature.name}, + "segment": {"id": segment2.id, "priority": 0}, + "enabled": True, + "value": {"type": "string", "string_value": "new_segment_value"}, + } + response = staff_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_204_NO_CONTENT + + # Refresh from DB + feature_segment1.refresh_from_db() + feature_segment2 = FeatureSegment.objects.get( + feature=feature, segment=segment2, environment=environment + ) + + # segment2 should be at priority 0, segment1 should have been pushed to priority 1 + assert feature_segment2.priority == 0 + assert feature_segment1.priority == 1 # Update Multiple Feature States Tests @@ -709,6 +768,72 @@ def test_update_existing_segment_override_with_priority_v2( assert feature_segment.priority == 2 +def test_create_new_segment_override_reorders_priorities_v2( + staff_client: APIClient, + feature: Feature, + environment: Environment, + project: Project, + with_environment_permissions: WithEnvironmentPermissionsCallable, +) -> None: + # Given + with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] + + segment1 = Segment.objects.create(name="segment1_v2", project=project) + segment2 = Segment.objects.create(name="segment2_v2", project=project) + + # Create existing segment override at priority 0 + feature_segment1 = FeatureSegment.objects.create( + feature=feature, + segment=segment1, + environment=environment, + priority=0, + ) + FeatureState.objects.create( + feature=feature, + environment=environment, + feature_segment=feature_segment1, + enabled=True, + ) + + url = reverse( + "api-experiments:update-flag-v2", + kwargs={"environment_key": environment.api_key}, + ) + + # When - Create new segment override at priority 0 (should push segment1 to priority 1) + data = { + "feature": {"name": feature.name}, + "environment_default": { + "enabled": True, + "value": {"type": "string", "string_value": "default"}, + }, + "segment_overrides": [ + { + "segment_id": segment2.id, + "priority": 0, + "enabled": True, + "value": {"type": "string", "string_value": "new_segment_value"}, + }, + ], + } + response = staff_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_204_NO_CONTENT + + # Refresh from DB + feature_segment1.refresh_from_db() + feature_segment2 = FeatureSegment.objects.get( + feature=feature, segment=segment2, environment=environment + ) + + # segment2 should be at priority 0, segment1 should have been pushed to priority 1 + assert feature_segment2.priority == 0 + assert feature_segment1.priority == 1 + + def test_update_flag_v1_returns_403_when_workflow_enabled( staff_client: APIClient, feature: Feature, From ce926825f33b17c8cb97e934a326f547b5a20305 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Thu, 4 Dec 2025 17:15:21 +0530 Subject: [PATCH 18/26] cleanup: refactor versioning service to remove early returns and use helper --- api/features/versioning/versioning_service.py | 32 ++++--------------- 1 file changed, 6 insertions(+), 26 deletions(-) diff --git a/api/features/versioning/versioning_service.py b/api/features/versioning/versioning_service.py index 8c3a8ea14851..7cce7807ef1b 100644 --- a/api/features/versioning/versioning_service.py +++ b/api/features/versioning/versioning_service.py @@ -139,9 +139,6 @@ def _update_flag_for_versioning_v2( environment=environment, ) - if change_set.segment_priority is not None: - feature_segment.to(change_set.segment_priority) - target_feature_state = FeatureState.objects.create( feature=feature, environment=environment, @@ -166,9 +163,7 @@ def _update_flag_for_versioning_v2( ) if change_set.segment_id and change_set.segment_priority is not None: - feature_segment = target_feature_state.feature_segment - if feature_segment: - feature_segment.to(change_set.segment_priority) + _update_segment_priority(target_feature_state, change_set.segment_priority) new_version.publish( published_by=change_set.user, published_by_api_key=change_set.api_key @@ -200,29 +195,16 @@ def _update_flag_for_versioning_v1( environment=environment, ) - if change_set.segment_priority is not None: - feature_segment.to(change_set.segment_priority) - target_feature_state: FeatureState = FeatureState.objects.create( feature=feature, environment=environment, feature_segment=feature_segment, enabled=change_set.enabled, ) - - _update_feature_state_value( - target_feature_state.feature_state_value, - change_set.feature_state_value, - change_set.type_, - ) - - return target_feature_state - - assert len(latest_feature_states) == 1 - - target_feature_state = list(latest_feature_states.values())[0] - target_feature_state.enabled = change_set.enabled - target_feature_state.save() + else: + target_feature_state = list(latest_feature_states.values())[0] + target_feature_state.enabled = change_set.enabled + target_feature_state.save() _update_feature_state_value( target_feature_state.feature_state_value, @@ -231,9 +213,7 @@ def _update_flag_for_versioning_v1( ) if change_set.segment_id and change_set.segment_priority is not None: - feature_segment = target_feature_state.feature_segment - if feature_segment: - feature_segment.to(change_set.segment_priority) + _update_segment_priority(target_feature_state, change_set.segment_priority) return target_feature_state From b8eeb6d113f149c12fe9a35d3e548b2ed4a46f19 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Thu, 4 Dec 2025 17:36:08 +0530 Subject: [PATCH 19/26] fix: validate segment belongs to project before creating segment override Prevents cross-project segment references and provides clear error messages for non-existent segments. --- api/features/feature_states/serializers.py | 34 ++++-- .../feature_states/test_serializers.py | 104 ++++++++++++++++++ 2 files changed, 131 insertions(+), 7 deletions(-) diff --git a/api/features/feature_states/serializers.py b/api/features/feature_states/serializers.py index 7b11b72fb338..875de1ec9f1e 100644 --- a/api/features/feature_states/serializers.py +++ b/api/features/feature_states/serializers.py @@ -8,18 +8,22 @@ SegmentOverrideChangeSet, ) from features.versioning.versioning_service import update_flag, update_flag_v2 +from segments.models import Segment class BaseFeatureUpdateSerializer(serializers.Serializer): # type: ignore[type-arg] - def get_feature(self) -> Feature: - feature_data = self.validated_data["feature"] - environment: Environment = self.context.get("environment") # type: ignore[assignment] - + @property + def environment(self) -> Environment: + environment: Environment | None = self.context.get("environment") if not environment: raise serializers.ValidationError("Environment context is required") + return environment + + def get_feature(self) -> Feature: + feature_data = self.validated_data["feature"] try: feature: Feature = Feature.objects.get( - project_id=environment.project_id, **feature_data + project_id=self.environment.project_id, **feature_data ) return feature except Feature.DoesNotExist: @@ -27,6 +31,14 @@ def get_feature(self) -> Feature: f"Feature '{feature_data}' not found in project" ) + def validate_segment_id(self, segment_id: int) -> None: + if not Segment.objects.filter( + id=segment_id, project_id=self.environment.project_id + ).exists(): + raise serializers.ValidationError( + f"Segment with id {segment_id} not found in project" + ) + class FeatureIdentifierSerializer(serializers.Serializer): # type: ignore[type-arg] name = serializers.CharField(required=False, allow_blank=False) @@ -79,6 +91,11 @@ class UpdateFlagSerializer(BaseFeatureUpdateSerializer): enabled = serializers.BooleanField(required=True) value = FeatureValueSerializer(required=True) + def validate_segment(self, value: dict) -> dict: # type: ignore[type-arg] + if value and value.get("id"): + self.validate_segment_id(value["id"]) + return value + @property def flag_change_set(self) -> FlagChangeSet: validated_data = self.validated_data @@ -98,7 +115,7 @@ def flag_change_set(self) -> FlagChangeSet: def save(self, **kwargs: object) -> FeatureState: feature = self.get_feature() - return update_flag(self.context["environment"], feature, self.flag_change_set) + return update_flag(self.environment, feature, self.flag_change_set) class EnvironmentDefaultSerializer(serializers.Serializer): # type: ignore[type-arg] @@ -131,6 +148,9 @@ def validate_segment_overrides( "Duplicate segment_id values are not allowed" ) + for segment_id in segment_ids: + self.validate_segment_id(segment_id) + return value @property @@ -167,4 +187,4 @@ def change_set_v2(self) -> FlagChangeSetV2: def save(self, **kwargs: object) -> dict: # type: ignore[type-arg] feature = self.get_feature() - return update_flag_v2(self.context["environment"], feature, self.change_set_v2) + return update_flag_v2(self.environment, feature, self.change_set_v2) diff --git a/api/tests/unit/features/feature_states/test_serializers.py b/api/tests/unit/features/feature_states/test_serializers.py index 5e9ab4f41160..e605db9fb8bb 100644 --- a/api/tests/unit/features/feature_states/test_serializers.py +++ b/api/tests/unit/features/feature_states/test_serializers.py @@ -1,11 +1,16 @@ +import typing + import pytest +from environments.models import Environment from features.feature_states.serializers import ( FeatureValueSerializer, UpdateFlagSerializer, UpdateFlagV2Serializer, ) from features.models import Feature +from projects.models import Project +from segments.models import Segment def test_get_feature_raises_error_when_environment_not_in_context( @@ -48,3 +53,102 @@ def test_feature_value_serializer_rejects_invalid_boolean() -> None: assert serializer.is_valid() is False assert "not a valid boolean" in str(serializer.errors) + + +@pytest.mark.parametrize( + "serializer_class,data_factory", + [ + ( + UpdateFlagSerializer, + lambda feature, segment_id: { + "feature": {"name": feature.name}, + "segment": {"id": segment_id}, + "enabled": True, + "value": {"type": "string", "string_value": "test"}, + }, + ), + ( + UpdateFlagV2Serializer, + lambda feature, segment_id: { + "feature": {"name": feature.name}, + "environment_default": { + "enabled": True, + "value": {"type": "string", "string_value": "default"}, + }, + "segment_overrides": [ + { + "segment_id": segment_id, + "enabled": True, + "value": {"type": "string", "string_value": "test"}, + }, + ], + }, + ), + ], +) +def test_serializer_rejects_nonexistent_segment( + feature: Feature, + environment: Environment, + serializer_class: type, + data_factory: typing.Callable[[Feature, int], dict], # type: ignore[type-arg] +) -> None: + serializer = serializer_class( + data=data_factory(feature, 999999), + context={"environment": environment}, + ) + + assert serializer.is_valid() is False + assert "not found in project" in str(serializer.errors) + + +@pytest.mark.parametrize( + "serializer_class,data_factory", + [ + ( + UpdateFlagSerializer, + lambda feature, segment_id: { + "feature": {"name": feature.name}, + "segment": {"id": segment_id}, + "enabled": True, + "value": {"type": "string", "string_value": "test"}, + }, + ), + ( + UpdateFlagV2Serializer, + lambda feature, segment_id: { + "feature": {"name": feature.name}, + "environment_default": { + "enabled": True, + "value": {"type": "string", "string_value": "default"}, + }, + "segment_overrides": [ + { + "segment_id": segment_id, + "enabled": True, + "value": {"type": "string", "string_value": "test"}, + }, + ], + }, + ), + ], +) +def test_serializer_rejects_cross_project_segment( + feature: Feature, + environment: Environment, + organisation: object, + serializer_class: type, + data_factory: typing.Callable[[Feature, int], dict], # type: ignore[type-arg] +) -> None: + other_project = Project.objects.create( + name="Other Project", + organisation=organisation, + ) + other_segment = Segment.objects.create(name="other_segment", project=other_project) + + serializer = serializer_class( + data=data_factory(feature, other_segment.id), + context={"environment": environment}, + ) + + assert serializer.is_valid() is False + assert "not found in project" in str(serializer.errors) From 2b78c5e38a8d55c037dd6c69c5c89021fc3b0df1 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Fri, 5 Dec 2025 11:10:14 +0530 Subject: [PATCH 20/26] chore: review fixes --- api/features/versioning/versioning_service.py | 3 ++ .../feature_states/test_serializers.py | 39 ++++++++++++++++--- 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/api/features/versioning/versioning_service.py b/api/features/versioning/versioning_service.py index 7cce7807ef1b..ff2a6bff191a 100644 --- a/api/features/versioning/versioning_service.py +++ b/api/features/versioning/versioning_service.py @@ -202,6 +202,7 @@ def _update_flag_for_versioning_v1( enabled=change_set.enabled, ) else: + assert len(latest_feature_states) == 1 target_feature_state = list(latest_feature_states.values())[0] target_feature_state.enabled = change_set.enabled target_feature_state.save() @@ -361,6 +362,8 @@ def _update_flag_v2_for_versioning_v1( updated_segments = [] for override in change_set.segment_overrides: + # TODO: optimise this once this is out of the + # experimentation stage segment_states = get_environment_flags_dict( environment=environment, feature_name=feature.name, diff --git a/api/tests/unit/features/feature_states/test_serializers.py b/api/tests/unit/features/feature_states/test_serializers.py index e605db9fb8bb..1761852967b3 100644 --- a/api/tests/unit/features/feature_states/test_serializers.py +++ b/api/tests/unit/features/feature_states/test_serializers.py @@ -1,6 +1,7 @@ import typing import pytest +from rest_framework import serializers from environments.models import Environment from features.feature_states.serializers import ( @@ -16,6 +17,7 @@ def test_get_feature_raises_error_when_environment_not_in_context( feature: Feature, ) -> None: + # Given serializer = UpdateFlagSerializer( data={ "feature": {"name": feature.name}, @@ -26,32 +28,48 @@ def test_get_feature_raises_error_when_environment_not_in_context( ) serializer.is_valid() - with pytest.raises(Exception) as exc_info: + # When + with pytest.raises(serializers.ValidationError) as exc_info: serializer.get_feature() + # Then assert "Environment context is required" in str(exc_info.value) def test_validate_segment_overrides_returns_empty_list() -> None: + # Given serializer = UpdateFlagV2Serializer() + + # When result = serializer.validate_segment_overrides([]) + # Then assert result == [] def test_feature_value_serializer_rejects_invalid_integer() -> None: + # Given serializer = FeatureValueSerializer( data={"type": "integer", "string_value": "not_a_number"} ) - assert serializer.is_valid() is False + # When + is_valid = serializer.is_valid() + + # Then + assert is_valid is False assert "not a valid integer" in str(serializer.errors) def test_feature_value_serializer_rejects_invalid_boolean() -> None: + # Given serializer = FeatureValueSerializer(data={"type": "boolean", "string_value": "yes"}) - assert serializer.is_valid() is False + # When + is_valid = serializer.is_valid() + + # Then + assert is_valid is False assert "not a valid boolean" in str(serializer.errors) @@ -92,12 +110,17 @@ def test_serializer_rejects_nonexistent_segment( serializer_class: type, data_factory: typing.Callable[[Feature, int], dict], # type: ignore[type-arg] ) -> None: + # Given serializer = serializer_class( data=data_factory(feature, 999999), context={"environment": environment}, ) - assert serializer.is_valid() is False + # When + is_valid = serializer.is_valid() + + # Then + assert is_valid is False assert "not found in project" in str(serializer.errors) @@ -139,16 +162,20 @@ def test_serializer_rejects_cross_project_segment( serializer_class: type, data_factory: typing.Callable[[Feature, int], dict], # type: ignore[type-arg] ) -> None: + # Given other_project = Project.objects.create( name="Other Project", organisation=organisation, ) other_segment = Segment.objects.create(name="other_segment", project=other_project) - serializer = serializer_class( data=data_factory(feature, other_segment.id), context={"environment": environment}, ) - assert serializer.is_valid() is False + # When + is_valid = serializer.is_valid() + + # Then + assert is_valid is False assert "not found in project" in str(serializer.errors) From 3704c9ca00846e84e79ba179e47446c817a6e99c Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Fri, 5 Dec 2025 12:39:00 +0530 Subject: [PATCH 21/26] chore: remove outdated TODO comment --- api/environments/identities/models.py | 1 - 1 file changed, 1 deletion(-) diff --git a/api/environments/identities/models.py b/api/environments/identities/models.py index 7cb1ea74214f..73b374f619ac 100644 --- a/api/environments/identities/models.py +++ b/api/environments/identities/models.py @@ -233,7 +233,6 @@ def generate_traits( def update_traits( self, - # TODO: this typing isn't true as it also supports regular dicts like {"trait_key": "foo", "trait_value": "bar"} trait_data_items: list[SDKTraitData], ) -> list[Trait]: """ From c979e289755aa3472536553c02967ef58c7364a9 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Tue, 9 Dec 2025 11:46:05 +0530 Subject: [PATCH 22/26] chore: address PR review feedback from khvn26 - Refactor AuditFieldsMixin into AuthorData dataclass with from_request factory - Add FeatureValueType Literal type for type safety - Remove unused return values from update_flag_v2 functions - Add TODO comment for segment validation optimization - Improve priority documentation wording --- api/features/feature_states/models.py | 5 +- api/features/feature_states/serializers.py | 28 +++++---- api/features/feature_states/views.py | 27 ++++----- api/features/versioning/dataclasses.py | 48 ++++++++------- api/features/versioning/versioning_service.py | 51 ++++++---------- .../features/feature_states/test_models.py | 9 ++- .../feature_states/test_serializers.py | 18 +++--- .../test_unit_feature_states_views.py | 58 +++++++++---------- .../test_unit_versioning_dataclasses.py | 25 +++----- 9 files changed, 124 insertions(+), 145 deletions(-) diff --git a/api/features/feature_states/models.py b/api/features/feature_states/models.py index 3f06d592333f..b75c005bbb12 100644 --- a/api/features/feature_states/models.py +++ b/api/features/feature_states/models.py @@ -1,4 +1,5 @@ import typing +from typing import Literal from django.conf import settings from django.db import models @@ -10,6 +11,8 @@ STRING, ) +FeatureValueType = Literal["string", "integer", "boolean"] + class AbstractBaseFeatureValueModel(models.Model): class Meta: @@ -39,7 +42,7 @@ def value(self) -> typing.Union[str, int, bool]: self.string_value, ) - def set_value(self, value: str, type_: str) -> None: + def set_value(self, value: str, type_: FeatureValueType) -> None: typed_value: str | int | bool match type_: case "string": diff --git a/api/features/feature_states/serializers.py b/api/features/feature_states/serializers.py index 875de1ec9f1e..d63bd1fbc075 100644 --- a/api/features/feature_states/serializers.py +++ b/api/features/feature_states/serializers.py @@ -3,6 +3,7 @@ from environments.models import Environment from features.models import Feature, FeatureState from features.versioning.dataclasses import ( + AuthorData, FlagChangeSet, FlagChangeSetV2, SegmentOverrideChangeSet, @@ -63,11 +64,11 @@ class FeatureValueSerializer(serializers.Serializer): # type: ignore[type-arg] type = serializers.ChoiceField( choices=["integer", "string", "boolean"], required=True ) - string_value = serializers.CharField(required=True, allow_blank=True) + value = serializers.CharField(required=True, allow_blank=True) def validate(self, data: dict) -> dict: # type: ignore[type-arg] value_type = data["type"] - string_val = data["string_value"] + string_val = data["value"] if value_type == "integer": try: @@ -102,17 +103,15 @@ def flag_change_set(self) -> FlagChangeSet: value_data = validated_data["value"] segment_data = validated_data.get("segment") - change_set = FlagChangeSet( + return FlagChangeSet( + author=AuthorData.from_request(self.context["request"]), enabled=validated_data["enabled"], - feature_state_value=value_data["string_value"], + feature_state_value=value_data["value"], type_=value_data["type"], segment_id=segment_data.get("id") if segment_data else None, segment_priority=segment_data.get("priority") if segment_data else None, ) - change_set.set_audit_fields_from_request(self.context["request"]) - return change_set - def save(self, **kwargs: object) -> FeatureState: feature = self.get_feature() return update_flag(self.environment, feature, self.flag_change_set) @@ -148,6 +147,7 @@ def validate_segment_overrides( "Duplicate segment_id values are not allowed" ) + # TODO: optimise this once out of experimentation for segment_id in segment_ids: self.validate_segment_id(segment_id) @@ -169,22 +169,20 @@ def change_set_v2(self) -> FlagChangeSetV2: segment_override = SegmentOverrideChangeSet( segment_id=override_data["segment_id"], enabled=override_data["enabled"], - feature_state_value=value_data["string_value"], + feature_state_value=value_data["value"], type_=value_data["type"], priority=override_data.get("priority"), ) segment_overrides.append(segment_override) - change_set = FlagChangeSetV2( + return FlagChangeSetV2( + author=AuthorData.from_request(self.context["request"]), environment_default_enabled=env_default["enabled"], - environment_default_value=env_value_data["string_value"], + environment_default_value=env_value_data["value"], environment_default_type=env_value_data["type"], segment_overrides=segment_overrides, ) - change_set.set_audit_fields_from_request(self.context["request"]) - return change_set - - def save(self, **kwargs: object) -> dict: # type: ignore[type-arg] + def save(self, **kwargs: object) -> None: feature = self.get_feature() - return update_flag_v2(self.environment, feature, self.change_set_v2) + update_flag_v2(self.environment, feature, self.change_set_v2) diff --git a/api/features/feature_states/views.py b/api/features/feature_states/views.py index 80d5610b9d7f..d992e42a7072 100644 --- a/api/features/feature_states/views.py +++ b/api/features/feature_states/views.py @@ -23,7 +23,7 @@ def _check_workflow_not_enabled(environment: Environment) -> None: @swagger_auto_schema( method="post", - operation_summary="Update single feature state (V1)", + operation_summary="Update single feature state", operation_description=""" **EXPERIMENTAL ENDPOINT** - Subject to change without notice. @@ -33,21 +33,19 @@ def _check_workflow_not_enabled(environment: Environment) -> None: **Feature Identification:** - Use `feature.name` OR `feature.id` (mutually exclusive) - - Feature must belong to the environment's project **Value Format:** - - Always use `string_value` field (value is always a string) + - The `value` field is always a string representation - The `type` field tells the server how to parse it - Available types: integer, string, boolean - Examples: - - `{"type": "integer", "string_value": "42"}` - - `{"type": "boolean", "string_value": "true"}` - - `{"type": "string", "string_value": "hello"}` + - `{"type": "integer", "value": "42"}` + - `{"type": "boolean", "value": "true"}` + - `{"type": "string", "value": "hello"}` **Segment Priority:** - Optional `segment.priority` field controls ordering - - If null/omitted, segment is appended to end - - Use specific priority value to reorder + - If field value is null or the field is omitted, lowest priority is assumed """, manual_parameters=[ openapi.Parameter( @@ -64,7 +62,7 @@ def _check_workflow_not_enabled(environment: Environment) -> None: description="Feature state updated successfully (no content returned)" ) }, - tags=["Experimental - Feature States"], + tags=["experimental"], ) # type: ignore[misc] @api_view(http_method_names=["POST"]) @permission_classes([IsAuthenticated, EnvironmentUpdateFeatureStatePermission]) @@ -99,16 +97,15 @@ def update_flag_v1(request: Request, environment_key: str) -> Response: **Feature Identification:** - Use `feature.name` OR `feature.id` (mutually exclusive) - - Feature must belong to the environment's project **Value Format:** - - Always use `string_value` field (value is always a string) + - The `value` field is always a string representation - The `type` field tells the server how to parse it - Available types: integer, string, boolean - Examples: - - `{"type": "string", "string_value": "production"}` - - `{"type": "integer", "string_value": "100"}` - - `{"type": "boolean", "string_value": "false"}` + - `{"type": "string", "value": "production"}` + - `{"type": "integer", "value": "100"}` + - `{"type": "boolean", "value": "false"}` **Segment Overrides:** - Provide array of segment override configurations @@ -132,7 +129,7 @@ def update_flag_v1(request: Request, environment_key: str) -> Response: description="Feature states updated successfully (no content returned)" ) }, - tags=["Experimental - Feature States"], + tags=["experimental"], ) # type: ignore[misc] @api_view(http_method_names=["POST"]) @permission_classes([IsAuthenticated, EnvironmentUpdateFeatureStatePermission]) diff --git a/api/features/versioning/dataclasses.py b/api/features/versioning/dataclasses.py index 48719bed1f41..029cfc2e75c2 100644 --- a/api/features/versioning/dataclasses.py +++ b/api/features/versioning/dataclasses.py @@ -6,12 +6,32 @@ from pydantic import BaseModel, computed_field +from features.feature_states.models import FeatureValueType + if TYPE_CHECKING: from rest_framework.request import Request + from api_keys.models import MasterAPIKey from users.models import FFAdminUser +@dataclass +class AuthorData: + user: FFAdminUser | None = None + api_key: MasterAPIKey | None = None + + @classmethod + def from_request(cls, request: Request) -> AuthorData: + from users.models import FFAdminUser + + if type(request.user) is FFAdminUser: + return cls(user=request.user) + elif hasattr(request.user, "key"): + return cls(api_key=request.user.key) + else: + raise ValueError("Request user must be FFAdminUser or have an API key") + + class Conflict(BaseModel): segment_id: int | None = None original_cr_id: int | None = None @@ -24,26 +44,11 @@ def is_environment_default(self) -> bool: @dataclass -class AuditFieldsMixin: - user: FFAdminUser | None = field(default=None, kw_only=True) - api_key: str | None = field(default=None, kw_only=True) - - def set_audit_fields_from_request(self, request: Request) -> None: - from users.models import FFAdminUser - - if type(request.user) is FFAdminUser: - self.user = request.user - elif hasattr(request.user, "key"): - self.api_key = request.user.key - else: - raise ValueError("Request user must be FFAdminUser or have an API key") - - -@dataclass -class FlagChangeSet(AuditFieldsMixin): +class FlagChangeSet: + author: AuthorData enabled: bool feature_state_value: str - type_: str + type_: FeatureValueType segment_id: int | None = None segment_priority: int | None = None @@ -54,14 +59,15 @@ class SegmentOverrideChangeSet: segment_id: int enabled: bool feature_state_value: str - type_: str + type_: FeatureValueType priority: int | None = None @dataclass -class FlagChangeSetV2(AuditFieldsMixin): +class FlagChangeSetV2: + author: AuthorData environment_default_enabled: bool environment_default_value: str - environment_default_type: str + environment_default_type: FeatureValueType segment_overrides: list[SegmentOverrideChangeSet] = field(default_factory=list) diff --git a/api/features/versioning/versioning_service.py b/api/features/versioning/versioning_service.py index ff2a6bff191a..0c9fd078a33e 100644 --- a/api/features/versioning/versioning_service.py +++ b/api/features/versioning/versioning_service.py @@ -5,6 +5,7 @@ from django.utils import timezone from environments.models import Environment +from features.feature_states.models import FeatureValueType from features.models import Feature, FeatureState, FeatureStateValue from features.versioning.dataclasses import ( FlagChangeSet, @@ -122,8 +123,8 @@ def _update_flag_for_versioning_v2( new_version = EnvironmentFeatureVersion.objects.create( environment=environment, feature=feature, - created_by=change_set.user, - created_by_api_key=change_set.api_key, + created_by=change_set.author.user, + created_by_api_key=change_set.author.api_key, ) if change_set.segment_id: @@ -166,7 +167,8 @@ def _update_flag_for_versioning_v2( _update_segment_priority(target_feature_state, change_set.segment_priority) new_version.publish( - published_by=change_set.user, published_by_api_key=change_set.api_key + published_by=change_set.author.user, + published_by_api_key=change_set.author.api_key, ) return target_feature_state @@ -219,7 +221,9 @@ def _update_flag_for_versioning_v1( return target_feature_state -def _update_feature_state_value(fsv: FeatureStateValue, value: str, type_: str) -> None: +def _update_feature_state_value( + fsv: FeatureStateValue, value: str, type_: FeatureValueType +) -> None: fsv.set_value(value, type_) fsv.save() @@ -262,21 +266,21 @@ def _update_segment_priority(feature_state: FeatureState, priority: int) -> None def update_flag_v2( environment: Environment, feature: Feature, change_set: FlagChangeSetV2 -) -> dict: # type: ignore[type-arg] +) -> None: if environment.use_v2_feature_versioning: - return _update_flag_v2_for_versioning_v2(environment, feature, change_set) + _update_flag_v2_for_versioning_v2(environment, feature, change_set) else: - return _update_flag_v2_for_versioning_v1(environment, feature, change_set) + _update_flag_v2_for_versioning_v1(environment, feature, change_set) def _update_flag_v2_for_versioning_v2( environment: Environment, feature: Feature, change_set: FlagChangeSetV2 -) -> dict: # type: ignore[type-arg] +) -> None: new_version = EnvironmentFeatureVersion.objects.create( environment=environment, feature=feature, - created_by=change_set.user, - created_by_api_key=change_set.api_key, + created_by=change_set.author.user, + created_by_api_key=change_set.author.api_key, ) env_default_state = new_version.feature_states.get( @@ -291,7 +295,6 @@ def _update_flag_v2_for_versioning_v2( change_set.environment_default_type, ) - updated_segments = [] for override in change_set.segment_overrides: try: segment_state = new_version.feature_states.get( @@ -324,25 +327,15 @@ def _update_flag_v2_for_versioning_v2( override.type_, ) - updated_segments.append( - {"segment_id": override.segment_id, "enabled": override.enabled} - ) - new_version.publish( - published_by=change_set.user, - published_by_api_key=change_set.api_key, + published_by=change_set.author.user, + published_by_api_key=change_set.author.api_key, ) - return { - "environment_default": {"enabled": change_set.environment_default_enabled}, - "segment_overrides": updated_segments, - "version": new_version.uuid, - } - def _update_flag_v2_for_versioning_v1( environment: Environment, feature: Feature, change_set: FlagChangeSetV2 -) -> dict: # type: ignore[type-arg] +) -> None: env_default_states = get_environment_flags_dict( environment=environment, feature_name=feature.name, @@ -360,7 +353,6 @@ def _update_flag_v2_for_versioning_v1( change_set.environment_default_type, ) - updated_segments = [] for override in change_set.segment_overrides: # TODO: optimise this once this is out of the # experimentation stage @@ -400,15 +392,6 @@ def _update_flag_v2_for_versioning_v1( if override.priority is not None: _update_segment_priority(segment_state, override.priority) - updated_segments.append( - {"segment_id": override.segment_id, "enabled": override.enabled} - ) - - return { - "environment_default": {"enabled": change_set.environment_default_enabled}, - "segment_overrides": updated_segments, - } - def get_updated_feature_states_for_version( version: EnvironmentFeatureVersion, diff --git a/api/tests/unit/features/feature_states/test_models.py b/api/tests/unit/features/feature_states/test_models.py index 528242b8acd6..90b763360177 100644 --- a/api/tests/unit/features/feature_states/test_models.py +++ b/api/tests/unit/features/feature_states/test_models.py @@ -1,5 +1,8 @@ +from typing import cast + import pytest +from features.feature_states.models import FeatureValueType from features.models import FeatureState from features.value_types import BOOLEAN, INTEGER, STRING @@ -17,7 +20,7 @@ def test_set_value( feature_state: FeatureState, value: str, - type_: str, + type_: FeatureValueType, expected_value: str | int | bool, expected_type: str, ) -> None: @@ -51,7 +54,7 @@ def test_set_value_invalid_raises_error( # When / Then with pytest.raises(ValueError) as exc_info: - fsv.set_value(value, type_) + fsv.set_value(value, cast(FeatureValueType, type_)) assert expected_error in str(exc_info.value) @@ -99,7 +102,7 @@ def test_set_value_preserves_original_value_on_error( # When - try to set invalid value with pytest.raises(ValueError): - fsv.set_value(invalid_value, invalid_type) + fsv.set_value(invalid_value, cast(FeatureValueType, invalid_type)) # Then - original value should be preserved assert fsv.integer_value == 42 diff --git a/api/tests/unit/features/feature_states/test_serializers.py b/api/tests/unit/features/feature_states/test_serializers.py index 1761852967b3..3b31a7fa9c7f 100644 --- a/api/tests/unit/features/feature_states/test_serializers.py +++ b/api/tests/unit/features/feature_states/test_serializers.py @@ -22,7 +22,7 @@ def test_get_feature_raises_error_when_environment_not_in_context( data={ "feature": {"name": feature.name}, "enabled": True, - "value": {"type": "string", "string_value": "test"}, + "value": {"type": "string", "value": "test"}, }, context={}, # No environment ) @@ -50,7 +50,7 @@ def test_validate_segment_overrides_returns_empty_list() -> None: def test_feature_value_serializer_rejects_invalid_integer() -> None: # Given serializer = FeatureValueSerializer( - data={"type": "integer", "string_value": "not_a_number"} + data={"type": "integer", "value": "not_a_number"} ) # When @@ -63,7 +63,7 @@ def test_feature_value_serializer_rejects_invalid_integer() -> None: def test_feature_value_serializer_rejects_invalid_boolean() -> None: # Given - serializer = FeatureValueSerializer(data={"type": "boolean", "string_value": "yes"}) + serializer = FeatureValueSerializer(data={"type": "boolean", "value": "yes"}) # When is_valid = serializer.is_valid() @@ -82,7 +82,7 @@ def test_feature_value_serializer_rejects_invalid_boolean() -> None: "feature": {"name": feature.name}, "segment": {"id": segment_id}, "enabled": True, - "value": {"type": "string", "string_value": "test"}, + "value": {"type": "string", "value": "test"}, }, ), ( @@ -91,13 +91,13 @@ def test_feature_value_serializer_rejects_invalid_boolean() -> None: "feature": {"name": feature.name}, "environment_default": { "enabled": True, - "value": {"type": "string", "string_value": "default"}, + "value": {"type": "string", "value": "default"}, }, "segment_overrides": [ { "segment_id": segment_id, "enabled": True, - "value": {"type": "string", "string_value": "test"}, + "value": {"type": "string", "value": "test"}, }, ], }, @@ -133,7 +133,7 @@ def test_serializer_rejects_nonexistent_segment( "feature": {"name": feature.name}, "segment": {"id": segment_id}, "enabled": True, - "value": {"type": "string", "string_value": "test"}, + "value": {"type": "string", "value": "test"}, }, ), ( @@ -142,13 +142,13 @@ def test_serializer_rejects_nonexistent_segment( "feature": {"name": feature.name}, "environment_default": { "enabled": True, - "value": {"type": "string", "string_value": "default"}, + "value": {"type": "string", "value": "default"}, }, "segment_overrides": [ { "segment_id": segment_id, "enabled": True, - "value": {"type": "string", "string_value": "test"}, + "value": {"type": "string", "value": "test"}, }, ], }, diff --git a/api/tests/unit/features/feature_states/test_unit_feature_states_views.py b/api/tests/unit/features/feature_states/test_unit_feature_states_views.py index 044c4e7d08c9..f290abdaed16 100644 --- a/api/tests/unit/features/feature_states/test_unit_feature_states_views.py +++ b/api/tests/unit/features/feature_states/test_unit_feature_states_views.py @@ -50,7 +50,7 @@ def test_update_flag_by_name( data = { "feature": {"name": feature.name}, "enabled": True, - "value": {"type": value_type, "string_value": string_value}, + "value": {"type": value_type, "value": string_value}, } # When @@ -89,7 +89,7 @@ def test_update_flag_by_id( data = { "feature": {"id": feature.id}, "enabled": False, - "value": {"type": "string", "string_value": "test_value"}, + "value": {"type": "string", "value": "test_value"}, } # When @@ -129,7 +129,7 @@ def test_update_flag_error_when_both_name_and_id_provided( data = { "feature": {"name": feature.name, "id": feature.id}, "enabled": True, - "value": {"type": "integer", "string_value": "42"}, + "value": {"type": "integer", "value": "42"}, } # When @@ -171,7 +171,7 @@ def test_update_flag_error_when_both_name_and_id_provided_for_different_features data = { "feature": {"name": feature.name, "id": another_feature.id}, "enabled": True, - "value": {"type": "integer", "string_value": "42"}, + "value": {"type": "integer", "value": "42"}, } # When @@ -206,7 +206,7 @@ def test_update_flag_error_when_neither_name_nor_id_provided( data = { "feature": {}, "enabled": True, - "value": {"type": "integer", "string_value": "42"}, + "value": {"type": "integer", "value": "42"}, } # When @@ -239,7 +239,7 @@ def test_update_flag_error_when_feature_not_found_by_name( data = { "feature": {"name": "non_existent_feature"}, "enabled": True, - "value": {"type": "integer", "string_value": "42"}, + "value": {"type": "integer", "value": "42"}, } # When @@ -271,7 +271,7 @@ def test_update_flag_error_when_feature_not_found_by_id( data = { "feature": {"id": 999999}, # Non-existent ID "enabled": True, - "value": {"type": "integer", "string_value": "42"}, + "value": {"type": "integer", "value": "42"}, } # When @@ -311,7 +311,7 @@ def test_update_flag_segment_override_by_name( "feature": {"name": feature.name}, "segment": {"id": segment.id, "priority": 1}, "enabled": False, - "value": {"type": "integer", "string_value": "999"}, + "value": {"type": "integer", "value": "999"}, } # When @@ -369,7 +369,7 @@ def test_update_flag_segment_override_creates_feature_segment_if_not_exists( "feature": {"name": feature.name}, "segment": {"id": segment.id, "priority": 10}, "enabled": True, - "value": {"type": "string", "string_value": "premium_feature"}, + "value": {"type": "string", "value": "premium_feature"}, } # When @@ -432,7 +432,7 @@ def test_create_new_segment_override_reorders_priorities_v1( "feature": {"name": feature.name}, "segment": {"id": segment2.id, "priority": 0}, "enabled": True, - "value": {"type": "string", "string_value": "new_segment_value"}, + "value": {"type": "string", "value": "new_segment_value"}, } response = staff_client.post( url, data=json.dumps(data), content_type="application/json" @@ -495,20 +495,20 @@ def test_update_feature_states_creates_new_segment_overrides( "feature": {"name": feature.name}, "environment_default": { "enabled": False, - "value": {"type": "string", "string_value": "default"}, + "value": {"type": "string", "value": "default"}, }, "segment_overrides": [ { "segment_id": segment1.id, "priority": 1, "enabled": True, - "value": {"type": "string", "string_value": "vip_feature"}, + "value": {"type": "string", "value": "vip_feature"}, }, { "segment_id": segment2.id, "priority": 2, "enabled": True, - "value": {"type": "string", "string_value": "beta_feature"}, + "value": {"type": "string", "value": "beta_feature"}, }, ], } @@ -565,7 +565,7 @@ def test_update_feature_states_environment_default_only( "feature": {"name": feature.name}, "environment_default": { "enabled": True, - "value": {"type": "integer", "string_value": "100"}, + "value": {"type": "integer", "value": "100"}, }, } @@ -610,18 +610,18 @@ def test_update_feature_states_rejects_duplicate_segment_ids( "feature": {"name": feature.name}, "environment_default": { "enabled": True, - "value": {"type": "string", "string_value": "default"}, + "value": {"type": "string", "value": "default"}, }, "segment_overrides": [ { "segment_id": segment1.id, "enabled": True, - "value": {"type": "string", "string_value": "value1"}, + "value": {"type": "string", "value": "value1"}, }, { "segment_id": segment1.id, # Duplicate! "enabled": False, - "value": {"type": "string", "string_value": "value2"}, + "value": {"type": "string", "value": "value2"}, }, ], } @@ -677,7 +677,7 @@ def test_update_existing_segment_override_with_priority_v1( "feature": {"name": feature.name}, "segment": {"id": segment.id, "priority": 1}, # Changed priority "enabled": False, # Changed enabled - "value": {"type": "integer", "string_value": "200"}, # Changed value + "value": {"type": "integer", "value": "200"}, # Changed value } response = staff_client.post( url, data=json.dumps(update_data), content_type="application/json" @@ -738,14 +738,14 @@ def test_update_existing_segment_override_with_priority_v2( "feature": {"name": feature.name}, "environment_default": { "enabled": True, - "value": {"type": "string", "string_value": "default"}, + "value": {"type": "string", "value": "default"}, }, "segment_overrides": [ { "segment_id": segment.id, "priority": 2, # Changed priority "enabled": False, # Changed enabled - "value": {"type": "string", "string_value": "updated"}, + "value": {"type": "string", "value": "updated"}, }, ], } @@ -805,14 +805,14 @@ def test_create_new_segment_override_reorders_priorities_v2( "feature": {"name": feature.name}, "environment_default": { "enabled": True, - "value": {"type": "string", "string_value": "default"}, + "value": {"type": "string", "value": "default"}, }, "segment_overrides": [ { "segment_id": segment2.id, "priority": 0, "enabled": True, - "value": {"type": "string", "string_value": "new_segment_value"}, + "value": {"type": "string", "value": "new_segment_value"}, }, ], } @@ -853,7 +853,7 @@ def test_update_flag_v1_returns_403_when_workflow_enabled( data = { "feature": {"name": feature.name}, "enabled": True, - "value": {"type": "string", "string_value": "test"}, + "value": {"type": "string", "value": "test"}, } # When @@ -886,7 +886,7 @@ def test_update_flag_v2_returns_403_when_workflow_enabled( "feature": {"name": feature.name}, "environment_default": { "enabled": True, - "value": {"type": "string", "string_value": "test"}, + "value": {"type": "string", "value": "test"}, }, } @@ -940,14 +940,14 @@ def test_update_existing_segment_override_v2_versioning( "feature": {"name": feature.name}, "environment_default": { "enabled": True, - "value": {"type": "string", "string_value": "default"}, + "value": {"type": "string", "value": "default"}, }, "segment_overrides": [ { "segment_id": segment.id, "priority": 1, # Changed priority from 5 to 1 "enabled": False, # Changed enabled from True to False - "value": {"type": "integer", "string_value": "999"}, + "value": {"type": "integer", "value": "999"}, }, ], } @@ -988,7 +988,7 @@ def test_update_flag_v1_returns_403_without_permission( data = { "feature": {"name": feature.name}, "enabled": True, - "value": {"type": "string", "string_value": "test"}, + "value": {"type": "string", "value": "test"}, } # When @@ -1015,7 +1015,7 @@ def test_update_flag_v2_returns_403_without_permission( "feature": {"name": feature.name}, "environment_default": { "enabled": True, - "value": {"type": "string", "string_value": "test"}, + "value": {"type": "string", "value": "test"}, }, } @@ -1040,7 +1040,7 @@ def test_update_flag_v1_returns_403_for_nonexistent_environment( data = { "feature": {"name": "any_feature"}, "enabled": True, - "value": {"type": "string", "string_value": "test"}, + "value": {"type": "string", "value": "test"}, } # When diff --git a/api/tests/unit/features/versioning/test_unit_versioning_dataclasses.py b/api/tests/unit/features/versioning/test_unit_versioning_dataclasses.py index ab689f9d4f3c..3f35f5c9df19 100644 --- a/api/tests/unit/features/versioning/test_unit_versioning_dataclasses.py +++ b/api/tests/unit/features/versioning/test_unit_versioning_dataclasses.py @@ -2,7 +2,7 @@ import pytest -from features.versioning.dataclasses import Conflict, FlagChangeSet +from features.versioning.dataclasses import AuthorData, Conflict @pytest.mark.parametrize("segment_id, expected_result", ((None, True), (1, False))) @@ -12,34 +12,23 @@ def test_conflict_is_environment_default( assert Conflict(segment_id=segment_id).is_environment_default is expected_result -def test_set_audit_fields_from_request_sets_api_key_for_non_user() -> None: +def test_author_data_from_request_sets_api_key_for_non_user() -> None: mock_api_key = Mock() mock_api_key.key = "test_api_key" mock_request = Mock() mock_request.user = mock_api_key - change_set = FlagChangeSet( - enabled=True, - feature_state_value="test", - type_="string", - ) - change_set.set_audit_fields_from_request(mock_request) + author = AuthorData.from_request(mock_request) - assert change_set.api_key == "test_api_key" - assert change_set.user is None + assert author.api_key == "test_api_key" + assert author.user is None -def test_set_audit_fields_from_request_raises_for_invalid_user() -> None: +def test_author_data_from_request_raises_for_invalid_user() -> None: mock_request = Mock() mock_request.user = object() # No .key attribute - change_set = FlagChangeSet( - enabled=True, - feature_state_value="test", - type_="string", - ) - with pytest.raises(ValueError) as exc_info: - change_set.set_audit_fields_from_request(mock_request) + AuthorData.from_request(mock_request) assert "must be FFAdminUser or have an API key" in str(exc_info.value) From 0d3bfbc64a5a865fb776d315784d1e6876a57592 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Tue, 9 Dec 2025 13:59:01 +0530 Subject: [PATCH 23/26] fix: set environment_feature_version on FeatureSegment for V2 versioning When creating new segment overrides in V2 versioning mode, the FeatureSegment was not being linked to the version. This caused priority ordering to be calculated across all segments with NULL version instead of being scoped to the specific version. --- api/features/versioning/versioning_service.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/api/features/versioning/versioning_service.py b/api/features/versioning/versioning_service.py index 0c9fd078a33e..820699577408 100644 --- a/api/features/versioning/versioning_service.py +++ b/api/features/versioning/versioning_service.py @@ -138,6 +138,7 @@ def _update_flag_for_versioning_v2( feature=feature, segment_id=change_set.segment_id, environment=environment, + environment_feature_version=new_version, ) target_feature_state = FeatureState.objects.create( @@ -242,6 +243,7 @@ def _create_segment_override( feature=feature, segment_id=segment_id, environment=environment, + environment_feature_version=version, ) if priority is not None: From d7e2434a98c11a2b96e4a7c76f6361e348962249 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Tue, 9 Dec 2025 14:13:01 +0530 Subject: [PATCH 24/26] fix: improve validation and test accuracy - Use key existence checks instead of truthiness in FeatureIdentifierSerializer to correctly handle id=0 edge case - Fix test mock to use MasterAPIKey instance instead of string, matching production behavior where api_key is a model instance --- api/features/feature_states/serializers.py | 6 ++++-- .../versioning/test_unit_versioning_dataclasses.py | 9 +++++---- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/api/features/feature_states/serializers.py b/api/features/feature_states/serializers.py index d63bd1fbc075..57afc815df83 100644 --- a/api/features/feature_states/serializers.py +++ b/api/features/feature_states/serializers.py @@ -46,11 +46,13 @@ class FeatureIdentifierSerializer(serializers.Serializer): # type: ignore[type- id = serializers.IntegerField(required=False) def validate(self, data: dict) -> dict: # type: ignore[type-arg] - if not data.get("name") and not data.get("id"): + has_name = "name" in data + has_id = "id" in data + if not has_name and not has_id: raise serializers.ValidationError( "Either 'name' or 'id' is required for feature identification" ) - if data.get("name") and data.get("id"): + if has_name and has_id: raise serializers.ValidationError("Provide either 'name' or 'id', not both") return data diff --git a/api/tests/unit/features/versioning/test_unit_versioning_dataclasses.py b/api/tests/unit/features/versioning/test_unit_versioning_dataclasses.py index 3f35f5c9df19..d169ab28cc13 100644 --- a/api/tests/unit/features/versioning/test_unit_versioning_dataclasses.py +++ b/api/tests/unit/features/versioning/test_unit_versioning_dataclasses.py @@ -13,14 +13,15 @@ def test_conflict_is_environment_default( def test_author_data_from_request_sets_api_key_for_non_user() -> None: - mock_api_key = Mock() - mock_api_key.key = "test_api_key" + mock_master_api_key = Mock(spec_set=["id", "name"]) + mock_api_key_user = Mock() + mock_api_key_user.key = mock_master_api_key mock_request = Mock() - mock_request.user = mock_api_key + mock_request.user = mock_api_key_user author = AuthorData.from_request(mock_request) - assert author.api_key == "test_api_key" + assert author.api_key is mock_master_api_key assert author.user is None From 20e515bee5863f47bf52b7c0623bbe1f87a3badc Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Tue, 9 Dec 2025 15:07:19 +0530 Subject: [PATCH 25/26] fix: use explicit None checks for segment_id instead of truthiness segment_id=0 would bypass validation and incorrectly update the environment default instead of returning an error. Changed all segment_id checks to use 'is not None' or key existence tests. --- api/features/feature_states/serializers.py | 2 +- api/features/versioning/versioning_service.py | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/api/features/feature_states/serializers.py b/api/features/feature_states/serializers.py index 57afc815df83..cbf6f4567587 100644 --- a/api/features/feature_states/serializers.py +++ b/api/features/feature_states/serializers.py @@ -95,7 +95,7 @@ class UpdateFlagSerializer(BaseFeatureUpdateSerializer): value = FeatureValueSerializer(required=True) def validate_segment(self, value: dict) -> dict: # type: ignore[type-arg] - if value and value.get("id"): + if value and "id" in value: self.validate_segment_id(value["id"]) return value diff --git a/api/features/versioning/versioning_service.py b/api/features/versioning/versioning_service.py index 820699577408..82093dc48df3 100644 --- a/api/features/versioning/versioning_service.py +++ b/api/features/versioning/versioning_service.py @@ -127,7 +127,7 @@ def _update_flag_for_versioning_v2( created_by_api_key=change_set.author.api_key, ) - if change_set.segment_id: + if change_set.segment_id is not None: # Segment override - may or may not exist try: target_feature_state: FeatureState = new_version.feature_states.get( @@ -164,7 +164,7 @@ def _update_flag_for_versioning_v2( change_set.type_, ) - if change_set.segment_id and change_set.segment_priority is not None: + if change_set.segment_id is not None and change_set.segment_priority is not None: _update_segment_priority(target_feature_state, change_set.segment_priority) new_version.publish( @@ -180,7 +180,7 @@ def _update_flag_for_versioning_v1( ) -> FeatureState: from features.models import FeatureSegment, FeatureState - if change_set.segment_id: + if change_set.segment_id is not None: additional_filters = Q(feature_segment__segment_id=change_set.segment_id) else: additional_filters = Q(feature_segment__isnull=True, identity_id__isnull=True) @@ -191,7 +191,7 @@ def _update_flag_for_versioning_v1( additional_filters=additional_filters, ) - if len(latest_feature_states) == 0 and change_set.segment_id: + if len(latest_feature_states) == 0 and change_set.segment_id is not None: feature_segment = FeatureSegment.objects.create( feature=feature, segment_id=change_set.segment_id, @@ -216,7 +216,7 @@ def _update_flag_for_versioning_v1( change_set.type_, ) - if change_set.segment_id and change_set.segment_priority is not None: + if change_set.segment_id is not None and change_set.segment_priority is not None: _update_segment_priority(target_feature_state, change_set.segment_priority) return target_feature_state From 6af33d27df0a102d6237b745c5c7cf106c428c95 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Tue, 9 Dec 2025 17:49:27 +0530 Subject: [PATCH 26/26] chore: add TODO for Pydantic TypeAdapter suggestion --- api/features/feature_states/models.py | 1 + 1 file changed, 1 insertion(+) diff --git a/api/features/feature_states/models.py b/api/features/feature_states/models.py index b75c005bbb12..05486661e97e 100644 --- a/api/features/feature_states/models.py +++ b/api/features/feature_states/models.py @@ -11,6 +11,7 @@ STRING, ) +# TODO: use Pydantic TypeAdapter to map serializer data to DTOs FeatureValueType = Literal["string", "integer", "boolean"]