From 87b1ad6e7351464c60e751b483d9dfce3a2d3382 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nad=C3=A8ge=20Michel?= Date: Fri, 19 Apr 2019 18:12:04 +0200 Subject: [PATCH] Fixed #30421 -- Allowed symmetrical intermediate table for self-referential ManyToManyField. --- AUTHORS | 1 + django/db/models/fields/related.py | 12 ---- .../db/models/fields/related_descriptors.py | 11 +-- docs/ref/checks.txt | 2 +- docs/ref/models/fields.txt | 21 ++++-- docs/releases/3.0.txt | 3 + .../test_relative_fields.py | 64 ----------------- tests/m2m_recursive/models.py | 7 ++ tests/m2m_recursive/tests.py | 58 +++++++++++++++ tests/m2m_through/models.py | 7 ++ tests/m2m_through/tests.py | 71 ++++++++++++++++++- 11 files changed, 167 insertions(+), 90 deletions(-) diff --git a/AUTHORS b/AUTHORS index 31f95e85cf..9bae13abb0 100644 --- a/AUTHORS +++ b/AUTHORS @@ -632,6 +632,7 @@ answer newbie questions, and generally made Django that much better: msundstr Mushtaq Ali Mykola Zamkovoi + Nadège Michel Nagy Károly Nasimul Haque Nasir Hussain diff --git a/django/db/models/fields/related.py b/django/db/models/fields/related.py index 1e54bd6c6e..56cee83a5e 100644 --- a/django/db/models/fields/related.py +++ b/django/db/models/fields/related.py @@ -1234,18 +1234,6 @@ class ManyToManyField(RelatedField): to_model_name = to_model._meta.object_name relationship_model_name = self.remote_field.through._meta.object_name self_referential = from_model == to_model - - # Check symmetrical attribute. - if (self_referential and self.remote_field.symmetrical and - not self.remote_field.through._meta.auto_created): - errors.append( - checks.Error( - 'Many-to-many fields with intermediate tables must not be symmetrical.', - obj=self, - id='fields.E332', - ) - ) - # Count foreign keys in intermediate model if self_referential: seen_self = sum( diff --git a/django/db/models/fields/related_descriptors.py b/django/db/models/fields/related_descriptors.py index b7330cf694..886e99e190 100644 --- a/django/db/models/fields/related_descriptors.py +++ b/django/db/models/fields/related_descriptors.py @@ -938,11 +938,14 @@ def create_forward_many_to_many_manager(superclass, rel, reverse): through_defaults=through_defaults, ) # If this is a symmetrical m2m relation to self, add the mirror - # entry in the m2m table. `through_defaults` aren't used here - # because of the system check error fields.E332: Many-to-many - # fields with intermediate tables must not be symmetrical. + # entry in the m2m table. if self.symmetrical: - self._add_items(self.target_field_name, self.source_field_name, *objs) + self._add_items( + self.target_field_name, + self.source_field_name, + *objs, + through_defaults=through_defaults, + ) add.alters_data = True def remove(self, *objs): diff --git a/docs/ref/checks.txt b/docs/ref/checks.txt index a15eb558fe..1a388c6b37 100644 --- a/docs/ref/checks.txt +++ b/docs/ref/checks.txt @@ -222,7 +222,7 @@ Related fields * **fields.E331**: Field specifies a many-to-many relation through model ````, which has not been installed. * **fields.E332**: Many-to-many fields with intermediate tables must not be - symmetrical. + symmetrical. *This check appeared before Django 3.0.* * **fields.E333**: The model is used as an intermediate model by ````, but it has more than two foreign keys to ````, which is ambiguous. You must specify which two foreign keys Django should use via the diff --git a/docs/ref/models/fields.txt b/docs/ref/models/fields.txt index 7ca6b2c619..758c21c7bc 100644 --- a/docs/ref/models/fields.txt +++ b/docs/ref/models/fields.txt @@ -1537,6 +1537,11 @@ that control how the relationship functions. add the descriptor for the reverse relationship, allowing :class:`ManyToManyField` relationships to be non-symmetrical. + .. versionchanged:: 3.0 + + Specifying ``symmetrical=True`` for recursive many-to-many + relationships using an intermediary model was allowed. + .. attribute:: ManyToManyField.through Django will automatically generate a table to manage many-to-many @@ -1549,6 +1554,16 @@ that control how the relationship functions. :ref:`extra data with a many-to-many relationship `. + .. note:: + + Recursive relationships using an intermediary model and defined as + symmetrical (that is, with :attr:`symmetrical=True + `, which is the default) can't determine + the reverse accessors names, as they would be the same. You need to set + a :attr:`~ForeignKey.related_name` to at least one of them. If you'd + prefer Django not to create a backwards relation, set ``related_name`` + to ``'+'``. + If you don't specify an explicit ``through`` model, there is still an implicit ``through`` model class you can use to directly access the table created to hold the association. It has three fields to link the models. @@ -1624,12 +1639,6 @@ that control how the relationship functions. foreign keys to the model, or you want to explicitly specify which two Django should use. - Recursive relationships using an intermediary model are always defined as - non-symmetrical -- that is, with :attr:`symmetrical=False ` - -- therefore, there is the concept of a "source" and a "target". In that - case ``'field1'`` will be treated as the "source" of the relationship and - ``'field2'`` as the "target". - .. attribute:: ManyToManyField.db_table The name of the table to create for storing the many-to-many data. If this diff --git a/docs/releases/3.0.txt b/docs/releases/3.0.txt index 51d7f7c8bf..62c6080cf7 100644 --- a/docs/releases/3.0.txt +++ b/docs/releases/3.0.txt @@ -254,6 +254,9 @@ Models * :class:`~django.db.models.FilePathField` now accepts a callable ``path``. +* Allowed symmetrical intermediate table for self-referential + :class:`~django.db.models.ManyToManyField`. + Requests and Responses ~~~~~~~~~~~~~~~~~~~~~~ diff --git a/tests/invalid_models_tests/test_relative_fields.py b/tests/invalid_models_tests/test_relative_fields.py index e68dd41c6f..4ac0c547a9 100644 --- a/tests/invalid_models_tests/test_relative_fields.py +++ b/tests/invalid_models_tests/test_relative_fields.py @@ -260,24 +260,6 @@ class RelativeFieldTests(SimpleTestCase): field = Group._meta.get_field('members') self.assertEqual(field.check(from_model=Group), []) - def test_symmetrical_self_referential_field(self): - class Person(models.Model): - # Implicit symmetrical=False. - friends = models.ManyToManyField('self', through="Relationship") - - class Relationship(models.Model): - first = models.ForeignKey(Person, models.CASCADE, related_name="rel_from_set") - second = models.ForeignKey(Person, models.CASCADE, related_name="rel_to_set") - - field = Person._meta.get_field('friends') - self.assertEqual(field.check(from_model=Person), [ - Error( - 'Many-to-many fields with intermediate tables must not be symmetrical.', - obj=field, - id='fields.E332', - ), - ]) - def test_too_many_foreign_keys_in_self_referential_model(self): class Person(models.Model): friends = models.ManyToManyField('self', through="InvalidRelationship", symmetrical=False) @@ -301,52 +283,6 @@ class RelativeFieldTests(SimpleTestCase): ), ]) - def test_symmetric_self_reference_with_intermediate_table(self): - class Person(models.Model): - # Explicit symmetrical=True. - friends = models.ManyToManyField('self', through="Relationship", symmetrical=True) - - class Relationship(models.Model): - first = models.ForeignKey(Person, models.CASCADE, related_name="rel_from_set") - second = models.ForeignKey(Person, models.CASCADE, related_name="rel_to_set") - - field = Person._meta.get_field('friends') - self.assertEqual(field.check(from_model=Person), [ - Error( - 'Many-to-many fields with intermediate tables must not be symmetrical.', - obj=field, - id='fields.E332', - ), - ]) - - def test_symmetric_self_reference_with_intermediate_table_and_through_fields(self): - """ - Using through_fields in a m2m with an intermediate model shouldn't - mask its incompatibility with symmetry. - """ - class Person(models.Model): - # Explicit symmetrical=True. - friends = models.ManyToManyField( - 'self', - symmetrical=True, - through="Relationship", - through_fields=('first', 'second'), - ) - - class Relationship(models.Model): - first = models.ForeignKey(Person, models.CASCADE, related_name="rel_from_set") - second = models.ForeignKey(Person, models.CASCADE, related_name="rel_to_set") - referee = models.ForeignKey(Person, models.CASCADE, related_name="referred") - - field = Person._meta.get_field('friends') - self.assertEqual(field.check(from_model=Person), [ - Error( - 'Many-to-many fields with intermediate tables must not be symmetrical.', - obj=field, - id='fields.E332', - ), - ]) - def test_foreign_key_to_abstract_model(self): class AbstractModel(models.Model): class Meta: diff --git a/tests/m2m_recursive/models.py b/tests/m2m_recursive/models.py index fd4f4ad166..a9f47770d6 100644 --- a/tests/m2m_recursive/models.py +++ b/tests/m2m_recursive/models.py @@ -22,7 +22,14 @@ from django.db import models class Person(models.Model): name = models.CharField(max_length=20) friends = models.ManyToManyField('self') + colleagues = models.ManyToManyField('self', symmetrical=True, through='Colleague') idols = models.ManyToManyField('self', symmetrical=False, related_name='stalkers') def __str__(self): return self.name + + +class Colleague(models.Model): + first = models.ForeignKey(Person, models.CASCADE) + second = models.ForeignKey(Person, models.CASCADE, related_name='+') + first_meet = models.DateField() diff --git a/tests/m2m_recursive/tests.py b/tests/m2m_recursive/tests.py index 0d992070a2..d0799f07d2 100644 --- a/tests/m2m_recursive/tests.py +++ b/tests/m2m_recursive/tests.py @@ -1,3 +1,5 @@ +import datetime + from django.test import TestCase from .models import Person @@ -59,3 +61,59 @@ class RecursiveM2MTests(TestCase): self.a.idols.add(self.a) self.assertSequenceEqual(self.a.idols.all(), [self.a]) self.assertSequenceEqual(self.a.stalkers.all(), [self.a]) + + +class RecursiveSymmetricalM2MThroughTests(TestCase): + @classmethod + def setUpTestData(cls): + cls.a, cls.b, cls.c, cls.d = [ + Person.objects.create(name=name) + for name in ['Anne', 'Bill', 'Chuck', 'David'] + ] + cls.a.colleagues.add(cls.b, cls.c, through_defaults={ + 'first_meet': datetime.date(2013, 1, 5), + }) + # Add m2m for Anne and Chuck in reverse direction. + cls.d.colleagues.add(cls.a, cls.c, through_defaults={ + 'first_meet': datetime.date(2015, 6, 15), + }) + + def test_recursive_m2m_all(self): + for person, colleagues in ( + (self.a, [self.b, self.c, self.d]), + (self.b, [self.a]), + (self.c, [self.a, self.d]), + (self.d, [self.a, self.c]), + ): + with self.subTest(person=person): + self.assertSequenceEqual(person.colleagues.all(), colleagues) + + def test_recursive_m2m_reverse_add(self): + # Add m2m for Anne in reverse direction. + self.b.colleagues.add(self.a, through_defaults={ + 'first_meet': datetime.date(2013, 1, 5), + }) + self.assertSequenceEqual(self.a.colleagues.all(), [self.b, self.c, self.d]) + self.assertSequenceEqual(self.b.colleagues.all(), [self.a]) + + def test_recursive_m2m_remove(self): + self.b.colleagues.remove(self.a) + self.assertSequenceEqual(self.a.colleagues.all(), [self.c, self.d]) + self.assertSequenceEqual(self.b.colleagues.all(), []) + + def test_recursive_m2m_clear(self): + # Clear m2m for Anne. + self.a.colleagues.clear() + self.assertSequenceEqual(self.a.friends.all(), []) + # Reverse m2m relationships is removed. + self.assertSequenceEqual(self.c.colleagues.all(), [self.d]) + self.assertSequenceEqual(self.d.colleagues.all(), [self.c]) + + def test_recursive_m2m_set(self): + # Set new relationships for Chuck. + self.c.colleagues.set([self.b, self.d], through_defaults={ + 'first_meet': datetime.date(2013, 1, 5), + }) + self.assertSequenceEqual(self.c.colleagues.order_by('name'), [self.b, self.d]) + # Reverse m2m relationships is removed. + self.assertSequenceEqual(self.a.colleagues.order_by('name'), [self.b, self.d]) diff --git a/tests/m2m_through/models.py b/tests/m2m_through/models.py index 141d02daf8..53d04a05f3 100644 --- a/tests/m2m_through/models.py +++ b/tests/m2m_through/models.py @@ -72,6 +72,7 @@ class TestNoDefaultsOrNulls(models.Model): class PersonSelfRefM2M(models.Model): name = models.CharField(max_length=5) friends = models.ManyToManyField('self', through="Friendship", symmetrical=False) + sym_friends = models.ManyToManyField('self', through='SymmetricalFriendship', symmetrical=True) def __str__(self): return self.name @@ -83,6 +84,12 @@ class Friendship(models.Model): date_friended = models.DateTimeField() +class SymmetricalFriendship(models.Model): + first = models.ForeignKey(PersonSelfRefM2M, models.CASCADE) + second = models.ForeignKey(PersonSelfRefM2M, models.CASCADE, related_name='+') + date_friended = models.DateField() + + # Custom through link fields class Event(models.Model): title = models.CharField(max_length=50) diff --git a/tests/m2m_through/tests.py b/tests/m2m_through/tests.py index 2921a1a260..deb9015ba6 100644 --- a/tests/m2m_through/tests.py +++ b/tests/m2m_through/tests.py @@ -1,4 +1,4 @@ -from datetime import datetime +from datetime import date, datetime from operator import attrgetter from django.db import IntegrityError @@ -7,7 +7,7 @@ from django.test import TestCase from .models import ( CustomMembership, Employee, Event, Friendship, Group, Ingredient, Invitation, Membership, Person, PersonSelfRefM2M, Recipe, RecipeIngredient, - Relationship, + Relationship, SymmetricalFriendship, ) @@ -401,7 +401,7 @@ class M2mThroughReferentialTests(TestCase): attrgetter("name") ) - def test_self_referential_symmetrical(self): + def test_self_referential_non_symmetrical_both(self): tony = PersonSelfRefM2M.objects.create(name="Tony") chris = PersonSelfRefM2M.objects.create(name="Chris") Friendship.objects.create( @@ -439,6 +439,71 @@ class M2mThroughReferentialTests(TestCase): attrgetter('name') ) + def test_self_referential_symmetrical(self): + tony = PersonSelfRefM2M.objects.create(name='Tony') + chris = PersonSelfRefM2M.objects.create(name='Chris') + SymmetricalFriendship.objects.create( + first=tony, second=chris, date_friended=date.today(), + ) + self.assertSequenceEqual(tony.sym_friends.all(), [chris]) + # Manually created symmetrical m2m relation doesn't add mirror entry + # automatically. + self.assertSequenceEqual(chris.sym_friends.all(), []) + SymmetricalFriendship.objects.create( + first=chris, second=tony, date_friended=date.today() + ) + self.assertSequenceEqual(chris.sym_friends.all(), [tony]) + + def test_add_on_symmetrical_m2m_with_intermediate_model(self): + tony = PersonSelfRefM2M.objects.create(name='Tony') + chris = PersonSelfRefM2M.objects.create(name='Chris') + date_friended = date(2017, 1, 3) + tony.sym_friends.add(chris, through_defaults={'date_friended': date_friended}) + self.assertSequenceEqual(tony.sym_friends.all(), [chris]) + self.assertSequenceEqual(chris.sym_friends.all(), [tony]) + friendship = tony.symmetricalfriendship_set.get() + self.assertEqual(friendship.date_friended, date_friended) + + def test_set_on_symmetrical_m2m_with_intermediate_model(self): + tony = PersonSelfRefM2M.objects.create(name='Tony') + chris = PersonSelfRefM2M.objects.create(name='Chris') + anne = PersonSelfRefM2M.objects.create(name='Anne') + kate = PersonSelfRefM2M.objects.create(name='Kate') + date_friended_add = date(2013, 1, 5) + date_friended_set = date.today() + tony.sym_friends.add( + anne, chris, + through_defaults={'date_friended': date_friended_add}, + ) + tony.sym_friends.set( + [anne, kate], + through_defaults={'date_friended': date_friended_set}, + ) + self.assertSequenceEqual(tony.sym_friends.all(), [anne, kate]) + self.assertSequenceEqual(anne.sym_friends.all(), [tony]) + self.assertSequenceEqual(kate.sym_friends.all(), [tony]) + self.assertEqual( + kate.symmetricalfriendship_set.get().date_friended, + date_friended_set, + ) + # Date is preserved. + self.assertEqual( + anne.symmetricalfriendship_set.get().date_friended, + date_friended_add, + ) + # Recreate relationship. + tony.sym_friends.set( + [anne], + clear=True, + through_defaults={'date_friended': date_friended_set}, + ) + self.assertSequenceEqual(tony.sym_friends.all(), [anne]) + self.assertSequenceEqual(anne.sym_friends.all(), [tony]) + self.assertEqual( + anne.symmetricalfriendship_set.get().date_friended, + date_friended_set, + ) + class M2mThroughToFieldsTests(TestCase): @classmethod