[10.x] Drop the primary key if it exists when adding a new primary key

[KieranFYI]

When attempting to change the primary key for a table, if one already exists this causes an error 1068. Attempting to use dropPrimary() along with primary() causes error 1075, due to primary and dropPrimary being two different commands.

The fix attempts to drop the PRIMARY index if it does exist.

[GrahamCampbell]

I don't think it is a good idea to implicitly do this. This could cause major issues for people.

[KieranFYI]

Care to explain why it's going to cause a major issue?

[taylorotwell]

@KieranFYI can you provide an example of how this would be used in a migration?

[KieranFYI]

@taylorotwell This is the code used to discover the issue. Was moving from a single primary index (id) to a multi column index due to needing the autoincrement constrained.

The specific line is as below.
$table->primary(['id', 'ticket_category_id', 'parent_type', 'parent_id']);

<?php

use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;

return new class extends Migration {
    /**
     * Run the migrations.
     */
    public function up(): void
    {
        Schema::table('ticket_users', function (Blueprint $table) {
            $table->dropConstrainedForeignId('ticket_id');
        });

        Schema::table('tickets', function (Blueprint $table) {
            $table->string('key')
                ->unique()
                ->index()
                ->first();

            $table->foreignId('ticket_category_id')
                ->after('user_id')
                ->constrained('ticket_categories')
                ->cascadeOnUpdate()
                ->restrictOnDelete();

            $table->primary(['id', 'ticket_category_id', 'parent_type', 'parent_id']);
        });

        Schema::table('ticket_users', function (Blueprint $table) {
            $table->string('ticket_key')
                ->after('id')
                ->references('key')
                ->on('tickets')
                ->cascadeOnUpdate()
                ->cascadeOnDelete();
        });
    }

    /**
     * Reverse the migrations.
     */
    public function down(): void
    {
        Schema::table('ticket_users', function (Blueprint $table) {
            $table->dropColumn('ticket_key');
        });
        
        Schema::table('tickets', function (Blueprint $table) {
            $table->primary(['id']);
        });

        Schema::table('tickets', function (Blueprint $table) {
            $table->dropColumn(['key']);
            $table->dropConstrainedForeignId('ticket_category_id');
        });

        Schema::table('ticket_users', function (Blueprint $table) {
            $table->foreignId('ticket_id')
                ->after('id')
                ->constrained('tickets')
                ->cascadeOnDelete()
                ->cascadeOnUpdate();
        });
    }
};

[vanodevium]

IMHO, either this behavior should be for all SQL databases, or it should be a separate method.

[lukehebb]

Would it not be better to make this opt-in rather than a default?

For example if you you can add something such as this:

$table->primary([...])->dropIfExists();

Retain the existing default behaviour but allow opting in if necessary

[KieranFYI]

@vanodevium From my understanding, this fix should work across the other SQL databases, but as i only use MySQL/MariaDB i'm unable to test the other options. If you'd be able to fix this, it would be welcome.

@lukehebb SQL by default only supports one primary index, So this isn't changing any behaviour except fixing a bug with Laravel.

[vanodevium]

There is not bug.
It is security: if you want to make new primary index, please remove old firstly.

[KieranFYI]

@vanodevium That would involve dropping the column too due to autoincrement, which is alot of effort just to update the index and would destroy any relations already using that id. Attempting to just drop the primary results in the following error.

Updated snippt from the original code.

        Schema::table('tickets', function (Blueprint $table) {
            $table->dropPrimary();
        });

        Schema::table('tickets', function (Blueprint $table) {
            $table->string('key')
                ->unique()
                ->index()
                ->first();

            $table->foreignId('ticket_category_id')
                ->after('user_id')
                ->constrained('ticket_categories')
                ->cascadeOnUpdate()
                ->restrictOnDelete();

            $table->primary(['id', 'ticket_category_id', 'parent_type', 'parent_id']);
        });

$ php artisan migrate

   INFO  Running migrations.

  2023_12_03_163800_update_tickets_table ................................................................................................. 11ms FAIL

   Illuminate\Database\QueryException

  SQLSTATE[42000]: Syntax error or access violation: 1075 Incorrect table definition; there can be only one auto column and it must be defined as a key (Connection: mysql, SQL: alter table `tickets` drop primary key)

  at vendor\laravel\framework\src\Illuminate\Database\Connection.php:822
    818▕                     $this->getName(), $query, $this->prepareBindings($bindings), $e
    819▕                 );
    820▕             }
    821▕
  ➜ 822▕             throw new QueryException(
    823▕                 $this->getName(), $query, $this->prepareBindings($bindings), $e
    824▕             );
    825▕         }
    826▕     }

  1   vendor\laravel\framework\src\Illuminate\Database\Connection.php:580
      PDOException::("SQLSTATE[42000]: Syntax error or access violation: 1075 Incorrect table definition; there can be only one auto column and it must be defined as a key")

  2   vendor\laravel\framework\src\Illuminate\Database\Connection.php:580
      PDOStatement::execute()

