Add rename primary and foreign key operation

[Brar]

Addresses #5662

I've kept separate commits for the three different constraint types but will squash them upon request.

• The code builds
• All but 2 tests pass (verified by running .\build.cmd -test on my system). The 2 failing tests seem to be completely unrelated to my changes as they also fail on a clean master checkout on my system:
  • Microsoft.EntityFrameworkCore.Scaffolding.SqlServerDatabaseModelFactoryTest.Default_value_matching_clr_default_is_not_stored fails for IgnoredDefault24 float NOT NULL DEFAULT 0.0E0 where null is expected and the actual value is 0.000000000000000e+000
  • Microsoft.EntityFrameworkCore.Scaffolding.SqlServerDatabaseModelFactoryTest.Hidden_columns_are_not_created throws an SqlException : Incorrect syntax near 'GENERATED'.\r\nIncorrect syntax near 'HIDDEN'.\r\nIncorrect syntax near the keyword 'with'. If this statement is a common table expression, an xmlnamespaces clause or a change tracking context clause, the previous statement must be terminated with a semicolon.
• Commit messages currently don't follow this format as they aren't finalized yet

    Summary of the changes
    - Detail 1
    - Detail 2

    Fixes #bugnumber

• Tests for the changes have been added (for bug fixes / features)
• Code meets the expectations our engineering guidelines. https://github.com/aspnet/Home/wiki/Engineering-guidelines.

Please review the guidelines for CONTRIBUTING.md for more details.

[Brar]

Sorry this was meant to be a Draft pull request but I accidentally created a normal one and it appears that you cant change this after it has been created (I've actually already bugged @natfriedman about this on twitter).

[Brar]

I've rebased this because it didn't apply cleanly anymore.
As rebasing while keeping several commits in order is a pain, I've also squashed the commits into one single commit.

[bricelam]

Looks good. Just a few comments...

[ajcvickers]

@Brar Just wanted to let you know that we haven't forgotten about this. However, due to other high-priority work it make take a bit of time for us to get back to this.

[Brar]

@ajcvickers thanks, no worries.
I'll try to come up with a suggestion addressing the comments.
We can pick up the discussion at that point as soon as you get to it.

[bricelam]

Also remember that today they generate drop and re-create operations so they would be no more broken than they already are. We have a separate issue to perform table and constraint rebuilds automatically as part of a migration.

[Brar]

Ok, this is my suggestion for further discussions.
There are still some areas in the code, where I'm uncertain, if my chosen solution is the best way to do it or even appropriate.
I've commented them in the code but I'll also list them here so that they don't get lost.

1. I had to move an IMigrationsAnnotationProvider down one level in the inheritance chain to src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs in order to be able to create annotations at this level. Please also look at the default instance and its MigrationsAnnotationProviderDependencies.
2. The default implementation of RenameForeignKeyOperation in src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs will reset the OnUpdate ReferentialAction to its default value.
3. I decided to still throw from the Generate overrides for the rename operations in src/EFCore.Sqlite.Core/Migrations/SqliteMigrationsSqlGenerator.cs because Sqlite doesn't currently support the add operations for primary/foreign keys and unique constraints so drop/create isn't an option here until Sqlite Migrations: Table rebuilds #329 has landed.
4. I'm uncertain if the exception message thrown from the Generate methods for the rename operations in src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs is the right one.

[Brar]

@ajcvickers @bricelam Now that 3.1 is out and you are beginning to work on 5.0 I assume that it's a good time for me to lobby for this PR again.
I'm willing to rebase it once again if there is time and dedication to review and possibly merge it in a timely manner.
Please go ahead and tell me if I should wait a litte and when it would best fit into your schedule. I just want to avoid working into the void.

cc: @roji

[bricelam]

I hope to get this in before the first preview of of 5.0.0. It might be early January before I get around to it. (I'm currently writing some docs to support the 3.1 release.)

[Brar]

Thanks @bricelam!
No hurry.
I'll rebase it in the next few weeks so that it's ready for re-review by January.

[Brar]

@bricelam I've rebased this as I had some spare time today.
Don't feel pressured - January is fine!

[bricelam]

Just need to clean up the breaking changes and use our dependency object patterns.

[roji]

New migration tests look great to me, thanks @Brar!

I think we can drop the ModelDiffer and MigrationSqlGenerator tests - the new migration tests exercise all those components as well, so these don't add any value. We still have these two test suites for tests which for some reason cannot be tested in MigrationsTestBase (#19668 is about cleaning that up). @bricelam can you confirm you're OK with that?

Other notes:

• Some tests are failing (at the very least some SQL baselines need updating)
• If we're adding rename support for constraints, we may as well do the same for checked constraints...
• I haven't reviewed anything else here other than the tests

[Brar]

Some tests are failing (at the very least some SQL baselines need updating)

Sorry this was my fault.
After 3 Visual Studio crashes while trying to run more then one test at once, I decided to push this and hope for the best, which - as we all know - never goes well.

I think we can drop the ModelDiffer and MigrationSqlGenerator tests

I haven't removed anything yet, but I can certainly do so. Just waiting for @bricelam to confirm that he's OK with that.

If we're adding rename support for constraints, we may as well do the same for checked constraints

👍

[bricelam]

Sounds good to me

[Brar]

Hi @bricelam and @roji
sorry for the delay.
A skiing-holiday and my day-job threw me back a little but now I think I've addressed all your feedback.

While most of the changes are pretty straightforward, I urge you to have a closer look at the last commit regarding the warning.
I'm by no means confident, that what I did there is the way it should be done.

[smitpatel]

skiing-holiday

@Brar - Skiiing always takes priority over anything else.

[Brar]

Ok @bricelam, I've rebased this once again.
Actually I've deemed it ready for another review since Feb 22 but I'm not sure that this has really come across.
As someone told me, the logging part is a no-go-area that only @ajcvickers dares to enter so he might want to have a look at the last commit where I attempt to do something I don't completely understand.

[Brar]

Since this has been inactive for quite a while and I don't think I can resume working on this any time soon I'm closing this now.