Admin system check prevents dynamic ModelAdmin attributes

[Malcolm Box]

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:

​https://github.com/django/django/blob/dae81c6ec62a76c1f28745ae3642c2d4a37ce259/django/contrib/admin/sites.py#L106-L110

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.

[Malcolm Box]

See also discussion on django-users here: ​https://groups.google.com/forum/#!topic/django-users/lsDP5oUWOsw

[Tim Graham]

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.

[Malcolm Box]

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.

[Tim Graham]

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.

[Simon Charette]

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.

[Tim Graham <timograham@…>]

In 1d8eb0ca:

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.