Django

Code

Ticket #6845 (new)

Opened 4 months ago

Last modified 2 days ago

Model validation and its propagation to ModelForms

Reported by: Honza_Kral Assigned to: nobody
Milestone: 1.0 beta Component: django.newforms
Version: SVN Keywords: newforms validation model modelform
Cc: listuser@peternixon.net, gabor@nekomancer.net, yatiohi@ideopolis.gr Triage Stage: Accepted
Has patch: 1 Needs documentation: 1
Needs tests: 1 Patch needs improvement: 0

Description

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

Attachments

6845-against-django-7338.diff (133.7 kB) - added by Honza_Kral on 03/20/08 15:13:34.
6845-against-django-7350.diff (134.9 kB) - added by Honza_Kral on 03/22/08 17:06:58.
6845-against-7424.diff (136.5 kB) - added by Honza_Kral on 04/18/08 03:27:54.
new version
6845-against-7625.diff (136.4 kB) - added by Honza_Kral on 06/14/08 11:37:13.
new working patch

Change History

03/20/08 15:13:34 changed by Honza_Kral

  • attachment 6845-against-django-7338.diff added.

03/22/08 17:05:54 changed by Honza_Kral

  • needs_better_patch changed.
  • needs_tests changed.
  • needs_docs changed.

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

03/22/08 17:06:58 changed by Honza_Kral

  • attachment 6845-against-django-7350.diff added.

04/18/08 03:27:54 changed by Honza_Kral

  • attachment 6845-against-7424.diff added.

new version

06/03/08 18:20:15 changed by BazzaniMarco <alfred.einstein@gmail.com>

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

(follow-up: ↓ 4 ) 06/03/08 18:23:40 changed by BazzaniMarco <alfred.einstein@gmail.com>

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

(in reply to: ↑ 3 ) 06/04/08 06:28:05 changed by Honza_Kral

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)

06/12/08 15:36:46 changed by mrts

Marked #4895 as duplicate of this.

06/12/08 16:31:34 changed by nix

  • cc set to listuser@peternixon.net.

06/13/08 01:53:23 changed by anonymous

  • cc changed from listuser@peternixon.net to listuser@peternixon.net, gabor@nekomancer.net.

06/13/08 11:20:58 changed by Karen Tracey <kmtracey@gmail.com>

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$ 


06/13/08 13:41:22 changed by alfred.einstein@gmail.com

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

06/14/08 10:48:58 changed by programmerq

  • stage changed from Unreviewed to Accepted.

06/14/08 11:37:13 changed by Honza_Kral

  • attachment 6845-against-7625.diff added.

new working patch

06/14/08 11:40:00 changed by Honza_Kral

  • needs_docs set to 1.
  • needs_tests set to 1.

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

06/16/08 12:58:13 changed by mrts

See #702

06/16/08 13:21:37 changed by 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?

06/18/08 09:27:41 changed by mrts

#4895 is another duplicate

07/03/08 04:28:09 changed by ctrochalakis

  • cc changed from listuser@peternixon.net, gabor@nekomancer.net to listuser@peternixon.net, gabor@nekomancer.net, yatiohi@ideopolis.gr.

Add/Change #6845 (Model validation and its propagation to ModelForms)




Change Properties
Action