#6845 closed (fixed)
Model validation and its propagation to ModelForms
Reported by: | Honza Král | Owned by: | Honza Král |
---|---|---|---|
Component: | Forms | Version: | dev |
Severity: | Keywords: | newforms validation model modelform ep2008 | |
Cc: | Christian Schilling, listuser@…, gabor@…, yatiohi@…, ondrej.kohohut@…, bronger@…, rajesh.dhawan@…, model-validation@…, erwin@…, django-trac@…, flori@…, gonz@…, aball@…, public@…, Jannis Leidel, Eduardo de Oliveira Padoan, Benjamin Schwarze, remco@…, piranha@…, moya@…, David Larlet, Carlo Pires, jim@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | yes |
Needs tests: | yes | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
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 throwsdjango.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
andvalidate
to_python
does the type coersion, throwsdjango.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
- hooks for validation individual fields on form should now be named
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 (11)
Change History (83)
by , 17 years ago
Attachment: | 6845-against-django-7338.diff added |
---|
comment:1 by , 17 years ago
by , 17 years ago
Attachment: | 6845-against-django-7350.diff added |
---|
comment:2 by , 17 years ago
the question that everybody wants to ask but nobody do:
when will it be merged into trunk ?
follow-up: 4 comment:3 by , 17 years ago
and another stupid question
is it me or diffs are all empty here?
comment:4 by , 17 years ago
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:
- *feedback* !! (works, does what everybody expects it to do, has all the options neede)
- *documentation* and code clenup
- tests (the existing tests were adapted, but more are needed)
comment:6 by , 17 years ago
Cc: | added |
---|
comment:7 by , 17 years ago
Cc: | added |
---|
comment:8 by , 17 years ago
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$
comment:9 by , 17 years ago
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
comment:10 by , 16 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:11 by , 16 years ago
Needs documentation: | set |
---|---|
Needs tests: | set |
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
comment:13 by , 16 years ago
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?
comment:15 by , 16 years ago
Cc: | added |
---|
by , 16 years ago
Attachment: | 6845-against-7871.patch added |
---|
comment:16 by , 16 years ago
Keywords: | ep2008 added |
---|
by , 16 years ago
Attachment: | 6845-against-7877.patch added |
---|
comment:17 by , 16 years ago
Cc: | added |
---|
comment:18 by , 16 years ago
Cc: | added |
---|
by , 16 years ago
Attachment: | 6845-against-7877.patch.gz added |
---|
comment:19 by , 16 years ago
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.
by , 16 years ago
Attachment: | 6845-against-7877.2.patch added |
---|
comment:20 by , 16 years ago
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
comment:21 by , 16 years ago
Cc: | added |
---|
follow-up: 23 comment:22 by , 16 years ago
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).
comment:23 by , 16 years ago
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.
comment:24 by , 16 years ago
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.
comment:25 by , 16 years ago
Cc: | added |
---|
comment:26 by , 16 years ago
Patch needs improvement: | set |
---|
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.
comment:27 by , 16 years ago
Cc: | added |
---|
follow-up: 29 comment:28 by , 16 years ago
Shouldn't this also address unique_for_[date,month,year] validation in ModelForms?
comment:29 by , 16 years ago
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
by , 16 years ago
Attachment: | 6845-against-8198.patch added |
---|
see attached comment for details about this patch
comment:30 by , 16 years ago
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'svalidate__FIELD
?? They need access to more than the field's value and that would make them candidates for the move. We could do this incontribute_to_class
. - How do we deal with value-altering
clean
procedures? I say we keep it as is - anybody can override the currentclean
implementation and do their thing - No validation for unique and
unique_together
informset
comment:31 by , 16 years ago
Cc: | added |
---|
comment:32 by , 16 years ago
Cc: | added |
---|
comment:33 by , 16 years ago
Cc: | added |
---|
comment:34 by , 16 years ago
milestone: | 1.0 beta → post-1.0 |
---|
Pushing to post-1.0 (see http://groups.google.com/group/django-developers/browse_thread/thread/d534c45a06b32e67).
comment:35 by , 16 years ago
Cc: | added |
---|
comment:36 by , 16 years ago
comment:37 by , 16 years ago
Cc: | added |
---|
comment:38 by , 16 years ago
Cc: | added |
---|
comment:39 by , 16 years ago
Cc: | added |
---|
comment:40 by , 16 years ago
Cc: | added |
---|
comment:41 by , 16 years ago
Cc: | added |
---|
comment:42 by , 16 years ago
Cc: | added |
---|
by , 16 years ago
Attachment: | 6845-against-9226.diff added |
---|
comment:43 by , 16 years ago
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.
comment:44 by , 16 years ago
Cc: | added |
---|
comment:45 by , 16 years ago
Hi Honza, Do you have any new code? I'd like to help here (this feature is critical for my current project)
Thank you
comment:46 by , 16 years ago
Cc: | added |
---|
comment:47 by , 16 years ago
Cc: | added |
---|
comment:48 by , 16 years ago
Is anyone working on this? I need to implement some form of model validation and could help with this.
comment:49 by , 16 years ago
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):
- register an account at http://github.com
- click the fork button at http://github.com/mrts/django/tree/master
- clone the fork to your machine:
git clone git@github.com:YOUR_NICK_HERE/django.git
- add my "upstream" repo for tracking:
cd django git remote add upstream git://github.com/mrts/django.git git fetch upstream
- work on code, commit locally
git commit -a -m "Implemented foo."
- publish changes to your GitHub repo
git push
- when my "upstream" branch changes, pull updates from it (
fetch
andmerge
in one step):git pull upstream master
- 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.
comment:50 by , 16 years ago
Be sure to check the overview/task list: http://wiki.github.com/mrts/django
comment:51 by , 16 years ago
@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.
comment:52 by , 16 years ago
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...
comment:53 by , 16 years ago
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.
comment:54 by , 16 years ago
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!
comment:55 by , 16 years ago
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:
comment:56 by , 16 years ago
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.
comment:57 by , 16 years ago
Cc: | added |
---|
@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.
comment:58 by , 16 years ago
Owner: | changed from | to
---|
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.
follow-up: 60 comment:59 by , 16 years ago
I have nothing against gruntwork :). I'll take care of porting 0.96 validators to django.db.models.validators
.
comment:60 by , 16 years ago
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
comment:61 by , 16 years ago
Interested parties should follow http://wiki.github.com/HonzaKral/django/model-validation
comment:63 by , 16 years ago
milestone: | → 1.2 |
---|
comment:64 by , 16 years ago
Cc: | added |
---|
follow-up: 67 comment:65 by , 15 years ago
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.
comment:67 by , 15 years ago
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.
follow-up: 69 comment:68 by , 15 years ago
hi all are there any news about this patch ?
is there a branch for it ?
what could we do to improve it ?
cheers
comment:69 by , 15 years ago
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
After a mail from Malcolm (thanks), I changed a few things:
Models now have
clean
,to_python
andvalidate
methods that behave the same way as on forms: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