diff --git a/django/db/models/sql/datastructures.py b/django/db/models/sql/datastructures.py index a245e451d6..1880f4558f 100644 --- a/django/db/models/sql/datastructures.py +++ b/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) diff --git a/tests/foreign_object/models/__init__.py b/tests/foreign_object/models/__init__.py index c7063ff387..8fe4ff10df 100644 --- a/tests/foreign_object/models/__init__.py +++ b/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', ] diff --git a/tests/foreign_object/models/empty_join.py b/tests/foreign_object/models/empty_join.py new file mode 100644 index 0000000000..3bf121bf39 --- /dev/null +++ b/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 diff --git a/tests/foreign_object/test_empty_join.py b/tests/foreign_object/test_empty_join.py new file mode 100644 index 0000000000..a043595ce2 --- /dev/null +++ b/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))