[Django] #34566: ModelAdmin get_field_queryset uses related admin ordering, but not related admin querysets.
Django
admin querysets.
-------------------------------------------+------------------------
Reporter: Sodium-Hydrogen | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 4.2
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------------+------------------------
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]
--
Ticket URL: <https://code.djangoproject.com/ticket/34566>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
admin querysets.
Reporter: Sodium-Hydrogen | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 4.2
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Sodium-Hydrogen:
Old description:
New description:
{{{
class UserLocalAdmin(UserAdmin):
{{{
class ReportFilterAdmin(dj_admin.ModelAdmin):
form=AdminReportFilterForm
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]
--
--
Ticket URL: <https://code.djangoproject.com/ticket/34566#comment:1>
Django
admin querysets.
---------------------------------+--------------------------------------
Reporter: Sodium-Hydrogen | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+--------------------------------------
Comment (by 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!
--
Ticket URL: <https://code.djangoproject.com/ticket/34566#comment:2>
Django
admin querysets.
---------------------------------+--------------------------------------
Reporter: Sodium-Hydrogen | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+--------------------------------------
Comment (by 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(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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34566#comment:3>
Django
admin querysets.
Reporter: Mike J | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 4.2
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Natalia Bidart):
* stage: Unreviewed => Accepted
Comment:
I'm tentatively accepting the bug because I think that the exception
raising is an issue. The proposed fix in the
[https://github.com/django/django/pull/16859 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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34566#comment:4>
Django
admin querysets.
-------------------------------+------------------------------------
Reporter: Mike J | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------
* needs_docs: 0 => 1
* needs_tests: 0 => 1
Comment:
This would be definitely backward incompatible. I'd start by checking why
`get_ordering()` was added like this in
d9330d5be2ee60b208dcab2616eb164ea2e6bf36.
--
Ticket URL: <https://code.djangoproject.com/ticket/34566#comment:5>
Django
admin querysets.
-------------------------------+------------------------------------
Reporter: Mike J | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------
Comment (by Natalia Bidart):
Replying to [comment:5 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!
--
Ticket URL: <https://code.djangoproject.com/ticket/34566#comment:6>
Django
admin querysets.
-------------------------------+------------------------------------
Reporter: Mike J | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------
* cc: fisadev, Ramiro Morales (added)
Comment:
Replying to [comment:6 Natalia Bidart]:
> Replying to [comment:5 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34566#comment:7>
Django
admin querysets.
-------------------------------+------------------------------------
Reporter: Mike J | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------
Comment (by 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!
--
Ticket URL: <https://code.djangoproject.com/ticket/34566#comment:8>
Django
admin querysets.
Reporter: Mike J | Owner: Krishna2864
Type: Bug | Status: assigned
Component: contrib.admin | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Krishna2864):
* cc: Krishna2864 (added)
* owner: nobody => Krishna2864
* status: new => assigned
Comment:
creating a small patch for this issue
--
Ticket URL: <https://code.djangoproject.com/ticket/34566#comment:9>
Django
admin querysets.
-------------------------------+---------------------------------------
Reporter: Mike J | Owner: Krishna2864
Type: Bug | Status: assigned
Component: contrib.admin | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+---------------------------------------
Changes (by Krishna2864):
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/34566#comment:10>
Django
admin querysets.
-------------------------------+---------------------------------------
Reporter: Mike J | Owner: Krishna2864
Type: Bug | Status: assigned
Component: contrib.admin | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+---------------------------------------
* needs_better_patch: 0 => 1
Comment:
Hello Krishna2864, as you know (since you set the flag), the PR lacks
tests, were you planning on adding those? Thank you.
--
Ticket URL: <https://code.djangoproject.com/ticket/34566#comment:11>
Django
admin querysets.
-------------------------------+---------------------------------------
Reporter: Mike J | Owner: Krishna2864
Type: Bug | Status: assigned
Component: contrib.admin | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+---------------------------------------
* needs_tests: 1 => 0
Comment:
Hi Natalia Bidart
I added test-cases for this patch and pushed latest commit Thankyou .
--
Ticket URL: <https://code.djangoproject.com/ticket/34566#comment:12>
Django
admin querysets.
-------------------------------+---------------------------------------
Reporter: Mike J | Owner: Krishna2864
Type: Bug | Status: assigned
Component: contrib.admin | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+---------------------------------------
Comment (by Natalia Bidart):
PR has code style and test failures that needs fixing.
--
Ticket URL: <https://code.djangoproject.com/ticket/34566#comment:13>
Django
admin querysets.
-------------------------------+---------------------------------------
Reporter: Mike J | Owner: Krishna2864
Type: Bug | Status: assigned
Component: contrib.admin | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+---------------------------------------
* cc: Ülgen Sarıkavak (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/34566#comment:14>
Django
admin querysets.
-------------------------------+---------------------------------------
Reporter: Mike J | Owner: Krishna2864
Type: Bug | Status: assigned
Component: contrib.admin | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+---------------------------------------
Can I work on this ticket?
--
Ticket URL: <https://code.djangoproject.com/ticket/34566#comment:15>
Django
admin querysets.
-------------------------------+-------------------------------------
Component: contrib.admin | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+-------------------------------------
* owner: Krishna2864 => JasonCalm
--
Ticket URL: <https://code.djangoproject.com/ticket/34566#comment:16>
Django
admin querysets.
Reporter: Mike J | Owner: Antoliny
Component: contrib.admin | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------
* owner: JasonCalm => Antoliny
--
Ticket URL: <https://code.djangoproject.com/ticket/34566#comment:17>
Django
admin querysets.
Reporter: Mike J | Owner: JasonCalm
Component: contrib.admin | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Antoliny):
* owner: Antoliny => JasonCalm
--
Ticket URL: <https://code.djangoproject.com/ticket/34566#comment:18>
Django
admin querysets.
-------------------------------+-------------------------------------
Reporter: Mike J | Owner: JasonCalm
Type: Bug | Status: assigned
Component: contrib.admin | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+-------------------------------------
oh..please let me know if you don't work on this ticket. @JasonCalm
--
Ticket URL: <https://code.djangoproject.com/ticket/34566#comment:19>
Django
admin querysets.
Reporter: Mike J | Owner: Antoliny
Component: contrib.admin | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------
* owner: JasonCalm => Antoliny
--
Ticket URL: <https://code.djangoproject.com/ticket/34566#comment:20>
Django
admin querysets.
Reporter: Mike J | Owner: JasonCalm
Component: contrib.admin | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by JasonCalm):
* owner: Antoliny => JasonCalm
I'm currently working on this ticket. I'll let you know if I need any
help.
--
Ticket URL: <https://code.djangoproject.com/ticket/34566#comment:21>
Django
admin querysets.
-------------------------------+-------------------------------------
Reporter: Mike J | Owner: JasonCalm
Type: Bug | Status: assigned
Component: contrib.admin | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+-------------------------------------
Replying to [comment:21 JasonCalm]:
> I am working on this ticket. Please let me know if you need any
assistance. Thank you for your patience
Jason, are you still working on this? If not, I'd like to work on this
issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/34566#comment:22>
Django
admin querysets.
-------------------------------+-------------------------------------
Reporter: Mike J | Owner: JasonCalm
Type: Bug | Status: assigned
Component: contrib.admin | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+-------------------------------------
Replying to [comment:22 Antoliny]:
issue.
your patience!
--
Ticket URL: <https://code.djangoproject.com/ticket/34566#comment:23>
Django
admin querysets.
Reporter: Mike J | Owner: Antoliny
Component: contrib.admin | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Antoliny):
* owner: JasonCalm => Antoliny
Ticket URL: <https://code.djangoproject.com/ticket/34566#comment:24>