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 Tim Graham, 9 years ago

Component: UncategorizedDatabase layer (models, ORM)

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?

comment:2 by Stavros Korokithakis, 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.

Last edited 9 years ago by Stavros Korokithakis (previous) (diff)

comment:3 by Tim Graham, 9 years ago

Triage Stage: UnreviewedAccepted

Someone will have to investigate to determine feasibility.

comment:4 by Federico Capoano, 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 Carl Meyer, 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 Joakim Israelsson, 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 Joakim Israelsson, 9 years ago

Owner: changed from nobody to Joakim Israelsson
Status: newassigned

comment:8 by Anssi Kääriäinen, 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 Carl Meyer, 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 Stavros Korokithakis, 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 Jacob Walls, 3 years ago

Has patch: set
Owner: changed from Joakim Israelsson to Jacob Walls

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:12 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In 28f66b27:

Refs #25467 -- Added test for excluding one-to-one relation with unsaved objects.

Fixed in 58da81a5a372a69f0bac801c412b57f3cce5f188.

comment:13 by Mariusz Felisiak, 3 years ago

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.
Back to Top