Merge pull request #5028 from Ekluv/ekluv_5004

fix unique=True validation for ChoiceField

Author: jpadilla

rest_framework/utils/field_mapping.py

@@ -123,18 +123,70 @@ def get_field_kwargs(field_name, model_field):
             kwargs['allow_folders'] = model_field.allow_folders
 
     if model_field.choices:
-        # If this model field contains choices, then return early.
-        # Further keyword arguments are not valid.
         kwargs['choices'] = model_field.choices
-        return kwargs
-
-    # Our decimal validation is handled in the field code, not validator code.
-    # (In Django 1.9+ this differs from previous style)
-    if isinstance(model_field, models.DecimalField) and DecimalValidator:
-        validator_kwarg = [
-            validator for validator in validator_kwarg
-            if not isinstance(validator, DecimalValidator)
-        ]
+    else:
+        # Ensure that max_value is passed explicitly as a keyword arg,
+        # rather than as a validator.
+        max_value = next((
+            validator.limit_value for validator in validator_kwarg
+            if isinstance(validator, validators.MaxValueValidator)
+        ), None)
+        if max_value is not None and isinstance(model_field, NUMERIC_FIELD_TYPES):
+            kwargs['max_value'] = max_value
+            validator_kwarg = [
+                validator for validator in validator_kwarg
+                if not isinstance(validator, validators.MaxValueValidator)
+            ]
+
+        # Ensure that max_value is passed explicitly as a keyword arg,
+        # rather than as a validator.
+        min_value = next((
+            validator.limit_value for validator in validator_kwarg
+            if isinstance(validator, validators.MinValueValidator)
+        ), None)
+        if min_value is not None and isinstance(model_field, NUMERIC_FIELD_TYPES):
+            kwargs['min_value'] = min_value
+            validator_kwarg = [
+                validator for validator in validator_kwarg
+                if not isinstance(validator, validators.MinValueValidator)
+            ]
+
+        # URLField does not need to include the URLValidator argument,
+        # as it is explicitly added in.
+        if isinstance(model_field, models.URLField):
+            validator_kwarg = [
+                validator for validator in validator_kwarg
+                if not isinstance(validator, validators.URLValidator)
+            ]
+
+        # EmailField does not need to include the validate_email argument,
+        # as it is explicitly added in.
+        if isinstance(model_field, models.EmailField):
+            validator_kwarg = [
+                validator for validator in validator_kwarg
+                if validator is not validators.validate_email
+            ]
+
+        # SlugField do not need to include the 'validate_slug' argument,
+        if isinstance(model_field, models.SlugField):
+            validator_kwarg = [
+                validator for validator in validator_kwarg
+                if validator is not validators.validate_slug
+            ]
+
+        # IPAddressField do not need to include the 'validate_ipv46_address' argument,
+        if isinstance(model_field, models.GenericIPAddressField):
+            validator_kwarg = [
+                validator for validator in validator_kwarg
+                if validator is not validators.validate_ipv46_address
+            ]
+        # Our decimal validation is handled in the field code, not validator code.
+        # (In Django 1.9+ this differs from previous style)
+        if isinstance(model_field, models.DecimalField) and DecimalValidator:
+            validator_kwarg = [
+                validator for validator in validator_kwarg
+                if not isinstance(validator, DecimalValidator)
+            ]
 
     # Ensure that max_length is passed explicitly as a keyword arg,
     # rather than as a validator.
@@ -160,62 +212,6 @@ def get_field_kwargs(field_name, model_field):
             if not isinstance(validator, validators.MinLengthValidator)
         ]
 
-    # Ensure that max_value is passed explicitly as a keyword arg,
-    # rather than as a validator.
-    max_value = next((
-        validator.limit_value for validator in validator_kwarg
-        if isinstance(validator, validators.MaxValueValidator)
-    ), None)
-    if max_value is not None and isinstance(model_field, NUMERIC_FIELD_TYPES):
-        kwargs['max_value'] = max_value
-        validator_kwarg = [
-            validator for validator in validator_kwarg
-            if not isinstance(validator, validators.MaxValueValidator)
-        ]
-
-    # Ensure that max_value is passed explicitly as a keyword arg,
-    # rather than as a validator.
-    min_value = next((
-        validator.limit_value for validator in validator_kwarg
-        if isinstance(validator, validators.MinValueValidator)
-    ), None)
-    if min_value is not None and isinstance(model_field, NUMERIC_FIELD_TYPES):
-        kwargs['min_value'] = min_value
-        validator_kwarg = [
-            validator for validator in validator_kwarg
-            if not isinstance(validator, validators.MinValueValidator)
-        ]
-
-    # URLField does not need to include the URLValidator argument,
-    # as it is explicitly added in.
-    if isinstance(model_field, models.URLField):
-        validator_kwarg = [
-            validator for validator in validator_kwarg
-            if not isinstance(validator, validators.URLValidator)
-        ]
-
-    # EmailField does not need to include the validate_email argument,
-    # as it is explicitly added in.
-    if isinstance(model_field, models.EmailField):
-        validator_kwarg = [
-            validator for validator in validator_kwarg
-            if validator is not validators.validate_email
-        ]
-
-    # SlugField do not need to include the 'validate_slug' argument,
-    if isinstance(model_field, models.SlugField):
-        validator_kwarg = [
-            validator for validator in validator_kwarg
-            if validator is not validators.validate_slug
-        ]
-
-    # IPAddressField do not need to include the 'validate_ipv46_address' argument,
-    if isinstance(model_field, models.GenericIPAddressField):
-        validator_kwarg = [
-            validator for validator in validator_kwarg
-            if validator is not validators.validate_ipv46_address
-        ]
-
     if getattr(model_field, 'unique', False):
         unique_error_message = model_field.error_messages.get('unique', None)
         if unique_error_message:

tests/test_model_serializer.py

@@ -99,6 +99,15 @@ class Issue3674ChildModel(models.Model):
     value = models.CharField(primary_key=True, max_length=64)
 
 
+class UniqueChoiceModel(models.Model):
+    CHOICES = (
+        ('choice1', 'choice 1'),
+        ('choice2', 'choice 1'),
+    )
+
+    name = models.CharField(max_length=254, unique=True, choices=CHOICES)
+
+
 class TestModelSerializer(TestCase):
     def test_create_method(self):
         class TestSerializer(serializers.ModelSerializer):
@@ -1080,3 +1089,16 @@ class Meta:
         with pytest.raises(AssertionError) as cm:
             TestSerializer(obj).fields
         cm.match(r'readonly_fields')
+
+
+class Test5004UniqueChoiceField(TestCase):
+    def test_unique_choice_field(self):
+        class TestUniqueChoiceSerializer(serializers.ModelSerializer):
+            class Meta:
+                model = UniqueChoiceModel
+                fields = '__all__'
+
+        UniqueChoiceModel.objects.create(name='choice1')
+        serializer = TestUniqueChoiceSerializer(data={'name': 'choice1'})
+        assert not serializer.is_valid()
+        assert serializer.errors == {'name': ['unique choice model with this name already exists.']}