Ticket #5374: modeladmin_validation2.diff

django/core/management/validation.py

@@ -143 +143 @@
                     if r.get_accessor_name() == rel_query_name:
                         e.add(opts, "Reverse query name for m2m field '%s' clashes with related field '%s.%s'. Add a related_name argument to the definition for '%s'." % (f.name, rel_opts.object_name, r.get_accessor_name(), f.name))
 
-        # Check admin attribute.
-        if opts.admin is not None:
-            # prepopulated_fields
-            if not isinstance(opts.admin.prepopulated_fields, dict):
-                e.add(opts, '"%s": prepopulated_fields should be a dictionary.' % f.name)
-            else:
-                for field_name, field_list in opts.admin.prepopulated_fields.items():
-                    if not isinstance(field_list, (list, tuple)):
-                        e.add(opts, '"%s": prepopulated_fields "%s" value should be a list or tuple.' % (f.name, field_name))
-
-            # list_display
-            if not isinstance(opts.admin.list_display, (list, tuple)):
-                e.add(opts, '"admin.list_display", if given, must be set to a list or tuple.')
-            else:
-                for fn in opts.admin.list_display:
-                    try:
-                        f = opts.get_field(fn)
-                    except models.FieldDoesNotExist:
-                        if not hasattr(cls, fn):
-                            e.add(opts, '"admin.list_display" refers to %r, which isn\'t an attribute, method or property.' % fn)
-                    else:
-                        if isinstance(f, models.ManyToManyField):
-                            e.add(opts, '"admin.list_display" doesn\'t support ManyToManyFields (%r).' % fn)
-            # list_display_links
-            if opts.admin.list_display_links and not opts.admin.list_display:
-                e.add(opts, '"admin.list_display" must be defined for "admin.list_display_links" to be used.')
-            if not isinstance(opts.admin.list_display_links, (list, tuple)):
-                e.add(opts, '"admin.list_display_links", if given, must be set to a list or tuple.')
-            else:
-                for fn in opts.admin.list_display_links:
-                    try:
-                        f = opts.get_field(fn)
-                    except models.FieldDoesNotExist:
-                        if not hasattr(cls, fn):
-                            e.add(opts, '"admin.list_display_links" refers to %r, which isn\'t an attribute, method or property.' % fn)
-                    if fn not in opts.admin.list_display:
-                        e.add(opts, '"admin.list_display_links" refers to %r, which is not defined in "admin.list_display".' % fn)
-            # list_filter
-            if not isinstance(opts.admin.list_filter, (list, tuple)):
-                e.add(opts, '"admin.list_filter", if given, must be set to a list or tuple.')
-            else:
-                for fn in opts.admin.list_filter:
-                    try:
-                        f = opts.get_field(fn)
-                    except models.FieldDoesNotExist:
-                        e.add(opts, '"admin.list_filter" refers to %r, which isn\'t a field.' % fn)
-            # date_hierarchy
-            if opts.admin.date_hierarchy:
-                try:
-                    f = opts.get_field(opts.admin.date_hierarchy)
-                except models.FieldDoesNotExist:
-                    e.add(opts, '"admin.date_hierarchy" refers to %r, which isn\'t a field.' % opts.admin.date_hierarchy)
-
         # Check ordering attribute.
         if opts.ordering:
             for field_name in opts.ordering:
@@ -213 +160 @@
                 except models.FieldDoesNotExist:
                     e.add(opts, '"ordering" refers to "%s", a field that doesn\'t exist.' % field_name)
 
-        # Check core=True, if needed.
-        for related in opts.get_followed_related_objects():
-            if not related.edit_inline:
-                continue
-            try:
-                for f in related.opts.fields:
-                    if f.core:
-                        raise StopIteration
-                e.add(related.opts, "At least one field in %s should have core=True, because it's being edited inline by %s.%s." % (related.opts.object_name, opts.module_name, opts.object_name))
-            except StopIteration:
-                pass
-
         # Check unique_together.
         for ut in opts.unique_together:
             for field_name in ut:

django/contrib/admin/validation.py

