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

[Carl Meyer]

Hi Eduardo,

On 04/26/2011 02:47 PM, legutierr wrote:
>> 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.
> 
> 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.

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():
>   

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

[legutierr]

Hi Carl-

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

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.

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

Odp: Re: Odp: Re: Storing language in session/cookie

[Mikołaj S .]

I don't see any good reason to store the language in session rather than 
cookie. And storing it in cookie has major advantage, that is avoiding 
creating unnnecessary sessions. I say if LocaleMiddleware is in our way to 
better behavior, let's just change it.

But I understand that revolution is not always a better idea than evolution, 
so Madis'es session-and-cookie storing is also an option for me, although I 
don't see how it could be useful.

If somebody has some arguments for session-based language storing, I'd see 
it gladly.

-- 
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: Odp: Re: Storing language in session/cookie

[Madis]

Well we are actually not trying to address the issue on ticket #13217
I think. We are addressing the way language gets stored for the
client.
Because LocaleMiddleware checks for a session anyway. If it was
decided wontfix then doing it the way I supposed should be OK I think.

If I'm mistaken - please correct me.

-- 
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: Odp: Re: Storing language in session/cookie

[Luke Plant]

On 26/04/11 14:21, Madis wrote:
> Well I think there is no need for another setting and I think there is
> no problem setting the lang twice - as then the cookie is not always
> accessed as it first checks for the language in the session. It only
> looks for it in the cookie if there is no session or the customer is
> returning to the site.
> So the cookie for language just lives there and is only looked in if
> there is no session available. It produces no overhead what so ever
> because set_lang is only called when changing languages.

This does not address the fact that accessing the session to check
causes a session to be created if it did not exist. This is what this
whole thread is about! See also the discussion on ticket #13217.

Luke

-- 
"I washed a sock. Then I put it in the dryer. When I took it out,
it was gone."  (Steven Wright)

Luke Plant || http://lukeplant.me.uk/

-- 
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: Odp: Re: Storing language in session/cookie

[Madis]

There is also a ticket here which proposes a patch -
http://code.djangoproject.com/ticket/14825
I used it last night - it also works but wont give the users the
freedom to choose if they want to set the lang cookie or not and also
adds overhead.

-- 
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: Odp: Re: Storing language in session/cookie

[Madis]

Well I think there is no need for another setting and I think there is
no problem setting the lang twice - as then the cookie is not always
accessed as it first checks for the language in the session. It only
looks for it in the cookie if there is no session or the customer is
returning to the site.
So the cookie for language just lives there and is only looked in if
there is no session available. It produces no overhead what so ever
because set_lang is only called when changing languages.

And if you want set_lang to store the cookie also then just configure
it in your urls.py - then we dont need another setting also. This can
be documented and maybe many people wont even need this - or want this
as their clients manly speak English. This would then also be
backwards compatible.

Well and the possible downside of this approach is that some people
dont want the language_cookie stored because they consider this
private data (for whatever reason there might be). So giving them an
option to set the cookie is or not is always good.

Right now I rewrote the set_lang view for my app so the cookie is also
stored and expires after 1 year - it works perfect after logout or
when revisiting the site.
http://dpaste.com/hold/535886/
Right now I have no options to set it or not because I want to use it.
But it would be very easy to write it optional with options to also
set the cookie and expiration time/date.

One thought is that maybe when revisiting and the session is also
available the middleware should write the language from the cookie to
the session again - so there is no additional call to look in the
cookie.

-- 
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: Help review tickets, get a prize!

[TiNo]

On Thu, Apr 21, 2011 at 04:31, Jacob Kaplan-Moss  wrote:

> On Wed, Apr 20, 2011 at 5:44 PM, Carl Meyer  wrote:
> > So you don't necessarily reproduce it yourself before marking Accepted?
>
> Mm, it depends. Sometimes I don't need to -- it's clearly a bug, and I
> see everything I need to track it down. I generally trust that if the
> user could be bothered to write a good, clear bug report that it's
> really a bug and I don't need to reproduce it. It's not like moving it
> to accepted and *then* finding that it's not really a bug is a
> problem. Perfect is the enemy of the good, and all that.
>
> All in all, I'd guess that of the "bug" category tickets I see I need
> to take the time to reproduce about half of them. Maybe 2/3rds.
>
> Jacob


Hi,

Thanks to both of you for the write ups. It makes the process a lot more
clear. Will try to put this to action the forthcoming days.

Tino

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

Odp: Re: Storing language in session/cookie

[Mikołaj S .]

As lukeplant commented on a ticket:

This is proposing the same thing as 
#12794, 
but for a different reason.

#13217  is related, but the 
solution proposed here was not proposed there, and doesn't have the same 
issues that caused the WONTFIX on that ticket. This change would help that 
problem if the cache is able to cache by language cookie.

I'm not in favour of yet another setting if we can avoid it - we need to 
know if there are any downsides to always storing in the cookie rather than 
the session. If not, we always store in the cookie. Since we've documented 
the current behaviour, we may however need a deprecation path involving a 
setting.


So it's now about whether to be backwards compatible but add another setting 
and complicate things a bit, or not to be backwards compatible and move 
current language to separate cookie permanently.

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

Odp: Re: Storing language in session/cookie

[Mikołaj S .]

I've created a ticket: http://code.djangoproject.com/ticket/15902

I agree with Madis. Having a setting to choose where the current language is 
stored would be convenient and also backwards compatible.

How do you think the setting should look like? I propose:

LANGUAGE_FORCE_COOKIE

...which defaults to False for backwards compatibility. And when it's True, 
the current language is also saved in a separate cookie, no matter if there 
is a session enabled.

But I also wonder if it won't be unnecessarily redundant to have the 
language stored both in a cookie and a session.

-- 
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: sitemaps and prepend_www

[Gert Van Gool]

Usually you would create a ticket on trac [1] which describes your problem
and reasoning.
And you can attach a patch file to it. This way, it will go through the
normal triaging and feature requests.

[1] http://code.djangoproject.com/newticket
-- Gert

Mobile: +32 498725202
Twitter: @gvangool 
Web: http://gert.selentic.net



On Fri, Apr 22, 2011 at 13:14, Nikolay Panov wrote:

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

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

A coupla patch reviews

[Julien Phalip]

Hi there,

I've just posted a patch for #14262 to add a new assignment_tag helper
function. Is anyone keen to review it?

Also, it would be great if someone could review the latest patch for
#15805. If that one gets checked in then we'll also be able to fix
#14608 and then I'll start work on #15838.

Many thanks!

Julien

http://code.djangoproject.com/ticket/14262
http://code.djangoproject.com/ticket/15805
http://code.djangoproject.com/ticket/14608
http://code.djangoproject.com/ticket/15838

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