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 )
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 , 17 months ago
Description: | modified (diff) |
---|
comment:2 by , 17 months ago
comment:3 by , 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
comment:4 by , 17 months ago
Triage Stage: | Unreviewed → Accepted |
---|
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
follow-up: 6 comment:5 by , 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.
follow-up: 7 comment:6 by , 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!
comment:7 by , 17 months ago
Cc: | 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 , 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 , 11 months ago
Cc: | added |
---|---|
Owner: | changed from | to
Status: | new → assigned |
Hi Natalia Bidart , i created a small [PR]https://github.com/django/django/pull/17535 for this issue
comment:10 by , 11 months ago
Needs documentation: | unset |
---|
comment:11 by , 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 , 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 .
comment:14 by , 3 months ago
Cc: | added |
---|
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!