ValidationError bug when passing a string message argument in model validation

[Oroku Saki]

In django.core.exceptions on line 45, you see the code is checking if the message passed in is a dict. If not, it sets self.messages = [message]. Then on line 67 update_error_dict() appears to do what's needed in this case of passing in just a string in, like: ValidationError('This is some model validation error in some_model_instance.clean()').

The exception comes at django.forms.models.py on line 320 in _clean_form() (I've commented the code below but not added any code suggestions because I'm not the best developer).

def _clean_form(self):
        """
        Runs the instance's clean method, then the form's. This is becuase the
        form will run validate_unique() by default, and we should run the
        model's clean method first.
        """
        try:
            self.instance.clean()
        except ValidationError, e:
            # Here it assumes that the exception raised has the attribute 'message_dict',
            # but since the errors in the __init__ were not set using (possibly) update_error_dict(),
            # there is just a list in e.messages.
            self._update_errors(e.message_dict)
        super(BaseModelForm, self)._clean_form()

[Oroku Saki]

I changed line 320 of django.forms.models (just as an ad hoc temp solution) to see if it'd change anything, and it did. I changed it to:

            self._update_errors(dict([['key', v] for v in e.messages]))

One thing I forgot to mention in my ticket was my code that caused the problem, well here it is:

class FooModel(models.Model):

    def clean(self):
        raise ValidationError('Some string error value.')

[Oroku Saki]

Update: (sorry to be so zealous but I've never found a bug before unless you count the dragonfly I hit yesterday on I95). I discovered that the issue came from Commit #12269.

[flebel]

Path on forms/models.py

[flebel]

I have included a tentative path. No unit tests has been done yet.

[flebel]

I shall add for the sake of clarity that this bug happens when you raise a ValidationError with a string as the message parameter in the clean* methods of a model (new-in-dev model validation: ​http://docs.djangoproject.com/en/dev/ref/models/instances/#id1 ).

As for my previous comment, "path" should've been "patch" ;)

[jkocherhans]

(In [12402]) Fixed #12698. Model.clean() used with a ModelForm no longer causes a KeyError when raising a ValidationError.
Note that previously it was possible to raise a ValidationError in the same place with a message_dict attribute. That behavior was a bug and will no longer have the same behavior.

[joel.rosen@…]

Has this been resolved? I'm still getting this error when I raise a ValidationError in a model's validate_unique() method. I looked at the code in the patch and then at the code in my django installation (1.3.1) and saw it hadn't been applied. I figured it must have been fixed in the SVN release, so I just upgraded, and it hasn't. Anyone?

[Karen Tracey]

This has been resolved, by changeset r12402, linked in the comment preceding the reopen. The committed code does differ from the patch supplied, so if you were looking for the patch code exactly you won't find it.

The patch took the approach of trying to determine whether the ValidationError raised from the instance's clean_fields, clean, and validate_unique had a message_dict or not and handle both cases.

The committed fix changed the code to expect that clean (and only clean) does NOT raise a ValidationError with a message_dict. This matches the documentation (​https://docs.djangoproject.com/en/dev/ref/models/instances/#django.db.models.Model.clean) which states that ValidationErrors raise by the instance clean method are associated with the special key NON_FIELD_ERRORS. The other two methods (clean_fields and validate_unique) are expected to raise ValidationErrors that have message dicts to properly associate error messages with the fields that they are associated with. (I'm curious -- why are you overriding the base validate_unique method? clean is the only one of these three that is generally expected to be overridden.)

[joel.rosen@…]

I see. Thanks for the explanation. Yes I see now in the documentation that it says that clean() should be used to provide custom model validation, but it doesn't say that other methods such as validate_unique should not be used. The documentation also doesn't explain how you are and are not supposed to use ValidationError, and I'm not sure how I'd be able to figure that out on my own, so thanks.

My reason for overriding validate_unique was, originally, that I had hoped to be able to create a unique_together for two fields: one field on a subclass model using multi-table inheritance, and one field on the parent model (similar to the question and apparently erroneous answer here: ​http://stackoverflow.com/questions/2297800/django-unique-together-for-on-sub-class-model-for-parent-attribute). Evidently that's not possible (although you'd think it should be, even if not at the database level, then at the django model validation level). So I planned to do it myself with my own code in validate_unique -- it seemed a more logical choice than clean(), whose name didn't really call out to me. That, however, also proved impossible, because evidently the model instance that gets passed into validate_unique does not contain all the fields on the model, so I couldn't write my validation code even without this trouble about raising a ValidationError.

[Kapura]

I also tried to override validate_unique for one of my models that would check the name field against other data in the app to verify uniqueness before the save. In my opinion, the documentation should specify the limitations of validate_unique to discourage its use for raising errors, or deprecate it entirely and move all of the relevant code to clean().