Re: ModelForm validation - a better way?

[legutierr]

> For reference, here's where we'd be in that case (I still prefer the
> context manager over the idea of two separate calls to something named
> "validate"):
>
> def my_view(request):
>     form = MyModelForm(request.POST or None)
>     try:
>         with form.validate(tweak=True) as obj:
>             obj.user = request.user
>     except ValidationError:
>         pass
>     else:
>         obj.save()
>         return redirect(obj)
>     return render(request, "foo.html", {"form": form})
>
> Or in the simple case, where no modifications to the object are needed:
>
> def my_view(request):
>     form = MyModelForm(request.POST or None)
>     try:
>         obj = form.validate()
>     except ValidationError:
>         pass
>     else:
>         obj.save()
>         return redirect(obj)
>     return render(request, "foo.html", {"form": form})
>
> Carl

In my opinion, this looks good.  I have some additional ideas as to
what the parameters to validate() should be, but in general I think
most of the most important issues are on the table.  I like the idea
of having part of the validation take place in __enter__ and part in
__exit__; I think that maybe this should be controllable using the
arguments to validate() (i.e., using parameters like 'defer',
'skip_model', 'immediate').  I am also not entirely sure about using
ValidationError; perhaps the exceptions raised should be subclasses of
ValidationError, so that they can easily be caught all at once, but I
think it would be very useful for simple form validation and model
validation to raise different exceptions, primarily as a way to give
people some level of control over how errors are displayed.  The
tricky nature of error generation inside model validation could be
helped a little if programmers were given the ability to distinguish
between error types by what subclass of ModelValidationError is
raised.  I am also assuming that form._errors would be populated
inside validate(), __enter__() and __exit__(); it might be helpful to
create an api that makes it easier to add errors to form._errors.

The *problem* that I see is this:

The changes that we are discussing here will give model forms a
different validation API to standard forms.  Yes, form.validate()
could also be added to the standard Form, but then what would the
context manager return from its __enter__ method?  If form.validate()
is a method only of ModelForm, then validation of Forms and ModelForms
would be very different.

Now, in the general case, having different validation processes for
Form and ModelForm wouldn't be too much of a *practical* problem (in
most cases a programmer knows whether they are dealing with a Form or
a ModelForm).  With regards to class-based views, however, and other
systems that depend on a consistent "abstract" Form API, an
inconsistent api between the two would be a headache, and possibly
limit the convenience and usability of such systems, I think.  Also,
saying that validation of ModelForm works differently than Form in a
substantive way just feels wrong.

Regards,

Eduardo

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@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.

Re: ModelForm validation - a better way?

[Carl Meyer]

On 04/29/2011 10:02 AM, Yishai Beeri wrote:
> Of course, cleanup need not be simplistic. In fact, I think the common
> coder would never expect a CM to actually save an object on __exit__ -
> and will be surprised by the proposed behavior.

Could be - the name "finish()" was intended to give the impression that
the purpose of the CM was to wrap up everything the modelform needs to
do. That currently includes saving the object.

I'm open to the idea that we change the name of the CM to
"form.validate()" and it never saves the object; you have to call
obj.save() yourself. In a way, it feels like making users of the API do
extra work for no good reason, and opening the door to mis-use of the
API (since all changes to the object should be completed within the body
of the context manager), but perhaps this is worth it to avoid
unexpected behavior.

For reference, here's where we'd be in that case (I still prefer the
context manager over the idea of two separate calls to something named
"validate"):

def my_view(request):
form = MyModelForm(request.POST or None)
try:
with form.validate(tweak=True) as obj:
obj.user = request.user
except ValidationError:
pass
else:
obj.save()
return redirect(obj)
return render(request, "foo.html", {"form": form})


Or in the simple case, where no modifications to the object are needed:


def my_view(request):
form = MyModelForm(request.POST or None)
try:
obj = form.validate()
except ValidationError:
pass
else:
obj.save()
return redirect(obj)
return render(request, "foo.html", {"form": form})


Carl

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@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.

