Ticket #15860: modelform.is_valid() and modelform.errors fail to anticipate database integrity errors, allows exceptions to reach the user
legutierr
redundant, in order to spur discussion. I'm willing to work on a
patch if there is a consensus as to the proper solution...
modelform.is_valid() fails to anticipate database integrity errors
when those errors involve any fields that are not part of that form
itself. Technically, this is because the modelform.validate_unique()
method uses the modelform._get_validation_exclusions() method (which
lists any model fields that are not in the form itself) to define the
exclusions for the call that is made to the ORM object's
validate_unique() method (see here:
http://code.djangoproject.com/browser/django/trunk/django/forms/models.py#L339).
In practical terms this is a bad thing because, in a variety of
circumstances, modelform.is_valid() returning False is the only thing
that will prevent modelform.save() from being called, and
modelform.save() will, in such a case, raise an IntegretyError that
will not be caught. In my opinion, modelform.is_valid() should always
report that a form is NOT valid if it is certain that a call to save()
will raise an exception.
The implementation problem here is either:
1. that modelform._get_validation_exclusions() is too liberal in
its exclusions,
2. that those liberal exclusions should not be passed at all to
instance.validate_unique(), or
3. that the implementation of instance.validate_unique() is using
those exclusions incorrectly.
It seems that the original logic was that model fields that are not
part of the form should be excluded from the decision whether to mark
a form as invalid. But a form *is* invalid if it cannot be saved to
the database, regardless of the reason. Now, an argument can be made
to the effect that model fields which are not form fields are not the
concern of the form and SHOULD cause an IntegrityError to be raised,
but that argument is not entirely relevant: instance.validate_unique()
excludes all validations that reference *any* of the excluded fields,
even if multiple-field constraints include fields that are, in fact,
part of the form. So, if the user changes a field on a form that
combines with another, hidden value to violate a constraint, the user
will see a 404 or exception page, instead of a meaningful error
message explaining how they can fix their mistake.
For me, this is a problem in the case of "unique_together" fields,
where one field is editable on the form, and the other is set at
record creation time or in some other programmatic way. It is
possible, even likely, that a uniqueness constraint will be violated
by a user changing the editable field, causing in the current
implementation an IntegrityError to rise to the top of the stack,
directly impacting the user. Instead, the user should be told that the
data they entered is not sufficiently unique.
Florian Apolloner
On Apr 20, 8:00 am, legutierr <leguti...@gmail.com> wrote:
> modelform.is_valid() fails to anticipate database integrity errors
> when those errors involve any fields that are not part of that form
> itself.
class SomeForm(ModelForm):
class Meta:
exclude = ['user']
model = …
Now in the view I do this:
if form.is_valid():
instance = form.save(commit=False)
instance.user = request.user
Now assume user is a not nullable Foreignkey. In that case the
is_valid on the form would throw an error -> BAD. The issue you
describe (fields not beeing run through model validation if not on the
form) got "added" after adding model validation, since the code I
describe is pretty common and would have broken all those apps. I
suggest you read the history on the development of modelvalidation;
especially: http://groups.google.com/group/django-developers/browse_thread/thread/cee43f109e0cf6b
Btw, just my humble opinion: If you exclude fields from the ModelForm
it's your duty to check for valid data.
Regards,
Florian
legutierr
> > modelform.is_valid() fails to anticipate database integrity errors
> > when those errors involve any fields that are not part of that form
> > itself.
>
> That is wanted behaviour, eg consider my workflow:
>
> class SomeForm(ModelForm):
> class Meta:
> exclude = ['user']
> model = …
>
> Now in the view I do this:
> if form.is_valid():
> instance = form.save(commit=False)
> instance.user = request.user
would be *incorrect* to individually validate fields that are excluded
from the form in the `is_valid()` model validation, especially in
light of this common idiom (which I do use myself). I think that I
described my problem too generically above; nonetheless, I do believe
that there is *still* something wrong with the current
implementation. I have modified the subject of this thread to reflect
real source of my complaint, which is described in the last paragraph
of my original ticket:
> For me, this is a problem in the case of "unique_together" fields,
> where one field is editable on the form, and the other is set at
> record creation time or in some other programmatic way. It is
> possible, even likely, that a uniqueness constraint will be violated
> by a user changing the editable field, causing in the current
> implementation an IntegrityError to rise to the top of the stack,
> directly impacting the user. Instead, the user should be told that the
> data they entered is not sufficiently unique.
referenced, it is (1) that users of a form should not be presented
with a validation error that they are unable to fix by modifying
submission data, and (2) that developers should get directly involved
if there is any error being generated at form submission time that is
out of the control of the user. In other words, it is a GOOD thing for
IntegrityError to be raised if instance data that is excluded from the
form violates a constraint. I think that the argument is convincing,
and I agree with all of these sentiments.
However, in the case of a tuple of fields that are "unique together",
the proper behavior should be that if *any* of those fields are
editable in the form, the constraint should be validated by
is_valid(). In the current implementation, *all* of the unique-
together fields have to be editable for the constraint to be
validated; THAT is the real bug here. It is always fully within the
power of a user to resolve a "unique together" constraint violation
simply by modifying any one of the fields subject to that constraint;
the only thing required is that the user be told which editable
field(s) are causing the constraint violation. Furthermore, such
violations are not uncommon, and should not require the intervention
of the developer.
Note that this is not just a problem for me, but is also a problem at
the root of several tickets that have already been accepted, several
over a year old. These two open tickets pertain to admin and content
types, and have the same root cause as I am describing above:
http://code.djangoproject.com/ticket/12028
http://code.djangoproject.com/ticket/13091
also:
http://code.djangoproject.com/ticket/13249
http://code.djangoproject.com/ticket/15326
Something to take note of regarding #13091, which seems to be the
canonical ticket: the current patch attached to this ticket would
eliminate *all* exclusions from the call to the validate_unique()
model validations. This I now disagree with (as I am sure you do,
too), although I originally proposed doing just that. I also think
that in the case of a "unique together" tuple where *none* of the
fields are editable, model validation should be skipped inside
is_valid(). However, I do think that a modification is warranted to
resolve the underlying error of the above-listed tickets, as well as
my own.
> Btw, just my humble opinion: If you exclude fields from the ModelForm
> it's your duty to check for valid data.
`unique_together`, you should relax this standard. In the case of
content-types and generic fields, it is very common to exclude the
content-type and object-id fields form the forms, but to also group
the content-type and object-id fields with some other editable field
in defining a "unique_together" constraint. In such a case, requiring
a developer to write boilerplate code to validate uniqueness seems
inconvenient, counterintuitive and kind of difficult, actually (IMHO,
of course). Furthermore, without adding a whole bunch of complex
special-case code to the admin, the current conflict between
"list_editable" and "unique_together" is only solvable with the kind
of change I am proposing be made to model validation.
legutierr
Carl Meyer
On 04/22/2011 02:42 AM, legutierr wrote:
> However, in the case of a tuple of fields that are "unique together",
> the proper behavior should be that if *any* of those fields are
> editable in the form, the constraint should be validated by
> is_valid(). In the current implementation, *all* of the unique-
> together fields have to be editable for the constraint to be
> validated; THAT is the real bug here. It is always fully within the
> power of a user to resolve a "unique together" constraint violation
> simply by modifying any one of the fields subject to that constraint;
> the only thing required is that the user be told which editable
> field(s) are causing the constraint violation. Furthermore, such
> violations are not uncommon, and should not require the intervention
> of the developer.
>
> Note that this is not just a problem for me, but is also a problem at
> the root of several tickets that have already been accepted, several
> over a year old. These two open tickets pertain to admin and content
> types, and have the same root cause as I am describing above:
>
> http://code.djangoproject.com/ticket/12028
> http://code.djangoproject.com/ticket/13091
>
> also:
>
> http://code.djangoproject.com/ticket/13249
> http://code.djangoproject.com/ticket/15326
Having reviewed all these related tickets, I do think that in _many_
cases it would be useful and expected to have ModelForm validate a
unique_together constraint if any of the unique_together fields are
included in the ModelForm, as you're proposing. I don't think it's
cut-and-dried, though -- I can imagine cases where the new behavior
would be wrong. For instance, this model:
class FavoriteBirthdayPresent(models.Model):
username = models.CharField(max_length=30)
year = models.IntegerField()
description = models.CharField(max_length=200)
class Meta:
unique_together = [["username", "year"]]
and this ModelForm:
class FavoriteBirthdayPresentForm(forms.ModelForm):
class Meta:
model = FavoriteBirthdayPresent
exclude = ["username"]
and this view code:
if form.is_valid():
fave = form.save(commit=False)
fave.username = request.user.username
fave.save()
With your proposed change, if I happen to have a FavoriteBirthdayPresent
in the database for a given year with an empty "username" field, it
would incorrectly prevent any user from creating their own
FavoriteBirthdayPresent for that year.
I'll readily admit that's a corner case that requires several
perhaps-unusual conditions:
- the excluded field that's part of a unique_together has a default
value which is also a valid value in the database, and is not None/NULL
(because NULL != NULL in SQL), and
- there actually might be a record in the database with that default value.
And I think there are probably many more cases where your proposed
behavior would be correct. I'd just be happier marking #13091 Accepted
if I could see a solution that seemed more clearly correct in all cases.
This is really giving me the itch to build a new context-manager-based
idiom for ModelForm validation in 1.4 that would allow modification of
the to-be-saved object within the context manager and always perform
full validation of the model on the way out. This idea was raised
briefly in discussions right around the time model-validation landed,
but was tabled due to the need (at that point) to support Python 2.4.
Seems like that could neatly resolve all these knotted issues around
validation of partial ModelForms.
> Something to take note of regarding #13091, which seems to be the
> canonical ticket: the current patch attached to this ticket would
> eliminate *all* exclusions from the call to the validate_unique()
> model validations. This I now disagree with (as I am sure you do,
> too), although I originally proposed doing just that. I also think
> that in the case of a "unique together" tuple where *none* of the
> fields are editable, model validation should be skipped inside
> is_valid(). However, I do think that a modification is warranted to
> resolve the underlying error of the above-listed tickets, as well as
> my own.
Yes, I agree that the current patch on #13091 is too aggressive.
Carl
Florian Apolloner
> This is really giving me the itch to build a new context-manager-based
> idiom for ModelForm validation in 1.4 that would allow modification of
> the to-be-saved object within the context manager and always perform
> full validation of the model on the way out. This idea was raised
> briefly in discussions right around the time model-validation landed,
> but was tabled due to the need (at that point) to support Python 2.4.
> Seems like that could neatly resolve all these knotted issues around
> validation of partial ModelForms.
regarding modelforms lately was that I wanted something like: "If you
don't fill out the user it's set to the current user", in the admin,
with as little modifications as possible. So save_model sounded like
the right spot but modelvalidation will make the form invalid (I want
the user field on the form…). Still no idea on how to solve that
properly and nicely; if the context managers can take care of that too
(though I doubt it since I can't change the is_valid call in the
admin) I am all in for it. Btw any threads around that subject I could
read?
Regards, Florian
Carl Meyer
On 04/23/2011 08:18 AM, Florian Apolloner wrote:
> Nice, something like that would be great. One of my bigger problems
> regarding modelforms lately was that I wanted something like: "If you
> don't fill out the user it's set to the current user", in the admin,
> with as little modifications as possible. So save_model sounded like
> the right spot but modelvalidation will make the form invalid (I want
> the user field on the form�). Still no idea on how to solve that
> properly and nicely; if the context managers can take care of that too
> (though I doubt it since I can't change the is_valid call in the
> admin) I am all in for it. Btw any threads around that subject I could
> read?
http://groups.google.com/group/django-developers/msg/3014f29c5125653a is
where it was briefly mentioned by Lukasz, I haven't seen any discussion
since.
As Joseph says later in that thread, two design constraints of model
validation were to avoid double-validation and avoid storing metadata on
the model about which validations have already been run. I think a
context manager might be able to avoid that problem by storing data
about which validations have been run temporarily on the context manager
itself, rather than on the model. This might allow form-validation on
the way in, full model validation on the way out. If there are problems
with that, it might work to just do no validation on the way in and full
model validation on the way out, though it'd be nicer if the code inside
the context manager could rely on at least validation of the fields
included in the form.
In any case, if we have this, I could see switching the admin to use it,
and perhaps adding an overridable method that's called from within the
context manager, to allow you to complete/tweak the model instance
before full validation is run. Seems like this would solve your problem?
Carl
Florian Apolloner
On Apr 23, 5:55 pm, Carl Meyer <c...@oddbird.net> wrote:
> http://groups.google.com/group/django-developers/msg/3014f29c5125653ais
> since.
> In any case, if we have this, I could see switching the admin to use it,
> and perhaps adding an overridable method that's called from within the
> context manager, to allow you to complete/tweak the model instance
> before full validation is run. Seems like this would solve your problem?
Regards,
Florian
Florian Apolloner
> > In any case, if we have this, I could see switching the admin to use it,
> > and perhaps adding an overridable method that's called from within the
> > context manager, to allow you to complete/tweak the model instance
> > before full validation is run. Seems like this would solve your problem?
>
> Hell yeah, that would solve all of my problems :þ
end (in my case the user)
Mikhail Korobov
legutierr
> With your proposed change, if I happen to have a FavoriteBirthdayPresent
> in the database for a given year with an empty "username" field, it
> would incorrectly prevent any user from creating their own
> FavoriteBirthdayPresent for that year.
>
> I'll readily admit that's a corner case that requires several
> perhaps-unusual conditions:
> - the excluded field that's part of a unique_together has a default
> value which is also a valid value in the database, and is not None/NULL
> (because NULL != NULL in SQL), and
> - there actually might be a record in the database with that default value.
>
> And I think there are probably many more cases where your proposed
> behavior would be correct. I'd just be happier marking #13091 Accepted
> if I could see a solution that seemed more clearly correct in all cases.
1) It would be feasible to treat the case where a default value is
defined on a field (or where the field is allowed to be null) as being
distinct from the case where the default value is not defined and the
field is not permitted to be null. In other words, in the case that
you cite the current behavior could be maintained (unique_together
tuples containing any field with a default value would be ignored in
model validation), while my proposed behavior would only be
implemented when model validation is certain not to create the
circumstances you describe. I would be happy to write a patch for
this + tests if you are OK with the approach.
2) I am not sure, however, that the case you site is really a
problem. So what if the user is told that the "year" data they have
supplied in such a case is not "sufficiently unique"? It would be
true (would it not?) that the default "username" would already have
their favorite birthday present assigned for that year (even if the
default is null), and it seems to me that such a fact is intelligible
to the user (The error message could read: "The data you supplied for
field 'year' is not sufficiently unique for username 'default'," or
perhaps simply "The 'year' you specified is not sufficiently
unique."). It would also be fully within the power of the user to
modify the form in order to get it to pass model validation and be
saved. Maybe this is a fine distinction, but I think that saying that
a field or set of fields is not "sufficiently unique" is perfectly
sufficient from a usability point of view. It is a lot better than
raising an exception after saving, and if the error message isn't good
enough in any circumstance, it would be as easy as it is now to
override it programmatically.
> This is really giving me the itch to build a new context-manager-based
> idiom for ModelForm validation in 1.4 that would allow modification of
> the to-be-saved object within the context manager and always perform
> full validation of the model on the way out. This idea was raised
> briefly in discussions right around the time model-validation landed,
> but was tabled due to the need (at that point) to support Python 2.4.
> Seems like that could neatly resolve all these knotted issues around
> validation of partial ModelForms.
the current approach, but unfortunately, I think that what you are
describing is a little over my head. Regardless, I hope that the
prospect of introducing this new idiom will not prevent #12082 and
#13091 from being resolved without the acceptance of a new approach.
While we are on the subject of new idioms, I am curious as to what
might be wrong with this slight amendment to the current idiom:
form = ModelForm(data)
form.instance.excluded_field = programatic_data
if form.is_valid():
form.save()
Is there some reason why programatic data needs to be assigned to the
form instance *after* validation takes place? Is there something
about the instance that is returned by form.save(commit=False) that
distinguishes it from form.instance in such a way that it would break
form.is_valid() or form.save()?
Regards,
Eduardo
Carl Meyer
Hmm, that's interesting. I'm not super-enthused about the complexity
there (Zen of Python: "if the implementation is hard to explain, it's a
bad idea"), but I think you're right that it's feasible. Note that
nullable fields would be ok to go ahead with (because NULL is not equal
to NULL in SQL, it won't cause false positives on the uniqueness check);
it's just fields with non-null defaults that could cause the false
positive if they are excluded from the form but included in a
unique-together check.
If the implementation (and documentation) for that patch doesn't look
too terrible, I'd consider it - I do think it gets the behavior closer
to right than what we do now, and I'm not sure it's really possible to
get it fully right in all cases as long as we're trying to do validation
on a partial model. I'd be interested in others' thoughts, of course.
> 2) I am not sure, however, that the case you site is really a
> problem. So what if the user is told that the "year" data they have
> supplied in such a case is not "sufficiently unique"? It would be
> true (would it not?) that the default "username" would already have
> their favorite birthday present assigned for that year (even if the
> default is null), and it seems to me that such a fact is intelligible
> to the user (The error message could read: "The data you supplied for
> field 'year' is not sufficiently unique for username 'default'," or
> perhaps simply "The 'year' you specified is not sufficiently
> unique.").
That error message is not intelligible to the user, because they aren't
trying to save data for the user "default" at all, they are trying to
save data for their own user (and they can't change the user, because
its excluded from the form and assigned in the view code after
validation). The error is wrong because its checking uniqueness for user
"default" when it ought to be checking for user "janedoe", and it
prevents Jane from saving data for herself for any year that "default"
has data for.
It would also be fully within the power of the user to
> modify the form in order to get it to pass model validation and be
> saved.
Only if they give up on saving any data for themselves for that year.
>> This is really giving me the itch to build a new context-manager-based
>> idiom for ModelForm validation in 1.4 that would allow modification of
>> the to-be-saved object within the context manager and always perform
>> full validation of the model on the way out. This idea was raised
>> briefly in discussions right around the time model-validation landed,
>> but was tabled due to the need (at that point) to support Python 2.4.
>> Seems like that could neatly resolve all these knotted issues around
>> validation of partial ModelForms.
>
> I am sure that whatever idiom you create will be an improvement over
> the current approach, but unfortunately, I think that what you are
> describing is a little over my head. Regardless, I hope that the
> prospect of introducing this new idiom will not prevent #12082 and
> #13091 from being resolved without the acceptance of a new approach.
I don't think it needs to.
> While we are on the subject of new idioms, I am curious as to what
> might be wrong with this slight amendment to the current idiom:
>
> form = ModelForm(data)
> form.instance.excluded_field = programatic_data
> if form.is_valid():
> form.save()
Nothing's wrong with that (except that it's a bit more verbose). In
fact, the context-manager approach I mentioned would really just be a
cleaner and documented way of doing pretty close to this.
But form.save(commit=False) after validation is documented and works
now, and is commonly used, so we still need to be careful not to break it.
Carl
legutierr
> Hmm, that's interesting. I'm not super-enthused about the complexity
> there (Zen of Python: "if the implementation is hard to explain, it's a
> bad idea"), but I think you're right that it's feasible. Note that
> nullable fields would be ok to go ahead with (because NULL is not equal
> to NULL in SQL, it won't cause false positives on the uniqueness check);
> it's just fields with non-null defaults that could cause the false
> positive if they are excluded from the form but included in a
> unique-together check.
>
> If the implementation (and documentation) for that patch doesn't look
> too terrible, I'd consider it - I do think it gets the behavior closer
> to right than what we do now, and I'm not sure it's really possible to
> get it fully right in all cases as long as we're trying to do validation
> on a partial model. I'd be interested in others' thoughts, of course.
hopefully works for you. I don't think it will be very complicated
implementation-wise, just a few additional lines, I think. With
regards to the documentation, I'll add a note here:
http://docs.djangoproject.com/en/1.3/ref/models/options/#unique-together
and here:
http://docs.djangoproject.com/en/1.3/ref/models/instances/#django.db.models.Model.validate_unique
Including a note saying that the behavior has changed
Regards,
Eduardo
Carl Meyer
On 04/27/2011 02:02 PM, legutierr wrote:
> Ok, I'll create a patch soon (with tests + documentation) that
> hopefully works for you. I don't think it will be very complicated
> implementation-wise, just a few additional lines, I think. With
> regards to the documentation, I'll add a note here:
>
> http://docs.djangoproject.com/en/1.3/ref/models/options/#unique-together
>
> and here:
>
> http://docs.djangoproject.com/en/1.3/ref/models/instances/#django.db.models.Model.validate_unique
>
> Including a note saying that the behavior has changed
Great, thanks. I think this behavior change only needs to be described
in one place (the validate_unique docs), but the text at the former link
is actually inaccurate ever since model validation - it should be
updated to mention that unique_together is also checked by model
validation, with a link to the validate_unique docs.
Carl
legutierr
with this discussion. As I note in the ticket, I kept the tests from
the prior patch, which tests were specifically relevant to the admin
as reported by the original ticket, but I also added additional, more
focused tests. I also had to modify one of the model_formsets tests,
as it was assuming the old behavior.
I ran my changes against the whole of the Django test suite, and no
relevant errors seem to have been generated.
I hope this is OK to check in :)
Regards,
Eduardo
On Apr 27, 3:13 pm, Carl Meyer <c...@oddbird.net> wrote:
> On 04/27/2011 02:02 PM, legutierr wrote:
>
> > Ok, I'll create a patch soon (with tests + documentation) that
> > hopefully works for you. I don't think it will be very complicated
> > implementation-wise, just a few additional lines, I think. With
> > regards to the documentation, I'll add a note here:
>
> >http://docs.djangoproject.com/en/1.3/ref/models/options/#unique-together
>
> > and here:
>
legutierr
legutierr
http://code.djangoproject.com/ticket/13091
Regards,
Eduardo
Carl Meyer
On 04/28/2011 12:35 PM, legutierr wrote:
> I just added a new patch in response to Carl's comments on the ticket.
>
> http://code.djangoproject.com/ticket/13091
So, in the process of reviewing and tweaking this patch for commit, I
checked in the #django-dev IRC channel for any other core dev opinionsm
since I didn't feel 100% confident that we were doing the right thing
here. I talked through the issue with Alex Gaynor, and he successfully
convinced me that we aren't. Specifically:
1. We have an idea in mind, as I mentioned above, for a new
modelform-validation idiom that would solve this problem fully, by
requiring tweaks to the model to happen pre-validation and then
validating the full model.
2. If we implement the new idiom, and convert the admin to use it, then
anyone who runs into the problems with the current partial-validation
scheme in their own code can simply switch to the new recommended idiom.
Nobody will be stuck.
3. The current proposed patch on #13091 only improves the current
situation very marginally; there are a lot of cases it still wouldn't
catch (anytime a field involved in a unique-together is modified
post-validation and pre-save, and the odd exclusions for default/blank
fields). It's very much an incomplete fix, and yet it introduces new
complexity both to the code and the documentation, when we already have
a better alternative fix in mind.
So I apologize for leading you to spend time on that patch and then
switching gears. In terms of what's best for Django, I think Alex is
right on this one, so I plan to just work on the better fix rather than
committing the current patch on #13091.
Carl
legutierr
it, I don't want to waste the momentum. What's the approach you are
thinking of, and how can I get started in the implementation?
Carl Meyer
On 04/28/2011 06:36 PM, legutierr wrote:
> This is extraordinarily discouraging.
I can understand why.
I've also spent a number of hours thinking about this, reviewing the
patch, considering alternatives, coming up with cases that might break,
etc. I'd like to set aside those sunk costs (which I don't think were
wasted in either case) and keep the focus on the best way to solve the
issue in Django moving forward - that's what I owe to the rest of the
core development team and to the community.
> This is the second time that I
> have devoted tremendous energy to a patch, trying to coordinate with
> core developers, not doing any work until I get the green light from
> core developers regarding an implementation plan (trying to avoid this
> very same eventuality), only to be told, after working code + tests +
> docs have been attached to the ticket, after several iterations of
> feedback: nope, this is not the way that we want to do this policy-
> wise, there's this other approach we want to take, so never mind.
I'm not certain what the other situation is that you're referring to, so
I can't speak to that. My observation has been that this isn't the
common experience (unfortunately, getting no attention to a bug/patch in
the first place has at times been a more common one, though that too is
getting better -- unreviewed bug count is currently zero!), but I'm
sorry you've experienced it, and I regret having contributed to it in
this case. I will certainly be more careful in the future about
expressing optimism that an approach might be workable, especially if
(as in this case) I have reservations about it from the start.
> I can understand going through the bureaucratic rigmarole that comes
> with contributing to Django--in fact, I support it--but to go through
> all of the discussion, justification, and *time* required to get a
> simple bug fix checked in (no, this really *is* a bug--look, there are
> five other tickets filed. sure, let's analyze the problem from every
> angle. sure, I'll rewrite it so it matches exactly your
> specifications.), only to be told that someone who wasn't even
> involved in this ticket and discussion *at all* until now thinks it
> isn't worth it, makes a guy like me want to tear his hair out. You
> say that this is "in the best interests of Django", but you must know
> that Django will suffer if people like me stop wanting to contribute
> because of things like this.
Indeed, and I hope that you don't lose interest in contributing.
I don't think that the time spent discussing and analyzing this, even
writing and reviewing a patch, is wasted. From my perspective, it has
clearly revealed that the current approach of trying to do
partial-model-validation is broken in concept and not reliably fixable.
That's useful information, and moves us (has already moved us) towards a
better solution.
I can't agree that this is a simple bug fix. The current behavior is
wrong in some cases. The behavior after this "fix" would still be wrong
in some cases, although fewer. A simple bug fix is one where the fix is
clear, obviously correct, and definitively solves the reported problem.
I don't think that describes this case. Model and form validation is a
complex area, and it's easy for seemingly small changes to have
unintended effects that cause more maintenance burdens down the road.
> How often has it been the case that some really cool new feature gets
> delayed because the core developer who was working on it was unable to
> dedicate the time they thought they would be able to? Can I help move
> it along, can you work with me to write it? Why can't we check this
> one in, close two tickets (as well as satisfying three or four
> duplicates) and then move on to the more definitive fix?
I'm committed to having these tickets closed one way or another before
Django 1.4 is released (and neither fix here would be a candidate for
backporting to a 1.3.X release anyway), so let's focus on making the
best fix we can. If the ideas we have in mind for that turn out to be
unworkable for some reason, I still think that the current patch would
be preferable to no change.
Carl
Carl Meyer
> I'm up for working on the new idiom now. I've put this much time into
> it, I don't want to waste the momentum. What's the approach you are
> thinking of, and how can I get started in the implementation?
Much appreciated. I have a half-written post about exactly that to begin
a new thread - should be up shortly!
Carl
Carl Meyer
another example of why attempting to do partial model validation is
simply broken. I think the context manager proposal (which I've posted
in a new thread) does fix this case as well, and I'd be happy for your
comments on it.
Carl
Carl Meyer
(moved from another thread)
On Apr 29, 12:16 am, Lior Sion <lior.s...@gmail.com> wrote:
> I looked at the sample you wrote on the other thread (unique together
> on username and date, and having a null username with a given date) of
> when the old behavior is the right one and it didn't quite convince me
> - I do believe that the right implementation would fail a case of
> NULLed username and repeating dates, when a unique together exists.
That would clearly violate the currently-established consistent
semantic for modelform validation, which is that the modelform only
validates the fields that are actually included in the form. Any
fields that are not included in the form, it must be assumed that they
might change before saving, and their current value is unreliable (it
might just be the default value for a new object, or something else
entirely, it's not necessarily a value that's actually intended to be
validated or saved). In essence, what you are proposing is validation
of "possibly-junk" data.
Because of this, any fix has to be absolutely 100% sure that it will
never generate a false positive on the unique_together check because
of the non-included field's value, or else we're violating the
documented expectation that non-included fields' values are not part
of validation. The exceptions for default and blank in the current
patch was my best attempt to achieve that, but I don't even think
that's sufficient - really, modelform validation can't assume anything
about the data on self.instance for fields that aren't in the form,
regardless of whether the field is blank or has a default.
This is why I'm saying that the current expected semantics, that a
modelform can provide meaningful validation while ignoring some fields
of the model, is inherently broken. No matter what you do with
constraints that include some included and some excluded fields
(whether they are unique_together or custom clean methods, as in
Mikhail's example), you're doing the wrong thing - you can't validate
the constraint because you can't use some of the data that's needed to
validate it, but if you drop the constraint entirely (as we do now)
you require the developer to re-validate everything themselves later
(and probably stuff those later model-validation errors back into the
form, too, and then break out of the is_valid clause because its no
longer valid), which is really ugly and painful in custom code and
currently not done at all in the admin.
And that's why I think the only long-term solution, that isn't just
going to cause more problems, is to begin moving towards a new
expectation for validating modelforms, where its expected that full
model validation is run, and any tweaks to the model must be made
before validation rather than after.
Carl
legutierr
Thanks for this professional reply. After rereading my post
(immediately after submitting it), I realized that I was much more
critical than I would normally think is fair, which is why I removed
it. It's sometimes necessary, I think, to remind ourselves that most
of us are volunteers here, including the core developers. I also have
to say that you have made yourself available in a very generous manner
of late; for that I am quite appreciative.
Regards,
Ed Gutierrez