ModelAdmin.raw_id_fields should be included in select_related() by change_view and used by raw id widget

[Yurii Zolot'ko]

Consider model A having many ForeignKey relationships to other models B,C,D...Z (common case in many applications).
ModelAdmin has raw_id_fields optimization option to avoid querying full contents of B...Z tables every time.
However in some cases this optimization is not enough as it still invokes several queries to B... tables to fetch single row from every of them to display __str__ and url of related object next to raw input field when admin's change_view is opened for single object.
So when there are 3 ForeignKeys there will at least 4 queries per request (1 for object itself, 3 for every ForeignKey). When there are 10 ForeignKeys there will be 11 queries per request and so on resulting in very high response time for complex objects even using raw_id_fields.
It looks like this problem is not possible to avoid now. I've tried to override get_queryset() method of ModelAdmin to select_related() raw_id_fields like this:

class ObjectAdmin(admin.ModelAdmin):
    raw_id_fields = (
        'field1', 'field2', ...)

    def get_queryset(self, request):
        qs = admin.ModelAdmin.get_queryset(self, request)
        return qs.select_related(*self.raw_id_fields)

But django.contrib.admin.widgets.ForeignKeyRawIdWidget.label_and_url_for_value() still invokes separate query for every ForeignKey relationship instead of getting it from attribute of already fetched object resulting in high response times of admin interface impossible to improve.

It would be significant performance improvement if ModelAdmin could select_related() raw_id_fields by default and ForeignKeyRawIdWidget wouldn't do extra queries for every related object when all necessary data is already present.

[Tim Graham]

I'm not sure that including raw_id_fields in select_related() will always be a net performance increase, so I'm skeptical about that proposal.

The idea about allowing ForeignKeyRawIdWidget to reuse the queryset seems interesting, however, I think you'll have to provide a patch to convince me that this it's feasible and sufficiently backwards compatible. Feel free to reopen the ticket if you provide that and someone will take another look.

[Jurrian Tromp]

I was about to create a new issue for what I think is the same thing so reopening, I ​also encountered this problem.

As stated the label_and_url_for_value() does a get() for each item. When using the raw_id_fields, this is something you don't want. Especially because it does not work with prefetching. I worked around the bug by subclassing it in my own project and actually did a benchmark:

Benchmark: change view page with 180 inline items with relations:

1. Using no raw_id_field: 1658 queries
2. With raw_id_field and prefetching in Django 3.1: 384 queries
3. Patched raw_id_field and prefetching: 18 queries

This proves that my patched ForeignKeyRawIdWidget considerably reduces queries as they already have been prefetched.
The culprit here is django.contrib.admin.widgets:176
obj = self.rel.model._default_manager.using(self.db).get(**{key: value})

This is a excerpt of what is needed to improve the behaviour which can be backwards compatible: we just need to try if value.pk exists, if not we use the default behaviour:

class ForeignKeyRawIdWidget(widgets.ForeignKeyRawIdWidget):
    def format_value(self, value):
        """Try to return the `pk` if value is an object, otherwise just return
         the value as fallback."""

        if value == '' or value is None:
            return None

        try:
            return str(value.pk)
        except AttributeError:
            return str(value)

    def label_and_url_for_value(self, value):
        """Instead of the original we do not have do a `get()` anymore instead
        access the instance directly so when value is prefetched this will
        prevent additional queries."""

        try:
            pk = value.pk
            meta = value._meta
        except AttributeError:
            # Fallback for compatibility with plain pk values
            return super().label_and_url_for_value(value)

Note that this is what I made in order to fix it in a project. If accepted I can provide a more permanent solution. Please let me know if this of interest.

[Carlton Gibson]

Hi Jurrian

It looks like the code example there isn't complete… 🤔 — Can I ask you to post a sample project somewhere showing the widget in action, so I can properly assess it?

If the value passed to label_and_url_for_value is a model instance then maybe we can avoid the lookup but I need to see how that all fits together to say properly.

Thanks.

[Jurrian Tromp]

You are right, I am sorry. I left out the BoundField part which is indeed elemental. See the attachment for a working example, ModelChoiceField and RawIdWidgetAdminMixin are just there to patch it to make it work in my project, they are not part of this.

The way I see it there is just one way to have the value supplied to label_and_url_for_value to be a model instance:

class BoundField(boundfield.BoundField):
    def value(self):
        if type(self.field.widget) == ForeignKeyRawIdWidget:
            try:
                return getattr(self.form.instance, self.name)
            except AttributeError:
                pass
...

However this feels hacky to me, although I can not find another way to get the model instance except for from the form. When being rendered, the ForeignKeyRawIdWidget has no access to the form anymore, so it seems the only place where we can pass it on is in the BoundField.
What do you think?

[Carlton Gibson]

Hi Jurrian — thanks for the follow-up.

I left out the BoundField part ...

Yes… 🤔 With this it's quite a lot of extra complexity. In particular, pulling the widget type into bound field feels wrong.

I'm going to say wontfix. Maybe it's worth a follow-up on the DevelopersMailingList to see if folks have any thoughts on a way forward. Otherwise though I suspect it's something that may just have to live in your project.