Opened 17 months ago

Last modified 3 months ago

#34566 assigned Bug

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

Reported by: Mike J Owned by: Krishna2864
Component: contrib.admin Version: 4.2
Severity: Normal Keywords:
Cc: fisadev, Ramiro Morales, Krishna2864, Ülgen Sarıkavak Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by 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

Change History (14)

comment:1 by Mike J, 17 months ago

Description: modified (diff)

comment:2 by Natalia Bidart, 17 months ago

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!

comment:3 by Natalia Bidart, 17 months ago

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
Last edited 17 months ago by Natalia Bidart (previous) (diff)

comment:4 by Natalia Bidart, 17 months ago

Triage Stage: UnreviewedAccepted

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

comment:5 by Mariusz Felisiak, 17 months ago

Needs documentation: set
Needs tests: set

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

in reply to:  5 ; comment:6 by Natalia Bidart, 17 months ago

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!

in reply to:  6 comment:7 by Mariusz Felisiak, 17 months ago

Cc: fisadev Ramiro Morales added

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.

comment:8 by fisadev, 11 months ago

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!

comment:9 by Krishna2864, 11 months ago

Cc: Krishna2864 added
Owner: changed from nobody to Krishna2864
Status: newassigned

Hi Natalia Bidart , i created a small [PR]https://github.com/django/django/pull/17535 for this issue

Last edited 11 months ago by Krishna2864 (previous) (diff)

comment:10 by Krishna2864, 11 months ago

Needs documentation: unset

comment:11 by Natalia Bidart, 10 months ago

Patch needs improvement: set

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

comment:12 by Krishna2864, 10 months ago

Needs tests: unset

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

Last edited 10 months ago by Krishna2864 (previous) (diff)

comment:13 by Natalia Bidart, 10 months ago

PR has code style and test failures that needs fixing.

comment:14 by Ülgen Sarıkavak, 3 months ago

Cc: Ülgen Sarıkavak added
Note: See TracTickets for help on using tickets.
Back to Top