Opened 6 years ago

Closed 4 years ago

Last modified 3 years ago

#12698 closed Uncategorized (fixed)

ValidationError bug when passing a string message argument in model validation

Reported by: orokusaki Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: core.exceptions, core.forms.models, model validation
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no


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 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.
        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.
        super(BaseModelForm, self)._clean_form()

Attachments (1)

r12344_ticket12698.diff (1.4 KB) - added by flebel 6 years ago.
Path on forms/

Download all attachments as: .zip

Change History (11)

comment:1 in reply to: ↑ description Changed 6 years ago by orokusaki

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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.')

comment:2 Changed 6 years ago by orokusaki

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.

Changed 6 years ago by flebel

Path on forms/

comment:3 Changed 6 years ago by flebel

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

comment:4 Changed 6 years ago by flebel

  • Has patch set
  • Keywords model validation added
  • Needs tests set
  • Summary changed from ValidationError requires dict() to function properly to ValidationError bug when passing a string message argument in model validation

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: ).

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

comment:5 Changed 6 years ago by kmtracey

  • Triage Stage changed from Unreviewed to Accepted

comment:6 Changed 6 years ago by jkocherhans

  • Resolution set to fixed
  • Status changed from new to closed

(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.

comment:7 Changed 4 years ago by joel.rosen@…

  • Easy pickings unset
  • Resolution fixed deleted
  • Severity set to Normal
  • Status changed from closed to reopened
  • Type set to Uncategorized
  • UI/UX unset

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?

comment:8 Changed 4 years ago by kmtracey

  • Resolution set to fixed
  • Status changed from reopened to closed

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 ( 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.)

comment:9 Changed 4 years ago by 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: 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.

comment:10 Changed 3 years ago by 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().

Note: See TracTickets for help on using tickets.
Back to Top