Opened 13 years ago

Closed 13 years ago

#15316 closed Bug (fixed)

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

Reported by: Piotr Czachur Owned by: Aleksandra Sendecka
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: asendecka@…, tomek@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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

Attachments (4)

15316.diff (608 bytes ) - added by Aleksandra Sendecka 13 years ago.
the patch without other patches (the previous one was generated badly)
15316_with_test.diff (2.4 KB ) - added by Aleksandra Sendecka 13 years ago.
patch with tests
ticket15316.diff (6.2 KB ) - added by Aleksandra Sendecka 13 years ago.
patch with more tests
ticket15316.2.diff (10.6 KB ) - added by Aleksandra Sendecka 13 years ago.
Changed patch --- in case isnull=False trim_join() skip trimming join_list

Download all attachments as: .zip

Change History (14)

comment:1 by Russell Keith-Magee, 13 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Łukasz Rekucki, 13 years ago

Severity: Normal
Type: Bug

by Aleksandra Sendecka, 13 years ago

Attachment: 15316.diff added

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

by Aleksandra Sendecka, 13 years ago

Attachment: 15316_with_test.diff added

patch with tests

comment:3 by Aleksandra Sendecka, 13 years ago

Cc: asendecka@… added
Easy pickings: unset
Has patch: set
UI/UX: unset

comment:4 by Russell Keith-Magee, 13 years ago

Patch needs improvement: set

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.

comment:5 by Tomek Paczkowski, 13 years ago

Cc: tomek@… added

by Aleksandra Sendecka, 13 years ago

Attachment: ticket15316.diff added

patch with more tests

comment:6 by Aleksandra Sendecka, 13 years ago

Owner: changed from nobody to 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 :).

comment:7 by Russell Keith-Magee, 13 years ago

Needs tests: set

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.

by Aleksandra Sendecka, 13 years ago

Attachment: ticket15316.2.diff added

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

comment:8 by Aleksandra Sendecka, 13 years ago

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).

comment:9 by Aleksandra Sendecka, 13 years ago

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

comment:10 by Malcolm Tredinnick, 13 years ago

Resolution: fixed
Status: newclosed

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.

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