diff --git a/AUTHORS.rst b/AUTHORS.rst index d61a0927c..6ae593cca 100644 --- a/AUTHORS.rst +++ b/AUTHORS.rst @@ -107,6 +107,7 @@ Authors - Stefan Borer (`sbor23 `_) - Steven Buss (`sbuss `_) - Steven Klass +- Tim Schilling (`tim-schilling `_) - Tommy Beadle (`tbeadle `_) - Trey Hunner (`treyhunner `_) - Ulysses Vilela diff --git a/CHANGES.rst b/CHANGES.rst index b71a133a1..bf20ebca2 100644 --- a/CHANGES.rst +++ b/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) diff --git a/docs/common_issues.rst b/docs/common_issues.rst index 7eac8eb8d..46101b7cc 100644 --- a/docs/common_issues.rst +++ b/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"])} + ) diff --git a/simple_history/models.py b/simple_history/models.py index 0c2ac1d74..0afb0b21c 100644 --- a/simple_history/models.py +++ b/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 diff --git a/simple_history/registry_tests/migration_test_app/migrations/0003_historicalmodelwithcustomattronetoonefield_modelwithcustomattronetoonefield.py b/simple_history/registry_tests/migration_test_app/migrations/0003_historicalmodelwithcustomattronetoonefield_modelwithcustomattronetoonefield.py new file mode 100644 index 000000000..cc97a14cf --- /dev/null +++ b/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), + ), + ] diff --git a/simple_history/registry_tests/migration_test_app/models.py b/simple_history/registry_tests/migration_test_app/models.py index ac726404f..8d93a83ef 100644 --- a/simple_history/registry_tests/migration_test_app/models.py +++ b/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"])} + ) diff --git a/simple_history/registry_tests/tests.py b/simple_history/registry_tests/tests.py index 727aa625f..edc30887d 100644 --- a/simple_history/registry_tests/tests.py +++ b/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): diff --git a/simple_history/tests/models.py b/simple_history/tests/models.py index 640c27d78..29d2ee4fd 100644 --- a/simple_history/tests/models.py +++ b/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()