From 41260fb93165a9fa18dba31b4766c8be50b430cb Mon Sep 17 00:00:00 2001 From: Jacob Kaplan-Moss Date: Sat, 18 Apr 2009 15:51:11 +0000 Subject: [PATCH] Fixed #10156: `ModelMultipleChoiceField.clean` now does a single query instead of O(N). Thanks, Alex Gaynor. Also, I ported a few more doctests to unittests. git-svn-id: http://code.djangoproject.com/svn/django/trunk@10582 bcc190cf-cafb-0310-a4f2-bffc1f526a37 --- django/forms/models.py | 18 +++--- .../model_forms_regress/models.py | 37 +---------- .../model_forms_regress/tests.py | 61 +++++++++++++++++++ 3 files changed, 73 insertions(+), 43 deletions(-) create mode 100644 tests/regressiontests/model_forms_regress/tests.py diff --git a/django/forms/models.py b/django/forms/models.py index de5f1abb45..95fc41e77f 100644 --- a/django/forms/models.py +++ b/django/forms/models.py @@ -794,14 +794,14 @@ class ModelMultipleChoiceField(ModelChoiceField): return [] if not isinstance(value, (list, tuple)): raise ValidationError(self.error_messages['list']) - final_values = [] - for val in value: + for pk in value: try: - obj = self.queryset.get(pk=val) - except self.queryset.model.DoesNotExist: - raise ValidationError(self.error_messages['invalid_choice'] % val) + self.queryset.filter(pk=pk) except ValueError: - raise ValidationError(self.error_messages['invalid_pk_value'] % val) - else: - final_values.append(obj) - return final_values + raise ValidationError(self.error_messages['invalid_pk_value'] % pk) + qs = self.queryset.filter(pk__in=value) + pks = set([force_unicode(o.pk) for o in qs]) + for val in value: + if force_unicode(val) not in pks: + raise ValidationError(self.error_messages['invalid_choice'] % val) + return qs diff --git a/tests/regressiontests/model_forms_regress/models.py b/tests/regressiontests/model_forms_regress/models.py index a8bd0a3fff..50a2d61531 100644 --- a/tests/regressiontests/model_forms_regress/models.py +++ b/tests/regressiontests/model_forms_regress/models.py @@ -1,47 +1,16 @@ import os from django.db import models -from django import forms + +class Person(models.Model): + name = models.CharField(max_length=100) class Triple(models.Model): left = models.IntegerField() middle = models.IntegerField() right = models.IntegerField() - def __unicode__(self): - return u"%d, %d, %d" % (self.left, self.middle, self.right) - class Meta: unique_together = (('left', 'middle'), (u'middle', u'right')) class FilePathModel(models.Model): path = models.FilePathField(path=os.path.dirname(__file__), match=".*\.py$", blank=True) - -__test__ = {'API_TESTS': """ -When the same field is involved in multiple unique_together constraints, we -need to make sure we don't remove the data for it before doing all the -validation checking (not just failing after the first one). - ->>> _ = Triple.objects.create(left=1, middle=2, right=3) ->>> class TripleForm(forms.ModelForm): -... class Meta: -... model = Triple - ->>> form = TripleForm({'left': '1', 'middle': '2', 'right': '3'}) ->>> form.is_valid() -False ->>> form = TripleForm({'left': '1', 'middle': '3', 'right': '1'}) ->>> form.is_valid() -True - -# Regression test for #8842: FilePathField(blank=True) ->>> class FPForm(forms.ModelForm): -... class Meta: -... model = FilePathModel - ->>> form = FPForm() ->>> names = [c[1] for c in form['path'].field.choices] ->>> names.sort() ->>> names -['---------', '__init__.py', 'models.py'] -"""} - diff --git a/tests/regressiontests/model_forms_regress/tests.py b/tests/regressiontests/model_forms_regress/tests.py new file mode 100644 index 0000000000..6547367812 --- /dev/null +++ b/tests/regressiontests/model_forms_regress/tests.py @@ -0,0 +1,61 @@ +from django import db +from django import forms +from django.conf import settings +from django.test import TestCase +from models import Person, Triple, FilePathModel + +class ModelMultipleChoiceFieldTests(TestCase): + + def setUp(self): + self.old_debug = settings.DEBUG + settings.DEBUG = True + + def tearDown(self): + settings.DEBUG = self.old_debug + + def test_model_multiple_choice_number_of_queries(self): + """ + Test that ModelMultipleChoiceField does O(1) queries instead of + O(n) (#10156). + """ + for i in range(30): + Person.objects.create(name="Person %s" % i) + + db.reset_queries() + f = forms.ModelMultipleChoiceField(queryset=Person.objects.all()) + selected = f.clean([1, 3, 5, 7, 9]) + self.assertEquals(len(db.connection.queries), 1) + +class TripleForm(forms.ModelForm): + class Meta: + model = Triple + +class UniqueTogetherTests(TestCase): + def test_multiple_field_unique_together(self): + """ + When the same field is involved in multiple unique_together + constraints, we need to make sure we don't remove the data for it + before doing all the validation checking (not just failing after + the first one). + """ + Triple.objects.create(left=1, middle=2, right=3) + + form = TripleForm({'left': '1', 'middle': '2', 'right': '3'}) + self.failIf(form.is_valid()) + + form = TripleForm({'left': '1', 'middle': '3', 'right': '1'}) + self.failUnless(form.is_valid()) + +class FPForm(forms.ModelForm): + class Meta: + model = FilePathModel + +class FilePathFieldTests(TestCase): + def test_file_path_field_blank(self): + """ + Regression test for #8842: FilePathField(blank=True) + """ + form = FPForm() + names = [p[1] for p in form['path'].field.choices] + names.sort() + self.assertEqual(names, ['---------', '__init__.py', 'models.py', 'tests.py'])