ModelForm validation - a better way?
Carl Meyer
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
Alex Gaynor
with form.finish():
pass
It's a tad silly I guess but not unreasonably I think.
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-d...@googlegroups.com.
To unsubscribe from this group, send email to django-develop...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Alex
--
"I disapprove of what you say, but I will defend to the death your right to say it." -- Evelyn Beatrice Hall (summarizing Voltaire)
"The people's good is the highest law." -- Cicero
Lior Sion
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.
Carl Meyer
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
Yishai Beeri
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 <ca...@oddbird.net> wrote:
> form.is_valid()?
Johannes Dollinger
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
Carl Meyer
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
Carl Meyer
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
Yishai Beeri
On Fri, 29 Apr 2011 17:42:32 +0300, Carl Meyer <ca...@oddbird.net> 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
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
legutierr
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