#28490 closed Bug (fixed)
Descriptors on Models are reported as nonexistent by System Check Framework for ModelAdmin.list_display if they return None
Reported by: | Hunter Richards | Owned by: | nobody |
---|---|---|---|
Component: | contrib.admin | Version: | dev |
Severity: | Normal | Keywords: | admin, descriptor, system, checks, framework, validation |
Cc: | Lucas Wiman | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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
:
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
:
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:
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:
-
django/contrib/admin/checks.py
diff --git a/django/contrib/admin/checks.py b/django/contrib/admin/checks.py index 830a190..b6b49a5 100644
a b class ModelAdminChecks(BaseModelAdminChecks): 592 592 field = model._meta.get_field(item) 593 593 except FieldDoesNotExist: 594 594 try: 595 field =getattr(model, item)595 getattr(model, item) 596 596 except AttributeError: 597 597 field = None 598 else: 599 field = True 598 600 599 601 if field is None: 600 602 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 callinggetattr(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:
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.
Attachments (1)
Change History (8)
by , 7 years ago
Attachment: | modeladmin_none_descriptor_error.diff added |
---|
comment:1 by , 7 years ago
comment:2 by , 7 years ago
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.
comment:3 by , 7 years ago
Triage Stage: | Unreviewed → Accepted |
---|
I think it would be fine to do the simplification but I'd omit the test for a broken descriptor that returns None
.
comment:6 by , 5 years ago
Unfortuantely this fix causes other bugs:
- if hasattr(obj.model, item) returns false then we go straight to the else clause which returns the error,
- 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
- 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?
- 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
- 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.
What's the use case for a descriptor returning
None
?