Re: [Django] #28312: ModelChoiceIterator uses cached length of queryset.

[Django]

#28312: ModelChoiceIterator uses cached length of queryset.
-+-
 Reporter:  Sjoerd Job Postmus   |Owner:  François
 |  Freitag
 Type:  Bug  |   Status:  closed
Component:  Forms|  Version:  1.8
 Severity:  Normal   |   Resolution:  fixed
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Tim Graham ):

 In [changeset:"d1413c5d703c60dfb9e2a418c79b3e4aed32ffac" d1413c5]:
 {{{
 #!CommitTicketReference repository=""
 revision="d1413c5d703c60dfb9e2a418c79b3e4aed32ffac"
 Refs #28312 -- Added an optimized __bool__() to ModelChoiceIterator.

 COUNT is more expensive than EXISTS; use the latter when possible.
 }}}

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.8c3cdc23a2c87386b876bf5065fabd13%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #28312: ModelChoiceIterator uses cached length of queryset.

[Django]

#28312: ModelChoiceIterator uses cached length of queryset.
-+-
 Reporter:  Sjoerd Job Postmus   |Owner:  François
 |  Freitag
 Type:  Bug  |   Status:  closed
Component:  Forms|  Version:  1.8
 Severity:  Normal   |   Resolution:  fixed
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Tim Graham ):

 * status:  assigned => closed
 * resolution:   => fixed


Comment:

 In [changeset:"3fca95e1ad5b355f7813b98c97a194a30f2ab47b" 3fca95e]:
 {{{
 #!CommitTicketReference repository=""
 revision="3fca95e1ad5b355f7813b98c97a194a30f2ab47b"
 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.
 }}}

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.53fe702b565cab22a6acdccffad8961a%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #28312: ModelChoiceIterator uses cached length of queryset.

[Django]

#28312: ModelChoiceIterator uses cached length of queryset.
-+-
 Reporter:  Sjoerd Job Postmus   |Owner:  François
 |  Freitag
 Type:  Bug  |   Status:  assigned
Component:  Forms|  Version:  1.8
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by François Freitag):

 * has_patch:  0 => 1


Comment:

 [https://github.com/django/django/pull/9891 PR]

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.3570c29ad8f23904d8e2d2ed135ec1ea%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #28312: ModelChoiceIterator uses cached length of queryset.

[Django]

#28312: ModelChoiceIterator uses cached length of queryset.
-+-
 Reporter:  Sjoerd Job Postmus   |Owner:  François
 |  Freitag
 Type:  Bug  |   Status:  assigned
Component:  Forms|  Version:  1.8
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Carlton Gibson):

 * has_patch:  1 => 0


Comment:

 Related discussion on [https://groups.google.com/d/topic/django-
 developers/7UZyY0IX5aw/discussion mailing list]

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.d8d0fbdefa6b94dabe8f958314e764af%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #28312: ModelChoiceIterator uses cached length of queryset.

[Django]

#28312: ModelChoiceIterator uses cached length of queryset.
-+-
 Reporter:  Sjoerd Job Postmus   |Owner:  François
 |  Freitag
 Type:  Bug  |   Status:  assigned
Component:  Forms|  Version:  1.8
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by François Freitag):

 * owner:  Srinivas Reddy Thatiparthy => François Freitag
 * needs_better_patch:  1 => 0


Comment:

 [,https://github.com/django/django/pull/9867 PR]

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.3bc638da7e0da171a879929bedeac3be%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #28312: ModelChoiceIterator uses cached length of queryset.

[Django]

#28312: ModelChoiceIterator uses cached length of queryset.
-+-
 Reporter:  Sjoerd Job Postmus   |Owner:  Srinivas
 |  Reddy Thatiparthy
 Type:  Bug  |   Status:  assigned
Component:  Forms|  Version:  1.8
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Srinivas Reddy Thatiparthy):

 * owner:  nobody => Srinivas Reddy Thatiparthy
 * status:  new => assigned


-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.1e55e8575895ab0173f9d971e1bfe689%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #28312: ModelChoiceIterator uses cached length of queryset.

[Django]

#28312: ModelChoiceIterator uses cached length of queryset.
+
 Reporter:  Sjoerd Job Postmus  |Owner:  nobody
 Type:  Bug |   Status:  new
