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 Aymeric Augustin)

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)

formset-optimisation.diff (1.8 KB ) - added by Simon Charette 12 years ago.
ticket-18597.0.diff

Download all attachments as: .zip

Change History (14)

by Simon Charette, 12 years ago

Attachment: formset-optimisation.diff added

ticket-18597.0.diff

comment:1 by Simon Charette, 12 years ago

Has patch: set
Needs tests: set

All tests pass on Python 2.7.3rc1 Postgres and Sqlite.

Last edited 12 years ago by Simon Charette (previous) (diff)

comment:2 by Aymeric Augustin, 12 years ago

Description: modified (diff)

comment:3 by Aymeric Augustin, 12 years ago

Triage Stage: UnreviewedAccepted
Type: Cleanup/optimizationBug

I'm not sold on the patch -- it looks suspiciously complex -- but the problem is real.

comment:4 by Hugo Osvaldo Barrera, 9 years ago

Cc: hugo@… added

comment:5 by Andreas Galazis, 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.

Last edited 4 years ago by Andreas Galazis (previous) (diff)

comment:6 by Andreas Galazis, 4 years ago

Type: BugCleanup/optimization
Version: 1.43.2

comment:7 by Andreas Galazis, 4 years ago

Patch needs improvement: set

comment:8 by Andreas Galazis, 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 Andreas Galazis, 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 Tony Narlock, 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 Florian Demmer, 3 years ago

Cc: Florian Demmer added

comment:12 by Steven Johnson, 11 months ago

Needs tests: unset
Owner: changed from nobody to Steven Johnson
Status: newassigned
Version: 3.2dev

Added new pull request with docs/tests:
https://github.com/django/django/pull/17818

comment:13 by Mariusz Felisiak, 9 months ago

Needs documentation: set
Patch needs improvement: set
Note: See TracTickets for help on using tickets.
Back to Top