Exclude ManyToManyFields when fetching excluded fields. (#707)

* Exclude ManyToManyFields when fetching excluded fields.

Includes a test that fails on master but passes with the change.

* Formatting fix.

* Fix existing failing test.

* Small naming fix.

* Add to CHANGES.rst

* Add a comment explaining the fix to #706.

Author: j-wing

AUTHORS.rst

@@ -59,6 +59,7 @@ Authors
 - Jonathan Loo (`alpha1d3d <https://github.com/alpha1d3d>`_)
 - Jonathan Sanchez
 - Jonathan Zvesper (`zvesp <https://github.com/zvesp>`_)
+- Jordon Wing  (`jordonwii <https://github.com/jordonwii`_)
 - Josh Fyne
 - Keith Hackbarth
 - Kevin Foster

CHANGES.rst

@@ -6,6 +6,7 @@ Unreleased
 - Add default date to ``bulk_create_with_history`` and ``bulk_update_with_history`` (gh-687)
 - Exclude ManyToManyFields when using ``bulk_create_with_history`` (gh-699)
 - Added ``--excluded_fields`` argument to ``clean_duplicate_history`` command (gh-674)
+- Exclude ManyToManyFields when fetching excluded fields (gh-707)
 
 2.11.0 (2020-06-20)
 -------------------

simple_history/models.py

@@ -14,7 +14,7 @@
 from django.contrib.auth import get_user_model
 from django.core.exceptions import ObjectDoesNotExist
 from django.db import models
-from django.db.models import Q
+from django.db.models import ManyToManyField, Q
 from django.db.models.fields.proxy import OrderWrt
 from django.forms.models import model_to_dict
 from django.urls import reverse
@@ -384,9 +384,12 @@ def get_instance(self):
                 field.attname: getattr(self, field.attname) for field in fields.values()
             }
             if self._history_excluded_fields:
+                # We don't add ManyToManyFields to this list because they may cause
+                # the subsequent `.get()` call to fail. See #706 for context.
                 excluded_attnames = [
                     model._meta.get_field(field).attname
                     for field in self._history_excluded_fields
+                    if not isinstance(model._meta.get_field(field), ManyToManyField)
                 ]
                 try:
                     values = (

simple_history/tests/models.py

@@ -695,11 +695,17 @@ class Street(models.Model):
     log = HistoricalRecords(related_name="history")
 
 
-class BulkCreateManyToManyModelOther(models.Model):
+class ManyToManyModelOther(models.Model):
     name = models.CharField(max_length=15, unique=True)
 
 
 class BulkCreateManyToManyModel(models.Model):
     name = models.CharField(max_length=15, unique=True)
-    other = models.ManyToManyField(BulkCreateManyToManyModelOther)
+    other = models.ManyToManyField(ManyToManyModelOther)
     history = HistoricalRecords()
+
+
+class ModelWithExcludedManyToMany(models.Model):
+    name = models.CharField(max_length=15, unique=True)
+    other = models.ManyToManyField(ManyToManyModelOther)
+    history = HistoricalRecords(excluded_fields=["other"])

simple_history/tests/tests/test_models.py

@@ -69,6 +69,8 @@
     OverrideModelNameUsingBaseModel1,
     MyOverrideModelNameRegisterMethod1,
     Library,
+    ManyToManyModelOther,
+    ModelWithExcludedManyToMany,
     ModelWithFkToModelWithHistoryUsingBaseModelDb,
     ModelWithHistoryInDifferentDb,
     ModelWithHistoryUsingBaseModelDb,
@@ -1004,6 +1006,16 @@ def test_as_of_nonexistant(self):
         poll.delete()
         self.assertRaises(Poll.DoesNotExist, poll.history.as_of, time)
 
+    def test_as_of_excluded_many_to_many_succeeds(self):
+        other1 = ManyToManyModelOther.objects.create(name="test1")
+        other2 = ManyToManyModelOther.objects.create(name="test2")
+
+        m = ModelWithExcludedManyToMany.objects.create(name="test")
+        m.other.add(other1, other2)
+
+        # This will fail if the ManyToMany field is not excluded.
+        self.assertEqual(m.history.as_of(datetime.now()), m)
+
     def test_foreignkey_field(self):
         why_poll = Poll.objects.create(question="why?", pub_date=today)
         how_poll = Poll.objects.create(question="how?", pub_date=today)
@@ -1163,7 +1175,14 @@ def test_migrations_include_order(self):
 
         model_state = state.ModelState.from_model(SeriesWork.history.model)
         found = False
-        for name, field in model_state.fields:
+        # `fields` is a dict in Django 3.1
+        fields = None
+        if isinstance(model_state.fields, dict):
+            fields = model_state.fields.items()
+        else:
+            fields = model_state.fields
+
+        for name, field in fields:
             if name == "_order":
                 found = True
                 self.assertEqual(type(field), models.IntegerField)