ModelAdmin get_field_queryset uses related admin ordering, but not related admin querysets.

[Mike J]

The ModelAdmin get_field_queryset function properly finds the related model admin and gets the ordering from that. However, it does not also get the fully annotated queryset which can lead to errors breaking the admin page.

In my case I created my own User admin page and added the user permission count to the queryset as follows:

class UserLocalAdmin(UserAdmin):

    def custom_perm_count(user):
        return user.user_permissions__count
    custom_perm_count.admin_order_field = "user_permissions__count"
    custom_perm_count.short_description = "Permission Count"

    def get_queryset(self, request):
        qs = super().get_queryset(request)
        return qs.annotate(models.Count("user_permissions"))

    list_display = ("username", "email", "is_staff", "is_superuser", custom_perm_count, "is_active","last_login")
    sortable_by = list_display
    ordering = ("-is_active", "-user_permissions__count", "-is_superuser", "username")

Then in a different model I use user as a foreign key titled owner. I also created an admin page for that model.

The issue is the drop down list created for the "owner" foreign key field fetches the admin page ordering, but the un-altered queryset.
This causes a FieldError to be raised and django fails to load the page.

Currently the only option I can see to handle this would be override the get_field_queryset function and allow it to reference the get_queryset function or provide custom ordering.

class ReportFilterAdmin(dj_admin.ModelAdmin):
    form=AdminReportFilterForm

    def get_field_queryset(self, db, db_field, request):
        """
        An override of the original to add the required annotations.

        If the ModelAdmin specifies ordering, the queryset should respect that
        ordering.  Otherwise don't specify the queryset, let the field decide
        (return None in that case).
        """
        related_admin = self.admin_site._registry.get(db_field.remote_field.model)
        if related_admin is not None:
            ordering = related_admin.get_ordering(request)
            if ordering is not None and ordering != ():
                # return db_field.remote_field.model._default_manager.using(db).order_by( # current line in official repo
                return related_admin.get_queryset(request).order_by(
                    *ordering
                )
        return None

I don't know if there are performance issues with my approach, but I believe it achieves more universally expected behavior.

original code: ​https://github.com/django/django/blob/7414704e88d73dafbcfbb85f9bc54cb6111439d3/django/contrib/admin/options.py#L253-L255
Pull Request: ​https://github.com/django/django/pull/16859

[Natalia Bidart]

Hello, could you please provide a small Django project to reproduce the issue? At least, please share all the relevant definitions of your models.py and admin.py for us to try to reproduce. Thank you!

[Natalia Bidart]

Actually I have managed to reproduce this issue. For the readers, these are minimal models.py and admin.py:

• models.py:

  from django.db import models
  from django.contrib.auth import get_user_model
  
  
  User = get_user_model()
  
  
  class Report(models.Model):
      title = models.TextField()
      owner = models.ForeignKey(User, on_delete=models.CASCADE)
  

• admin.py:

  from django.contrib import admin
  from django.contrib.auth import get_user_model
  from django.contrib.auth.admin import UserAdmin
  from django.db import models
  
  from .models import Report
  
  
  User = get_user_model()
  
  
  class UserLocalAdmin(UserAdmin):
      list_display = ("username", "email", "custom_perm_count")
      ordering = ["-user_permissions__count"]
  
      def get_queryset(self, request):
          qs = super().get_queryset(request)
          return qs.annotate(models.Count("user_permissions"))
  
      def custom_perm_count(self, user):
          return user.user_permissions__count
  
      custom_perm_count.admin_order_field = "user_permissions__count"
      custom_perm_count.short_description = "Perm Count"
  
  
  class ReportAdmin(admin.ModelAdmin):
      filter_fields = ['title']
  
  
  admin.site.unregister(User)
  admin.site.register(User, UserLocalAdmin)
  admin.site.register(Report, ReportAdmin)
  

Visiting the Report admin detail page (either for add or modify), will raise:

  File "/home/nessita/fellowship/django/django/db/models/sql/query.py", line 1681, in names_to_path
    raise FieldError(
django.core.exceptions.FieldError: Cannot resolve keyword 'count' into field. Choices are: codename, content_type, content_type_id, group, id, name, user

[Natalia Bidart]

I'm tentatively accepting the bug because I think that the exception raising is an issue. The proposed fix in the ​associated PR (which needs tests and docs) does make sense to me but I would definitely seek for opinions from more experienced contributors.

I believe that a way to solve this in the user code would be to change the ReportAdmin to include formfield_for_foreignkey like this:

    def formfield_for_foreignkey(self, db_field, request, **kwargs):
        formfield = super().formfield_for_foreignkey(db_field, request, **kwargs)
        if db_field.name == "owner":
            related_admin = self.admin_site._registry.get(db_field.remote_field.model)
            formfield.queryset = related_admin.get_queryset(request)
        return formfield

[Mariusz Felisiak]

This would be definitely backward incompatible. I'd start by checking why get_ordering() was added like this in d9330d5be2ee60b208dcab2616eb164ea2e6bf36.

[Natalia Bidart]

Replying to Mariusz Felisiak:

This would be definitely backward incompatible. I'd start by checking why get_ordering() was added like this in d9330d5be2ee60b208dcab2616eb164ea2e6bf36.

Do you have any suggestion about how to check why get_ordering was added the way it was? Shall we contact the contributors? Shall we ask the merger? Thanks!

[Mariusz Felisiak]

Replying to Natalia Bidart:

Replying to Mariusz Felisiak:

This would be definitely backward incompatible. I'd start by checking why get_ordering() was added like this in d9330d5be2ee60b208dcab2616eb164ea2e6bf36.

Do you have any suggestion about how to check why get_ordering was added the way it was? Shall we contact the contributors? Shall we ask the merger? Thanks!

Not sure why, maybe get_queryset() didn't exist then and the default manager was the most reasonable choice, shrug. It's hard to tell. This code hasn't changed in the last 10 years so we should be very careful. We can try to ping Ramiro and Juan.

[fisadev]

Hi! I'm, Juan, the one who added that get_field_queryset override making use of get_ordering ( ​https://github.com/django/django/commit/d9330d5be2ee60b208dcab2616eb164ea2e6bf36 ).

I'm not entirely sure what was the reasoning behind the decision to use get_ordering instead of get_queryset, which AFAIK was already present at that time. Maybe we didn't realize because we were focused on solving the ordering problem, so calling get_ordering was the most obvious solution and we didn't notice we could be using get_queryset? Or maybe it was because of performance reasons or sounded overkill to use get_queryset, as maybe get_queryset tends to get more data than needed just to populate the drop down in the related model admin? But that might be a little too opinionated.

I'm not sure what would be the best today. Sorry!

[Krishna2864]

creating a small patch for this issue

[Natalia Bidart]

Hello Krishna2864, as you know (since you set the flag), the PR lacks tests, were you planning on adding those? Thank you.

[Krishna2864]

Hi Natalia Bidart
I added test-cases for this patch and pushed latest commit here - ​https://github.com/django/django/pull/17535
Thankyou .

[Natalia Bidart]

PR has code style and test failures that needs fixing.

[Antoliny]

Can I work on this ticket?

[Antoliny]

oh..please let me know if you don't work on this ticket. @JasonCalm

[JasonCalm]

I am working on this ticket. Please let me know if you need any assistance. Thank you for your patience