django-simple-history icon indicating copy to clipboard operation
django-simple-history copied to clipboard

Proxy models don't have a way to share the history table

Open sknutsonsf opened this issue 5 years ago • 13 comments

Describe the bug I have a proxy model with no new fields. I want the derived class to use the same history table as the parent table.

Using register(SubItem, inherit=True) results in history going into a different historical database table and there is no way to override this. If I give the table_name of the parent objects' history table, the migration fails since that table already exists.

To Reproduce Each of the models below end up with a separate history table. There should be a way for all three models to share the same historical table.

class Equipment(models.Model):
    equipment_type = models.IntegerField()
    name = models.TextField()
    properties = JSONField(null=True)
    history = HistoricalRecords(inherit=True)

class Pump(Equipment):
    class Meta():
        proxy = True

    @staticmethod
    def create_pump(name):
        return Pump(name=name, equipment_type=1, properties={})

   # various getter/setter for the properties, like gallons_per_minute

class Building(Equipment):
    class Meta():
        proxy = True

    @staticmethod
    def create_building(name):
        return Building(name=name, equipment_type=2)
   # getter/setter like number_of_floors

Expected behavior

There should be a way for these models to share the same historical table.

I suggest there be another argument to the register function to allow this.
It can't be automatic, since proxy models may have extra fields

Environment (please complete the following information):

  • Django Simple History Version: 2.7
  • Django Version: 1.11.8
  • Database Version: PostgreSQL 10.5.0

sknutsonsf avatar Apr 14 '19 01:04 sknutsonsf

Workaround (hack really) is to edit the migrations after they are created and before running, to add managed=False.

            options={
                'ordering': ('-history_date', '-history_id'),
                'db_table': 'equipment_historicalequipment',
                'verbose_name': 'historical sensor item',
                'get_latest_by': 'history_date',
                'managed': False
            },

Then apply the migration into all appropriate databases including production.

What makes it a hack is you need to edit the migration file AFTER it was applied remove the managed clause so "makemigrations" won't see any changes the next time you run it. (and forget about tests working on a freshly created db)

sknutsonsf avatar Apr 14 '19 02:04 sknutsonsf

What if you make inherit=False?

rossmechanic avatar Apr 15 '19 13:04 rossmechanic

@sknutsonsf any update?

rossmechanic avatar Apr 23 '19 13:04 rossmechanic

I was not able to work around the need to edit the generated migration. I looked briefly into the code and it would take at least 4 or 5 hours to fix the code to add an option to have the migration generated correctly. The workaround is doing just great, we've been able to make later migrations without any issue.

sknutsonsf avatar Apr 25 '19 22:04 sknutsonsf

I had the same problem recently and thanks to @sknutsonsf found a workaround that doesn't require editing the migration after applying it and doesn't mess with tests either.

On the main model (Equipment in OPs example), provide the table_name argument so that it looks like this

history = HistoricalRecords(inherit=True, table_name='app_name_table_name')

You have to set the table name to the complete name of the table in the DB so something like myapp_mytablename. Now generate migrations. This will generate an AlterModelTable migration for your main model, and a migration with CreateModel for your proxy.

Edit the proxy's migration to have 'managed': False like @sknutsonsf suggests and then apply your migrations. You'll see that no new tables are created.

Now run makemigrations again and you'll see another migration created for the proxy's historical table apply this one as well as it's not changing anything, simply applying Meta options to the unmanaged historical table.

That's it, you should now be able to save your proxy model instances and have the history inserted into the correct table.

@rossmechanic If this solution is usable enough, it would be nice to add to the Common Issues section of the docs.

ckm2k1 avatar Jul 18 '19 16:07 ckm2k1

By overriding get_meta_options(), an inherited HistoricalRecords' Meta class can be set manged = False without modifying a migration file manually.

class ProxyAwareHistoricalRecords(HistoricalRecords):
    def get_meta_options(self, model, *args, **kwargs):
        meta_options = super().get_meta_options(model, *args, **kwargs)
        if model._meta.proxy:
            meta_options['managed'] = False
        return meta_options

class Parent(models.Model):
    history = ProxyAwareHistoricalRecords(inherit=True, table_name='history_table')

class ProxyParent(Parent):
    class Meta:
        proxy = True

Because the django Admin doesn't support register the same model twice with different options, proxy models are used for the workaround of this problem. 1 In this case, I'd share the history table between the proxy models.

pjxiao avatar Feb 25 '20 05:02 pjxiao

I fix this issue in my project with making history of proxy models as new proxy models.

Here is my implementation:

from django.apps import apps
from django.utils.encoding import python_2_unicode_compatible
from simple_history.models import HistoricalRecords, registered_models


