Django: Prevent HistoricalModel Creation When Inheriting from Abstract Model

[xs2hamzashah]

I have an abstract base model, CompanyAwareModel, which includes historical tracking using TenantHistoricalRecords:

class CompanyAwareModel(RequestChangeReasonModelMixin, TenantModel, TimeStampedModel, metaclass=CompanyAwareMeta):
    tenant_id = 'company_id'
    company = models.ForeignKey(Company, on_delete=models.CASCADE)

    history = TenantHistoricalRecords(inherit=True)

    class Meta:
        abstract = True

To create a variant of this model without historical tracking, I defined another abstract model:

class NoHistoryCompanyAwareModel(CompanyAwareModel):
    history = None

    class Meta:
        abstract = True

Then, I define a concrete model inheriting from NoHistoryCompanyAwareModel:

class Counter(NoHistoryCompanyAwareModel):
    ...

However, when I run makemigrations, Django still creates a HistoricalCounter model, even though I explicitly set history = None in NoHistoryCompanyAwareModel.

Interestingly, if I define NoHistoryCompanyAwareModel without inheriting from CompanyAwareModel, like this:

Then makemigrations does not create a historical table for Counter.

Question:

Why does inheriting from CompanyAwareModel still create a historical table, even when history = None is explicitly set? And how can I ensure that Counter does not have a HistoricalCounter model while still inheriting from CompanyAwareModel?

Any insights would be greatly appreciated!

[KenWhitesell]

Welcome @xs2hamzashah !

We don’t have any way to determine whether there’s another one of these classes causing this model to be created.

I don’t think we could properly answer all your questions about the behavior of your model inheritance without seeing the full code for all the models involved, especially considering that you’re using a custom metaclass for the creation of these models and you’re using multiple base classes that aren’t shown here.

[xs2hamzashah]

okay @KenWhitesell Sir let me show it
these are the models involved in here

counter model

class Counter(NoHistoryCompanyAwareModel):
    company = models.ForeignKey(Company, on_delete=models.CASCADE, blank=True, null=True)
    key = models.CharField(max_length=50, unique=True)
    count = models.BigIntegerField(default=0)

class CompanyAwareMeta(ModelBase):
    def __init__(cls, *args, **kwargs):
        if not cls._meta.abstract:
            manager = getattr(cls, 'objects')
            if isinstance(manager, models.Manager) and not isinstance(manager, TenantManagerMixin):
                raise AttributeError("`objects` must be an instance of TenantManagerMixin")


class CompanyAwareModel(RequestChangeReasonModelMixin, TenantModel, TimeStampedModel, metaclass=CompanyAwareMeta):
    tenant_id = 'company_id'
    company = models.ForeignKey(Company, on_delete=models.CASCADE)

    history = TenantHistoricalRecords(inherit=True)

    class Meta:
        abstract = True


class NoHistoryCompanyAwareModel(CompanyAwareModel):
    history = None

    class Meta:
        abstract = True

class RequestChangeReasonModelMixin:
    __change_reason = None

    @property
    def _change_reason(self):
        """
        Use request.method and request.path_info as change reason by default
        requires simple_history.middleware.HistoryRequestMiddleware
        """

        if self.__change_reason:
            return self.__change_reason
        else:
            try:
                request = HistoricalRecords.thread.request
                change_reason = "{method} {path}".format(method=request.method, path=request.path_info)  # TODO: Consider request.get_full_path_info()
                return change_reason
            except AttributeError:
                pass

    @_change_reason.setter
    def _change_reason(self, value):
        self.__change_reason = value

class TenantModel(TenantModelMixin, models.Model):
    # Abstract model which all the models related to tenant inherit.

    objects = TenantManager()

    class Meta:
        abstract = True

