fix: prevent metadata loss due to bad request (#6172)

Author: gagantrivedi

api/environments/serializers.py

@@ -91,6 +91,12 @@ def validate(self, attrs: dict[str, Any]) -> dict[str, Any]:
         self._validate_required_metadata(organisation, attrs.get("metadata", []))
         return attrs
 
+    def create(self, validated_data: dict[str, Any]) -> Environment:
+        metadata_data = validated_data.pop("metadata", [])
+        environment = super().create(validated_data)  # type: ignore[no-untyped-call]
+        self._update_metadata(environment, metadata_data)
+        return environment  # type: ignore[no-any-return]
+
     def update(
         self, environment: Environment, validated_data: dict[str, Any]
     ) -> Environment:

api/features/serializers.py

@@ -348,6 +348,12 @@ def validate(self, attrs: dict[str, Any]) -> dict[str, Any]:
         self._validate_required_metadata(organisation, attrs.get("metadata", []))
         return attrs
 
+    def create(self, validated_data: dict[str, Any]) -> Feature:
+        metadata_data = validated_data.pop("metadata", [])
+        feature = super().create(validated_data)
+        self._update_metadata(feature, metadata_data)
+        return feature
+
     def update(self, feature: Feature, validated_data: dict[str, Any]) -> Feature:
         metadata = validated_data.pop("metadata", [])
         feature = super().update(feature, validated_data)

api/tests/unit/environments/test_unit_environments_views.py

@@ -914,6 +914,100 @@ def test_update_environment_metadata(  # type: ignore[no-untyped-def]
     ]
 
 
+def test_create_multiple_environments_with_metadata_keeps_metadata_isolated(
+    project: Project,
+    admin_client_new: APIClient,
+    optional_b_environment_metadata_field: MetadataModelField,
+) -> None:
+    """
+    Test that creating multiple environments with metadata keeps metadata properly isolated.
+    This is a regression test for the bug where creating a second environment would remove
+    metadata from the first environment if the user sent the first environment's metadata ID.
+    """
+    # Given
+    url = reverse("api-v1:environments:environment-list")
+
+    # Create first environment with metadata
+    first_environment_data = {
+        "name": "First Environment",
+        "project": project.id,
+        "description": "First environment description",
+        "metadata": [
+            {
+                "model_field": optional_b_environment_metadata_field.id,
+                "field_value": "100",
+            },
+        ],
+    }
+
+    # When - Create first environment
+    first_response = admin_client_new.post(
+        url, data=json.dumps(first_environment_data), content_type="application/json"
+    )
+
+    # Then - First environment should be created successfully
+    assert first_response.status_code == status.HTTP_201_CREATED
+    first_environment_api_key = first_response.json()["api_key"]
+    first_metadata = first_response.json()["metadata"]
+    assert len(first_metadata) == 1
+    assert first_metadata[0]["field_value"] == "100"
+    first_metadata_id = first_metadata[0]["id"]
+
+    # Given - Create second environment
+    second_environment_data = {
+        "name": "Second Environment",
+        "project": project.id,
+        "description": "Second environment description",
+        "metadata": [
+            {
+                "id": first_metadata_id,  # user mistakenly sends existing metadata ID
+                "model_field": optional_b_environment_metadata_field.id,
+                "field_value": "200",
+            },
+        ],
+    }
+
+    # When - Create second environment
+    second_response = admin_client_new.post(
+        url, data=json.dumps(second_environment_data), content_type="application/json"
+    )
+
+    # Then - Second environment should be created successfully
+    assert second_response.status_code == status.HTTP_201_CREATED
+    second_environment_api_key = second_response.json()["api_key"]
+    second_metadata = second_response.json()["metadata"]
+    assert len(second_metadata) == 1
+    assert second_metadata[0]["field_value"] == "200"
+    # The metadata ID should be different (new metadata instance created)
+    assert second_metadata[0]["id"] != first_metadata_id
+
+    # When - Retrieve first environment to verify metadata is still there
+    first_environment_url = reverse(
+        "api-v1:environments:environment-detail", args=[first_environment_api_key]
+    )
+    first_environment_check = admin_client_new.get(first_environment_url)
+
+    # Then - First environment should still have its metadata
+    assert first_environment_check.status_code == status.HTTP_200_OK
+    first_environment_metadata_after = first_environment_check.json()["metadata"]
+    assert len(first_environment_metadata_after) == 1
+    assert first_environment_metadata_after[0]["field_value"] == "100"
+    assert first_environment_metadata_after[0]["id"] == first_metadata_id
+
+    # When - Retrieve second environment to verify metadata
+    second_environment_url = reverse(
+        "api-v1:environments:environment-detail", args=[second_environment_api_key]
+    )
+    second_environment_check = admin_client_new.get(second_environment_url)
+
+    # Then - Second environment should have its own metadata
+    assert second_environment_check.status_code == status.HTTP_200_OK
+    second_environment_metadata_after = second_environment_check.json()["metadata"]
+    assert len(second_environment_metadata_after) == 1
+    assert second_environment_metadata_after[0]["field_value"] == "200"
+    assert second_environment_metadata_after[0]["id"] != first_metadata_id
+
+
 def test_audit_log_entry_created_when_environment_updated(
     environment: Environment, project: Project, admin_client_new: APIClient
 ) -> None:

api/tests/unit/features/test_unit_features_views.py

@@ -4075,3 +4075,95 @@ def test_get_multivariate_options_feature_not_found_responds_404(
 
     # Then
     assert response.status_code == status.HTTP_404_NOT_FOUND
+
+
+def test_create_multiple_features_with_metadata_keeps_metadata_isolated(
+    admin_client_new: APIClient,
+    project: Project,
+    optional_b_feature_metadata_field: MetadataModelField,
+) -> None:
+    """
+    Test that creating multiple features with metadata keeps metadata properly isolated.
+    This is a regression test for the bug where creating a second feature would remove
+    metadata from the first feature if the user sent the first feature's metadata ID.
+    """
+    # Given
+    url = reverse("api-v1:projects:project-features-list", args=[project.id])
+
+    # Create first feature with metadata
+    first_feature_data = {
+        "name": "First Feature",
+        "description": "First feature description",
+        "metadata": [
+            {
+                "model_field": optional_b_feature_metadata_field.id,
+                "field_value": "100",
+            },
+        ],
+    }
+
+    # When - Create first feature
+    first_response = admin_client_new.post(
+        url, data=json.dumps(first_feature_data), content_type="application/json"
+    )
+
+    # Then - First feature should be created successfully
+    assert first_response.status_code == status.HTTP_201_CREATED
+    first_feature_id = first_response.json()["id"]
+    first_metadata = first_response.json()["metadata"]
+    assert len(first_metadata) == 1
+    assert first_metadata[0]["field_value"] == "100"
+    first_metadata_id = first_metadata[0]["id"]
+
+    # Given - Create second feature
+    second_feature_data = {
+        "name": "Second Feature",
+        "description": "Second feature description",
+        "metadata": [
+            {
+                "id": first_metadata_id,  # user mistakenly sends existing metadata ID
+                "model_field": optional_b_feature_metadata_field.id,
+                "field_value": "200",
+            },
+        ],
+    }
+
+    # When - Create second feature
+    second_response = admin_client_new.post(
+        url, data=json.dumps(second_feature_data), content_type="application/json"
+    )
+
+    # Then - Second feature should be created successfully
+    assert second_response.status_code == status.HTTP_201_CREATED
+    second_feature_id = second_response.json()["id"]
+    second_metadata = second_response.json()["metadata"]
+    assert len(second_metadata) == 1
+    assert second_metadata[0]["field_value"] == "200"
+    # The metadata ID should be different (new metadata instance created)
+    assert second_metadata[0]["id"] != first_metadata_id
+
+    # When - Retrieve first feature to verify metadata is still there
+    first_feature_url = reverse(
+        "api-v1:projects:project-features-detail", args=[project.id, first_feature_id]
+    )
+    first_feature_check = admin_client_new.get(first_feature_url)
+
+    # Then - First feature should still have its metadata
+    assert first_feature_check.status_code == status.HTTP_200_OK
+    first_feature_metadata_after = first_feature_check.json()["metadata"]
+    assert len(first_feature_metadata_after) == 1
+    assert first_feature_metadata_after[0]["field_value"] == "100"
+    assert first_feature_metadata_after[0]["id"] == first_metadata_id
+
+    # When - Retrieve second feature to verify metadata
+    second_feature_url = reverse(
+        "api-v1:projects:project-features-detail",
+        args=[project.id, second_feature_id],
+    )
+    second_feature_check = admin_client_new.get(second_feature_url)
+
+    # Then - Second feature should have its own metadata
+    assert second_feature_check.status_code == status.HTTP_200_OK
+    second_feature_metadata_after = second_feature_check.json()["metadata"]
+    assert len(second_feature_metadata_after) == 1
+    assert second_feature_metadata_after[0]["field_value"] == "200"

api/tests/unit/segments/test_unit_segments_views.py

@@ -1523,6 +1523,102 @@ def test_create_segment_with_optional_metadata_returns_201(
     assert response.json()["metadata"][0]["field_value"] == str(field_value)
 
 
+def test_create_multiple_segments_with_metadata_keeps_metadata_isolated(
+    project: Project,
+    admin_client_new: APIClient,
+    optional_b_segment_metadata_field: MetadataModelField,
+) -> None:
+    """
+    Test that creating multiple segments with metadata keeps metadata properly isolated.
+    This is a regression test for the bug where creating a second segment would remove
+    metadata from the first segment if the user sent the first segment's metadata ID.
+    """
+    # Given
+    url = reverse("api-v1:projects:project-segments-list", args=[project.id])
+
+    # Create first segment with metadata
+    first_segment_data = {
+        "name": "First Segment",
+        "description": "First segment description",
+        "project": project.id,
+        "rules": [{"type": "ALL", "rules": [], "conditions": []}],
+        "metadata": [
+            {
+                "model_field": optional_b_segment_metadata_field.id,
+                "field_value": "100",
+            },
+        ],
+    }
+
+    # When - Create first segment
+    first_response = admin_client_new.post(
+        url, data=json.dumps(first_segment_data), content_type="application/json"
+    )
+
+    # Then - First segment should be created successfully
+    assert first_response.status_code == status.HTTP_201_CREATED
+    first_segment_id = first_response.json()["id"]
+    first_metadata = first_response.json()["metadata"]
+    assert len(first_metadata) == 1
+    assert first_metadata[0]["field_value"] == "100"
+    first_metadata_id = first_metadata[0]["id"]
+
+    # Given - Create second segment
+    second_segment_data = {
+        "name": "Second Segment",
+        "description": "Second segment description",
+        "project": project.id,
+        "rules": [{"type": "ALL", "rules": [], "conditions": []}],
+        "metadata": [
+            {
+                "id": first_metadata_id,  # user mistakenly sends existing metadata ID
+                "model_field": optional_b_segment_metadata_field.id,
+                "field_value": "200",
+            },
+        ],
+    }
+
+    # When - Create second segment
+    second_response = admin_client_new.post(
+        url, data=json.dumps(second_segment_data), content_type="application/json"
+    )
+
+    # Then - Second segment should be created successfully
+    assert second_response.status_code == status.HTTP_201_CREATED
+    second_segment_id = second_response.json()["id"]
+    second_metadata = second_response.json()["metadata"]
+    assert len(second_metadata) == 1
+    assert second_metadata[0]["field_value"] == "200"
+    # The metadata ID should be different (new metadata instance created)
+    assert second_metadata[0]["id"] != first_metadata_id
+
+    # When - Retrieve first segment to verify metadata is still there
+    first_segment_url = reverse(
+        "api-v1:projects:project-segments-detail", args=[project.id, first_segment_id]
+    )
+    first_segment_check = admin_client_new.get(first_segment_url)
+
+    # Then - First segment should still have its metadata
+    assert first_segment_check.status_code == status.HTTP_200_OK
+    first_segment_metadata_after = first_segment_check.json()["metadata"]
+    assert len(first_segment_metadata_after) == 1
+    assert first_segment_metadata_after[0]["field_value"] == "100"
+    assert first_segment_metadata_after[0]["id"] == first_metadata_id
+
+    # When - Retrieve second segment to verify metadata
+    second_segment_url = reverse(
+        "api-v1:projects:project-segments-detail", args=[project.id, second_segment_id]
+    )
+    second_segment_check = admin_client_new.get(second_segment_url)
+
+    # Then - Second segment should have its own metadata
+    assert second_segment_check.status_code == status.HTTP_200_OK
+    second_segment_metadata_after = second_segment_check.json()["metadata"]
+    assert len(second_segment_metadata_after) == 1
+    assert second_segment_metadata_after[0]["field_value"] == "200"
+    assert second_segment_metadata_after[0]["id"] != first_metadata_id
+
+
 def test_update_segment_evades_max_conditions_when_whitelisted(
     project: Project,
     admin_client: APIClient,