#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 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 
https://groups.google.com/d/msgid/django-updates/067.9473083ed0f6b9b75d2306552d4ad019%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Reply via email to