Opened 3 years ago

Closed 3 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 3 years ago.
Reproduction testcase (simplified)

Download all attachments as: .zip

Change History (18)

comment:1 Changed 3 years ago by weidwonder

Description: modified (diff)

comment:2 Changed 3 years ago by Baptiste Mispelon

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.

Changed 3 years ago by Baptiste Mispelon

Attachment: testcase-26983.diff added

Reproduction testcase (simplified)

comment:3 Changed 3 years ago by Baptiste Mispelon

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 3 years ago by Baptiste Mispelon (previous) (diff)

comment:4 Changed 3 years ago by Claude Paroz

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

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

?

comment:6 in reply to:  5 Changed 3 years ago by Simon Charette

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

Has patch: set

PR resulting of discussions with bmispelon on IRC.

comment:8 Changed 3 years ago by Simon Charette

Triage Stage: AcceptedReady for checkin

comment:9 Changed 3 years ago by Claude Paroz <claude@…>

Resolution: fixed
Status: newclosed

In 272eccf7:

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

Thanks weidwonder for the report.

comment:10 Changed 3 years ago by Claude Paroz <claude@…>

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

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

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

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.

comment:14 in reply to:  13 Changed 3 years ago by Chris Lamb

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 Changed 3 years ago by Tim Graham <timograham@…>

In 9751326:

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

comment:16 Changed 3 years ago by Tim Graham <timograham@…>

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

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