Opened 12 years ago

Last modified 4 years ago

#20024 new Bug

QuerySet.exclude() does not work with lists containing a 'None' element.

Reported by: stillwater.ke@… Owned by:
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Carsten Fuchs, Simon Charette Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Luke Plant)

For example:

Entry.objects.exclude(foo__in=[None, 1])

It is supposed to return all items whose foo field is not None or 1, but it actually returns an empty query set.

Change History (14)

comment:1 by Anssi Kääriäinen, 12 years ago

Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

This is a known failure (tested in queries/tests.py, test_col_not_in_list_containing_null()).

This is somewhat hard to fix as SQL's "somecol NOT IN lst_containing_null" is weird - it will always return False (well, technically UNKNOWN but this doesn't matter here), and this is what causes the bug.

It would be possible to check for presence of None in the given list. If present, remove it and add in a proper IS NULL check instead. But then a query like .exclude(somecol__in=qs_containig_null) would work differently from .exclude(somecol__in=list(qs_containig_null)). I guess that could be classified as known abstraction leak.

comment:2 by Aymeric Augustin, 12 years ago

Let's not be too smart... NULL in SQL is nowhere near as consistent as None in Python because of SQL's tri-valued boolean logic; we cannot hide this.

comment:3 by Luke Plant, 12 years ago

Description: modified (diff)

comment:4 by Tim Graham, 9 years ago

#13768 is a duplicate.

comment:5 by Tim Graham, 9 years ago

Summary: 'exclude' does not work with lists containing a 'None' element.QuerySet.exclude() does not work with lists containing a 'None' element.

comment:6 by Carsten Fuchs, 6 years ago

Cc: Carsten Fuchs added

comment:7 by Adam Sołtysik, 4 years ago

I think this should be fixed, this a seriously surprising flaw in the ORM. I've proposed several possible fixes for this issue in #31883, I'll try to summarize them here.

The simplest would be to just ignore Nones in the list. This would be consistent with how filter currently works. When dealing with querysets (__in=qs.values('col')), it should be enough to add WHERE col IS NOT NULL in the SELECT subquery.

A better solution, but a more breaking change, would be to add a proper IS [NOT] NULL check, in both exclude and filter cases, if None is in the list. This would allow writing filter(col__in=[something, None]) instead of filter(Q(col=something) | Q(col=None)). Some additional checks for inner querysets would also be required, so that the behaviour is consistent, or the queryset could be just converted to a list beforehand.

comment:8 by Adam Sołtysik, 4 years ago

Owner: changed from nobody to Adam Sołtysik
Status: newassigned
Version: 1.4master

I wanted to fix this, and I've found that it has already been partially fixed in #31667. However, there are no tests for exclude, and the behaviour is now inconsistent, since exclude(col__in=queryset.values('col2')) still gives no results when col2 contains NULL. I'll try to provide a patch to clean this up.

comment:9 by Simon Charette, 4 years ago

Cc: Simon Charette added

Thanks for surfacing this inconsistency Adam.

It looks like adding an else clause to deal with a query right hand side value that has explicitly selected fields that can be null (primary keys can't)

https://github.com/django/django/blob/632ccffc49658de5348c51a8918719364be54f37/django/db/models/lookups.py#L387-L389

The RelatedIn lookup will also need to be adjusted

https://github.com/django/django/blob/e74b3d724e5ddfef96d1d66bd1c58e7aae26fc85/django/db/models/fields/related_lookups.py#L98-L99

comment:10 by Adam Sołtysik, 4 years ago

Has patch: set

I've created a patch. No PR yet, though, because one test fails: WindowFunctionTests.test_window_expression_within_subquery. SQL doesn't support filtering by window functions, so I guess the code should be corrected to use another subquery (or maybe an EXISTS query). Unfortunately I don't know how to write this without using QuerySet utilities.

As for RelatedIn, the things are weird here. It seems this case is handled differently and already works correctly, and worked even before #31667 (test_ticket20024_related_in). It was probably fixed in #13815 long ago. There is IS NOT NULL in the subquery, actually there are two next to each other. One is coming from split_exclude() (called in build_filter()), but the other one -- I couldn't find. Also, what looks like another thing to take a closer look at -- there is NOT IN (None) instead of NOT IN (NULL) in the SQL for the first query in that test, when the rhs.discard(None) line is removed.

Edit: I've moved the code to build_filter(), it fits better. New patch here.

Last edited 4 years ago by Adam Sołtysik (previous) (diff)

comment:11 by Adam Sołtysik, 4 years ago

Owner: Adam Sołtysik removed
Status: assignednew

It would be best to make the behaviour of In lookup consistent with RelatedIn. Sadly, I wasn't able to understand how the latter works. So my patch currently crashes with window functions, and it also crashes with OuterRef due to #31714. As much as I'd like to finish this, I don't have enough time and knowledge to look into it further. If anyone comes up with a solution, feel free to claim this issue.

comment:12 by Mariusz Felisiak, 4 years ago

Has patch: unset

comment:13 by Simon Charette, 4 years ago

Owner: set to Simon Charette
Status: newassigned

comment:14 by Simon Charette, 4 years ago

Owner: Simon Charette removed
Status: assignednew

I fear I won't be able to work on this issue in the near future so I'm deassigning myself.

Now that we filter out literal None values from __in=[None] lookup (#31667) we do have a mismatch with __in=nullable_expression but at least the behaviour is consistent with the nullable_field=F('nullable_field') and nullable_field=None asymmetry (#32043).

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