attribute__isnull=True vs attribute__exact=None (Ticket 2737)
Russell Keith-Magee
I've hit upon an interesting bug, which is related to ticket 2737
(currently listed as a feature request, but I think there is an
argument that the requested feature is required as a bug fix).
Consider the following model:
class Poll(models.Model):
name = models.CharField(maxlength=100)
class Choice(models.Model):
name = models.CharField(maxlength=100)
poll = models.ForeignKey(Choice)
Create some dummy data, then run the following sequence of commands:
> p = Poll()
> p.choice_set.all()
i.e., a related manager on unsaved object. This returns _all_ Choice
objects in the database. It turns out that this is because the related
manager is based upon a 'relatedname__pk=%d' % pk_value' query.
However:
> Choice.objects.filter(poll__pk=None)
returns all Choice objects in the database, because any query where
value=None is outright ignored - no error, no warning. As a side note,
this also means that:
> Choice.objects.filter(foo__pk=None)
_also_ returns all choice objects in the database, with no error, even
though foo isn't a valid attribute name.
To me, all three of these behaviours are counterintuitive. To me, the
choice_set of an unsaved object should be empty, pk=None (and, by
extension, id__exact=None) corresponds nicely to id__isnull=True, and
an invalid attribute should always raise an error.
Ticket 2737 requests a feature where __exact=None would be interpreted
as __isnull=True. This would fix all three problems I have described.
The proposed patch isn't correct, but I feel the idea is valid.
As a quick test, I removed line 710 of query.py (the line that exludes
value=None queries), and none of the unit tests failed, so it doesn't
appear that ignoring value=None queries is explicitly required.
Obviously, it is desirable for other query types, like __range=None,
but there hasn't been an explicit test-driven requirement that
value=None queries are ignored. However, this could be an indication
of deficiencies in the regression suite.
Also, fixing this problem is problematic because it encroaches upon
Malcolm's refactor of the QuerySet code.
Comments? Have I missed an obvious problem? Have I missed a ticket in
the database that describes this problem already? Malcolm, can you
comment on the potential impact a short term fix for this problem
would have on your refactor work?
Yours,
Russ Magee %-)
Matthew Flanagan
Is http://code.djangoproject.com/ticket/2400 is related to this as well?
Malcolm Tredinnick
[...]
> Also, fixing this problem is problematic because it encroaches upon
> Malcolm's refactor of the QuerySet code.
>
> Comments? Have I missed an obvious problem? Have I missed a ticket in
> the database that describes this problem already? Malcolm, can you
> comment on the potential impact a short term fix for this problem
> would have on your refactor work?
Ignore the impact on my stuff. I'll catch up with that change.
I agree the second two cases are bugs. I'm probably -0 on making
exact=None be the same as isnull=True, because it's a synonym for
something you can already do (we already have isnull). Still, I guess
it's a harmless change (which is why I'm not -1).
Regards,
Malcolm
>
> Yours,
> Russ Magee %-)
>
> >
>
Malcolm Tredinnick
> Hi Russ,
>
> Is http://code.djangoproject.com/ticket/2400 is related to this as well?
I don't think so. #2400 is asking for bonus functionality (from an
implementation perspective). It requires twisting the generated SQL
slightly in order to allow for the NULL possibility.
I think I have that change covered in my current changes. That's why
it's assigned to me at the moment.
Regards,
Malcolm
Russell Keith-Magee
> I agree the second two cases are bugs. I'm probably -0 on making
> exact=None be the same as isnull=True, because it's a synonym for
> something you can already do (we already have isnull). Still, I guess
> it's a harmless change (which is why I'm not -1).
I agree that it duplicates functionality. Obviously, keeping __isnull
around for legacy purposes doesn't harm anyone, so we might as well
keep it around. However, I would argue that:
1) __isnull is a special case. Special cases bug me, as they usually
indicate an inconsistency in usage somewhere (in this case, the
inconsistency of using None as a query value)
2) Adding the extra logic throughout the Managers to handle unsaved
objects strikes me as much more inelegant (and much more likely to
get forgotten/unexpectedly broken) that the few lines that would be
required to implement the fix at the query level.
Yours,
Russ Magee %-)
Michael Radziej
> Ticket 2737 requests a feature where __exact=None would be interpreted
> as __isnull=True. This would fix all three problems I have described.
> The proposed patch isn't correct, but I feel the idea is valid.
My brain has been wired in SQL mode, perhaps for too long, so I
consider __exact=None as the equivalent of ... WHERE ...=NULL,
which gives an empty result set. Perhaps this comes to a surprise
for an SQL newbie, but it is seriously my intuitive expectation,
being consistent with the SQL boolean algebra.
Well. If it's only me, ignore me, it would still be an
improvement above the current situation, and I probably can get
over it ;-)
Michael
Russell Keith-Magee
>
> Russell Keith-Magee schrieb:
> > Ticket 2737 requests a feature where __exact=None would be interpreted
> > as __isnull=True. This would fix all three problems I have described.
> > The proposed patch isn't correct, but I feel the idea is valid.
>
> My brain has been wired in SQL mode, perhaps for too long, so I
> consider __exact=None as the equivalent of ... WHERE ...=NULL,
> which gives an empty result set. Perhaps this comes to a surprise
> for an SQL newbie, but it is seriously my intuitive expectation,
> being consistent with the SQL boolean algebra.
Sorry - I'm confused; Are you agreeing with the proposed change, or
saying it contradicts your expectations? (I think you are agreeing - I
just want to make sure)
Yours,
Russ Magee %-)
Michael Radziej
> Sorry - I'm confused; Are you agreeing with the proposed change, or
> saying it contradicts your expectations? (I think you are agreeing - I
> just want to make sure)
My highest preference is to make __exact=None behave like WHERE
xxx=NULL, i.e. returning an empty set, against current behaviour
*and* against your proposal.
My middle preference is what you propose: making __exact=None a
replacement for __isnull=True.
My lowest preference is to keep it as it is: ignore __exact=None
Please, excuse me for being so complicated today ;-)
Michael
Russell Keith-Magee
> > Sorry - I'm confused; Are you agreeing with the proposed change, or
> > saying it contradicts your expectations? (I think you are agreeing - I
> > just want to make sure)
>
> My highest preference is to make __exact=None behave like WHERE
> xxx=NULL, i.e. returning an empty set, against current behaviour
> *and* against your proposal.
Ah - I missed the subtle difference you were advocating (=NULL vs IS
NULL). I agree completely; committed as r3902.
Yours,
Russ Magee
Malcolm Tredinnick
I really don't like this particular portion of the change. Using "=
NULL" is a poor construction: legal, but pointless. Nothing should be
equal to NULL, ever (except in Oracle, which likes to compare empty
strings as being equal to NULL, but it's broken in that respect). NULL
is not even equal to itself.
So doesn't this defeat the purpose of the exercise now? Using
__exact=None is now a synonym for "please don't ever return anything
(except on Oracle)" whereas I thought your were shooting for "where this
field is NULL".
Regards,
Malcolm
Russell Keith-Magee
>
> Hey Russell,
>
> On Sat, 2006-10-14 at 10:54 +0800, Russell Keith-Magee wrote:
> > On 10/12/06, Michael Radziej <m...@noris.de> wrote:
> > >
> > > Russell Keith-Magee:
> > > > Sorry - I'm confused; Are you agreeing with the proposed change, or
> > > > saying it contradicts your expectations? (I think you are agreeing - I
> > > > just want to make sure)
> > >
> > > My highest preference is to make __exact=None behave like WHERE
> > > xxx=NULL, i.e. returning an empty set, against current behaviour
> > > *and* against your proposal.
> >
> > Ah - I missed the subtle difference you were advocating (=NULL vs IS
> > NULL). I agree completely; committed as r3902.
>
> I really don't like this particular portion of the change. Using "=
> NULL" is a poor construction: legal, but pointless. Nothing should be
> equal to NULL, ever (except in Oracle, which likes to compare empty
> strings as being equal to NULL, but it's broken in that respect). NULL
> is not even equal to itself.
Hence the documentation to that effect. No, it isn't a particularly
useful expression, but it does have at least one use case (discussed
below).
I wasn't aware of the Oracle issue; but that strikes me as a problem
that should be fixed at the backend level, rather than as a reason to
exclude a feature.
> So doesn't this defeat the purpose of the exercise now? Using
> __exact=None is now a synonym for "please don't ever return anything
> (except on Oracle)" whereas I thought your were shooting for "where this
> field is NULL".
The use case that started this for me is in the regression test - a
related manager for an unsaved object. In this case, 'don't ever
return anything' is _exactly_ the right response, because nothing can
be related to the unsaved object.
Mapping __exact=None to _isnull=True will give effectively the same
results, but via a 'return everything with a primary key value of NULL
- which is nothing' interpretation of the problem.
However, as you noted earlier, this second approach is redundant given
the existence of _isnull. It also hides a path to the expression of
some valid (albiet edge case) SQL. The interpretation suggested by
Michael struck me as a good way to avoid the redundancy, fix the bug I
found, expose a small corner of legal SQL statements, and ultimately
required less code to implement.
The biggest problem I could see was the potential for '__exact=None
doesn't mean what you think it does' amongst newcomers. For me, the
best solution to this problem is documentation.
Yours,
Russ Magee %-)
Deryck Hodge
On 10/11/06, Russell Keith-Magee <freakb...@gmail.com> wrote:
> > Choice.objects.filter(poll__pk=None)
>
> returns all Choice objects in the database, because any query where
> value=None is outright ignored - no error, no warning. As a side note,
> this also means that:
>
There is some value in this when, for example, querying for objects
where query parameters may be set by optional form elements. Say I've
got a "Search for Restaurants" form with a checkbox for "Has boat
dock" and a text input for "Limit to city". I can do
Restaraunt.objects.filter(city__pk=city, boat_dock__exact=boat_dock)
where city and boat_dock were just picked up from the GET dictionary
or set to None if not present. With really large forms, having this
use of None can make the lookup simple. Just lookup for all possible
parameters, knowing that any unused parameters will be set to None and
safely ignored.
We used this several times at Naples News while I was there. I always
assumed, perhaps incorrectly :-), that this use of None was
intentional.
Cheers,
deryck
--
Deryck Hodge http://www.devurandom.org/
Samba Team http://www.samba.org/