From d34043fd2510b2c59a6a62a9a71b5cc54266d509 Mon Sep 17 00:00:00 2001 From: Jack Cushman Date: Fri, 9 Feb 2018 14:39:14 -0500 Subject: [PATCH] Avoid fetching deferred fields in has_changed --- AUTHORS.rst | 1 + CHANGES.rst | 3 ++ docs/utilities.rst | 6 ++++ model_utils/tracker.py | 19 +++++++++++ tests/test_fields/test_field_tracker.py | 44 ++++++++++++++++++++++--- 5 files changed, 68 insertions(+), 5 deletions(-) diff --git a/AUTHORS.rst b/AUTHORS.rst index 4dd3044..a5841d5 100644 --- a/AUTHORS.rst +++ b/AUTHORS.rst @@ -42,3 +42,4 @@ Trey Hunner Karl Wan Nan Wo zyegfryed Radosław Jan Ganczarek +Jack Cushman diff --git a/CHANGES.rst b/CHANGES.rst index 922821d..a5a29e0 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -4,6 +4,9 @@ CHANGES master (unreleased) ------------------- +- Fix `FieldTracker.has_changed()` and `FieldTracker.previous()` to return + correct responses for deferred fields. + 3.1.1 (2017.12.17) ------------------ diff --git a/docs/utilities.rst b/docs/utilities.rst index 44824f5..b763ba0 100644 --- a/docs/utilities.rst +++ b/docs/utilities.rst @@ -150,6 +150,10 @@ Returns the value of the given field during the last save: Returns ``None`` when the model instance isn't saved yet. +If a field is `deferred`_, calling ``previous()`` will load the previous value from the database. + +.. _deferred: https://docs.djangoproject.com/en/2.0/ref/models/querysets/#defer + has_changed ~~~~~~~~~~~ @@ -167,6 +171,8 @@ Returns ``True`` if the given field has changed since the last save. The ``has_c The ``has_changed`` method relies on ``previous`` to determine whether a field's values has changed. +If a field is `deferred`_ and has been assigned locally, calling ``has_changed()`` +will load the previous value from the database to perform the comparison. changed ~~~~~~~ diff --git a/model_utils/tracker.py b/model_utils/tracker.py index 93e4a5d..3583a68 100644 --- a/model_utils/tracker.py +++ b/model_utils/tracker.py @@ -67,12 +67,31 @@ class FieldInstanceTracker(object): def has_changed(self, field): """Returns ``True`` if field has changed from currently saved value""" if field in self.fields: + # deferred fields haven't changed + if field in self.instance._deferred_fields and field not in self.instance.__dict__: + return False return self.previous(field) != self.get_field_value(field) else: raise FieldError('field "%s" not tracked' % field) def previous(self, field): """Returns currently saved value of given field""" + + # handle deferred fields that have not yet been loaded from the database + if self.instance.pk and field in self.instance._deferred_fields and field not in self.saved_data: + + # if the field has not been assigned locally, simply fetch and un-defer the value + if field not in self.instance.__dict__: + self.get_field_value(field) + + # if the field has been assigned locally, store the local value, fetch the database value, + # store database value to saved_data, and restore the local value + else: + current_value = self.get_field_value(field) + self.instance.refresh_from_db(fields=[field]) + self.saved_data[field] = deepcopy(self.get_field_value(field)) + setattr(self.instance, self.field_map[field], current_value) + return self.saved_data.get(field) def changed(self): diff --git a/tests/test_fields/test_field_tracker.py b/tests/test_fields/test_field_tracker.py index 00c28b3..5580773 100644 --- a/tests/test_fields/test_field_tracker.py +++ b/tests/test_fields/test_field_tracker.py @@ -180,20 +180,54 @@ class FieldTrackerTests(FieldTrackerTestCase, FieldTrackerCommonTests): self.instance.name = 'new age' self.instance.number = 1 self.instance.save() - item = list(self.tracked_class.objects.only('name').all())[0] + item = self.tracked_class.objects.only('name').first() self.assertTrue(item._deferred_fields) - self.assertEqual(item.tracker.previous('number'), None) - self.assertTrue('number' in item._deferred_fields) + # has_changed() returns False for deferred fields, without un-deferring them. + # Use an if because ModelTracked doesn't support has_changed() in this case. + if self.tracked_class == Tracked: + self.assertFalse(item.tracker.has_changed('number')) + self.assertTrue('number' in item._deferred_fields) + # previous() un-defers field and returns value + self.assertEqual(item.tracker.previous('number'), 1) + self.assertTrue('number' not in item._deferred_fields) + + # examining a deferred field un-defers it + item = self.tracked_class.objects.only('name').first() self.assertEqual(item.number, 1) self.assertTrue('number' not in item._deferred_fields) - self.assertEqual(item.tracker.previous('number'), 1) - self.assertFalse(item.tracker.has_changed('number')) + # has_changed() returns correct values after deferred field is examined + self.assertFalse(item.tracker.has_changed('number')) item.number = 2 self.assertTrue(item.tracker.has_changed('number')) + # previous() returns correct value after deferred field is examined + self.assertEqual(item.tracker.previous('number'), 1) + + # assigning to a deferred field un-defers it + # Use an if because ModelTracked doesn't handle this case. + if self.tracked_class == Tracked: + + item = self.tracked_class.objects.only('name').first() + item.number = 2 + + # _deferred_fields is not updated by assignment + self.assertTrue('number' in item._deferred_fields) + + # previous() fetches correct value from database after deferred field is assigned + self.assertEqual(item.tracker.previous('number'), 1) + + # database fetch of previous() value doesn't affect current value + self.assertEqual(item.number, 2) + + # has_changed() returns correct values after deferred field is assigned + self.assertTrue(item.tracker.has_changed('number')) + item.number = 1 + self.assertFalse(item.tracker.has_changed('number')) + + class FieldTrackerMultipleInstancesTests(TestCase):