#30543: admin.E108 is raised on fields accessible only via instance.
--------------------------------------+------------------------------------
Reporter: ajcsimons | Owner: nobody
Type: Bug | Status: new
Component: Core (System checks) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Description changed by ajcsimons:
Old description:
> 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 []
> }}}
New description:
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 = ['number', '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 'number' to 'no_number' 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/30543#comment:4>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/067.ba0ec15fa4966146eca096a4fb97c1ee%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
