Fixed #25064 -- Allowed empty join columns.

django/db/models/sql/datastructures.py

@@ -66,30 +66,40 @@ class Join(object):
            LEFT OUTER JOIN sometable ON sometable.somecol = othertable.othercol, params
         clause for this join.
         """
+        join_conditions = []
         params = []
-        sql = []
-        alias_str = '' if self.table_alias == self.table_name else (' %s' % self.table_alias)
         qn = compiler.quote_name_unless_alias
         qn2 = connection.ops.quote_name
-        sql.append('%s %s%s ON (' % (self.join_type, qn(self.table_name), alias_str))
+
+        # Add a join condition for each pair of joining columns.
         for index, (lhs_col, rhs_col) in enumerate(self.join_cols):
-            if index != 0:
-                sql.append(' AND ')
-            sql.append('%s.%s = %s.%s' % (
+            join_conditions.append('%s.%s = %s.%s' % (
                 qn(self.parent_alias),
                 qn2(lhs_col),
                 qn(self.table_alias),
                 qn2(rhs_col),
             ))
+
+        # Add a single condition inside parentheses for whatever
+        # get_extra_restriction() returns.
         extra_cond = self.join_field.get_extra_restriction(
             compiler.query.where_class, self.table_alias, self.parent_alias)
         if extra_cond:
             extra_sql, extra_params = compiler.compile(extra_cond)
-            extra_sql = 'AND (%s)' % extra_sql
+            join_conditions.append('(%s)' % extra_sql)
             params.extend(extra_params)
-            sql.append('%s' % extra_sql)
-        sql.append(')')
-        return ' '.join(sql), params
+
+        if not join_conditions:
+            # This might be a rel on the other end of an actual declared field.
+            declared_field = getattr(self.join_field, 'field', self.join_field)
+            raise ValueError(
+                "Join generated an empty ON clause. %s did not yield either "
+                "joining columns or extra restrictions." % declared_field.__class__
+            )
+        on_clause_sql = ' AND '.join(join_conditions)
+        alias_str = '' if self.table_alias == self.table_name else (' %s' % self.table_alias)
+        sql = '%s %s%s ON (%s)' % (self.join_type, qn(self.table_name), alias_str, on_clause_sql)
+        return sql, params
 
     def relabeled_clone(self, change_map):
         new_parent_alias = change_map.get(self.parent_alias, self.parent_alias)

tests/foreign_object/models/__init__.py

@@ -1,9 +1,10 @@
 from .article import (
     Article, ArticleIdea, ArticleTag, ArticleTranslation, NewsArticle,
 )
+from .empty_join import SlugPage
 from .person import Country, Friendship, Group, Membership, Person
 
 __all__ = [
     'Article', 'ArticleIdea', 'ArticleTag', 'ArticleTranslation', 'Country',
-    'Friendship', 'Group', 'Membership', 'NewsArticle', 'Person',
+    'Friendship', 'Group', 'Membership', 'NewsArticle', 'Person', 'SlugPage',
 ]

tests/foreign_object/models/empty_join.py

@@ -0,0 +1,100 @@
+from django.db import models
+from django.db.models.fields.related import (
+    ForeignObjectRel, ForeignRelatedObjectsDescriptor,
+)
+from django.db.models.lookups import StartsWith
+from django.db.models.query_utils import PathInfo
+from django.utils.encoding import python_2_unicode_compatible
+
+
+class CustomForeignObjectRel(ForeignObjectRel):
+    """
+    Define some extra Field methods so this Rel acts more like a Field, which
+    lets us use ForeignRelatedObjectsDescriptor in both directions.
+    """
+    @property
+    def foreign_related_fields(self):
+        return tuple(lhs_field for lhs_field, rhs_field in self.field.related_fields)
+
+    def get_attname(self):
+        return self.name
+
+
+class StartsWithRelation(models.ForeignObject):
+    """
+    A ForeignObject that uses StartsWith operator in its joins instead of
+    the default equality operator. This is logically a many-to-many relation
+    and creates a ForeignRelatedObjectsDescriptor in both directions.
+    """
+    auto_created = False
+
+    many_to_many = False
+    many_to_one = True
+    one_to_many = False
+    one_to_one = False
+
+    rel_class = CustomForeignObjectRel
+
+    def __init__(self, *args, **kwargs):
+        kwargs['on_delete'] = models.DO_NOTHING
+        super(StartsWithRelation, self).__init__(*args, **kwargs)
+
+    @property
+    def field(self):
+        """
+        Makes ForeignRelatedObjectsDescriptor work in both directions.
+        """
+        return self.remote_field
+
+    def get_extra_restriction(self, where_class, alias, related_alias):
+        to_field = self.remote_field.model._meta.get_field(self.to_fields[0])
+        from_field = self.model._meta.get_field(self.from_fields[0])
+        return StartsWith(to_field.get_col(alias), from_field.get_col(related_alias))
+
+    def get_joining_columns(self, reverse_join=False):
+        return tuple()
+
+    def get_path_info(self):
+        to_opts = self.remote_field.model._meta
+        from_opts = self.model._meta
+        return [PathInfo(from_opts, to_opts, (to_opts.pk,), self, False, False)]
+
+    def get_reverse_path_info(self):
+        to_opts = self.model._meta
+        from_opts = self.remote_field.model._meta
+        return [PathInfo(from_opts, to_opts, (to_opts.pk,), self.remote_field, False, False)]
+
+    def contribute_to_class(self, cls, name, virtual_only=False):
+        super(StartsWithRelation, self).contribute_to_class(cls, name, virtual_only)
+        setattr(cls, self.name, ForeignRelatedObjectsDescriptor(self))
+
+
+class BrokenContainsRelation(StartsWithRelation):
+    """
+    This model is designed to yield no join conditions and
+    raise an exception in ``Join.as_sql()``.
+    """
+    def get_extra_restriction(self, where_class, alias, related_alias):
+        return None
+
+
+@python_2_unicode_compatible
+class SlugPage(models.Model):
+    slug = models.CharField(max_length=20)
+    descendants = StartsWithRelation(
+        'self',
+        from_fields=['slug'],
+        to_fields=['slug'],
+        related_name='ascendants',
+    )
+    containers = BrokenContainsRelation(
+        'self',
+        from_fields=['slug'],
+        to_fields=['slug'],
+    )
+
+    class Meta:
+        ordering = ['slug']
+
+    def __str__(self):
+        return 'SlugPage %s' % self.slug

tests/foreign_object/test_empty_join.py

@@ -0,0 +1,47 @@
+from django.test import TestCase
+
+from .models import SlugPage
+
+
+class RestrictedConditionsTests(TestCase):
+    def setUp(self):
+        slugs = [
+            'a',
+            'a/a',
+            'a/b',
+            'a/b/a',
+            'x',
+            'x/y/z',
+        ]
+        SlugPage.objects.bulk_create([SlugPage(slug=slug) for slug in slugs])
+
+    def test_restrictions_with_no_joining_columns(self):
+        """
+        Test that it's possible to create a working related field that doesn't
+        use any joining columns, as long as an extra restriction is supplied.
+        """
+        a = SlugPage.objects.get(slug='a')
+        self.assertListEqual(
+            [p.slug for p in SlugPage.objects.filter(ascendants=a)],
+            ['a', 'a/a', 'a/b', 'a/b/a'],
+        )
+        self.assertEqual(
+            [p.slug for p in a.descendants.all()],
+            ['a', 'a/a', 'a/b', 'a/b/a'],
+        )
+
+        aba = SlugPage.objects.get(slug='a/b/a')
+        self.assertListEqual(
+            [p.slug for p in SlugPage.objects.filter(descendants__in=[aba])],
+            ['a', 'a/b', 'a/b/a'],
+        )
+        self.assertListEqual(
+            [p.slug for p in aba.ascendants.all()],
+            ['a', 'a/b', 'a/b/a'],
+        )
+
+    def test_empty_join_conditions(self):
+        x = SlugPage.objects.get(slug='x')
+        message = "Join generated an empty ON clause."
+        with self.assertRaisesMessage(ValueError, message):
+            list(SlugPage.objects.filter(containers=x))