Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix migrate to bigautofield #411

Merged
merged 5 commits into from
May 28, 2021
Merged

Fix migrate to bigautofield #411

merged 5 commits into from
May 28, 2021

Conversation

mom1
Copy link
Contributor

@mom1 mom1 commented May 25, 2021

Fix #410

@Andrew-Chen-Wang
Copy link
Member

Thanks @mom1 two questions:

  1. Does this create a new migrations file?
  2. If you were to first create a new migrations file right now without this PR (like many people may have already done in their virtualenv), then using this PR, would a new migrations still be needed?

Thanks for this!

@mom1
Copy link
Contributor Author

mom1 commented May 26, 2021

For example if this pr will be include to 4.8.0

  1. Fresh install 4.8.0 - No new file will be created.
  2. 4.6.0 -> 4.8.0 - No new file will be created.
  3. 4.7.0 -> 4.8.0 - If was created file 0009... and was applied to db befor update 4.8.0 then new file will be created.
    For resolve problem:
    • need run python manage.py migrate 0008.
    • need delete file 0009... or reinstall djangorestframework-simplejwt
      Else creating a new migration will be broken.

Thanks for this library!

@Andrew-Chen-Wang
Copy link
Member

Andrew-Chen-Wang commented May 27, 2021

tl;dr will an additional migrations file that is the exact same as user generated migrations break Django because they are the exact same?

Thanks for the diagnosis @mom1 ! I think what this PR has is good, but we need to make sure the release doesn't break any production builds, too.

What we could do is create a new migrations file that is titled with a datetime (e.g. 0007_auto_20171017_2214.py; stupid of me not to double check these migration file names before argggg sorry), so something like 0009_auto_20210526_0101asdfh2jkfdu892.py since these prod databases have a migrations table that logs which migrations files were migrated and checks which files haven't. So even if these people create a migrations file titled 0009_auto_20210525_0101.py, we can add a migrations file for the next release that guarantees no conflicting file name by appending random letters (example from above 0009 example).

The remaining question is: we know for a fact that the user generated migrations file and the new file we're adding is the same, but will that break Django because they are the exact same? My intuition is saying "no," but obviously we'd need to test this.

Thoughts?

Sorry edit note:
The reason I'd like to do this is for the precise quote here:

need delete file 0009... or reinstall djangorestframework-simplejwt Else creating a new migration will be broken.

Not everyone uses a virtual env for production or has multiple servers that are brought down and up all the time like on cloud hosted instances, so that's why I'm recommending this approach be added.

@mom1
Copy link
Contributor Author

mom1 commented May 27, 2021

@Andrew-Chen-Wang I understand your concerns.
But It seems to me that this problem cannot be solved now.

If the user created the file 0009_auto_20210525_0101.py, and we created the file0009_auto_20210526_0101asdfh2jkfdu892.py, then these files will have the same

dependencies = [
        ('token_blacklist', '0008_migrate_to_bigautofield'),
    ]

and the next time the migration is created, django will offer to create a third file with

dependencies = [
        ('token_blacklist', '0009_auto_20210525_0101'),
        ('token_blacklist', '0009_auto_20210526_0101asdfh2jkfdu892'),
    ]

@Andrew-Chen-Wang
Copy link
Member

Andrew-Chen-Wang commented May 27, 2021

@mom1 I'm assuming you're talking about the merge conflict and thus a new file that tries to linearize the migration history, right? Something like X_merge_X_auto_etc.py? Here's a different strategy (this time I wrote it and tested it from both perspectives of those who have a 0009 migrations file and those who do not):

We create a new migration file 0010_auto_*.py based on the deleted models.BigAutoField. This creates the conflict since both depend on 0008_auto_etc.py. So in addition we create a migration file 0011 that has no operations but linearizes the history. This file has to balance those who didn't migrate and those who did using their virtual environment, so 0011 looks like:

import os, fnmatch
from pathlib import Path

from django.db import migrations

parent_dir = Path(__file__).resolve(strict=True).parent


class Migration(migrations.Migration):

    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.dependencies = [
            ('token_blacklist', '0010_auto_20210527_1746')
        ]
        _m = sorted(fnmatch.filter(os.listdir(parent_dir), "000*.py"))
        if len(_m) == 9:
            self.dependencies.insert(0, ('token_blacklist', os.path.splitext(_m[8])[0]))

    operations = [
    ]

It looks quite unstable since we're using files to check, but I couldn't find a way to hack with SQL to the django_migrations table to grab which migrations had already been migrated. However, because of what was user-generated by 0009 migrations, we also have to do a bit of work in 0010:

# Generated by Django 3.2.3 on 2021-05-27 17:46

import os, fnmatch
from pathlib import Path

from django.db import migrations, models

parent_dir = Path(__file__).resolve(strict=True).parent


class Migration(migrations.Migration):

    dependencies = [
        ('token_blacklist', '0008_migrate_to_bigautofield'),
    ]

    operations = [
         migrations.AlterField(
             model_name='blacklistedtoken',
             name='id',
             field=models.BigAutoField(primary_key=True, serialize=False),
          ),
          migrations.AlterField(
              model_name='outstandingtoken',
              name='id',
              field=models.BigAutoField(primary_key=True, serialize=False),
          ),
    ]

Finally, we add those id fields directly back in otherwise another new file will be generated (token_blacklist.models):

id = models.BigAutoField(primary_key=True, serialize=False)
# This goes into both OutstandingToken and BlacklistedToken

So those three changes (two new files, and one changed file i.e. models.py) makes sure no new migrations file is needed. Sorry for not trying this out earlier; was just swamped by work. Let me know if you find anything errneous!

Note: I wrote all this code down here since I'd like for you to create a PR for it. I can't approve my own PRs, so your help is really appreciated!

@mom1
Copy link
Contributor Author

mom1 commented May 27, 2021

Sounds great!
But, maybe can we still do without adding id fields in models?

p.s. Added your code.

@Andrew-Chen-Wang
Copy link
Member

@mom1 Unfortunately I tried it without adding and those migrating from 4.6.0 to 4.8.0 or who didn't migrate would need to create a new migrations file like those who did migrate on 4.7.0, so it's necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistency 0008_migrate_to_bigautofield and field id in models
2 participants