Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#25284 closed Cleanup/optimization (fixed)

QuerySet.filter(related_id=queryset) no longer does implicit __in lookup

Reported by: Richard Eames Owned by: nobody
Component: Documentation Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Consider the following:

Model.objects.filter(related_id = RelatedModel.objects.all())

In django 1.8, the resulting query contains "related_id IN (SELECT id FROM ...)"
In django 1.9, the "IN" is changed to an "=", which causes the query to break in MySql and Postgres.

Is this an intentional change? If so, it needs to be documented as a breaking/backwards in-compatible change.

Test case:

class Egg(models.Model):
  class Meta:
    app_label = 'spam'
  value = models.IntegerField()

class Spam(models.Model):
  class Meta:
    app_label = 'spam'
  egg = models.ForeignKey(Egg)

class TestAssign(TestCase):
  def test_subquery(self):
    egg1 = Egg.objects.create(value=1)
    egg11 = Egg.objects.create(value=1)

    Spam.objects.create(egg=egg1)
    Spam.objects.create(egg=egg11)

    eggs1=Egg.objects.filter(value=1)
    q = Spam.objects.filter(egg__in=eggs1)
    self.assertEqual(2, len(list(q)))
                

Attachments (2)

25284-test.diff (590 bytes) - added by Tim Graham 3 years ago.
25284-test.2.diff (570 bytes) - added by Richard Eames 3 years ago.
Updated test

Download all attachments as: .zip

Change History (16)

comment:1 Changed 3 years ago by Tim Graham

I couldn't reproduce the behavior you describe using your models and tests, or with the attached test for Django's test suite.

Changed 3 years ago by Tim Graham

Attachment: 25284-test.diff added

comment:2 Changed 3 years ago by Richard Eames

Oh, need to remove the "_ _in".
This line needs to change:
q = Spam.objects.filter(egg__in=eggs1)
To:
q = Spam.objects.filter(egg=eggs1)

And in the patch:

+        self.assertEqual(len(Annotation.objects.filter(tag__in=qs1)), 1)

Should be:

+        self.assertEqual(len(Annotation.objects.filter(tag=qs1)), 1)

Changed 3 years ago by Richard Eames

Attachment: 25284-test.2.diff added

Updated test

comment:3 Changed 3 years ago by Richard Eames

Updated the test case, and verified that it fails on PostgreSQL.

comment:4 Changed 3 years ago by Tim Graham

Bisected to b68212f539f206679580afbfd008e7d329c9cd31. It seems to me the implicit __in lookup probably only worked by accident but other opinions are welcome.

comment:5 Changed 3 years ago by Richard Eames

I agree that it worked by accident. My opinion is that it's changed to be explicitly not allowed so the behaviour doesn't differ across backends, and also documented as a breaking change.

comment:6 Changed 3 years ago by Anssi Kääriäinen

This seems to have been non-documented and non-tested feature (probably accidental, too). Changing the behavior definitely counts as a bug fix (even more for cases where doing qs.filter(related__range=otherqs) would result in an IN lookup).

Lets advertise this as loudly as we can in the release notes as a breaking change. It is going to be hard to find these cases in your code manually, but I think 99% of such cases will result in an error in 1.9, as some_id = (select id from other model) will result in an error if there are more than one id returned by the subquery.

comment:7 Changed 3 years ago by Tim Graham

Component: Database layer (models, ORM)Documentation
Has patch: set
Summary: Regression when filtering by subqueryQuerySet.filter(related_id=queryset) no longer does implicit __in lookup
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

comment:8 Changed 3 years ago by Richard Eames

What about explicitly forbidding it? Currently, it only errors in Postgresql. With Mysql it will error if the subquery returns more than one result (but not when there's only one). And with SQLite, it doesn't error at all.

comment:9 Changed 3 years ago by Tim Graham

That might be feasible as a separate enhancement. After a quick look, I couldn't determine if the implementation would be trivial or not.

comment:10 Changed 3 years ago by Tim Graham

Created #25298 for that follow up.

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

Resolution: fixed
Status: newclosed

In d3bc86e:

Fixed #25284 -- Documented removal of implicit QuerySet in lookups.

comment:12 Changed 2 years ago by Tim Graham <timograham@…>

In 9e4fd330:

Refs #25284 -- Corrected an obsolete implicit in lookup example.

Thanks IRC alias rpkilby for the report.

comment:13 Changed 2 years ago by Tim Graham <timograham@…>

In 65cdded2:

[1.10.x] Refs #25284 -- Corrected an obsolete implicit in lookup example.

Thanks IRC alias rpkilby for the report.

Backport of 9e4fd3301d0a4e40cab8df4b51a2b04274f61da0 from master

comment:14 Changed 2 years ago by Tim Graham <timograham@…>

In 82701c3c:

[1.9.x] Refs #25284 -- Corrected an obsolete implicit in lookup example.

Thanks IRC alias rpkilby for the report.

Backport of 9e4fd3301d0a4e40cab8df4b51a2b04274f61da0 from master

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