Opened 9 years ago
Closed 9 years ago
#25374 closed Bug (fixed)
Admin system check prevents dynamic ModelAdmin attributes
Reported by: | Malcolm Box | Owned by: | nobody |
---|---|---|---|
Component: | contrib.admin | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Simon Charette | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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.
Change History (7)
comment:1 by , 9 years ago
comment:2 by , 9 years ago
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.
comment:3 by , 9 years ago
Has patch: | set |
---|
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.
comment:4 by , 9 years ago
I'm not sure; it might be a good idea to discuss the idea further on django-developers.
Luke recently 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.
comment:5 by , 9 years ago
Cc: | added |
---|
The main argument I see about running checks against instances instead of subclasses of ModelAdmin
is the fact they are the actual objects 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.
I would also argue that the 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 its 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 bound db.Model
is also part of its string representation.
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.
Edit: It looks like the string representation of ModelAdmin
is not currently used in checks message but it's bound model is referred when it makes sense to.
comment:6 by , 9 years ago
Triage Stage: | Unreviewed → Ready for checkin |
---|
See also discussion on django-users here: https://groups.google.com/forum/#!topic/django-users/lsDP5oUWOsw