Opened 9 years ago
Closed 3 years ago
#25467 closed Bug (fixed)
Excluding an object with no ID excludes everything.
Reported by: | Stavros Korokithakis | Owned by: | Jacob Walls |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 1.8 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
This behaviour is surprising:
In [26]: UserAlias.objects.filter(alias="foo@example.com") Out[26]: [<UserAlias: foo@example.com>] In [27]: UserAlias.objects.filter(alias="foo@example.com").exclude(user=User()) Out[27]: []
The exclude should have no effect in this case, it should not exclude every user.
Change History (13)
comment:1 by , 9 years ago
Component: | Uncategorized → Database layer (models, ORM) |
---|
comment:2 by , 9 years ago
My use case is:
(user if user else User()).get_thing("foo")
where get_thing does:
def get_thing(self, name): Things.filter(name=name).exclude(user=self)
(i.e. I only use this to access the model method in case I don't have an object to exclude)
I would expect that excluding users with no ID would not return any of them in the queryset. Since there can't be a user with no ID in the database anyway, I was expecting this to exclude nothing.
comment:3 by , 9 years ago
Triage Stage: | Unreviewed → Accepted |
---|
Someone will have to investigate to determine feasibility.
comment:4 by , 9 years ago
Relation lookups rely on the primary key to generate the query. Passing an unsaved instance to lookups like exclude
or filter
results in an invalid query, see the two following with the postgres backend:
>>> Node.objects.exclude(user=u) (0.001) SELECT "nodes_node"."id", "nodes_node"."user_id" FROM "nodes_node" WHERE NOT ("nodes_node"."user_id" = NULL AND "nodes_node"."user_id" IS NOT NULL) LIMIT 21; args=(None,)
>>> Node.objects.filter(user=u) (0.001) SELECT "nodes_node"."id", "nodes_node"."user_id" FROM "nodes_node" WHERE "nodes_node"."user_id" = NULL LIMIT 21; args=(None,) []
It makes sense to raise an exception to eliminate any ambiguity.
comment:5 by , 9 years ago
I'm not sure there is any ambiguity here. Clearly an unsaved instance doesn't match any row in the database, so if it does anything besides raise an exception, it should behave as "no match," just as the OP desired.
That said, if "behave as no match" turns out to be infeasible to implement cleanly, I'm not opposed to just raising an exception. I don't think this is an important use case, there are alternative workarounds.
comment:6 by , 9 years ago
Posting some thoughts on this one...
If we don't raise an exception, we would instead need to define how a None is handled in all Related lookups. Semantically it means "compare to something that is not in the database" right? At least that's how I would interpret it.
The lookups returned by a ForeignObject are in
, exact
, gt
, gte
, lt
, lte
and isnull
. The isnull
one is not really a problem.
The scenarios I can think of for the other ones are:
__in=[1,2,None]
__in=[None]
__exact=None
- Plus the greater/less than comparisons, what does it mean to say "greater than None" where None semantically means "no matching row"?
comment:7 by , 9 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:8 by , 9 years ago
Quick comments, I'm not sure if these apply to this ticket.
- .filter() and exclude() should return complements of each other. This causes problems for queries like lt=None.
- None is only allowed for exact and iexact lookup. The same should apply for relational lookups.
- in=[val, None] doesn't work correctly in the pythonic sense currently. It never matches anything.
comment:9 by , 9 years ago
Some more musings on this:
Our strongest existing precedent for the meaning of None
in an ORM comparison is that __exact=None
is equivalent to __isnull=True
, or SQL IS NULL
.
If we accept that as a consistent semantic, it would imply that we should fix __in=[val, None]
generally (not just for related fields) to remove the None
from the in
list and add an OR IS NULL
clause alongside it. This seems better than the current behavior to me, but perhaps low priority.
It would also imply that .exclude(related_field=unsaved_instance)
would exclude records where the related_field
FK is NULL
. This is subtly different from the OPs requested behavior, which is that such an exclude
would never exclude anything. I find the latter behavior (to consider unsaved_instance
to match nothing, rather than matching NULL) more intuitive. Which means we either a) use different semantics for instance-with-None-pk in comparisons than we use for None
in comparisons otherwise, b) accept a less-intuitive behavior for instance-with-None-pk for consistency with use of None
elsewhere, or c) declare that the choice between (a) and (b) is too ambiguous (that I was wrong earlier when I said "there's no ambiguity"), and therefore we should just raise an exception after all.
I think I would put these in preference order (a) (c) (b), but could live with any of them; and complexity-of-implementation could easily sway me to prefer (c) over (a).
comment:10 by , 9 years ago
I think that thinking about this in terms of the value of the ID field is misleading. The ID value being None is just an implementation detail, it could very well be the future numeric ID (if Django could know this before talking to the DB), in which case the query would become .exclude(related_field=<an ID that's not in the DB yet>)
, which matches the intuitive reasoning behind that exclude
call.
For that reason, i would strongly suggest option (a) that you mentioned over the other two.
comment:11 by , 3 years ago
Has patch: | set |
---|---|
Owner: | changed from | to
If I have captured the original case in the linked PR correctly, option (b) -- just treat the unsaved object as IS NULL -- was implemented in 58da81a5a372a69f0bac801c412b57f3cce5f188 (#27985).
From https://code.djangoproject.com/ticket/27985#comment:14 I see Simon suggesting this:
"I feel like preventing unsaved objects from being used in all related filters in future versions of Django would be the best course of action here."
I suggest closing this ticket, or else reframe with Simon's suggestion to raise an exception.
comment:13 by , 3 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Fixed in 58da81a5a372a69f0bac801c412b57f3cce5f188.
If possible, it seems reasonable to throw an exception if the primary key is null as this seems likely to be a programming error. Can anyone think of a case where this should pass silently?