@@ +1 @@
+# Possible improvements: gather all errors and raise in the end instead of
+# bailing out on the first one (like core/management/validation.py does)
+# ---
+# Another issue brought up by jkocherhans:
+# <jkocherhans> mrts: we're probably not going to be able to design a nice
+# system for validation before 1.0, so a function that does the validation is
+# probably fine... no need to over-engieer anything or provide hook for custom
+# classes to allow their own validation.
+# <mrts> it doesn't hurt though...
+# <jkocherhans> I'd argue that it *does* hurt though, cause then we're
+# committing to an api even if it isn't documented, people will use it
+# <mrts> should I remove the calls to _call_validation_hook(cls, model) then?
+# <jkocherhans> mrts: I'd say yes, but you should probably get a second opinion
+
+from django.core.exceptions import ImproperlyConfigured
+from django.db import models
+from django.newforms.forms import BaseForm
+from django.newforms.formsets import BaseFormSet
+from django.contrib.admin.options import flatten_fieldsets, BaseModelAdmin
+from django.contrib.admin.options import HORIZONTAL, VERTICAL
+
+def validate(cls, model):
+    """
+    Does basic ModelAdmin option validation. Calls custom validation
+    classmethod in the end if it is provided in cls. The signature of the
+    custom validation classmethod should be: def validate(cls, model).
+    """
+    opts = model._meta
+    _validate_base(cls, model)
+
+    # currying is expensive, use wrappers instead
+    def _check_istuplew(label, obj):
+        _check_istuple(cls, label, obj)
+
+    def _check_isdictw(label, obj):
+        _check_isdict(cls, label, obj)
+
+    def _check_field_existsw(label, field):
+        return _check_field_exists(cls, model, opts, label, field)
+
+    def _check_attr_existsw(label, field):
+        _check_attr_exists(cls, model, opts, label, field)
+
+    # list_display
+    if not cls.list_display:
+        raise ImproperlyConfigured("%s.list_display can not be empty." %
+                cls.__name__)
+    _check_istuplew('list_display', cls.list_display)
+    for idx, field in enumerate(cls.list_display):
+        f = _check_attr_existsw("list_display['%d']" % idx, field)
+        if isinstance(f, models.ManyToManyField):
+            raise ImproperlyConfigured("`%s.list_display['%d']`, `%s` is a "
+                    "ManyToManyField which is not supported."
+                    % (cls.__name__, idx, field))
+
+    # list_display_links
+    if cls.list_display_links:
+        _check_istuplew('list_display_links', cls.list_display_links)
+        for idx, field in enumerate(cls.list_display_links):
+            _check_attr_existsw('list_display_links[%d]' % idx, field)
+            if field not in cls.list_display:
+                raise ImproperlyConfigured("`%s.list_display_links['%d']`"
+                        "refers to `%s` which is not defined in `list_display`."
+                        % (cls.__name__, idx, field))
+
+    # list_filter
+    if cls.list_filter:
+        _check_istuplew('list_filter', cls.list_filter)
+        for idx, field in enumerate(cls.list_filter):
+            _check_field_existsw('list_filter[%d]' % idx, field)
+
+    # list_per_page = 100
+    if not isinstance(cls.list_per_page, int):
+        raise ImproperlyConfigured("`%s.list_per_page` should be a integer."
+                % cls.__name__)
+
+    # search_fields = ()
+    if cls.search_fields:
+        _check_istuplew('search_fields', cls.search_fields)
+        # TODO: search field validation is quite complex (restrictions,
+        # follow fields etc), will skip it as of now
+        # for idx, field in enumerate(cls.search_fields):
+            # _check_field_existsw('search_fields[%d]' % idx, field)
+
+    # date_hierarchy = None
+    if cls.date_hierarchy:
+        f = _check_field_existsw('date_hierarchy', cls.date_hierarchy)
+        if not isinstance(f, (models.DateField, models.DateTimeField)):
+            raise ImproperlyConfigured("`%s.date_hierarchy is "
+                    "neither an instance of DateField nor DateTimeField."
+                    % cls.__name__)
+
+    # ordering = None
+    if cls.ordering:
+        _check_istuplew('ordering', cls.ordering)
+        for idx, field in enumerate(cls.ordering):
+            if field == '?' and len(cls.ordering) != 1:
+                raise ImproperlyConfigured("`%s.ordering` has the random "
+                        "ordering marker `?`, but contains other fields as "
+                        "well. Please either remove `?` or the other fields."
+                        % cls.__name__)
+            if field.startswith('-'):
+                field = field[1:]
+            _check_field_existsw('ordering[%d]' % idx, field)
+
+    # list_select_related = False
+    # save_as = False
+    # save_on_top = False
+    for attr in ('list_select_related', 'save_as', 'save_on_top'):
+        if not isinstance(getattr(cls, attr), bool):
+            raise ImproperlyConfigured("`%s.%s` should be a boolean."
+                    % (cls.__name__, attr))
+
+    # inlines = []
+    if cls.inlines:
+        _check_istuplew('inlines', cls.inlines)
+        for idx, inline in enumerate(cls.inlines):
+            if not issubclass(inline, BaseModelAdmin):
+                raise ImproperlyConfigured("`%s.inlines[%d]` does not inherit "
+                        "from BaseModelAdmin." % (cls.__name__, idx))
+            if not inline.model:
+                raise ImproperlyConfigured("`model` is a required attribute "
+                        "of `%s.inlines[%d]`." % (cls.__name__, idx))
+            if not issubclass(inline.model, models.Model):
+                raise ImproperlyConfigured("`%s.inlines[%d].model` does not "
+                        "inherit from models.Model." % (cls.__name__, idx))
+            _validate_base(inline, inline.model)
+            _validate_inline(inline)
+
+    # TODO: check that the templates exist if given
+    # change_form_template = None
+    # change_list_template = None
+    # delete_confirmation_template = None
+    # object_history_template = None
+
+    # hook for custom validation
+    _call_validation_hook(cls, model)
+
+def _validate_inline(cls):
+    # model is already verified to exist and be a Model
+    if cls.fk_name:
+        f = _check_field_exists(cls, cls.model, cls.model._meta,
+                'fk_name', cls.fk_name)
+        if not isinstance(f, models.ForeignKey):
+            raise ImproperlyConfigured("`%s.fk_name is not an instance of "
+                    "models.ForeignKey." % cls.__name__)
+    # extra = 3
+    # max_num = 0
+    for attr in ('extra', 'max_num'):
+        if not isinstance(getattr(cls, attr), int):
+            raise ImproperlyConfigured("`%s.%s` should be a integer."
+                    % (cls.__name__, attr))
+
+    # formset
+    if cls.formset and not issubclass(cls.formset, BaseFormSet):
+        raise ImproperlyConfigured("`%s.formset` does not inherit from "
+                "BaseFormSet." % cls.__name__)
+
+    # TODO: check the following as other templates above
+    # template = None
+
+    # TODO: is there a need to validate the following?
+    # verbose_name = None
+    # verbose_name_plural = None
+
+    # hook for custom validation
+    _call_validation_hook(cls, cls.model)
+
+def _validate_base(cls, model):
+    opts = model._meta
+    # currying is expensive, use wrappers instead
+    def _check_istuplew(label, obj):
+        _check_istuple(cls, label, obj)
+
+    def _check_isdictw(label, obj):
+        _check_isdict(cls, label, obj)
+
+    def _check_field_existsw(label, field):
+        return _check_field_exists(cls, model, opts, label, field)
+
+    # raw_id_fields
+    if cls.raw_id_fields:
+        _check_istuplew('raw_id_fields', cls.raw_id_fields)
+        for field in cls.raw_id_fields:
+            _check_field_existsw('raw_id_fields', field)
+
+    # fields
+    if cls.fields:
+        for field in cls.fields:
+            _check_field_existsw('fields', field)
+
+    # fieldsets
+    if cls.fieldsets:
+        _check_istuplew('fieldsets', cls.fieldsets)
+        for idx, fieldset in enumerate(cls.fieldsets):
+            _check_istuplew('fieldsets[%d]' % idx, fieldset)
+            if len(fieldset) != 2:
+                raise ImproperlyConfigured("`%s.fieldsets[%d]` does not "
+                        "have exactly two elements." % (cls.__name__, idx))
+            _check_isdictw('fieldsets[%d][1]' % idx, fieldset[1])
+            if 'fields' not in fieldset[1]:
+                raise ImproperlyConfigured("`fields` key is required in "
+                        "%s.fieldsets[%d][1] field options dict."
+                        % (cls.__name__, idx))
+        for field in flatten_fieldsets(cls.fieldsets):
+            _check_field_existsw("fieldsets[%d][1]['fields']" % idx, field)
+
+    # form
+    if cls.form and not issubclass(cls.form, BaseForm):
+        raise ImproperlyConfigured("%s.form does not inherit from BaseForm."
+                % cls.__name__)
+
+    # filter_vertical
+    if cls.filter_vertical:
+        _check_istuplew('filter_vertical', cls.filter_vertical)
+        for field in cls.filter_vertical:
+            _check_field_existsw('filter_vertical', field)
+
+    # filter_horizontal
+    if cls.filter_horizontal:
+        _check_istuplew('filter_horizontal', cls.filter_horizontal)
+        for field in cls.filter_horizontal:
+            _check_field_existsw('filter_horizontal', field)
+
+    # radio_fields
+    if cls.radio_fields:
+        _check_isdictw('radio_fields', cls.radio_fields)
+        for field, val in cls.radio_fields.items():
+            f = _check_field_existsw('radio_fields', field)
+            if not (isinstance(f, models.ForeignKey) or f.choices):
+                raise ImproperlyConfigured("`%s.radio_fields['%s']` "
+                        "is neither an instance of ForeignKey nor does "
+                        "have choices set." % (cls.__name__, field))
+            if not val in (HORIZONTAL, VERTICAL):
+                raise ImproperlyConfigured("`%s.radio_fields['%s']` "
+                        "is neither admin.HORIZONTAL nor admin.VERTICAL."
+                        % (cls.__name__, field))
+
+    # prepopulated_fields
+    if cls.prepopulated_fields:
+        _check_isdictw('prepopulated_fields', cls.prepopulated_fields)
+        for field, val in cls.prepopulated_fields.items():
+            f = _check_field_existsw('prepopulated_fields', field)
+            if isinstance(f, (models.DateTimeField, models.ForeignKey,
+                models.ManyToManyField)):
+                raise ImproperlyConfigured("`%s.prepopulated_fields['%s']` "
+                        "is either a DateTimeField, ForeignKey or "
+                        "ManyToManyField. This isn't allowed."
+                        % (cls.__name__, field))
+            _check_istuplew("prepopulated_fields['%s']" % field, val)
+            for idx, f in enumerate(val):
+                _check_field_existsw("prepopulated_fields['%s'][%d]"
+                        % (f, idx), f)
+
+def _call_validation_hook(cls, model):
+    if hasattr(cls, 'validate'):
+        if not callable(cls.validate):
+            raise ImproperlyConfigured("`%s.validate` should be a callable "
+                    "(class method)." % cls.__name__)
+        cls.validate(model)
+
+def _check_istuple(cls, label, obj):
+    if not isinstance(obj, (list, tuple)):
+        raise ImproperlyConfigured("`%s.%s` must be a "
+                "list or tuple." % (cls.__name__, label))
+
+def _check_isdict(cls, label, obj):
+    if not isinstance(obj, dict):
+        raise ImproperlyConfigured("`%s.%s` must be a dictionary."
+                % (cls.__name__, label))
+
+def _check_field_exists(cls, model, opts, label, field):
+    try:
+        return opts.get_field(field)
+    except models.FieldDoesNotExist:
+        raise ImproperlyConfigured("`%s.%s` refers to "
+                "field `%s` that is missing from model `%s`."
+                % (cls.__name__, label, field, model.__name__))
+
+def _check_attr_exists(cls, model, opts, label, field):
+    try:
+        return opts.get_field(field)
+    except models.FieldDoesNotExist:
+        if not hasattr(model, field):
+            raise ImproperlyConfigured("`%s.%s` refers to "
+                    "`%s` that is neither a field, method or property "
+                    "of model `%s`."
+                    % (cls.__name__, label, field, model.__name__))
+        return getattr(model, field)

django/contrib/admin/sites.py

@@ -7 +7 @@
 from django.utils.text import capfirst
 from django.utils.translation import ugettext_lazy, ugettext as _
 from django.views.decorators.cache import never_cache
+from django.conf import settings
 import base64
 import cPickle as pickle
 import datetime
@@ -65 +66 @@
 
         If a model is already registered, this will raise AlreadyRegistered.
         """
+        do_validate = admin_class and settings.DEBUG
+        if do_validate:
+            # don't import the humongous validation code unless required
+            from django.contrib.admin.validation import validate
         admin_class = admin_class or ModelAdmin
         # TODO: Handle options
         if isinstance(model_or_iterable, ModelBase):
@@ -72 +77 @@
         for model in model_or_iterable:
             if model in self._registry:
                 raise AlreadyRegistered('The model %s is already registered' % model.__name__)
+            if do_validate:
+                validate(admin_class, model)
             self._registry[model] = admin_class(model, self)
 
     def unregister(self, model_or_iterable):