Discussion:
Ticket #15860: modelform.is_valid() and modelform.errors fail to anticipate database integrity errors, allows exceptions to reach the user
(too old to reply)
legutierr
2011-04-20 06:00:39 UTC
Permalink
I hope it's OK to copy the body of the ticket here, even if it is
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.
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
To unsubscribe from this group, send email to django-developers+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Florian Apolloner
2011-04-20 08:56:35 UTC
Permalink
Hi,
Post by 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

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
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
To unsubscribe from this group, send email to django-developers+***@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
legutierr
2011-04-22 07:42:09 UTC
Permalink
Thanks for your response, Florian.
Post by legutierr
modelform.is_valid() fails to anticipate database integrity errors
when those errors involve any fields that are not part of that form
itself.
    exclude = ['user']
    model = …
    instance = form.save(commit=False)
    instance.user = request.user
After reading the thread that you referenced, I agree with you that it
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
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.
If I understood the gist of the model-validation thread you
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.
I think this is partly true. However, I believe that with respect to
`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.
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
To unsubscribe from this group, send email to django-developers+***@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
legutierr
2011-04-22 07:50:53 UTC
Permalink
The subject didn't make any sense.
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
To unsubscribe from this group, send email to django-developers+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Carl Meyer
2011-04-23 05:23:00 UTC
Permalink
Post by legutierr
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
http://code.djangoproject.com/ticket/12028
http://code.djangoproject.com/ticket/13091
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.
Post by legutierr
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
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
To unsubscribe from this group, send email to django-developers+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Florian Apolloner
2011-04-23 13:18:59 UTC
Permalink
Post by Carl Meyer
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.
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?

Regards, Florian
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
To unsubscribe from this group, send email to django-developers+***@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Carl Meyer
2011-04-23 15:55:27 UTC
Permalink
Hi Florian,
Post by Florian Apolloner
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
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
To unsubscribe from this group, send email to django-developers+***@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Florian Apolloner
2011-04-23 17:12:54 UTC
Permalink
Hey Carl,
http://groups.google.com/group/django-developers/msg/3014f29c5125653ais
where it was briefly mentioned by Lukasz, I haven't seen any discussion
since.
Thx
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 :þ

Regards,
Florian
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
To unsubscribe from this group, send email to django-developers+***@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Florian Apolloner
2011-04-23 17:16:42 UTC
Permalink
Post by Florian Apolloner
Post by Carl Meyer
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 :þ
At least if I can defer validation of some fields on the form to the
end (in my case the user)
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
To unsubscribe from this group, send email to django-developers+***@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Mikhail Korobov
2011-04-24 12:46:00 UTC
Permalink
Hi Carl,

The issue is not only with unique_together indeed. Please correct me if I'm
wrong, but it seems there is no way currently to use model validation with
fields dependent on each other when one of these fields is not POSTed
directly by user. Example:

# models.py
class Ticket(models.Model):
created_by = models.ForeignKey(User, related_name='created_tickets')
responsible = models.ForeignKey(User, related_name='todo')

def clean():
# we don't want to allow tickets where created_by == responsible for
some reason
from django.core.exceptions import ValidationError
if self.created_by == self.responsible:
raise ValidationError('Responsible is incorrect.')

# views.py
class TicketForm(forms.ModelForm):
class Meta:
model = Ticket
exclude = ['created_by']

@login_required
def add_ticket(request):
form = TicketForm(request.POST or None)
if form.is_valid():
ticket = form.save(commit=False)
ticket.created_by = request.user

# todo: handle ticket.full_clean()

ticket.save()
return redirect('ticket_list')
return TemplateResponse(request, 'tickets/add.html', {'form': form})

Model.clean method is always called on form validation (unlike field
validators that are excluded if field is excluded from form). And we can't
write validator for 'responsible' field or for 'created_by' field (so that
it will be excluded from validation on form.is_valid) because this validator
won't have an access to other model fields. So the only option here seems to
move validation from model level to form level.

Please consider this use case while rethinking model validation. The
proposed context manager seems to be a very good idea and I hope it will fix
this issue as well.
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
To unsubscribe from this group, send email to django-developers+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Carl Meyer
2011-04-29 13:14:02 UTC
Permalink
Hi Mikhail,
Post by Mikhail Korobov
The issue is not only with unique_together indeed. Please correct me if I'm
wrong, but it seems there is no way currently to use model validation with
fields dependent on each other when one of these fields is not POSTed
# models.py
    created_by = models.ForeignKey(User, related_name='created_tickets')
    responsible = models.ForeignKey(User, related_name='todo')
        # we don't want to allow tickets where created_by == responsible for
some reason
        from django.core.exceptions import ValidationError
        if self.created_by == self.responsible:  
            raise ValidationError('Responsible is incorrect.')
# views.py
        model = Ticket
        exclude = ['created_by']
@login_required
    form = TicketForm(request.POST or None)
        ticket = form.save(commit=False)
        ticket.created_by = request.user
        # todo: handle ticket.full_clean()
        ticket.save()
        return redirect('ticket_list')
    return TemplateResponse(request, 'tickets/add.html', {'form': form})
Model.clean method is always called on form validation (unlike field
validators that are excluded if field is excluded from form). And we can't
write validator for 'responsible' field or for 'created_by' field (so that
it will be excluded from validation on form.is_valid) because this validator
won't have an access to other model fields. So the only option here seems to
move validation from model level to form level.
Please consider this use case while rethinking model validation. The
proposed context manager seems to be a very good idea and I hope it will fix
this issue as well.
Sorry, I missed this somehow earlier. I agree with you that this is
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
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
To unsubscribe from this group, send email to django-developers+***@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
legutierr
2011-04-26 19:47:53 UTC
Permalink
Hi Carl-
Post by Carl Meyer
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
- 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.
Regarding this, I have two somewhat contradictory responses:

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.
Post by Carl Meyer
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.

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
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
To unsubscribe from this group, send email to django-developers+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Carl Meyer
2011-04-27 03:49:36 UTC
Permalink
Hi Eduardo,
Post by legutierr
Post by Carl Meyer
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
- 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.
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.
Post by legutierr
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
Post by legutierr
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.
Post by legutierr
Post by Carl Meyer
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.
Post by legutierr
While we are on the subject of new idioms, I am curious as to what
form = ModelForm(data)
form.instance.excluded_field = programatic_data
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
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
To unsubscribe from this group, send email to django-developers+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
legutierr
2011-04-27 19:02:44 UTC
Permalink
Hi Carl-
Post by 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.
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

Regards,

Eduardo
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
To unsubscribe from this group, send email to django-developers+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Carl Meyer
2011-04-27 19:13:57 UTC
Permalink
Post by legutierr
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
http://docs.djangoproject.com/en/1.3/ref/models/options/#unique-together
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
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
To unsubscribe from this group, send email to django-developers+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
legutierr
2011-04-28 05:43:25 UTC
Permalink
OK, I just uploaded a patch against trunk that should be consistent
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
Post by Carl Meyer
Post by legutierr
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
http://docs.djangoproject.com/en/1.3/ref/models/options/#unique-together
http://docs.djangoproject.com/en/1.3/ref/models/instances/#django.db....
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
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
To unsubscribe from this group, send email to django-developers+***@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
legutierr
2011-04-28 06:02:14 UTC
Permalink
I didn't link to where I uploded the patch. It is here:
http://code.djangoproject.com/ticket/13091
Post by legutierr
OK, I just uploaded a patch against trunk that should be consistent
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
Post by Carl Meyer
Post by legutierr
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
http://docs.djangoproject.com/en/1.3/ref/models/options/#unique-together
http://docs.djangoproject.com/en/1.3/ref/models/instances/#django.db....
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
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
To unsubscribe from this group, send email to django-developers+***@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
legutierr
2011-04-28 17:35:16 UTC
Permalink
I just added a new patch in response to Carl's comments on the ticket.

http://code.djangoproject.com/ticket/13091

Regards,

Eduardo
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
To unsubscribe from this group, send email to django-developers+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Carl Meyer
2011-04-28 20:28:08 UTC
Permalink
Hi Eduardo,
Post by legutierr
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
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
To unsubscribe from this group, send email to django-developers+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
legutierr
2011-04-28 23:36:01 UTC
Permalink
This is extraordinarily discouraging. 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 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.

I accept your apology, truly, but can I ask something else of you?
Don't treat your contributors' time as something so easy to throw
away. You and I (and others) have been talking about this for days.
I have spent at least a day coding and analyzing. When the whole
thing gets thrown away because of an IRC discussion, I don't know what
to think. Carl, I asked you specifically in this very thread whether
your new idiom would prevent this fix from getting checked in, before
I started coding, and you answered that you "didn't think so". Should
I have read that differently, more in the negative than in the
affirmative? Perhaps, and I certainly will ask for clarification from
you in the future. But you also gave me the green light to go ahead
an write the patch in a manner that we discussed.

Even if sometimes in the final event your original decision to pursue
a certain path might prove not to be optimal, can I ask you to stick
with it, so that people like me don't have to worry about the rug
being pulled out from underneath us at the last moment? To place this
in perspective: this is a *small* bug fix that adds six (6) lines of
code to the file and prevents exceptions being raised to the user in
the admin (and elsewhere). The "problematic" special case here is a
rare edge case that, conceivably, could even be left out from the
implementation. And yet, as someone who is still trying to figure out
what kind of effort to put into contribute to Django, for me it is
seriously discouraging for the work to be discarded.
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
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?

Regards,

Eduardo

PS I wrote much of this in anger, so it is rather more critical than
anything I would normally post, but I think that it needs to be said.
Hi Eduardo,
Post by legutierr
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
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
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
To unsubscribe from this group, send email to django-developers+***@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Carl Meyer
2011-04-29 00:34:16 UTC
Permalink
Hi Eduardo,
Post by legutierr
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.
Post by legutierr
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.
Post by legutierr
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.
Post by legutierr
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
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
To unsubscribe from this group, send email to django-developers+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
legutierr
2011-04-29 19:46:30 UTC
Permalink
Carl,

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
Post by Carl Meyer
Hi Eduardo,
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
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
To unsubscribe from this group, send email to django-developers+***@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
legutierr
2011-04-29 00:12:16 UTC
Permalink
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?
Post by Carl Meyer
Hi Eduardo,
Post by legutierr
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
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
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
To unsubscribe from this group, send email to django-developers+***@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Carl Meyer
2011-04-29 00:37:26 UTC
Permalink
Post by legutierr
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
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
To unsubscribe from this group, send email to django-developers+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Carl Meyer
2011-04-29 13:40:37 UTC
Permalink
Hi Lior,

(moved from another thread)
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
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
To unsubscribe from this group, send email to django-developers+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/***@public.gmane.org
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Loading...