bennylope / django-organizations

:couple: Multi-user accounts for Django projects
http://django-organizations.readthedocs.org/
BSD 2-Clause "Simplified" License
1.31k stars 212 forks source link

Custom `AUTH_USER_MODEL` Creates Migration Circular Reference? #215

Open vector-kerr opened 3 years ago

vector-kerr commented 3 years ago

Greetings!

I'm having a go at using django-organizations, and I've been getting a circular reference error when attempting to perform migrations, which I believe I can trace back to django-organizations and using a custom AUTH_USER_MODEL.

I say 'I believe' because I'm not 100% sure that I haven't done something incorrectly, but having read the documentation on using a custom user model and proxy models, I think I've covered myself...

I've included a set of steps to reproduce below which should hopefully demonstrate that either I'm doing something woefully wrong, or there's something weird going on with custom user models.

If you need any more information, or if I've stupidly broken it somehow, please let me know. :)

Thanks for your help and your time! - Vector

Steps to reproduce

  1. Create and activate a virtual environment:
    python3 -m venv .venv
    source .venv/bin/activate
  2. Install the following python requirements:
    appdirs==1.4.4
    asgiref==3.3.4
    distlib==0.3.1
    Django==3.2
    django-extensions==3.1.3
    django-organizations==2.0.0
    filelock==3.0.12
    pytz==2021.1
    six==1.15.0
    sqlparse==0.4.1
    virtualenv==20.4.2
  3. Create a new django project (I've called mine demo) and enter the directory:
    django-admin startproject demo
    cd demo
  4. Create an app called core which will be used for models and migrations
    python3 manage.py startapp core
  5. Put the following content into core/models.py:

    from django.db import models
    from django.contrib.auth.models import AbstractUser
    from organizations.models import Organization
    from organizations.models import OrganizationUser
    
    class User(AbstractUser):
       created_at = models.DateTimeField(auto_now_add=True, editable=False)
       updated_at = models.DateTimeField(auto_now_add=True)
       def __str__(self):
           return self.username
    
    class Tenant(Organization):
       class Meta:
           proxy = True
    
    class TenantUser(OrganizationUser):
       class Meta:
           proxy = True
  6. Update demo/settings.py to include the organizations and core apps, and set the custom user model:

    INSTALLED_APPS = [
       # ...
       'organizations',
       'core',
    ]
    
    AUTH_USER_MODEL = 'core.User'
  7. Create the migrations for the project:
    python3 manage.py makemigrations

    At this point, I get the first bit of weird output - a migration is created against the organizations module in the virtual environment:

    Migrations for 'organizations':
     .venv/lib/python3.9/site-packages/organizations/migrations/0005_auto_20210430_0635.py
       - Alter field id on organization
       - Alter field id on organizationinvitation
       - Alter field id on organizationowner
       - Alter field id on organizationuser
    Migrations for 'core':
     core/migrations/0001_initial.py
       - Create proxy model Tenant
       - Create proxy model TenantUser
       - Create model User
  8. Finally, attempt to create the migrations again. This time, I receive a circular reference error, which links back to the previously created migration:
    python3 manage.py makemigrations
    Traceback (most recent call last):
    [...snip...]
    django.db.migrations.exceptions.CircularDependencyError: core.0001_initial, organizations.0001_initial, organizations.0002_model_update, organizations.0003_field_fix_and_editable, organizations.0004_organizationinvitation, organizations.0005_auto_20210430_0635
iamprakashom commented 2 years ago

Were you able to resolve this?

vector-kerr commented 2 years ago

Unfortunately not, sorry @iamprakashom .

robshep commented 2 years ago

1. Circular Dependency Issue

I found this occurs when BOTH the initial user account migration AND the django-organizations migrations are being generated at the same time.

We found ourselves in this scenario this after doing some refactoring before our first production release (we hosed all the migrations with the intention of just re-creating all migrations from scratch (after changing a few module names etc)) The solution for us was to comment out all mention of django-organizations, then run makemigrations so it only picks up the custom auth user model (and any other unrelated) ... THEN uncomment organizations back in and run makemigrations again.

2. weird output - a migration is created against the organizations module in the virtual environment

Yes, we nearly missed this before release to staging etc. We don't commit the virtual environment to source control - as is typical, but do run makemigrations as part of the commit cycle so the migrations are under source control.
But in doing so, these migrations are lost between DEV and other environments if makemigrations is not run.

As I understand it....

Option A. It's possible to override migration lookups for django apps by configuring:

MIGRATION_MODULES = {
    'organizations': 'my_app.extra_migrations.organizations'
}

BUT then you have to copy the original ones into here, before running makemigrations BUT when you upgrade django-organizations and any new migrations comes in - for a fix or feature - then you'll miss these unless you look out for them and copy new ones over manually

EDIT: to keep on top of this, we have a small script inside my_app.extra_migrations.organizations.__init__.py

import pkgutil
from organizations import migrations as vendor_migrations

local_migrations = [x.name for x in pkgutil.iter_modules(__path__)]

for vendor_migration in pkgutil.iter_modules(vendor_migrations.__path__):
    if vendor_migration.name not in local_migrations:
        print("WARNING !! django-organizations has new migrations: ", vendor_migration.name)

Option B. commit .venv into source control.

Neither option seems brilliant.

good luck

gfclabs commented 2 years ago

Thanks for this. Splitting up into two sets of migrations worked well. Luckily commenting all the stuff out was a viable option for me. Don't know what I would have done if had had a bunch of code to isolate.

pfcodes commented 1 year ago

This should really be fixed

bennylope commented 1 year ago

This should really be fixed

@pfcodes would you mind rewording that in the active voice?

The folks on this issue thread have put work into describing the issue they've encountered. I for one would heartily welcome a pull request to resolve this issue, and if you'd like to put in the work it will be most welcome!

bennylope commented 1 year ago

Following the exact example by @vector-kerr above reproduces the exact issue as described.

  1. The initial migration in core depends on the latest migration from organizations
  2. Subsequent run of makemigrations fails with circular dependency issues

Amending the example to instead include the custom user model in a separate users app - i.e. defining the custom user model in a separate app from the organizations - resolves the problem entirely.

  1. The initial migraiton in core depends on the latest migration from organizations
  2. The initial migration in users depends on the latest migration from auth
  3. No other migrates are created
  4. Subsequent runs of makemigrations simply report that there are no changes detected

It's hard to know if the example is just contrived for simplicity (which is completely reasonable) or representative of typical use. However given what I've seen here and what I've seen in the migrations, my current suspicion is that this is the case.

The initial organizations migration includes a swappable dependency on the AUTH_USER_MODEL and if that model is defined in the same app as models that in turn depend on on it then 💥 circular dependency.

I've never included custom users in the same app as custom organizations and so never even thought to ask if that was the production use case. If it is I would gently recommend against doing that, this issue notwithstanding. However if that is the common scenario then the solution here is going to be installation guidelines, because I do not think there is any code solution to the issue as described.

vector-kerr commented 1 year ago

Hey @bennylope, thank you for taking the time to look into this!

It's been quite some time since I encountered this issue, but from what I can recall, I was trying to stand up a basic use case, hence the all-in-one-app.

If it doesn't exist, some guidelines or a note to indicate that custom users should exist in a separate app would probably be more than sufficient to consider this done.

Thanks again!