Code

Opened 3 years ago

Closed 3 years ago

#15316 closed Bug (fixed)

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

Reported by: zimnyx Owned by: ethlinn
Component: Database layer (models, ORM) Version: master
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 ethlinn 3 years ago.
the patch without other patches (the previous one was generated badly)
15316_with_test.diff (2.4 KB) - added by ethlinn 3 years ago.
patch with tests
ticket15316.diff (6.2 KB) - added by ethlinn 3 years ago.
patch with more tests
ticket15316.2.diff (10.6 KB) - added by ethlinn 3 years ago.
Changed patch --- in case isnull=False trim_join() skip trimming join_list

Download all attachments as: .zip

Change History (14)

comment:1 Changed 3 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 3 years ago by lrekucki

  • Severity set to Normal
  • Type set to Bug

Changed 3 years ago by ethlinn

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

Changed 3 years ago by ethlinn

patch with tests

comment:3 Changed 3 years ago by ethlinn

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

comment:4 Changed 3 years ago by russellm

  • 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 Changed 3 years ago by oinopion

  • Cc tomek@… added

Changed 3 years ago by ethlinn

patch with more tests

comment:6 Changed 3 years ago by ethlinn

  • Owner changed from nobody to ethlinn

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

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

Changed 3 years ago by ethlinn

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

comment:8 Changed 3 years ago by ethlinn

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

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

  • Resolution set to fixed
  • Status changed from new to closed

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.