Discussion:
Model validation incompatibility with existing Django idioms
(too old to reply)
Simon Willison
2010-01-06 18:49:18 UTC
Permalink
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.

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
2010-01-06 19:54:00 UTC
Permalink
Post by 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
form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} )
   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.
Post by Simon Willison
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
2010-01-06 21:26:25 UTC
Permalink
I've only scanned the docs the other day and haven't actually used the
new model validation stuff, so my impressions may be a little off,
but...
Post by Joseph Kocherhans
Post by 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
form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} )
   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]
Post by Joseph Kocherhans
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).
Post by Joseph Kocherhans
One (probably unhelpful) way to make ModelForm validate your whole
           # 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
2010-01-06 21:48:52 UTC
Permalink
First time posting here, so please excuse me if my opinions aren't very
useful and my bad English.
Post by Waylan Limberg
I've only scanned the docs the other day and haven't actually used the
new model validation stuff, so my impressions may be a little off,
but...
Post by Joseph Kocherhans
Post by 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
form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} )
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.
form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} )
p = form.save(commit=False)
p.user = request.user
p.primary_contact = somecontact
if p.full_validate(): # <<<<< note this line
p.save()
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.
Post by Waylan Limberg
[snip]
Post by Joseph Kocherhans
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).
Post by Joseph Kocherhans
One (probably unhelpful) way to make ModelForm validate your whole
# 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)``?
This seems like a good idea.
Post by Waylan Limberg
--
----
\X/ /-\ `/ |_ /-\ |\|
Waylan Limberg
--
You received this message because you are subscribed to the Google Groups
"Django developers" group.
To unsubscribe from this group, send email to
.
For more options, visit this group at
http://groups.google.com/group/django-developers?hl=en.
--
Łukasz Rekucki
Brian Rosner
2010-01-06 22:09:09 UTC
Permalink
Post by Łukasz Rekucki
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.

Brian Rosner
http://oebfare.com
http://twitter.com/brosner
Joseph Kocherhans
2010-01-06 21:56:30 UTC
Permalink
Post by Waylan Limberg
I've only scanned the docs the other day and haven't actually used the
new model validation stuff, so my impressions may be a little off,
but...
Post by Joseph Kocherhans
Post by 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
form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} )
   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.
   form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} )
       p = form.save(commit=False)
       p.user = request.user
       p.primary_contact = somecontact
       if p.full_validate():        # <<<<< note this line
           p.save()
[snip]
There are a couple of problems with p.full_validate() there. First, it
would run validation a second time. Honza went to a bunch of trouble
in the design to make sure that each field would only need to be
validated once. Second, p.full_validate() would raise
ValidationErrors, not return True or False.
Post by Waylan Limberg
Post by Joseph Kocherhans
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).
Post by Joseph Kocherhans
One (probably unhelpful) way to make ModelForm validate your whole
           # Defaults to False
           validate_model = True
Well, what if one view uses that ModelForm one way and another view
uses the same ModelForm the other way? What about
``form.is_valid(validate_model=True)``?
That's a possibility, but I think it suffers from the same problems
that the Meta option does. It just pushes the decision closer to
runtime than coding time. That's helpful in some cases, but it doesn't
solve the main part of the problem for me, which is that I don't think
model validation should be an opt-in-only thing. Maybe it needs to be
for a couple of releases though.

I had another idea that I think might work out. What if
ModelForm.validate() only validated fields on the form, like it worked
before the merge, and ModelForm.save() would automatically validate
the excluded fields, raising ValidationError if there was a problem?
Validation for each field would only happen once, it would accommodate
the commit=False idiom, and it would still ensure that the model
itself is validated in common usage.

I think this *might* also lead to a workable solution for ticket
#12507 [1], but I need to dig into the code a little more.

Joseph

[1] http://code.djangoproject.com/ticket/12507
Brian Rosner
2010-01-06 22:06:53 UTC
Permalink
Post by Joseph Kocherhans
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
2010-01-06 22:08:23 UTC
Permalink
Post by Brian Rosner
Post by Joseph Kocherhans
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
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
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
Jeremy Dunck
2010-01-06 22:46:50 UTC
Permalink
On Wed, Jan 6, 2010 at 3:56 PM, Joseph Kocherhans <jkocherhans-***@public.gmane.org> wrote:
...
...
Post by Joseph Kocherhans
Post by Simon Willison
form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} )
   p = form.save(commit=False)
   p.user = request.user
   p.primary_contact = somecontact
   p.save()
...
Post by Joseph Kocherhans
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
2010-01-06 22:57:26 UTC
Permalink
Post by Jeremy Dunck
...
...
Post by Joseph Kocherhans
Post by Simon Willison
form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} )
p = form.save(commit=False)
p.user = request.user
p.primary_contact = somecontact
p.save()
...
Post by Joseph Kocherhans
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?
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.

Brian Rosner
http://oebfare.com
http://twitter.com/brosner
Brian Rosner
2010-01-06 23:00:55 UTC
Permalink
Post by 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.
Actually, I just realized that Model.save doesn't do validation now
does it?

Brian Rosner
http://oebfare.com
http://twitter.com/brosner
Joseph Kocherhans
2010-01-06 22:59:28 UTC
Permalink
Post by Jeremy Dunck
...
...
Post by Joseph Kocherhans
Post by Simon Willison
form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} )
   p = form.save(commit=False)
   p.user = request.user
   p.primary_contact = somecontact
   p.save()
...
Post by Joseph Kocherhans
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?
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
2010-01-06 23:06:17 UTC
Permalink
Post by Joseph Kocherhans
Post by Jeremy Dunck
...
...
Post by Joseph Kocherhans
Post by Simon Willison
form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} )
p = form.save(commit=False)
p.user = request.user
p.primary_contact = somecontact
p.save()
...
Post by Joseph Kocherhans
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?
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.
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.

Brian Rosner
http://oebfare.com
http://twitter.com/brosner
Tai Lee
2010-01-07 05:29:29 UTC
Permalink
It makes sense to me that the developer should first check that their
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
2010-01-07 16:40:56 UTC
Permalink
Hi everybody, sorry for the late reply, was busy.


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.Kral-***@public.gmane.org
Phone: +420 606 678585
David Cramer
2010-01-08 04:40:58 UTC
Permalink
For us we definitely use this behavior, and I'm guessing this is about
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?
Post by Honza Král
Hi everybody, sorry for the late reply, was busy.
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.
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?
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
Phone:  +420 606 678585
Skylar Saveland
2010-01-08 07:36:15 UTC
Permalink
Post by Honza Král
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.
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
Post by Honza Král
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
Post by Honza Král
only when editing (and not adding) models and have other method (or
enforced commit=False) for retrieval of the partial model
.
Post by Honza Král
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.
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()?
Post by Honza Král
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.
Post by Honza Král
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
2010-01-08 09:03:00 UTC
Permalink
Post by Honza Král
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.
Post by Honza Král
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
2010-01-08 10:59:22 UTC
Permalink
Post by James Bennett
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
2010-01-08 13:36:10 UTC
Permalink
Post by koenb
Post by James Bennett
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).
Post by koenb
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
Post by koenb
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
2010-01-08 14:43:33 UTC
Permalink
Post by Honza Král
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
Tai Lee
2010-01-10 05:42:38 UTC
Permalink
Post by koenb
Post by James Bennett
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.
Post by koenb
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
2010-01-10 13:02:29 UTC
Permalink
Post by Tai Lee
Post by koenb
Post by James Bennett
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.
Post by koenb
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.
This is only true as long as we're talking about simple validators. If there
are constraints on the model that include more then one field - one which
comes from the form + a generated one - then returning it as an error may be
useful. Only validating the form is probably a good strategy, but I think
there should be an easy way of returning model validation errors to the
user.
Post by Tai Lee
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 unsubscribe from this group, send email to
.
For more options, visit this group at
http://groups.google.com/group/django-developers?hl=en.
--
Łukasz Rekucki
Tobias McNulty
2010-01-08 15:11:23 UTC
Permalink
Post by James Bennett
Post by Honza Král
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.
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,
Tobias
--
Tobias McNulty
Caktus Consulting Group, LLC
P.O. Box 1454
Carrboro, NC 27510
(919) 951-0052
http://www.caktusgroup.com
Ivan Sagalaev
2010-01-09 12:46:02 UTC
Permalink
Post by Tobias McNulty
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...
Post by Tobias McNulty
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
2010-01-09 20:08:22 UTC
Permalink
Post by Tobias McNulty
I too would opt for an implementation that makes model validation optional,
Post by Tobias McNulty
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.
I don't see why model validation should be bound up with forms at all. 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.

Again - pardon my ignorance if there's something that I'm missing. I was
just alarmed to find a thread about forms + model validation given the
current state of the release notes.

Tobias
--
Tobias McNulty
Caktus Consulting Group, LLC
P.O. Box 1454
Carrboro, NC 27510
(919) 951-0052
http://www.caktusgroup.com
Ivan Sagalaev
2010-01-09 20:22:33 UTC
Permalink
Post by Tobias McNulty
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.
Post by Tobias McNulty
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
2010-01-09 23:19:48 UTC
Permalink
Post by 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
form = SecretQuestionForm( {"secret_question":"foo", "answer":"bar"} )
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

[1] http://code.djangoproject.com/ticket/12521
Ivan Sagalaev
2010-01-10 00:25:29 UTC
Permalink
Post by Joseph Kocherhans
# 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
2010-01-10 00:33:47 UTC
Permalink
On Sat, Jan 9, 2010 at 6:25 PM, Ivan Sagalaev
Post by 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? :-)
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
2010-01-10 01:29:02 UTC
Permalink
-----Original Message-----
From: Ivan Sagalaev <***@softwaremaniacs.org>
Date: Sun, 10 Jan 2010 03:25:29
To: <django-***@googlegroups.com>
Subject: Re: Model validation incompatibility with existing Django idioms
Post by Joseph Kocherhans
# 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
2010-01-12 02:38:57 UTC
Permalink
Post by Simon Willison
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

[1] http://code.djangoproject.com/changeset/12206
Raffaele Salmaso
2010-01-12 09:06:08 UTC
Permalink
Post by Joseph Kocherhans
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
2010-01-14 17:27:22 UTC
Permalink
Post by Raffaele Salmaso
Post by Joseph Kocherhans
regressions?
http://code.djangoproject.com/ticket/12577
Ok, BaseGenericInlineFormSet doesn't have save_new to save the generic fk.pk
Reenabled and everything go as before.
--
()_() | 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
2010-01-19 07:04:39 UTC
Permalink
Post by Raffaele Salmaso
Post by Joseph Kocherhans
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
--
()_() | 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 |
Joseph Kocherhans
2010-01-20 15:28:59 UTC
Permalink
On Tue, Jan 19, 2010 at 1:04 AM, Raffaele Salmaso
Post by Raffaele Salmaso
Post by Raffaele Salmaso
Post by Joseph Kocherhans
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
2010-01-20 15:35:25 UTC
Permalink
Post by Joseph Kocherhans
Ok. I see it. ;)
:D
Post by Joseph Kocherhans
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
--
()_() | 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 |
Loading...