Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add excluded_field_kwargs to support custom OneToOneFields. #871

Merged
merged 2 commits into from
Oct 6, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
22 changes: 22 additions & 0 deletions docs/common_issues.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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"])}
)
17 changes: 17 additions & 0 deletions simple_history/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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),
),
]
26 changes: 26 additions & 0 deletions simple_history/registry_tests/migration_test_app/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"])}
)
9 changes: 9 additions & 0 deletions simple_history/registry_tests/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
InheritTracking3,
InheritTracking4,
ModelWithCustomAttrForeignKey,
ModelWithCustomAttrOneToOneField,
ModelWithHistoryInDifferentApp,
Poll,
Restaurant,
Expand Down Expand Up @@ -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):
Expand Down
22 changes: 22 additions & 0 deletions simple_history/tests/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down