From d34043fd2510b2c59a6a62a9a71b5cc54266d509 Mon Sep 17 00:00:00 2001 From: Jack Cushman Date: Fri, 9 Feb 2018 14:39:14 -0500 Subject: [PATCH 01/16] 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): From 0fc0b44c95de3e39b71a1e01a3ef36a6bc887d12 Mon Sep 17 00:00:00 2001 From: Lucas Wiman Date: Mon, 2 Apr 2018 17:00:03 -0700 Subject: [PATCH 02/16] Remove version checks for django<1.8. Support for older versions of django was dropped in 3.0.0. --- model_utils/managers.py | 30 +++----------- model_utils/tracker.py | 40 ++++++------------- .../test_managers/test_inheritance_manager.py | 3 -- 3 files changed, 18 insertions(+), 55 deletions(-) diff --git a/model_utils/managers.py b/model_utils/managers.py index 2f2d1d2..98a2a54 100644 --- a/model_utils/managers.py +++ b/model_utils/managers.py @@ -48,11 +48,10 @@ class InheritanceIterable(ModelIterable): class InheritanceQuerySetMixin(object): def __init__(self, *args, **kwargs): super(InheritanceQuerySetMixin, self).__init__(*args, **kwargs) - if django.VERSION > (1, 8): - self._iterable_class = InheritanceIterable + self._iterable_class = InheritanceIterable def select_subclasses(self, *subclasses): - levels = self._get_maximum_depth() + levels = None calculated_subclasses = self._get_subclasses_recurse( self.model, levels=levels) # if none were passed in, we can just short circuit and select all @@ -151,12 +150,9 @@ class InheritanceQuerySetMixin(object): recursively, returning a `list` of strings representing the relations for select_related """ - if django.VERSION < (1, 8): - related_objects = model._meta.get_all_related_objects() - else: - related_objects = [ - f for f in model._meta.get_fields() - if isinstance(f, OneToOneRel)] + related_objects = [ + f for f in model._meta.get_fields() + if isinstance(f, OneToOneRel)] rels = [ rel for rel in related_objects @@ -199,10 +195,7 @@ class InheritanceQuerySetMixin(object): related = parent_link.remote_field ancestry.insert(0, related.get_accessor_name()) if levels or levels is None: - if django.VERSION < (1, 8): - parent_model = related.parent_model - else: - parent_model = related.model + parent_model = related.model parent_link = parent_model._meta.get_ancestor_link( self.model) else: @@ -230,17 +223,6 @@ class InheritanceQuerySetMixin(object): def get_subclass(self, *args, **kwargs): return self.select_subclasses().get(*args, **kwargs) - def _get_maximum_depth(self): - """ - Under Django versions < 1.6, to avoid triggering - https://code.djangoproject.com/ticket/16572 we can only look - as far as children. - """ - levels = None - if django.VERSION < (1, 6, 0): - levels = 1 - return levels - class InheritanceQuerySet(InheritanceQuerySetMixin, QuerySet): pass diff --git a/model_utils/tracker.py b/model_utils/tracker.py index 93e4a5d..0fec85d 100644 --- a/model_utils/tracker.py +++ b/model_utils/tracker.py @@ -97,35 +97,19 @@ class FieldInstanceTracker(object): def _get_field_name(self): return self.field.name - if django.VERSION >= (1, 8): - self.instance._deferred_fields = self.instance.get_deferred_fields() - for field in self.instance._deferred_fields: - if django.VERSION >= (1, 10): - field_obj = getattr(self.instance.__class__, field) - else: - field_obj = self.instance.__class__.__dict__.get(field) - if isinstance(field_obj, FileDescriptor): - field_tracker = FileDescriptorTracker(field_obj.field) - setattr(self.instance.__class__, field, field_tracker) - else: - field_tracker = DeferredAttributeTracker( - field_obj.field_name, None) - setattr(self.instance.__class__, field, field_tracker) - else: - for field in self.fields: + self.instance._deferred_fields = self.instance.get_deferred_fields() + for field in self.instance._deferred_fields: + if django.VERSION >= (1, 10): + field_obj = getattr(self.instance.__class__, field) + else: field_obj = self.instance.__class__.__dict__.get(field) - if isinstance(field_obj, DeferredAttribute): - self.instance._deferred_fields.add(field) - - # Django 1.4 - if django.VERSION >= (1, 5): - model = None - else: - model = field_obj.model_ref() - - field_tracker = DeferredAttributeTracker( - field_obj.field_name, model) - setattr(self.instance.__class__, field, field_tracker) + if isinstance(field_obj, FileDescriptor): + field_tracker = FileDescriptorTracker(field_obj.field) + setattr(self.instance.__class__, field, field_tracker) + else: + field_tracker = DeferredAttributeTracker( + field_obj.field_name, None) + setattr(self.instance.__class__, field, field_tracker) class FieldTracker(object): diff --git a/tests/test_managers/test_inheritance_manager.py b/tests/test_managers/test_inheritance_manager.py index 4509175..39c694e 100644 --- a/tests/test_managers/test_inheritance_manager.py +++ b/tests/test_managers/test_inheritance_manager.py @@ -115,9 +115,6 @@ class InheritanceManagerTests(TestCase): "inheritancemanagertestchild2").get(pk=self.child1.pk) obj.inheritancemanagertestchild1 - def test_version_determining_any_depth(self): - self.assertIsNone(self.get_manager().all()._get_maximum_depth()) - def test_manually_specifying_parent_fk_including_grandchildren(self): """ given a Model which inherits from another Model, but also declares From be52bc929009b58535de75dfdb40e6aa58bb4836 Mon Sep 17 00:00:00 2001 From: Lucas Wiman Date: Tue, 3 Apr 2018 13:18:03 -0700 Subject: [PATCH 03/16] Add failing test for deferred attributes. --- model_utils/tracker.py | 3 +- tests/models.py | 35 +++++++++++++++ tests/test_models/test_deferred_fields.py | 53 +++++++++++++++++++++++ 3 files changed, 89 insertions(+), 2 deletions(-) create mode 100644 tests/test_models/test_deferred_fields.py diff --git a/model_utils/tracker.py b/model_utils/tracker.py index 0fec85d..5da9dd4 100644 --- a/model_utils/tracker.py +++ b/model_utils/tracker.py @@ -107,8 +107,7 @@ class FieldInstanceTracker(object): field_tracker = FileDescriptorTracker(field_obj.field) setattr(self.instance.__class__, field, field_tracker) else: - field_tracker = DeferredAttributeTracker( - field_obj.field_name, None) + field_tracker = DeferredAttributeTracker(field, type(self.instance)) setattr(self.instance.__class__, field, field_tracker) diff --git a/tests/models.py b/tests/models.py index a65d499..841f612 100644 --- a/tests/models.py +++ b/tests/models.py @@ -1,6 +1,7 @@ from __future__ import unicode_literals, absolute_import from django.db import models +from django.db.models.query_utils import DeferredAttribute from django.db.models import Manager from django.utils.encoding import python_2_unicode_compatible from django.utils.translation import ugettext_lazy as _ @@ -331,3 +332,37 @@ class CustomSoftDelete(SoftDeletableModel): is_read = models.BooleanField(default=False) objects = CustomSoftDeleteManager() + + +class StringyDescriptor(object): + """ + Descriptor that returns a string version of the underlying integer value. + """ + def __init__(self, name): + self.name = name + + def __get__(self, obj, cls=None): + if obj is None: + return self + if self.name in obj.get_deferred_fields(): + # This queries the database, and sets the value on the instance. + DeferredAttribute(field_name=self.name, model=cls).__get__(obj, cls) + return str(obj.__dict__[self.name]) + + def __set__(self, obj, value): + obj.__dict__[self.name] = int(value) + + +class CustomDescriptorField(models.IntegerField): + def contribute_to_class(self, cls, name, **kwargs): + super(CustomDescriptorField, self).contribute_to_class(cls, name, **kwargs) + setattr(cls, name, StringyDescriptor(name)) + + +class ModelWithCustomDescriptor(models.Model): + custom_field = CustomDescriptorField() + tracked_custom_field = CustomDescriptorField() + regular_field = models.IntegerField() + tracked_regular_field = models.IntegerField() + + tracker = FieldTracker(fields=['tracked_custom_field', 'tracked_regular_field']) diff --git a/tests/test_models/test_deferred_fields.py b/tests/test_models/test_deferred_fields.py new file mode 100644 index 0000000..6a159be --- /dev/null +++ b/tests/test_models/test_deferred_fields.py @@ -0,0 +1,53 @@ +from __future__ import unicode_literals + +from django.test import TestCase + +from tests.models import ModelWithCustomDescriptor + + +class CustomDescriptorTests(TestCase): + def setUp(self): + self.instance = ModelWithCustomDescriptor.objects.create( + custom_field='1', + tracked_custom_field='1', + regular_field=1, + tracked_regular_field=1, + ) + + def test_custom_descriptor_works(self): + instance = self.instance + self.assertEqual(instance.custom_field, '1') + self.assertEqual(instance.__dict__['custom_field'], 1) + self.assertEqual(instance.regular_field, 1) + instance.custom_field = 2 + self.assertEqual(instance.custom_field, '2') + self.assertEqual(instance.__dict__['custom_field'], 2) + instance.save() + intance = ModelWithCustomDescriptor.objects.get(pk=instance.pk) + self.assertEqual(instance.custom_field, '2') + self.assertEqual(instance.__dict__['custom_field'], 2) + + def test_deferred(self): + instance = ModelWithCustomDescriptor.objects.only('id').get( + pk=self.instance.pk) + self.assertIn('custom_field', instance.get_deferred_fields()) + self.assertEqual(instance.custom_field, '1') + self.assertNotIn('custom_field', instance.get_deferred_fields()) + self.assertEqual(instance.regular_field, 1) + self.assertEqual(instance.tracked_custom_field, '1') + self.assertEqual(instance.tracked_regular_field, 1) + + self.assertFalse(instance.tracker.has_changed('tracked_custom_field')) + self.assertFalse(instance.tracker.has_changed('tracked_regular_field')) + + instance.tracked_custom_field = 2 + instance.tracked_regular_field = 2 + self.assertTrue(instance.tracker.has_changed('tracked_custom_field')) + self.assertTrue(instance.tracker.has_changed('tracked_regular_field')) + instance.save() + + instance = ModelWithCustomDescriptor.objects.get(pk=instance.pk) + self.assertEqual(instance.custom_field, '1') + self.assertEqual(instance.regular_field, 1) + self.assertEqual(instance.tracked_custom_field, '2') + self.assertEqual(instance.tracked_regular_field, 2) From 80b099f12918d28c297005fbbb15ed1ebaf10a12 Mon Sep 17 00:00:00 2001 From: Lucas Wiman Date: Tue, 3 Apr 2018 15:43:29 -0700 Subject: [PATCH 04/16] Do not override custom descriptors when present. This commit adds a collection of wrapper classes for tracking fields while still using custom descriptors that may be present. This fixes a bug where deferring a model field with a custom descriptor meant that the descriptor was overridden in all subsequent queries. --- model_utils/tracker.py | 77 ++++++++++++++++++++++- tests/test_fields/test_field_tracker.py | 15 ++++- tests/test_models/test_deferred_fields.py | 11 +++- 3 files changed, 95 insertions(+), 8 deletions(-) diff --git a/model_utils/tracker.py b/model_utils/tracker.py index 5da9dd4..095d87e 100644 --- a/model_utils/tracker.py +++ b/model_utils/tracker.py @@ -29,12 +29,73 @@ class DescriptorMixin(object): return self.field_name +class DescriptorWrapper(object): + + def __init__(self, field_name, descriptor, tracker_attname): + self.field_name = field_name + self.descriptor = descriptor + self.tracker_attname = tracker_attname + + def __get__(self, instance, owner): + if instance is None: + return self + was_deferred = self.field_name in instance.get_deferred_fields() + if self.descriptor: + value = self.descriptor.__get__(instance, owner) + else: + value = instance.__dict__[self.field_name] + if was_deferred: + tracker_instance = getattr(instance, self.tracker_attname) + tracker_instance.saved_data[self.field_name] = deepcopy(value) + return value + + @staticmethod + def cls_for_descriptor(descriptor): + has_set = hasattr(descriptor, '__set__') + has_del = hasattr(descriptor, '__delete__') + if has_set and has_del: + return FullDescriptorWrapper + elif has_set: + return SettableDescriptorWrapper + elif has_del: + return DeleteableDescriptorWrapper + else: + return DescriptorWrapper + + +class SettableDescriptorWrapper(DescriptorWrapper): + """ + Descriptor wrapper for descriptors with a __delete__ method. + + This should not be used for descriptors + """ + def __set__(self, instance, value): + return self.descriptor.__set__(instance, value) + + +class DeleteableDescriptorWrapper(DescriptorWrapper): + """ + Descriptor wrapper for descriptors with a __delete__ method. + + This should not be used for descriptors + """ + def __delete__(self, instance): + self.descriptor.__delete__(instance) + + +class FullDescriptorWrapper(SettableDescriptorWrapper, DeleteableDescriptorWrapper): + """ + Wrapper for descriptors with all three descriptor methods. + """ + + class FieldInstanceTracker(object): def __init__(self, instance, fields, field_map): self.instance = instance self.fields = fields self.field_map = field_map - self.init_deferred_fields() + if django.VERSION < (1, 10): + self.init_deferred_fields() def get_field_value(self, field): return getattr(self.instance, self.field_map[field]) @@ -54,10 +115,11 @@ class FieldInstanceTracker(object): def current(self, fields=None): """Returns dict of current values for all tracked fields""" if fields is None: - if self.instance._deferred_fields: + deferred_fields = self.instance._deferred_fields if django.VERSION < (1, 10) else self.instance.get_deferred_fields() + if deferred_fields: fields = [ field for field in self.fields - if field not in self.instance._deferred_fields + if field not in deferred_fields ] else: fields = self.fields @@ -135,6 +197,15 @@ class FieldTracker(object): if self.fields is None: self.fields = (field.attname for field in sender._meta.fields) self.fields = set(self.fields) + if django.VERSION >= (1, 10): + for field_name in self.fields: + if django.VERSION >= (1, 10): + descriptor = getattr(sender, field_name) + else: + descriptor = sender.__dict__.get(field_name) + wrapper_cls = DescriptorWrapper.cls_for_descriptor(descriptor) + wrapped_descriptor = wrapper_cls(field_name, descriptor, self.attname) + setattr(sender, field_name, wrapped_descriptor) self.field_map = self.get_field_map(sender) models.signals.post_init.connect(self.initialize_tracker) self.model_class = sender diff --git a/tests/test_fields/test_field_tracker.py b/tests/test_fields/test_field_tracker.py index 00c28b3..b389861 100644 --- a/tests/test_fields/test_field_tracker.py +++ b/tests/test_fields/test_field_tracker.py @@ -181,13 +181,22 @@ class FieldTrackerTests(FieldTrackerTestCase, FieldTrackerCommonTests): self.instance.number = 1 self.instance.save() item = list(self.tracked_class.objects.only('name').all())[0] - self.assertTrue(item._deferred_fields) + if django.VERSION >= (1, 10): + self.assertTrue(item.get_deferred_fields()) + else: + self.assertTrue(item._deferred_fields) self.assertEqual(item.tracker.previous('number'), None) - self.assertTrue('number' in item._deferred_fields) + if django.VERSION >= (1, 10): + self.assertTrue('number' in item.get_deferred_fields()) + else: + self.assertTrue('number' in item._deferred_fields) self.assertEqual(item.number, 1) - self.assertTrue('number' not in item._deferred_fields) + if django.VERSION >= (1, 10): + self.assertTrue('number' not in item.get_deferred_fields()) + else: + self.assertTrue('number' not in item._deferred_fields) self.assertEqual(item.tracker.previous('number'), 1) self.assertFalse(item.tracker.has_changed('number')) diff --git a/tests/test_models/test_deferred_fields.py b/tests/test_models/test_deferred_fields.py index 6a159be..b235843 100644 --- a/tests/test_models/test_deferred_fields.py +++ b/tests/test_models/test_deferred_fields.py @@ -1,5 +1,6 @@ from __future__ import unicode_literals +import django from django.test import TestCase from tests.models import ModelWithCustomDescriptor @@ -30,9 +31,15 @@ class CustomDescriptorTests(TestCase): def test_deferred(self): instance = ModelWithCustomDescriptor.objects.only('id').get( pk=self.instance.pk) - self.assertIn('custom_field', instance.get_deferred_fields()) + if django.VERSION >= (1, 10): + self.assertIn('custom_field', instance.get_deferred_fields()) + else: + self.assertIn('custom_field', instance._deferred_fields) self.assertEqual(instance.custom_field, '1') - self.assertNotIn('custom_field', instance.get_deferred_fields()) + if django.VERSION >= (1, 10): + self.assertNotIn('custom_field', instance.get_deferred_fields()) + else: + self.assertNotIn('custom_field', instance._deferred_fields) self.assertEqual(instance.regular_field, 1) self.assertEqual(instance.tracked_custom_field, '1') self.assertEqual(instance.tracked_regular_field, 1) From be1a7d92811f9e57fd43b14b735218612f30f6ff Mon Sep 17 00:00:00 2001 From: Lucas Wiman Date: Tue, 3 Apr 2018 16:10:30 -0700 Subject: [PATCH 05/16] Update AUTHORS and CHANGES. As far as I can tell, no changes to documentation is required. --- AUTHORS.rst | 1 + CHANGES.rst | 1 + 2 files changed, 2 insertions(+) diff --git a/AUTHORS.rst b/AUTHORS.rst index fd350c0..33224c5 100644 --- a/AUTHORS.rst +++ b/AUTHORS.rst @@ -43,3 +43,4 @@ Trey Hunner Karl Wan Nan Wo zyegfryed Radosław Jan Ganczarek +Lucas Wiman diff --git a/CHANGES.rst b/CHANGES.rst index 84ea8d7..9bd6129 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -3,6 +3,7 @@ CHANGES master (unreleased) ------------------- +- Fix handling of deferred attributes on Django 1.10+, fixes GH-278 3.1.2 (2018.05.09) ------------------ From 90ed7fc9052f46ea1007af26ff9953f3f1e198b7 Mon Sep 17 00:00:00 2001 From: Lucas Wiman Date: Wed, 4 Apr 2018 10:02:46 -0700 Subject: [PATCH 06/16] Improve coverage. --- model_utils/tracker.py | 33 +++++------------------ tests/models.py | 3 +++ tests/test_models/test_deferred_fields.py | 11 ++++++++ 3 files changed, 21 insertions(+), 26 deletions(-) diff --git a/model_utils/tracker.py b/model_utils/tracker.py index 095d87e..0ec3044 100644 --- a/model_utils/tracker.py +++ b/model_utils/tracker.py @@ -40,10 +40,7 @@ class DescriptorWrapper(object): if instance is None: return self was_deferred = self.field_name in instance.get_deferred_fields() - if self.descriptor: - value = self.descriptor.__get__(instance, owner) - else: - value = instance.__dict__[self.field_name] + value = self.descriptor.__get__(instance, owner) if was_deferred: tracker_instance = getattr(instance, self.tracker_attname) tracker_instance.saved_data[self.field_name] = deepcopy(value) @@ -53,12 +50,10 @@ class DescriptorWrapper(object): def cls_for_descriptor(descriptor): has_set = hasattr(descriptor, '__set__') has_del = hasattr(descriptor, '__delete__') - if has_set and has_del: + if has_del: return FullDescriptorWrapper elif has_set: return SettableDescriptorWrapper - elif has_del: - return DeleteableDescriptorWrapper else: return DescriptorWrapper @@ -73,20 +68,12 @@ class SettableDescriptorWrapper(DescriptorWrapper): return self.descriptor.__set__(instance, value) -class DeleteableDescriptorWrapper(DescriptorWrapper): - """ - Descriptor wrapper for descriptors with a __delete__ method. - - This should not be used for descriptors - """ - def __delete__(self, instance): - self.descriptor.__delete__(instance) - - -class FullDescriptorWrapper(SettableDescriptorWrapper, DeleteableDescriptorWrapper): +class FullDescriptorWrapper(SettableDescriptorWrapper): """ Wrapper for descriptors with all three descriptor methods. """ + def __delete__(self, obj): + self.descriptor.__delete__(obj) class FieldInstanceTracker(object): @@ -161,10 +148,7 @@ class FieldInstanceTracker(object): self.instance._deferred_fields = self.instance.get_deferred_fields() for field in self.instance._deferred_fields: - if django.VERSION >= (1, 10): - field_obj = getattr(self.instance.__class__, field) - else: - field_obj = self.instance.__class__.__dict__.get(field) + field_obj = self.instance.__class__.__dict__.get(field) if isinstance(field_obj, FileDescriptor): field_tracker = FileDescriptorTracker(field_obj.field) setattr(self.instance.__class__, field, field_tracker) @@ -199,10 +183,7 @@ class FieldTracker(object): self.fields = set(self.fields) if django.VERSION >= (1, 10): for field_name in self.fields: - if django.VERSION >= (1, 10): - descriptor = getattr(sender, field_name) - else: - descriptor = sender.__dict__.get(field_name) + descriptor = getattr(sender, field_name) wrapper_cls = DescriptorWrapper.cls_for_descriptor(descriptor) wrapped_descriptor = wrapper_cls(field_name, descriptor, self.attname) setattr(sender, field_name, wrapped_descriptor) diff --git a/tests/models.py b/tests/models.py index 841f612..91df0b6 100644 --- a/tests/models.py +++ b/tests/models.py @@ -352,6 +352,9 @@ class StringyDescriptor(object): def __set__(self, obj, value): obj.__dict__[self.name] = int(value) + def __delete__(self, obj): + del obj.__dict__[self.name] + class CustomDescriptorField(models.IntegerField): def contribute_to_class(self, cls, name, **kwargs): diff --git a/tests/test_models/test_deferred_fields.py b/tests/test_models/test_deferred_fields.py index b235843..05ba336 100644 --- a/tests/test_models/test_deferred_fields.py +++ b/tests/test_models/test_deferred_fields.py @@ -58,3 +58,14 @@ class CustomDescriptorTests(TestCase): self.assertEqual(instance.regular_field, 1) self.assertEqual(instance.tracked_custom_field, '2') self.assertEqual(instance.tracked_regular_field, 2) + + instance = ModelWithCustomDescriptor.objects.only('id').get(pk=instance.pk) + if django.VERSION >= (1, 10): + # This fails on 1.8 and 1.9, which is a bug in the deferred field + # implementation on those versions. + instance.tracked_custom_field = 3 + self.assertEqual(instance.tracked_custom_field, '3') + self.assertTrue(instance.tracker.has_changed('tracked_custom_field')) + del instance.tracked_custom_field + self.assertEqual(instance.tracked_custom_field, '2') + self.assertFalse(instance.tracker.has_changed('tracked_custom_field')) From 5d410e9ccceb092947ae9c0777e4b42b93b8376a Mon Sep 17 00:00:00 2001 From: Lucas Wiman Date: Thu, 21 Jun 2018 13:07:13 -0700 Subject: [PATCH 07/16] Fix test failures from merge. --- model_utils/tracker.py | 10 +++++++--- tests/test_fields/test_field_tracker.py | 9 ++++----- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/model_utils/tracker.py b/model_utils/tracker.py index d1101e2..e69d177 100644 --- a/model_utils/tracker.py +++ b/model_utils/tracker.py @@ -84,6 +84,10 @@ class FieldInstanceTracker(object): if django.VERSION < (1, 10): self.init_deferred_fields() + @property + def deferred_fields(self): + return self.instance._deferred_fields if django.VERSION < (1, 10) else self.instance.get_deferred_fields() + def get_field_value(self, field): return getattr(self.instance, self.field_map[field]) @@ -102,7 +106,7 @@ class FieldInstanceTracker(object): def current(self, fields=None): """Returns dict of current values for all tracked fields""" if fields is None: - deferred_fields = self.instance._deferred_fields if django.VERSION < (1, 10) else self.instance.get_deferred_fields() + deferred_fields = self.deferred_fields if deferred_fields: fields = [ field for field in self.fields @@ -117,7 +121,7 @@ class FieldInstanceTracker(object): """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__: + if field in self.deferred_fields and field not in self.instance.__dict__: return False return self.previous(field) != self.get_field_value(field) else: @@ -127,7 +131,7 @@ class FieldInstanceTracker(object): """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 self.instance.pk and field in self.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__: diff --git a/tests/test_fields/test_field_tracker.py b/tests/test_fields/test_field_tracker.py index bc0da4c..dc29e70 100644 --- a/tests/test_fields/test_field_tracker.py +++ b/tests/test_fields/test_field_tracker.py @@ -189,7 +189,6 @@ class FieldTrackerTests(FieldTrackerTestCase, FieldTrackerCommonTests): # 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.assertEqual(item.tracker.previous('number'), None) if django.VERSION >= (1, 10): self.assertTrue('number' in item.get_deferred_fields()) else: @@ -197,7 +196,10 @@ class FieldTrackerTests(FieldTrackerTestCase, FieldTrackerCommonTests): # previous() un-defers field and returns value self.assertEqual(item.tracker.previous('number'), 1) - self.assertTrue('number' not in item._deferred_fields) + if django.VERSION >= (1, 10): + self.assertNotIn('number', item.get_deferred_fields()) + else: + self.assertNotIn('number', item._deferred_fields) # examining a deferred field un-defers it item = self.tracked_class.objects.only('name').first() @@ -224,9 +226,6 @@ class FieldTrackerTests(FieldTrackerTestCase, FieldTrackerCommonTests): 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) From a84c3afdddfe75520ac2caf92d99bda580c793bc Mon Sep 17 00:00:00 2001 From: Lucas Wiman Date: Thu, 28 Jun 2018 13:09:42 -0700 Subject: [PATCH 08/16] Fix behavior of .previous() in Django 1.10+. The complications are that when the attribute is set in Django 1.10, it no longer counts as a deferred attribute, and it is not retrieved from the database. Naively updating __set__ to retrieve the value if it is deferred leads to infinite recursion because accessing the attribute involves loading data from the database and trying to set the attribute based on that value. This commit introduces a somewhat hacky flag that records whether we're already trying to set the attribute further up in the call stack. --- model_utils/tracker.py | 40 ++++++++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/model_utils/tracker.py b/model_utils/tracker.py index e69d177..a145b1a 100644 --- a/model_utils/tracker.py +++ b/model_utils/tracker.py @@ -46,29 +46,40 @@ class DescriptorWrapper(object): tracker_instance.saved_data[self.field_name] = deepcopy(value) return value + def __set__(self, instance, value): + initialized = hasattr(instance, '_instance_intialized') + was_deferred = self.field_name in instance.get_deferred_fields() + + # Sentinel attribute to detect whether we are already trying to + # set the attribute higher up the stack. This prevents infinite + # recursion when retrieving deferred values from the database. + recursion_sentinel_attname = '_setting_' + self.field_name + already_setting = hasattr(instance, recursion_sentinel_attname) + + if initialized and was_deferred and not already_setting: + setattr(instance, recursion_sentinel_attname, True) + try: + # Retrieve the value to set the saved_data value. + # This will undefer the field + getattr(instance, self.field_name) + finally: + if already_setting: + instance.__dict__.pop(recursion_sentinel_attname, None) + if hasattr(self.descriptor, '__set__'): + self.descriptor.__set__(instance, value) + else: + instance.__dict__[self.field_name] = value + @staticmethod def cls_for_descriptor(descriptor): - has_set = hasattr(descriptor, '__set__') has_del = hasattr(descriptor, '__delete__') if has_del: return FullDescriptorWrapper - elif has_set: - return SettableDescriptorWrapper else: return DescriptorWrapper -class SettableDescriptorWrapper(DescriptorWrapper): - """ - Descriptor wrapper for descriptors with a __delete__ method. - - This should not be used for descriptors - """ - def __set__(self, instance, value): - return self.descriptor.__set__(instance, value) - - -class FullDescriptorWrapper(SettableDescriptorWrapper): +class FullDescriptorWrapper(DescriptorWrapper): """ Wrapper for descriptors with all three descriptor methods. """ @@ -222,6 +233,7 @@ class FieldTracker(object): setattr(instance, self.attname, tracker) tracker.set_saved_fields() self.patch_save(instance) + instance._instance_intialized = True def patch_save(self, instance): original_save = instance.save From 15f9393bb21e2ea349113868d38eeb180dee97cb Mon Sep 17 00:00:00 2001 From: Lucas Wiman Date: Thu, 28 Jun 2018 13:16:33 -0700 Subject: [PATCH 09/16] Handle API change in DeferredAttribute descriptor in django-trunk. This should maintain compatibility with the next version of django. --- model_utils/tracker.py | 5 ++++- tests/models.py | 6 +++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/model_utils/tracker.py b/model_utils/tracker.py index a145b1a..a0bc0d0 100644 --- a/model_utils/tracker.py +++ b/model_utils/tracker.py @@ -187,7 +187,10 @@ class FieldInstanceTracker(object): field_tracker = FileDescriptorTracker(field_obj.field) setattr(self.instance.__class__, field, field_tracker) else: - field_tracker = DeferredAttributeTracker(field, type(self.instance)) + if django.VERSION < (2, 1): + field_tracker = DeferredAttributeTracker(field, type(self.instance)) + else: + field_tracker = DeferredAttributeTracker(field) setattr(self.instance.__class__, field, field_tracker) diff --git a/tests/models.py b/tests/models.py index 91df0b6..7c9bb56 100644 --- a/tests/models.py +++ b/tests/models.py @@ -1,5 +1,6 @@ from __future__ import unicode_literals, absolute_import +import django from django.db import models from django.db.models.query_utils import DeferredAttribute from django.db.models import Manager @@ -346,7 +347,10 @@ class StringyDescriptor(object): return self if self.name in obj.get_deferred_fields(): # This queries the database, and sets the value on the instance. - DeferredAttribute(field_name=self.name, model=cls).__get__(obj, cls) + if django.VERSION < (2, 1): + DeferredAttribute(field_name=self.name, model=cls).__get__(obj, cls) + else: + DeferredAttribute(field_name=self.name).__get__(obj, cls) return str(obj.__dict__[self.name]) def __set__(self, obj, value): From 4740ab43ec161cbe5b80da5bc6ee59c644161b16 Mon Sep 17 00:00:00 2001 From: Lucas Wiman Date: Thu, 28 Jun 2018 13:41:09 -0700 Subject: [PATCH 10/16] Update passed environment variables to match codecov documentation. Hopefully this will combine the coverage reports. --- tox.ini | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tox.ini b/tox.ini index fe42bb1..861074c 100644 --- a/tox.ini +++ b/tox.ini @@ -18,6 +18,10 @@ deps = pytest-cov ignore_outcome = djangotrunk: True +passenv = + CI + TRAVIS + TRAVIS_* commands = pip install -e . From c16a275bd7c04d42ac35ede7fb7be005a479d7dd Mon Sep 17 00:00:00 2001 From: Lucas Wiman Date: Thu, 28 Jun 2018 13:46:39 -0700 Subject: [PATCH 11/16] Use --cov-append option in travis build to include coverage data from all tox environments run on the travis environment. Note that it is run as one per python version, but multiple versions of django are tested in each. --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 83a070b..295d123 100644 --- a/.travis.yml +++ b/.travis.yml @@ -8,7 +8,7 @@ python: - 3.6 install: pip install tox-travis codecov # positional args ({posargs}) to pass into tox.ini -script: tox -- --cov +script: tox -- --cov --cov-append after_success: codecov deploy: provider: pypi From 59347ef36fc032717ffa462bb56edd17b2a09d45 Mon Sep 17 00:00:00 2001 From: Lucas Wiman Date: Thu, 28 Jun 2018 13:52:52 -0700 Subject: [PATCH 12/16] Correctly clean up recursion sentinel value. --- model_utils/tracker.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/model_utils/tracker.py b/model_utils/tracker.py index a0bc0d0..e1e61fe 100644 --- a/model_utils/tracker.py +++ b/model_utils/tracker.py @@ -63,8 +63,7 @@ class DescriptorWrapper(object): # This will undefer the field getattr(instance, self.field_name) finally: - if already_setting: - instance.__dict__.pop(recursion_sentinel_attname, None) + instance.__dict__.pop(recursion_sentinel_attname, None) if hasattr(self.descriptor, '__set__'): self.descriptor.__set__(instance, value) else: From cde1d706afb16ef3a48eeef414310da9ae75aa36 Mon Sep 17 00:00:00 2001 From: Lucas Wiman Date: Thu, 28 Jun 2018 14:08:03 -0700 Subject: [PATCH 13/16] Cover a branch in `has_changed`. --- tests/test_fields/test_field_tracker.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_fields/test_field_tracker.py b/tests/test_fields/test_field_tracker.py index dc29e70..604ec60 100644 --- a/tests/test_fields/test_field_tracker.py +++ b/tests/test_fields/test_field_tracker.py @@ -189,6 +189,7 @@ class FieldTrackerTests(FieldTrackerTestCase, FieldTrackerCommonTests): # 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')) if django.VERSION >= (1, 10): self.assertTrue('number' in item.get_deferred_fields()) else: From ca2fbb4ccd27e85dbe95744bd3d1fe5412a4d72d Mon Sep 17 00:00:00 2001 From: Lucas Wiman Date: Thu, 28 Jun 2018 16:59:30 -0700 Subject: [PATCH 14/16] Fix coverage for a codepath only executed in <1.10 environments. --- model_utils/tracker.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/model_utils/tracker.py b/model_utils/tracker.py index e1e61fe..9c6de5a 100644 --- a/model_utils/tracker.py +++ b/model_utils/tracker.py @@ -186,10 +186,7 @@ class FieldInstanceTracker(object): field_tracker = FileDescriptorTracker(field_obj.field) setattr(self.instance.__class__, field, field_tracker) else: - if django.VERSION < (2, 1): - field_tracker = DeferredAttributeTracker(field, type(self.instance)) - else: - field_tracker = DeferredAttributeTracker(field) + field_tracker = DeferredAttributeTracker(field, type(self.instance)) setattr(self.instance.__class__, field, field_tracker) From 7d6b45f0c1943c1af50c60305ccfdd703f4290ea Mon Sep 17 00:00:00 2001 From: Lucas Wiman Date: Thu, 28 Jun 2018 17:04:57 -0700 Subject: [PATCH 15/16] Increase coverage: verify that accessing the descriptor from the class yields the descriptor object. --- model_utils/tracker.py | 3 +-- tests/test_fields/test_field_tracker.py | 2 ++ 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/model_utils/tracker.py b/model_utils/tracker.py index 9c6de5a..582ae06 100644 --- a/model_utils/tracker.py +++ b/model_utils/tracker.py @@ -71,8 +71,7 @@ class DescriptorWrapper(object): @staticmethod def cls_for_descriptor(descriptor): - has_del = hasattr(descriptor, '__delete__') - if has_del: + if hasattr(descriptor, '__delete__'): return FullDescriptorWrapper else: return DescriptorWrapper diff --git a/tests/test_fields/test_field_tracker.py b/tests/test_fields/test_field_tracker.py index 604ec60..93b9efa 100644 --- a/tests/test_fields/test_field_tracker.py +++ b/tests/test_fields/test_field_tracker.py @@ -7,6 +7,7 @@ from django.core.exceptions import FieldError from django.test import TestCase from model_utils import FieldTracker +from model_utils.tracker import DescriptorWrapper from tests.models import ( Tracked, TrackedFK, InheritedTrackedFK, TrackedNotDefault, TrackedNonFieldAttr, TrackedMultiple, InheritedTracked, TrackedFileField, @@ -191,6 +192,7 @@ class FieldTrackerTests(FieldTrackerTestCase, FieldTrackerCommonTests): if self.tracked_class == Tracked: self.assertFalse(item.tracker.has_changed('number')) if django.VERSION >= (1, 10): + self.assertIsInstance(item.__class__.number, DescriptorWrapper) self.assertTrue('number' in item.get_deferred_fields()) else: self.assertTrue('number' in item._deferred_fields) From 45502c0ec2044f3a35183756d3b46ce456486853 Mon Sep 17 00:00:00 2001 From: Lucas Wiman Date: Sat, 30 Jun 2018 15:52:00 -0700 Subject: [PATCH 16/16] Put changelog in the right place. --- CHANGES.rst | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index b367ada..85cf4cb 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -4,16 +4,14 @@ CHANGES master (unreleased) ------------------- - Fix handling of deferred attributes on Django 1.10+, fixes GH-278 +- Fix `FieldTracker.has_changed()` and `FieldTracker.previous()` to return + correct responses for deferred fields. 3.1.2 (2018.05.09) ------------------ * Update InheritanceIterable to inherit from ModelIterable instead of BaseIterable, fixes GH-277. - -- Fix `FieldTracker.has_changed()` and `FieldTracker.previous()` to return - correct responses for deferred fields. - 3.1.1 (2017.12.17) ------------------