citusdata / django-multitenant

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

If Tenant reference is named tenant, tenant_id collides #98

Closed s4ke closed 1 year ago

s4ke commented 3 years ago

We have models that have a tenant field set as tenant = .... and django therefore generates tenant_id as part of the django model. This collides with the usage of self.tenant_id.

To work around this, we have to manually override

    @property
    def tenant_field(self):
        return 'tenant_id'
s4ke commented 3 years ago

This should either be documented as a workaround, or we should check if maybe accessing tenant_id via self.__class__.tenant_id works instead.

s4ke commented 3 years ago

With the workaround in #99 in mind I don't think the proposed solution works. I think for our usecase will need to create custom Mixins?

KrYpTeD974 commented 1 year ago

This workaround works really well, I've tested it on a big project.

This is the base class of every tenant related models we have.

class TenantAwareModel(TenantModel):
    tenant = models.ForeignKey(Tenant, on_delete=models.CASCADE)

    @property
    def tenant_field(self):
        return 'tenant_id'

    class Meta:
        abstract = True

Big thank you to @s4ke ! And +1 about adding this workaround in the documentation.

KrYpTeD974 commented 1 year ago

Huum Ok actually this does not completely work.

I have some weird cases where there's a MaximumRecursionError that is triggered when tenant_value gets called. This is what the program does:

  1. Get the tenant value
  2. Try to read the value from the cache
  3. Value does not exists
  4. Set the value
  5. Go into set_attr of TenantModelMixin
  6. Get the tenant_value (step 1)
  7. [loop again and again]

Currently I did not find a better solution than monkey_patching the __set_attr__ method to remove it

def monkey_patch_tenant_model_mixin_setattr():
    from django_multitenant.mixins import TenantModelMixin
    from django.db.models import Model
    TenantModelMixin.__setattr__ = Model.__setattr__

In settings.py

from tenants.utils import monkey_patch_tenant_model_mixin_setattr
monkey_patch_tenant_model_mixin_setattr()

Just make to avoid modifying the tenant_id of an existing object.

Maybe there is a better way in django to detect that the tenant field has changed ?

gurkanindibay commented 1 year ago

For now, please change the tenant_id columns with different name. tenant_id is kind of reserved keyword. I will try to find a way to remove this constraint soon

gurkanindibay commented 1 year ago

@s4ke @KrYpTeD974 Fixed with the pr https://github.com/citusdata/django-multitenant/pull/151