citusdata / django-multitenant

Python/Django support for distributed multi-tenant databases like Postgres+Citus
MIT License
710 stars 116 forks source link

__set_attr__ of TenantModelMixin breaks migrations #99

Closed s4ke closed 1 year ago

s4ke commented 3 years ago

Hey,

With the current version of django-multitenant, we are having problems with using classes with the mixin in migrations. Basically, the issue is that __set_attr__ does not play well with objects that are resolved like this:

    Model = apps.get_model('model', 'Model')
    for _model in Model.all_objects.all():
        ...

The reason for this is that Django resolves the classes one by one and when resolving the TenantModelMixin it calls __set_attr__ differently than in normal mode. Then self.tenant_field somehow triggers a Django reload from database which that then makes everything crash:

File "/<snip>/venv/lib/python3.8/site-packages/django/db/models/query.py", line 287, in __iter__
    self._fetch_all()
  File "/<snip>/venv/lib/python3.8/site-packages/django/db/models/query.py", line 1308, in _fetch_all
    self._result_cache = list(self._iterable_class(self))
  File "/<snip>/venv/lib/python3.8/site-packages/django/db/models/query.py", line 71, in __iter__
    obj = model_cls.from_db(db, init_list, row[model_fields_start:model_fields_end])
  File "/<snip>/venv/lib/python3.8/site-packages/django/db/models/base.py", line 513, in from_db
    new = cls(*values)
  File "/<snip>/venv/lib/python3.8/site-packages/django_multitenant/mixins.py", line 58, in __init__
    super(TenantModelMixin, self).__init__(*args, **kwargs)
  File "/<snip>/venv/lib/python3.8/site-packages/django/db/models/base.py", line 416, in __init__
    self._state = ModelState()
  File "/<snip>/venv/lib/python3.8/site-packages/django_multitenant/mixins.py", line 62, in __setattr__
    if (attrname in (self.tenant_field, get_tenant_field(self).name)
  File "/<snip>/venv/lib/python3.8/site-packages/django_multitenant/mixins.py", line 115, in tenant_field
    return self.tenant_id
  File "/<snip>/venv/lib/python3.8/site-packages/django/db/models/query_utils.py", line 149, in __get__
    instance.refresh_from_db(fields=[field_name])
  File "/<snip>/venv/lib/python3.8/site-packages/django/db/models/base.py", line 623, in refresh_from_db
    db_instance_qs = self.__class__._base_manager.db_manager(using, hints=hints).filter(pk=self.pk)
  File "/<snip>/venv/lib/python3.8/site-packages/django/db/models/base.py", line 573, in _get_pk_val
    return getattr(self, meta.pk.attname)
  File "/<snip>/venv/lib/python3.8/site-packages/django/db/models/query_utils.py", line 147, in __get__
    val = self._check_parent_chain(instance)
  File "/<snip>/venv/lib/python3.8/site-packages/django/db/models/query_utils.py", line 163, in _check_parent_chain
    return getattr(instance, link_field.attname)
AttributeError: 'NoneType' object has no attribute 'attname'

To reproduce, you can simply create a migration and try to use the model either via all_objects or via any constructing method (e.g. Model.objects.create).

My workaround for now is to run this code at the top of my urls.py:

def monkey_patch_multitenant() -> None:

    def __setattr__(self: Any, attrname: Any, val: Any) -> Any:
        if (
            attrname in ('tenant_id', 'tenant')
            and not self._state.adding
            and val
            and self.tenant_value
            and val != self.tenant_value
            and val != self.tenant_object
        ):
            self._try_update_tenant = True

        return super(TenantModelMixin, self).__setattr__(attrname, val)

    TenantModelMixin.__setattr__ = __setattr__

    @property
    def tenant_field(self) -> Any:
        return 'tenant_id'

    TenantModelMixin.tenant_field = tenant_field

    @property
    def tenant_value(self) -> Any:
        return self.tenant_id

    TenantModelMixin.tenant_value = tenant_value

    @property
    def tenant_object(self) -> Any:
        return self.tenant

    TenantModelMixin.tenant_object = tenant_object

This is obviously not the fix for everyone, but i think illustrates the issue rather nicely.

s4ke commented 3 years ago

This concerns Django 3.1 compatibility.

quinceleaf commented 3 years ago

Running into the same issue when trying to add django_multitenant to project.

Only spent a few minutes with @s4ke's workaround, haven't gotten to work yet.

For clarification, @s4ke, run your workaround in your urls.py (root level? app level? haven't had luck with either location)

s4ke commented 3 years ago

@quinceleaf I added it in urls.py

s4ke commented 3 years ago

But in the end it varies on your import order. You have to run this as soon as possible so that models are not initialized yet.

Keekteng commented 1 year ago

@quinceleaf Did u manage to solve this issue? Tried the solution by s4ke but doesnt seem to work for me either.

gurkanindibay commented 1 year ago

@s4ke @Keekteng @quinceleaf started working on the issue. However, as a workaraound, you can check this issue https://github.com/citusdata/django-multitenant/issues/138

gurkanindibay commented 1 year ago

I added a test which shows this issue does not exist any more. PR that tests this case https://github.com/citusdata/django-multitenant/pull/152