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

Invalid SQL generated when changing the primary key in migration with .AddPrimaryKey(...) #1348

Open
Mertsch opened this issue Mar 15, 2021 · 8 comments
Assignees
Labels
Milestone

Comments

@Mertsch
Copy link

Mertsch commented Mar 15, 2021

Steps to reproduce

See code in https://github.com/Mertsch/PomeloPrimaryKeyBug

  1. Have a primary key
  2. Add migration
  3. Change primary key
  4. Add migration
  5. Execute migrations (get exception on the 2nd)

The issue

The migration step

migrationBuilder.AddPrimaryKey(
                name: "PK_tasks_workers",
                table: "tasks_workers",
                columns: new[] { "TrekkTemplateId", "WorkerId" });

generates the SQL statement

ALTER TABLE `tasks_workers` ADD CONSTRAINT `TemplateContainsTemplateWorkers`
 FOREIGN KEY (`TrekkTemplateId`) -- Correct, this is the actual column name
 REFERENCES `tasks` (`TrekkTemplateId`) -- Incorrect!, this is the column name from FOREIGN KEY, not the real one.
 ON DELETE CASCADE;

Exception

Failed executing DbCommand (25ms) [Parameters=[], CommandType='Text', CommandTimeout='30']
ALTER TABLE `tasks_workers` ADD CONSTRAINT `TemplateContainsTemplateWorkers` FOREIGN KEY (`TrekkTemplateId`) REFERENCES `tasks` (`TrekkTemplateId`) ON DELETE CASCADE;
MySqlConnector.MySqlException (0x80004005): Can't create table `repobugdb`.`tasks_workers` (errno: 150 "Foreign key constraint is incorrectly formed")
 ---> MySqlConnector.MySqlException (0x80004005): Can't create table `repobugdb`.`tasks_workers` (errno: 150 "Foreign key constraint is incorrectly formed")
   at MySqlConnector.Core.ResultSet.ReadResultSetHeaderAsync(IOBehavior ioBehavior) in /_/src/MySqlConnector/Core/ResultSet.cs:line 49
   at MySqlConnector.MySqlDataReader.ActivateResultSet(CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlDataReader.cs:line 131
   at MySqlConnector.MySqlDataReader.CreateAsync(CommandListPosition commandListPosition, ICommandPayloadCreator payloadCreator, IDictionary`2 cachedProcedures, IMySqlCommand command, CommandBehavior behavior, IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlDataReader.cs:line 436
   at MySqlConnector.Core.CommandExecutor.ExecuteReaderAsync(IReadOnlyList`1 commands, ICommandPayloadCreator payloadCreator, CommandBehavior behavior, IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/Core/CommandExecutor.cs:line 60
   at MySqlConnector.MySqlCommand.ExecuteNonQueryAsync(IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlCommand.cs:line 264
   at MySqlConnector.MySqlCommand.ExecuteNonQuery() in /_/src/MySqlConnector/MySqlCommand.cs:line 100
   at Microsoft.EntityFrameworkCore.Storage.RelationalCommand.ExecuteNonQuery(RelationalCommandParameterObject parameterObject)
   at Microsoft.EntityFrameworkCore.Migrations.MigrationCommand.ExecuteNonQuery(IRelationalConnection connection, IReadOnlyDictionary`2 parameterValues)
   at Microsoft.EntityFrameworkCore.Migrations.Internal.MigrationCommandExecutor.ExecuteNonQuery(IEnumerable`1 migrationCommands, IRelationalConnection connection)
   at Microsoft.EntityFrameworkCore.Migrations.Internal.Migrator.Migrate(String targetMigration)
   at Microsoft.EntityFrameworkCore.Design.Internal.MigrationsOperations.UpdateDatabase(String targetMigration, String connectionString, String contextType)
   at Microsoft.EntityFrameworkCore.Design.OperationExecutor.UpdateDatabaseImpl(String targetMigration, String connectionString, String contextType)
   at Microsoft.EntityFrameworkCore.Design.OperationExecutor.UpdateDatabase.<>c__DisplayClass0_0.<.ctor>b__0()
   at Microsoft.EntityFrameworkCore.Design.OperationExecutor.OperationBase.Execute(Action action)

Further technical details

MySQL version: 10.4.17-MariaDB - mariadb.org binary distribution
Operating system: Win 10 x64
Pomelo.EntityFrameworkCore.MySql version: 5.0.0-alpha.2
Microsoft.AspNetCore.App version: 5.0.4

@lauxjpn
Copy link
Collaborator

lauxjpn commented Apr 2, 2021

I am able to reproduce this with 5.0.0-alpha.2. However, it looks like we already fixed this issue in the nightly builds.
@Mertsch Please try the latest nightly build and try-out, if the issue still happens.

@Mertsch
Copy link
Author

Mertsch commented Apr 6, 2021

@lauxjpn I have tried 5.0.0-alpha.3.ci.20210405222846 and the error in the sample repo has been replaced by a different error
I dropped the DB, but kept the migrations .cs files as is, recreated all migrations (which has exactly the same code)

Applying migration '20210315180016_InititalMigration'.
Applying migration '20210315180105_ChangeDbTrekkTemplateUserKey'.
Failed executing DbCommand (3ms) [Parameters=[], CommandType='Text', CommandTimeout='30']
ALTER TABLE `tasks_workers` DROP INDEX `IX_tasks_workers_TrekkTemplateId`;
MySqlConnector.MySqlException (0x80004005): Cannot drop index 'IX_tasks_workers_TrekkTemplateId': needed in a foreign key constraint
 ---> MySqlConnector.MySqlException (0x80004005): Cannot drop index 'IX_tasks_workers_TrekkTemplateId': needed in a foreign key constraint
   at MySqlConnector.Core.ResultSet.ReadResultSetHeaderAsync(IOBehavior ioBehavior) in /_/src/MySqlConnector/Core/ResultSet.cs:line 49
   at MySqlConnector.MySqlDataReader.ActivateResultSet(CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlDataReader.cs:line 131
   at MySqlConnector.MySqlDataReader.CreateAsync(CommandListPosition commandListPosition, ICommandPayloadCreator payloadCreator, IDictionary`2 cachedProcedures, IMySqlCommand command, CommandBehavior behavior, IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlDataReader.cs:line 436
   at MySqlConnector.Core.CommandExecutor.ExecuteReaderAsync(IReadOnlyList`1 commands, ICommandPayloadCreator payloadCreator, CommandBehavior behavior, IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/Core/CommandExecutor.cs:line 60
   at MySqlConnector.MySqlCommand.ExecuteNonQueryAsync(IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlCommand.cs:line 264
   at MySqlConnector.MySqlCommand.ExecuteNonQuery() in /_/src/MySqlConnector/MySqlCommand.cs:line 100
   at Microsoft.EntityFrameworkCore.Storage.RelationalCommand.ExecuteNonQuery(RelationalCommandParameterObject parameterObject)
   at Microsoft.EntityFrameworkCore.Migrations.MigrationCommand.ExecuteNonQuery(IRelationalConnection connection, IReadOnlyDictionary`2 parameterValues)
   at Microsoft.EntityFrameworkCore.Migrations.Internal.MigrationCommandExecutor.ExecuteNonQuery(IEnumerable`1 migrationCommands, IRelationalConnection connection)
   at Microsoft.EntityFrameworkCore.Migrations.Internal.Migrator.Migrate(String targetMigration)
   at Microsoft.EntityFrameworkCore.Design.Internal.MigrationsOperations.UpdateDatabase(String targetMigration, String connectionString, String contextType)
   at Microsoft.EntityFrameworkCore.Design.OperationExecutor.UpdateDatabaseImpl(String targetMigration, String connectionString, String contextType)
   at Microsoft.EntityFrameworkCore.Design.OperationExecutor.UpdateDatabase.<>c__DisplayClass0_0.<.ctor>b__0()
   at Microsoft.EntityFrameworkCore.Design.OperationExecutor.OperationBase.Execute(Action action)
Cannot drop index 'IX_tasks_workers_TrekkTemplateId': needed in a foreign key constraint

I have updated the same repo accordingly

@lauxjpn
Copy link
Collaborator

lauxjpn commented Apr 6, 2021

The issue now is the following call in your second migrations Up() method:

migrationBuilder.DropIndex(
    name: "IX_tasks_workers_TrekkTemplateId",
    table: "tasks_workers");

This call shouldn't be there, but appears to be generated by the EF Core differ for some inexplicable reason.

So as a quick workaround, remove the line from the Up() method, and your code should run fine.

The following sample code reproduces the issue for MySQL and SQL Server:

using System;
using System.Collections.Generic;
using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.Logging;

namespace IssueConsoleTemplate
{
    public class IceCream
    {
        public int IceCreamId { get; set; }

        public ICollection<IceCream_IceCreamParlor> IceCreamIceCreamParlors { get; set; }
    }

    public class IceCreamParlor
    {
        public int IceCreamParlorId { get; set; }

        public ICollection<IceCream_IceCreamParlor> IceCreamIceCreamParlors{ get; set; }
    }
    
    public class IceCream_IceCreamParlor
    {
#if !SECOND_MIGRATION
        public int AccidentalId { get; set; } // <-- Remove for the second migration
#endif
        public int IceCreamId { get; set; }
        public int IceCreamParlorId { get; set; }
        
        public IceCream IceCream { get; set; }
        public IceCreamParlor IceCreamParlor { get; set; }
    }

    public class Context : DbContext
    {
        public DbSet<IceCream> IceCreams { get; set; }
        public DbSet<IceCreamParlor> IceCreamParlors { get; set; }

        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            if (!optionsBuilder.IsConfigured)
            {
                // optionsBuilder.UseMySql("server=127.0.0.1;port=3306;user=root;password=;database=Issue1348_02", new MySqlServerVersion(new Version(8, 0, 21))) 
                optionsBuilder.UseSqlServer(@"Data Source=.\MSSQL14;Integrated Security=SSPI;Initial Catalog=Issue1348_02") 
                    .UseLoggerFactory(
                        LoggerFactory.Create(
                            configure => configure
                                .AddConsole()
                                .AddFilter(level => level >= LogLevel.Information)))
                    .EnableSensitiveDataLogging()
                    .EnableDetailedErrors();
            }
        }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            modelBuilder.Entity<IceCream_IceCreamParlor>(
                entity =>
                {
#if !SECOND_MIGRATION
                    entity.HasKey(e => e.AccidentalId); // <-- Remove for second migration
#else
                    entity.HasKey(e => new {e.IceCreamId, e.IceCreamParlorId}); // <-- Add for second migration
#endif              
                    // WORKAROUND:
                    // Explicitly defining the index will prevent EF Core from dropping it later.
                    // entity.HasIndex(e => e.IceCreamId);
                    
                    // This is never needed, because this index is never being dropped by EF Core.
                    // entity.HasIndex(e => e.IceCreamParlorId);

                    entity.HasOne(e => e.IceCream)
                        .WithMany()
                        .HasForeignKey(e => e.IceCreamId);

                    entity.HasOne(e => e.IceCreamParlor)
                        .WithMany()
                        .HasForeignKey(e => e.IceCreamParlorId);
                });
        }
    }

    internal static class Program
    {
        private static void Main(string[] args)
        {
            using var context = new Context();

            context.Database.EnsureDeleted();
            context.Database.EnsureCreated();
        }
    }
}

Run the following PowerShell command from the project directory, to generate the two migrations:

rmdir .\Migrations\ -Recurse -ErrorAction SilentlyContinue; dotnet build '-p:DefineConstants=DEBUG NET5_0 NET'; dotnet ef migrations add Initial --verbose --no-build; dotnet build '-p:DefineConstants=DEBUG NET5_0 NET SECOND_MIGRATION'; dotnet ef migrations add ChangePrimaryKey --verbose --no-build

In this sample code, the unexpected call, that drops the index that is unrelated to the primary key, is the following from the ..._ChangePrimaryKey.cs file:

protected override void Up(MigrationBuilder migrationBuilder)
{
    migrationBuilder.DropPrimaryKey(
        name: "PK_IceCream_IceCreamParlor",
        table: "IceCream_IceCreamParlor");

    //
    // This call should not be here:
    //
    migrationBuilder.DropIndex(
        name: "IX_IceCream_IceCreamParlor_IceCreamId",
        table: "IceCream_IceCreamParlor");

    migrationBuilder.DropColumn(
        name: "AccidentalId",
        table: "IceCream_IceCreamParlor");

    migrationBuilder.AddPrimaryKey(
        name: "PK_IceCream_IceCreamParlor",
        table: "IceCream_IceCreamParlor",
        columns: new[] { "IceCreamId", "IceCreamParlorId" });
}

It tries to drop the index of the IceCreamId column, even though it is the AccidentalId column that is being relieved from being the primary key, before it is later dropped completely.

This does not happen if the index has been explicitly defined in the model.

This issue does not seem to have any negative side effects for SQL Server when the generated SQL drops the index, but MySQL is not as forgiving and returns an error if you try to drop an index, that is being used for an existing foreign key relationship.

/cc @ajcvickers @bricelam

@bricelam
Copy link

Can you submit a new issue on dotnet/efcore so we can investigate further?

@bricelam
Copy link

This provider may need to be responsible for rebuilding the foreign keys (similar to dotnet/efcore#12586), but I want to make sure I fully understand the issue first.

@lauxjpn
Copy link
Collaborator

lauxjpn commented Apr 12, 2021

This provider may need to be responsible for rebuilding the foreign keys (similar to dotnet/efcore#12586), but I want to make sure I fully understand the issue first.

@bricelam I'll open an issue on the EF Core repo. The issue is, that the index gets dropped in the first place. It is the wrong index (it has nothing to do with the PK that is being dropped).

@lauxjpn
Copy link
Collaborator

lauxjpn commented Sep 23, 2021

The migration operation order is not going to change in EF Core, so we will implement a Pomelo specific fix for this.

@lauxjpn lauxjpn added this to the 6.0.0 milestone Sep 23, 2021
@lauxjpn lauxjpn modified the milestones: 6.0.0, 7.0.0 Nov 9, 2021
@lauxjpn lauxjpn modified the milestones: 7.0.0, 8.0.0 Jan 16, 2023
@lauxjpn lauxjpn modified the milestones: 8.0.0, 9.0.0 Mar 16, 2024
@andreiTuduce
Copy link

Are there any news to this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants