From e5bd585c6eb1e13e2f8aac030b33c077b0b70c05 Mon Sep 17 00:00:00 2001 From: Tim Graham Date: Thu, 31 Aug 2017 09:34:44 -0400 Subject: [PATCH] Fixed #28543 -- Prevented ManyToManyField.value_from_object() from being lazy. Previously, it was a QuerySet which could reevaluate to a new value if the model's data changes. This is inconsistent with other Field.value_from_object() methods. This allows reverting the fix in the admin for refs #27998. --- django/contrib/admin/options.py | 4 ---- django/db/models/fields/related.py | 4 +--- docs/releases/1.11.5.txt | 5 ++++ tests/model_fields/models.py | 4 ++++ tests/model_fields/test_manytomanyfield.py | 27 +++++++++++++--------- tests/model_forms/tests.py | 15 ++++++++++++ 6 files changed, 41 insertions(+), 18 deletions(-) diff --git a/django/contrib/admin/options.py b/django/contrib/admin/options.py index a88c867d85..808f4709ac 100644 --- a/django/contrib/admin/options.py +++ b/django/contrib/admin/options.py @@ -1468,10 +1468,6 @@ class ModelAdmin(BaseModelAdmin): new_object = form.instance formsets, inline_instances = self._create_formsets(request, new_object, change=not add) if all_valid(formsets) and form_validated: - if not add: - # Evalute querysets in form.initial so that changes to - # ManyToManyFields are reflected in this change's LogEntry. - form.has_changed() self.save_model(request, new_object, form, not add) self.save_related(request, form, formsets, not add) change_message = self.construct_change_message(request, form, formsets, add) diff --git a/django/db/models/fields/related.py b/django/db/models/fields/related.py index f6b87b7de8..0e0910277f 100644 --- a/django/db/models/fields/related.py +++ b/django/db/models/fields/related.py @@ -1591,9 +1591,7 @@ class ManyToManyField(RelatedField): pass def value_from_object(self, obj): - if obj.pk is None: - return self.related_model.objects.none() - return getattr(obj, self.attname).all() + return [] if obj.pk is None else list(getattr(obj, self.attname).all()) def save_form_data(self, instance, data): getattr(instance, self.attname).set(data) diff --git a/docs/releases/1.11.5.txt b/docs/releases/1.11.5.txt index 91620eb740..cb9e466248 100644 --- a/docs/releases/1.11.5.txt +++ b/docs/releases/1.11.5.txt @@ -35,3 +35,8 @@ Bugfixes * Fixed a regression in 1.11.4 where ``runserver`` crashed with non-Unicode system encodings on Python 2 + Windows (:ticket:`28487`). + +* Fixed a regression in Django 1.10 where changes to a ``ManyToManyField`` + weren't logged in the admin change history (:ticket:`27998`) and prevented + ``ManyToManyField`` initial data in model forms from being affected by + subsequent model changes (:ticket:`28543`). diff --git a/tests/model_fields/models.py b/tests/model_fields/models.py index 2208a8d4b8..1d18e78869 100644 --- a/tests/model_fields/models.py +++ b/tests/model_fields/models.py @@ -360,6 +360,10 @@ class AllFieldsModel(models.Model): gr = GenericRelation(DataModel) +class ManyToMany(models.Model): + m2m = models.ManyToManyField('self') + + ############################################################################### diff --git a/tests/model_fields/test_manytomanyfield.py b/tests/model_fields/test_manytomanyfield.py index b270048bd8..5724fe9384 100644 --- a/tests/model_fields/test_manytomanyfield.py +++ b/tests/model_fields/test_manytomanyfield.py @@ -1,21 +1,13 @@ from django.apps import apps from django.db import models -from django.test import SimpleTestCase +from django.test import SimpleTestCase, TestCase from django.test.utils import isolate_apps +from .models import ManyToMany + class ManyToManyFieldTests(SimpleTestCase): - @isolate_apps('model_fields') - def test_value_from_object_instance_without_pk(self): - class ManyToManyModel(models.Model): - m2m = models.ManyToManyField('self', models.CASCADE) - - instance = ManyToManyModel() - qs = instance._meta.get_field('m2m').value_from_object(instance) - self.assertEqual(qs.model, ManyToManyModel) - self.assertEqual(list(qs), []) - def test_abstract_model_pending_operations(self): """ Many-to-many fields declared on abstract models should not add lazy @@ -66,3 +58,16 @@ class ManyToManyFieldTests(SimpleTestCase): assert_app_model_resolved('model_fields') assert_app_model_resolved('tests') + + +class ManyToManyFieldDBTests(TestCase): + + def test_value_from_object_instance_without_pk(self): + obj = ManyToMany() + self.assertEqual(obj._meta.get_field('m2m').value_from_object(obj), []) + + def test_value_from_object_instance_with_pk(self): + obj = ManyToMany.objects.create() + related_obj = ManyToMany.objects.create() + obj.m2m.add(related_obj) + self.assertEqual(obj._meta.get_field('m2m').value_from_object(obj), [related_obj]) diff --git a/tests/model_forms/tests.py b/tests/model_forms/tests.py index 0cfc18659f..7af2aff825 100644 --- a/tests/model_forms/tests.py +++ b/tests/model_forms/tests.py @@ -3113,3 +3113,18 @@ class StrictAssignmentTests(TestCase): '__all__': ['Cannot set attribute'], 'title': ['This field cannot be blank.'] }) + + +class ModelToDictTests(TestCase): + def test_many_to_many(self): + """Data for a ManyToManyField is a list rather than a lazy QuerySet.""" + blue = Colour.objects.create(name='blue') + red = Colour.objects.create(name='red') + item = ColourfulItem.objects.create() + item.colours.set([blue]) + data = model_to_dict(item)['colours'] + self.assertEqual(data, [blue]) + item.colours.set([red]) + # If data were a QuerySet, it would be reevaluated here and give "red" + # instead of the original value. + self.assertEqual(data, [blue])