#6845 - model validation DDN
Honza Král
in Chicago sprint I started work on model-aware validation and I
managed to almost finish it in Vilnius. However, there are a few
oustanding issues that I need to sort out before I can attempt to wrap
it up.
question 1)
class MyForm( ModelForm ):
class Meta:
model = User
fields = ("username",)
is this form valid? I don't think it can ever be used for creating a
user since it doesn't contain fields that are marked as required
(blank=False).
this is one of the tests that fail with the patch.
question 2)
How do we deal with InlineFormset? It can currently accept an empty
instance of the parent class (pk=None) for use in save_as_new behavior
in admin. But I can imagine model-based validation that have to have
access to the object its referencing and such validation wouldn't work
in this case.
this is why model_formsets tests are failing
question 3)
I split up to_python and validate on FormFields and made it difficult
for people wanting to alter their data based on other fields. I don't
think that's a good use case for a form - that kind of logic should be
outside, or in a method not bound into the form's inner workings. But
people seem to want this.
I kept the clean_FIELD hooks on the form, so this can be done there.
missing feature)
I still dont do validation for unique_together and unique on formsets.
Its not a big problem, but its rather complicated. I believe we can
live without this in first release.
I will be glad on any feedback. As soon as I figure these things out,
I can finish the work and try and push for inclusion.
btw. Kevin Fricovsky (big thanks!) has volunteered to write up the
docs, once the code is stable.
Honza Král
E-Mail: Honza...@gmail.com
ICQ#: 107471613
Phone: +420 606 678585
Gary Wilson Jr.
> in Chicago sprint I started work on model-aware validation and I
> managed to almost finish it in Vilnius. However, there are a few
> oustanding issues that I need to sort out before I can attempt to wrap
> it up.
Hi Honza,
I would like to take a look at this tonight and Jacob and Malcolm will
be working on this during tomorrow's sprint. It would be nice if you
could (or someone else familiar with the patch) provide a short
writeup about what the end user interface currently looks like for
validating/cleaning/saving models and form objects.
Gary
Gary Wilson Jr.
> question 1)
>
> class MyForm( ModelForm ):
> class Meta:
> model = User
> fields = ("username",)
>
> is this form valid? I don't think it can ever be used for creating a
> user since it doesn't contain fields that are marked as required
> (blank=False).
I would say this is a valid form, and that (form and model) validation should
only happen on the username field. The programmer could then do what they
want with the unsaved User object, and validate/save later.
> question 2)
>
> How do we deal with InlineFormset? It can currently accept an empty
> instance of the parent class (pk=None) for use in save_as_new behavior
> in admin. But I can imagine model-based validation that have to have
> access to the object its referencing and such validation wouldn't work
> in this case.
Isn't this the same as what goes on when creating a new object anyway? With:
user = User(first_name='jane', last_name='doe')
user.clean()
user.save()
...we didn't have the pk before we validated.
> this is why model_formsets tests are failing
>
> question 3)
>
> I split up to_python and validate on FormFields and made it difficult
> for people wanting to alter their data based on other fields. I don't
> think that's a good use case for a form - that kind of logic should be
> outside, or in a method not bound into the form's inner workings. But
> people seem to want this.
>
> I kept the clean_FIELD hooks on the form, so this can be done there.
Yeah, can't this be done in the validate_<field> hooks with access to
self.cleaned_data? This type of logic wouldn't really be in the Fields
themselves, unless maybe with a ComboField.
> missing feature)
>
> I still dont do validation for unique_together and unique on formsets.
> Its not a big problem, but its rather complicated. I believe we can
> live without this in first release.
As long as we have the interface set, we can fix these sorts of issues later.
Gary
Gary Wilson Jr.
> I will be glad on any feedback. As soon as I figure these things out,
> I can finish the work and try and push for inclusion.
My notes, comments, and suggestions with the patch are below. Hopefully, this
will prove useful to those sprinting on this tomorrow.
validators
==========
* New validators (replacements for many, but not all of the old validators)
have been put in ``django.core.validators``, and the old validators that used
to live there have been moved to ``django.oldforms.validators``.
* Not all of the old validators have replacements. We are probably going to
want to bring over any that haven't been converted already.
* The new validators are only passed the value for the single field, whereas
the old validators received both the value for the single field and the values
for all fields.
* The new validators all have message_dict parameters, which I assume is
supposed to be similar to the error_messages of of form Fields. If this is
supposed to mimic error_messages of form Fields, then I think we should rename
it to match. It doesn't look it's being used yet, and this is going to
require either:
1. Moving all the error message strings to a local dict within the
validator function. Any error messages passed to the validator will override
the defaults.
2. Making a Validator class (that all validators inherit from) to handle
the error_message overriding. This would also be more future proof since
validators become objects instead of just functions.
* The ValidationError class has moved from forms.utils to
django.core.exceptions now that it is used for more than just forms.
contrib.admin.views.template
============================
* I think the import here should be "from django.core import exceptions"
instead of "from django.core import validation". The raise statement needs
correcting too.
django.db.models.Model class
============================
* The methods to_python() and clean() have been added, and the old validate()
method has been changed slightly.
* clean() simply calls to_python(), then validate().
- to_python() calls each field's to_python() and builds up an error_dict of
errors. If there are errors, a ValidationError is raised.
- validate() runs the field values through any validate_<field> methods
defined on the Model, building up an error_dict of errors. It will raise a
ValidationError if there are any errors.
- One thing I notice here is that if to_python() has any errors, none of
validate() will run. This means that you could have a field that shows no
errors one time, and then starts showing errors the next time if there was at
least one error in to_python() the first run and then no errors in to_python()
but errors in validate() the next run. The old validate() method seemed to
handle this better since it attempted to do both field.to_python() and
field.validate_full() in one fell swoop, skipping any field.validate_full()'s
for fields that didn't make it through the field.to_python().
Model fields (django.db.models.fields)
======================================
* Model fields get a new "validators" attribute that is a list of "new"
validators. The "validator_list" __init__ parameter also still exists for
oldforms compatibility and we can remove it when we remove oldforms.
* The validate_full() method has been renamed to validate().
- There is a comment here suggesting we add the unique and unique_for_* to
the Model's validate_<field> method(s) via contribute_to_class, but I think
it's perfectly fine where it is. These are options available to every field,
so it makes sense to perform these checks in the Field class. Dynamically
adding the checks to various validate_<field> methods seems messy and complex
(how would you then override a validate_<field> method, for example).
* There are fields now using the "old" validators that have been moved to
django.oldforms.validators. It looks like all of these are in
oldforms-related methods, such as get_manipulator_fields(), so this is OK.
They will be removed with oldforms. However, some of these being used are of
the RequiredIfOtherFieldNotGiven variety. Is this same functionality
available to the mechanisms used by newforms?
Form Fields (django.forms.fields)
=================================
* Form Fields get a new "validators" attribute that is a list of "new" validators.
* The to_python() method remains and a validate() method have been added.
Fields have had their clean() methods split into to_python() and validate()
methods.
* clean() now calls to_python() then validate().
django.forms.forms.BaseForm class
=================================
* Form gets a new validate() method, which is meant to replace clean(), and
full_clean() now looks for validate_<field> methods, which are meant to
replace clean_<field> methods. full_clean() also uses the Form.validate()
instead of Form.clean() to set the cleaned_data.
- The validate_<field> methods are passed the Field.clean()'ed value (yea
for fixing #3896).
- The clean() method and calling of clean_<field> methods wasn't removed,
for backwards compatibility. If these are going to be deprecated (and I think
they should), and if we are to follow our official release policy of:
"Major point releases will always remove deprecated features immediately."
...then we should just go ahead and remove them now.
django.forms.models.BaseModelForm class
=======================================
* Gets a validate() method that calls BaseForm.validate(), then
self.instance.clean().
- If there are any errors after the BaseForm.validate(), then
self.instance.clean() is not called. I think we want to be calling both here
and collecting all the errors, otherwise we have the same problem mentioned
earlier where two levels of errors are possible and you don't reach the second
level of errors until there are no errors in the first level.
django.forms.models.BaseInlineFormset class
===========================================
* Gained a _construct_form() method. I'm not familiar at all with the Formset
functionality, so I'm not exactly sure why this method was added (a docstring
would be nice).
Suggestions
===========
clean() vs. validate()
~~~~~~~~~~~~~~~~~~~~~~
One thing that annoys me a bit is the fact that, for Models and Forms, clean()
is the higher level method that calls to_python() and validate(). While I
think the bikeshed would look nicer with validate() being the high level
method, calling to_python() and clean(), where you would have something like:
user = User(first_name='jane', last_name='doe')
user.validate()
...instead of:
user = User(first_name='jane', last_name='doe')
user.clean()
...I guess I wouldn't really like how that would mean that instead of
validators we would have cleaners, bah.
Model.is_valid()?
~~~~~~~~~~~~~~~~~
An is_valid() method on Model could be convenient, as it is on Forms. This
way, you could just do a check with is_valid():
user = User(first_name='jane', last_name='doe')
if user.is_valid():
user.save()
else:
...
...instead of calling clean() and having to catch the ValidationError:
user = User(first_name='jane', last_name='doe')
try:
user.clean()
except ValidationError:
...
user.save()
Should Model.save() validate first?
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Since the things that model validation is doing are defined as options on the
Model and its fields, it seems that a save() should validate first. This
could be combined with the is_valid() above along with caching the errors on
the Model instance so that subsequent calls to is_valid() are no-ops. That
way you could explicitly call is_valid() before calling save, and when you do
call save, validation wouldn't need to be rerun.
Gary
Jacob Kaplan-Moss
> My notes, comments, and suggestions with the patch are below. Hopefully, this
> will prove useful to those sprinting on this tomorrow.
Wow, Gary, this is fantastic -- many thanks. I'm still only a few sips
into my morning coffee so I've only skimmed, but this'll really help
Malcolm and I sit down and go through the changes.
I'll post a summary of what we talk about and our conclusions later.
Jacob
Simon Willison
> * The new validators are only passed the value for the single field, whereas
> the old validators received both the value for the single field and the values
> for all fields.
to a model that needs to inspect more than one field? For example,
"name and e-mail address fields are required, but ONLY if the
registered_user foreign key field has not been populated".
> Should Model.save() validate first?
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Since the things that model validation is doing are defined as options on the
> Model and its fields, it seems that a save() should validate first.
minimise the possibility of a database exception being raised when you
attempt to perform a save(). Regular validation errors are much easier
to understand and recover from than database referential integrity
exceptions or what-have-you.
Cheers,
Simon
Jacob Kaplan-Moss
> I'm probably missing something, but how do you apply a validation rule
> to a model that needs to inspect more than one field? For example,
> "name and e-mail address fields are required, but ONLY if the
> registered_user foreign key field has not been populated".
You're not missing anything; as the patch stands this is impossible.
Which means it's going to be an uphill battle convincing me to regress
here -- it basically means that high-level validators are impossible.
See things like ``RequiredIfOtherFieldGiven`` in the oldforms
validators; these things are pretty critical and useful, and not
having a way to do them is, IMO, a major problem.
I'm working on a more complete review of the changes now, but I have
to say that this missing feature is probably going to be my biggest
complaint. I use the shit out of this stuff...
Jacob
Jacob Kaplan-Moss
So I've had a chance to review this patch closely at today's sprint.
I'm pretty happy with the general direction, and this'll make a pretty
fantastic improvement to Django.
However...
There's some pretty serious design decisions still to be made (I
already complained loudly about one of them) and there's also a bunch
of smaller stuff (which Gary did a good job rounding up) that still
needs to be changed.
Further, this will destabilize the tree quite a bit, which means we
need lots of time to shake out all the bugs. Given that, we'd need to
get this feature checked in like *this weekend* to give enough time to
shake stuff down.
I don't see us making these decisions and then implementing all the
missing features in the next 48 hours.
I've reviewed this with Adrian, and he concurs, so this is one of
these rare times when we're going to weird the BDFL had and dictate
that, unfortunately, this can't happen in time for 1.0.
I'm perfectly aware that this sucks, but it sucks less than delaying
the release, and a lot less than releasing something half-baked.
Honza, I *really* appreciate your work on this -- I really hope you
won't be disheartened!
Jacob
David Zhou
> weird the BDFL had
wear the BDFL hat? ;)
---
David Zhou
da...@nodnod.net
Jacob Kaplan-Moss
>
> On Aug 8, 2008, at 5:43 PM, Jacob Kaplan-Moss wrote:
>
>> weird the BDFL had
>
>
> wear the BDFL hat? ;)
See, that's how uncomfortable I am in this particular hat -- I
temporarily lost all control of the English language.
Port sausage unstop the blog barrel.
Jacob
Steve Holden
> On Fri, Aug 8, 2008 at 4:51 PM, David Zhou <da...@nodnod.net> wrote:
>> On Aug 8, 2008, at 5:43 PM, Jacob Kaplan-Moss wrote:
>>
>>> weird the BDFL had
>>
>> wear the BDFL hat? ;)
>
> See, that's how uncomfortable I am in this particular hat -- I
> temporarily lost all control of the English language.
>
> Port sausage unstop the blog barrel.
>
Meat inspector.
regards
Steve
--
Steve Holden +1 571 484 6266 +1 800 494 3119
Holden Web LLC http://www.holdenweb.com/
Honza Král
thanks for the feedback, I am still on vacation (haven't opened my
notebook for days, what a feeling ;) ). I will be back in a couple of
days and this thread is right at the top of my ToDo list.
I plan to see this thing through unless somebody beats me to it or the
design decisions go exactly opposite to my ideas (unlikely, even after
reading this) and have set aside some time to do it next week.
Thanks again for the feedback and patience...
Honza Král
E-Mail: Honza...@gmail.com
ICQ#: 107471613
Phone: +420 606 678585
tobydeh
What's the story with this? Are you aiming for the 1.0 release?
On Aug 12, 8:14 pm, "Honza Král" <honza.k...@gmail.com> wrote:
> Hey all,
> thanks for the feedback, I am still on vacation (haven't opened my
> notebook for days, what a feeling ;) ). I will be back in a couple of
> days and this thread is right at the top of my ToDo list.
>
> I plan to see this thing through unless somebody beats me to it or the
> design decisions go exactly opposite to my ideas (unlikely, even after
> reading this) and have set aside some time to do it next week.
>
> Thanks again for the feedback and patience...
>
> Honza Král
tobydeh
What a shame, i really didn't want to be stuck with newforms
validation in 1.0
Never mind, keep up the good work :)
mrts
What's the status? Getting model validation in (if the powers that be
agree) would help to fix a couple of annoying bugs listed below that
are present in 1.0.
http://code.djangoproject.com/ticket/9039
http://code.djangoproject.com/ticket/8882
All tickets tagged as model-validation:
http://code.djangoproject.com/query?status=new&status=assigned&status=reopened&keywords=~model-validation&order=priority
On Aug 12, 10:14 pm, "Honza Král" <honza.k...@gmail.com> wrote:
> Hey all,
> thanks for the feedback, I am still on vacation (haven't opened my
> notebook for days, what a feeling ;) ). I will be back in a couple of
> days and this thread is right at the top of my ToDo list.
>
> I plan to see this thing through unless somebody beats me to it or the
> design decisions go exactly opposite to my ideas (unlikely, even after
> reading this) and have set aside some time to do it next week.
>
> Thanks again for the feedback and patience...
>
> Honza Král
Honza Kral
I am working on this, I have everything planned and designed based on
my talk with Jacob and Malcolm during DjangoCon. I should have
something ready within a few weeks, will post a patch to the ticket
page.
Thanks for the support
Honza
On Sep 25, 12:35 pm, mrts <m...@mrts.pri.ee> wrote:
> Hi Honza, thank you for your work on model validation!
>
> What's the status? Getting model validation in (if the powers that be
> agree) would help to fix a couple of annoying bugs listed below that
> are present in 1.0.
>
>
> All tickets tagged as model-validation:http://code.djangoproject.com/query?status=new&status=assigned&status...