[vanodevium]

Ok. Thanks. I see your point.

I would insist on $table->primary([...])->dropIfExists(); and only when this option then it have to make query with sub-query

drop index if exists `PRIMARY`,

[KieranFYI]

@vanodevium I was hoping to approach this using that method, but i don't have the time or the energy to write all the code and tests required for all the different aspects of laravel for 31 characters.

[taylorotwell]

I'm having a bit of a hard time understanding the fears around this PR.

You have a migration that declares a primary key.

You later write a migration that declares a new primary key. A table can only have one primary key so the primary key is moved to your new declaration.

That seems... kinda fine? What am I missing?

[vanodevium]

This code is a "hundred years" old.
Everyone used it and no one had any problems.

What it means:

• or no one has ever done such an operation, because there would be issues
• or they did it through raw query in the migration

imho

[KieranFYI]

The only other method to fix this issue would be to change the compiler to combine compilePrimary and compileDropPrimary, but this would be alot of effort and most likely isn't support across the other schema's.

[KieranFYI]

@vanodevium I'm not sure i understand what your problem is here, This is how SQL works, it's not going to cause issues.

[taylorotwell]

I've seen several mentions of "issues" and "problems". Please, anyone, elaborate on those issues that would be caused by this PR in very practical, real-world terms.

[Ballard-dev]

I'm having a bit of a hard time understanding the fears around this PR.

You have a migration that declares a primary key.

You later write a migration that declares a new primary key. A table can only have one primary key so the primary key is moved to your new declaration.

That seems... kinda fine? What am I missing?

Checks out

[afonic]

The error message in @KieranFYI's example sent me here. I resolved my issue by dropping the primary key in one migration and adding a new one in the subsequent migration (different file).

This PR would greatly speed up things in development. The only problem I can foresee is that if a user accidentally declares a new primary key, it would drop the old one, likely breaking existing relationships. However, since migrating on a production environment can generally cause significant issues, I don't thing that it applies here.

[KieranFYI]

@afonic dropping a primary key should have no effect on foreign keys, only dropping the column would cause issues. Would you be able to explain yourself a bit more?

[afonic]

@KieranFYI yeah you're right, I meant dropping the column after removing the primary index. Honestly, I can't see any negatives about this.

[hackerESQ]

This is PR is causing a regression in my testing.

SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'if exists PRIMARY, add primary key (key, tenant)' at line 1 (Connection: tenant, SQL: alter table settings drop index if exists PRIMARY, add primary key (key, tenant))

I initially create a table that looks like this:

Schema::create('settings', function (Blueprint $table) {
    $table->string('key')->unique()->index();
    $table->text('value')->nullable();
    $table->primary('key');
});

Then update it like this to create a compound key. This used to work as expected:

Schema::table('settings', function (Blueprint $table) {
    $table->string('key')->index()->change();
    $table->string('tenant')->nullable();
    $table->dropPrimary('key');
    $table->dropUnique('settings_key_unique');

    $table->primary(['key', 'tenant']);
    $table->unique(['key', 'tenant']);
});

Which seems to be executed as:

alter table settings drop index if exists PRIMARY, add primary key (key, tenant)

Note the drop and add in the same query.

Before this PR, there was an expectation that you had to explicitly drop the primary key before we could add a new key and these would run as separate queries.

I've narrowed the issue down to v10.38.0 and this PR.

Still testing what's causing MySQL to squawk specifically, but I'm suspecting MySQL doesn't like dropping keys and adding keys in the same query. You can either do one or the other. I think.

Any suggestions are appreciated.

[vanodevium]

as I said before, this should not be a global change, because it will cause many problems for people

[muhajirinlpu]

I made temporary workaround with custom MysqlGrammar, I drop it here in case anyone need it

<?php

namespace App\Database\Schema\Grammars;

use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Fluent;

class MysqlGrammar extends \Illuminate\Database\Schema\Grammars\MySqlGrammar
{
    public function compilePrimary(Blueprint $blueprint, Fluent $command)
    {
        return sprintf('alter table %s add primary key %s(%s)',
            $this->wrapTable($blueprint),
            $command->algorithm ? 'using '.$command->algorithm : '',
            $this->columnize($command->columns)
        );
    }
}

and add into AppServiceProvider.php in boot method

        DB::connection('mysql')->setSchemaGrammar(new MysqlGrammar());

[dnldsht]

@taylorotwell i'm also facing the same issue that @hackerESQ reported. in mysql 8 seems that you can't do
alter table settings drop index if exists PRIMARY

it gives the following syntax error
[42000][1064] You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'if exists PRIMARY' at line 1

[taylorotwell]

Reverted.