class MyHistoricalRecords(HistoricalRecords):
    def create_history_model(self, model, inherited):
        # For proxy models; historymodel can be proxy too! :D, I change history model to create a simple model that
        # extends it's parent history and meta proxy is True
        base_history = None
        if model._meta.proxy:
            for parent_class in model._meta.parents.keys():
                if hasattr(parent_class, 'history'):
                    base_history = parent_class.history.model
        if base_history:
            attrs = {
                "__module__": self.module,
                "_history_excluded_fields": self.excluded_fields,
            }
            app_module = "%s.models" % model._meta.app_label
            if inherited:
                # inherited use models module
                attrs["__module__"] = model.__module__
            elif model.__module__ != self.module:
                # registered under different app
                attrs["__module__"] = self.module
            elif app_module != self.module:
                # Abuse an internal API because the app registry is loading.
                app = apps.app_configs[model._meta.app_label]
                models_module = app.name
                attrs["__module__"] = models_module

            attrs.update(Meta=type(str("Meta"), (), self.get_meta_options(model)))
            name = self.get_history_model_name(model)

            registered_models[model._meta.db_table] = model
            return python_2_unicode_compatible(type(str(name), (base_history, ), attrs))
        return super().create_history_model(model, inherited)

    def get_meta_options(self, model, *args, **kwargs):
        meta_options = super().get_meta_options(model, *args, **kwargs)
        if model._meta.proxy:
            meta_options['proxy'] = True
        return meta_options

# models.py
class Parent(models.Model):
    history = MyHistoricalRecords(inherit=True)

class Child(Parent):
    class Meta:
        proxy = True

ATofighi avatar Sep 24 '20 22:09 ATofighi

@ATofighi & @pjxiao I am noticing that the management command clean_duplicate_history --auto does not seem to pick up on models using MyHistoricalRecords or ProxyAwareHistoricalRecords. Have you noticed the same? Any thoughts on how to resolve this?

JackAtOmenApps avatar Jun 01 '21 16:06 JackAtOmenApps

I'm afraid that I'm no longer working with django, so I have no idea. Sorry.

pjxiao avatar Jun 01 '21 23:06 pjxiao

class Parent(models.Model):
    history = HistoricalRecords()

class Child(Parent):
    class Meta:
        proxy = True

    def save(self, *args, **kwargs):
        self.__class__ = Parent
        super(Parent, self).save(*args, **kwargs)
        self.__class__ = Child

NikolayCherniy avatar Jun 23 '22 20:06 NikolayCherniy

This is my solution so far, a combination of the snippets in this thread. Big thanks and credit to everyone:

class ProxyAwareHistoricalRecords(HistoricalRecords):
    # inspired by https://github.com/jazzband/django-simple-history/issues/544
    def _find_base_history(self, opts):
        base_history = None
        for parent_class in opts.parents.keys():
            if hasattr(parent_class, 'history'):
                base_history = parent_class.history.model
        return base_history

    def create_history_model(self, model, inherited):
        opts = model._meta
        if opts.proxy:
            base_history = self._find_base_history(opts)
            if base_history:
                return self.create_proxy_history_model(model, inherited, base_history)

        return super().create_history_model(model, inherited)

    def create_proxy_history_model(self, model, inherited, base_history):
        opts = model._meta
        attrs = {
            '__module__': self.module,
            '_history_excluded_fields': self.excluded_fields,
        }
        app_module = f'{opts.app_label}.models'
        if inherited:
            attrs['__module__'] = model.__module__
        elif model.__module__ != self.module:
            # registered under different app
            attrs['__module__'] = self.module
        elif app_module != self.module:
            # Abuse an internal API because the app registry is loading.
            app = apps.app_configs[opts.app_label]
            models_module = app.name
            attrs['__module__'] = models_module

        attrs.update(
            Meta=type(
                'Meta',
                (),
                {**self.get_meta_options(model), 'proxy': True}
            )
        )
        if self.table_name is not None:
            attrs['Meta'].db_table = self.table_name

        name = self.get_history_model_name(model)
        registered_models[opts.db_table] = model
        return type(str(name), (base_history,), attrs)

If there is interest, I'll open a pull request so the library can natively support this.

keyvanm avatar May 08 '23 15:05 keyvanm

@keyvanm that worked for us! I just had to remember to set inherit=True on the history field, and now we can track history on the child proxy models.

One odd thing, however, is that it produces a no-op migration for each child proxy model to attempt to create a historical table with no fields, resulting in a migration that looks like BEGIN; COMMIT;. No harm from that, though, since full history is tracked in the parent concrete model.

pwnage101 avatar Jul 12 '23 22:07 pwnage101

The solution seems not work for historical field with m2m fields, it will raise below on starting my project. RuntimeError: Conflicting 'historicalmaterial_icp_attachment' models in application 'materials': <class 'srm.materials.models.HistoricalMaterial_icp_attachment'> and <class 'srm.materials.models.material.HistoricalMaterial_icp_attachment'>.


  history = ProxyAwareHistoricalRecords(
      user_db_constraint=False,
      table_name='materials"."material_warehouse_history',
      m2m_fields=[icp_attachment, msds_attachment, other_attachment],
      inherit=True,
  )

anyidea avatar Mar 25 '24 02:03 anyidea