Ensure custom arguments for fields are included in historical models' fields. (#462)

Previously, if a custom field class was used for a foreign key and that
class accepted custom kwargs, those kwargs would not be included in the
creation of the historical model's field and would break the migration.
To address this, we'll use the deconstruct() method of the field to get
the arguments that should be passed to it and override the ones we need
to for the historical model's field.

Author: tbeadle

CHANGES.rst

@@ -7,6 +7,7 @@ Unreleased
 - Fix header on history pages when custom site_header is used (gh-448)
 - Modify `pre_create_historircal_record` to pass `history_instance` for ease of customization (gh-421)
 - Raise warning if HistoricalRecords(inherit=False) is in an abstract model (gh-341)
+- Ensure custom arguments for fields are included in historical models' fields (gh-431)
 
 2.5.1 (2018-10-19)
 ------------------

simple_history/models.py

@@ -184,29 +184,22 @@ def copy_fields(self, model):
                 field.__class__ = models.IntegerField
             if isinstance(field, models.ForeignKey):
                 old_field = field
-                field_arguments = {'db_constraint': False}
+                old_swappable = old_field.swappable
+                old_field.swappable = False
+                try:
+                    _name, _path, args, field_args = old_field.deconstruct()
+                finally:
+                    old_field.swappable = old_swappable
                 if getattr(old_field, 'one_to_one', False) \
                    or isinstance(old_field, models.OneToOneField):
                     FieldType = models.ForeignKey
                 else:
                     FieldType = type(old_field)
-                if getattr(old_field, 'to_fields', []):
-                    field_arguments['to_field'] = old_field.to_fields[0]
-                if getattr(old_field, 'db_column', None):
-                    field_arguments['db_column'] = old_field.db_column
-
-                # If old_field.remote_field.model is 'self' then we have a
-                # case where object has a foreign key to itself. In this case
-                # we need to set the `model` value of the field to a model. We
-                # can use the old_field.model value.
-                if isinstance(old_field.remote_field.model, str) and \
-                   old_field.remote_field.model == 'self':
-                    object_to = old_field.model
-                else:
-                    object_to = old_field.remote_field.model
 
-                field = FieldType(
-                    object_to,
+                # Override certain arguments passed when creating the field
+                # so that they work for the historical field.
+                field_args.update(
+                    db_constraint=False,
                     related_name='+',
                     null=True,
                     blank=True,
@@ -215,7 +208,10 @@ def copy_fields(self, model):
                     serialize=True,
                     unique=False,
                     on_delete=models.DO_NOTHING,
-                    **field_arguments
+                )
+                field = FieldType(
+                    *args,
+                    **field_args
                 )
                 field.name = old_field.name
             else:

simple_history/registry_tests/migration_test_app/migrations/0002_historicalmodelwithcustomattrforeignkey_modelwithcustomattrforeignkey.py

@@ -0,0 +1,42 @@
+# Generated by Django 2.1 on 2018-10-19 21:53
+
+from django.conf import settings
+from django.db import migrations, models
+import django.db.models.deletion
+import simple_history.models
+from .. import models as my_models
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        migrations.swappable_dependency(settings.AUTH_USER_MODEL),
+        ('migration_test_app', '0001_initial'),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name='HistoricalModelWithCustomAttrForeignKey',
+            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_change_reason', models.CharField(max_length=100, null=True)),
+                ('history_date', models.DateTimeField()),
+                ('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', my_models.CustomAttrNameForeignKey(attr_name='custom_attr_name', 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 foreign key',
+                'ordering': ('-history_date', '-history_id'),
+                'get_latest_by': 'history_date',
+            },
+            bases=(simple_history.models.HistoricalChanges, models.Model),
+        ),
+        migrations.CreateModel(
+            name='ModelWithCustomAttrForeignKey',
+            fields=[
+                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
+                ('what_i_mean', my_models.CustomAttrNameForeignKey(attr_name='custom_attr_name', on_delete=django.db.models.deletion.CASCADE, to='migration_test_app.WhatIMean')),
+            ],
+        ),
+    ]

simple_history/registry_tests/migration_test_app/models.py

@@ -13,3 +13,31 @@ class WhatIMean(DoYouKnow):
 class Yar(models.Model):
     what = models.ForeignKey(WhatIMean, on_delete=models.CASCADE)
     history = HistoricalRecords()
+
+
+class CustomAttrNameForeignKey(models.ForeignKey):
+    def __init__(self, *args, **kwargs):
+        self.attr_name = kwargs.pop("attr_name", None)
+        super(CustomAttrNameForeignKey, self).__init__(*args, **kwargs)
+
+    def get_attname(self):
+        return self.attr_name or super(
+            CustomAttrNameForeignKey, self
+        ).get_attname()
+
+    def deconstruct(self):
+        name, path, args, kwargs = super(
+            CustomAttrNameForeignKey, self
+        ).deconstruct()
+        if self.attr_name:
+            kwargs["attr_name"] = self.attr_name
+        return name, path, args, kwargs
+
+
+class ModelWithCustomAttrForeignKey(models.Model):
+    what_i_mean = CustomAttrNameForeignKey(
+        WhatIMean,
+        models.CASCADE,
+        attr_name='custom_attr_name',
+    )
+    history = HistoricalRecords()

simple_history/registry_tests/tests.py

@@ -16,7 +16,8 @@
                             Restaurant, TrackedAbstractBaseA,
                             TrackedAbstractBaseB, TrackedWithAbstractBase,
                             TrackedWithConcreteBase, UserAccessorDefault,
-                            UserAccessorOverride, UUIDRegisterModel, Voter)
+                            UserAccessorOverride, UUIDRegisterModel, Voter,
+                            ModelWithCustomAttrForeignKey)
 
 get_model = apps.get_model
 User = get_user_model()
@@ -109,15 +110,15 @@ def test_tracked_abstract_base(self):
 
     def test_tracked_concrete_base(self):
         self.assertEqual(
-            [
+            sorted(
                 f.attname
                 for f in TrackedWithConcreteBase.history.model._meta.fields
-            ],
-            [
+            ),
+            sorted([
                 'id', 'trackedconcretebase_ptr_id', 'history_id',
                 'history_change_reason', 'history_date', 'history_user_id',
                 'history_type',
-            ],
+            ]),
         )
 
     def test_multiple_tracked_bases(self):
@@ -128,39 +129,55 @@ class TrackedWithMultipleAbstractBases(
 
     def test_tracked_abstract_and_untracked_concrete_base(self):
         self.assertEqual(
-            [f.attname for f in InheritTracking1.history.model._meta.fields],
-            [
+            sorted(
+                f.attname for f in InheritTracking1.history.model._meta.fields
+            ),
+            sorted([
                 'id', 'untrackedconcretebase_ptr_id', 'history_id',
                 'history_change_reason', 'history_date',
                 'history_user_id', 'history_type',
-            ],
+            ]),
         )
 
     def test_indirect_tracked_abstract_base(self):
         self.assertEqual(
-            [f.attname for f in InheritTracking2.history.model._meta.fields],
-            [
+            sorted(
+                f.attname for f in InheritTracking2.history.model._meta.fields
+            ),
+            sorted([
                 'id', 'baseinherittracking2_ptr_id', 'history_id',
                 'history_change_reason', 'history_date',
                 'history_user_id', 'history_type',
-            ],
+            ]),
         )
 
     def test_indirect_tracked_concrete_base(self):
         self.assertEqual(
-            [f.attname for f in InheritTracking3.history.model._meta.fields],
-            [
+            sorted(
+                f.attname for f in InheritTracking3.history.model._meta.fields
+            ),
+            sorted([
                 'id', 'baseinherittracking3_ptr_id', 'history_id',
                 'history_change_reason', 'history_date',
                 'history_user_id', 'history_type',
-            ],
+            ]),
         )
 
     def test_registering_with_tracked_abstract_base(self):
         with self.assertRaises(exceptions.MultipleRegistrationsError):
             register(InheritTracking4)
 
 
+class TestCustomAttrForeignKey(TestCase):
+    """ https://github.com/treyhunner/django-simple-history/issues/431 """
+
+    def test_custom_attr(self):
+        field = ModelWithCustomAttrForeignKey.history.model._meta.get_field(
+            'poll',
+        )
+        self.assertEqual(field.attr_name, 'custom_poll')
+
+
 class TestMigrate(TestCase):
 
     def test_makemigration_command(self):

simple_history/tests/models.py

@@ -59,6 +59,34 @@ def get_absolute_url(self):
         return reverse('poll-detail', kwargs={'pk': self.pk})
 
 
+class CustomAttrNameForeignKey(models.ForeignKey):
+    def __init__(self, *args, **kwargs):
+        self.attr_name = kwargs.pop("attr_name", None)
+        super(CustomAttrNameForeignKey, self).__init__(*args, **kwargs)
+
+    def get_attname(self):
+        return self.attr_name or super(
+            CustomAttrNameForeignKey, self
+        ).get_attname()
+
+    def deconstruct(self):
+        name, path, args, kwargs = super(
+            CustomAttrNameForeignKey, self
+        ).deconstruct()
+        if self.attr_name:
+            kwargs["attr_name"] = self.attr_name
+        return name, path, args, kwargs
+
+
+class ModelWithCustomAttrForeignKey(models.Model):
+    poll = CustomAttrNameForeignKey(
+        Poll,
+        models.CASCADE,
+        attr_name='custom_poll',
+    )
+    history = HistoricalRecords()
+
+
 class Temperature(models.Model):
     location = models.CharField(max_length=200)
     temperature = models.IntegerField()