bernardopires / django-tenant-schemas

Tenant support for Django using PostgreSQL schemas.
https://django-tenant-schemas.readthedocs.org/en/latest/
MIT License
1.45k stars 425 forks source link

Tenant-only app South migration gets faked on tenant creation #64

Closed humbled closed 11 years ago

humbled commented 11 years ago

I create my Shared schema initially:

bin/django sync_schemas --shared
bin/django migrate_schemas
...
from management.models import Client
tenant = Client(domain_url='localhost',
  schema_name='public',
  name='Schemas Inc.',
  on_trial=False)
tenant.save()

Then create a tenant:

from management.models import Client
tenant = Client(domain_url='tenant1.localhost',
  schema_name='tenant1',
  name='The Tenant1 Inc.',
  on_trial=False)
tenant.save()

This causes the middleware to create the new schema. But for my tenant-only app (fids_admin), the tables don't get created. Reason being the South migrations are faked:

=== Running migrate for schema: tenant1
Running migrations for fids_admin:
 - Migrating forwards to 0002_auto__add_field_flight_recurrences.
 > fids_admin:0001_initial
   (faked)
 > fids_admin:0002_auto__add_field_flight_recurrences
   (faked)

To work around this I delete all rows from tenantschema.south_migrationhistory then run:

bin/django migrate_schemas

Here's my apps config in settings:

SHARED_APPS = (

    'tenant_schemas',  # mandatory

    # everything below here is optional
    'django.contrib.auth',
    'django.contrib.contenttypes',
    'django.contrib.sessions',
    'django.contrib.messages',
    'django.contrib.admin',
    'django.contrib.staticfiles',

    'south',

    'management',
)

TENANT_APPS = (
    # The following Django contrib apps must be in TENANT_APPS
    'django.contrib.admin',
    'django.contrib.auth',
    'django.contrib.contenttypes',
    'django.contrib.staticfiles',

    'south',

    # your tenant-specific apps
    'fids_admin',
    'recurrence',
)

Why would the tenant creation fake the migration steps?

tenant-schemas v1.3.1 South v0.7.6

Thanks

bernardopires commented 11 years ago

Hi humbled,

here's why I fake the migrations when creating a tenant: it's slow! However, this does not mean the migrations are being ignored, it just means that when the tenant is created, the models are created with its latest state, so no migrations will be needed. That's why the option migrate_all is passed to syncdb. This is the equivalent of syncdb --migrate, which is basically already migrating the tables for you.

When you say no tables are being created, do you mean your tenant is completely empty or just specific tables weren't created?

humbled commented 11 years ago

Hi

Just the 2 tables that the migrations would create weren't created. I agree on second thoughts that they should have been created based on the current models. But they weren't. Here's the full output:

bin/django-interpreter dev_helpers/inserttenants2.py
=== Running syncdb for schema: tenant1
Syncing...
Creating tables ...
Creating table auth_permission
Creating table auth_group_permissions
Creating table auth_group
Creating table auth_user_groups
Creating table auth_user_user_permissions
Creating table auth_user
Creating table django_content_type
Creating table django_admin_log
Creating table south_migrationhistory
Creating table recurrence_recurrence
Creating table recurrence_rule
Creating table recurrence_date
Creating table recurrence_param
Installing custom SQL ...
Installing indexes ...
Installed 0 object(s) from 0 fixture(s)

Synced:
 > tenant_schemas
 > django.contrib.auth
 > django.contrib.contenttypes
 > django.contrib.sessions
 > django.contrib.messages
 > django.contrib.admin
 > django.contrib.staticfiles
 > south
 > management
 > fids_admin
 > recurrence

Not synced (use migrations):
 -
(use ./manage.py migrate to migrate these)
=== Running migrate for schema: tenant1
Running migrations for fids_admin:
 - Migrating forwards to 0002_auto__add_field_flight_recurrences.
 > fids_admin:0001_initial
   (faked)
 > fids_admin:0002_auto__add_field_flight_recurrences
   (faked)

I'll look into it closer shortly

Brad

