[Django] #30543: admin.E108 is raised on fields accessible only via instance.
Django
------------------------------------------------+------------------------
Reporter: ajcsimons | Owner: nobody
Type: Bug | Status: new
Component: Core (System checks) | Version: master
Severity: Normal | Keywords:
Triage Stage: Accepted | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
#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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30543>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: ajcsimons | Owner: nobody
Type: Bug | Status: new
Component: Core (System checks) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* Attachment "30543.diff" added.
fix
Django
Reporter: ajcsimons | Owner: nobody
Type: Bug | Status: new
Component: Core (System checks) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by felixxm):
Fix is quite simple but a regression test can be tricky.
--
Ticket URL: <https://code.djangoproject.com/ticket/30543#comment:1>
Django
Reporter: ajcsimons | Owner: nobody
Type: Bug | Status: new
Component: Core (System checks) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by ajcsimons):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30543#comment:2>
Django
Reporter: ajcsimons | Owner: nobody
Type: Bug | Status: new
Component: Core (System checks) | Version: master
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:
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
Reporter: ajcsimons | Owner: nobody
Type: Bug | Status: new
Component: Core (System checks) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by ajcsimons:
Old description:
New description:
@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
--
--
Ticket URL: <https://code.djangoproject.com/ticket/30543#comment:4>
Django
Reporter: ajcsimons | Owner: nobody
Type: Bug | Status: new
Component: Core (System checks) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by ajcsimons):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30543#comment:5>
Django
Reporter: ajcsimons | Owner: ajcsimons
Type: Bug | Status: assigned
Component: Core (System | Version: master
checks) |
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
Changes (by ajcsimons):
* status: new => assigned
* owner: nobody => ajcsimons
--
Ticket URL: <https://code.djangoproject.com/ticket/30543#comment:6>
Django
Reporter: ajcsimons | Owner: ajcsimons
Type: Bug | Status: assigned
Component: Core (System | Version: master
checks) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by ajcsimons):
* has_patch: 0 => 1
Comment:
My proposed changes as pull request:
[https://github.com/django/django/pull/11444/ PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/30543#comment:7>
Django
Reporter: ajcsimons | Owner: ajcsimons
Type: Bug | Status: assigned
Component: Core (System | Version: master
checks) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30543#comment:8>
Django
Reporter: ajcsimons | Owner: ajcsimons
Type: Bug | Status: assigned
Component: Core (System | Version: master
checks) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Hasan Ramezani):
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/30543#comment:9>
Django
Reporter: ajcsimons | Owner: ajcsimons
Component: Core (System | Version: master
checks) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"ed668796f6c7e59ca3f35e9d87d940e043a3eff3" ed668796]:
{{{
#!CommitTicketReference repository=""
revision="ed668796f6c7e59ca3f35e9d87d940e043a3eff3"
Fixed #30543 -- Fixed checks of ModelAdmin.list_display for fields
accessible only via instance.
Co-Authored-By: Andrew Simons <andrew...@bubblegroup.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30543#comment:10>
Django
Reporter: ajcsimons | Owner: ajcsimons
Type: Bug | Status: closed
Component: Core (System | Version: master
checks) |
Severity: Normal | Resolution: fixed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by goblinJoel):
(I found this thread after already finding the commit that caused this
problem for me, so this is copied/edited from
[https://github.com/django/django/commit/47016adbf54b54143d4cf052eeb29fc72d27e6b1#commitcomment-34636977
my comment there])
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
[https://github.com/django/django/commit/9f6b704769ba5bc0daafc25340d3dc28b18d8fb1
from 1.10 on], but I thought I'd note this in case there was some reason
the behavior I described should be supported.
--
Ticket URL: <https://code.djangoproject.com/ticket/30543#comment:11>