On Wed, Jan 6, 2010 at 3:26 PM, Waylan Limberg <way...@gmail.com> wrote: > I've only scanned the docs the other day and haven't actually used the > new model validation stuff, so my impressions may be a little off, > but... > > On Wed, Jan 6, 2010 at 2:54 PM, Joseph Kocherhans <jkocherh...@gmail.com> > wrote: >> On Wed, Jan 6, 2010 at 12:49 PM, Simon Willison <si...@simonwillison.net> >> wrote: >>> A couple of related tickets filed today about model validation: >>> >>> http://code.djangoproject.com/ticket/12513 >>> http://code.djangoproject.com/ticket/12521 >>> >>> The first one describes the issue best - the new model validation code >>> breaks the following common Django convention: >>> >>> form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} ) >>> if form.is_valid(): >>> p = form.save(commit=False) >>> p.user = request.user >>> p.primary_contact = somecontact >>> p.save() >>> > > My initial reaction to this snippet was to wonder why the model was > not being validated after the additional data was added/altered. > Shouldn't you be doing: > > form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} ) > if form.is_valid(): > p = form.save(commit=False) > p.user = request.user > p.primary_contact = somecontact > if p.full_validate(): # <<<<< note this line > p.save() > > [snip]
There are a couple of problems with p.full_validate() there. First, it would run validation a second time. Honza went to a bunch of trouble in the design to make sure that each field would only need to be validated once. Second, p.full_validate() would raise ValidationErrors, not return True or False. >> Once again, that means ModelForm won't really validate your model, >> only your form. form.save() might still throw a database error just >> like before the merge. We can slap a big warning in the ModelForm docs >> though. > > Well, that's why I expected the extra validation check on the model > itself. I understand the desire to have the ModelForm actually > validate the model and work in one step, but sometimes we need the > other way too (as you acknowledge). > >> One (probably unhelpful) way to make ModelForm validate your whole >> model would be to add a Meta flag to ModelForm: >> >> class UserForm(ModelForm): >> class Meta: >> # Defaults to False >> validate_model = True > > Well, what if one view uses that ModelForm one way and another view > uses the same ModelForm the other way? What about > ``form.is_valid(validate_model=True)``? That's a possibility, but I think it suffers from the same problems that the Meta option does. It just pushes the decision closer to runtime than coding time. That's helpful in some cases, but it doesn't solve the main part of the problem for me, which is that I don't think model validation should be an opt-in-only thing. Maybe it needs to be for a couple of releases though. I had another idea that I think might work out. What if ModelForm.validate() only validated fields on the form, like it worked before the merge, and ModelForm.save() would automatically validate the excluded fields, raising ValidationError if there was a problem? Validation for each field would only happen once, it would accommodate the commit=False idiom, and it would still ensure that the model itself is validated in common usage. I think this *might* also lead to a workable solution for ticket #12507 [1], but I need to dig into the code a little more. Joseph [1] http://code.djangoproject.com/ticket/12507
-- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
