From b9f954074c4988b4f6ffc02991a92e9286c469c6 Mon Sep 17 00:00:00 2001 From: Mikhail Silonov Date: Thu, 8 Aug 2013 13:18:33 +0400 Subject: [PATCH 1/3] Fixed a bug causing `KeyError` when saving with the parameter `update_fields` in which there are untracked fields. --- AUTHORS.rst | 1 + CHANGES.rst | 3 +++ model_utils/tests/tests.py | 12 +++++++++++- model_utils/tracker.py | 16 ++++++++++++++-- 4 files changed, 29 insertions(+), 3 deletions(-) diff --git a/AUTHORS.rst b/AUTHORS.rst index 3a33840..5ea4eba 100644 --- a/AUTHORS.rst +++ b/AUTHORS.rst @@ -19,3 +19,4 @@ Simon Meers sayane Trey Hunner zyegfryed +Mikhail Silonov diff --git a/CHANGES.rst b/CHANGES.rst index 9b3bb75..44d3959 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -7,6 +7,9 @@ master (unreleased) * `Choices` now `__contains__` its Python identifier values. Thanks Keryn Knight. (Merge of GH-69). +* Fixed a bug causing ``KeyError`` when saving with the parameter + ``update_fields`` in which there are untracked fields. + 1.4.0 (2013.06.03) ------------------ diff --git a/model_utils/tests/tests.py b/model_utils/tests/tests.py index ab60a62..b85b08d 100644 --- a/model_utils/tests/tests.py +++ b/model_utils/tests/tests.py @@ -923,6 +923,16 @@ class FieldTrackedModelCustomTests(FieldTrackerTestCase, self.instance.save() self.assertCurrent(name='new age') + def test_update_fields(self): + # Django 1.4 doesn't have update_fields + if django.VERSION >= (1, 5, 0): + self.update_instance(name='retro', number=4) + self.assertChanged() + self.instance.name = 'new age' + self.instance.number = 8 + self.instance.save(update_fields=['name', 'number']) + self.assertChanged() + class FieldTrackedModelAttributeTests(FieldTrackerTestCase): @@ -976,7 +986,7 @@ class FieldTrackedModelAttributeTests(FieldTrackerTestCase): class FieldTrackedModelMultiTests(FieldTrackerTestCase, - FieldTrackerCommonTests): + FieldTrackerCommonTests): tracked_class = TrackedMultiple diff --git a/model_utils/tracker.py b/model_utils/tracker.py index 31fd0f2..aecf27c 100644 --- a/model_utils/tracker.py +++ b/model_utils/tracker.py @@ -68,7 +68,8 @@ class FieldTracker(object): def finalize_class(self, sender, **kwargs): if self.fields is None: - self.fields = [field.attname for field in sender._meta.local_fields] + self.fields = (field.attname for field in sender._meta.local_fields) + self.fields = set(self.fields) self.field_map = self.get_field_map(sender) models.signals.post_init.connect(self.initialize_tracker, sender=sender) setattr(sender, self.name, self) @@ -83,8 +84,19 @@ class FieldTracker(object): original_save = instance.save def save(**kwargs): ret = original_save(**kwargs) + update_fields = kwargs.get('update_fields') + if not update_fields and update_fields is not None: # () or [] + fields = update_fields + elif update_fields is None: + fields = None + else: + fields = ( + field for field in update_fields if + field in self.fields + ) getattr(instance, self.attname).set_saved_fields( - fields=kwargs.get('update_fields')) + fields=fields + ) return ret instance.save = save From 9a3abce7f54ec0792524db4cd2162220288d9843 Mon Sep 17 00:00:00 2001 From: Carl Meyer Date: Thu, 8 Aug 2013 09:43:46 -0600 Subject: [PATCH 2/3] Update testing idioms. --- model_utils/tests/tests.py | 135 ++++++++++++++++++------------------- 1 file changed, 65 insertions(+), 70 deletions(-) diff --git a/model_utils/tests/tests.py b/model_utils/tests/tests.py index b85b08d..01827b8 100644 --- a/model_utils/tests/tests.py +++ b/model_utils/tests/tests.py @@ -106,15 +106,13 @@ class SplitFieldTests(TestCase): def test_assign_to_excerpt(self): - def _invalid_assignment(): + with self.assertRaises(AttributeError): self.post.body.excerpt = 'this should fail' - self.assertRaises(AttributeError, _invalid_assignment) def test_access_via_class(self): - def _invalid_access(): + with self.assertRaises(AttributeError): Article.body - self.assertRaises(AttributeError, _invalid_access) def test_none(self): @@ -169,7 +167,8 @@ class MonitorFieldTests(TestCase): def test_no_monitor_arg(self): - self.assertRaises(TypeError, MonitorField) + with self.assertRaises(TypeError): + MonitorField() class StatusFieldTests(TestCase): @@ -221,7 +220,8 @@ class ChoicesTests(TestCase): def test_wrong_length_tuple(self): - self.assertRaises(ValueError, Choices, ('a',)) + with self.assertRaises(ValueError): + Choices(('a',)) def test_contains_value(self): self.assertTrue('PUBLISHED' in self.STATUS) @@ -385,23 +385,23 @@ class InheritanceManagerTests(TestCase): ) + @skipUnless(django.VERSION >= (1, 6, 0), "test only applies to Django 1.6+") def test_select_specific_grandchildren(self): - if django.VERSION >= (1, 6, 0): - children = set([ - InheritanceManagerTestParent(pk=self.child1.pk), - InheritanceManagerTestParent(pk=self.child2.pk), - self.grandchild1, - InheritanceManagerTestParent(pk=self.grandchild1_2.pk), - ]) - self.assertEqual( - set( - self.get_manager().select_subclasses( - "inheritancemanagertestchild1__" - "inheritancemanagertestgrandchild1" - ) - ), - children, - ) + children = set([ + InheritanceManagerTestParent(pk=self.child1.pk), + InheritanceManagerTestParent(pk=self.child2.pk), + self.grandchild1, + InheritanceManagerTestParent(pk=self.grandchild1_2.pk), + ]) + self.assertEqual( + set( + self.get_manager().select_subclasses( + "inheritancemanagertestchild1__" + "inheritancemanagertestgrandchild1" + ) + ), + children, + ) def test_get_subclass(self): @@ -411,13 +411,11 @@ class InheritanceManagerTests(TestCase): def test_prior_select_related(self): - # Django 1.2 doesn't have assertNumQueries - if django.VERSION >= (1, 3): - with self.assertNumQueries(1): - obj = self.get_manager().select_related( - "inheritancemanagertestchild1").select_subclasses( - "inheritancemanagertestchild2").get(pk=self.child1.pk) - obj.inheritancemanagertestchild1 + with self.assertNumQueries(1): + obj = self.get_manager().select_related( + "inheritancemanagertestchild1").select_subclasses( + "inheritancemanagertestchild2").get(pk=self.child1.pk) + obj.inheritancemanagertestchild1 @@ -521,10 +519,9 @@ class TimeFrameManagerAddedTests(TestCase): def test_conflict_error(self): - def _run(): + with self.assertRaises(ImproperlyConfigured): class ErrorModel(TimeFramedModel): timeframed = models.BooleanField() - self.assertRaises(ImproperlyConfigured, _run) @@ -575,14 +572,13 @@ class StatusManagerAddedTests(TestCase): def test_conflict_error(self): - def _run(): + with self.assertRaises(ImproperlyConfigured): class ErrorModel(StatusModel): STATUS = ( ('active', 'active'), ('deleted', 'deleted'), ) active = models.BooleanField() - self.assertRaises(ImproperlyConfigured, _run) @@ -629,9 +625,8 @@ class SouthFreezingTests(TestCase): def test_no_excerpt_field_works(self): from .models import NoRendered - self.assertRaises(FieldDoesNotExist, - NoRendered._meta.get_field, - '_body_excerpt') + with self.assertRaises(FieldDoesNotExist): + NoRendered._meta.get_field('_body_excerpt') def test_status_field_no_check_for_status(self): sf = StatusFieldDefaultFilled._meta.get_field('status') @@ -658,9 +653,8 @@ class PassThroughManagerTests(TestCase): def test_manager_only_methods(self): stats = Dude.abiders.get_stats() self.assertEqual(stats['rug_count'], 1) - def notonqs(): + with self.assertRaises(AttributeError): Dude.abiders.all().get_stats() - self.assertRaises(AttributeError, notonqs) def test_queryset_pickling(self): @@ -716,7 +710,8 @@ class FieldTrackerTestCase(TestCase): tracker = kwargs.pop('tracker', self.tracker) for field, value in kwargs.items(): if value is None: - self.assertRaises(FieldError, tracker.has_changed, field) + with self.assertRaises(FieldError): + tracker.has_changed(field) else: self.assertEqual(tracker.has_changed(field), value) @@ -793,8 +788,8 @@ class FieldTrackerTests(FieldTrackerTestCase, FieldTrackerCommonTests): self.assertPrevious(name=None, number=None) self.assertCurrent(name='retro', number=4, id=None) self.assertChanged(name=None, number=None) - self.assertRaises(ValueError, self.instance.save, - update_fields=['number']) + with self.assertRaises(ValueError): + self.instance.save(update_fields=['number']) def test_post_save_has_changed(self): self.update_instance(name='retro', number=4) @@ -830,26 +825,26 @@ class FieldTrackerTests(FieldTrackerTestCase, FieldTrackerCommonTests): self.instance.save() self.assertCurrent(id=self.instance.id, name='new age', number=8) + @skipUnless( + django.VERSION >= (1, 5, 0), "Django 1.4 doesn't have update_fields") def test_update_fields(self): - # Django 1.4 doesn't have update_fields - if django.VERSION >= (1, 5, 0): - self.update_instance(name='retro', number=4) - self.assertChanged() - self.instance.name = 'new age' - self.instance.number = 8 - self.assertChanged(name='retro', number=4) - self.instance.save(update_fields=[]) - self.assertChanged(name='retro', number=4) - self.instance.save(update_fields=['name']) - in_db = self.tracked_class.objects.get(id=self.instance.id) - self.assertEqual(in_db.name, self.instance.name) - self.assertNotEqual(in_db.number, self.instance.number) - self.assertChanged(number=4) - self.instance.save(update_fields=['number']) - self.assertChanged() - in_db = self.tracked_class.objects.get(id=self.instance.id) - self.assertEqual(in_db.name, self.instance.name) - self.assertEqual(in_db.number, self.instance.number) + self.update_instance(name='retro', number=4) + self.assertChanged() + self.instance.name = 'new age' + self.instance.number = 8 + self.assertChanged(name='retro', number=4) + self.instance.save(update_fields=[]) + self.assertChanged(name='retro', number=4) + self.instance.save(update_fields=['name']) + in_db = self.tracked_class.objects.get(id=self.instance.id) + self.assertEqual(in_db.name, self.instance.name) + self.assertNotEqual(in_db.number, self.instance.number) + self.assertChanged(number=4) + self.instance.save(update_fields=['number']) + self.assertChanged() + in_db = self.tracked_class.objects.get(id=self.instance.id) + self.assertEqual(in_db.name, self.instance.name) + self.assertEqual(in_db.number, self.instance.number) class FieldTrackedModelCustomTests(FieldTrackerTestCase, @@ -923,15 +918,15 @@ class FieldTrackedModelCustomTests(FieldTrackerTestCase, self.instance.save() self.assertCurrent(name='new age') + @skipUnless( + django.VERSION >= (1, 5, 0), "Django 1.4 doesn't have update_fields") def test_update_fields(self): - # Django 1.4 doesn't have update_fields - if django.VERSION >= (1, 5, 0): - self.update_instance(name='retro', number=4) - self.assertChanged() - self.instance.name = 'new age' - self.instance.number = 8 - self.instance.save(update_fields=['name', 'number']) - self.assertChanged() + self.update_instance(name='retro', number=4) + self.assertChanged() + self.instance.name = 'new age' + self.instance.number = 8 + self.instance.save(update_fields=['name', 'number']) + self.assertChanged() class FieldTrackedModelAttributeTests(FieldTrackerTestCase): @@ -1150,8 +1145,8 @@ class ModelTrackerTests(FieldTrackerTests): self.assertPrevious(name=None, number=None) self.assertCurrent(name='retro', number=4, id=None) self.assertChanged() - self.assertRaises(ValueError, self.instance.save, - update_fields=['number']) + with self.assertRaises(ValueError): + self.instance.save(update_fields=['number']) def test_pre_save_has_changed(self): self.assertHasChanged(name=True, number=True) From 2eacb11b45af5a56a7e5054c17acd45f62a54f4e Mon Sep 17 00:00:00 2001 From: Carl Meyer Date: Thu, 8 Aug 2013 09:45:49 -0600 Subject: [PATCH 3/3] Tweak AUTHOR list and changelog. --- AUTHORS.rst | 4 ++-- CHANGES.rst | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/AUTHORS.rst b/AUTHORS.rst index 5ea4eba..f1a3ea3 100644 --- a/AUTHORS.rst +++ b/AUTHORS.rst @@ -6,12 +6,13 @@ Donald Stufft Facundo Gaich Felipe Prenholato Gregor Müllegger +ivirabyan James Oakley Jannis Leidel Javier García Sogo Jeff Elmore Keryn Knight -ivirabyan +Mikhail Silonov Paul McLanahan Rinat Shigapov Ryan Kaskel @@ -19,4 +20,3 @@ Simon Meers sayane Trey Hunner zyegfryed -Mikhail Silonov diff --git a/CHANGES.rst b/CHANGES.rst index 44d3959..b6759da 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -8,7 +8,8 @@ master (unreleased) Knight. (Merge of GH-69). * Fixed a bug causing ``KeyError`` when saving with the parameter - ``update_fields`` in which there are untracked fields. + ``update_fields`` in which there are untracked fields. Thanks Mikhail + Silonov. (Merge of GH-70, fixes GH-71). 1.4.0 (2013.06.03)