#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
--------------------------------------+------------------------------------
Old description:
> #28490 caused a regression in 47016adbf54b54143d4cf052eeb29fc72d27e6b1,
> i.e.
>
> 1. if hasattr(obj.model, item) returns false then we go straight to the
> else clause which returns the error,
> 2. whereas before the else clause did another check for
> model._meta.get_field(item) and would only return the error if that
> raised a FieldDoesNotExist exception
> 3. So how is it that hasattr(model, item) can return false, but
> model._meta.get_field(item) will return something meaning the check
> should not return an error?
> 4. Answer: the field is a special one which is only valid to access on
> instances of the model (whose ModelAdmin we are verifying) and not the
> model class itself. An example of this is the PositionField from the
> django-positions library (which inherits from djangos
> models.IntegerField):
> {{{
> def __get__(self, instance, owner):
> if instance is None:
> raise AttributeError("%s must be accessed via instance." %
> self.name)
> current, updated = getattr(instance, self.get_cache_name())
> return current if updated is None else updated
> }}}
> 5. So the simplification of the hasattr branch was valid, but the removal
> of the else branch to no longer check get_field doesn't throw before
> returning an error was not a refactor but a functionality altering change
> which makes this method return errors in cases where it used not to.
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 = ['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 []
}}}
--
Comment (by ajcsimons):
Updated description with detail from #30545
--
Ticket URL: <https://code.djangoproject.com/ticket/30543#comment:3>
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.9473083ed0f6b9b75d2306552d4ad019%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
