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.K...@gmail.com ICQ#: 107471613 Phone: +420 606 678585
Honza Král wrote: > 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.
> 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.
Honza Král wrote: > 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.
* 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.
On Fri, Aug 8, 2008 at 1:09 AM, Gary Wilson Jr. <gary.wil...@gmail.com> wrote:
> 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.
On Aug 8, 7:09 am, "Gary Wilson Jr." <gary.wil...@gmail.com> wrote:
> * 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.
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".
> 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 sounds essential to me. The aim of model validation should be to
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.
On Fri, Aug 8, 2008 at 9:12 AM, Simon Willison <si...@simonwillison.net> wrote: > 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...
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 Kaplan-Moss wrote: > 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.
It's a dirty rotten job, but someone's got to do it.
> 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/
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 E-Mail: Honza.K...@gmail.com ICQ#: 107471613 Phone: +420 606 678585
On Sat, Aug 9, 2008 at 01:40, Steve Holden <holden...@gmail.com> wrote:
> Jacob Kaplan-Moss wrote: >> 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.
> It's a dirty rotten job, but someone's got to do it.
>> 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/
> 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.
> On Sat, Aug 9, 2008 at 01:40, Steve Holden <holden...@gmail.com> wrote:
> > Jacob Kaplan-Moss wrote:
> >> 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.
> > It's a dirty rotten job, but someone's got to do it.
> >> 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/
> 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.
> > On Sat, Aug 9, 2008 at 01:40, Steve Holden <holden...@gmail.com> wrote:
> > > Jacob Kaplan-Moss wrote:
> > >> 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.
> > > It's a dirty rotten job, but someone's got to do it.
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.
> 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.
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.
> 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.
|
