Opened 12 years ago
Last modified 9 months ago
#18597 assigned Cleanup/optimization
`BaseInlineFormSet` should attempt to get it's queryset from it's instance related manager before falling back to it's model's default manager
Reported by: | Simon Charette | Owned by: | Steven Johnson |
---|---|---|---|
Component: | Forms | Version: | dev |
Severity: | Normal | Keywords: | model formset inline |
Cc: | hugo@…, Florian Demmer | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | yes |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
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 it won't even try to get its needed `queryset` from the related manager.
i.e.
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.
Attachments (1)
Change History (14)
by , 12 years ago
Attachment: | formset-optimisation.diff added |
---|
comment:1 by , 12 years ago
Has patch: | set |
---|---|
Needs tests: | set |
All tests pass on Python 2.7.3rc1 Postgres and Sqlite.
comment:2 by , 12 years ago
Description: | modified (diff) |
---|
comment:3 by , 12 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Type: | Cleanup/optimization → Bug |
I'm not sold on the patch -- it looks suspiciously complex -- but the problem is real.
comment:4 by , 9 years ago
Cc: | added |
---|
comment:5 by , 4 years ago
Transfering my concerns from the duplicate issue. I there is some code cleanup needed before attempting to fix this.
This will at least allow people to extend existing base classes to create custom inlines that can do this ( facilitating the feature in the core framework is secondary)
It is not sufficient to just use parent manager 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.
comment:6 by , 4 years ago
Type: | Bug → Cleanup/optimization |
---|---|
Version: | 1.4 → 3.2 |
comment:7 by , 4 years ago
Patch needs improvement: | set |
---|
comment:8 by , 4 years ago
Patch needs improvement: | unset |
---|
added a PR related to the code cleanup needed to support custom inlines :
https://github.com/django/django/pull/14188
comment:9 by , 4 years ago
Can somebody review my PR I literally didn't change anything apart from moving code around so that we can overwrite functionality
comment:10 by , 4 years ago
For context, this is a blocker for a downstream plugin, see https://github.com/theatlantic/django-nested-admin/issues/76
comment:11 by , 3 years ago
Cc: | added |
---|
comment:12 by , 11 months ago
Needs tests: | unset |
---|---|
Owner: | changed from | to
Status: | new → assigned |
Version: | 3.2 → dev |
Added new pull request with docs/tests:
https://github.com/django/django/pull/17818
comment:13 by , 9 months ago
Needs documentation: | set |
---|---|
Patch needs improvement: | set |
ticket-18597.0.diff