citusdata / django-multitenant

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

Calls to TenantModel.delete() cause issues in Citus #15

Closed gsgalloway closed 6 years ago

gsgalloway commented 6 years ago

Example:

from django.db import models
from django_multitenant import set_current_tenant, TenantModel

class Corporation(models.Model):
    class Meta:
        db_table = 'corporation'
    pass

class Store(TenantModel):
    class Meta:
        db_table = 'store'
    corporation = models.ForeignKey(Corporation)
    isStoreOpen = models.BooleanField()
    tenant_id = 'corporation_id'

corporation = Corporation()
corporation.save()
store = Store(corporation=corporation, isStoreOpen=True)
store.save()

set_current_tenant(corporation)

Store.objects.filter(isStoreOpen=True).delete()

The SQL for the delete statement:

SELECT "store"."id",
       "store"."corporation_id",
       "store"."isStoreOpen"
FROM "store"
WHERE ("store"."corporation_id" = 2
       AND "store"."isStoreOpen" = TRUE);

DELETE
FROM "store"
WHERE "store"."id" IN (2);

ERROR:  multi-task query about to be executed

This one may be tough to fix, since the logic behind crafting DELETE queries seems to be outside of TenantQuerySet's control: https://github.com/django/django/blob/1.11.8/django/db/models/sql/subqueries.py#L48-L81

gsgalloway commented 6 years ago

The library django-compositekey uses monkeypatching to address this issue: https://github.com/simone/django-compositekey/blob/master/src/compositekey/db/models/sql/subqueries.py

gsgalloway commented 6 years ago

A working fix. Can publish as a PR if it seems reasonable

# models.py
from .patch import patch_delete_queries_to_include_tenant_ids

class TenantModel(models.Model):
    def __init__(self, *args, **kwargs):
        patch_delete_queries_to_include_tenant_ids()
        super(TenantModel, self).__init__(*args, **kwargs)

from .patch import activate_delete_monkey_patch
# patch.py
import logging
from django.db.models.sql.subqueries import DeleteQuery

from .utils import get_tenant_column, get_current_tenant

logger = logging.getLogger(__name__)

def wrap_get_compiler(original_get_compiler):

    def get_compiler(obj, *args, **kwargs):
        from .models import TenantModel
        if issubclass(obj.model, TenantModel):
            current_tenant = get_current_tenant()
            if current_tenant:
                tenant_column = get_tenant_column(obj.model)
                db_table = obj.model._meta.db_table
                extra_sql = ['{table}."{column}" = %s'.format(
                    table=db_table, column=tenant_column)]
                extra_params = [current_tenant.id]
                obj.add_extra([],[],extra_sql,extra_params,[],[])
        return original_get_compiler(obj, *args, **kwargs)

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

def patch_delete_queries_to_include_tenant_ids():
    '''
    Patches the `get_compiler` method of DeleteQuery to include an additional
    filter on tenant_id for any deletions that occur while a tenant is set.

    See https://github.com/citusdata/django-multitenant/issues/15 for a more
    detailed description.
    '''
    if not hasattr(DeleteQuery.delete_batch, "_sign"):
        logger.debug("Patching DeleteQuery.get_compiler()")
        DeleteQuery.get_compiler = wrap_get_compiler(DeleteQuery.get_compiler)
gsgalloway commented 6 years ago

Resultant sql for the same python example above

DELETE FROM "store"
WHERE ("store"."id" IN (9)
AND (store."corporation_id" = 7)
AND (store."corporation_id" = 7));
saicitus commented 6 years ago

Closing this issue because #16 fixes the issue.