Model validation and its propagation to ModelForms

[Honza Král]

Hi all, during a sprint at PyCON 2008 I wrote first simple version of model validation.

Main features:

• model fields can define list of validators (kwarg validators) - validator is a function that accepts one argument and throws django.core.validation.validationError if anything doesn't look right
• models now have a validate method that
  • call the validation for fields
  • can be overridden to allow for custom validation
  • raises django.core.validation.validationError if anything goes amiss
• formfields' clean method has been deprecated and split into two - to_python and validate
  • to_python does the type coersion, throws django.core.validation.TypeCoersionError
  • validate doesnt return anything, just raises an exception
  • new kwarg validators allows for passing validators to formfields as well
• form method clean has been renamed to validate and also doesn't return anything.
  • hooks for validation individual fields on form should now be named validate_FIELD_NAME and take the value to validate as only parameter

Steps that remain to be done:

• verify how much code has been broken by this
• clean the code, update comments
• add more tests for the new behavior
• add documentation
• add validation to formsets to validate for unique and unique_together across all the forms in the formset
• allow to turn some of the validation off ???
  • unique(_together) for performance reasons

[Honza Král]

After a mail from Malcolm (thanks), I changed a few things:

Models now have clean, to_python and validate methods that behave the same way as on forms:

• to_python loops through the fields and does type coercion (and save the new value)
• validate loops through the fields and validates them, if new_data attribute is supplied, it would look for the data there, do ad-hoc type coercion (which it wouldn't save -- not sure about this) and then run it through field's validate
• clean calls to_python and validate

Consequences are that if to_python raises an error, validate doesn't run when to_python fails

FormField's clean method is now called from form.full_clean instead of to_python and validate, this should allow for easy backwards compatibility without the need of any special field class.

I changed fi and br localflavors to conform with the rest of django - fields should return unicode strings. I didn't do that for FISocialSecurityNumber because it would break localflavor_fi_tests - it would throw UnicodeDecodeError before ValidationError, in my opinion that's what it should do when passed incorrect data (or perhaps TypeCoercionError). What do you think?

Corrected the typo in TypeCoer(s/c)ionError

[Honza Král]

new version

[BazzaniMarco <alfred.einstein@…>]

the question that everybody wants to ask but nobody do:
when will it be merged into trunk ?

[BazzaniMarco <alfred.einstein@…>]

and another stupid question
is it me or diffs are all empty here?

[Honza Král]

Replying to BazzaniMarco <alfred.einstein@gmail.com>:

and another stupid question
is it me or diffs are all empty here?

the diffs are not empty, they are just not parsed by TRAC, try downloading them

Replying to BazzaniMarco <alfred.einstein@gmail.com>:

the question that everybody wants to ask but nobody do:
when will it be merged into trunk ?

when its ready, so far very few people has tested this and/or expressed their opinion on the matter.

the missing parts:

1. *feedback* !! (works, does what everybody expects it to do, has all the options neede)
2. *documentation* and code clenup
3. tests (the existing tests were adapted, but more are needed)

[mrts]

Marked #4895 as duplicate of this.

[Karen Tracey <kmtracey@…>]

I'm confused by the patch 6845-against-7625.diff and can't get it to apply. I'm using an svn working copy at r7625, no local mods, yet I get some failures and many applies with offsets and one "Reversed (or previously applied) patch detected!". The first failure (django/contrib/auth/create_superuser.py) appears to reference code that was moved out of that file in r7590. Is this patch really against r7625 and if so what am I doing wrong?

kmt@lbox:~/tmp/django/trunk$ svn info
Path: .
URL: http://code.djangoproject.com/svn/django/trunk
Repository Root: http://code.djangoproject.com/svn
Repository UUID: bcc190cf-cafb-0310-a4f2-bffc1f526a37
Revision: 7625
Node Kind: directory
Schedule: normal
Last Changed Author: russellm
Last Changed Rev: 7625
Last Changed Date: 2008-06-12 09:19:37 -0400 (Thu, 12 Jun 2008)

kmt@lbox:~/tmp/django/trunk$ svn status -u
       *     7625   django/core/management/commands/dumpdata.py
?                   6845-against-7625.diff
Status against revision:   7630
kmt@lbox:~/tmp/django/trunk$ patch -p1 --dry-run <6845-against-7625.diff 
patching file AUTHORS
Hunk #1 succeeded at 387 (offset 6 lines).
patching file django/contrib/admin/views/template.py
patching file django/contrib/auth/create_superuser.py
Hunk #1 FAILED at 61.
1 out of 1 hunk FAILED -- saving rejects to file django/contrib/auth/create_superuser.py.rej
patching file django/contrib/auth/forms.py
patching file django/contrib/auth/models.py
Hunk #2 succeeded at 135 (offset 7 lines).
patching file django/contrib/comments/views/comments.py
patching file django/contrib/flatpages/models.py
patching file django/contrib/localflavor/br/forms.py
patching file django/contrib/localflavor/fi/forms.py
patching file django/contrib/localflavor/jp/forms.py
patching file django/core/exceptions.py
Reversed (or previously applied) patch detected!  Assume -R? [n]  
Apply anyway? [n] 
Skipping patch.
1 out of 1 hunk ignored -- saving rejects to file django/core/exceptions.py.rej
patching file django/core/validation.py
patching file django/core/validators.py
patching file django/db/models/__init__.py
patching file django/db/models/base.py
Hunk #1 FAILED at 1.
Hunk #2 succeeded at 19 with fuzz 2 (offset 6 lines).
Hunk #3 succeeded at 347 with fuzz 2 (offset 69 lines).
1 out of 3 hunks FAILED -- saving rejects to file django/db/models/base.py.rej
patching file django/db/models/fields/__init__.py
Hunk #1 succeeded at 12 with fuzz 1 (offset 2 lines).
Hunk #2 FAILED at 81.
Hunk #3 succeeded at 105 (offset 6 lines).
Hunk #4 succeeded at 173 with fuzz 2 (offset 18 lines).
Hunk #5 succeeded at 348 (offset 21 lines).
Hunk #6 succeeded at 544 (offset 21 lines).
Hunk #7 succeeded at 736 (offset 21 lines).
Hunk #8 succeeded at 744 (offset 21 lines).
Hunk #9 succeeded at 777 (offset 21 lines).
Hunk #10 succeeded at 788 (offset 21 lines).
Hunk #11 succeeded at 946 (offset 21 lines).
Hunk #12 succeeded at 958 (offset 21 lines).
Hunk #13 succeeded at 988 (offset 21 lines).
Hunk #14 succeeded at 1027 (offset 21 lines).
Hunk #15 succeeded at 1125 (offset 21 lines).
1 out of 15 hunks FAILED -- saving rejects to file django/db/models/fields/__init__.py.rej
patching file django/db/models/fields/related.py
Hunk #1 succeeded at 792 (offset 45 lines).
patching file django/newforms/__init__.py
patching file django/newforms/fields.py
patching file django/newforms/forms.py
patching file django/newforms/models.py
patching file django/newforms/util.py
patching file django/oldforms/__init__.py
patching file django/oldforms/validators.py
patching file tests/modeltests/manipulators/models.py
patching file tests/modeltests/model_forms/models.py
patching file tests/modeltests/validation/models.py
patching file tests/regressiontests/core/models.py
patching file tests/regressiontests/core/tests.py
patching file tests/regressiontests/forms/error_messages.py
patching file tests/regressiontests/forms/extra.py
patching file tests/regressiontests/forms/fields.py
patching file tests/regressiontests/forms/localflavor/br.py
patching file tests/regressiontests/forms/localflavor/generic.py
patching file tests/regressiontests/forms/util.py
kmt@lbox:~/tmp/django/trunk$ 

[alfred.einstein@…]

same problems here patching with latest trunk 7629

bzr patch ../../6845-against-7625.diff --strip=1
1 out of 1 hunk FAILED -- saving rejects to file django/contrib/auth/create_superuser.py.rej
1 out of 1 hunk FAILED -- saving rejects to file django/core/exceptions.py.rej
Patch attempted to create file django/core/validation.py, which already exists.
1 out of 1 hunk FAILED -- saving rejects to file django/core/validation.py.rej
1 out of 3 hunks FAILED -- saving rejects to file django/db/models/base.py.rej
1 out of 15 hunks FAILED -- saving rejects to file django/db/models/fields/__init__.py.rej
Patch attempted to create file django/oldforms/validators.py, which already exists.
1 out of 1 hunk FAILED -- saving rejects to file django/oldforms/validators.py.rej
Patch attempted to create file tests/regressiontests/core/models.py, which already exists.
1 out of 1 hunk FAILED -- saving rejects to file tests/regressiontests/core/models.py.rej
Patch attempted to create file tests/regressiontests/core/tests.py, which already exists.
1 out of 1 hunk FAILED -- saving rejects to file tests/regressiontests/core/tests.py.rej
bzr: ERROR: Patch application failed

[Honza Král]

new working patch

[Honza Král]

Should work now, sorry for the mixup:

king all % git reset HEAD --hard
HEAD is now at c5699f6 Fixed #7327 -- Added documentation and test case for defining subqueries. Thanks, Sebastian Noack.
king all % patch -p1 < 6845-against-7625.diff
patching file AUTHORS
patching file django/contrib/admin/views/template.py
patching file django/contrib/auth/forms.py
patching file django/contrib/auth/management/commands/createsuperuser.py
patching file django/contrib/auth/models.py
patching file django/contrib/comments/views/comments.py
patching file django/contrib/flatpages/models.py
patching file django/contrib/localflavor/br/forms.py
patching file django/contrib/localflavor/fi/forms.py
patching file django/contrib/localflavor/jp/forms.py
patching file django/core/validation.py
patching file django/core/validators.py
patching file django/db/models/__init__.py
patching file django/db/models/base.py
patching file django/db/models/fields/__init__.py
patching file django/db/models/fields/related.py
patching file django/newforms/__init__.py
patching file django/newforms/fields.py
patching file django/newforms/forms.py
patching file django/newforms/models.py
patching file django/newforms/util.py
patching file django/oldforms/__init__.py
patching file django/oldforms/validators.py
patching file tests/modeltests/manipulators/models.py
patching file tests/modeltests/model_forms/models.py
patching file tests/modeltests/validation/models.py
patching file tests/regressiontests/core/models.py
patching file tests/regressiontests/core/tests.py
patching file tests/regressiontests/forms/error_messages.py
patching file tests/regressiontests/forms/extra.py
patching file tests/regressiontests/forms/fields.py
patching file tests/regressiontests/forms/localflavor/br.py
patching file tests/regressiontests/forms/localflavor/generic.py
patching file tests/regressiontests/forms/util.py

[mrts]

See #702

[mrts]

Tagging related tickets as model-validation.

<mrts> jacobkm: there are several tickets related to model validation (#1021, #702), should we create a keyword for them? They can't be marked as duplicates of #6845 as they describe various corner cases that can be easily missed by model validation patch.
<DjangoBot> http://code.djangoproject.com/ticket/1021
 http://code.djangoproject.com/ticket/702
 http://code.djangoproject.com/ticket/6845
<jacobkm> mrts: yeah, just wanna tag them "model-validation" or something?

[mrts]

#4895 is another duplicate

[Honza Král]

Just uploaded new patch that deals with the outstanding issues of stuff placement: ValidationError is moved to django.core.exceptions, I got rid of TypeCoercionError (the distinction between validation errors and type coercion error was too unclear for it to make sense) and ErrorList has moved to django.newforms.util. I introduced FormValidationError that takes inherits from ValidationError and uses ErrorList to represent itself.

[Honza Král]

I stopped for a moment, talked on the IRC and realized that I am an idiot and I don't need the ErrorList to be inside the ValidationError but can exist separately.

enjoy

[Victor Andrée <victor.andree@…>]

Uh, would it still be possible to manipulate cleaned_data in Form.validate? I currently have a clean-method which creates a cleaned_data-item out of several fields, i.e. it constructs a Python value from several fields. I consider it a clean solution in my code (albeit unorthodox).

[Honza Král]

Replying to Victor Andrée <victor.andree@gmail.com>:

Uh, would it still be possible to manipulate cleaned_data in Form.validate? I currently have a clean-method which creates a cleaned_data-item out of several fields, i.e. it constructs a Python value from several fields. I consider it a clean solution in my code (albeit unorthodox).

No, validate shouldn't manipulate data, but you can still override clean() when necessary.

[Malcolm Tredinnick]

We probably do need to find a way to let the "normalise + validate" pass change the data, Honza. This is possible now and quite a useful feature (particularly in Form.clean(). It's not entirely clear where the natural responsibility lies in theto_python and validate split, since you kind of want to validate before munging. We should think about this. It might a "phase 2" feature if it becomes clear that there is a solution but we bump the implementation a bit, but I'd like to be sure there is a way to do this.

Example use cases are things where you combine multiple fields into one value to validate them as a pair and then, since you've done all that work, make them available via the form's data. Now it's not a showstopper if this isn't possible, since you can make a method/property that does the conversion from multiple form fields to one, but it's a bit of an arbitrary restriction.

Anyway, something to think about. I'd forgotten about this situation, so I'll have to reread the patch with that situation in mind.

[Honza Král]

new patch after 1.0 alpha

[Honza Král]

I attached new version of the patch, no changes in functionality, just maybe some typos and stuff introduced during the hectic rebasing on trunk.

I haven't had any time to think about Malcolms comment, but I agree that there are some issues we still need to resolve.

Some tests are failing - and it is IMHO a DDN: the auth tests are failing because UserCreationForm doesn't contain all the required fields and so the model.validate shoots it down.

I have no idea why the formset tests are failing, I will work on it.

This is by no means a patch suitable in any way for merging, it is work in progress only meant for people who wish to work on this issue.

I hope to find some time tomorrow to do it properly, but I cannot promise anything.

[Jökull Sólberg Auðunsson <jokullsolberg@…>]

Shouldn't this also address unique_for_[date,month,year] validation in ModelForms?

[Honza Král]

Replying to Jökull Sólberg Auðunsson <jokullsolberg@gmail.com>:

Shouldn't this also address unique_for_[date,month,year] validation in ModelForms?

It should indeed, thanks for the catch. I will try and incorporate that into the next patch

[Honza Král]

see attached comment for details about this patch

[Honza Král]

I attached new version of this ticket, some tests fail. Issues:

• Failed test auth should ModelForm restricted to some fields validate even if some required (blank=False) fields were omitted?
• Failed test model_formsets I haven't figure out how to run validation on InlineFormset with new instance (I don't have all the data needed to run validation)
• UNTESTED should we move validation for unique, unique_for_date etc. to model's validate__FIELD ?? They need access to more than the field's value and that would make them candidates for the move. We could do this in contribute_to_class.
• How do we deal with value-altering clean procedures? I say we keep it as is - anybody can override the current clean implementation and do their thing
• No validation for unique and unique_together in formset

[Jacob]

Pushing to post-1.0 (see ​http://groups.google.com/group/django-developers/browse_thread/thread/d534c45a06b32e67).

[Gary Wilson]

(In [8348]) Fixed #8206 -- Removed validate methods of Model and Model fields. They are are unsupported for 1.0 and will be replaced with more complete model validation (refs #6845).

[Honza Král]

Another version of the patch.

Implemented:

• moved ValidationError to django.core.exceptions, leaving ErrorList in django.forms
• field.validate and field.clean on db fields
• model.clean and model.validate on models
• moved validate_unique to models
• have formsets call form.save instead of save_instance (split save_instance into two functions for that purpose)

Not yet implemented:

• validators and their use in both db and form fields
• custom error messages (being able to override the messages from models in forms etc.)

Broken:

• save_as_new on formsets - the model without the FK doesn't validate well, I don't know what to do with this one - on one hand it shouldn't validate (the model IS invalid), on the otherhand it's useful feature. Maybe when I get around to enable turning model validation off for a form instance, that might help a bit. Or some sort of delayed validation. suggestions welcome.

Changed and untested:

• BaseGenericInlineFormSet

stay tuned for more.

[alerades]

Hi Honza, Do you have any new code? I'd like to help here (this feature is critical for my current project)

Thank you

[Carlo Pires]

Is anyone working on this? I need to implement some form of model validation and could help with this.

[mrts]

I'll semi-champion and coordinate this (see ​http://groups.google.com/group/django-developers/msg/23e95656e30195ef ).

​http://github.com/mrts/django/tree/master created with the latest patch. Everybody is most welcome to contribute.

A short howto (fork-based process seems best to me):

1. register an account at ​http://github.com
2. click the fork button at ​http://github.com/mrts/django/tree/master
3. clone the fork to your machine:

   git clone git@github.com:YOUR_NICK_HERE/django.git
   

4. add my "upstream" repo for tracking:

   cd django
   git remote add upstream git://github.com/mrts/django.git
   git fetch upstream
   

5. work on code, commit locally

   git commit -a -m "Implemented foo."
   

6. publish changes to your GitHub repo

   git push
   

7. when my "upstream" branch changes, pull updates from it (fetch and merge in one step):

   git pull upstream master
   

8. when you want to get your changes merged back to my tree, send me a pull request by clicking pull request at your fork's home page.

[mrts]

Be sure to check the overview/task list: ​http://wiki.github.com/mrts/django

[Malcolm Tredinnick]

@mrts: This is already well under control. Honza is working on it and coordinating with myself and Jacob. Let's not please have parallel implementations running, since Honza has spent a lot of time gathering requirements already.

[mrts]

Good to hear that Honza is working on it, but it's unfortunate that this wasn't mentioned neither here nor on the mailing list -- what a waste of my time...

[Malcolm Tredinnick]

Except that it has been mentioned on the mailing list multiple times (most recently last week when ericflo asked about it) and has been on the Version1.1Features page for ages. A quick glance at the producer of all the patches would show some evidence of Honza working on it as well.

[mrts]

Arguing is not on my Favourite Pastimes List, but last update here is from October (followed by two requests for status updates) and the only mention since ages on the mailing list is a line "I'll check in with Honza and make sure that validation is under control" by yourself a few days ago -- which doesn't really say much (whether it is or is not under control remains open). Thus, there were sufficient grounds to conclude that no progress has been made since the last patch.

Anyhow, good luck, Honza, and thanks for working on this!

[Honza Král]

I am the last one to say that everything is moving according to plan, but I AM working on this - I have set aside almost entire next week to update the patch and rebase it to new version of django.

The least I would expect is that somebody would contact me directly and ask about the status before assuming my death. Only Malcolm did that, and I answered that I should have new version within a couple of days.

btw. the version you based your wiki on is obsolete, there is a new one availible:

​http://groups.google.com/group/django-developers/browse_thread/thread/6143408977f7ad/d09c52e1fb561469

[mrts]

Aha, good. Do you want me to assist you? Perhaps you could fork the django svn clone on github yourself and publish your work so that all who are interested can contribute (the fork-based workflow is really nice and flexible)? The automatically updated django svn clone is at ​http://github.com/django/django/tree/master.

[Carlo Pires]

@Honza, I didn't understand if your last code is 6845-against-9226.diff or you are working on new one for a few days.

[Honza Král]

I will post my commits here: ​http://github.com/HonzaKral/django/tree/master

After this weekend I hope to have a lot of tasks for anybody willing to help, now I have to update the base of the code to match current state of django.

If you have time now, you can definitely start working on validators (not glamorous task, I agree, but it needs to be done) so that we can later integrate them.

@carlopires: I am working on new code, but the main thing I am doing now is cleaning and updating the last patch to trunk, will push to github as soon as that's done.

[mrts]

I have nothing against gruntwork :). I'll take care of porting 0.96 validators to django.db.models.validators.

[Honza Král]

Replying to mrts:

I have nothing against gruntwork :). I'll take care of porting 0.96 validators to django.db.models.validators.

please don't put it there - it should server for form validation as well.

Validators are functions like this:

def validate_something( value, all_values={}, instance=None ): 
  if i_dont_like( value ): 
    raise ValidationError( 'I dont like %r' % value, error_code=validators.SOME_CODE ) 

where error code is some constant that will later be used to remap the default errors in custom_messages

[mrts]

Interested parties should follow ​http://wiki.github.com/HonzaKral/django/model-validation

[(none)]

Milestone post-1.0 deleted

[sampablokuper]

This might not be the right place to ask the question, but can anyone point me towards a document explaining the rationale under which Django's developers decided that:

• most validation rules must be specified at the "forms" layer, although an a handful of them, e.g. max_length, can be specified at the model layer if desired

rather than deciding that:

• validation rules should all be specified at the "model" layer and bubble up from there, with the option to override them for specific forms if needed

?

Many thanks.

[sampablokuper]

Typo in the above:

s/an a handful/an arbitrary handful/

[Honza Král]

Replying to sampablokuper:

This might not be the right place to ask the question, but can anyone point me towards a document explaining the rationale under which Django's developers decided that:

• most validation rules must be specified at the "forms" layer, although an a handful of them, e.g. max_length, can be specified at the model layer if desired

rather than deciding that:

• validation rules should all be specified at the "model" layer and bubble up from there, with the option to override them for specific forms if needed

This was not decided, it just wasn't implemented. This ticked and the accompanying patch fixes that for you. After this ticket is resolved you should be able to define all your model validation on Models and only use form validation for some extra stuff or for forms that aren't ModelForms.

And yes, this isn't exactly a good place to ask a question like this, try the django-dev mailing list if you want more information.

[Marco Bazzani]

hi all are there any news about this patch ?
is there a branch for it ?
what could we do to improve it ?
cheers

[Honza Král]

Replying to visik7:

hi all are there any news about this patch ?
is there a branch for it ?
what could we do to improve it ?
cheers

The patch is ready for testing in soc2009/model-validation branch

[Tobias McNulty]

closed #11998 as duplicate of this case

[jkocherhans]

Closed by [12098].

[Jacob]

Milestone 1.2 deleted