class TimeStampedModel(models.Model):
    """
    An abstract base class model that provides self-updating
    ``created`` and ``modified`` fields.

    """
    created = AutoCreatedField(_('created'))
    modified = AutoLastModifiedField(_('modified'))

    def save(self, *args, **kwargs):
        """
        Overriding the save method in order to make sure that
        modified field is updated even if it is not given as
        a parameter to the update field argument.
        """
        update_fields = kwargs.get('update_fields', None)
        if update_fields:
            kwargs['update_fields'] = set(update_fields).union({'modified'})

        super().save(*args, **kwargs)

    class Meta:
        abstract = True

[KenWhitesell]

Very helpful, thanks.

I think we still need to see the class definition for TenantHistoricalRecords

[xs2hamzashah]

@KenWhitesell

@KenWhitesell

TenantHistoricalRecords

okay

from django.db import models
from django_multitenant.mixins import TenantModelMixin, TenantManagerMixin
from simple_history.manager import HistoryManager
from simple_history.models import HistoricalRecords, _default_get_user, _history_user_getter, _history_user_setter


class TenantHistoricalModelMixin(TenantModelMixin):
    tenant_id = 'company_id'


class TenantHistoryManager(HistoryManager, TenantManagerMixin):
    def get_super_queryset(self):
        return TenantManagerMixin.get_queryset(self)


class TenantHistoryDescriptor(object):
    def __init__(self, model):
        self.model = model

    def __get__(self, instance, owner):
        if instance is None:
            return TenantHistoryManager(self.model)
        return TenantHistoryManager(self.model, instance)


class TenantHistoricalRecords(HistoricalRecords):
    def __init__(
            self,
            verbose_name=None,
            bases=(TenantHistoricalModelMixin, models.Model),
            user_related_name="+",
            table_name=None,
            inherit=False,
            excluded_fields=None,
            history_id_field=None,
            history_change_reason_field=None,
            user_model=None,
            get_user=_default_get_user,
            cascade_delete_history=False,
            custom_model_name=None,
            app=None,
            history_user_id_field=None,
            history_user_getter=_history_user_getter,
            history_user_setter=_history_user_setter,
            related_name=None,
            use_base_model_db=False,
            user_db_constraint=True,
    ):
        super().__init__(
            verbose_name=verbose_name,
            bases=bases,
            user_related_name=user_related_name,
            table_name=table_name,
            inherit=inherit,
            excluded_fields=excluded_fields,
            history_id_field=history_id_field,
            history_change_reason_field=history_change_reason_field,
            user_model=user_model,
            get_user=get_user,
            cascade_delete_history=cascade_delete_history,
            custom_model_name=custom_model_name,
            app=app,
            history_user_id_field=history_user_id_field,
            history_user_getter=history_user_getter,
            history_user_setter=history_user_setter,
            related_name=related_name,
            use_base_model_db=use_base_model_db,
            user_db_constraint=user_db_constraint,
        )

    def finalize(self, sender, **kwargs):
        super().finalize(sender, **kwargs)
        descriptor = getattr(sender, self.manager_name, None)
        if descriptor:
            tenant_descriptor = TenantHistoryDescriptor(descriptor.model)
            setattr(sender, self.manager_name, tenant_descriptor)

[xs2hamzashah]

@KenWhitesell kindly please review my question

[KenWhitesell]

I have. Unfortunately, you’re getting into areas of the ORM that goes beyond my knowledge and experience.

I can make some conjectures, but ultimately, I’m not a person that can help you here.

<conjecture>
I would guess that the issue is that the act of parsing and creating the class for CompanyAwareModel, is creating an internal class for TenantHistoricalRecords, even though that field isn’t implemented in NoHistoryCompanyAwareModel.

From a more abstract perspective, I would say that your class hierarchy is improperly designed. I’ve always considered it a “bad code smell” when you try to remove attributes from a parent class. If it were me, I’d refactor the class structure such that the classes without a history are parent classes to those with - allowing you to create subclasses from either case.
Either that, or make the use of the history attribute a Mixin of its own.
</conjecture>

Take these thoughts for what they’re worth - probably not much.