Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#33208 closed New feature (wontfix)

Allow globally defining custom (i.e. with select_related) querysets for ModelChoiceFields/ForeignKeys

Reported by: Matthijs Kooijman Owned by: nobody
Component: contrib.admin Version: 3.2
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

TL;DR: In cases where you have models with a foreignkey, a ModelChoiceField (e.g. using ModelAdmin), and a __str__ method that uses a related field, a lot of queries might be duplicated. There does not currently seem to be a clean way to add select_related in the right place to fix this cleanly.

e.g. consider something like this:

from django.db import models
from django.contrib import admin

class Author(models.Model):
    name = models.CharField(max_length=255)

class Book(models.Model):
    name = models.CharField(max_length=255)
    author = models.ForeignKey(Author, on_delete=models.CASCADE)
    def __str__(self):
        return "{} by {}".format(self.name, self.author.name)

class BookReview(models.Model):
    book = models.ForeignKey(Book, on_delete=models.CASCADE)
    rating = models.IntegerField()

@admin.register(BookReview)
class BookReviewAdmin(admin.ModelAdmin):
    pass

When showing the admin change view for a Review, I get a ton of queries when the ModelChoiceField tries to render all options for the book field. For each Book in the database, it calls __str__, which does a single query to retrieve self.author.name.

The obvious fix for this would be to ensure that the ModelChoiceField queryset uses select_related to prefetch all data. I've found that this is possible by defining formfield_for_foreignkey on (in this case) BookReviewAdmin, e.g. something like:

    def formfield_for_foreignkey(self, db_field, request, **kwargs):
        if db_field.name == "book":
            kwargs['queryset'] = Book.objects.all().select_related('author')
        return super().formfield_for_foreignkey(db_field, request, **kwargs)

This particular implementation has the downside that it bypasses the default queryset processing of ModelAdmin (currently only applying the ordering for the ModelAdmin registered for Book, if any). It would be nicer to override ModelAdmin.get_field_queryset, but that does not seem to be a public API.

In any case, the bigger problem with this approach, is that it has to be applied for *every* ModelAdmin that has a ForeignKey to Book, which leads to duplication. Ideally, and that is the point of this issue, it would be possible to define this select_related business in the Book model or in BookAdmin, so it is used automatically.

I'm not quite sure *how* this would would work and where to put it, though. Conceptually, the select_related is needed because of how __str__ is defined on the model, so defining this in the model would make sense. There is already the concept of different managers that can prepare querysets for different usecases, but I'm not sure if this case would warrant a new manager name. If it would, then when would this manager be used?

Conceptually I guess it's not even linked to the ForeignKey/ModelChoiceField itself, but it would be the "manager to use when you want to call str". Since using ModelChoiceField implies calling __str__, using this custom manager for ModelChoiceField (and/or from ForeignKey.dbfield) would also solve the problem, but might apply the select_related also in cases where it is not really needed (expanding this further, you could of course just add the select_related to the default manager, which would work, but end up selecting too much in a lot of cases).

One pragmatic alternative could be to try and fix this at the admin level instead, where in this case BookAdmin could define something like get_queryset_for_references() or so, and have the BookReviewAdmin.get_field_queryset() use that (similar to how it already applies the ordering from the related field admin). Again, not sure how to exactly define this in an elegant way...

Change History (3)

comment:1 by Mariusz Felisiak, 3 years ago

Component: Uncategorizedcontrib.admin
Resolution: wontfix
Status: newclosed

Thanks for the report, however I don't think a new hook is necessary you can use ModelAdmin.autocomplete_fields to avoid selecting all related instances to display in the dropdown, as documented:

"By default, the admin uses a select-box interface (<select>) for those fields. Sometimes you don’t want to incur the overhead of selecting all the related instances to display in the dropdown. The Select2 input looks similar to the default input but comes with a search feature that loads the options asynchronously. This is faster and more user-friendly if the related model has many instances."

For example:

@admin.register(Book)
class BookAdmin(admin.ModelAdmin):
    search_fields = ['name', 'author__name']

# Register your models here.
@admin.register(BookReview)
class BookReviewAdmin(admin.ModelAdmin):
    autocomplete_fields = ['book']

comment:2 by Carlton Gibson, 3 years ago

Also, there are both established ways of optimising this and customising choice generation (but these are topics for the Using category on the Forum rather than here 🙂)

comment:3 by Matthijs Kooijman, 3 years ago

Thanks for your responses.

Thanks for the report, however I don't think a new hook is necessary you can use ​ModelAdmin.autocomplete_fields to avoid selecting all related instances to display in the dropdown, as documented:

I would not really consider that a solution of this particular problem. It significantly changes UI (which might not be what I want here) which prevents this prefetching from being needed, but IMHO does not really solve the problem itself. But yeah, it does prevent the problem from being a problem, that's true :-)

Also, there are both ​established ways of optimising this and ​customising choice generation (but these are topics for ​the Using category on the Forum rather than here 🙂)

AFAICS those all work on the form end of things, which would be similar to (maybe even more verbose than) the formfield_for_foreignkey()-based approach I suggested above. However, the point of this ticket was to talk about solving this more centrally on the original model side of things.

Having said that, I can see that fixing this is not easy and can be considered out of scope, so closing as wontfix is perfectly acceptable. Also, thanks for the links, I've learned a few new things from them :-)

Note: See TracTickets for help on using tickets.
Back to Top