[Django] #25374: Admin system check prevents dynamic ModelAdmin attributes
Django
-------------------------------+--------------------
Reporter: mbox | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
With a ModelAdmin that defines some dynamic attributes via __getattr__,
and uses these in fields or readonly_fields (or list_display etc), the
system check admin.E035 error is raised.
Simple example below - note that in the full use case, the __getattr__ is
part of a baseclass/mixin so it can be reused across multiple ModelAdmins
:
{{{
class ExampleAdmin(models.ModelAdmin):
fields = ['image_thumbnail', 'image', 'another_image',
'another_image_thumbnail']
readonly_fields = ['image_thumbnail', 'another_image_thumbnail']
list_display = ['image_thumbnail']
def __getattr__(self, name):
if name.endswith('_thumbnail'):
def thumbnail_function(obj):
# ... generate appropriate thumbnail method
return thumbnail_function
else:
raise AttributeError
}}}
This code worked fine in 1.6, and it works fine in 1.8/1.9 if the system
check is silenced.
The root cause is that the system check uses the class to verify that
fields exist rather than an instance:
Which means that anything that's defined on an instance of the ModelAdmin
but not on the class itself will fail the check, whereas the code will
work.
According to the documentation, for readonly_fields, the values are
"similar to list_display", and list_display is documented to allow "A
string representing an attribute on the ModelAdmin". However this fails
system check admin.E108
There's two issues raised by this:
* Is it reasonable for system checks to fail when the actual code will
work - or is this *always* a bug in the system check?
* In this specific case, should the admin system check inspect ModelAdmin
instances rather than classes so that the check is closer to the
implementation.
--
Ticket URL: <https://code.djangoproject.com/ticket/25374>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: mbox | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by mbox):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
See also discussion on django-users here:
https://groups.google.com/forum/#!topic/django-users/lsDP5oUWOsw
--
Ticket URL: <https://code.djangoproject.com/ticket/25374#comment:1>
Django
Reporter: mbox | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
So far the system checks are limited to static code analysis. I'm not sure
that validating instances will be feasible as this requires instantiating
the class with the appropriate parameters. We could use mock objects, but
then if the validation encounters such objects, it probably won't work
correctly.
--
Ticket URL: <https://code.djangoproject.com/ticket/25374#comment:2>
Django
Reporter: mbox | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by mbox):
* has_patch: 0 => 1
Comment:
There's a PR with a proposed fix here:
https://github.com/django/django/pull/5256 which implements the checks on
instances.
This shifts the instance creation to before the checks, and then checks
that instance. It's a fairly small change to the code - most of the patch
is fixing up the test suite to match.
Is it a general design goal/feature that system checks can *only* be
static? If so, then a bunch of these admin system checks would need to be
downgraded to warnings because otherwise they block perfectly valid code.
--
Ticket URL: <https://code.djangoproject.com/ticket/25374#comment:3>
Django
Reporter: mbox | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
I'm not sure; it might be a good idea to discuss the idea further on
django-developers.
Luke recently [https://groups.google.com/d/msg/django-
developers/WrnhmTyLHuY/LI-MROj_J1cJ mentioned about admin checks]:
I think the problems with the false negatives for the admin checks may
need to be solved another way - namely by removing them. That's
unfortunate, but I found them tricky in the past - they may prevent common
errors, but they also prevent more advanced ways that you might want to
use the admin.
--
Ticket URL: <https://code.djangoproject.com/ticket/25374#comment:4>
Django
Reporter: mbox | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* cc: charettes (added)
Comment:
The main argument I see about running checks against instance instead of
subclasses of `ModelAdmin` is the fact they are the actual objects that
registered to the site, Django doesn't actually use `ModelAdmin` classes
directly. This would be consistent with how checks are ran against
`db.Field` instance compared to `db.Models` classes.
If you take a look at the admin check methods signatures you can clearly
see most of them can't be performed if they are not provided the
`db.Model` the `ModelAdmin` must be bound to. The model bounding is
something that happens at `ModelAdmin` initialization compared to
`InlineModelAdmin` which is done at class definition. I think it would
make more sense to run these checks against "bound" objects instead which
is `ModelAdmin` instances and `InlineModelAdmin` classes.
I would also argue that string representation of `ModelAdmin` should be
changed to the reflect its full class path and the model it's bound to
instead of its bound model `app_label` and the class name which doesn't
make much sense anyway. Combined with the proposed changes it should make
it easier to identify the check offending `ModelAdmin` instance given the
`db.Model` it's bound to is displayed.
As Tim said It wouldn't hurt writing to the mailing list to get extra
feedback but IMHO we should tentatively accept this ticket as a
cleanup/optimization of the admin checks.
--
Ticket URL: <https://code.djangoproject.com/ticket/25374#comment:5>
Django
Reporter: mbox | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Unreviewed => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/25374#comment:6>
Django
Reporter: mbox | Owner: nobody
Type: Bug | Status: closed
Component: contrib.admin | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"1d8eb0cae57731b481a88dca272b2cb0d645bd8e" 1d8eb0ca]:
{{{
#!CommitTicketReference repository=""
revision="1d8eb0cae57731b481a88dca272b2cb0d645bd8e"
Fixed #25374 -- Made ModelAdmin checks work on instances instead of
classes.
This allows dynamically-generated attributes to be specified in
checked ModelAdmin attributes without triggering errors.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25374#comment:7>