humbled commented 11 years ago

Well I haven't come up with any discoveries. To me the key bit seems to be:

Synced:
 > tenant_schemas
 > django.contrib.auth
 > django.contrib.contenttypes
 > django.contrib.sessions
 > django.contrib.messages
 > django.contrib.admin
 > django.contrib.staticfiles
 > south
 > management
 > fids_admin
 > recurrence

Not synced (use migrations):
 -

Meaning that it thinks it has synced fids_admin already, given that it's in the list of synced, and the list of 'not synced' is empty. But I have 2 models and their tables are not created here. But they do exist in the South migrations, hence migrating forwards creating them.

The model classes are just standard, extending from django.db.models.base import Model, right?

Thanks Brad

kissgyorgy commented 11 years ago

Yes, you doesn't need to do anything else with the models, only the PostgreSQL search path is modified. But they extend django.db.models.Model, not django.db.models.base.Model. As I see, they are essentially the same right now, but if you want to stay compatible with future Django API I think you should extend them from db.Model.

humbled commented 11 years ago

Thanks @Walkman, that got me on the right path. The critical thing was to not import the class into the namespace. ie:

from django.db.models import Model
class Blah(Model):

DOES NOT WORK (tables don't get created during tenant creation)

from django.db import models
class Blah(models.Model)

works

kissgyorgy commented 11 years ago

This is strange. Good to know!

bernardopires commented 11 years ago

Wow, very odd... thanks Walkman for the help solving this app's issues/doubts!

DanielFerreiraJorge commented 10 years ago

Is there a way to not fake the migration? The problem is that I need to create an index on a json column using south's db.execute(), like this:

db.execute("CREATE UNIQUE INDEX index_name ON table ((column->'nest1'->'nest2'->'nest3'->>'data_to_index'));")

So, I add this line to the forwards method in the migration, and this is not executed when I'm creating the tenant... I have to manually downgrade and upgrade after creating the tenant...

Thanks!

bernardopires commented 10 years ago

Hmm... I see. Can your unique index only be created with a migration? Can't your django model define this unique index as well? When tenants are synced, they are synced to the last version (in theory equivalent to running all migrations), so this would allow you to avoid not faking a migration.

DanielFerreiraJorge commented 10 years ago

No, actually it cannot... indexing a native json data type in postgresql is new in 9.3, and I think django/south does not support this... This is not an index in a column... it is a index in the json "fields" inside a column... like this sample json below:

{ 'field1' : 'data', 'field2' : 'data', 'field3' : {'field31': 'data', 'field32' : 'data'}}

if I save this json document in a json type column in pg 9.3, I can create an index on "field32" for instance... I don't think django is prepared for that, since it is a very new feature of pg 9.3...

Is there a way to not fake the migration?

Thank you very much for your answer!

bernardopires commented 10 years ago

Thanks for explaining why it doesn't fit a normal django model! There is currently no way to do what you want. When a tenant is created, the following commands are ran:

# tenant_schemas/models.py
call_command('sync_schemas',
             schema_name=self.schema_name,
             tenant=True,
             public=False,
             interactive=False, 
             migrate_all=True,  # migrate all apps directly to last version
             verbosity=verbosity,
            )

call_command('migrate_schemas', 
             fake=True, # fake all migrations
             schema_name=self.schema_name, 
             verbosity=verbosity
            )

notice on sync_schemas the migrate_all=True. One option would be to turn this to False and removing fake=True from the second command. Would this solve your problem? We could maybe turn this into a django-tenant-models setting, what do you think?

DanielFerreiraJorge commented 10 years ago

I think it would be great to turn this into a setting. I just tested the "ugly" solution and it worked fine... It is strange how something so simple can take a whole afternoon... im fighting this for 5 hours, thinking that the sintax of the query is wrong, posting on stackoverflow, until I noticed the (faked) notice when creating a tenant... hahaha

bernardopires commented 10 years ago

Just added a new version with the new setting TENANT_CREATION_FAKES_MIGRATIONS, please let me know if it works as you needed it. Documentation has already been added. Thanks!