[5.x]: Restoring a database backup after a failed migration leaves newly created tables around

[MoritzLost]

What happened?

Description

I got an error when running all the pending migrations for the Craft 5 upgrade using php craft up. The actual error was related to a plugin and not important here. The CLI command then asked whether I wanted to restore the database backup created by the command before running the migrations. I did that, then fixed the problem in my code that caused the migration to fail.

Then I ran php craft up again, but this time I got this error:

*** applying m230617_070415_entrify_matrix_blocks
    > alter column sectionId in table {{%entries}} to integer ... done (time: 0.036s)
    > add column primaryOwnerId integer AFTER `parentId` to table {{%entries}} ... done (time: 0.012s)
    > add column fieldId integer AFTER `primaryOwnerId` to table {{%entries}} ... done (time: 0.012s)
    > add column deletedWithOwner boolean NULL DEFAULT NULL AFTER `dateDeleted` to table {{%elements}} ... done (time: 0.092s)
    > create table {{%elements_owners}} ...Exception: SQLSTATE[42S01]: Base table or view already exists: 1050 Table 'cb_elements_owners' already exists

Checked the migration in question – this one creates the elements_owners table, so it was created during the first attempt at running migrations, but not deleted when restoring the database backup.

Steps to reproduce

1. Create a migration that will create a table and then return an error.
2. Run the migration using php craft up, answering yes when asked whether you want to create a database backup and whether you want to restore that backup after the migration fails.

Expected behavior

If I restore a database backup after a failed migration, I expect the database to be restored to the exact state as before that migration. I don't expect tables that were created before the error occurred to stick around, causing additional errors. I would expect to be able to run migrations again, without additional errors due to the first attempt.

Actual behavior

Tables created during the failing migration (or any previous migration) stick around, even when the database backup is restored.

The db/restore command explicitly asks whether to drop all tables before restoring the backup. The migrate/up command should do this as a default. It could also add the same prompt asking whether I want to drop all tables before restoring the backup, though I can't see any case where I wouldn't want this behaviour.

Craft CMS version

5.1.2

PHP version

8.2

Operating system and version

No response

Database type and version

MySQL

Image driver and version

No response

Installed plugins and versions

[brandonkelly]

Thanks for pointing that out! Normally migrations will explicitly delete tables they’re about to create, if they happen to already exist, but we overlooked that for a couple tables. Fixed for the next release.

[brandonkelly]

Craft 5.1.3 is out with that fix. Thanks again.

[MoritzLost]

@brandonkelly Thanks!

Still, couldn't the restoration be handled better by the migrate command itself? I think I recall having the same problem with some plugin migrations … wouldn't it make sense for the migrate command to restore the original state from the DB backup exactly?

[brandonkelly]

The migrate command has no knowledge of what tables are about to be added; only the migration themselves know that.

And similarly, we can’t do it from db/restore because really that’s just executing a file with a bunch of SQL statements, which will recreate any DB tables + data that existed at the time the backup was made. But the backup has no way of knowing about DB tables that haven’t been created yet, so it couldn’t possibly include DROP TABLE statements for them.

[MoritzLost]

@brandonkelly Not sure I understand the problem 🤔

The db/restore command already has an option to drop all tables before the backup is restored: https://github.com/craftcms/cms/blob/5.x/src/console/controllers/DbController.php#L126-L147

This just finds and drops all database tables, then restores the database backup. Why couldn't the migrate/up action do the same thing? This wouldn't require knowing which tables were created during the failed or any previous migration. This behaviour would make sense from a user perspective: The command asks whether I want to restore my database backup, so dropping all tables and then applying the backup would be exactly what I expect to happen.

[brandonkelly]

Sorry, you’re right, db/restore can drop all tables and then restore the DB backup, which works around the problem. It’s relatively safe to do that because you would only be running that command if you wanted to discard all of your current data, in favor of using the backup. That is obviously not the case when you are running migrate/up though…

[MoritzLost]

@brandonkelly Hm, when migrate/up fails and I agree to restoring my database backup, I would expect the same behaviour as db/restore. Not sure why I would ever want to retain a half-migrated state after I've explicitly opted into restoring the backup I just created. Anyway, not that important, agree to disagree I guess.