Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unique=True validation not working with models.CharField with choices #5004

Closed
5 of 6 tasks
Ekluv opened this issue Mar 18, 2017 · 7 comments
Closed
5 of 6 tasks

unique=True validation not working with models.CharField with choices #5004

Ekluv opened this issue Mar 18, 2017 · 7 comments
Labels
Milestone

Comments

@Ekluv
Copy link
Contributor

Ekluv commented Mar 18, 2017

Checklist

  • I have verified that that issue exists against the master branch of Django REST framework.
  • I have searched for similar issues in both open and closed tickets and cannot find a duplicate.
  • This is not a usage question. (Those should be directed to the discussion group instead.)
  • This cannot be dealt with as a third party library. (We prefer new functionality to be in the form of third party libraries where possible.)
  • I have reduced the issue to the simplest possible case.
  • I have included a failing test as a pull request. (If you are unable to do so we can still accept the issue.)

Steps to reproduce

Lets take a model

MY_CHOICES = (
    ('choice1', _('choice 1')),
    ('choice2', _('choice 1')),
)

class Poll(models.Model):
    form_name = models.CharField('name', max_length=254,
                                 unique=True, choices=MY_CHOICES)

class PollSerializer(serializers.ModelSerializer):
	class Meta:
		model = Poll
		fields = '__all__'

class PollCreateView(generics.CreateAPIView):
	serializer_class = PollSerializer

Then making a POST request with {"form_name": "choice1"} first time returns { "id": 1, "form_name": "choice1" }

then by making same request again with data {"form_name": "choice1"} instead of giving valid error response, django's IntegrityError exception is thrown.

This behaviour is only coming if models.CharField is used with choices and unique=True and not when models.CharField is used without choices

Expected behavior

Valid error response should be returned instead of throwing django's IntegrityError exception
like { "form_name": [ "poll with this name already exists." ] }

Actual behavior

Throwing django's IntegrityError exception when models.CharField is used with choices and unique=True

@Ekluv
Copy link
Contributor Author

Ekluv commented Mar 27, 2017

any update on this ?

@tomchristie
Copy link
Member

tomchristie commented Mar 27, 2017

Confirmed...

>>> from rest_framework.serializers import ModelSerializer
>>> from snippets.models import Poll
>>> class Foo(ModelSerializer):
...     class Meta:
...         model = Poll
...         fields = '__all__'
... 
>>> print(Foo())
Foo():
    id = IntegerField(label='ID', read_only=True)
    form_name = ChoiceField(choices=(('choice1', 'choice 1'), ('choice2', 'choice 1')), label='Name')

uniqueness validation has not been applied to the ChoiceField.

@jpadilla
Copy link
Member

Seems like this is the culprit.

@tomchristie
Copy link
Member

Indeed yup, some careful rejigging will be needed there, since there are some keyword arguments that we don't want to include for ChoiceField.
Right now we're erroneously stripping validators tho, so we want to add just that back in.

@jpadilla
Copy link
Member

@tomchristie just removing that early return seems to work since all the checks below have constraint around the field type already.

PollSerializer():
    id = IntegerField(label='ID', read_only=True)
    form_name = ChoiceField(choices=((u'choice1', u'choice 1'), (u'choice2', u'choice 1')), label='Name', validators=[<UniqueValidator(queryset=Poll.objects.all())>])

@Ekluv
Copy link
Contributor Author

Ekluv commented Mar 27, 2017

@jpadilla: in #5026 just removing early return may create issues. It will also include validation for max_value and min_value which may not be required for choice field.
#5028 take cares of this.
@tomchristie : please give your concerns if any.

@jpadilla
Copy link
Member

Fixed via #5028

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants