Add excluded_field_kwargs to support custom OneToOneFields. (#871)

Coercing OneToOneFields to be ForeignKeys on historical
models causes issues when the OneToOneField is a custom
subclass that includes additional arguments that do not
exist on the ForeignKey.__init__  method.

These additional arguments can be specified via excluded_field_kwargs
to allow these custom OneToOneFields be coerced into ForeignKeys
on the historical model.

Fixes #870

Co-authored-by: Jim King <jim.king@cloudtruth.com>

Author: tim-schilling

AUTHORS.rst

@@ -107,6 +107,7 @@ Authors
 - Stefan Borer (`sbor23 <https://github.com/sbor23>`_)
 - Steven Buss (`sbuss <https://github.com/sbuss>`_)
 - Steven Klass
+- Tim Schilling (`tim-schilling <https://github.com/tim-schilling>`_)
 - Tommy Beadle (`tbeadle <https://github.com/tbeadle>`_)
 - Trey Hunner (`treyhunner <https://github.com/treyhunner>`_)
 - Ulysses Vilela

CHANGES.rst

@@ -11,6 +11,8 @@ Upgrade Implications:
 Full list of changes:
 
 - Added index on `history_date` column; opt-out with setting `SIMPLE_HISTORY_DATE_INDEX` (gh-565)
+- Added ``excluded_field_kwargs`` to support custom ``OneToOneField`` that have
+  additional arguments that don't exist on ``ForeignKey``. (gh-870)
 - Fixed ``prev_record`` and ``next_record`` performance when using ``excluded_fields`` (gh-791)
 - Fixed `update_change_reason` in pk (gh-806)
 - Fixed bug where serializer of djangorestframework crashed if used with ``OrderingFilter`` (gh-821)

docs/common_issues.rst

@@ -254,3 +254,25 @@ Working with BitBucket Pipelines
 
 When using BitBucket Pipelines to test your Django project with the
 django-simple-history middleware, you will run into an error relating to missing migrations relating to the historic User model from the auth app. This is because the migration file is not held within either your project or django-simple-history.  In order to pypass the error you need to add a ```python manage.py makemigrations auth``` step into your YML file prior to running the tests.
+
+
+Using custom OneToOneFields
+---------------------------
+
+If you are using a custom OneToOneField that has additional arguments and receiving
+the the following ``TypeError``::
+
+..
+    TypeError: __init__() got an unexpected keyword argument
+
+This is because Django Simple History coerces ``OneToOneField`` into ``ForeignKey``
+on the historical model. You can work around this by excluded those additional
+arguments using ``excluded_field_kwargs`` as follows:
+
+.. code-block:: python
+
+    class Poll(models.Model):
+        organizer = CustomOneToOneField(Organizer, ..., custom_argument="some_value")
+        history = HistoricalRecords(
+            excluded_field_kwargs={"organizer": set(["custom_argument"])}
+        )

simple_history/models.py

@@ -79,6 +79,7 @@ def __init__(
         related_name=None,
         use_base_model_db=False,
         user_db_constraint=True,
+        excluded_field_kwargs=None,
     ):
         self.user_set_verbose_name = verbose_name
         self.user_related_name = user_related_name
@@ -101,6 +102,10 @@ def __init__(
         if excluded_fields is None:
             excluded_fields = []
         self.excluded_fields = excluded_fields
+
+        if excluded_field_kwargs is None:
+            excluded_field_kwargs = {}
+        self.excluded_field_kwargs = excluded_field_kwargs
         try:
             if isinstance(bases, str):
                 raise TypeError
@@ -235,6 +240,12 @@ def fields_included(self, model):
                 fields.append(field)
         return fields
 
+    def field_excluded_kwargs(self, field):
+        """
+        Find the excluded kwargs for a given field.
+        """
+        return self.excluded_field_kwargs.get(field.name, set())
+
     def copy_fields(self, model):
         """
         Creates copies of the model's original fields, returning
@@ -262,6 +273,12 @@ def copy_fields(self, model):
                 else:
                     FieldType = type(old_field)
 
+                # Remove any excluded kwargs for the field.
+                # This is useful when a custom OneToOneField is being used that
+                # has a different set of arguments than ForeignKey
+                for exclude_arg in self.field_excluded_kwargs(old_field):
+                    field_args.pop(exclude_arg, None)
+
                 # If field_args['to'] is 'self' then we have a case where the object
                 # has a foreign key to itself. If we pass the historical record's
                 # field to = 'self', the foreign key will point to an historical

simple_history/registry_tests/migration_test_app/migrations/0003_historicalmodelwithcustomattronetoonefield_modelwithcustomattronetoonefield.py

@@ -0,0 +1,91 @@
+# Generated by Django 3.2.6 on 2021-08-24 10:36
+
+import django.db.models.deletion
+from django.conf import settings
+from django.db import migrations, models
+
+import simple_history.models
+import simple_history.registry_tests.migration_test_app.models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        migrations.swappable_dependency(settings.AUTH_USER_MODEL),
+        (
+            "migration_test_app",
+            "0002_historicalmodelwithcustomattrforeignkey_modelwithcustomattrforeignkey",
+        ),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name="ModelWithCustomAttrOneToOneField",
+            fields=[
+                (
+                    "id",
+                    models.AutoField(
+                        auto_created=True,
+                        primary_key=True,
+                        serialize=False,
+                        verbose_name="ID",
+                    ),
+                ),
+                (
+                    "what_i_mean",
+                    simple_history.registry_tests.migration_test_app.models.CustomAttrNameOneToOneField(
+                        attr_name="custom_attr_name",
+                        on_delete=django.db.models.deletion.CASCADE,
+                        to="migration_test_app.whatimean",
+                    ),
+                ),
+            ],
+        ),
+        migrations.CreateModel(
+            name="HistoricalModelWithCustomAttrOneToOneField",
+            fields=[
+                (
+                    "id",
+                    models.IntegerField(
+                        auto_created=True, blank=True, db_index=True, verbose_name="ID"
+                    ),
+                ),
+                ("history_id", models.AutoField(primary_key=True, serialize=False)),
+                ("history_date", models.DateTimeField()),
+                ("history_change_reason", models.CharField(max_length=100, null=True)),
+                (
+                    "history_type",
+                    models.CharField(
+                        choices=[("+", "Created"), ("~", "Changed"), ("-", "Deleted")],
+                        max_length=1,
+                    ),
+                ),
+                (
+                    "history_user",
+                    models.ForeignKey(
+                        null=True,
+                        on_delete=django.db.models.deletion.SET_NULL,
+                        related_name="+",
+                        to=settings.AUTH_USER_MODEL,
+                    ),
+                ),
+                (
+                    "what_i_mean",
+                    models.ForeignKey(
+                        blank=True,
+                        db_constraint=False,
+                        null=True,
+                        on_delete=django.db.models.deletion.DO_NOTHING,
+                        related_name="+",
+                        to="migration_test_app.whatimean",
+                    ),
+                ),
+            ],
+            options={
+                "verbose_name": "historical model with custom attr one to one field",
+                "ordering": ("-history_date", "-history_id"),
+                "get_latest_by": "history_date",
+            },
+            bases=(simple_history.models.HistoricalChanges, models.Model),
+        ),
+    ]

simple_history/registry_tests/migration_test_app/models.py

@@ -36,3 +36,29 @@ class ModelWithCustomAttrForeignKey(models.Model):
         WhatIMean, models.CASCADE, attr_name="custom_attr_name"
     )
     history = HistoricalRecords()
+
+
+class CustomAttrNameOneToOneField(models.OneToOneField):
+    def __init__(self, *args, **kwargs):
+        self.attr_name = kwargs.pop("attr_name", None)
+        super(CustomAttrNameOneToOneField, self).__init__(*args, **kwargs)
+
+    def get_attname(self):
+        return self.attr_name or super(CustomAttrNameOneToOneField, self).get_attname()
+
+    def deconstruct(self):
+        name, path, args, kwargs = super(
+            CustomAttrNameOneToOneField, self
+        ).deconstruct()
+        if self.attr_name:
+            kwargs["attr_name"] = self.attr_name
+        return name, path, args, kwargs
+
+
+class ModelWithCustomAttrOneToOneField(models.Model):
+    what_i_mean = CustomAttrNameOneToOneField(
+        WhatIMean, models.CASCADE, attr_name="custom_attr_name"
+    )
+    history = HistoricalRecords(
+        excluded_field_kwargs={"what_i_mean": set(["attr_name"])}
+    )

simple_history/registry_tests/tests.py

@@ -17,6 +17,7 @@
     InheritTracking3,
     InheritTracking4,
     ModelWithCustomAttrForeignKey,
+    ModelWithCustomAttrOneToOneField,
     ModelWithHistoryInDifferentApp,
     Poll,
     Restaurant,
@@ -205,6 +206,14 @@ def test_custom_attr(self):
         self.assertEqual(field.attr_name, "custom_poll")
 
 
+class TestCustomAttrOneToOneField(TestCase):
+    """https://github.com/jazzband/django-simple-history/issues/870"""
+
+    def test_custom_attr(self):
+        field = ModelWithCustomAttrOneToOneField.history.model._meta.get_field("poll")
+        self.assertFalse(hasattr(field, "attr_name"))
+
+
 @override_settings(MIGRATION_MODULES={})
 class TestMigrate(TestCase):
     def test_makemigration_command(self):

simple_history/tests/models.py

@@ -119,6 +119,28 @@ class ModelWithCustomAttrForeignKey(models.Model):
     history = HistoricalRecords()
 
 
+class CustomAttrNameOneToOneField(models.OneToOneField):
+    def __init__(self, *args, **kwargs):
+        self.attr_name = kwargs.pop("attr_name", None)
+        super(CustomAttrNameOneToOneField, self).__init__(*args, **kwargs)
+
+    def get_attname(self):
+        return self.attr_name or super(CustomAttrNameOneToOneField, self).get_attname()
+
+    def deconstruct(self):
+        name, path, args, kwargs = super(
+            CustomAttrNameOneToOneField, self
+        ).deconstruct()
+        if self.attr_name:
+            kwargs["attr_name"] = self.attr_name
+        return name, path, args, kwargs
+
+
+class ModelWithCustomAttrOneToOneField(models.Model):
+    poll = CustomAttrNameOneToOneField(Poll, models.CASCADE, attr_name="custom_poll")
+    history = HistoricalRecords(excluded_field_kwargs={"poll": set(["attr_name"])})
+
+
 class Temperature(models.Model):
     location = models.CharField(max_length=200)
     temperature = models.IntegerField()