[Django] #28490: Descriptors on Models are reported as nonexistent by System Check Framework for ModelAdmin.list_display if they return None
Django
for ModelAdmin.list_display if they return None
-------------------------------------+-------------------------------------
Reporter: Hunter | Owner: nobody
Richards |
Type: Bug | Status: new
Component: | Version: master
contrib.admin | Keywords: admin, descriptor,
Severity: Normal | system, checks, framework,
Triage Stage: | validation
Unreviewed | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
= Brief Description
The Django ModelAdmin System Checks will erroneously mark as invalid any
reference in `list_display` to a Descriptor on that ModelAdmin's model
that returns `None` when accessed on that model's class, even if the
Descriptor would return non-null values in the resulting ListView.
This error is due to code that subtly conflates a reference to a field
descriptor with the return value of that descriptor. In this ticket, I
give an in-depth explanation of the bug and a short fix, presented inline,
for demonstration purposes, and a larger fix containing an exciting
refactor and a justification for it.
= Example
Given a model with a field that is a Descriptor that returns `None`:
{{{#!python
class DescriptorThatReturnsNone(object):
def __get__(self, obj, objtype):
return None
def __set__(self, obj, value):
pass
class ValidationTestModel(models.Model):
some_other_field = models.CharField(max_length=100)
none_descriptor = DescriptorThatReturnsNone()
}}}
and a ModelAdmin that includes that Descriptor field in `list_display`:
{{{#!python
class TestModelAdmin(ModelAdmin):
list_display = ('some_other_field', 'none_descriptor')
}}}
then the system checks that run at project start will fail to find that
Descriptor attribute with the following error message:
{{{
(admin.E108) The value of 'list_display[4]' refers to 'none_descriptor',
which is not a callable, an attribute of 'TestModelAdmin', or an attribute
or method on 'modeladmin.ValidationTestModel'.
}}}
= Location of Error
The location of the error is in the following code:
{{{#!python
elif hasattr(model, item):
# getattr(model, item) could be an X_RelatedObjectsDescriptor
try:
field = model._meta.get_field(item)
except FieldDoesNotExist:
try:
field = getattr(model, item)
except AttributeError:
field = None
if field is None:
return [
checks.Error([...]
}}}
We follow the branch for `has_attr(model, item)`, and `field =
model._meta.get_field(item)` throws an error, so we call `field =
getattr(model, item)` for the purpose of either getting a reference to
this non-field attribute or its value itself, depending on the type of
`item`. Supposedly, if `getattr` were to throw an error, we'd set `field`
to none and return E108 (more on this below), but in the Descriptor case
above, we don't. But `field` is still `None`, i.e. the return value of
the descriptor, so E108 is triggered anyway, even though the Descriptor
field is a perfectly valid value to display in a ListView.
The cause of the error comes from confusing the "type" of `field`: when
using Descriptors, it is not always the case that `getattr(SomeClass,
some_attr_name)` will return a reference to the field in question and
`getattr(some_class_instance, some_attr_name)` will return the actual
value of the attribute, which is the unstated assumption of the quoted
code. Therefore, `field` sometimes references a field itself, and
sometimes a field's value.
= A Workaround and a Quick Fix
I triggered this error with a slightly more complicated Descriptor that
returned the value of a related field on its declared model, and `None` if
that were not possible. Realizing the special value `None` was at the
heart of the error, as a workaround I rewrote the Descriptor to return the
empty string as an "empty" value instead of `None` which is
indistinguishable from `None` in a ListView.
As an example of how I intend to fix the error, here's The Simplest Thing
that Could Possibly Work:
{{{#!diff
diff --git a/django/contrib/admin/checks.py
b/django/contrib/admin/checks.py
index 830a190..b6b49a5 100644
--- a/django/contrib/admin/checks.py
+++ b/django/contrib/admin/checks.py
@@ -592,9 +592,11 @@ class ModelAdminChecks(BaseModelAdminChecks):
field = model._meta.get_field(item)
except FieldDoesNotExist:
try:
- field = getattr(model, item)
+ getattr(model, item)
except AttributeError:
field = None
+ else:
+ field = True
if field is None:
return [
}}}
In this fix, I discard the return value of `getattr` and just set `field`
based on whether `getattr` succeeded or not. The "type" of `field` is
still a bit confused since `True` isn't a field type, but this fix won't
miss any fields that happen to have the value `None` on classes.
= A Better Fix and a Refactor
But wait! We've rightly discarded the return value of `getattr` above,
but how'd we get to this line of code in the first place? By having
`hasattr(model, item)` be `True`. According to the Python docs, `hasattr`
is implemented using `getattr`:
https://docs.python.org/2/library/functions.html#hasattr
https://docs.python.org/3/library/functions.html#hasattr
> [`hasattr`] is implemented by calling `getattr(object, name)` and seeing
whether it raises an exception or not.
So if we've reached the call to `getattr` above, then it follows by the
definition of `hasattr` that we've already run `getattr` successfully.
If we continue this line of thinking, we realize that `getattr` thus
cannot possibly fail in this branch, and in our new "simple fix" version
above, `item` in this `elif` branch will never refer to an attribute that
cannot be gotten through `getattr` for successful display in the ListView.
We can thus remove the first duplicated instance where E108 is raised, and
thereby simplify the control flow greatly, so that the final version of
this function resembles the following:
{{{#!python
def _check_list_display_item(self, obj, model, item, label):
if callable(item):
return []
elif hasattr(obj, item):
return []
elif hasattr(model, item):
try:
field = model._meta.get_field(item)
except FieldDoesNotExist:
return []
else:
if isinstance(field, models.ManyToManyField):
return [checks.Error([...E109: Disallowed M2Ms...])]
else:
return []
else:
return [checks.Error([...E108...])]
}}}
which coincidentally matches the text of E108 much more closely.
= Submitting a Patch
I've attached a patch version of my changes to this PR for convenience,
but I intend to open a Pull Request on GitHub that will contain all
changes mentioned in this Ticket. Both the patch and the PR will contain
tests recapitulating and then demonstrating the fixing of this error.
--
Ticket URL: <https://code.djangoproject.com/ticket/28490>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
for ModelAdmin.list_display if they return None
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: admin, descriptor, | Triage Stage:
system, checks, framework, | Unreviewed
validation |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "modeladmin_none_descriptor_error.diff" added.
Django
for ModelAdmin.list_display if they return None
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: admin, descriptor, | Triage Stage:
system, checks, framework, | Unreviewed
validation |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
What's the use case for a descriptor returning `None`?
--
Ticket URL: <https://code.djangoproject.com/ticket/28490#comment:1>
Django
for ModelAdmin.list_display if they return None
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: admin, descriptor, | Triage Stage:
system, checks, framework, | Unreviewed
validation |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Hunter Richards):
Thanks for the reply, Tim!
I can't think of a use case for a Descriptor that returns `None` that
triggers the error reported here, but the Descriptor API makes it very
easy to shoot yourself in the foot by writing one, and the Python
Descriptor documentation includes many examples that would trigger this
error, along with no word on having to manually handle the `if obj is
None` case (although the `@property` example is correctly written):
https://docs.python.org/3/howto/descriptor.html
We initially discovered this bug due to such an improperly written
Descriptor, and it took us a long time to track down the reason due to the
triggering of the error message being so deep. We wanted to save other
Djangonauts that time in the future!
Might I also point out that this PR includes a simplification of the
affected Admin Check code, including the removal of a known code
duplication, regardless of the prevalence of `None`-returning Descriptors.
--
Ticket URL: <https://code.djangoproject.com/ticket/28490#comment:2>
Django
for ModelAdmin.list_display if they return None
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
system, checks, framework, |
validation |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
I think it would be fine to do the simplification but I'd omit the test
for a broken descriptor that returns `None`.
--
Ticket URL: <https://code.djangoproject.com/ticket/28490#comment:3>
Django
for ModelAdmin.list_display if they return None
-------------------------------------+-------------------------------------
Component: contrib.admin | Version: master
Severity: Normal | Resolution: fixed
Keywords: admin, descriptor, | Triage Stage: Accepted
system, checks, framework, |
validation |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"47016adbf54b54143d4cf052eeb29fc72d27e6b1" 47016adb]:
{{{
#!CommitTicketReference repository=""
revision="47016adbf54b54143d4cf052eeb29fc72d27e6b1"
Fixed #28490 -- Removed unused code in admin.E108 check.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28490#comment:4>
Django
for ModelAdmin.list_display if they return None
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: contrib.admin | Version: master
Severity: Normal | Resolution: fixed
Keywords: admin, descriptor, | Triage Stage: Accepted
system, checks, framework, |
validation |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Hunter Richards):
Thanks for the help and review, Tim!
--
Ticket URL: <https://code.djangoproject.com/ticket/28490#comment:5>
Django
for ModelAdmin.list_display if they return None
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: contrib.admin | Version: master
Severity: Normal | Resolution: fixed
Keywords: admin, descriptor, | Triage Stage: Accepted
system, checks, framework, |
validation |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by ajcsimons):
Unfortuantely this fix causes other bugs:
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28490#comment:6>
Django
for ModelAdmin.list_display if they return None
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: contrib.admin | Version: master
Severity: Normal | Resolution: fixed
Keywords: admin, descriptor, | Triage Stage: Accepted
system, checks, framework, |
validation |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by felixxm):
Thanks for the report, I added a separate ticket #30543 for this issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/28490#comment:7>