Bug when generating sql for single (AND: (EverythingNode))
trybik
when generating sql for WhereNode represented as (AND:
(EverythingNode)), in http://code.djangoproject.com/browser/django/trunk/django/db/models/sql/where.py#L104
the empty variable will stay False, thus throwing the EmptyResultSet
further on (L120) and in consequence e.g. returning EmptyQuerySet on
higher level. Imvho, in case of FullResultSet handling you're missing
setting empty variable to True (can be first thing done after catching
the exception).
How one can get (AND: (EverythingNode)) WhereNode to be generated?
http://code.djangoproject.com/browser/django/trunk/django/db/models/sql/query.py#L487
Best regards,
Mikołaj
trybik
completely confusing. A little correction:
On 25 Lut, 14:14, trybik <mikolaj.rybin...@gmail.com> wrote:
> Hi,
>
> when generating sql forWhereNoderepresented as (AND:
> (EverythingNode)), inhttp://code.djangoproject.com/browser/django/trunk/django/db/models/s...
be reached because of treacherous 'continue' in L113.
> thus throwing the EmptyResultSet
> further on (L120) and in consequence e.g. returning EmptyQuerySet on
> higher level. Imvho, in case of FullResultSet handling you're missing
> setting empty variable to True
also do the trick in this case.
Best regards,
Mikołaj
> (can be first thing done after catching the exception).
>
>
> Best regards,
> Miko³aj
Łukasz Rekucki
On 4 April 2011 19:46, trybik <mikolaj....@gmail.com> wrote:
> Omg, I've just reviewed this bug, it's still there but this message is
> completely confusing. A little correction:
It's best to report bugs on the tracker. Otherwise, they'll die in
infinite depths of everyone's mailboxes.
>
> On 25 Lut, 14:14, trybik <mikolaj.rybin...@gmail.com> wrote:
>> Hi,
>>
>> when generating sql forWhereNoderepresented as (AND:
>> (EverythingNode)), inhttp://code.djangoproject.com/browser/django/trunk/django/db/models/s...
>> the empty variable will stay False,
>
> True of course (L88), not False... the point is that L105 will never
> be reached because of treacherous 'continue' in L113.
>
>> thus throwing the EmptyResultSet
>> further on (L120) and in consequence e.g. returning EmptyQuerySet on
>> higher level. Imvho, in case of FullResultSet handling you're missing
>> setting empty variable to True
>
> and here False, not True (between L104 and L105); removing L113 would
> also do the trick in this case.
It's still not very clear to me what problem are you describing. If
you could produce a queryset that generates wrong SQL, it would
clearify things.
>> How one can get (AND: (EverythingNode))WhereNodeto be generated?http://code.djangoproject.com/browser/django/trunk/django/db/models/s...
Good question. You can see it's generated in combine()[1], but I'm not
sure how you can make a Query object with "self.where" attribute that
would evaluate to False.
[1]: http://code.djangoproject.com/browser/django/trunk/django/db/models/sql/query.py#L478
--
Łukasz Rekucki
trybik
On Apr 4, 8:43 pm, Łukasz Rekucki <lreku...@gmail.com> wrote:
> It's best to report bugs on the tracker. Otherwise, they'll die in
> infinite depths of everyone's mailboxes.
this. Here's the "safe" patch:
diff -rupN django/db/models/sql/where.py django.fix//db/models/sql/
where.py
--- django/db/models/sql/where.py 2011-04-06 19:48:07.000000000
+0200
+++ django.fix//db/models/sql/where.py 2011-04-06 22:05:32.000000000
+0200
@@ -102,6 +102,7 @@ class WhereNode(tree.Node):
empty = False
continue
except FullResultSet:
+ empty = False
if self.connector == OR:
if self.negated:
empty = True
> It's still not very clear to me what problem are you describing.
as a child, then calling `as_sql` method on that WhereNode instance
results with EmptyResultSet exception, which is bad. That's because in
such case, in the body of the `WhereNode.as_sql` method a variable
called `empty` will never get set to False value, although it looks
like it definitely should.
> If
> you could produce a queryset that generates wrong SQL, it would
> clearify things.
>
> >> How one can get (AND: (EverythingNode))WhereNodeto be generated?http://code.djangoproject.com/browser/django/trunk/django/db/models/s...
>
> Good question. You can see it's generated in combine()[1], but I'm not
> sure how you can make a Query object with "self.where" attribute that
> would evaluate to False.
bool(User.objects.all().query.where)
A QuerySet instance to reproduce this bug is a "too much hassle"
thing. It's not easy for me to generate QuerySet containing query with
WhereNode with singleton EverythinNode child. (E.g.
User.objects.filter(username__isnull=False) & User.objects.all() does
not do the trick.) I've managed to capture that in a running system
with big query on the lhs of the & operator and mptt tree managers
query set on the rhs. A self-contained description based on that would
require disproportional amount of work from my side in comparison to
just comprehending the bug's essence + assurance that this case is
actually reachable (which, is not important at all even if it
shouldn't be reachable - the bug is still in the `as_sql` method). I
guess some swift Django ORM developer could have produce such test
case much more effectively, thus only mail from my side. Imvho, a safe
fix is clear and instant (cf. removing `continue` to reach
`empty=False` later on).
Best regards,
Mikołaj
Jacob Kaplan-Moss
> Hi Łukasz,
>
> On Apr 4, 8:43 pm, Łukasz Rekucki <lreku...@gmail.com> wrote:
>> It's best to report bugs on the tracker. Otherwise, they'll die in
>> infinite depths of everyone's mailboxes.
>
> that sounds like too much hassle for me. Please, feel free to report
> this.
No, you don't get to do this.
If it's too much hassle for you to open a ticket, then it's too much
hassle for us to work with you.
We have thousands of people all with their own patches, feature
requests, and support needs. We're going to prioritize the ones who
are willing to meet us halfway, help out, and don't place demands on
our volunteer time.
Jacob
trybik
prima balerina mode. I really appreciate your volunteer time work and
the need to control the chaos and would like to help out respecting
that. "Submission rejected as potential spam (BlogSpam says content is
spam (badip:state/blacklist.d/127.0.0.1))" is really to much hassle
for me. I will gladly submit a ticket if you please fix that.
As for now, again, feel free to submit the ticket or tackle the
problem directly, without too much bureaucracy involved (suggested).
No demands placed.
Best regards,
Mikołaj
On Apr 6, 11:01 pm, Jacob Kaplan-Moss <ja...@jacobian.org> wrote: