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.
