wildfish / django-gdpr-assist

Tools to help manage user data in the age of GDPR
Other
174 stars 16 forks source link

extra migrations for user model - keeps on adding migrations with manager #6

Closed tomaszputon closed 4 years ago

tomaszputon commented 5 years ago

The problem

Django keeps on adding extra migration for setting user manager for User class which inherits from AbstractUser.

The setup

In the examples below I work on MySQL (5.7.23), use Python 3.7.3 and mysqlclient==1.3.13.

How to replicate the problem:

  1. update settings:
INSTALLED_APPS = [
    # ...
    "gdpr_assist",
]

DATABASES = {
    "default": {
        "ENGINE": "django.db.backends.mysql",
        # ...
    },
    "gdpr_log": {
        "ENGINE": "django.db.backends.mysql",
        "NAME": "gdpr_log",
        "HOST": "127.0.0.1",
        "USER": "change_it",
        "PASSWORD": "change_it",
    },
}

DATABASE_ROUTERS = ["gdpr_assist.routers.EventLogRouter"]
  1. make changes to database:
python manage.py migrate
python manage.py migrate --database=gdpr_log

and:

python manage.py makemigrations

gives me 'No changes detected' - so far so good.

  1. add privacy settings for User class, which extends AbstractUser - I'm trying to make private only fields defined on AbstractUser, not my on User.
class User(AbstractUser):
    some_new_field_not_important_actually = models.BooleanField(default=False)

class UserPrivacyMeta:
    fields = [
        "first_name",
        "last_name",
    ]

from gdpr_assist import register
register(User, UserPrivacyMeta)
  1. make migrations:
    python manage.py makemigrations

    gives me:

Migrations for 'users': project/users/migrations/0058_auto_20190716_1308.py

  • Change managers on user
    • Add field anonymised to user

The interesting thing for me to note was that the manager seems to have changed.

  1. migrate:

    python manage.py migrate
    python manage.py migrate --database=gdpr_log

    and again - migrations have been applied in both cases, looks good:

    Applying users.0058_auto_20190716_1308... OK

  2. and now when I try to make migrations again:

    python scripts/manage.py makemigrations

I get:

sym_poc/users/migrations/0059_auto_20190716_1332.py

  • Change managers on user

WHY? The change that Django is trying to make relates only to changing manager, and not to changing the anonymised field (which has already been added to the database).

tengqm commented 5 years ago

struggling on the same issue for quite some time.

jkkjonah commented 4 years ago

I did some digging and narrowed it down to the issue being that gdpr_assist, on load, automatically casts the model manager for all models with PrivacyMeta classes defined as CastPrivacy<model manager class>.

During a run of the makemigrations command, if a model has a default manager with use_in_migrations = true, the command will attempt to compare the previous manager instances w/ the new manager instances and generate a migration if there is a diff. There is some mechanism in django (that I don't fully understand) for generating "previous" app model state to compare to the current app model state for generating migrations. The issue stems from the fact that when the "previous" state is generated, it will show that the default model manager is the one currently defined (e.g. UserManager), when the "current" state is generated it will always show the cast version of the manager CastPrivacyUserManager, so no matter what you do, as long as that model is marked for use in migrations, it will produce a diff. I suspect this will also have the same affect on other models with managers that are marked for use in migrations (did not verify this as I don't have any such models).

The workaround I have currently implemented is to define a custom model manager that inherits from UserManager that sets use_in_migrations = false which causes this model manager to be excluded from makemigrations diffs. This is not ideal because it causes the behavior of the User model with respect to migrations, but it doesn't seem to be necessary for my codebase so I'll live with it for now.

Unfortunately, I don't really understand the django migration architecture enough to suggest any potential root cause fixes.