Historical migrations break when models are removed

[ledburyb]

Due to https://github.com/citusdata/django-multitenant/blob/master/django_multitenant/backends/postgresql/base.py#L79 the migrations are dependent on the current state of the application, not on the state that it was at when the migration was created.

This means that if you remove a model your migrations will no longer be able to run cleanly from start to finish and will instead hit the error from https://github.com/citusdata/django-multitenant/blob/master/django_multitenant/backends/postgresql/base.py#L85

[ledburyb]

@louiseGrandjonc - This is working perfectly for me, thanks! However, is there any chance of adding something for #17? I have a local implementation working but it doesn't yet include the Citus specific SQL and I keep having to update to it match django-multitenant changes.

[louiseGrandjonc]

Hi! Great that it works and sorry about the trouble of multiple releases! We do have this ticket for a while, we have had a hard time making it a priority because it raises other issues. Like the fact that it means that we would maybe have to change the Django admin to handle the case where id 1 is duplicated in tenant x and y. I haven’t looked too much into this. Including a automated creation of composite primary keys in the database should be easy, it’s more the rest of the ecosystem that I wanted to think about before releasing this. However I am curious on seeing your code that you implemented for this. Do you mind sharing it ? Louise

…

Le 18 mai 2019 à 02:37, Ben Ledbury ***@***.***> a écrit : @louiseGrandjonc - This is working perfectly for me, thanks! However, is there any chance of adding something for #17? I have a local implementation working but it doesn't yet include the Citus specific SQL and I keep having to update to it match django-multitenant changes. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[ledburyb]

Hi Lousie, I'm happy to share my solution. I did actually already share some of it on the other ticket from a different account.

Extended postgresql backend

from django_multitenant.backends.postgresql.base import (
    DatabaseWrapper as PostgresqlDatabaseWrapper,
    DatabaseSchemaEditor as PostgresqlDatabaseSchemaEditor,
)
from machinelabs.common.django.fields import TenantPrimaryKey


class DatabaseSchemaEditor(PostgresqlDatabaseSchemaEditor):
    # Override
    def column_sql(self, model, field, include_default=False):
        if isinstance(field, TenantPrimaryKey):
            field.primary_key = False
        sql, params = super().column_sql(model, field, include_default)
        return sql, params

    # Override
    def create_model(self, model):
        super().create_model(model)
        for field in model._meta.local_fields:
            if isinstance(field, TenantPrimaryKey):
                tenant_column = self.quote_name(field.tenant_id)
                table_name = model._meta.db_table
                constraint_name = self.quote_name(f'{table_name}_pkey')
                quoted_table_name = self.quote_name(table_name)
                self.deferred_sql.extend([
                    f"""
                    ---
                    --- Add composite primary key to {table_name}
                    ---
                    ALTER TABLE {quoted_table_name}
                    ADD CONSTRAINT {constraint_name}
                    PRIMARY KEY ({tenant_column}, id)
                    """
                ])


class DatabaseWrapper(PostgresqlDatabaseWrapper):
    # Override
    SchemaEditorClass = DatabaseSchemaEditor

Primary key class

from django.db import models


class TenantPrimaryKey(models.AutoField):
    def __init__(self, *args, **kwargs):
        kwargs['primary_key'] = True
        self.tenant_id = kwargs.pop('tenant_id', None)
        super().__init__(*args, **kwargs)

    def deconstruct(self):
        name, path, args, kwargs = super().deconstruct()
        kwargs['tenant_id'] = self.tenant_id
        return name, path, args, kwargs

Example model

class BrandModel(TenantModel):
    id = TenantPrimaryKey(tenant_id='brand_id')
    brand = models.ForeignKey('Brand', on_delete=models.CASCADE, related_name="%(class)ss")
    tenant_id = 'brand_id'

    class Meta:
        abstract = True

The fiddling with the primary_key is because Django really doesn't like you trying to create a model without a primary key.

Also, I've not tested it but I think the solution for deleted models you've just implemented might fail with the following not-uncommon scenario:

1. Create TenantModel ModelOne
2. Create TenantModel ModelTwo
3. Add a custom migration to copy data from ModelOne to ModelTwo
4. Delete ModelOne

If you instead required that each TenantModel had to have one and only one TenantPrimaryKey then the tenant_ids needed should always be stored against the fields involved in the migration and you could drop the reliance on apps.get_models() entirely.

An example implementation of this, tested only briefly but seems to work:

def get_tenant_id(model):
    for field in model._meta.fields:
        if isinstance(field, TenantPrimaryKey):
            return field.tenant_id
    raise ValueError(f"{model} has no TenantPrimaryKey")

class DatabaseSchemaEditor(PostgresqlDatabaseSchemaEditor):
    def _create_fk_sql(self, model, field, suffix):
        if isinstance(field, TenantForeignKey):
            from_columns = field.column, get_tenant_id(model)
            to_columns = field.target_field.column, get_tenant_id(field.target_field.model)
            suffix = suffix % {
                "to_table": field.target_field.model._meta.db_table,
                "to_column": '_'.join(to_columns),
            }

            return self.sql_create_fk % {
                "table": self.quote_name(model._meta.db_table),
                "name": self.quote_name(self._create_index_name(model, from_columns, suffix=suffix)),
                "column": ', '.join([self.quote_name(from_col) for from_col in from_columns]),
                "to_table": self.quote_name(field.target_field.model._meta.db_table),
                "to_column": ', '.join([self.quote_name(to_col) for to_col in to_columns]),
                "deferrable": self.connection.ops.deferrable_sql(),
            }
        return super(PostgresqlDatabaseSchemaEditor, self)._create_fk_sql(model, field, suffix)

If you have concerns about compatibility for other people using the library perhaps you could just bind some of these features behind settings flags so that people who do need them can enable them. I was also hoping you'd be able to add a flag which determines whether or not the SELECT create_distributed_table Citus SQL commands are added to the migrations as well as this would both keep compatibility for people not, or not yet, using Citus but also make migrations less manual if/once they are.

As for the Django admin problem I'm not sure that's something this library can easily solve. it's been an open Django issue for 14 years with no resolution (https://code.djangoproject.com/ticket/373). Thankfully I'm not actually using the admin for anything so it's not a problem for me. The best I can think is that you might be able to have siloed admins based on the tenant id and have multiple admin URLs such as /admin/1/, /admin/2/ and a corresponding middleware to call set_current_tenant.