diff --git a/api/features/serializers.py b/api/features/serializers.py index 87d120109a43..b124d58e7b0a 100644 --- a/api/features/serializers.py +++ b/api/features/serializers.py @@ -11,7 +11,7 @@ FeatureStateValueSerializer, ) from common.projects.permissions import VIEW_PROJECT -from django.db import models +from django.db import IntegrityError, models from drf_spectacular.utils import extend_schema_field from drf_writable_nested import ( # type: ignore[attr-defined] WritableNestedModelSerializer, @@ -58,6 +58,11 @@ from .multivariate.models import MultivariateFeatureOption from .multivariate.serializers import NestedMultivariateFeatureOptionSerializer +DUPLICATE_FEATURE_NAME_ERROR = ( + "Feature with that name already exists for this project. Note that feature " + "names are case insensitive." +) + class FeatureStateSerializerSmall(serializers.ModelSerializer): # type: ignore[type-arg] feature_state_value = serializers.SerializerMethodField() @@ -275,7 +280,14 @@ def create(self, validated_data: dict) -> Feature: # type: ignore[type-arg] group_owners: list[UserPermissionGroup] = validated_data.pop("group_owners", []) user = validated_data.pop("user", None) - instance = super(CreateFeatureSerializer, self).create(validated_data) # type: ignore[no-untyped-call] + try: + instance = super(CreateFeatureSerializer, self).create(validated_data) # type: ignore[no-untyped-call] + except IntegrityError: + # A concurrent request may create a feature with the same name after + # `validate_name` has run but before this insert, tripping the + # case-insensitive unique constraint. Surface this as a 400 rather + # than letting the IntegrityError bubble up as an HTTP 500. + raise serializers.ValidationError({"name": [DUPLICATE_FEATURE_NAME_ERROR]}) if owners: instance.owners.add(*owners) @@ -369,11 +381,7 @@ def validate_name(self, name: str): # type: ignore[no-untyped-def] ) if existing_feature_queryset.exists(): - raise serializers.ValidationError( - "Feature with that name already exists for this " - "project. Note that feature names are case " - "insensitive." - ) + raise serializers.ValidationError(DUPLICATE_FEATURE_NAME_ERROR) return name diff --git a/api/tests/unit/features/test_unit_features_views.py b/api/tests/unit/features/test_unit_features_views.py index 7cc60b0bd522..b6574f46c6df 100644 --- a/api/tests/unit/features/test_unit_features_views.py +++ b/api/tests/unit/features/test_unit_features_views.py @@ -1806,6 +1806,35 @@ def test_create_feature__name_does_not_match_regex__returns_400( ) +def test_create_feature__name_race_condition__returns_400( + admin_client_new: APIClient, + project: Project, + feature: Feature, + mocker: MockerFixture, +) -> None: + # Given + # Simulate a race where `validate_name` passes (e.g. a concurrent request + # creates the feature after validation runs) but the database's + # case-insensitive unique constraint rejects the duplicate name on insert. + mocker.patch( + "features.serializers.CreateFeatureSerializer.validate_name", + side_effect=lambda name: name, + ) + + url = reverse("api-v1:projects:project-features-list", args=[project.id]) + data = {"name": feature.name, "type": STANDARD, "project": project.id} + + # When + response = admin_client_new.post(url, data=data) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.json()["name"][0] == ( + "Feature with that name already exists for this project. Note that " + "feature names are case insensitive." + ) + + def test_create_feature__valid_data__creates_audit_log( admin_client_new: APIClient, project: Project,