[Django] #30545: Admin checks for list_display incorrectly reports error if hasattr(model, item) returns false but model._meta.get_field(item) returns something
Django
item) returns false but model._meta.get_field(item) returns something
-----------------------------------------+------------------------
Reporter: ajcsimons | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 2.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 |
-----------------------------------------+------------------------
As part of startup django validates the ModelAdmin's list_display
list/tuple for correctness
(django.admin.contrib.checks._check_list_display). Having upgraded django
from 2.07 to 2.2.1 I found that a ModelAdmin with a list display that used
to pass the checks and work fine in admin now fails validation, preventing
django from starting. A PositionField from the django-positions library
triggers this bug, explanation why follows.
{{{
from django.db import models
from position.Fields import PositionField
class Thing(models.Model)
number = models.IntegerField(default=0)
order = PositionField()
}}}
{{{
from django.contrib import admin
from .models import Thing
@admin.register(Thing)
class ThingAdmin(admin.ModelAdmin)
list_display = ['name', 'order']
}}}
Under 2.2.1 this raises an incorrect admin.E108 message saying "The value
of list_display[1] refers to 'order' which is not a callable...".
Under 2.0.7 django starts up successfully.
If you change 'name' to 'no_name' or 'order' to 'no_order' then the
validation correctly complains about those.
The reason for this bug is commit
https://github.com/django/django/commit/47016adbf54b54143d4cf052eeb29fc72d27e6b1
which was proposed and accepted as a fix for bug
https://code.djangoproject.com/ticket/28490. The problem is while it fixed
that bug it broke the functionality of _check_list_display_item in other
cases. The rationale for that change was that after field=getattr(model,
item) field could be None if item was a descriptor returning None, but
subsequent code incorrectly interpreted field being None as meaning
getattr raised an AttributeError. As this was done **after** trying field
= model._meta.get_field(item) and that failing that meant the validation
error should be returned. However, after the above change if
hasattr(model, item) is false then we no longer even try field =
model._meta.get_field(item) before returning an error. The reason
hasattr(model, item) is false in the case of a PositionField is its
__get__ method throws an exception if called on an instance of the
PositionField class on the Thing model class, rather than a Thing
instance.
For clarity, here are the various logical tests that
_check_list_display_item needs to deal with and the behaviour before the
above change, after it, and the correct behaviour (which my suggested
patch exhibits). Note this is assuming the first 2 tests callable(item)
and hasattr(obj, item) are both false (corresponding to item is an actual
function/lambda rather than string or an attribute of ThingAdmin).
* hasattr(model, item) returns True or False (which is the same as seeing
if getattr(model, item) raises AttributeError)
* model._meta.get_field(item) returns a field or raises FieldDoesNotExist
Get a field from somewhere, could either be from getattr(model,item) if
hasattr was True or from get_field.
* Is that field an instance of ManyToManyField?
* Is that field None? (True in case of bug 28490)
||= hasattr =||= get_field =||= field is None? =||= field ManyToMany?
=||= 2.0 returns =||= 2.2 returns =||= Correct behaviour =||= Comments =||
|| True || ok || False || False || [] || [] || [] || - ||
|| True || ok || False || True || E109 || E109 || E109 || - ||
|| True || ok || True || False || E108 || [] || [] || good bit of 28490
fix, 2.0 was wrong ||
|| True || raises || False || False || [] || [] || [] || - ||
|| True || raises || False || True || E109 || [] || E109 || Another bug
introduced by 28490 fix, fails to check if ManyToMany in get_field raise
case ||
|| True || raises || True || False || E108 || [] || [] || good bit of
28490 fix, 2.0 was wrong ||
|| False || ok || False || False || [] || E108 || [] || bad bit of 28490
fix, bug hit with PositionField ||
|| False || ok || False || True || [] || E108 || E109 || both 2.0 and 2.2
wrong ||
|| False || ok || True || False || [] || E108 || [] || bad 28490 fix ||
|| False || raises || False || False || E108 || E108 || E108 || - ||
|| False || raises || False || True || E108 || E108 || E108 || impossible
condition, we got no field assigned to be a ManyToMany ||
|| False || raises || True || False || E108 || E108 || E108 || impossible
condition, we got no field assigned to be None ||
The following code exhibits the correct behaviour in all cases. The key
changes are there is no longer a check for hasattr(model, item), as that
being false should not prevent us form attempting to get the field via
get_field, and only return an E108 in the case both of them fail. If
either of those means or procuring it are successful then we need to check
if it's a ManyToMany. Whether or not the field is None is irrelevant, and
behaviour is contained within the exception catching blocks that should
cause it instead of signalled through a variable being set to None which
is a source of conflation of different cases.
{{{
def _check_list_display_item(self, obj, item, label):
if callable(item):
return []
elif hasattr(obj, item):
return []
else:
try:
field = obj.model._meta.get_field(item)
except FieldDoesNotExist:
try:
field = getattr(obj.model, item)
except AttributeError:
return [
checks.Error(
"The value of '%s' refers to '%s', which is not a
callable, "
"an attribute of '%s', or an attribute or method
on '%s.%s'." % (
label, item, obj.__class__.__name__,
obj.model._meta.app_label,
obj.model._meta.object_name,
),
obj=obj.__class__,
id='admin.E108',
)
]
if isinstance(field, models.ManyToManyField):
return [
checks.Error(
"The value of '%s' must not be a ManyToManyField." %
label,
obj=obj.__class__,
id='admin.E109',
)
]
return []
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30545>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------+--------------------------------------
Reporter: ajcsimons | Owner: nobody
Type: Bug | Status: closed
Component: contrib.admin | Version: master
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* status: new => closed
* version: 2.2 => master
* resolution: => duplicate
Comment:
Thanks for such detailed description! I've already created ticket for this
issue. We can continue there (you should be able to modify description).
Duplicate of #30543.
--
Ticket URL: <https://code.djangoproject.com/ticket/30545#comment:1>