Model validation incompatibility with existing Django idioms
Simon Willison
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()
The problem is that is_valid() notices that some of the required
fields in SecretQuestionForm (a ModelForm) have not been provided,
even if those fields are excluded from validation using the excludes=
or fields= properties. The exception raised is a
UnresolvableValidationError.
This definitely needs to be fixed as it presumably breaks backwards
compatibility with lots of existing code (it breaks a common
ModelAdmin subclass convention as well, see #12521). Can we just
change the is_valid() logic to ignore model validation errors raised
against fields which aren't part of the ModelForm, or is it more
complicated than that?
Cheers,
Simon
Joseph Kocherhans
> 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()
>
> The problem is that is_valid() notices that some of the required
> fields in SecretQuestionForm (a ModelForm) have not been provided,
> even if those fields are excluded from validation using the excludes=
> or fields= properties. The exception raised is a
> UnresolvableValidationError.
I'll start off with the reasoning behind the implementation, but I
agree that the current implementation is going to bite too many people
to just call the old implementation a bug.
Form.is_valid() now triggers model validation, and the model isn't
valid. It's missing a couple of required fields. Presenting those
errors to the user filling out the form is unacceptable because there
is nothing the user can do to correct the error, and the developer
will never get a notification about a problem that can only be solved
with code.
> This definitely needs to be fixed as it presumably breaks backwards
> compatibility with lots of existing code (it breaks a common
> ModelAdmin subclass convention as well, see #12521). Can we just
> change the is_valid() logic to ignore model validation errors raised
> against fields which aren't part of the ModelForm, or is it more
> complicated than that?
It shouldn't be much more complicated than that. Model.full_validate()
takes an exclude parameter that we can use to provide a list of fields
that aren't on the form. Or we can catch those errors and just throw
them away. However, this means that part of the problem that
model-validation was meant to solve will no longer be solved.
ModelForm.is_valid() will only mean that your *form* is valid, not
your *model* (which is the pre-merge semantics anyhow).
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.
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
That would make it easy to trigger model validation, but it doesn't
really help with the convention you're talking about. I don't know. Do
people think triggering model validation in a ModelForm is something
they need to do in a practical sense? Or is validating your form
enough?
Joseph
Waylan Limberg
new model validation stuff, so my impressions may be a little off,
but...
On Wed, Jan 6, 2010 at 2:54 PM, Joseph Kocherhans <jkoch...@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]
> 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)``?
--
----
\X/ /-\ `/ |_ /-\ |\|
Waylan Limberg
Łukasz Rekucki
[snip]
Well, that's why I expected the extra validation check on the model
> 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.
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).
Well, what if one view uses that ModelForm one way and another view
> 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
uses the same ModelForm the other way? What about
``form.is_valid(validate_model=True)``?
--
----
\X/ /-\ `/ |_ /-\ |\|
Waylan Limberg
--
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.
Łukasz Rekucki
Joseph Kocherhans
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
Brian Rosner
On Jan 6, 2010, at 2:56 PM, Joseph Kocherhans wrote:
> 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 like this. It would then provide far superior error messages in cases where there was real developer error.
Brian Rosner
http://oebfare.com
http://twitter.com/brosner
Alex Gaynor
> 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.
>
>
>
>
I agree with Brian. I also really like the context manager based API,
so for 1.3 I think it would be nice to include something like that.
Alex
--
"I disapprove of what you say, but I will defend to the death your
right to say it." -- Voltaire
"The people's good is the highest law." -- Cicero
"Code can always be simpler than you think, but never as simple as you
want" -- Me
Brian Rosner
On Jan 6, 2010, at 2:48 PM, Łukasz Rekucki wrote:
> Maybe you could do something like this:
>
> with form.valid_model() as model: # i'm not good at inventing names
> model.user = request.user
> model.primary_contact = somecontact
>
> The valid_model() would be a context manager that does form validation and form.save(commit=False) on enter + model validation and save() on exit. Of course this will only work on Python 2.5+, so it's probably no good for django 1.2. Just wanted to share an idea.
FTR, this is a pretty neat idea. The naming is a bit off, but that can be worked out. Unfortunately, we couldn't much with it now, but I'd like to look at the possibility for 1.3. Thanks for sharing.
Jeremy Dunck
......
>>>> 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()
> 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.
Note that p in the example above is the unsaved model instance, not
the ModelForm. So to fix the idiom, the excluded field validation
would need to be done on Model.save, not ModelForm.save, right?
Brian Rosner
Yeah, I think that must have been a typo in Joseph's mail. The way I read it that the model knows what fields it has already validated and the call to a Model.save would validate the rest.
Joseph Kocherhans
Ugh. Yes it would. I did mean ModelForm.save(), but as you've pointed
out, that won't work (at least for the idiom). One of the early
decisions was that Model.save() would never trigger validation for
backwards compatibility purposes. Maybe something from the idea is
salvageable, but it won't work as I presented it. I think having the
model track which validators have been run and which haven't is a
non-starter. That's something Honza actively avoided in the design.
Joseph
Brian Rosner
On Jan 6, 3:57 pm, Brian Rosner <bros...@gmail.com> wrote:
> Yeah, I think that must have been a typo in Joseph's mail. The way I read it that the model knows what fields it has already validated and the call to a Model.save would validate the rest.
Actually, I just realized that Model.save doesn't do validation now
does it?
Brian Rosner
Saw this after my e-mails. This does present an issue. For sake of backwards compatibility it would seem that the second step of validation could just be done by the developer? This is mostly to prevent double validation, but also maintain compatibility with the Django idiom.
Tai Lee
form is valid and second check that their model object is valid when
calling ModelForm.save(commit=False). This could be done by having the
developer check the result of model.full_validate() before calling
model.save(), or by having model objects returned by ModelForm.save
(commit=False) automatically trigger model validation when model.save
() is called, even if model.save() does not normally trigger model
validation?
Cheers.
Tai.
Honza Král
Just to add few points that lead us to do it this way:
ModelForm has a save() method that saves the model. It is reasonable
to assume that if the form is valid, the save() method call should
succeed, that's why the entire model is validated.
We could create a PartialModelForm, that would have save() method only
when editing (and not adding) models and have other method (or
enforced commit=False) for retrieval of the partial model. This form
would only call validation on the modelfields that are part of the
form (not being excluded). This is the behavior I understand everybody
expects from ModelForm, so, for backwards compatibility, we could call
it just ModelForm.
Also please mind that it could prove difficult to do half the
validation in one place and other fields elsewhere without a form.
Models, as opposed to forms, don't have a state in sense of
validated/raw and they don't track changes to their fields' data.
That's why validation is run every time and the results are not cached
on the instance.
Few quick questions to get some ideas:
1) If editing a model (passed an instance parameter to __init__), even
with a partial form, do you expect model.full_validate being called?
2) Are you OK with ripping save(commit=False) functionality to some
other method? (current functionality can stay with a deprecation
warning for example)
3) Would you want errors raised in model validation after it being
created by a form (commit=False) to appear on the form?
on subject of inlines:
1) Is it acceptable to create a model and not it's inlines in the
admin if the modelform validates but the inlines don't?
2) How can we limit people's validation code to avoid validating FKs
in inlines since users can (and should) create their own validation
logic on Models? Especially when we try and treat admin as "just a
cool app" and not something people should really design for.
3) How would you feel on creating an option to enable behavior (1) )
and document that models with side effects in their save and delete
should really go for that?
4) Making 3) the default and enable switching it off if people want
their admin page to save atomically.
Please let me know what you think
Thanks!
Honza Král
E-Mail: Honza...@gmail.com
Phone: +420 606 678585
David Cramer
to bite us in the ass. I would think a simple fix would be to have a
commit=False, and validate=True keyword arg. By default, validate is
NoInput, but if commit is False it defaults to False. Wouldn't that be
a simple enough backwards compatible workaround?
> E-Mail: Honza.K...@gmail.com
> Phone: +420 606 678585
Skylar Saveland
> to assume that if the form is valid, the save() method call should
> succeed, that's why the entire model is validated.
mf.is_valid() I have understood as validation strictly on the included
fields in the form. I abuse this "feature". Once I know that
something has been done validly by the user, I can bring into motion
all kinds of things. As you can see all of these forms and their ilk
are intentionally excluding known required fields.
class MinimalContactForm(forms.ModelForm):
class Meta:
model = Contact
fields = ('first', 'm', 'last')
class PrimaryContactForm(forms.ModelForm):
class Meta:
model = Contact
exclude = ('user','primary', 'address', 'email')
class ContactForm(forms.ModelForm):
class Meta:
model = Contact
exclude = ('user',)
I know that you know what I'm saying, so let's see
> We could create a PartialModelForm, that would have save() method only
> when editing (and not adding) models and have other method (or
> enforced commit=False) for retrieval of the partial model. This form
> would only call validation on the modelfields that are part of the
> form (not being excluded). This is the behavior I understand everybody
> expects from ModelForm, so, for backwards compatibility, we could call
> it just ModelForm.
not entirely sure what you mean
> only when editing (and not adding) models and have other method (or
> enforced commit=False) for retrieval of the partial model
.
> Also please mind that it could prove difficult to do half the
> validation in one place and other fields elsewhere without a form.
> Models, as opposed to forms, don't have a state in sense of
> validated/raw and they don't track changes to their fields' data.
> That's why validation is run every time and the results are not cached
> on the instance.
>
> Few quick questions to get some ideas:
>
> 1) If editing a model (passed an instance parameter to __init__), even
> with a partial form, do you expect model.full_validate being called?
>
> 2) Are you OK with ripping save(commit=False) functionality to some
> other method? (current functionality can stay with a deprecation
> warning for example)
I wouldn't like to see that idiom changed. We can create another attr
on the form but leave is_valid()?
> 3) Would you want errors raised in model validation after it being
> created by a form (commit=False) to appear on the form?
I suppose that I have been guilty of taking advantage of modelforms to
an extreme degree. I don't picture needing model validation on my
modelforms right now. I sometimes have a bunch of small forms (if the
case needs) like:
<h3>Applicant information</h3>
{{form|as_uni_form}}
{{profile_form|as_uni_form}}
<h4>How may we contact you?</h4>
{{contact_form|as_uni_form}}
<h4>How did you hear about us?</h4>
{{hear_about|as_uni_form}}
<h4>Terms and Conditions</h4>
{{tos_form|as_uni_form}}
if form.is_valid() and profile_form.is_valid()...:
... do magic to create user ...
add the required user field to the other objects with commit=False
idiom
Perhaps in this way I am also abusing the relational db. But, I
always find occasion to add required fields (often FK to the other
modelforms) after I know the form "is
valid" (UnresolvableValidationError).
But, I would like another attr, so I could call modelform.model_errors
() or modelform.model_is_valid() or something. Just plugging my own
self interest here though really.
I would like to see everything that I use behave exactly as I have
come to know it and then see other new features popping up around
that.
>
> on subject of inlines:
> 1) Is it acceptable to create a model and not it's inlines in the
> admin if the modelform validates but the inlines don't?
>
> 2) How can we limit people's validation code to avoid validating FKs
> in inlines since users can (and should) create their own validation
> logic on Models? Especially when we try and treat admin as "just a
> cool app" and not something people should really design for.
>
> 3) How would you feel on creating an option to enable behavior (1) )
> and document that models with side effects in their save and delete
> should really go for that?
>
> 4) Making 3) the default and enable switching it off if people want
> their admin page to save atomically.
>
> Please let me know what you think
I don't really know enough about the internals of the admin to say
much about that.
James Bennett
> ModelForm has a save() method that saves the model. It is reasonable
> to assume that if the form is valid, the save() method call should
> succeed, that's why the entire model is validated.
Actually, I can see a strong argument against this: if I've defined a
form class that I think constructs valid instances, and it doesn't,
that's a bug in my form and Django should be making this as clear as
possible.
Of course, the flip side is that the bug in my form may be something
subtle which breaks a constraint specified at the Python level but not
at the DB level, and so the additional automatic validation could be
the only way to catch it.
> 2) Are you OK with ripping save(commit=False) functionality to some
> other method? (current functionality can stay with a deprecation
> warning for example)
No.
Suppose I have a ModelForm and call save(commit=False) to get the
instance so I can do some more work on it. I'm basically saying to
Django "I don't think this object is ready to be saved yet, but I need
the object so I can do stuff to it". If Django goes ahead and does
implicit model validation there and raises an exception, it's just
telling me what I already knew: the object's not ready to be saved.
But now I get to do extra work to catch the exception, and that's bad
and wrong.
Calling ModelForm.save(commit=False) should simply disable model
validation, period. If I want to validate the instance before saving
it, I'll validate the instance before saving it, but commit=False is
as clear a way as we have of saying "I'm not going to save this yet"
and model validation should respect that.
--
"Bureaucrat Conrad, you are technically correct -- the best kind of correct."
koenb
On 8 jan, 10:03, James Bennett <ubernost...@gmail.com> wrote:
> Suppose I have a ModelForm and call save(commit=False) to get the
> instance so I can do some more work on it. I'm basically saying to
> Django "I don't think this object is ready to be saved yet, but I need
> the object so I can do stuff to it". If Django goes ahead and does
> implicit model validation there and raises an exception, it's just
> telling me what I already knew: the object's not ready to be saved.
> But now I get to do extra work to catch the exception, and that's bad
> and wrong.
>
> Calling ModelForm.save(commit=False) should simply disable model
> validation, period. If I want to validate the instance before saving
> it, I'll validate the instance before saving it, but commit=False is
> as clear a way as we have of saying "I'm not going to save this yet"
> and model validation should respect that.
>
The problem is that in the idiom the is_valid() call on the modelform
tries to full_validate the instance, it is not in the save
(commit=False) call.
I'm with Simon here: on an incomplete modelform, let's just ignore the
errors on excluded fields and only validate the user input at that
point. If the developer wants to be sure the model validates after he
adds the necessary extra information, it is his job to call validation
before saving.
Koen
Honza Král
>
>
> On 8 jan, 10:03, James Bennett <ubernost...@gmail.com> wrote:
>
>> Suppose I have a ModelForm and call save(commit=False) to get the
>> instance so I can do some more work on it. I'm basically saying to
>> Django "I don't think this object is ready to be saved yet, but I need
>> the object so I can do stuff to it". If Django goes ahead and does
>> implicit model validation there and raises an exception, it's just
>> telling me what I already knew: the object's not ready to be saved.
>> But now I get to do extra work to catch the exception, and that's bad
>> and wrong.
>>
>> Calling ModelForm.save(commit=False) should simply disable model
>> validation, period. If I want to validate the instance before saving
>> it, I'll validate the instance before saving it, but commit=False is
>> as clear a way as we have of saying "I'm not going to save this yet"
>> and model validation should respect that.
I just hate the name save(commit=False) when it has nothing to do with
saving or committing, it just DOESN'T call save, it's imho misleading.
I understand why that is and how it came to be, I just don't like it.
I wasn't, however, proposing we get rid of this feature, just that we
extract it to a separate method or just tell people to use
form.instance (which is all what it does + creating save_m2m which can
be put elsewhere).
>
> The problem is that in the idiom the is_valid() call on the modelform
> tries to full_validate the instance, it is not in the save
> (commit=False) call.
Yes
> I'm with Simon here: on an incomplete modelform, let's just ignore the
> errors on excluded fields and only validate the user input at that
> point. If the developer wants to be sure the model validates after he
> adds the necessary extra information, it is his job to call validation
> before saving.
The only problem I see here is that you cannot run model.validate (so
check for unique etc.) because the user might define some custom
validation there that you have no control over and that can check
fields you don't want it to touch. We could move validate_unique
elsewhere, but that could create problem for people not wanting to
have their fields validated for uniqueness (expensive hits to the DB)
But overall I feel that "we only call model.full_validate when no
field is excluded or we are editing an existing instance" is the
desired behavior by most people.
In code that would mean that self.validate in full_clean would only be
called if not exclude and excluded form fields would be passed to
model.full_clean when adding an instance (not when editing), that's a
very simple change. We just need to document this behavior thoroughly
because it can cause confusion.
The question remains how to validate inlines in Admin. I think there
is no question that we want to call full_validate on the inlined model
eventually, the question is how to do it in a most backwards
compatible and sane way.
We need the FK to do the validation and we cannot get the FK without
the save() of the parent model. So imho it's just a question of how
much risk we are taking and at which point do we call model.save() on
the parent (after the form validates, after the inline form fields
validate).
If we take the new proposal for ModelForm behavior it could work:
MainForm.clean()
for f in inline: f.validate()
if all.valid:
MainForm.save()
for inline in inlines:
for form in inline;
f.instance.full_validate()
inline.save()
the problem is when f.instance.full_validate() fails on the inline.
That's a point where the FK must exist so the parent object has to be
saved already. There is no way around this if we want to have
model-validation in admin uncrippled.
Tobias McNulty
I just hate the name save(commit=False) when it has nothing to do with
saving or committing, it just DOESN'T call save, it's imho misleading.
I understand why that is and how it came to be, I just don't like it.
I wasn't, however, proposing we get rid of this feature, just that we
extract it to a separate method or just tell people to use
form.instance (which is all what it does + creating save_m2m which can
be put elsewhere).
There is /a lot/ of code that relies on this naming and, while it might not be the greatest name choice in the world, it's one you get used to after making use of it time and time again. I'm fairly certain the 'commit' argument is not the only instance of imperfect naming in the Django core, and fixing all of these would leave Django users with a relatively insurmountable quantity of deprecated code. Frankly I'm not sure it's worth it.
Tobias
Tobias McNulty
Caktus Consulting Group, LLC
P.O. Box 1454
Carrboro, NC 27510
(919) 951-0052
http://www.caktusgroup.com
Tobias McNulty
On Thu, Jan 7, 2010 at 10:40 AM, Honza Král <honza...@gmail.com> wrote:Actually, I can see a strong argument against this: if I've defined a
> ModelForm has a save() method that saves the model. It is reasonable
> to assume that if the form is valid, the save() method call should
> succeed, that's why the entire model is validated.
form class that I think constructs valid instances, and it doesn't,
that's a bug in my form and Django should be making this as clear as
possible.
Of course, the flip side is that the bug in my form may be something
subtle which breaks a constraint specified at the Python level but not
at the DB level, and so the additional automatic validation could be
the only way to catch it.
For another alternative still, a constraint may exist at the database level that Python doesn't know about (or may not be able to enforce efficiently).
I regret and apologize that I'm arriving to this thread rather late. Is there a document somewhere that discusses, or could someone describe, how model validation fits between the existing form validation in Django and constraints that are specified on the database side?
I too would opt for an implementation that makes model validation optional, i.e., a call that developers must explicitly make, if they choose, before saving a model. I've always been and I continue to be wary of trying to implement data constraints at the application level; that's something good relational databases have been doing and improving upon for a long time and I have little faith in my own capacity to reproduce or replace such functionality.
Cheers,
Ivan Sagalaev
> I regret and apologize that I'm arriving to this thread rather late.
To support the tradition, I'm apoligizing for this too :-). Though it's
funny how everyone thinks they're "late" on a couple-of-days-old thread :-).
Anyway...
> I too would opt for an implementation that makes model validation
> optional, i.e., a call that developers must explicitly make, if they
> choose, before saving a model.
I'm +1 on some way of validating a form without *fully* validating a
model. But for a bit different reason that I didn't see in this thread yet.
The thing is that validating a ModelForm may not be strictly related to
subsequent saving of an instance. A use-case:
I have a PreviewForm that generates a preview for a comment that user is
writing in a form. It uses model fields to validate currently typed text
and selected markup filter and then creates a model instance to do
actual formatting:
class PreviewForm(ModelForm):
class Meta:
model = Comment
fields = ['text', 'markup_filter']
def preview(self):
comment = Comment(
text = self.cleaned_data['text'],
filter = self.cleaned_data['markup_filter'],
)
return comment.html()
It is then used like this:
def preview_view(request):
form = PreviewForm(request.POST)
if form.is_valid(): # <- this now breaks
return HttpResponse(PreviewForm.preview())
The thing is that the code has no intention to save an instance into a
DB. It just needs it at application level. This is why I think that
decisions on "full" vs. "partial" validation should not be bound to
`save()`.
(FYI currently the first patch (Joseph's) in #12521 does fix this
problem while the second patch (Honza's) doesn't. From a quick look this
is beacuse the second patch only excludes fields from validation that
listed in `exclude`).
Tobias McNulty
I'm +1 on some way of validating a form without *fully* validating a model. But for a bit different reason that I didn't see in this thread yet.I too would opt for an implementation that makes model validation optional, i.e., a call that developers must explicitly make, if they choose, before saving a model.
Ivan Sagalaev
> I don't see why model validation should be bound up with forms at all.
I'm not the one who designed it, so it's just me view. I think this is
just useful: if you have a code validating, say, a CharField at the
model level why not reuse it at the form level?
What's important is that *entire validity* of a form should not be bound
to that of a model. This was a bug which Joseph Kocherhans is now fixing.
> The current release notes for model validation state that it won't be
> done unless explicitly requested by the developer, and it seems that
> validating the model inside a form (whether fully or partially) breaks
> that contract.
Well, I think we can fix release notes for the next release :-).
Joseph Kocherhans
>
> 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()
>
> The problem is that is_valid() notices that some of the required
> fields in SecretQuestionForm (a ModelForm) have not been provided,
> even if those fields are excluded from validation using the excludes=
> or fields= properties. The exception raised is a
> UnresolvableValidationError.
OK. I've attached a patch for another shot at this to #12521 [1].
form.is_valid *should* act like it did before the model-validation
merge. This is trickier than it sounds though, and there are probably
a few more corner cases. ModelForm validation uses validation from
model fields and validators, not just the form fields, so simply
skipping validation for excluded fields isn't enough.
model.full_validate() is *only* for validating an entire model. It
calls validate_fields, validate_unique, the the validate hook in
order.
model.validate_fields(fields=None)
Validate the fields specified, or all fields if fields is None. fields
should be a list of field names.
model.validate_unique(fields=None)
Validate the uniqueness of the specified fields, or all fields if
fields is None. fields should be a list of field names.
form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} )
if form.is_valid():
p = form.save(commit=False)
p.user = request.user
p.primary_contact = somecontact
# You probably won't actually want to run model validation this
# way, but hopefully this shows what ModelForm isn't doing.
try:
# Run validation that was missed by the form.
p.validate_fields(fields=['user', 'primary_contact'])
p.validate_unique(fields=['user', 'primary_contact'])
p.validate()
# Or run *all* validation again.
p.full_validate()
except ValidationError, e:
pass
# I don't know how you'd even really recover from error here.
# it's too late to show any form errors. At least you
# know your model is valid before saving though.
p.save()
Thoughts?
Joseph
Ivan Sagalaev
> # Run validation that was missed by the form.
> p.validate_fields(fields=['user', 'primary_contact'])
> p.validate_unique(fields=['user', 'primary_contact'])
> p.validate()
Can this be shortcut to
p.full_validate(fields=['user', 'primary_contact'])
?
If not, why not? :-)
Joseph Kocherhans
Hmm... I guess I'm -0. The caveats with that validate_unique method
are such that I'd rather not abstract it behind something else. I
don't think you'd want to pass the same fields to validate_fields and
validate_unique in most cases. Also, it doesn't make a whole lot of
sense to call validate unless you're validating everything, so we'd
have to document that as well. In practice, I don't think people will
do this a whole lot, so 3 method calls shouldn't be a big deal. We can
always add it later if people really need it in practice.
Joseph
Ben Phillips
Tai Lee
> On Fri, Jan 8, 2010 at 11:59 AM, koenb <koen.bierm...@werk.belgie.be> wrote:
>
> > On 8 jan, 10:03, James Bennett <ubernost...@gmail.com> wrote:
>
> >> Suppose I have a ModelForm and call save(commit=False) to get the
> >> instance so I can do some more work on it. I'm basically saying to
> >> Django "I don't think this object is ready to be saved yet, but I need
> >> the object so I can do stuff to it". If Django goes ahead and does
> >> implicit model validation there and raises an exception, it's just
> >> telling me what I already knew: the object's not ready to be saved.
> >> But now I get to do extra work to catch the exception, and that's bad
> >> and wrong.
>
> >> Calling ModelForm.save(commit=False) should simply disable model
> >> validation, period. If I want to validate the instance before saving
> >> it, I'll validate the instance before saving it, but commit=False is
> >> as clear a way as we have of saying "I'm not going to save this yet"
> >> and model validation should respect that.
I agree with this completely. Calling ModelForm.save(commit=False)
should disable model validation, period. We're explicitly telling
Django not to save the model because it's not ready to be saved,
therefore no model validation needs to be performed.
> > The problem is that in the idiom the is_valid() call on the modelform
> > tries to full_validate the instance, it is not in the save
> > (commit=False) call.
>
> Yes
This is what I think should be changed. ModelForm.is_valid() should
only validate the form. Model validation errors should not be (and I
believe are not currently) returned to the user as form errors,
because they're not form errors and the user can't do anything about
them. They're errors for the developer at the point when he or she is
ready to save a model. Model validation should be moved out of
ModelForm.is_valid() and into ModelForm.save(), but only if
`commit=True`. Otherwise, model validation should be performed when
explicitly requested.
Cheers.
Tai.
Łukasz Rekucki
They're errors for the developer at the point when he or she is
ready to save a model. Model validation should be moved out of
ModelForm.is_valid() and into ModelForm.save(), but only if
`commit=True`. Otherwise, model validation should be performed when
explicitly requested.
Cheers.
Tai.
--
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.
Łukasz Rekucki
Joseph Kocherhans
> A couple of related tickets filed today about model validation:
>
> http://code.djangoproject.com/ticket/12513
> http://code.djangoproject.com/ticket/12521
This has been fixed in r12206 [1]. Could people who had issues please
check things out again and let me (or trac) know if you find any
regressions? I think Honza and I managed to hit most of the ones that
had tickets, but there were quite a few corner cases that had to be
fixed in this changeset. I specifically added a test for the
commit=False idiom, but I'm sure people have more complicated
scenarios out there.
Joseph
Raffaele Salmaso
> regressions?
http://code.djangoproject.com/ticket/12577
--
()_() | That said, I didn't actually _test_ my patch. | +----
(o.o) | That's what users are for! | +---+
'm m' | (Linus Torvalds) | O |
(___) | raffaele dot salmaso at gmail dot com |
Raffaele Salmaso
> Joseph Kocherhans wrote:
>> regressions?
> http://code.djangoproject.com/ticket/12577
Reenabled and everything go as before.
Raffaele Salmaso
> Joseph Kocherhans wrote:
>> regressions?
> http://code.djangoproject.com/ticket/12577
Sorry if I seem rude, but there is a severe regression an no one care to
say at least 'ok I see it', even when there is an *explicit* request for
regressions...
I've resolved with an horrible monkeypatch, but at least I've now a
working copy of django
Joseph Kocherhans
<raffaele...@gmail.com> wrote:
> Raffaele Salmaso wrote:
>> Joseph Kocherhans wrote:
>>> regressions?
>> http://code.djangoproject.com/ticket/12577
> Hello, is anybody out there?
> Sorry if I seem rude, but there is a severe regression an no one care to
> say at least 'ok I see it', even when there is an *explicit* request for
> regressions...
> I've resolved with an horrible monkeypatch, but at least I've now a
> working copy of django
Ok. I see it. ;) Sorry, I've been out of town for a while with no
internet access. 12577 is near the top of my list to look at.
Joseph
Raffaele Salmaso
> Ok. I see it. ;)
> Sorry, I've been out of town for a while with no
> internet access. 12577 is near the top of my list to look at.
ok thanks :D