#28490: Descriptors on Models are reported as nonexistent by System Check 
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`:

 class DescriptorThatReturnsNone(object):
     def __get__(self, obj, objtype):
         return None

     def __set__(self, obj, value):

 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
                 field = model._meta.get_field(item)
             except FieldDoesNotExist:
                     field = getattr(model, item)
                 except AttributeError:
                     field = None

             if field is None:
                 return [

 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 --git a/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:
 -                    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`:


 > [`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:

     def _check_list_display_item(self, obj, model, item, label):
         if callable(item):
             return []
         elif hasattr(obj, item):
             return []
         elif hasattr(model, item):
                 field = model._meta.get_field(item)
             except FieldDoesNotExist:
                 return []
                 if isinstance(field, models.ManyToManyField):
                     return [checks.Error([...E109: Disallowed M2Ms...])]
                     return []
             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.

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 django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
For more options, visit https://groups.google.com/d/optout.

Reply via email to