Re: "unique_together" only validated in modelform.is_valid() if ALL of the referenced fields (Ticket #13091)

[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

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: [5-for-1] Review request

[Gabriel Hurley]

I noticed the "TODO" on the bottom of that page you linked... if you 
actually want to write something up for the docs about reviewing tickets 
(based on the discussion that's happened on the list the last few days) I'd 
suggest adding it to the /docs/howto/contribute section. Feel free to ping 
me if you get a patch written and I'll work with you on getting it in.

- Gabriel

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: [5-for-1] Review request

[Jacob Kaplan-Moss]

On Fri, Apr 22, 2011 at 3:26 AM, Aymeric Augustin
 wrote:
> I have retroactively added 5 tickets for Alex's review, plus 5 tickets that
> I'd like to trade against a review of #15255, especially the second comment.

Done!

BTW, I'm pretty sure Alex was joking about you needing to do five
more, so double thanks. I really appreciate it - let's keep it up!

Jacob

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

sitemaps and prepend_www

[Nikolay Panov]

Hi,

I cannot use www in current_site.domain since .domain is used in many
places (e.g. emails) where www is not required. BTW, I'm using
PREPEND_WWW=True setting, since it makes url looks better.
Now, google webmaster tools rejecting all links in sitemap of my
project because lack of 'www'.

My question is: why we cannot have something like:

--- django_old/contrib/sitemaps/__init__.py   2008-11-19
08:44:26.0 +0300
+++ django_new/contrib/sitemaps/__init__.py 2010-11-24
18:57:34.0 +0300
@@ -1,4 +1,5 @@
 from django.core import urlresolvers, paginator
+from django.conf import settings
 import urllib

 PING_URL = "http://www.google.com/webmasters/tools/ping;
@@ -29,7 +30,10 @@

 from django.contrib.sites.models import Site
 current_site = Site.objects.get_current()
-url = "http://%s%s; % (current_site.domain, sitemap_url)
+domain = current_site.domain
+if settings.PREPEND_WWW and not domain.startswith('www.'):
+domain = 'www.' + domain
+url = "http://%s%s; % (domain, sitemap_url)
 params = urllib.urlencode({'sitemap':url})
 urllib.urlopen("%s?%s" % (ping_url, params))

@@ -62,9 +66,12 @@
 def get_urls(self, page=1):
 from django.contrib.sites.models import Site
 current_site = Site.objects.get_current()
+domain = current_site.domain
+if settings.PREPEND_WWW and not domain.startswith('www.'):
+domain = 'www.' + domain
 urls = []
 for item in self.paginator.page(page).object_list:
-loc = "http://%s%s; % (current_site.domain,
self.__get('location', item))
+loc = "http://%s%s; % (domain, self.__get('location', item))
 url_info = {
 'location':   loc,
 'lastmod':self.__get('lastmod', item, None),
and

--- django_old/contrib/sitemaps/views.py  2008-11-19
08:44:26.0 +0300
+++ django_new/contrib/sitemaps/views.py2010-11-24
18:57:58.0 +0300
@@ -4,6 +4,8 @@
 from django.core import urlresolvers
 from django.utils.encoding import smart_str
 from django.core.paginator import EmptyPage, PageNotAnInteger
+from django.conf import settings
+

 def index(request, sitemaps):
 current_site = Site.objects.get_current()
@@ -15,10 +17,13 @@
 else:
 pages = site.paginator.num_pages
 sitemap_url =
urlresolvers.reverse('django.contrib.sitemaps.views.sitemap',
kwargs={'section': section})
-sites.append('%s://%s%s' % (protocol, current_site.domain,
sitemap_url))
+domain = current_site.domain
+if settings.PREPEND_WWW and not domain.startswith('www.'):
+domain = 'www.' + domain
+sites.append('%s://%s%s' % (protocol, domain, sitemap_url))
 if pages > 1:
 for page in range(2, pages+1):
-sites.append('%s://%s%s?p=%s' % (protocol,
current_site.domain, sitemap_url, page))
+sites.append('%s://%s%s?p=%s' % (protocol, domain,
sitemap_url, page))
 xml = loader.render_to_string('sitemap_index.xml', {'sitemaps': sites})
 return HttpResponse(xml, mimetype='application/xml')


This fix works well for me, but it will be great to have this into
official django repo.

Have a nice day,
   Nikolay.

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: Policy for Duplicate Tickets - Bug vs. Feature Request

[Michael Radziej]

On Thu, 21 Apr 2011 21:33:45 -0500, Jacob Kaplan-Moss  
wrote:
> That's more or less how I look at things, with one additional caveat:

Thanks for the clarification, Jacob.


Kind regards

Michael

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: [5-for-1] Review request

[Aymeric Augustin]

2011/4/21 Alex Gaynor 

> As did I, do we have to retroactively require more reviews ;)
>

I'm now tracking my 5-for-1 deals here:
http://myks.org/stuff/django_5-for-1.txt

I have retroactively added 5 tickets for Alex's review, plus 5 tickets that
I'd like to trade against a review of #15255, especially the second comment.
Thanks!

By the way, we're down to 6 unreviewed tickets. Not everyone claims his
reviews :)

-- 
Aymeric Augustin.

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

"unique_together" only validated in modelform.is_valid() if ALL of the referenced fields are included in the form (Ticket #13091)

[legutierr]

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@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

"unique_together" only validated in modelform.is_valid() if ALL of the referenced fields (Ticket #13091)

[legutierr]

Thanks for your response, Florian.

> > 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

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
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.

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,