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

Fixed #25231 -- Record squashed migrations in the migrate command #5112

Closed

Conversation

Projects
None yet
4 participants
@mlavin
Copy link
Contributor

commented Aug 7, 2015

Ensured squashed migrations are recorded as applied when the migrate command is run and all of the original migrations have previously been applied.

New test test_migrate_record_squashed fails prior to this change and passes with change to the migrate management command.

See https://code.djangoproject.com/ticket/25231

@charettes

View changes

tests/migrations/test_commands.py Outdated
out = six.StringIO()
call_command("migrate", "migrations", verbosity=0)
call_command("migrate", "migrations", list=True, stdout=out, no_color=True)
# No changes were actually applied so there is nothing to rollbac

This comment has been minimized.

Copy link
@charettes

charettes Aug 7, 2015

Member

to rollback.

@mlavin mlavin force-pushed the mlavin:25231-record-squashed-migrations branch Aug 7, 2015

@charettes

This comment has been minimized.

Copy link
Member

commented Aug 7, 2015

A final review from people more familar with the migrations internal wouldn't hurt but I'm pretty confident this is RFC based on the detailed report and the work that lead to #24628 being fixed.

@mlavin

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2015

@carljm worked on the original fix. Perhaps he would be willing to weigh in.

@carljm

This comment has been minimized.

Copy link
Member

commented Aug 7, 2015

This looks fine to me. Technically in the long run it should not be necessary; I think this case can only occur if the replaced migrations were originally migrated pre 1.8.3. So this is really more of an upgrade shim from older versions, to correct the corruption they caused in the migration state, which is why I hadn't included it in the original fix.

But I think it's a shim worth having, and harmless to keep around in the long run. So +1

@mlavin

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2015

I think this case can only occur if the replaced migrations were originally migrated pre 1.8.3.

No this isn't true. I ran into this problem on a project which was started from scratch on 1.8.3.

@mlavin

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2015

The workflow problem is as follows. Start project -> create initial migration -> run initial migrations -> create more migrations -> apply more migrations. Then you decide to squash them (0001 --> 000N) which generates 0001_sqaushed_000N. Then when you run migrate <app_name> the planner notices that 0001 --> 000N have already been run, considers 0001_sqaushed_000N to have already been run, and completes the migrate command without ever calling executor.migrate (and hence never recording 0001_sqaushed_000N as applied through your previous fix) because the plan is empty. If you then remove 0001 --> 000N and call migrate --list it will show as unapplied and likewise trying to call migrate will fail because it will try to create the already existing tables. That's what the failing test case demonstrates: the planner considers it applied but the recorder did not.

@carljm

This comment has been minimized.

Copy link
Member

commented Aug 7, 2015

Ah, yes, my comment above is wrong. This case is of course possible purely on 1.8.3. Thanks for correction, fix looks good.

@timgraham

View changes

tests/migrations/test_commands.py Outdated
recorder = MigrationRecorder(connection)
out = six.StringIO()
call_command("migrate", "migrations", verbosity=0)
call_command("migrate", "migrations", list=True, stdout=out, no_color=True)

This comment has been minimized.

Copy link
@timgraham

timgraham Aug 7, 2015

Member

Use showmigrations instead of "migrate", list=True which is deprecated (we seem to have a bug in @ignore_warnings that causes it to leak state as the tests should catch the use of deprecated features and fail).

This comment has been minimized.

Copy link
@mlavin

mlavin Aug 7, 2015

Author Contributor

Didn't realize that. I'll update.

@timgraham

View changes

tests/migrations/test_commands.py Outdated
("migrations", "0001_squashed_0002"),
recorder.applied_migrations()
)

This comment has been minimized.

Copy link
@timgraham

timgraham Aug 7, 2015

Member

Missing rollback?

This comment has been minimized.

Copy link
@mlavin

mlavin Aug 7, 2015

Author Contributor

Maybe I should move the comment. There is nothing to rollback here. The original migrations are faked by using the recorder (which already handled by the tearDown). The migrate for the squashed migration in this case is a no-op but it does need to be recorded as applied.

@timgraham

View changes

tests/migrations/test_commands.py Outdated
@override_settings(MIGRATION_MODULES={"migrations": "migrations.test_migrations_squashed"})
def test_migrate_record_squashed(self):
"""
Regression test for #25231. Running migrate for a squashed migration

This comment has been minimized.

Copy link
@timgraham

timgraham Aug 7, 2015

Member

usual style is to put the ticket number in parenthesis at the end of the sentence

@timgraham

View changes

tests/migrations/test_commands.py Outdated
call_command("migrate", "migrations", verbosity=0)
call_command("migrate", "migrations", list=True, stdout=out, no_color=True)
# No changes were actually applied so there is nothing to rollback
self.assertEqual(

This comment has been minimized.

Copy link
@timgraham

timgraham Aug 7, 2015

Member

I noticed this assertion will pass if you put if before call_command("migrate", "migrations", verbosity=0) -- basically, it seems to infer that the squashed migration is applied even if it's not marked as such in the database. Do you think it's a separate bug?

This comment has been minimized.

Copy link
@mlavin

mlavin Aug 7, 2015

Author Contributor

I could see it as a bug or a potential problem. If you are using the listing to see if you need to run migrate then this would tell you that you do not need to run it. However, you do need to run migrate to ensure that this gets recorded before removing the old migrations.

However, it also feels weird that using --list or showmigrations would modify the DB state.

This comment has been minimized.

Copy link
@timgraham

timgraham Aug 7, 2015

Member

If squashmigrations could check for replacements, that seems ideal, but not sure introducing the executor into that command is a good idea.

This comment has been minimized.

Copy link
@mlavin

mlavin Aug 7, 2015

Author Contributor

squashmigrations is only run by one person whereas the migration applied state needs to be updated in all instances of the project (dev environments, staging, prod, etc).

Here is where the loader considers them as applied

self.applied_migrations.add(key)
Perhaps it should just be recorded then. That would mean the current check_replacements could be removed from the executor.

Fixed #25231 -- Record squashed migrations in the migrate command.
Ensured squashed migrations are recorded as applied when
the migrate command is run and all of the original migrations
have previously been applied.

@mlavin mlavin force-pushed the mlavin:25231-record-squashed-migrations branch to 5a5b520 Aug 7, 2015

@mlavin

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2015

I think I hit all of the comments with my last update. I've added https://code.djangoproject.com/ticket/25250 to try to capture follow up work related to our discussion here and on #django-dev. Please update/add if it seems like missed something in that new ticket.

@timgraham

This comment has been minimized.

Copy link
Member

commented Aug 7, 2015

merged in 69db1c7, thanks!

@timgraham timgraham closed this Aug 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.