kitloong / laravel-migrations-generator

Laravel Migrations Generator: Automatically generate your migrations from an existing database schema.
MIT License
2.43k stars 269 forks source link

Migrations try to recreate FKs even with table checks #181

Closed Synchro closed 1 year ago

Synchro commented 1 year ago

A common situation for using this package is that you have accumulated a bunch of migrations that work, but want to reduce them to a single one, or a set of fresh ones. That means you will want to delete all old migrations, generate new ones, but not run them because the DB is already configured to be the same as the migration target.

In the config there is the --with-has-table option, which allows a migration to run, and ignores the attempt to create a table if it already exists. That's all ok. The problem occurs if you have foreign keys, when the sense of the table check is reversed.

It's doing this to make a table:

if (!Schema::hasTable('x')) {
            Schema::create(

and later does this to add the FK:

        if (Schema::hasTable('x')) {
            Schema::table(
                'x',
                function (Blueprint $table) {
                    $table->foreign(['y_id'])->references(['id'])->on('y')->onUpdate('CASCADE')->onDelete('CASCADE');

Of course, this means that it will always run the FK creation because the table is guaranteed to exist at this point – but if the FK already exists because this migration is a consolidation of earlier migrations, this will fail, and that's what I've found that it does:

artisan migrate

   INFO  Running migrations.  

  2023_05_22_064953_create_x_table ................................................................................................ 85ms FAIL

   Illuminate\Database\QueryException 

  SQLSTATE[HY000]: General error: 1826 Duplicate foreign key constraint name 'x_id_foreign' (Connection: mysql, SQL: alter table `x` add constraint `x_id_foreign` foreign key (`y_id`) references `y` (`id`) on delete CASCADE on update CASCADE)

I'm not sure of the best way to fix this - the hasTable checks are mostly useless for the FKs, so they could be dropped altogether, unless there a way to check for FKs in the same way as tables - hasForeign or something? One workaround (if you're using this package purely for consolidation of previous migrations) is to have it simply return if some critical table (like users) is already present:

if (Schema::hasTable('users')) {
    return;
}

To Reproduce Steps to reproduce the behavior:

  1. In an app with existing migrations including FKs
  2. Delete old migrations
  3. Run php artisan migrate:generate --with-has-table ...
  4. Run php artisan migrate
  5. See error on the first FK creation

Details (please complete the following information):

kitloong commented 1 year ago

Hi @Synchro , thank you for using the migrations generator.

That means you will want to delete all old migrations, generate new ones, but not run them because the DB is already configured to be the same as the migration target.

Regarding this use case, when you run the command, the generator will prompt you for following inputs.

php artisan migrate:generate --squash
Using connection: mysql

Generating migrations for: failed_jobs,password_reset_tokens,personal_access_tokens,users

 Do you want to log these migrations in the migrations table? (yes/no) [yes]:
 > yes

 Next Batch Number is: 2. We recommend using Batch Number 0 so that it becomes the "first" migration. [Default: 0] [0]:
 > 2

yes to log migrations and then please provide your next batch number, in my case, it is 2.

By this, when you run php artisan migrate your new migrations it will skip the squashed migrations.

It should work, but please let me know if it doesn't.

the hasTable checks are mostly useless for the FKs, so they could be dropped altogether, unless there a way to check for FKs in the same way as tables - hasForeign or something?

I regret to tell you I do not have a good solution too. hasForeign does not exist, and there is no equivalent check available from the framework.

if (Schema::hasTable('users')) {
    return;
}

This looks feasible, at least it makes more sense now by skipping the foreign key generation.

I would like to further explore it.

kitloong commented 1 year ago
if (Schema::hasTable('users')) {
    return;
}

Including this check in the foreign key migration can be acceptable if the users table already exists.

However, in a different scenario where another team member attempts to run this migration to create a completely new schema, all tables will be created first according to their order. As a result, the foreign key will never be created.