Model validation and its propagation to ModelForms

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

new version

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

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

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)

Marked #4895 as duplicate of this.

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$ 

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

new working patch

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

See #702

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?

#4895 is another duplicate

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.

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

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

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.

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.

new patch after 1.0 alpha

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.

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

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

see attached comment for details about this patch

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

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

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

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.

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

Thank you

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

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.

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

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

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

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.

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!

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

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.

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

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.

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

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

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

Milestone post-1.0 deleted