Filter with isnull=False failing when isnull checked on subclass of FK model

[Piotr Czachur]

class List(models.Model):      
    name = models.CharField(max_length=10) 

class SpecificList(List):      
    specific_name = models.CharField(max_length=10) 
    

class ListItem(models.Model):  
    list = models.ForeignKey(List)  

# Find ListItems that are connected to List which is not SpecificList
>>> ListItem.objects.filter(list__specificlist__isnull=True)
# SELECT **
# FROM list_listitem INNER JOIN list_list ON (list_listitem.list_id = list_list.id) LEFT OUTER JOIN list_specificlist ON (list_list.id = list_specificlist.list_ptr_id) 
# WHERE list_specificlist.list_ptr_id IS NULL

This query looks fine - looks like isnull=True is supported (LEFT INNER JOIN). Let's try isnull=False

# Find ListItems that are connected to List which is SpecificList
>>> ListItem.objects.filter(list__specificlist__isnull=False)
# SELECT ** FROM list_listitem WHERE list_listitem.list_id IS NOT NULL

1) There is no JOIN to list_specificlist. Query results are not what we would expect - it returns all items.

2) WHERE is redundant: list_id is field of type NOT NULL

[Aleksandra Sendecka]

the patch without other patches (the previous one was generated badly)

[Aleksandra Sendecka]

patch with tests

[Russell Keith-Magee]

Looks good to me -- I'd like to see a couple of minor fixes before marking it RFC.

The current if statement checks "value is True" and "not negate" as two separate checks; the patch adds a test to make sure that the "value is True" part isn't actually required, but doesn't validate that it also isn't required for the negation case. There are essentially 4 cases that need to be tested:

• filter(isnull=True)
• exclude(isnull=True)
• filter(isnull=False)
• exclude(isnull=False)

It's entirely possible that some of these cases might be checked elsewhere, but it doesn't hurt to validate all four just to be certain.

It would also be good to validate at a stronger level than count -- for example, if you get the joins wrong, it's possible that you might be validating the existence of a DumbCategory rather than a NamedCategory. It would be better to validate against something that clearly identified that the right CategoryItem object was being returned, rather than just validating that only 1 instance is returned.

Lastly, I think two nasty ORM bugs is enough to earn you a line in the AUTHORS file :-) Feel free to include that change in the updated patch, too.

[Aleksandra Sendecka]

patch with more tests

[Aleksandra Sendecka]

I added the patch with tests for four cases mentioned above. I also changed models for the tests. Validation, apart from checking the count, is now done with asserting if a tested QuerySet is the same as a QuerySet that is filtered with pks of correct objects.

I hope, it is exactly what I was expected to do. I also added myself to AUTHORS (Yay!) as russellm suggested :).

[Russell Keith-Magee]

Malcolm and I have just taken a closer look at this, and while it passes the test, we're not convinced it's the right approach.

The suggested patch is really masking the problem, not fixing it. By promoting the join, it is prevents the subsequent trim_joins from removing the extra joins (that are required) -- but the actual problem is with the trim_joins, because queries with is_null=False shouldn't be promoted (since once you're saying is_null=False, you're guaranteeing that the related data exists, so you don't need to use a LEFT OUTER JOIN)

Malcolm's intuition was that if you tried to reproduce this problem with actual OneToOneFields instead of inheritance, the problem probably wouldn't exist.

[Aleksandra Sendecka]

Changed patch --- in case isnull=False trim_join() skip trimming join_list

[Aleksandra Sendecka]

I changed the patch. Instead of promoting the join I added extra parameter (isnull_skip) which if set to True breaks while loop in trim_joins function. Not sure if it should launch trim_join() at all in case isnull=False. Maybe it's easier just to set values of col and alias in add_filter() in this case (I'll look at this possibility tomorrow). I also added tests for OneToOneField instead of inheritance (just in case).

[Aleksandra Sendecka]

I think I'll change name isnull_skip to sth more appropriate (like 'is_not_null') and add some explanations in trim_join.

[Malcolm Tredinnick]

In [16656]:

Fixed an isnull=False filtering edge-case. Fixes #15316.

The bulk of this patch is due to some fine analysis from Aleksandra
Sendecka.