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

[11.x] Enhance database migrations #51373

Open
wants to merge 7 commits into
base: 11.x
Choose a base branch
from

Conversation

hafezdivandari
Copy link
Contributor

@hafezdivandari hafezdivandari commented May 10, 2024

This PR does a lot of improvements and unlocks many possibilities on database migrations, also makes it easier to maintain!

New Features / Enhancements πŸš€

Extend SQLite support to 3.26+

Laravel currently requires SQLite 3.35+, this PR extends SQLite support to 3.26+

Add and Drop Foreign Keys on SQLite

SQLite doesn't support adding / dropping foreign keys with alter table command, but as suggested on the docs, you may recreate the table with arbitrary changes, that's what this PR do to implement these commands.

This PR adds support for blueprint's $table->foreign(), $table->dropForeign() methods and ->constrained() modifier on SQLite:

Schema::table('posts', function (Blueprint $table) {
    $table->unsignedBigInteger('user_id');

    $table->foreign('user_id')->references('id')->on('users'); // Now works as expected on SQLite
});

Schema::table('posts', function (Blueprint $table) {
    $table->foreignId('user_id')
        ->constrained(); // Now works as expected on SQLite
});

Schema::table('posts', function (Blueprint $table) {
    $table->dropForeign(['user_id']); // Now works as expected on SQLite, no exception will be thrown anymore!
});

Fixes #51318, SQLite doesn't allow dropping a column if it is used in a foreign key constraint, this PR makes it possible:

Schema::table('sessions', function (Blueprint $table) {
    $table->dropForeign(['user_id']);
    $table->dropColumn('user_id');
});

// or with more magic...
Schema::table('sessions', function (Blueprint $table) {
    $table->dropConstrainedForeignId('user_id');
});

Add and Drop the primary key on SQLite

This PR adds support for blueprint's $table->primary(), $table->dropPrimary() methods, ->primary(), and ->primary(false)->change() modifiers when modifying table on SQLite:

Schema::table('posts', function (Blueprint $table) {
    $table->dropPrimary();              // Drop the current primary key
    $table->string('email');            // Add 'email' column
    $table->primary(['name', 'email']); // Add a composite primary key 
});

Schema::table('posts', function (Blueprint $table) {
    $table->uuid('id')->primary();  // Add 'id' column and make it primary
});

Schema::table('posts', function (Blueprint $table) {
    $table->string('id')->primary(false)->change();  // Change the type of 'id' column to `string` and drop primary key
});

Preserve the order of commands

This is the most important change on this PR; When updating table and compiling blueprint commands to SQL, the order of commands wasn't respected:

  • First compile all modified columns at one query
  • Second compile all added columns at once
  • Then compile other commands

As discussed on #50925, this will causes issue, unexpected results and leads to error most of the time when updating table. For example, the following code raises The 'username' column already exists! SQL error on all databases, because add column command was compiled before rename column:

Schema::table('users', function (Blueprint $table) {
    $table->renameColumn('username', 'email');
    $table->string('username');
});

This PR fixes this by preserving the order of commands as declared on the blueprint. You don't have to call each command in a separate Schema::table anymore that breaks migration transaction!

Blueprint State for SQLite

As explained before, when updating table on SQLite, we have to recreate the table to do arbitrary changes. To do so we need to know the current state of the table on each command. This PR adds BlueprintState class to keep track of changes on the table. After this PR we are able to mix any combination of blueprint commands in a single Schema::table (as demonstrated on the added integration tests) and it works as expected in the same order!

