cleanup

Author: gagantrivedi

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
@@ -22,37 +19,34 @@ def get_feature(self) -> Feature:
         if not environment:
             raise serializers.ValidationError("Environment context is required")
 
+        # FeatureIdentifierSerializer validates exactly one of id/name is present
+        feature_id = feature_data.get("id")
+        feature_name = feature_data.get("name")
+
         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"))
+            if feature_id is not None:
+                feature: Feature = Feature.objects.get(
+                    project_id=environment.project_id, id=feature_id
+                )
+                return feature
+            feature = Feature.objects.get(
+                project_id=environment.project_id, name=feature_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(
-                f"Feature with identifier '{identifier}' not found in project"
+                f"Feature with identifier '{feature_id or feature_name}' not found in project"
             )
 
-    def _add_audit_fields(
-        self, change_set: typing.Union[FlagChangeSet, FlagChangeSetV2]
-    ) -> None:
+    def _add_audit_fields(self, change_set: 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 +71,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,7 +105,7 @@ 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,
@@ -115,9 +114,9 @@ def flag_change_set(self) -> FlagChangeSet:
         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()
+    def save(self, **kwargs: object) -> FeatureState:
+        environment: Environment = kwargs["environment"]  # type: ignore[assignment]
+        feature = self.get_feature()
 
         return update_flag(environment, feature, self.flag_change_set)
 
@@ -170,24 +169,24 @@ 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"),
             )
             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_value=env_value_data["string_value"],
             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()
+    def save(self, **kwargs: object) -> dict:  # type: ignore[type-arg]
+        environment: Environment = kwargs["environment"]  # type: ignore[assignment]
+        feature = self.get_feature()
 
         return update_flag_v2(environment, feature, self.change_set_v2)

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,11 +68,7 @@ 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)
@@ -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,11 +133,7 @@ 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)

api/features/versioning/dataclasses.py

@@ -1,11 +1,12 @@
-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 users.models import FFAdminUser
 
 
@@ -22,14 +23,14 @@ 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)
 
 
 @dataclass
 class FlagChangeSet(AuditFieldsMixin):
     enabled: bool
-    feature_state_value: typing.Any
+    feature_state_value: str
     type_: str
 
     segment_id: int | None = None
@@ -40,15 +41,15 @@ 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
 
 
 @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)

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()

api/tests/unit/features/feature_states/test_serializers.py

@@ -69,12 +69,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)

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,
 )
@@ -285,9 +285,6 @@ def test_update_flag_error_when_feature_not_found_by_id(
     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")),
@@ -598,23 +595,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 +657,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 +710,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,