jazzband / django-two-factor-auth

Complete Two-Factor Authentication for Django providing the easiest integration into most Django projects.
MIT License
1.71k stars 448 forks source link

Migration from 1.13.1 to 1.15.1 is broken #611

Closed mick88 closed 1 year ago

mick88 commented 1 year ago

Migration from 1.13.1 to 1.15.1 is possible, but migration from 1.13.1 to 1.15.1 raises a database error in the squashed migration phonenumber.0001_squashed_0001_initial:

django.db.utils.ProgrammingError: relation "two_factor_phonedevice" already exists.

Migration to 1.15.1 is only possible if 1.15 migrations are applied.

claudep commented 1 year ago

Sorry for that error. Frankly I don't know if we could have done better, but ideas welcome, of course. As a workaround, you should fake this migration.

matthewhegarty commented 1 year ago

As a workaround, you should fake this migration.

Is it possible you can provide more detail on what this entails? thanks

claudep commented 1 year ago

Sure: python manage.py migrate phonenumber 0001_squashed_0001_initial --fake

matthewhegarty commented 1 year ago

thanks - is it likely this issue will be fixed in future without the need for faking the migration?

claudep commented 1 year ago

Not sure, it was already hard to reach the point where we are, and considering the lack of manpower on the module, I have doubts…

Natureshadow commented 1 year ago

What is the difficulty here? Isn't that a matter of deleting one of the two migrations and making a git tag?

claudep commented 1 year ago

No, the issue is that for new installs, the model has to be created, while for existing users it should not. The combination between moving a model between apps and squashing migrations leads to a complicated situation.

mick88 commented 1 year ago

@claudep could you link to the relevant ticket with discussion and PR that introduced the regression?

Natureshadow commented 1 year ago

No, the issue is that for new installs, the model has to be created, while for existing users it should not. The combination between moving a model between apps and squashing migrations leads to a complicated situation.

So, leaving this as it is obvioulsy is not an option. The two options are "fix it" or "discontinue this app as it is unusable".

Why have the migrations been squashed in the first place? Moving a model from one app to another is trivial (in cases where the source app is guaranteed to be installed).

Maybe the answer to @mick88's question will shed some light on it...

mick88 commented 1 year ago

Why have the migrations been squashed in the first place? Moving a model from one app to another is trivial (in cases where the source app is guaranteed to be installed).

My understanding was that migrations were squashed because one of the migrations required phonenumbers library. This functionality has since been moved to an optional app, so to an end user without historical context this looks like a bug. Squashing these migrations allows new installations to apply a new empty migration instead of going through the history of creating and deleting the model.

Unfortunately, the way the migrations are squashed is not correct, because the new migration does not do exactly what original migrations did. The original migrations created the model and then deleted only model state while keeping the db table. The squashed migration does not have this side effect.

mick88 commented 1 year ago

I looked into this issue this evening and concluded that it is not possible to clean-up migrations in a way that works for all old installations and does not break for users who already installed 1.5.1.

My recommendations:

Workaround: My recommended workaround for running migrations for users who messed up their migration history by upgrading to 1.5.1 from a version older than 1.5 is to run migrate with --fake-initial. This ensures that initial migration for phonenumber uses existing table if one already exists.

python manage.py migrate phonenumber --fake-initial

Use of --fake alone can be dangerous and I recommend against using it as a one fits all solution for all users who encounter this error as it will indiscriminately mark migrations as ran and will be even more difficult to untangle. --fake-initial is much safer and can be used even if migration does not need to be faked. Django will decide whether migration will be ran based on whether table exists in the db schema.

mdalp commented 1 year ago

Another workaround that would likely help is this PR (596), which essentially avoids the need for the migration if you're not using phonedevice.

gfranxman commented 1 year ago

Isn't the problem here that the migrations were squashed, and the original migrations removed? Just restore the old migrations alongside the squashed one. New users start at the squash, and existing users continue through the squashed migrations.

mick88 commented 1 year ago

No, the problem is that the squashed migrations do not exactly mirror original operations 1:1.

Depending what version you are migrating from, you could be running original migrations from the two_factor app, and a squashed migration from phonenumber app.

If you run only squashed migrations in both apps, it'll work fine. If you run original migrations in both apps, it'll also work fine.