From 5f7991c42cff73b6278106d499d719b726f85ead Mon Sep 17 00:00:00 2001 From: Mariusz Felisiak Date: Mon, 15 Apr 2019 10:35:37 +0200 Subject: [PATCH] Fixed #30325 -- Reverted "Fixed #29725 -- Removed unnecessary join in QuerySet.count() and exists() on a many-to-many relation." This reverts commit 1299421cadc4fcf63585f2f88337078e43e660e0 due to a regression with custom managers. --- .../db/models/fields/related_descriptors.py | 27 ------------ docs/releases/2.2.1.txt | 4 ++ tests/many_to_many/models.py | 10 ----- tests/many_to_many/tests.py | 42 +------------------ 4 files changed, 6 insertions(+), 77 deletions(-) diff --git a/django/db/models/fields/related_descriptors.py b/django/db/models/fields/related_descriptors.py index 2b426c37a5..c1295ee960 100644 --- a/django/db/models/fields/related_descriptors.py +++ b/django/db/models/fields/related_descriptors.py @@ -929,33 +929,6 @@ def create_forward_many_to_many_manager(superclass, rel, reverse): False, ) - @property - def constrained_target(self): - # If the through relation's target field's foreign integrity is - # enforced, the query can be performed solely against the through - # table as the INNER JOIN'ing against target table is unnecessary. - if not self.target_field.db_constraint: - return None - db = router.db_for_read(self.through, instance=self.instance) - if not connections[db].features.supports_foreign_keys: - return None - hints = {'instance': self.instance} - manager = self.through._base_manager.db_manager(db, hints=hints) - filters = {self.source_field_name: self.instance.pk} - # Nullable target rows must be excluded as well as they would have - # been filtered out from an INNER JOIN. - if self.target_field.null: - filters['%s__isnull' % self.target_field_name] = False - return manager.filter(**filters) - - def exists(self): - constrained_target = self.constrained_target - return constrained_target.exists() if constrained_target else super().exists() - - def count(self): - constrained_target = self.constrained_target - return constrained_target.count() if constrained_target else super().count() - def add(self, *objs, through_defaults=None): self._remove_prefetched_objects() db = router.db_for_write(self.through, instance=self.instance) diff --git a/docs/releases/2.2.1.txt b/docs/releases/2.2.1.txt index dcad58b2fd..3a2453dbd1 100644 --- a/docs/releases/2.2.1.txt +++ b/docs/releases/2.2.1.txt @@ -29,3 +29,7 @@ Bugfixes * Prevented :djadmin:`makemigrations` from generating infinite migrations for check constraints and partial indexes when ``condition`` contains a :class:`~python:range` object (:ticket:`30350`). + +* Reverted an optimization in Django 2.2 (:ticket:`29725`) that caused the + inconsistent behavior of ``count()`` and ``exists()`` on a reverse + many-to-many relationship with a custom manager (:ticket:`30325`). diff --git a/tests/many_to_many/models.py b/tests/many_to_many/models.py index a2590b8b36..28d23f2a82 100644 --- a/tests/many_to_many/models.py +++ b/tests/many_to_many/models.py @@ -55,13 +55,3 @@ class InheritedArticleA(AbstractArticle): class InheritedArticleB(AbstractArticle): pass - - -class NullableTargetArticle(models.Model): - headline = models.CharField(max_length=100) - publications = models.ManyToManyField(Publication, through='NullablePublicationThrough') - - -class NullablePublicationThrough(models.Model): - article = models.ForeignKey(NullableTargetArticle, models.CASCADE) - publication = models.ForeignKey(Publication, models.CASCADE, null=True) diff --git a/tests/many_to_many/tests.py b/tests/many_to_many/tests.py index f0156bbe8d..9dc53307a1 100644 --- a/tests/many_to_many/tests.py +++ b/tests/many_to_many/tests.py @@ -1,13 +1,9 @@ from unittest import mock -from django.db import connection, transaction +from django.db import transaction from django.test import TestCase, skipUnlessDBFeature -from django.test.utils import CaptureQueriesContext -from .models import ( - Article, InheritedArticleA, InheritedArticleB, NullablePublicationThrough, - NullableTargetArticle, Publication, -) +from .models import Article, InheritedArticleA, InheritedArticleB, Publication class ManyToManyTests(TestCase): @@ -584,37 +580,3 @@ class ManyToManyTests(TestCase): ] ) self.assertQuerysetEqual(b.publications.all(), ['']) - - -class ManyToManyQueryTests(TestCase): - @classmethod - def setUpTestData(cls): - cls.article = Article.objects.create(headline='Django lets you build Web apps easily') - cls.nullable_target_article = NullableTargetArticle.objects.create(headline='The python is good') - NullablePublicationThrough.objects.create(article=cls.nullable_target_article, publication=None) - - @skipUnlessDBFeature('supports_foreign_keys') - def test_count_join_optimization(self): - with CaptureQueriesContext(connection) as query: - self.article.publications.count() - self.assertNotIn('JOIN', query[0]['sql']) - self.assertEqual(self.nullable_target_article.publications.count(), 0) - - def test_count_join_optimization_disabled(self): - with mock.patch.object(connection.features, 'supports_foreign_keys', False), \ - CaptureQueriesContext(connection) as query: - self.article.publications.count() - self.assertIn('JOIN', query[0]['sql']) - - @skipUnlessDBFeature('supports_foreign_keys') - def test_exists_join_optimization(self): - with CaptureQueriesContext(connection) as query: - self.article.publications.exists() - self.assertNotIn('JOIN', query[0]['sql']) - self.assertIs(self.nullable_target_article.publications.exists(), False) - - def test_exists_join_optimization_disabled(self): - with mock.patch.object(connection.features, 'supports_foreign_keys', False), \ - CaptureQueriesContext(connection) as query: - self.article.publications.exists() - self.assertIn('JOIN', query[0]['sql'])