Django: Ticket #12698: ValidationError bug when passing a string message argument in model validationhttps://code.djangoproject.com/ticket/12698
<p>
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()').
</p>
<p>
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).
</p>
<pre class="wiki">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()
</pre>en-usDjangohttps://www.djangoproject.com/s/img/site/hdr_logo.gifhttps://code.djangoproject.com/ticket/12698
Trac 1.0.4orokusakiTue, 26 Jan 2010 19:04:24 GMTneeds_docs, needs_tests, needs_better_patch sethttps://code.djangoproject.com/ticket/12698#comment:1
https://code.djangoproject.com/ticket/12698#comment:1
<ul>
<li><strong>needs_docs</strong>
unset
</li>
<li><strong>needs_tests</strong>
unset
</li>
<li><strong>needs_better_patch</strong>
unset
</li>
</ul>
<p>
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:
</p>
<pre class="wiki"> self._update_errors(dict([['key', v] for v in e.messages]))
</pre><p>
One thing I forgot to mention in my ticket was my code that caused the problem, well here it is:
</p>
<pre class="wiki">class FooModel(models.Model):
def clean(self):
raise ValidationError('Some string error value.')
</pre>
TicketorokusakiTue, 26 Jan 2010 21:30:55 GMThttps://code.djangoproject.com/ticket/12698#comment:2
https://code.djangoproject.com/ticket/12698#comment:2
<p>
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 <a class="closed ticket" href="https://code.djangoproject.com/ticket/12269" title="exception instead of 404 in production (non-debug) mode (closed: invalid)">#12269</a>.
</p>
TicketflebelThu, 28 Jan 2010 18:07:53 GMTattachment sethttps://code.djangoproject.com/ticket/12698
https://code.djangoproject.com/ticket/12698
<ul>
<li><strong>attachment</strong>
set to <em>r12344_ticket12698.diff</em>
</li>
</ul>
<p>
Path on forms/models.py
</p>
TicketflebelThu, 28 Jan 2010 18:08:35 GMThttps://code.djangoproject.com/ticket/12698#comment:3
https://code.djangoproject.com/ticket/12698#comment:3
<p>
I have included a tentative path. No unit tests has been done yet.
</p>
TicketflebelThu, 28 Jan 2010 18:18:12 GMTkeywords, has_patch, needs_tests, summary changedhttps://code.djangoproject.com/ticket/12698#comment:4
https://code.djangoproject.com/ticket/12698#comment:4
<ul>
<li><strong>keywords</strong>
<em>model</em> <em>validation</em> added
</li>
<li><strong>has_patch</strong>
set
</li>
<li><strong>needs_tests</strong>
set
</li>
<li><strong>summary</strong>
changed from <em>ValidationError requires dict() to function properly</em> to <em>ValidationError bug when passing a string message argument in model validation</em>
</li>
</ul>
<p>
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: <a class="ext-link" href="http://docs.djangoproject.com/en/dev/ref/models/instances/#id1"><span class="icon">​</span>http://docs.djangoproject.com/en/dev/ref/models/instances/#id1</a> ).
</p>
<p>
As for my previous comment, "path" should've been "patch" ;)
</p>
TicketkmtraceyTue, 09 Feb 2010 18:43:20 GMTstage changedhttps://code.djangoproject.com/ticket/12698#comment:5
https://code.djangoproject.com/ticket/12698#comment:5
<ul>
<li><strong>stage</strong>
changed from <em>Unreviewed</em> to <em>Accepted</em>
</li>
</ul>
TicketjkocherhansWed, 10 Feb 2010 00:34:46 GMTstatus changed; resolution sethttps://code.djangoproject.com/ticket/12698#comment:6
https://code.djangoproject.com/ticket/12698#comment:6
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
</ul>
<p>
(In <a class="changeset" href="https://code.djangoproject.com/changeset/12402/">[12402]</a>) Fixed <a class="closed ticket" href="https://code.djangoproject.com/ticket/12698" title="Uncategorized: ValidationError bug when passing a string message argument in model ... (closed: fixed)">#12698</a>. 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.
</p>
Ticketjoel.rosen@…Thu, 03 Nov 2011 18:02:32 GMTstatus changed; severity, easy, ui_ux, type set; resolution deletedhttps://code.djangoproject.com/ticket/12698#comment:7
https://code.djangoproject.com/ticket/12698#comment:7
<ul>
<li><strong>status</strong>
changed from <em>closed</em> to <em>reopened</em>
</li>
<li><strong>severity</strong>
set to <em>Normal</em>
</li>
<li><strong>resolution</strong>
<em>fixed</em> deleted
</li>
<li><strong>easy</strong>
unset
</li>
<li><strong>ui_ux</strong>
unset
</li>
<li><strong>type</strong>
set to <em>Uncategorized</em>
</li>
</ul>
<p>
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?
</p>
TicketkmtraceySat, 05 Nov 2011 23:49:45 GMTstatus changed; resolution sethttps://code.djangoproject.com/ticket/12698#comment:8
https://code.djangoproject.com/ticket/12698#comment:8
<ul>
<li><strong>status</strong>
changed from <em>reopened</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
</ul>
<p>
This has been resolved, by changeset <a class="changeset" href="https://code.djangoproject.com/changeset/12402/">r12402</a>, 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.
</p>
<p>
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.
</p>
<p>
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 (<a class="ext-link" href="https://docs.djangoproject.com/en/dev/ref/models/instances/#django.db.models.Model.clean"><span class="icon">​</span>https://docs.djangoproject.com/en/dev/ref/models/instances/#django.db.models.Model.clean</a>) 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.)
</p>
Ticketjoel.rosen@…Sun, 06 Nov 2011 03:32:52 GMThttps://code.djangoproject.com/ticket/12698#comment:9
https://code.djangoproject.com/ticket/12698#comment:9
<p>
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 <em>not</em> 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.
</p>
<p>
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: <a class="ext-link" href="http://stackoverflow.com/questions/2297800/django-unique-together-for-on-sub-class-model-for-parent-attribute"><span class="icon">​</span>http://stackoverflow.com/questions/2297800/django-unique-together-for-on-sub-class-model-for-parent-attribute</a>). 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.
</p>
TicketKapuraWed, 29 Aug 2012 19:17:28 GMThttps://code.djangoproject.com/ticket/12698#comment:10
https://code.djangoproject.com/ticket/12698#comment:10
<p>
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().
</p>
Ticket