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

[10.x] Add support for native column modifying #45487

Merged
merged 6 commits into from
Jan 4, 2023
Merged

[10.x] Add support for native column modifying #45487

merged 6 commits into from
Jan 4, 2023

Conversation

hafezdivandari
Copy link
Contributor

@hafezdivandari hafezdivandari commented Jan 2, 2023

Related to #45258, this PR adds support for native column modifying and drops necessity of installing doctrine/dbal when modifying columns using change() on MariaDB, MySQL, PostgreSQL and SQL Server.

Supported databases, modifiers and types:

Database Native Support Supported column modifiers Supported column types
MariaDB ✅ (docs) Same as MySQL All types
MySQL ✅ (docs) after, first, autoIncrement, from, storedAs, storedAsJson, virtualAs, virtualAsJson, invisible, unsigned, nullable, default, charset, collation, comment, useCurrent, useCurrentOnUpdate, srid, renameTo2 All types
PostgreSQL ✅ (docs) autoIncrement3, from, storedAs4, generatedAs, always, nullable, default, collation, comment, useCurrent, isGeometry, projection All types except enum5
SQLite ❌ (docs) N/A N/A
SQL Server ✅ (docs) autoIncrement6, nullable, default, collation, persisted, useCurrent All types except enum7

Notes:

  1. According to MySQL docs, for column definition changes using CHANGE or MODIFY, the definition must include the data type and all attributes that should apply to the new column, other than index attributes such as PRIMARY KEY or UNIQUE. Attributes present in the original definition but not specified for the new definition are not carried forward. Suppose that a column col1 is defined as:
    $table->integer('col1')->unsigned()->default(1)->comment('my column'); // `col1` int unsigned not null default '1' comment 'my column'
    and you modify the column as follows, intending to change only INT to BIGINT:
    $table->bigInteger('col1')->change(); // alter table `t1` modify `col1` bigint not null
    That statement changes the data type from INT to BIGINT, but it also drops the UNSIGNED, DEFAULT, and COMMENT attributes. To retain them, the statement must include them explicitly:
    $table->bigInteger('col1')->unsigned()->default(1)->comment('my column')->change(); // alter table `t1` modify `col1` bigint unsigned not null default '1' comment 'my column'
    The same approach is also used on PostgreSQL and SQL Server, you have to redeclare all the attributes you want to keep on the column definition.
  2. Currently Laravel compiles added and modified column before other commands, It means to change and rename a same column you should call renameColumn command after change command:
    $table->enum('difficulty', ['easy', 'hard'])->change();
    $table->renameColumn('difficulty', 'hardness');
    However, this PR adds renameTo column modifier on MySQL to rename and modify a column at one go:
    $table->enum('difficulty', ['easy', 'hard'])->renameTo('hardness')->change();
  3. Laravel compiles autoIncrement on PostgreSQL as primary key and also causes column type to be serial. This PR partially supports autoIncrement, it means the autoIncrement modifier still causes a column type to be serial type and you may use from modifier to set it's starting value, but it doesn't add or drop the primary key. You may use primary() and primary(false) commands explicitly to achieve this.
  4. PostgreSQL doesn't have a native alter command for modifying the expression of a generated column or turning a regular column to a stored generated column, it means storedAs modifier is not supported on this PR. However, it is supported to set storedAs(null) explicitly on PostgreSQL >= 13.0 to drop the generated column expression (turns a stored generated column into a normal base column).
  5. PostgeSQL has native enum but the syntax is different, Laravel uses a check constraint on a varchar when adding an enum type. This PR doesn't handle modifying enum type on PostgreSQL.
  6. Laravel compiles autoIncrement modifier as identity primary key on SQL Server. SQL Server doesn't support adding / dropping identity and primary key when using alter column. So this PR also doesn't support that. You may use primary() and primary(false) commands explicitly to achieve this.
  7. SQL server doesn't have a native enum and Laravel is using a check constraint on a nvarchar when adding an enum type. This PR doesn't support modifying enum on SQL Server.

What if I have already installed doctrine/dbal?

Just like #45258, if you have already installed Doctrine DBAL, you have to call Schema::useNativeSchemaOperationsIfPossible() method within the boot method of your App\Providers\AppServiceProvider class or within your migrations files to be able to use native schema operations.

