From fa1e2fc3194c8676e3ae5a2beeb7de25c6d8f219 Mon Sep 17 00:00:00 2001 From: Matt Westcott Date: Fri, 7 Apr 2017 02:18:11 +0100 Subject: [PATCH] Fix regression on plain TaggableManagers used in ClusterForm (#78) Only postpone instance.save() after save_m2m() if we really have to, as indicated by the presence of fields with the _need_commit_after_assignment flag set. --- modelcluster/contrib/taggit.py | 2 + modelcluster/fields.py | 1 + modelcluster/forms.py | 51 +++++++++++++++------ tests/migrations/0004_auto_20170406_1734.py | 41 +++++++++++++++++ tests/models.py | 19 ++++++++ tests/tests/test_tag.py | 40 +++++++++++++++- 6 files changed, 138 insertions(+), 16 deletions(-) create mode 100644 tests/migrations/0004_auto_20170406_1734.py diff --git a/modelcluster/contrib/taggit.py b/modelcluster/contrib/taggit.py index 83a1d13..4647cdd 100644 --- a/modelcluster/contrib/taggit.py +++ b/modelcluster/contrib/taggit.py @@ -80,6 +80,8 @@ def clear(self): class ClusterTaggableManager(TaggableManager): + _need_commit_after_assignment = True + def __get__(self, instance, model): # override TaggableManager's requirement for instance to have a primary key # before we can access its tags diff --git a/modelcluster/fields.py b/modelcluster/fields.py index e34752c..0b10aa5 100644 --- a/modelcluster/fields.py +++ b/modelcluster/fields.py @@ -470,6 +470,7 @@ def child_object_manager_cls(self): class ParentalManyToManyField(ManyToManyField): related_accessor_class = ParentalManyToManyDescriptor + _need_commit_after_assignment = True def contribute_to_class(self, cls, name, **kwargs): # ManyToManyField does not (as of Django 1.10) respect related_accessor_class, diff --git a/modelcluster/forms.py b/modelcluster/forms.py index 6c381a0..5d1ae21 100644 --- a/modelcluster/forms.py +++ b/modelcluster/forms.py @@ -274,22 +274,43 @@ def media(self): return media def save(self, commit=True): - # call ModelForm.save() with commit=False so that we can fix up the behaviour for - # M2M fields before saving - instance = super(ClusterForm, self).save(commit=False) - - # Always call save_m2m(), even for commit=False. The M2M field types allowed in a - # ClusterForm (ParentalManyToManyField and ClusterTaggableManager) are designed to - # update the local instance when written, rather than updating the database directly, - # and this happens on save_m2m. - # - # To actually save to the database, we need an explicit 'commit' step, which happens on - # instance.save(). We therefore ensure that save_m2m() happens first, so that the - # updated relation data exists on the local in-memory instance at the point of saving. - self.save_m2m() + # do we have any fields that expect us to call save_m2m immediately? + save_m2m_now = False + exclude = self._meta.exclude + fields = self._meta.fields + + for f in self.instance._meta.get_fields(): + if fields and f.name not in fields: + continue + if exclude and f.name in exclude: + continue + if getattr(f, '_need_commit_after_assignment', False): + save_m2m_now = True + break + + instance = super(ClusterForm, self).save(commit=(commit and not save_m2m_now)) + + # The M2M-like fields designed for use with ClusterForm (currently + # ParentalManyToManyField and ClusterTaggableManager) will manage their own in-memory + # relations, and not immediately write to the database when we assign to them. + # For these fields (identified by the _need_commit_after_assignment + # flag), save_m2m() is a safe operation that does not affect the database and is thus + # valid for commit=False. In the commit=True case, committing to the database happens + # in the subsequent instance.save (so this needs to happen after save_m2m to ensure + # we have the updated relation data in place). + + # For annoying legacy reasons we sometimes need to accommodate 'classic' M2M fields + # (particularly taggit.TaggableManager) within ClusterForm. These fields + # generally do require our instance to exist in the database at the point we call + # save_m2m() - for this reason, we only proceed with the customisation described above + # (i.e. postpone the instance.save() operation until after save_m2m) if there's a + # _need_commit_after_assignment field on the form that demands it. + + if save_m2m_now: + self.save_m2m() - if commit: - instance.save() + if commit: + instance.save() for formset in self.formsets.values(): formset.instance = instance diff --git a/tests/migrations/0004_auto_20170406_1734.py b/tests/migrations/0004_auto_20170406_1734.py new file mode 100644 index 0000000..e287b97 --- /dev/null +++ b/tests/migrations/0004_auto_20170406_1734.py @@ -0,0 +1,41 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.10.6 on 2017-04-06 22:34 +from __future__ import unicode_literals + +from django.db import migrations, models +import django.db.models.deletion +import taggit.managers + + +class Migration(migrations.Migration): + + dependencies = [ + ('taggit', '0002_auto_20150616_2121'), + ('tests', '0003_gallery_galleryimage'), + ] + + operations = [ + migrations.CreateModel( + name='NonClusterPlace', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('name', models.CharField(max_length=255)), + ], + ), + migrations.CreateModel( + name='TaggedNonClusterPlace', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('content_object', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='tagged_items', to='tests.NonClusterPlace')), + ('tag', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='tests_taggednonclusterplace_items', to='taggit.Tag')), + ], + options={ + 'abstract': False, + }, + ), + migrations.AddField( + model_name='nonclusterplace', + name='tags', + field=taggit.managers.TaggableManager(blank=True, help_text='A comma-separated list of tags.', through='tests.TaggedNonClusterPlace', to='taggit.Tag', verbose_name='Tags'), + ), + ] diff --git a/tests/models.py b/tests/models.py index d991953..6e67b7b 100644 --- a/tests/models.py +++ b/tests/models.py @@ -4,6 +4,7 @@ from django.utils.encoding import python_2_unicode_compatible from modelcluster.contrib.taggit import ClusterTaggableManager +from taggit.managers import TaggableManager from taggit.models import TaggedItemBase from modelcluster.fields import ParentalKey, ParentalManyToManyField @@ -61,6 +62,24 @@ class Restaurant(Place): proprietor = models.ForeignKey('Chef', null=True, blank=True, on_delete=models.SET_NULL, related_name='restaurants') +class TaggedNonClusterPlace(TaggedItemBase): + content_object = models.ForeignKey('NonClusterPlace', related_name='tagged_items') + + +@python_2_unicode_compatible +class NonClusterPlace(models.Model): + """ + For backwards compatibility we need ClusterModel to work with + plain TaggableManagers (as opposed to ClusterTaggableManager), albeit + without the in-memory relation behaviour + """ + name = models.CharField(max_length=255) + tags = TaggableManager(through=TaggedNonClusterPlace, blank=True) + + def __str__(self): + return self.name + + @python_2_unicode_compatible class Dish(models.Model): name = models.CharField(max_length=255) diff --git a/tests/tests/test_tag.py b/tests/tests/test_tag.py index 6c91c67..27553c6 100644 --- a/tests/tests/test_tag.py +++ b/tests/tests/test_tag.py @@ -6,7 +6,7 @@ from taggit.models import Tag from modelcluster.forms import ClusterForm -from tests.models import Place, TaggedPlace +from tests.models import NonClusterPlace, Place, TaggedPlace class TagTest(TestCase): @@ -94,6 +94,44 @@ class Meta: self.assertTrue(Tag.objects.get(name='fajita') in mission_burrito.tags.all()) self.assertFalse(Tag.objects.get(name='mexican') in mission_burrito.tags.all()) + def test_create_with_tags(self): + class PlaceForm(ClusterForm): + class Meta: + model = Place + exclude_formsets = ['tagged_items', 'reviews'] + fields = ['name', 'tags'] + + form = PlaceForm({ + 'name': "Mission Burrito", + 'tags': "burrito, fajita" + }, instance=Place()) + self.assertTrue(form.is_valid()) + mission_burrito = form.save() + reloaded_mission_burrito = Place.objects.get(pk=mission_burrito.pk) + self.assertEqual( + set(reloaded_mission_burrito.tags.all()), + set([Tag.objects.get(name='burrito'), Tag.objects.get(name='fajita')]) + ) + + def test_create_with_tags_with_plain_taggable_manager(self): + class PlaceForm(ClusterForm): + class Meta: + model = NonClusterPlace + exclude_formsets = ['tagged_items', 'reviews'] + fields = ['name', 'tags'] + + form = PlaceForm({ + 'name': "Mission Burrito", + 'tags': "burrito, fajita" + }, instance=NonClusterPlace()) + self.assertTrue(form.is_valid()) + mission_burrito = form.save() + reloaded_mission_burrito = NonClusterPlace.objects.get(pk=mission_burrito.pk) + self.assertEqual( + set(reloaded_mission_burrito.tags.all()), + set([Tag.objects.get(name='burrito'), Tag.objects.get(name='fajita')]) + ) + @override_settings(TAGGIT_CASE_INSENSITIVE=True) def test_case_insensitive_tags(self): mission_burrito = Place(name='Mission Burrito')