Fixed -- 28785 Made makemigrations warn if migrations are missing

[manav014]

ticket-28785

Supersedes #9339

[olivierdalang]

Thanks for this !

IMO, this check should not only run for makemigrations, but also for other commands (e.g. runserver, createsuperuser, etc.)

For unapplied migrations, management/base.py produces this warning :

You have 2 unapplied migration(s). Your project may not work properly until you apply the migrations for app(s): core. Run 'python manage.py migrate' to apply them.

Both cases are quite similar, so having the same type of warning for both would make sense. Can we have something like :

You have 2 applied migration(s) that are missing the original migration file for app(s): core. Ensure you did not delete or renamed migrations files, and/or that you are working on the correct branch. Your project may not work properly until you fix this situation.

[jacobtylerwalls]

IMO, this check should not only run for makemigrations, but also for other commands (e.g. runserver, createsuperuser, etc.)

This sounds vague. I would rather the author stick to the scope of the ticket as accepted. Doing so helps both authors and reviewers.

[olivierdalang]

I don't know what's vague in my comment (maybe the wording, I'm not a native English speaker, sorry). Management commands even have a requires_migrations_checks property documented like this :

A boolean; if True, the command prints a warning if the set of migrations on disk don’t match the migrations in the database. A warning doesn’t prevent the command from executing. Default value is False.

This exactly matches the scope of what this PR is about, doesn't it ?

[jacobtylerwalls]

Ah, thanks for clarifying. Your wording was fine. 👍🏻 It just took a second read through the ticket for me to see that the makemigrations command was not singled out until after the ticket was originally accepted, my apologies.

[jacobtylerwalls]

Management commands even have a requires_migrations_checks property documented like this :

Thanks, excellent find. In case helpful for others, here's the status quo for that property on various commands:

• BaseCommand: False
• changepassword: True
• createsuperuser: True
• runserver: categorically runs the unapplied migrations check regardless of the class attribute because that check ran as part of runserver before the introduction of the class attribute in 50931df.

@manav014 with that in mind, do you have an opinion about how to proceed here?

I would need to think more myself. The first comment on the ticket cautions this might pester folks who turn squashed migrations into normal ones, so I'm hesitant to patch this into runserver categorically. (Or, unlike the unapplied migrations check have this check observe the class attribute?)

Another thing is that we have ticket-26760 for dropping migrations from the db when they don't exist on disk, and some discussion about whether to execute that automatically vs. manually. That would make this situation less frequent, no?

Thanks for your input @olivierdalang; good find, and once again, apologies if I sounded out of breath earlier.

[olivierdalang]

@jacobtylerwalls Are you referring to this ?

django/docs/topics/migrations.txt

Lines 816 to 822 in bd9cfbb

	You must then transition the squashed migration to a normal migration by:
	- Deleting all the migration files it replaces.
	- Updating all migrations that depend on the deleted migrations to depend on
	the squashed migration instead.
	- Removing the ``replaces`` attribute in the ``Migration`` class of the
	squashed migration (this is how Django tells that it is a squashed migration).

That's indeed a case where the warning would show. But I'm not sure the docs give a good advice there (is it mandatory to remove replaces, or can it list deleted migrations ?).

As for this PR, maybe it could add a line in that part of the docs warning that doing that process would lead to warnings displaying at users. And the warning itself could mention the case of squashed migrations and suggest to remove the rows in the migration tables if that's the case. It's a bit hard to tell how frequently this is being used.

All in all, I think it's a deeper issue in the squashing process (alongside other issue with it, see https://groups.google.com/g/django-developers/c/xpeFRpMTBZw/m/lDq78EedAwAJ), and I'd see this PR as beneficial anyways as it could actually help highlight some existing potential issues, including the one mentioned in ticket 26760.

[manav014]

@manav014 with that in mind, do you have an opinion about how to proceed here?

@jacobtylerwalls I think I need to dig more into this. But as of now, What I have in mind is that we shall use requires_migrations_checks to show the warning that means if it is set to true we shall display the warning like in changepassword and createsuperuser commands this is set to true.

[jacobtylerwalls]

Are you referring to this ?

Yep, you found the right bit in the docs re: "transition the squashed migration to a normal migration".

That's indeed a case where the warning would show. But I'm not sure the docs give a good advice there (is it mandatory to remove replaces, or can it list deleted migrations ?).

Recent triage decisions have stuck to this literal advice that it's mandatory, see ticket-32205.

[jacobtylerwalls]

But as of now, What I have in mind is that we shall use requires_migrations_checks to show the warning that means if it is set to true we shall display the warning like in changepassword and createsuperuser commands this is set to true.

Nice, that sounds like a good start. Then we can get a decision on runserver after that. Thanks.

[felixxm]

@manav014 Thanks for this patch 👍

I'm afraid that we cannot move it forward without solving ticket-26760, because it will raise warnings for squashed migrations which were removed.

[felixxm]

I would prefer the solution from #9339 where we list all missing files not only all labels.

[manav014]

I will commit the desired changes.

[felixxm]

Do we need app_labels? IMO it should be fine to check all apps like in check_consistent_history().

[manav014]

I will commit the desired changes.

[manav014]

@felixxm Thank you for your feedback.
I will make all the commented changes and also I totally agree with the point that we can not move it forward without solving ticket-26760. Till then let's wait for the ticket-26760 to be closed. I will try to assign the ticket-26760 to myself as soon as I will get some time.

[jacobtylerwalls]

Hi @manav014 just a note to say ticket-26760 was recently fixed with a feature to call migrate --prune to remove references to deleted migrations, so maybe in the case that was blocking you here with deleted squashed migrations you can advise the user to do that, or something similar. Hope that helps.

[manav014]

Thanks, @jacobtylerwalls . I will add a warning message to warn the user about migrate --prune .

[felixxm]

Closing due to inactivity.