Component:  Forms   |  Version:  1.8
 Severity:  Normal  |   Resolution:
 Keywords:  | Triage Stage:  Accepted
Has patch:  1   |  Needs documentation:  0
  Needs tests:  0   |  Patch needs improvement:  1
Easy pickings:  0   |UI/UX:  0
+
Changes (by Tim Graham):

 * needs_better_patch:  0 => 1


Comment:

 I've closed my PR, someone else is welcome to continue this.

--
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.0a176fb43635a2322a118fc222384395%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #28312: ModelChoiceIterator uses cached length of queryset.

[Django]

#28312: ModelChoiceIterator uses cached length of queryset.
+
 Reporter:  Sjoerd Job Postmus  |Owner:  nobody
 Type:  Bug |   Status:  new
Component:  Forms   |  Version:  1.8
 Severity:  Normal  |   Resolution:
 Keywords:  | Triage Stage:  Accepted
Has patch:  1   |  Needs documentation:  0
  Needs tests:  0   |  Patch needs improvement:  0
Easy pickings:  0   |UI/UX:  0
+
Changes (by Tim Graham):

 * has_patch:  0 => 1
 * stage:  Unreviewed => Accepted


Comment:

 Yes, I think the current behavior of `__len__()` seems inappropriate.
 [https://github.com/django/django/pull/8649 PR]

 You are correct that 1.8 only receives security/data loss fixes at this
 time. This fix does not qualify for a backport to any stable branch based
 on our [https://docs.djangoproject.com/en/dev/internals/release-process
 /#supported-versions supported versions policy].

--
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.9095fb591e86c30cc5b2f9950817398b%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #28312: ModelChoiceIterator uses cached length of queryset.

[Django]

#28312: ModelChoiceIterator uses cached length of queryset.
+--
 Reporter:  Sjoerd Job Postmus  |Owner:  nobody
 Type:  Bug |   Status:  new
Component:  Forms   |  Version:  1.8
 Severity:  Normal  |   Resolution:
 Keywords:  | Triage Stage:  Unreviewed
Has patch:  0   |  Needs documentation:  0
  Needs tests:  0   |  Patch needs improvement:  0
Easy pickings:  0   |UI/UX:  0
+--

Comment (by Sjoerd Job Postmus):

 Perhaps this can be closed, as (our specific case) is resolved by #27563.
 My apologies.

 Extra context:

 We ran into this problem as our form contained logic as follows:

 {{{
 if len(self.fields['status'].choices) <= 1:
 self.fields.pop('status')
 }}}

 I tested this: count = 1: no field. Add a `Status`, reload page: still no
 field. Turns out the count was being "cached". Since it is a Django 1.8
 project, above fix is not taken into account, and the cached value did
 indeed live longer than the current request. Looking at the changes, I
 expect the behaviour to be correct in Django 2.0.

 Even so: the `ModelChoiceIterator` uses `.all()` or `.iterator()` in the
 `__iter__` method, and thus forces obtaining a fresh value. But `__len__`
 does not do so, and that is an inconsistency that might still qualify as a
 bug.

 Are there any plans to backport #27563 to Django 1.8? I expect not, as
 it's past the "End of mainstream support".

--
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.f321e29552862dac39afe6daad7b3f25%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #28312: ModelChoiceIterator uses cached length of queryset.

[Django]

#28312: ModelChoiceIterator uses cached length of queryset.
+--
 Reporter:  Sjoerd Job Postmus  |Owner:  nobody
 Type:  Bug |   Status:  new
Component:  Forms   |  Version:  1.8
 Severity:  Normal  |   Resolution:
 Keywords:  | Triage Stage:  Unreviewed
Has patch:  0   |  Needs documentation:  0
  Needs tests:  0   |  Patch needs improvement:  0
Easy pickings:  0   |UI/UX:  0
+--

Comment (by Sjoerd Job Postmus):

 The following two tests that show how I would expect the
 `ModelMultipleChoiceField` to work. The first tests `__iter__` (and
 works), the second tests `__len__` (and fails: `AssertionError: 3 != 4`).

 {{{
 diff --git a/tests/model_forms/tests.py b/tests/model_forms/tests.py
 index c85eb2a..3c6241d 100644
 --- a/tests/model_forms/tests.py
 +++ b/tests/model_forms/tests.py
 @@ -1992,6 +1992,27 @@ class ModelMultipleChoiceFieldTests(TestCase):
  form = ArticleCategoriesForm(instance=article)
  self.assertCountEqual(form['categories'].value(), [self.c2.slug,
 self.c3.slug])

 +def test_added_fields_gets_rendered(self):
 +f = forms.ModelMultipleChoiceField(Category.objects.all())
 +self.assertEqual(list(f.choices), [
 +(self.c1.pk, 'Entertainment'),
 +(self.c2.pk, "It's a test"),
 +(self.c3.pk, 'Third')])
 +c4 = Category.objects.create(
 +name="Now you see me", slug="now", url="added")
 +self.assertEqual(list(f.choices), [
 +(self.c1.pk, 'Entertainment'),
 +(self.c2.pk, "It's a test"),
 +(self.c3.pk, 'Third'),
 +(c4.pk, 'Now you see me')])
 +
 +def test_added_fields_gets_counted(self):
 +f = forms.ModelMultipleChoiceField(Category.objects.all())
 +self.assertEqual(len(f.choices), 3)
 +c4 = Category.objects.create(
 +name="Now you see me", slug="now", url="added")
 +self.assertEqual(len(f.choices), 4)
 +

  class ModelOneToOneFieldTests(TestCase):
  def test_modelform_onetoonefield(self):
 }}}

 This means that the "invariant" that `len(f.choices) ==
 len(list(f.choices))` does not hold.

 In particular: note that (currently) the value is calculated just once:
 the first time `__len__` is called. This caching is not limited to 'per
 request', but is cached for the lifetime of the process handling requests.

--
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.61efd7585181757bd7e02a215427c251%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #28312: ModelChoiceIterator uses cached length of queryset.

[Django]

#28312: ModelChoiceIterator uses cached length of queryset.
+--
 Reporter:  Sjoerd Job Postmus  |Owner:  nobody
 Type:  Bug |   Status:  new
Component:  Forms   |  Version:  1.8
 Severity:  Normal  |   Resolution:
 Keywords:  | Triage Stage:  Unreviewed
Has patch:  0   |  Needs documentation:  0
  Needs tests:  0   |  Patch needs improvement:  0
Easy pickings:  0   |UI/UX:  0
+--
Changes (by Tim Graham):

 * component:  Uncategorized => Forms


Comment:

 I don't understand the "bug" aspect to the report. Can you give a test
 case that demonstrates incorrect behavior?

 I understand the point about memory consumption. I guess switching to
 `count()` might be better, although it adds a query and could require some
 adapting of `assertNumQueries()` tests (there is one test in Django's
 `model_forms` tests that breaks in this way).

--
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.ac81b4f8307a1cd5f77463d599d03deb%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

[Django] #28312: ModelChoiceIterator uses cached length of queryset.

[Django]

#28312: ModelChoiceIterator uses cached length of queryset.
--+
   Reporter:  Sjoerd Job Postmus  |  Owner:  nobody
   Type:  Bug | Status:  new
  Component:  Uncategorized   |Version:  1.8
   Severity:  Normal  |   Keywords:
   Triage Stage:  Unreviewed  |  Has patch:  0
Needs documentation:  0   |Needs tests:  0
Patch needs improvement:  0   |  Easy pickings:  0
  UI/UX:  0   |
--+
 In Django 1.8 (but also master), `ModelChoiceIterator` has the following
 implementation:

 {{{
 class ModelChoiceIterator:
 def __init__(self, field):
 self.field = field
 self.queryset = field.queryset

 def __iter__(self):
 if self.field.empty_label is not None:
 yield ("", self.field.empty_label)
 queryset = self.queryset.all()
 # Can't use iterator() when queryset uses prefetch_related()
 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))

 def choice(self, obj):
 return (self.field.prepare_value(obj),
 self.field.label_from_instance(obj))
 }}}

 As can be seen, `__iter__` actually does a `.all()` to make sure it gets
 fresh results. However, `__len__` does not. Also: it currently caches all
 objects inside `self.queryset`, only releasing them again on shutdown
 which might be problematic if there are a lot of results (or even a few
 "large" results).

 Suggested implementation

 {{{
 def __len__(self):
 return self.queryset.count() + (1 if self.field.empty_label is not
 None else 0))
 }}}

--
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/052.73d125547a8a71124fc821e4502de92c%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.