Opened 8 years ago

Closed 8 years ago

#26983 closed Bug (fixed)

Boolean filter "field__isnull=False" not working with ForeignKey with to_field

Reported by: weidwonder Owned by: nobody
Component: Database layer (models, ORM) Version: 1.10
Severity: Normal Keywords: queryset
Cc: 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 weidwonder)

models Manager:

class ModelManager(models.Manager):
    def get_queryset(self):
        return super(ModelManager, self).get_queryset().filter(parent__isnull=False)

models:

class Category(models.Model):
    parent = models.ForeignKey('Category', to_field='slug', db_column='parent',
                               max_length=32, blank=True, null=True,
                               related_name='models')
    name = models.CharField(max_length=50, blank=True, null=True)
    ....
    vehicle_models = ModelManager()

    class Meta:
        db_table = 'open_category'
        ordering = ['pinyin']

query:

Category.vehicle_models.filter(slug=model_slug).values('name').query

query's sql output is:

SELECT `open_category`.`name` FROM `open_category` WHERE (`open_category`.`parent` IS NULL AND `open_category`.`slug` = "suteng") ORDER BY `open_category`.`pinyin` ASC

it should be open_category.parent IS NOT NULL, I downgrade django to 1.9.9, it works.

Attachments (1)

testcase-26983.diff (1.0 KB ) - added by Baptiste Mispelon 8 years ago.
Reproduction testcase (simplified)

Download all attachments as: .zip

Change History (18)

comment:1 by weidwonder, 8 years ago

Description: modified (diff)

comment:2 by Baptiste Mispelon, 8 years ago

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Hi,

Using the attached testcase based on your description, I managed to confirm the issue and tracked it down to commit 388bb5bd9aa3cd43825cd8a3632a57d8204f875f.

Thanks.

by Baptiste Mispelon, 8 years ago

Attachment: testcase-26983.diff added

Reproduction testcase (simplified)

comment:3 by Baptiste Mispelon, 8 years ago

I simplified the testcase a bit to get rid of some "moving parts".

The key to the failure seems to be the to_field attribute of the ForeignKey. Without it, the query seems to be generated correctly.

--Edit--

Following the advice of kezabelle on IRC, I placed a PDB trace right before https://github.com/django/django/blob/master/django/db/models/lookups.py#L436 and found out that at this point, the value of self.rhs is 'False', that is the string 'False' rather than a boolean object, which triggers the wrong branch of the if statement.

--Edit 2--
Digging a bit deeper, it turns out that this line is where False gets converted to 'False': https://github.com/django/django/blob/master/django/db/models/fields/related_lookups.py#L100

Last edited 8 years ago by Baptiste Mispelon (previous) (diff)

comment:4 by Claude Paroz, 8 years ago

Summary: models.Manager query "filter(field__isnull=False)" not working in 1.10Boolean filter "field__isnull=False" not working with ForeignKey with to_field

comment:5 by Matthew Wilkes, 8 years ago

I don't believe that IsNull lookups should be converting their values to the type of the parent field, however the RelatedLookupMixin has useful looking code for ensuring that MultiColSource is handled correctly.

Would the fix for this be as simple as:

class RelatedIsNull(RelatedLookupMixin, IsNull):
    def get_prep_lookup(self):
        return self.rhs

?

in reply to:  5 comment:6 by Simon Charette, 8 years ago

Replying to MatthewWilkes:

I don't believe that IsNull lookups should be converting their values to the type of the parent field, however the RelatedLookupMixin has useful looking code for ensuring that MultiColSource is handled correctly.

Would the fix for this be as simple as:

class RelatedIsNull(RelatedLookupMixin, IsNull):
    def get_prep_lookup(self):
        return self.rhs

?

return super(RelatedLookupMixin, self).get_prep_lookup() would be more appropriate.

comment:7 by Claude Paroz, 8 years ago

Has patch: set

PR resulting of discussions with bmispelon on IRC.

comment:8 by Simon Charette, 8 years ago

Triage Stage: AcceptedReady for checkin

comment:9 by Claude Paroz <claude@…>, 8 years ago

Resolution: fixed
Status: newclosed

In 272eccf7:

Fixed #26983 -- Fixed isnull filtering on ForeignKey with to_field

Thanks weidwonder for the report.

comment:10 by Claude Paroz <claude@…>, 8 years ago

In 6757c946:

[1.10.x] Fixed #26983 -- Fixed isnull filtering on ForeignKey with to_field

Thanks weidwonder for the report.
Backport of 272eccf7ff0ced609e5a5e3bb6b4a40e773e0e66 from master.

comment:11 by Chris Lamb, 8 years ago

Resolution: fixed
Status: closednew

This also happens for field_isnull=False where field is a ForeignKey to a model with a CharField that is primary_key=True. Again, self.rhs is converted from False to 'False, ie a bool to a str`. Reopening as the pach does not fix this.

comment:12 by Chris Lamb, 8 years ago

After writing a testcase (available here: https://gist.github.com/lamby/394b712e9e9ff1e868a20e67d4ba482b/raw) "git bisect" told me that it was indeed fixed with the above change. Investigating more, what happened was that I was applying the patch against RelatedIninstead of RelatedLookupMixin. Apologies for the noise there.

However, I am still correct in that the bug is not strictly to_field specific as it affects this primary_key case. Therefore, I suggest:

  • You update the release documentation, etc. to reflect this
  • You add my test (or similar) to prevent a regression

In light of this, I am not resolving this ticket so that these action items do not get overlooked.

comment:13 by Tim Graham, 8 years ago

Has patch: unset
Severity: Release blockerNormal
Triage Stage: Ready for checkinAccepted

Could you send a pull request with those items? You can use "Refs #26983 --" in the commit message.

in reply to:  13 comment:14 by Chris Lamb, 8 years ago

Replying to timgraham:

Could you send a pull request with those items? You can use "Refs #26983 --" in the commit message.

https://github.com/django/django/pull/7104

comment:15 by Tim Graham <timograham@…>, 8 years ago

In 9751326:

Refs #26983 -- Added test for isnull lookup to CharField with primary_key=True.

comment:16 by Tim Graham <timograham@…>, 8 years ago

In c24a47b3:

[1.10.x] Refs #26983 -- Added test for isnull lookup to CharField with primary_key=True.

Backport of 97513269d73520d10722bbd10404be6ac4d48d07 from master

comment:17 by Tim Graham, 8 years ago

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top