From a3b86f101a7a675efd5f979bffa2244f9bcc4c56 Mon Sep 17 00:00:00 2001 From: Ross Mechanic Date: Fri, 15 Jun 2018 12:27:05 -0400 Subject: [PATCH] Don't populate excluded fields in populate history (#410) * Adds tests for two excluded-fields fixes: - tests the management command on an historical record which has excluded fields - tests the excluded fields are retrieved from the current model instance * Adds test for foreign-key excluded field. * Reduces number of queries made by `History-Model.get_instance`. Previously, it was pre-loading directly related objects. * Don't populate excluded fields while populating existing model history * Adjustments * Updated AUTHORS and log CHANGES * Renames excluded_fields -> _history_excluded_fields update_record -> initial_history_record * Fixed accidental deletions to CHANGES.rst and AUTHORS.rst --- AUTHORS.rst | 2 + CHANGES.rst | 1 + simple_history/manager.py | 1 + simple_history/models.py | 19 ++++++-- simple_history/tests/models.py | 8 ++++ simple_history/tests/tests/test_commands.py | 12 ++++- simple_history/tests/tests/test_models.py | 50 +++++++++++++++++++++ 7 files changed, 89 insertions(+), 4 deletions(-) diff --git a/AUTHORS.rst b/AUTHORS.rst index 445acea76..226d27a04 100644 --- a/AUTHORS.rst +++ b/AUTHORS.rst @@ -62,6 +62,8 @@ Authors - Mike Spainhower - Alexander Anikeev - Kyle Seever +- Adnan Umer (@uadnan) +- Jonathan Zvesper (@zvesp) Background ========== diff --git a/CHANGES.rst b/CHANGES.rst index 1b8873ddb..5e4de0be9 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -4,6 +4,7 @@ Changes Unreleased ---------- - Fixed out-of-memory exception when running populate_history management command (gh-408) +- Fix TypeError on populate_history if excluded_fields are specified (gh-410) 2.1.0 (2018-06-04) ------------------ diff --git a/simple_history/manager.py b/simple_history/manager.py index ccffc1960..59ecd44bb 100755 --- a/simple_history/manager.py +++ b/simple_history/manager.py @@ -100,6 +100,7 @@ def bulk_history_create(self, objs, batch_size=None): **{ field.attname: getattr(instance, field.attname) for field in instance._meta.fields + if field.name not in self.model._history_excluded_fields } ) for instance in objs] diff --git a/simple_history/models.py b/simple_history/models.py index 27df2e948..679481733 100644 --- a/simple_history/models.py +++ b/simple_history/models.py @@ -113,7 +113,10 @@ def create_history_model(self, model, inherited): """ Creates a historical model to associate with the model provided. """ - attrs = {'__module__': self.module} + attrs = { + '__module__': self.module, + '_history_excluded_fields': self.excluded_fields + } app_module = '%s.models' % model._meta.app_label @@ -220,10 +223,20 @@ def revert_url(self): ) def get_instance(self): - return model(**{ + attrs = { field.attname: getattr(self, field.attname) for field in fields.values() - }) + } + if self._history_excluded_fields: + excluded_attnames = [ + model._meta.get_field(field).attname + for field in self._history_excluded_fields + ] + values = model.objects.filter( + pk=getattr(self, model._meta.pk.attname) + ).values(*excluded_attnames).get() + attrs.update(values) + return model(**attrs) def get_next_record(self): """ diff --git a/simple_history/tests/models.py b/simple_history/tests/models.py index 5050a9082..39df3301a 100644 --- a/simple_history/tests/models.py +++ b/simple_history/tests/models.py @@ -32,6 +32,14 @@ class PollWithExcludeFields(models.Model): history = HistoricalRecords(excluded_fields=['pub_date']) +class PollWithExcludedFKField(models.Model): + question = models.CharField(max_length=200) + pub_date = models.DateTimeField('date published') + place = models.ForeignKey('Place', on_delete=models.CASCADE) + + history = HistoricalRecords(excluded_fields=['place']) + + class Temperature(models.Model): location = models.CharField(max_length=200) temperature = models.IntegerField() diff --git a/simple_history/tests/tests/test_commands.py b/simple_history/tests/tests/test_commands.py index 7b68dc742..eb9d34796 100644 --- a/simple_history/tests/tests/test_commands.py +++ b/simple_history/tests/tests/test_commands.py @@ -7,7 +7,7 @@ from simple_history import models as sh_models from simple_history.management.commands import populate_history -from ..models import Book, Poll, Restaurant +from ..models import Book, Poll, PollWithExcludeFields, Restaurant @contextmanager @@ -142,3 +142,13 @@ def test_stdout_printed_when_verbosity_is_not_specified(self): stdout=out, stderr=StringIO()) self.assertNotEqual(out.getvalue(), '') + + def test_excluded_fields(self): + poll = PollWithExcludeFields.objects.create( + question="Will this work?", pub_date=datetime.now()) + PollWithExcludeFields.history.all().delete() + management.call_command(self.command_name, + 'tests.pollwithexcludefields', auto=True, + stdout=StringIO(), stderr=StringIO()) + initial_history_record = PollWithExcludeFields.history.all()[0] + self.assertEqual(initial_history_record.question, poll.question) diff --git a/simple_history/tests/tests/test_models.py b/simple_history/tests/tests/test_models.py index e99057299..020d6f3a6 100644 --- a/simple_history/tests/tests/test_models.py +++ b/simple_history/tests/tests/test_models.py @@ -40,9 +40,11 @@ Library, MultiOneToOne, Person, + Place, Poll, PollInfo, PollWithExcludeFields, + PollWithExcludedFKField, Province, Restaurant, SelfFK, @@ -946,3 +948,51 @@ def test_custom_table_name_from_register(self): self.get_table_name(ContactRegister.history), 'contacts_register_history', ) + + +class ExcludeFieldsTest(TestCase): + def test_restore_pollwithexclude(self): + poll = PollWithExcludeFields.objects.create(question="what's up?", + pub_date=today) + historical = poll.history.order_by('pk')[0] + with self.assertRaises(AttributeError): + historical.pub_date + original = historical.instance + self.assertEqual(original.pub_date, poll.pub_date) + + +class ExcludeForeignKeyTest(TestCase): + def setUp(self): + self.poll = PollWithExcludedFKField.objects.create( + question="Is it?", pub_date=today, + place=Place.objects.create(name="Somewhere") + ) + + def get_first_historical(self): + """ + Retrieve the idx'th HistoricalPoll, ordered by time. + """ + return self.poll.history.order_by('history_date')[0] + + def test_instance_fk_value(self): + historical = self.get_first_historical() + original = historical.instance + self.assertEqual(original.place, self.poll.place) + + def test_history_lacks_fk(self): + historical = self.get_first_historical() + with self.assertRaises(AttributeError): + historical.place + + def test_nb_queries(self): + with self.assertNumQueries(2): + historical = self.get_first_historical() + historical.instance + + def test_changed_value_lost(self): + new_place = Place.objects.create(name="More precise") + self.poll.place = new_place + self.poll.save() + historical = self.get_first_historical() + instance = historical.instance + self.assertEqual(instance.place, new_place)