Re: ModelForm validation - a better way?

[Yishai Beeri]

Hi Carl,

On Fri, 29 Apr 2011 17:42:32 +0300, Carl Meyer  wrote:


Hi Yishai,

On 04/29/2011 08:53 AM, Yishai Beeri wrote:

First, the logic tied into the context manager does not match the
idiomatic use of context managers in Python. One expects a context
manager to "clean up" on exit, rather than *save* a new object.


I'd argue it's not totally off base. When the action you're performing
in the context manager is "tweaking this model instance before its
saved", I think it's reasonable to consider "save it if it validates,
otherwise populate form._errors" to be appropriate and expected
"cleaning up" from that action.


For
instance, what if I throw an exception inside the with block? The idiom
tells us the object will be saved - but in this suggested approach I
probably want the opposite.


The fact that context managers imply cleanup doesn't mean that cleanup
has to be defined simplistically ("save the object no matter what"). The
context manager performs appropriate cleanup. That might be saving,



Of course, cleanup need not be simplistic. In fact, I think the common  
coder would never expect a CM to actually save an object on __exit__ - and  
will be surprised by the proposed behavior.

Another thing not expected by

 or

populating form._errors, or (if you raise an exception inside the body)
probably neither.



I think 'neither' is also not a natural answer for what should a CM do  
when exiting via an exception.




Also unclear is what happens if the form

fails validation (inside the __enter__ on form.finish); an exception?


In the original proposal there would be no form validation on __enter__,
only full validation on __exit__. The proposal could be modified to do
both - that gets into the territory of your and Johannes' alternative
proposals, which are interesting.


I agree. Perhaps saying this is a "validation" context manager, rather  
than a "saving" context manager, is enough to eliminate the confusion.  
That, of course, means adding an explicit form.save() call after the with  
block.


...


and obviously model validation cannot rely on

per-field idioms like form validation does.


I'm not sure what you mean by this last bit - model validation and
modelform validation are actually pretty close to the same thing in the
current code (modelform validation calls model validation and catches
any ValidationErrors), with the exception of possibly excluding some
fields (and doing additional form-specific validation).



I actually referred to the difference between form validation (not  
ModelForm), and Model validation. In other words, part of the validation  
is mostly per-field, and usually based directly on the field definition  
(be it a simple form field or one created in the modelform based on the  
underlying model field); this part is more tightly related to what the  
user sees and does on the form, and naturally does not need the missing  
knowledge about the missing fields (or later tweaks). Another part of  
validation is what I called "model validation" - everything that has to do  
with the model as a whole. My thrust is that it is wise to keep them  
separate (or at least easily separable).



As a very rough sketch, perhaps something along the lines of:

try:
   form.validate()
   obj = form.save(commit=False)
   obj.user = request.user
   obj.validate(form=form)
   obj.save()
   redirect(obj)
except FormValidationError, ModelValidationError:
   redisplay the form

of course, the above can also be written with a couple of if/else
clauses, but perhaps routing validation errors as exceptions may feel
more natural in Python.

I don't really like the obj.validate(form=form) line; perhaps it needs
to be form.validate_object(obj), or even a context manager such as:

with form.validation():
   form.validate()
   ...
   obj.validate() # having the form.validation CM allows the object
validation to somehow tie the errors to the form fields, if so desired
   ...


This ends up being pretty similar to Johannes' idea - I'll reply to his,
feel free to note any differences you think are important.


Yes, you're right. In a nutshell:

