Efcore: Add support for renaming primary/foreign keys.

[MicahZoltu]

The issue

There is a MigrationBuilder method for RenameColumn and RenameIndex but there is no method for RenameForeignKey or RenamePrimaryKey. Renaming both the column and the index leaves the foreign key named with its originally created name. I am not a DB expert, but I believe this could just be a helper method for calling DropForeignKey followed by a AddForeignKey with the settings that the original key had (except for the name). I believe the same is true for the primary key.

Further technical details

EF Core version: 1.0.0-rc2-final
Operating system: Windows 10
Visual Studio version: VS 2015

[divega]

We discussed this originally but we decided that there wasn't enough benefit in having a method to rename foreign and primary key constraints vs. dropping them and recreating them. With RenameIndex() for instance, the benefit is that you don't need to rebuild the index if you only wanted to have it with a different name.

[divega]

BTW, we can always consider doing this in the future if we keep getting feedback on it.

[egmfrs]

@divega How can a migration be added to rename foreign key without dropping existing data in the table?

[smitpatel]

@egmfrs - umm, you drop the constraint and recreate it with different name. The underlying columns of constraint are untouched. How is it going to cause existing data loss?

[egmfrs]

@smitpatel your advice is fundamentally sound in generic terms. I actually meant rename a foreign key _column_, and everything with it. I asked on StackOverflow and got the solution for achieving it in EF Core: https://stackoverflow.com/questions/49799627/rename-a-foreign-key-in-entity-framework-core-without-dropping-data

[divega]

@egmfrs looks like a good and simple solution. Thanks for posting it. I up-voted it.

[Brar]

@divega wrote:

We discussed this originally but we decided that there wasn't enough benefit in having a method to rename foreign and primary key constraints vs. dropping them and recreating them.

I just stumbled into an issue where there actually is a benefit in having a method to rename a primary key constraint vs. dropping it and recreating it.

Most database systems won't let you drop a primary key constraint if its column is referenced by a foreign key constraint.

For tables that are heavily referenced by other tables this means dropping and recreating a lot of foreign key constraints just because you want to rename a primary key constraint.

When hand-tuning ef core migrations (MigrationBuilder.RenameTable isn't generated) it is also easy to miss one of the referencing foreign keys which results in a failing migration.

I can't tell if this is enough benefit to consider adding this feature but I think it is a benefit at least.

After all this is probably why you can do this in most database systems (ALTER TABLE [...] RENAME CONSTRAINT [...] in PostgreSQL, sp_rename in SQL Server).

[ajcvickers]

We re-discussed this in triage and decided that it does make sense to support this primitive, assuming there are SQL dialects where this can be done without a rebuild.

[Brar]

While teaching myself som EF Core internals I'm thinking about creating a PR for this.

If I do: Should it be one method for all constraint types like MigrationBuilder.RenameConstraint(...) or one method per constraint type like MigrationBuilder.RenamePrimaryKey(...), MigrationBuilder.RenameUniqueConstraint(...), MigrationBuilder.RenameForeignKey(...)?

Following the system for the Add/Drop-Methods that already exist in MigrationBuilder it should probably be the latter but that would be a lot of duplicate code since the syntax that is generated for renaming is exactly the same for all constraint types (at least for PostgreSQL and SQL server).

[Brar]

I've started drafting an implementation for this using one method per constraint type but I'll certainly change it to use only one method for all constraint types upon request.
You can look at my feature branch if you want to see some code.
I can also make a draft PR if this is more feasible for discussion.

[roji]

@Brar I think (but am not sure) that a method per constraint type is the way to go - this follows the existing separation of constraint types in MigrationBuilder, and it's not inconceivable for another database to either having varying syntax for the different constraints, or even not to support some renames altogether. For the PostgreSQL case all methods can delegate to the same one to prevent duplication.

Note that SQL Server does seem to support renaming primary keys - at the very least - so ideally that would also be part of the PR. I'd be surprised if Sqlite does, though.

[Brar]

@roji

Note that SQL Server does seem to support renaming primary keys - at the very least - so ideally that would also be part of the PR. I'd be surprised if Sqlite does, though.

That's exactly how it is and how I've currently implemented it in my repository.

Actually the way it is suggested in the StackOverflow-Link you provided seems to be wrong as - according to the docs but not explicitly verified by me - the constraint name has to be in the form schema.constraint (sp_rename @objname = N'[dbo].[PK_dbo.Notes]', @newname = N'PK_Notes) instead of [schema.]table.constraint (sp_rename @objname = N'[Notes].[PK_dbo.Notes]', @newname = N'PK_Notes').

I'm actually ready to do a draft pull request if this is OK so we can discuss the actual implementation.
The most relevant part are probably the changes to Microsoft.EntityFrameworkCore.Migrations.Internal.MigrationsModelDiffer with a lot of copy/paste, search/replace code around it (which still should get some review as this is where things go wrong too).

[roji]

Great, feel free to submit a WIP PR and we'll try to review it as soon as possible.