Skip to content

fix: add missing migration for token_blacklist app #894

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

Merged
merged 4 commits into from
Apr 28, 2025

Conversation

juanbailon
Copy link
Contributor

PR #879 modified both the OutstandingToken and BlacklistedToken models Meta class by adding the attributes verbose_name and verbose_name_plural, but did not include the required migration file.

This PR adds the missing migration to ensure the database schema remains consistent with the models.

juanbailon and others added 2 commits March 23, 2025 02:21
PR jazzband#879 modified both the OutstandingToken and BlacklistedToken models Meta class, but did not include the migration. This commit adds the missing migration file.
@vgrozdanic
Copy link
Contributor

Thank you for fixing this, i have missed that in a review!

@Andrew-Chen-Wang
Copy link
Member

this doesn't account for people who already accidentally created a migration

@juanbailon
Copy link
Contributor Author

this doesn't account for people who already accidentally created a migration

Thanks for the feedback! I tried to conditionally make the migration by checking the verbose_name and verbose_name_plural values using migrations.RunPython, but Django does not allow altering the Meta class attributes at runtime. Doing something like:

def update_verbose_name(apps, schema_editor):
    OutstandingToken = apps.get_model("token_blacklist", "OutstandingToken")
    
    current_verbose_name = OutstandingToken._meta.verbose_name
    desired_verbose_name = "Outstanding Token"

    if current_verbose_name != desired_verbose_name:
        OutstandingToken._meta.verbose_name = "Outstanding Token"
        OutstandingToken._meta.verbose_name_plural = "Outstanding Tokens"
        OutstandingToken._meta.ordering = ("user",)
        

class Migration(migrations.Migration):
    dependencies = [
        ("token_blacklist", "0012_alter_outstandingtoken_user"),
    ]

    operations = [
        migrations.RunPython(update_verbose_name),
    ]

won’t persist or trigger Django’s migration framework to recognize the change. Since AlterModelOptions affects Django’s internal model metadata (not the database), it requires an explicit migration.

Because of this, the only viable solution i found was doing it this way:

operations = [
        migrations.AlterModelOptions(
            name='outstandingtoken',
            options={'ordering': ('user',), 'verbose_name': 'Outstanding Token', 'verbose_name_plural': 'Outstanding Tokens'},
        ),
]

which is the approach currently in the PR.

The migration conflict you mentioned only affects users who had the package in editable mode (pip install -e .). In a normal install, Django does not track migrations inside installed packages, so this wouldn’t be an issue.

Let me know your thoughts or if you have alternative suggestions to handle this more gracefully.

@vgrozdanic
Copy link
Contributor

this doesn't account for people who already accidentally created a migration

Also, i would add that the code without migration wasn't part of the latest release, so only the devs that follow the latest master (and not the latest release) would be affected

@Andrew-Chen-Wang
Copy link
Member

suggestion: in the changelog, I would write "Major Patch for Effected Users" and write how they need to rollback the migration in jazzband IF they have it and supply a django CLI command.

@Andrew-Chen-Wang
Copy link
Member

TODO we need a test to detect if database changes are available... to ensure this doesn't happen again

@vgrozdanic
Copy link
Contributor

suggestion: in the changelog, I would write "Major Patch for Effected Users" and write how they need to rollback the migration in jazzband IF they have it and supply a django CLI command.
Sounds good to me!

Maybe just mention to migrate the one in this PR with --fake flag: https://docs.djangoproject.com/en/5.1/ref/django-admin/#cmdoption-migrate-fake?

juanbailon and others added 2 commits March 24, 2025 19:14
This adds a test to ensure all model changes are reflected in migrations.
If ungenerated migrations are detected, the test will fail, enforcing
migration consistency.

References:
- PR jazzband#894
- Issue jazzband#895
@juanbailon
Copy link
Contributor Author

Added test to check for ungenerated migrations
issue #895

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice solution, thank you!

Copy link
Member

@Andrew-Chen-Wang Andrew-Chen-Wang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The migration conflict you mentioned only affects users who had the package in editable mode (pip install -e .). In a normal install, Django does not track migrations inside installed packages, so this wouldn’t be an issue.

The problem is many users in their prod deployment scripts may write python manage.py makemigrations. Why? Honestly, idk but this case did come up once ha 😅

I do think the case for that is a rarity (i.e. why is one creating a migration file in prod) and the solution in this PR is fine.

I do have one more request: can you add a small CHANGELOG edit to mention why this happened? The steps needed is to check if you accidentally have a 0013 migration for the blacklist module in the django_migrations table; if so, write some steps to unmigrate, upgrade SimpleJWT's version, then redeploy with the new version

Thank you!

@Andrew-Chen-Wang
Copy link
Member

after this, will add to #891

@vgrozdanic
Copy link
Contributor

@Andrew-Chen-Wang what do you say as this for changelog:

Fixed

Missing Migration for rest_framework_simplejwt.token_blacklist App A previously missing migration (0013_blacklist) has now been added.This issue arose because the migration file was mistakenly not generated earlier. This migration was never part of an official release, but users following the latest master branch may have encountered it.

Notes for Users

If you previously ran makemigrations in production and have a 0013_blacklist migration in your django_migrations table, follow these steps before upgrading:

  1. Roll back to the last known migration:
python manage.py migrate rest_framework_simplejwt.token_blacklist 0012
  1. Upgrade djangorestframework-simplejwt to the latest version.
  2. Apply the migrations correctly:
python manage.py migrate

Important: If other migrations depend on 0013_blacklist, be cautious when removing it. You may need to adjust or regenerate dependent migrations to ensure database integrity.

@vgrozdanic vgrozdanic merged commit 774a6f8 into jazzband:master Apr 28, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants