Fixes #537 - Avoid migration errors with custom user models

[codecov[bot]]

Codecov Report

Merging #544 (23463f4) into master (8d1ecfe) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #544   +/-   ##
=======================================
  Coverage   98.18%   98.18%           
=======================================
  Files          62       62           
  Lines        2751     2751           
  Branches      288      288           
=======================================
  Hits         2701     2701           
  Misses         29       29           
  Partials       21       21           

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[chipx86]

I'm looking forward to seeing this land.

I'm not sure it's limited to custom models, though. I'm seeing this in a database using a standard User that has old PhoneDevice data in it.

Django's migration system sets up mock versions of models when it runs operations. These are built using state set by prior migrations, such as a CreateModel. The base classes for the mock models are defined by that state. These versions of the models are then used for all relations during migration, so that they can be altered as migrations are run, and are also used during a RunPython operation.

Unfortunately, Django does not include AbstractUser in the base migrations, meaning that get_username() never actually works, hence the issue you and I are both seeing (you with a custom User model, me with the default).

We can test this on a brand-new Django project (using 3.2 LTS for this test) with the only modifications being:

1. The addition of the two_factor module (using 1.13.2 due to 1.14 having another bug with migrations, which was fixed in 880d7b370c8b285c92abcf1262fefcc12f9bb5e5).

2. Some debug output in 0003_auto_20150817_1733.py:

   def migrate_phone_numbers(apps, schema_editor):
       PhoneDevice = apps.get_model("two_factor", "PhoneDevice")
   
       user_cls = PhoneDevice._meta.get_field('user').target_field.model
       print('*** user_cls = %r' % user_cls)
       print('*** mro = %r' % (user_cls.__mro__,))
       print('*** get_username = %r' % getattr(user_cls, 'get_username', None))

Upon migrating, we get this output:

*** user_cls = <class '__fake__.User'>
*** mro = (<class '__fake__.User'>, <class 'django.db.models.base.Model'>, <class 'object'>)
*** get_username = None

What we'd ideally see (if base classes were correct) would be:

*** mro = (<class '__fake__.User'>, <class 'django.contrib.auth.models.AbstractUser'>, <class 'django.contrib.auth.base_user.AbstractBaseUser'>, <class 'django.contrib.auth.models.PermissionsMixin'>, <class 'django.db.models.base.Model'>, <class 'object'>)
*** get_username = <function AbstractBaseUser.get_username at 0x101d24ca0>

It's possible that all this worked at some point in Django's past. Much of the migration code has heavily evolved over time (I've been deep in the internals of it for years as the primary developer on https://github.com/beanbaginc/django-evolution/, an alternate, optimized migration system meant for self-installable services that avoids some of these issues through its own system but also works cooperatively with Django's migrations — not advocating its use, this is special-purpose). I'm not sure whether or not it has, but I have to imagine this migration worked for someone at some point.

Either way, your fix is the right one, and I can verify it solves my migration issue using a standard User and a PhoneDevice with rows.

I just wanted to provide insight into the reason this was ultimately failing, for both code archaeological reasons and for anyone else bitten by this, and to help with the description of your pull request.

[claudep]

Wow, thanks a lot for this extensive explanation, nice!!

About landing, the problem currently is that we miss active reviewers in this project. @moggers87 looks like the only other reviewer right now and has little time for this project. As the project is set up so as each PR must be reviewed by a reviewer with write access, my own PRs are stuck :-(

[chipx86]

Thanks to both of you for getting this in!