[Django] #25467: Excluding an object with no ID excludes everything.
Django
-------------------------------+--------------------
Reporter: skorokithakis | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 1.8
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
This behaviour is surprising:
{{{
In [26]: UserAlias.objects.filter(alias="f...@example.com")
Out[26]: [<UserAlias: f...@example.com>]
In [27]:
UserAlias.objects.filter(alias="f...@example.com").exclude(user=User())
Out[27]: []
}}}
The exclude should have no effect in this case, it should not exclude
every user.
--
Ticket URL: <https://code.djangoproject.com/ticket/25467>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: skorokithakis | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* component: Uncategorized => Database layer (models, ORM)
* needs_tests: => 0
* needs_docs: => 0
Comment:
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/25467#comment:1>
Django
Reporter: skorokithakis | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by skorokithakis):
My use case is:
{{{
(user if user else User()).get_thing()
}}}
(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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25467#comment:2>
Django
Reporter: skorokithakis | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Unreviewed => Accepted
Comment:
Someone will have to investigate to determine feasibility.
--
Ticket URL: <https://code.djangoproject.com/ticket/25467#comment:3>
Django
Reporter: skorokithakis | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by nemesisdesign):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25467#comment:4>
Django
Reporter: skorokithakis | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25467#comment:5>
Django
Reporter: skorokithakis | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by HuffAndPuff):
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"?
--
Ticket URL: <https://code.djangoproject.com/ticket/25467#comment:6>
Django
Reporter: skorokithakis | Owner:
| HuffAndPuff
Type: Bug | Status: assigned
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by HuffAndPuff):
* owner: nobody => HuffAndPuff
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/25467#comment:7>
Django
Reporter: skorokithakis | Owner:
| HuffAndPuff
Type: Bug | Status: assigned
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by akaariai):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25467#comment:8>
Django
Reporter: skorokithakis | Owner:
| HuffAndPuff
Type: Bug | Status: assigned
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
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).
--
Ticket URL: <https://code.djangoproject.com/ticket/25467#comment:9>
Django
Reporter: skorokithakis | Owner:
| HuffAndPuff
Type: Bug | Status: assigned
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by skorokithakis):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25467#comment:10>
Django
Reporter: Stavros | Owner: Jacob
Korokithakis | Walls
Type: Bug | Status: assigned
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* owner: Joakim Israelsson => Jacob Walls
* has_patch: 0 => 1
Comment:
If I have captured the original case in the
[https://github.com/django/django/pull/14927 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25467#comment:11>
Django
Reporter: Stavros | Owner: Jacob
Korokithakis | Walls
Type: Bug | Status: assigned
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"28f66b2783e52d5be51887c31cd2be907b820e11" 28f66b27]:
{{{
#!CommitTicketReference repository=""
revision="28f66b2783e52d5be51887c31cd2be907b820e11"
Refs #25467 -- Added test for excluding one-to-one relation with unsaved
objects.
Fixed in 58da81a5a372a69f0bac801c412b57f3cce5f188.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25467#comment:12>
Django
Reporter: Stavros | Owner: Jacob
Korokithakis | Walls
Component: Database layer | Version: 1.8
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* status: assigned => closed
* resolution: => fixed
Comment:
Fixed in 58da81a5a372a69f0bac801c412b57f3cce5f188.
--
Ticket URL: <https://code.djangoproject.com/ticket/25467#comment:13>