a. keep two distinct validation phases [explicit in my suggestion,  
implicit in the __enter__ and __exit__ phases of Johannes' idea]
b. allow some way for the later validation (which I called for simplicity  
"model validation") to still relate to the form, hang errors on form  
elements etc.
c. Make the save action (at least one that commits to the DB) explicit  
[here is where my suggestion differs from both your and Johannes' ones]


It feels that for (b), a context manager of some sort is the way to go.  
Adding (c) to Johannes' approach might be simply to require a call to  
obj.save() after the with block




Carl



Yishai

--
You received this message because you are subscribed to the Google Groups "Django 
developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this 

Re: ModelForm validation - a better way?

[Carl Meyer]

Hi Johannes,

On 04/29/2011 09:02 AM, Johannes Dollinger wrote:
> Here's my take on the API:
> 
> def my_view(request):
> form = MyModelForm(request.POST or None)
> try:
> with form.finish() as obj:
> obj.user = request.user
> return redirect(obj)
> except ValidationError:
> return render(request, "foo.html", {"form": form})
> 
> The context manager would validate the form on __enter__  (only
> included fields) and raise ValidationError if it does not validate.
> On __exit__, it would raise ValidationError if model validation
> fails. This let's you defer expensive computations/queries until
> after a sanity check. 

I like this. It's a little weird that we re-use ValidationError for
something rather different than its usual use (normally it represents a
specific validation error and carries data about that error - in this
case we're effectively re-raising a sort of meta-ValidationError to
represent that the entire form has failed validation. Actually I think
that's probably fine, it just took me a moment to think through).

I think it would also be more correct to use try-except-else, and put
the success-case handling in the else clause.

It looks like this view is supposed to handle both GET and POST, so I
guess your assumption is that the context manager would also raise
ValidationError in case of an unbound form? That feels odd, but it
certainly simplifies the view code. The more-explicit alternative would
be something like this:

def my_view(request):
form = MyModelForm(request.POST or None)
if request.method == "POST":
try:
with form.finish() as obj:
obj.user = request.user
except ValidationError:
pass
else:
return redirect(obj)
return render(request, "foo.html", {"form": form})

That's not too bad either.

Optionally, finish() might take a flag to defer
> all validation until __exit__, which may be required if you want to
> display all validation erros at once. 

Yes, I think that would be useful.

As an added benefit, this makes
> dealing with multiple forms and formset in a single view straight
> forward, as you can simply add more `with` blocks inside the try
> block, and validation across forms can simple raise a ValidationError
> on its own.

Yes - in the cross-form-validation case, the ValidationError might want
to carry a message for the template, but I suppose the developer can
handle that themselves.

I like this direction. I guess the main objection here might be that it
requires try-except handling for every form-handling view. I don't
necessarily consider that a problem: there's something that feels right
about exception-handling, rather than if-else clauses, as the idiom for
managing form validation errors. (Clearly it has something to recommend
it, since that's already the way we handle the details of validation
internally).

Carl

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@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.

Re: ModelForm validation - a better way?

[Carl Meyer]

Hi Yishai,

On 04/29/2011 08:53 AM, Yishai Beeri wrote:
> First, the logic tied into the context manager does not match the
> idiomatic use of context managers in Python. One expects a context
> manager to "clean up" on exit, rather than *save* a new object. 

I'd argue it's not totally off base. When the action you're performing
in the context manager is "tweaking this model instance before its
saved", I think it's reasonable to consider "save it if it validates,
otherwise populate form._errors" to be appropriate and expected
"cleaning up" from that action.

> For
> instance, what if I throw an exception inside the with block? The idiom
> tells us the object will be saved - but in this suggested approach I
> probably want the opposite. 

The fact that context managers imply cleanup doesn't mean that cleanup
has to be defined simplistically ("save the object no matter what"). The
context manager performs appropriate cleanup. That might be saving, or
populating form._errors, or (if you raise an exception inside the body)
probably neither.

Also unclear is what happens if the form
> fails validation (inside the __enter__ on form.finish); an exception?

In the original proposal there would be no form validation on __enter__,
only full validation on __exit__. The proposal could be modified to do
both - that gets into the territory of your and Johannes' alternative
proposals, which are interesting.

> Second, and this is a general issue underlying partial validation -
> probably part of what makes this issue so hairy - the full-model
> validation, and the resulting error messages, run the risk of being
> pretty remote from what the user actually did. It feels to me that form
> validation needs to be a step that focuses on values the user entered in
> the form, and that full-model validation should come as a second step,
> possibly adding more messages and tying them to particular form
> elements.

It would have to be up to the developer tweaking the model instance to
ensure that they don't do so in a way that results in validation errors
that are confusing to the user or that the user can't fix. This is
really no different from the current situation, where if you tweak the
model before saving you're responsible to avoid IntegrityError.

That said, I do see reasons why it would be nice to have the partial
sanity check of the current style of form validation before doing the
extra work that might be involved in tweaking the model for form
validation. Both your and Johannes' proposals do that.

 I think in many cases the two types of validation deserve
> separation in code; model validation might need to be more expensive
> (e.g., hit the DB), 

Already, modelform validation can itself just as easily hit the DB, if
you have unique constraints involving fields included in the form.

and obviously model validation cannot rely on
> per-field idioms like form validation does.

I'm not sure what you mean by this last bit - model validation and
modelform validation are actually pretty close to the same thing in the
current code (modelform validation calls model validation and catches
any ValidationErrors), with the exception of possibly excluding some
fields (and doing additional form-specific validation).

> As a very rough sketch, perhaps something along the lines of:
> 
> try:
>form.validate()
>obj = form.save(commit=False)
>obj.user = request.user
>obj.validate(form=form)
>obj.save()
>redirect(obj)
> except FormValidationError, ModelValidationError:
>redisplay the form
> 
> of course, the above can also be written with a couple of if/else
> clauses, but perhaps routing validation errors as exceptions may feel
> more natural in Python.
> 
> I don't really like the obj.validate(form=form) line; perhaps it needs
> to be form.validate_object(obj), or even a context manager such as:
> 
> with form.validation():
>form.validate()
>...
>obj.validate() # having the form.validation CM allows the object
> validation to somehow tie the errors to the form fields, if so desired
>...

This ends up being pretty similar to Johannes' idea - I'll reply to his,
feel free to note any differences you think are important.

Carl

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@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.

Re: ModelForm validation - a better way?

[Johannes Dollinger]

Am 29.04.2011 um 04:13 schrieb Carl Meyer:

> Hi all,
> 
> We have a number of tickets open (at least #12028, #13249, #13091,
> #15326, and #15860 -- #13091 is the active one) reporting problems with
> unique_together constraints in our attempts to validate arbitrary
> partial models, when validating a ModelForm with some fields excluded.
> 
> Eduardo Gutierrez and I have spent some time looking at this problem
> recently, and my main feeling is that validation of arbitrarily partial
> models in the face of unique_together constraints has no reliably right
> answer, and we'd do better to move away from it altogether.
> 
> Fortunately, I think we can do better. The reason we have to validate
> partial models is because of this idiom:
> 
> if form.is_valid():
>obj = form.save(commit=False)
>obj.user = request.user # for instance
>obj.save()
> 
> But there is no reason those tweaks to the model have to happen after
> form validation. If we encouraged an idiom where the tweaks happen
> before form validation, we could just run full model validation and
> avoid all the error-prone complexity of validating partial models.
> 
> Alex and I talked over some possibilities for a context manager
> available from a new method on ModelForms, that you'd use like this
> (idea originally from Łukasz Rekucki [1], somewhat modified):
> 
>def my_view(request):
>if request.method == "POST":
>form = MyModelForm(request.POST)
>with form.finish() as obj:
>obj.user = request.user
>if form.is_valid():
>return redirect(obj)
>else:
>form = MyForm()
>return render(request, "foo.html", {"form": form})
> 
> form.finish() returns a context manager which returns form.instance from
> its __enter__ method, as "obj" here, allowing the user to do any
> tweaking they like within the body of the context manager. On exit, the
> context manager performs _full_ model validation. Any validation errors
> are saved to the form, as usual. If validation succeeds, the model
> instance is saved.
> 
> The following check to form.is_valid(), then, is just for purposes of
> managing view logic (redirect, or redisplay form?). Actual validation
> has already happened, so it would just be checking self._errors (this
> isn't a change, that's already how .is_valid() works).
> 
> For backwards compatibility, there would be no change to the existing
> behavior if you don't use the new .finish() context manager -
> form.is_valid() would trigger possibly-partial validation just as it
> does now, and do the best it can. But the admin could be converted to
> use the new idiom (with a new ModelAdmin method that would be called
> from within the context manager to allow model-tweaking before
> validation), which would solve the admin-related bugs. And the
> documentation could recommend the new idiom over the old, particularly
> for incomplete modelforms with unique_together constraints.
> 
> Open questions:
> 
> 1. What if you don't need to tweak the instance, but you do want to
> trigger the new full validation behavior (i.e. your form excludes some
> fields, but you initially passed an instance to the form that you're
> confident has the excluded fields already set correctly - this would be
> the case for e.g. the parent FK in admin inlines)? Perhaps form.finish()
> could take an argument that determines whether it returns the context
> manager or just returns form.instance directly?
> 
> 2. Is it going to be too weird for people to adjust to the idea that
> they get their model instance out of the form before they (explicitly)
> call form.is_valid()?
> 
> Other issues we've overlooked, or ways this could be improved? Use cases
> this doesn't handle?

Here's my take on the API:

def my_view(request):
form = MyModelForm(request.POST or None)
try:
with form.finish() as obj:
obj.user = request.user
return redirect(obj)
except ValidationError:
return render(request, "foo.html", {"form": form})

The context manager would validate the form on __enter__  (only included 
fields) and raise ValidationError if it does not validate. On __exit__, it 
would raise ValidationError if model validation fails. This let's you defer 
expensive computations/queries until after a sanity check. Optionally, finish() 
might take a flag to defer all validation until __exit__, which may be required 
if you want to display all validation erros at once.
As an added benefit, this makes dealing with multiple forms and formset in a 
single view straight forward, as you can simply add more `with` blocks inside 
the try block, and validation across forms can simple raise a ValidationError 
on its own.

__
Johannes


-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email 

Re: ModelForm validation - a better way?

[Yishai Beeri]

Without really suggesting a better alternative, I'd like to highlight two  
problems I see with this approach:


First, the logic tied into the context manager does not match the  
idiomatic use of context managers in Python. One expects a context manager  
to "clean up" on exit, rather than *save* a new object. For instance, what  
if I throw an exception inside the with block? The idiom tells us the  
object will be saved - but in this suggested approach I probably want the  
opposite. Also unclear is what happens if the form fails validation  
(inside the __enter__ on form.finish); an exception?


Second, and this is a general issue underlying partial validation -  
probably part of what makes this issue so hairy - the full-model  
validation, and the resulting error messages, run the risk of being pretty  
remote from what the user actually did. It feels to me that form  
validation needs to be a step that focuses on values the user entered in  
the form, and that full-model validation should come as a second step,  
possibly adding more messages and tying them to particular form elements.  
I think in many cases the two types of validation deserve separation in  
code; model validation might need to be more expensive (e.g., hit the DB),  
and obviously model validation cannot rely on per-field idioms like form  
validation does.


As a very rough sketch, perhaps something along the lines of:

try:
   form.validate()
   obj = form.save(commit=False)
   obj.user = request.user
   obj.validate(form=form)
   obj.save()
   redirect(obj)
except FormValidationError, ModelValidationError:
   redisplay the form

of course, the above can also be written with a couple of if/else clauses,  
but perhaps routing validation errors as exceptions may feel more natural  
in Python.


I don't really like the obj.validate(form=form) line; perhaps it needs to  
be form.validate_object(obj), or even a context manager such as:


with form.validation():
   form.validate()
   ...
   obj.validate() # having the form.validation CM allows the object  
validation to somehow tie the errors to the form fields, if so desired

   ...


Yishai





On Fri, 29 Apr 2011 05:13:31 +0300, Carl Meyer  wrote:


form.is_valid()?



--
You received this message because you are subscribed to the Google Groups "Django 
developers" group.
To post to this group, send email to django-developers@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.

Re: ModelForm validation - a better way?

[Carl Meyer]

Hi Lior, thanks for commenting.

On 04/29/2011 12:16 AM, Lior Sion wrote:
> I think that forcing programmers to remember quite a long process of
> for validation each time is a wrong approach, especially if only done
> to support backward code that behaves in "unnatural" way.

I'm not sure why you think it's "quite long," other than that one of the
examples above included a full view and the other one didn't. For
reference, here are equivalent side-by-side examples for the case where
you don't want to modify the object before saving:

current:

if form.is_valid():
obj = form.save()
redirect(obj)

proposed:

obj = form.finish()
if form.is_valid():
redirect(obj)

And for the case where you do want to modify the object before saving:

current:

if form.is_valid():
obj = form.save(commit=False)
obj.user = request.user
obj.save()
redirect(obj)

proposed:

with form.finish(tweak=True) as obj:
obj.user = request.user
if form.is_valid():
redirect(obj)

(This is assuming we use a flag argument to determine whether you want
the context manager. I'm not a huge fan of this, but I like it better
than requiring the context manager and using "pass").

The proposal here is the same length as current code in the first case,
and one line shorter in the second case. So there may be valid
objections to the proposal, but I won't accept length as one of them ;-)

> I looked at the sample you wrote on the other thread

I'm happy to continue conversation on that, but I'll do it in the other
thread for clarity.

Thanks,

Carl

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@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.

Re: ModelForm validation - a better way?

[Lior Sion]

Carl,

I also ran into this issue and opened one of the tickets about it, so
I do have an interest in it.

I think that forcing programmers to remember quite a long process of
for validation each time is a wrong approach, especially if only done
to support backward code that behaves in "unnatural" way.

I looked at the sample you wrote on the other thread (unique together
on username and date, and having a null username with a given date) of
when the old behavior is the right one and it didn't quite convince me
- I do believe that the right implementation would fail a case of
NULLed username and repeating dates, when a unique together exists.

On Apr 29, 5:13 am, Carl Meyer  wrote:
> Hi all,
>
> We have a number of tickets open (at least #12028, #13249, #13091,
> #15326, and #15860 -- #13091 is the active one) reporting problems with
> unique_together constraints in our attempts to validate arbitrary
> partial models, when validating a ModelForm with some fields excluded.
>
> Eduardo Gutierrez and I have spent some time looking at this problem
> recently, and my main feeling is that validation of arbitrarily partial
> models in the face of unique_together constraints has no reliably right
> answer, and we'd do better to move away from it altogether.
>
> Fortunately, I think we can do better. The reason we have to validate
> partial models is because of this idiom:
>
> if form.is_valid():
>     obj = form.save(commit=False)
>     obj.user = request.user # for instance
>     obj.save()
>
> But there is no reason those tweaks to the model have to happen after
> form validation. If we encouraged an idiom where the tweaks happen
> before form validation, we could just run full model validation and
> avoid all the error-prone complexity of validating partial models.
>
> Alex and I talked over some possibilities for a context manager
> available from a new method on ModelForms, that you'd use like this
> (idea originally from Łukasz Rekucki [1], somewhat modified):
>
>     def my_view(request):
>         if request.method == "POST":
>             form = MyModelForm(request.POST)
>             with form.finish() as obj:
>                 obj.user = request.user
>             if form.is_valid():
>                 return redirect(obj)
>         else:
>             form = MyForm()
>         return render(request, "foo.html", {"form": form})
>
> form.finish() returns a context manager which returns form.instance from
> its __enter__ method, as "obj" here, allowing the user to do any
> tweaking they like within the body of the context manager. On exit, the
> context manager performs _full_ model validation. Any validation errors
> are saved to the form, as usual. If validation succeeds, the model
> instance is saved.
>
> The following check to form.is_valid(), then, is just for purposes of
> managing view logic (redirect, or redisplay form?). Actual validation
> has already happened, so it would just be checking self._errors (this
> isn't a change, that's already how .is_valid() works).
>
> For backwards compatibility, there would be no change to the existing
> behavior if you don't use the new .finish() context manager -
> form.is_valid() would trigger possibly-partial validation just as it
> does now, and do the best it can. But the admin could be converted to
> use the new idiom (with a new ModelAdmin method that would be called
> from within the context manager to allow model-tweaking before
> validation), which would solve the admin-related bugs. And the
> documentation could recommend the new idiom over the old, particularly
> for incomplete modelforms with unique_together constraints.
>
> Open questions:
>
> 1. What if you don't need to tweak the instance, but you do want to
> trigger the new full validation behavior (i.e. your form excludes some
> fields, but you initially passed an instance to the form that you're
> confident has the excluded fields already set correctly - this would be
> the case for e.g. the parent FK in admin inlines)? Perhaps form.finish()
> could take an argument that determines whether it returns the context
> manager or just returns form.instance directly?
>
> 2. Is it going to be too weird for people to adjust to the idea that
> they get their model instance out of the form before they (explicitly)
> call form.is_valid()?
>
> Other issues we've overlooked, or ways this could be improved? Use cases
> this doesn't handle?
>
> Thanks,
>
> Carl

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@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.

ModelForm validation - a better way?

[Carl Meyer]

Hi all,

We have a number of tickets open (at least #12028, #13249, #13091,
#15326, and #15860 -- #13091 is the active one) reporting problems with
unique_together constraints in our attempts to validate arbitrary
partial models, when validating a ModelForm with some fields excluded.

Eduardo Gutierrez and I have spent some time looking at this problem
recently, and my main feeling is that validation of arbitrarily partial
models in the face of unique_together constraints has no reliably right
answer, and we'd do better to move away from it altogether.

Fortunately, I think we can do better. The reason we have to validate
partial models is because of this idiom:

if form.is_valid():
obj = form.save(commit=False)
obj.user = request.user # for instance
obj.save()

But there is no reason those tweaks to the model have to happen after
form validation. If we encouraged an idiom where the tweaks happen
before form validation, we could just run full model validation and
avoid all the error-prone complexity of validating partial models.

Alex and I talked over some possibilities for a context manager
available from a new method on ModelForms, that you'd use like this
(idea originally from Łukasz Rekucki [1], somewhat modified):

def my_view(request):
if request.method == "POST":
form = MyModelForm(request.POST)
with form.finish() as obj:
obj.user = request.user
if form.is_valid():
return redirect(obj)
else:
form = MyForm()
return render(request, "foo.html", {"form": form})

form.finish() returns a context manager which returns form.instance from
its __enter__ method, as "obj" here, allowing the user to do any
tweaking they like within the body of the context manager. On exit, the
context manager performs _full_ model validation. Any validation errors
are saved to the form, as usual. If validation succeeds, the model
instance is saved.

The following check to form.is_valid(), then, is just for purposes of
managing view logic (redirect, or redisplay form?). Actual validation
has already happened, so it would just be checking self._errors (this
isn't a change, that's already how .is_valid() works).

For backwards compatibility, there would be no change to the existing
behavior if you don't use the new .finish() context manager -
form.is_valid() would trigger possibly-partial validation just as it
does now, and do the best it can. But the admin could be converted to
use the new idiom (with a new ModelAdmin method that would be called
from within the context manager to allow model-tweaking before
validation), which would solve the admin-related bugs. And the
documentation could recommend the new idiom over the old, particularly
for incomplete modelforms with unique_together constraints.

Open questions:

1. What if you don't need to tweak the instance, but you do want to
trigger the new full validation behavior (i.e. your form excludes some
fields, but you initially passed an instance to the form that you're
confident has the excluded fields already set correctly - this would be
the case for e.g. the parent FK in admin inlines)? Perhaps form.finish()
could take an argument that determines whether it returns the context
manager or just returns form.instance directly?

2. Is it going to be too weird for people to adjust to the idea that
they get their model instance out of the form before they (explicitly)
call form.is_valid()?

Other issues we've overlooked, or ways this could be improved? Use cases
this doesn't handle?

Thanks,

Carl

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@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.