Notes

  1. Unlike other database drivers, foreign keys don't have name on SQLite, so obviously you can't drop a foreign key by its name on SQLite:

    Schema::table('posts', function (Blueprint $table) {
        $table->dropForeign('posts_user_id_foreign'); // ❌ doesn't work on SQLite
    
        $table->dropForeign(['user_id']);             // βœ”οΈ works as expected
    }
  2. We probably need to remove withSqlite trait on orchestral/testbench package, it seems we don't need that anymore if it's merged?

Copy link

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

@shaedrich
Copy link

Is there a reason, you mixed the SQLite and migration execution order changes into one PR instead of two separate ones?

@ocleroux
Copy link

I'm eager to see this merged!

@hafezdivandari
Copy link
Contributor Author

@shaedrich we can't add SQLite features without fixing the migration commands' orders.

@shaedrich
Copy link

shaedrich commented May 13, 2024

@hafezdivandari You could have created the SQLite PR as draft, added a comment concerning the PR merging order and have backmerged the main branch after the migration command order PR was merged. But in the end, it's Taylor's choice to make

@hafezdivandari
Copy link
Contributor Author

@shaedrich It's fine ;)

@taylorotwell
Copy link
Member

taylorotwell commented May 23, 2024

To be honest this just is too much for us to take on reviewing and maintaining at the moment and I'm not sure the return on investment is there for end users.

Feels like something maybe better reserved for later this year or early next year for Laravel 12.

@hafezdivandari hafezdivandari deleted the 11.x-enhance-schema-migrations branch May 23, 2024 18:58
@hafezdivandari hafezdivandari restored the 11.x-enhance-schema-migrations branch May 23, 2024 18:59
@Gandhi11
Copy link

@taylorotwell How are you guys dropping columns with foreign key in SQLite currently? I do not understand how this is not a major issue for a lot a Laravel applications.

@driesvints
Copy link
Member

Hi @hafezdivandari. I think we should still give a part of your PR a chance but like Taylor said and like we said on the Passport PR keep in mind to keep PR's small and separate because reviewing and testing such a large PR as this is simply too much for us at this time. Sending in incremental PR's with the different topics from this PR will be much much more easier to review.

I feel like the most important part of this bit is the dropping of foreign keys in SQLite. Could we maybe just focus on that one for now so we can get that back into Laravel v11?

Besides that I went through your code changes and while I feel most of this looks okay I do have a question why you decided to move column modifications to different alter lines instead of packing them into a single query statement with add like it does now. Because with this PR it could lead to a head more query statements. Not sure if that's particularly bad but it did caught my eye and I'd like to get your opinion about it.

@hafezdivandari
Copy link
Contributor Author

hafezdivandari commented May 24, 2024

Hi @driesvints,

I think we should still give a part of your PR a chance but like Taylor said and like we said on the Passport PR keep in mind to keep PR's small and separate because reviewing and testing such a large PR as this is simply too much for us at this time. Sending in incremental PR's with the different topics from this PR will be much much more easier to review.

First of all, sorry for sending large PRs πŸ˜… but this one is a little different from the PR on the Passport, separating this into more PRs (if technically possible) doesn't make it easier to review. Because although this PR fixes 2 main problems at the same time, it doesn't mean the changes can necessarily be sent on 2 separate PRs.

But I can add comments on each part of the code changes and explain in more details if you want. IMHO that would be really helpful when reviewing.

I feel like the most important part of this bit is the dropping of foreign keys in SQLite. Could we maybe just focus on that one for now so we can get that back into Laravel v11?

As I tried to explain on this comment earlier and this one, It is not possible to fix the drop foreign on SQLite (or any other commands that needs recreating the table on SQLite) without fixing the "order of commands" problem first.

Besides that I went through your code changes and while I feel most of this looks okay I do have a question why you decided to move column modifications to different alter lines instead of packing them into a single query statement with add like it does now. Because with this PR it could lead to a head more query statements. Not sure if that's particularly bad but it did caught my eye and I'd like to get your opinion about it.

Because preserving the order of commands matters. Please read the "Preserve the order of commands" section above. The rule is simple, compile commands one by one, instead of packing them together and losing the order.

@taylorotwell taylorotwell reopened this May 24, 2024
@driesvints
Copy link
Member

We're gonna have another look at this but it could be a few weeks before we get to it. Thanks!

@gsxdsm
Copy link

gsxdsm commented May 26, 2024

FWIW I’m running into the issue of not being able to drop foreign keys in SQLite and would very much appreciate having this fix in. It’s making a mess of things on my end.

Simounet added a commit to Simounet/flox that referenced this pull request May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Laravel 11 SQLite Migration Failure: after drop column: unknown column "XXXXXX" in foreign key definition
7 participants