Opened 3 years ago

Last modified 3 years ago

#25467 assigned Bug

Excluding an object with no ID excludes everything.

Reported by: Stavros Korokithakis Owned by: Joakim Israelsson
Component: Database layer (models, ORM) Version: 1.8
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no 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 (10)

comment:1 Changed 3 years ago by Tim Graham

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 Changed 3 years ago by Stavros Korokithakis

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 3 years ago by Stavros Korokithakis (previous) (diff)

comment:3 Changed 3 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

Someone will have to investigate to determine feasibility.

comment:4 Changed 3 years ago by Federico Capoano

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 Changed 3 years ago by Carl Meyer

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 Changed 3 years ago by Joakim Israelsson

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 Changed 3 years ago by Joakim Israelsson

Owner: changed from nobody to Joakim Israelsson
Status: newassigned

comment:8 Changed 3 years ago by Anssi Kääriäinen

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 Changed 3 years ago by Carl Meyer

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 Changed 3 years ago by Stavros Korokithakis

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.

Note: See TracTickets for help on using tickets.
Back to Top