Opened 5 years ago
Closed 5 years ago
#30545 closed Bug (duplicate)
admin.E108 is raised on fields accessible only via instance.
Reported by: | ajcsimons | Owned by: | nobody |
---|---|---|---|
Component: | contrib.admin | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Unreviewed | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
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 []
Change History (1)
comment:1 by , 5 years ago
Resolution: | → duplicate |
---|---|
Status: | new → closed |
Summary: | Admin checks for list_display incorrectly reports error if hasattr(model, item) returns false but model._meta.get_field(item) returns something → admin.E108 is raised on fields accessible only via instance. |
Version: | 2.2 → master |
Thanks for such detailed description! I've already created ticket for this issue. We can continue there (you should be able to modify description).
Duplicate of #30543.