@hafezdivandari hafezdivandari marked this pull request as ready for review January 2, 2023 21:57
@taylorotwell
Copy link
Member

taylorotwell commented Jan 4, 2023

Can you elaborate on point 3 and point 6 in your PR description? This only applies to changing columns? Can you give some examples of what would work and what wouldn't work?

Laravel compiles autoIncrement on PostgreSQL as primary key and also causes column type to be serial. This PR partially supports autoIncrement, it means the autoIncrement modifier still causes a column type to be serial type and you may use from modifier to set it's starting value, but it doesn't add or drop the primary key. You may use primary and dropPrimary commands explicitly to achieve this.

@hafezdivandari
Copy link
Contributor Author

hafezdivandari commented Jan 4, 2023

Can you elaborate on point 3 and point 6 in your PR description? This only applies to changing columns? Can you give some examples of what would work and what wouldn't work?

@taylorotwell sure. It's not just about modifying columns, currently on creating a table or adding a column, the autoIncrement modifier means both "auto-increment" and "primary key". This may not cause a problem at most cases, but it does in some cases for example in MySql you can do this:

CREATE TABLE users (
     id bigint unsigned not null auto_increment,
     name varchar(255) not null,
     primary key (id, name)
);

but you can't do this on Laravel, because autoIncrement modifier compiles to auto_increment primary key instead of just auto_increment. (I wanted to send another PR maybe to fix this on 10.x)

Ok that was the whole picture, but when you want to modify a column with autoIncrement modifier on MySQL, this is still not a big problem because MySQL accepts this (although that primary key part may not be what the user intended to add):

alter table `users` modify `id` bigint unsigned not null auto_increment primary key

but PostgreSQL and SQL Server doesn't accept that when altering a column. it means you can't control the adding/dropping of a primary key when changing a column. That is what I tried to explain on point 3 and 6. As I said this needs another BC PR to change what autoIncrement modifier means in Laravel when we already have ->primary() to handle that.

PS: Related to this, the SingleStore DB has withoutPrimaryKey modifier as a workaround for this: https://github.com/singlestore-labs/singlestoredb-laravel-driver#increment-columns-without-primary-key

@taylorotwell
Copy link
Member

taylorotwell commented Jan 4, 2023

OK, but for adding columns everything still works as before on 9.x? However, when modifying columns, can you give an example of some actual migration code that would now throw an error after this PR?

@hafezdivandari
Copy link
Contributor Author

hafezdivandari commented Jan 4, 2023

Yes, for adding everything works as before. And when modifying an auto-increment column, no error would be thrown but there are some differences in compare to when adding a column:

Examples of autoIncrement modifier

MySQL / MariaDB

// when adding
$table->integer('id')->autoIncrement(); // alter table `users` add `id` int not null auto_increment primary key
$table->increments('id'); // alias to above

// when modifying
$table->integer('id')->autoIncrement()->change(); // alter table `users` modify `id` int not null auto_increment primary key
$table->increments('id')->change(); // alias to above

✅ Everything works as expected.

PostgreSQL

// when adding
$table->integer('id')->autoIncrement(); // alter table "users" add column "id" serial not null primary key
$table->increments('id'); // alias to above

// when modifying
$table->integer('id')->autoIncrement()->change(); // alter table "users" alter column "id" type serial, alter column "id" set not null, alter column "id" drop default, alter column "id" drop identity if exists; comment on column "users"."id" is NULL
$table->increments('id')->change(); // alias to above

⚠️ The column type is serial as expected, but we don't handle the primary key.

SQL Server

// when adding
$table->integer('id')->autoIncrement(); // alter table "users" add "id" int not null identity primary key
$table->increments('id'); // alias to above

// when modifying
$table->integer('id')->autoIncrement()->change(); // alter table "users" alter column "id" int not null
$table->increments('id')->change(); // alias to above

❌ Both identity and primary key are missing and autoIncrement is ignored.

@taylorotwell
Copy link
Member

OK - this is a pretty risky PR I think but I'm willing to take a chance on it since it would be nice to not have doctrine/dbal for changing columns.

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.

2 participants