[Django] #18597: `BaseInlineFormSet` should attempt to get it's queryset from it's instance related manager before falling back to it's model's default manager
Django
related manager before falling back to it's model's default manager
--------------------------------------+----------------------------------
Reporter: charettes | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: 1.4
Severity: Normal | Keywords: model formset inline
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+----------------------------------
The newly introduced `prefetch_related` method can be quite handy to avoid
unnecessary queries (thanks Luke :), however it's quite useless when used
with `BaseInlineFormSet` since
[https://github.com/django/django/blob/4a103086d5c67fa4fcc53c106c9fdf644c742dd8/django/forms/models.py#L694
it won't even try to get its needed `queryset` from the related manager].
i.e.
{{{#!python
from django.db import models
class Author(models.Model):
name = models.CharField(max_length=255)
class Book(models.Model):
name = models.CharField(max_length=255)
author = models.ForeignKey(Author, related_name='books')
class Meta:
# Needed because `BaseModelFormSet.get_query_set` wants an ordered
set or issue another query
ordering = ('pk',)
}}}
{{{
In [1]: from django.conf import settings
In [2]: from django.db import connection
In [3]: from library.models import *
In [4]: from django.forms.models import inlineformset_factory
In [5]: a = Author.objects.create(name='David Abram')
In [6]: b1 = Book.objects.create(name='Becoming Animal', author=a)
In [7]: b2 = Book.objects.create(name='The Spell of the Sensuous',
author=a)
In [8]: BookInlineFormSet = inlineformset_factory(Author, Book)
In [9]: settings.DEBUG = True
In [10]: instance = Author.objects.prefetch_related('books').get()
In [11]: BookInlineFormSet(instance=instance)
Out[11]: <django.forms.formsets.BookFormFormSet at 0x3c68d50>
In [12]: print connection.queries
[{u'time': u'0.000', u'sql': u'SELECT "library_author"."id",
"library_author"."name" FROM "library_author"'}, {u'time': u'0.000',
u'sql': u'SELECT "library_book"."id", "library_book"."name",
"library_book"."author_id" FROM "library_book" WHERE
"library_book"."author_id" IN (1) ORDER BY "library_book"."id" ASC'},
{u'time': u'0.000', u'sql': u'SELECT "library_book"."id",
"library_book"."name", "library_book"."author_id" FROM "library_book"
WHERE "library_book"."author_id" = 1 ORDER BY "library_book"."id" ASC'}]
}}}
I guess it's only a matter of time before people start trying to optimize
their formsets (or their `ModelAdmin` by overriding `get_queryset`) and
stumble on this ''limitation''.
I've written a patch (I'll create a pull request) for which I'll write
tests if this optimization is ''Accepted''.
--
Ticket URL: <https://code.djangoproject.com/ticket/18597>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
related manager before falling back to it's model's default manager
Reporter: charettes | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: 1.4
Component: Forms | Resolution:
Severity: Normal | Triage Stage:
Keywords: model formset | Unreviewed
inline | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by charettes):
* needs_better_patch: => 0
* has_patch: 0 => 1
* needs_tests: => 1
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/18597#comment:1>
Django
related manager before falling back to it's model's default manager
Reporter: charettes | Owner: nobody
Cleanup/optimization | Version: 1.4
Component: Forms | Resolution:
Severity: Normal | Triage Stage:
Keywords: model formset | Unreviewed
inline | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Old description:
--
--
Ticket URL: <https://code.djangoproject.com/ticket/18597#comment:2>
Django
related manager before falling back to it's model's default manager
Reporter: charettes | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.4
Severity: Normal | Resolution:
Keywords: model formset inline | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Changes (by aaugustin):
* type: Cleanup/optimization => Bug
* stage: Unreviewed => Accepted
Comment:
I'm not sold on the patch -- it looks suspiciously complex -- but the
problem is real.
--
Ticket URL: <https://code.djangoproject.com/ticket/18597#comment:3>
Django
related manager before falling back to it's model's default manager
Reporter: charettes | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.4
Severity: Normal | Resolution:
Keywords: model formset inline | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by hobarrera):
* cc: hugo@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/18597#comment:4>
Django
related manager before falling back to it's model's default manager
Reporter: Simon Charette | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.4
Severity: Normal | Resolution:
Keywords: model formset inline | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Andreas Galazis):
This would help support prefetching configuration on parent admin's
queryset.
This is not sufficient to support prefetching since there are multiple (I
have found at least 3) instances in inline admin and inline formset where
`self.get_queryset()[i]` paradigm is used.
At least perform some code cleanup?
Even if this issue a won't fix for any reason at least abstract fetching
instance by in index in a `ge_instance_by_index` method so that we can
implement admins that do what we want of top of current base classes. If
this is done then we will be able to overwrite ge_instance_by_index to
return `list(self.get_queryset())[i]` instead of
`self.get_queryset()[i]` on inlines that support prefetching
Also I believe that the logic about preparing queryset in
BaseInlineFormSet should exist in a separate function called
`prepare_queryset`:
{{{
def prepare_queryset(queryset):
if queryset is None:
queryset = self.model._default_manager
if self.instance.pk is not None:
qs = queryset.filter(**{self.fk.name: self.instance})
else:
qs = queryset.none()
return qs
}}}
instead if throwing it `__init__`. Init could just call the above to get
the value . This way filtering wouldn't be enforced in the case of related
manager querysets since we could overwrite the method.
--
Ticket URL: <https://code.djangoproject.com/ticket/18597#comment:5>
Django
related manager before falling back to it's model's default manager
Reporter: Simon Charette | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: 3.2
Severity: Normal | Resolution:
Keywords: model formset inline | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Andreas Galazis):
* version: 1.4 => 3.2
* type: Bug => Cleanup/optimization
--
Ticket URL: <https://code.djangoproject.com/ticket/18597#comment:6>
Django
related manager before falling back to it's model's default manager
Reporter: Simon Charette | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: 3.2
Severity: Normal | Resolution:
Keywords: model formset inline | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Andreas Galazis):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/18597#comment:7>
Django
related manager before falling back to it's model's default manager
Reporter: Simon Charette | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: 3.2
Severity: Normal | Resolution:
Keywords: model formset inline | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Andreas Galazis):
* needs_better_patch: 1 => 0
Comment:
added a PR related to the code cleanup needed to support custom inlines :
https://github.com/django/django/pull/14188
--
Ticket URL: <https://code.djangoproject.com/ticket/18597#comment:8>
Django
related manager before falling back to it's model's default manager
Reporter: Simon Charette | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: 3.2
Severity: Normal | Resolution:
Keywords: model formset inline | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Andreas Galazis):
Can somebody review my PR I literally didn't change anything apart from
moving code around so that we can overwrite functionality
--
Ticket URL: <https://code.djangoproject.com/ticket/18597#comment:9>
Django
related manager before falling back to it's model's default manager
Reporter: Simon Charette | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: 3.2
Severity: Normal | Resolution:
Keywords: model formset inline | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tony Narlock):
For context, this is a blocker for a downstream plugin, see
https://github.com/theatlantic/django-nested-admin/issues/76
--
Ticket URL: <https://code.djangoproject.com/ticket/18597#comment:10>
Django
related manager before falling back to it's model's default manager
Reporter: Simon Charette | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: 3.2
Severity: Normal | Resolution:
Keywords: model formset inline | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Florian Demmer):
* cc: Florian Demmer (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/18597#comment:11>
Django
related manager before falling back to it's model's default manager
Reporter: Simon Charette | Owner: Steven
Type: | Johnson
Cleanup/optimization | Status: assigned
Component: Forms | Version: dev
Severity: Normal | Resolution:
Keywords: model formset | Triage Stage: Accepted
inline |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Steven Johnson):
* owner: nobody => Steven Johnson
* status: new => assigned
* version: 3.2 => dev
* needs_tests: 1 => 0
Comment:
Added new pull request with docs/tests:
https://github.com/django/django/pull/17818
--
Ticket URL: <https://code.djangoproject.com/ticket/18597#comment:12>
Django
related manager before falling back to it's model's default manager
Reporter: Simon Charette | Owner: Steven
Type: | Johnson
Cleanup/optimization | Status: assigned
Component: Forms | Version: dev
Severity: Normal | Resolution:
Keywords: model formset | Triage Stage: Accepted
inline |
Needs tests: 0 | Patch needs improvement: 1
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/18597#comment:13>