citusdata / django-multitenant

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

Issue with model inheritance and deletion: Can only delete from one table at a time #101

Open llostris opened 3 years ago

llostris commented 3 years ago

Hi! First off, thank you for building this library, we were quite excited to get started using it, however, we've run into an issue.

Issue descripion We have our models structured like this:

class Tenant(TenantModelMixin):
    name = models.CharField(max_length=40)
    tenant_id = 'id'

    objects = TenantManager()

class Item(TenantModel):
    name = models.CharField(max_length=255)
    tenant = TenantForeignKey(Tenant, on_delete=models.PROTECT)
    tenant_id = 'tenant_id'

class SideItem(Item):
     overlay_photo = models.ForeignKey(Photo, on_delete=models.PROTECT)

SideItem's primary key is item_ptr_id, which is the primary key of Item model. Due to how the models are structured, tenant field for SideItem exists in Item's table.

Everything works fine until we try to delete a SideItem (one, or many, or all – that is irrelevant):

SideItem.objects.all().delete()

We get the following error:


  File "/Users/llostris/Code/web/app/hubs/tests/test_bags_for.py", line 1090, in test_repeated_calls
    Item.objects.all().delete()
  File "/Users/llostris/Virtualenvs/app/lib/python3.7/site-packages/django/db/models/query.py", line 722, in delete
    deleted, _rows_count = collector.delete()
  File "/Users/llostris/Virtualenvs/app/lib/python3.7/site-packages/django_multitenant/query.py", line 77, in delete
    return base_delete(obj)
  File "/Users/llostris/Virtualenvs/app/lib/python3.7/site-packages/django/db/models/deletion.py", line 332, in delete
    count = query.delete_batch(pk_list, self.using)
  File "/Users/llostris/Virtualenvs/app/lib/python3.7/site-packages/django/db/models/sql/subqueries.py", line 41, in delete_batch
    num_deleted += self.do_query(self.get_meta().db_table, self.where, using=using)
  File "/Users/llostris/Virtualenvs/app/lib/python3.7/site-packages/django/db/models/sql/subqueries.py", line 24, in do_query
    cursor = self.get_compiler(using).execute_sql(CURSOR)
  File "/Users/llostris/Virtualenvs/app/lib/python3.7/site-packages/django/db/models/sql/compiler.py", line 1144, in execute_sql
    sql, params = self.as_sql()
  File "/Users/llostris/Virtualenvs/app/lib/python3.7/site-packages/django/db/models/sql/compiler.py", line 1430, in as_sql
    "Can only delete from one table at a time."
AssertionError: Can only delete from one table at a time.

Libraries used

Question: Is this a bug, or is that type of model inheritance simply not supported with django-multitenant?

Or maybe there is a specific way to use the library with models structured like this, and we've just set things up incorrectly?

maziar-dandc commented 3 years ago

Hey @llostris were you able to workaround this issue?

llostris commented 3 years ago

Hi @maziar-dandc, sorry for the late reply!

We've noticed this issue is resolved on Django 3.1 for us, as the problematic code in Django was updated :) That's probably the safest solution.

However, at the time we were still on Django 3.0, and we managed to build a workaround as follows:

from django.db.models.deletion import Collector
from django.db.models.sql import DeleteQuery
from django_multitenant.deletion import related_objects
from django_multitenant.models import TenantModel
from django_multitenant.query import add_tenant_filters_on_query, wrap_delete

def is_model_using_multi_table_inheritance(model):
    """
    Add your models that are inheriting from another model/table here.
    """
    # List your model classes below
    models_using_multi_table_inheritance = [MyModel]
    return model in models_using_multi_table_inheritance

def wrap_get_compiler(base_get_compiler):
    """
    This is a copy-paste of django-multitenant's wrap_get_compiler class, with a small adjustment:
    it won't inject tenant filter queries if a model we're deleting is using multi-table inheritance
    and it would always trigger "Can only delete from one table at a time" error.
    """

    def get_compiler(obj, *args, **kwargs):
        if not is_model_using_multi_table_inheritance(obj.model):
            add_tenant_filters_on_query(obj)
        return base_get_compiler(obj, *args, **kwargs)

    get_compiler._sign = "get_compiler django-multitenant"
    return get_compiler

class ECTenantModel(TenantModel):
    """
    TenantModel class which overrides TenantModelMixin's __init__ method, to use our custom version
    of wrap_get_compiler.

    For this to work, you need to use ECTenantModel class *for all models* that had previously inherited from
    TenantModel!
    """

    class Meta:
        abstract = True

    def __init__(self, *args, **kwargs):
        if not hasattr(DeleteQuery.get_compiler, "_sign"):
            DeleteQuery.get_compiler = wrap_get_compiler(DeleteQuery.get_compiler)
            Collector.related_objects = related_objects
            Collector.delete = wrap_delete(Collector.delete)

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

So, long story short: we're not adding django-multitenant's tenant filters if the DELETE query is happening on the select few models that are using the multi-table inheritance.

Disclaimer: we were confident this is a good enough solution for us after inspecting generated DELETE queries with django-multitenant and without, and taking into account the fact our database is not distributed. This might not be a good solution in other cases.

Hope that helps!

maziar-dandc commented 3 years ago

Hi @llostris Thanks for getting back to me, really appeciate it!

DB I'm dealing with is distributed so the workaround you posted (thanks for that!) wouldn't be useful, I kept the tables separated and added a TenantOneToOne field on one of them to link back to main table with the generic fields, only tough part was when you want to create a new model instance which I handled with a custom model manager:

class UniversalPluginManager(models.Manager, TenantManagerMixin):
    def create(self, *args, **kwargs):
        from apps.cms.models import Plugin

        with transaction.atomic():
            base_plugin = Plugin(
                placeholder=kwargs.pop('placeholder'),
                position=kwargs.pop('position'),
                language=kwargs.pop('language'),
                plugin_type=kwargs.pop('plugin_type'),
            )
            base_plugin.save()

            actual_plugin = self.model(
                cms_plugin_ptr=base_plugin,
                **kwargs
            )
            actual_plugin.save()

            return actual_plugin

Hope that helps someone out there. Really sad that multi-table-inheritance doesn't work out-of-the-box.

UPDATE: use https://github.com/craigds/django-typed-models instead of my approach, results in way cleaner code.