Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#30543 closed Bug (fixed)

admin.E108 is raised on fields accessible only via instance.

Reported by: ajcsimons Owned by: ajcsimons
Component: Core (System checks) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by ajcsimons)

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 = ['number', '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 'number' to 'no_number' 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 []

Attachments (1)

30543.diff (1.6 KB ) - added by Mariusz Felisiak 5 years ago.
fix

Download all attachments as: .zip

Change History (12)

by Mariusz Felisiak, 5 years ago

Attachment: 30543.diff added

fix

comment:1 by Mariusz Felisiak, 5 years ago

Fix is quite simple but a regression test can be tricky.

comment:2 by ajcsimons, 5 years ago

Hi felixxm, I also just made a ticket #30545 with more details. Working through all the logical combinations I think both the old code and new code have other bugs and I've posted a suggested fix there.

comment:3 by ajcsimons, 5 years ago

Description: modified (diff)

Updated description with detail from #30545

comment:4 by ajcsimons, 5 years ago

Description: modified (diff)

comment:5 by ajcsimons, 5 years ago

I think there is a potential simplification of my solution: if get_field raises the FieldDoesNotExist exception then I suspect getattr(obj.model, item) might always fail too (so that test could be omitted and go straight to returning the E108 error), but I'm not sure because the inner workings of get_field are rather complicated.

comment:6 by ajcsimons, 5 years ago

Owner: changed from nobody to ajcsimons
Status: newassigned

comment:7 by ajcsimons, 5 years ago

Has patch: set

My proposed changes as pull request: PR

comment:8 by Mariusz Felisiak, 5 years ago

Needs tests: set

comment:9 by Hasan Ramezani, 5 years ago

Needs tests: unset
Version 0, edited 5 years ago by Hasan Ramezani (next)

comment:10 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In ed668796:

Fixed #30543 -- Fixed checks of ModelAdmin.list_display for fields accessible only via instance.

Co-Authored-By: Andrew Simons <andrewsimons@…>

comment:11 by goblinJoel, 5 years ago

(I found this thread after already finding the commit that caused this problem for me, so this is copied/edited from my comment there. I haven't checked whether this thread's fix works.)

While upgrading from Django 1.11 to 2.2, I found this change causes one of our custom field types to fail system checks when used in an admin's list_display. The dev who made the field had overridden contribute_to_class() to assign a descriptor class that raises an AttributeError on get() if no instance is supplied, making the field attribute only accessible from instances and not from the class itself.

Previously, this still worked, as model._meta.get_field(item) returned true. Now, hasattr() must also be true.

I'm pretty sure I can change our get() to just return the descriptor in that case, as ImageField's descriptor does from 1.10 on, but I thought I'd note this in case there was some reason the behavior I described should be supported.

Last edited 5 years ago by goblinJoel (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top