From 3fca95e1ad5b355f7813b98c97a194a30f2ab47b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Freitag?= Date: Fri, 13 Apr 2018 18:15:22 -0700 Subject: [PATCH] Fixed #28312 -- Made ModelChoiceIterator.__len__() more memory-efficient. Instead of loading all QuerySet results in memory, count the number of entries. This adds an extra query when list() or tuple() is called on the choices (because both call __len__() then __iter__()) but uses less memory since the QuerySet results won't be cached. In most cases, the choices will only be iterated on, meaning that __len__() won't be called and only one query will be executed. --- django/forms/models.py | 7 +++-- tests/model_forms/test_modelchoicefield.py | 30 ++++++++++++++-------- tests/model_forms/tests.py | 2 +- 3 files changed, 25 insertions(+), 14 deletions(-) diff --git a/django/forms/models.py b/django/forms/models.py index d8db4b006a..67006ba390 100644 --- a/django/forms/models.py +++ b/django/forms/models.py @@ -1134,13 +1134,16 @@ class ModelChoiceIterator: yield ("", self.field.empty_label) queryset = self.queryset # Can't use iterator() when queryset uses prefetch_related() - if not queryset._prefetch_related_lookups and queryset._result_cache is None: + if not queryset._prefetch_related_lookups: queryset = queryset.iterator() for obj in queryset: yield self.choice(obj) def __len__(self): - return len(self.queryset) + (1 if self.field.empty_label is not None else 0) + # count() adds a query but uses less memory since the QuerySet results + # won't be cached. In most cases, the choices will only be iterated on, + # and __len__() won't be called. + return self.queryset.count() + (1 if self.field.empty_label is not None else 0) def choice(self, obj): return (self.field.prepare_value(obj), self.field.label_from_instance(obj)) diff --git a/tests/model_forms/test_modelchoicefield.py b/tests/model_forms/test_modelchoicefield.py index 733175823f..975873c7f9 100644 --- a/tests/model_forms/test_modelchoicefield.py +++ b/tests/model_forms/test_modelchoicefield.py @@ -96,6 +96,25 @@ class ModelChoiceFieldTests(TestCase): (self.c3.pk, 'category Third'), ]) + def test_choices_freshness(self): + f = forms.ModelChoiceField(Category.objects.all()) + self.assertEqual(len(f.choices), 4) + self.assertEqual(list(f.choices), [ + ('', '---------'), + (self.c1.pk, 'Entertainment'), + (self.c2.pk, 'A test'), + (self.c3.pk, 'Third'), + ]) + c4 = Category.objects.create(name='Fourth', slug='4th', url='4th') + self.assertEqual(len(f.choices), 5) + self.assertEqual(list(f.choices), [ + ('', '---------'), + (self.c1.pk, 'Entertainment'), + (self.c2.pk, 'A test'), + (self.c3.pk, 'Third'), + (c4.pk, 'Fourth'), + ]) + def test_deepcopies_widget(self): class ModelChoiceForm(forms.Form): category = forms.ModelChoiceField(Category.objects.all()) @@ -257,17 +276,6 @@ class ModelChoiceFieldTests(TestCase): (self.c3.pk, 'Third'), ]) - def test_queryset_result_cache_is_reused(self): - f = forms.ModelChoiceField(Category.objects.all()) - with self.assertNumQueries(1): - # list() calls __len__() and __iter__(); no duplicate queries. - self.assertEqual(list(f.choices), [ - ('', '---------'), - (self.c1.pk, 'Entertainment'), - (self.c2.pk, 'A test'), - (self.c3.pk, 'Third'), - ]) - def test_num_queries(self): """ Widgets that render multiple subwidgets shouldn't make more than one diff --git a/tests/model_forms/tests.py b/tests/model_forms/tests.py index 4276bc68f1..1d6d9efa96 100644 --- a/tests/model_forms/tests.py +++ b/tests/model_forms/tests.py @@ -2345,7 +2345,7 @@ class OtherModelFormTests(TestCase): return ', '.join(c.name for c in obj.colours.all()) field = ColorModelChoiceField(ColourfulItem.objects.prefetch_related('colours')) - with self.assertNumQueries(2): # would be 3 if prefetch is ignored + with self.assertNumQueries(3): # would be 4 if prefetch is ignored self.assertEqual(tuple(field.choices), ( ('', '---------'), (multicolor_item.pk, 'blue, red'),