dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.73k stars 3.18k forks source link

SQL Server Migrations: Idempotent scripts fails with 'invalid column name' (needs EXEC) #12911

Closed christianholsen closed 4 years ago

christianholsen commented 6 years ago

I have encountered a problem with idempotent migration scripts that make them fail in our continous integration system. The reason is that some migration scripts are parsed even though they are not going to be executed.

This issue is much like issue #10717. This issue can be reproduced by manipulating EF types i a sequence of migrations.

In example I have these two migrations, the first is adding a unique index for a nullable column, the second is removing the column:

IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20180519095519_AddIndexToMyTable')
BEGIN
        CREATE UNIQUE INDEX [NameIndex] ON [MyTable] ([Name]) WHERE [Name] IS NOT NULL;
END;

IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20180720070812_RemoveNameFromMyTable')
BEGIN
    DROP INDEX [NameIndex] ON [MyTable];
END;

GO

IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20180720070812_RemoveNameFromMyTable')
BEGIN
    DECLARE @var19 sysname;
    SELECT @var19 = [d].[name]
    FROM [sys].[default_constraints] [d]
    INNER JOIN [sys].[columns] [c] ON [d].[parent_column_id] = [c].[column_id] AND [d].[parent_object_id] = [c].[object_id]
    WHERE ([d].[parent_object_id] = OBJECT_ID(N'[MyTable]') AND [c].[name] = N'Name');
    IF @var19 IS NOT NULL EXEC(N'ALTER TABLE [MyTable] DROP CONSTRAINT [' + @var19 + '];');
    ALTER TABLE [MyTable] DROP COLUMN [Name];
END;
GO

This will work fine in the first execution, just as #10717

The second time this in run in our CI system, MyTable will no longer have the column "Name" and will fail in the execution of

IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20180519095519_AddIndexToMyTable')
BEGIN
        CREATE UNIQUE INDEX [NameIndex] ON [MyTable] ([Name]) WHERE [Name] IS NOT NULL;
END;

The SQL error message is

Invalid column name 'Name'.

This happens even though the migration "AddIndexToMyTable" has been installed and the "If not exists.." statement should avoid execution of the script, but as i happens it is parsed anyways, making it fail!

Steps to reproduce

1) Create a entity type with a nullable field with a unique index using fluent API

protected override void OnModelCreating(ModelBuilder builder)
        {
            builder.Entity<MyTable>().HasIndex(c => c.Name).IsUnique();
        }

2) Make a new migration 3) Remove the field Name from MyTable and remove the unique index in OnModelCreating 4) Make a new migration 5) Create an idempotent migration script with

dotnet ef migrations script -o migrationscript.sql --startup-project MyProject.csproj --configuration release --idempotent

6) execute migrationscript.sql twice on the database making it fail

Proposal for a solution

This problem only occurs because the script section is parsed in the sql server even though it is not going to be executed. If this could be avoided the problem would not occur. It could be solved by using dynamic a sql script, as:

IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20180519095519_AddIndexToMyTable')
BEGIN
        EXEC('CREATE UNIQUE INDEX [NameIndex] ON [MyTable] ([Name]) WHERE [Name] IS NOT NULL;')
END;

As it is i'm adding the exec to the migration script with powershell in CI, but I believe that everyone would be happier if we could rely on the script produced by EFCore :-)

Further technical details

EF Core version: 2.1.1

ajcvickers commented 6 years ago

Note for triage: I was able to reproduce this both in SQL Management Studio and when running the commands directly from ADO.NET:

Migrating once:
Migrating twice:

Unhandled Exception: System.Data.SqlClient.SqlException: Invalid column name 'Name'.
   at System.Data.SqlClient.SqlConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
   at System.Data.SqlClient.TdsParser.ThrowExceptionAndWarning(TdsParserStateObject stateObj, Boolean callerHasConnectionLock, Boolean asyncClose)
   at System.Data.SqlClient.TdsParser.TryRun(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataReader dataStream, BulkCopySimpleResultSet bulkCopyHandler, TdsParserStateObject stateObj, Boolean& dataReady)
   at System.Data.SqlClient.SqlCommand.RunExecuteNonQueryTds(String methodName, Boolean async, Int32 timeout, Boolean asyncWrite)
   at System.Data.SqlClient.SqlCommand.InternalExecuteNonQuery(TaskCompletionSource`1 completion, Boolean sendToPipe, Int32 timeout, Boolean asyncWrite, String methodName)
   at System.Data.SqlClient.SqlCommand.ExecuteNonQuery()
   at Program.ApplyMigrations(DbConnection connection) in C:\Stuff\TwoOneCore\TwoOneCore\Program.cs:line 210
   at Program.Main() in C:\Stuff\TwoOneCore\TwoOneCore\Program.cs:line 167

ADO.NET repro code:

public class BloggingContext : DbContext
{
    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder
            .UseSqlServer(@"Server=(localdb)\mssqllocaldb;Database=Test;ConnectRetryCount=0");
}

public class Program
{
    private static readonly string[] migrations =
    {
        @"IF OBJECT_ID(N'[__EFMigrationsHistory]') IS NULL
BEGIN
    CREATE TABLE [__EFMigrationsHistory] (
        [MigrationId] nvarchar(150) NOT NULL,
        [ProductVersion] nvarchar(32) NOT NULL,
        CONSTRAINT [PK___EFMigrationsHistory] PRIMARY KEY ([MigrationId])
    );
END;
",
        @"IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20180810192355_Initial')
BEGIN
    CREATE TABLE [Blog] (
        [Id] int NOT NULL IDENTITY,
        CONSTRAINT [PK_Blog] PRIMARY KEY ([Id])
    );
END;
",
        @"IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20180810192355_Initial')
BEGIN
    CREATE TABLE [Post] (
        [Id] int NOT NULL IDENTITY,
        [BlogId] int NULL,
        CONSTRAINT [PK_Post] PRIMARY KEY ([Id]),
        CONSTRAINT [FK_Post_Blog_BlogId] FOREIGN KEY ([BlogId]) REFERENCES [Blog] ([Id]) ON DELETE NO ACTION
    );
END;
",
        @"IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20180810192355_Initial')
BEGIN
    CREATE INDEX [IX_Post_BlogId] ON [Post] ([BlogId]);
END;
",
        @"IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20180810192355_Initial')
BEGIN
    INSERT INTO [__EFMigrationsHistory] ([MigrationId], [ProductVersion])
    VALUES (N'20180810192355_Initial', N'2.1.1-rtm-30846');
END;
",
        @"IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20180810192452_AddName')
BEGIN
    ALTER TABLE [Blog] ADD [Name] nvarchar(max) NULL;
END;
",
        @"IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20180810192452_AddName')
BEGIN
    INSERT INTO [__EFMigrationsHistory] ([MigrationId], [ProductVersion])
    VALUES (N'20180810192452_AddName', N'2.1.1-rtm-30846');
END;
",
        @"IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20180810192552_AddIndex')
BEGIN
    DECLARE @var0 sysname;
    SELECT @var0 = [d].[name]
    FROM [sys].[default_constraints] [d]
    INNER JOIN [sys].[columns] [c] ON [d].[parent_column_id] = [c].[column_id] AND [d].[parent_object_id] = [c].[object_id]
    WHERE ([d].[parent_object_id] = OBJECT_ID(N'[Blog]') AND [c].[name] = N'Name');
    IF @var0 IS NOT NULL EXEC(N'ALTER TABLE [Blog] DROP CONSTRAINT [' + @var0 + '];');
    ALTER TABLE [Blog] ALTER COLUMN [Name] nvarchar(450) NULL;
END;
",
        @"IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20180810192552_AddIndex')
BEGIN
    CREATE UNIQUE INDEX [IX_Blog_Name] ON [Blog] ([Name]) WHERE [Name] IS NOT NULL;
END;
",
        @"IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20180810192552_AddIndex')
BEGIN
    INSERT INTO [__EFMigrationsHistory] ([MigrationId], [ProductVersion])
    VALUES (N'20180810192552_AddIndex', N'2.1.1-rtm-30846');
END;
",
        @"IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20180810192708_RemoveName')
BEGIN
    DROP INDEX [IX_Blog_Name] ON [Blog];
END;
",
        @"IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20180810192708_RemoveName')
BEGIN
    DECLARE @var1 sysname;
    SELECT @var1 = [d].[name]
    FROM [sys].[default_constraints] [d]
    INNER JOIN [sys].[columns] [c] ON [d].[parent_column_id] = [c].[column_id] AND [d].[parent_object_id] = [c].[object_id]
    WHERE ([d].[parent_object_id] = OBJECT_ID(N'[Blog]') AND [c].[name] = N'Name');
    IF @var1 IS NOT NULL EXEC(N'ALTER TABLE [Blog] DROP CONSTRAINT [' + @var1 + '];');
    ALTER TABLE [Blog] DROP COLUMN [Name];
END;
",
        @"IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20180810192708_RemoveName')
BEGIN
    INSERT INTO [__EFMigrationsHistory] ([MigrationId], [ProductVersion])
    VALUES (N'20180810192708_RemoveName', N'2.1.1-rtm-30846');
END;
"
    };

    public static void Main()
    {
        using (var context = new BloggingContext())
        {
            context.Database.EnsureDeleted();
            context.Database.EnsureCreated();

            var connection = context.Database.GetDbConnection();

            Console.WriteLine("Migrating once:");
            ApplyMigrations(connection);

            Console.WriteLine("Migrating twice:");
            ApplyMigrations(connection);
        }
   }

    private static void ApplyMigrations(DbConnection connection)
    {
        connection.Open();
        foreach (var migration in migrations)
        {
            var command = connection.CreateCommand();
            command.CommandText = migration;
            command.ExecuteNonQuery();
        }

        connection.Close();
    }
}
ajcvickers commented 6 years ago

Triage: for 3.0, we will investigate the minimal set of places where we have to wrap in an exec call to avoid the SQL Server parsing issue. Worst case, we will have to do it everywhere. Note that this should only be done when generating idempotent scripts.

tonven commented 6 years ago

@christianholsen can you paste a gist with a powershell script that you use to prepare migration file? Thanks in advance.

christianholsen commented 6 years ago

Yep. As a wrote above the only problem I found was the specific scenario specific where I have created an index for a nullable column and later remove it. The problem arises when the CREATE UNIQUE INDEX statement is evaluated even though it is not executed. So I have chosen to wrap all CREATE UNIQUE INDEX statements in the SQL in EXEC blocks. This has the effect that the script is dynamic and will only be evaluated when it actually is executed - fixing the immediate problem.

I use Team City as my build tools. You should be able to use this approach with any build tool.

I have a build step where I create the idempotent SQL file as mentioned in my first post. This step is executed as a command line step using the dotnet cli : dotnet ef migration script...etc (see my first post) This creates the idempotent script 'migrationscript.sql'

Next I have a powershell script step where I replace CREATE UNIQUE INDEX using regular expressions. The code is here:

<#

Replace all 
CREATE UNIQUE INDEX [...]
with 
EXEC('CREATE UNIQUE INDEX [...]')
in migrationscript.sql

#>
$regexA = '\s*(CREATE UNIQUE INDEX.+)'
$encoding = New-Object System.Text.UTF8Encoding
$invocation = (Get-Variable MyInvocation).Value
Get-ChildItem  "migrationscript.sql" | % {
  $c = (Get-Content $_.FullName) -replace $regexA,'EXEC(''$0'')' -join "`r`n"
  [IO.File]::WriteAllText("$((Get-Item -Path ".\").FullName)\\migrationscript.sql", $c, $encoding)
}
Write-Host ("migrationscript.sql has been fixed")

As I understand it this problem will be fixed at some point in EF CORE, so this is working fine for me for now. :-)

Hope this helps, /Christian

tonven commented 6 years ago

@christianholsen Thanks for response. I found different solution. I am producing all migrations separately and in Octopus Deploy run only those migrations, that were not applied. So I am waiting as well when this will be fixed, so I can switch back to migrations.sql. :)

Kukkimonsuta commented 6 years ago

We are experiencing the same issue with hand written updates after column was removed/renamed: migrationBuilder.Sql(@"UPDATE [Foo] SET [Bar] = 15 WHERE [Bar] = 10"); - it would be great if this was also wrapped when generating a script.

mitchelsellers commented 5 years ago

We are also experiencing this issue. The workaround that @christianholsen used seems to have gotten us past this, as we are using Azure DevOps for deployments we were able to patch the script before we release.

acarrau commented 5 years ago

The same is occurring to us with Idempotent Script Generation. In our case it is not happening with an INDEX creation, but with an IDENTITY INSERT.

Shtong commented 5 years ago

Another project where this problem occurs (without involving indexes) is the current version of the Contoso University sample app (available here https://github.com/aspnet/Docs/tree/0ee6b101d9d4b4022add3583ed25f0d89674b87b/aspnetcore/data/ef-mvc/intro/samples/cu-final). The Inheritance migration creates a temporary column (OldID) that is created and immediately removed in the same migration ; and after the first run, the database rejects the itempotent migration script because the column does not exist.

Unfortunately in this case, wrapping the instructions in an EXEC call with a simple regex replace won't be enough, as we would need to escape the quotes inside the EXEC argument, like in that case:

IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20170816195930_Inheritance')
BEGIN
    UPDATE dbo.Enrollment SET StudentId = (SELECT ID FROM dbo.Person WHERE OldId = Enrollment.StudentId AND Discriminator = 'Student')
END;
adria3a3 commented 5 years ago

I'm having the same problem with the migrations. When the idempotent flag is set not all statements are wrapped with a EXEC statement and the migration fails.

It there an ETA know for this issue to be resolved?

bricelam commented 5 years ago

We still hope to address this issue before the final 3.0.0 release (this September).

jacqueskang commented 5 years ago

We are having this issue too and it would be nice to have it fixed for 2.x too

mitchelsellers commented 5 years ago

For those using Azure DevOps, we did release a set of .NET Core Pipeline tasks. A temporary workaround for this issue is included as an option in our tasks that script the migrations (https://marketplace.visualstudio.com/items?itemName=iowacomputergurus.dotnetcore-pipeline-tasks)

Just a workaround until this can be truly fixed

ghost commented 5 years ago

Hi,

Another workaround is to globally wrap SQL scripts with EXECUTE() statement. In case of someone looking for such an approach here is very dumb (roughly tested) implementation:

    public class DynamicSqlRelationalCommandBuilder : RelationalCommandBuilder
    {
        public DynamicSqlRelationalCommandBuilder(IDiagnosticsLogger<DbLoggerCategory.Database.Command> logger, IRelationalTypeMappingSource typeMappingSource) : base(logger, typeMappingSource)
        {
        }

        protected override IRelationalCommand BuildCore(IDiagnosticsLogger<DbLoggerCategory.Database.Command> logger, string commandText, IReadOnlyList<IRelationalParameter> parameters)
        {
            commandText = "EXECUTE ('" + commandText.Replace("'", "''") + "')";
            return base.BuildCore(logger, commandText, parameters);
        }
    }
    public class DynamicSqlRelationalCommandBuilderFactory : RelationalCommandBuilderFactory
    {
        public DynamicSqlRelationalCommandBuilderFactory(IDiagnosticsLogger<DbLoggerCategory.Database.Command> logger, IRelationalTypeMappingSource typeMappingSource) : base(logger, typeMappingSource)
        {
        }

        protected override IRelationalCommandBuilder CreateCore(IDiagnosticsLogger<DbLoggerCategory.Database.Command> logger,
            IRelationalTypeMappingSource relationalTypeMappingSource)
        {
            return new DynamicSqlRelationalCommandBuilder(logger, relationalTypeMappingSource);
        }
    }

Then somewhere in your IDesignTimeDbContextFactory implementation: options.ReplaceService<IRelationalCommandBuilderFactory, DynamicSqlRelationalCommandBuilderFactory>();

oising commented 5 years ago

I came here looking for issues around creating VIEWs and TRIGGERs with an --idempotent flavoured migration script, and I get a different error, albeit should probably be thrown into the same bucket as this one. The script generated by dotnet ef migrations script ... won't parse at all because of the way it groups CREATE VIEW and CREATE TRIGGER statements.

According to the SQL laws laid down by Microsoft, CREATE VIEW needs to be the first statement in the query batch, but it isn't, because there's an IF NOT EXISTS statement there first. Here's an example from my idempotent script that was generated:

IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20190617133214_Add_Crossing_Coordinate_View')
BEGIN
    CREATE VIEW [dbo].[crossing_coordinate_view] 
                                        AS
                                        SELECT Crossing.Id,

This incurs Incorrect syntax near the keyword 'VIEW'. when attempting to parse it.

More info here:

https://dba.stackexchange.com/questions/215699/if-not-exists-using-object-id-doesnt-work-for-views-and-triggers-why

This ultimately makes dotnet ef migrations script unusable for release automation, arguably it's principal purpose. Please fix (one solution is to emit a DROP VIEW guarded by the IF, and always recreate the VIEW [or trigger]).

oising commented 5 years ago

Ok, I've worked around this by using a powershell task in my release pipeline to munge the migration script in the artifact drop folder before executing it. I'm using regex to wrap all CREATE VIEW and CREATE TRIGGER blocks with EXEC('...'). Here's my inline script:

$sql = get-content .\migrate.sql -raw
[regex]::replace($sql, "BEGIN\s+(CREATE (?:VIEW|TRIGGER).+?)END", "BEGIN`nEXEC('`$1');`nEND", "ignorecase,singleline") > migrate.sql

Make sure you tick the box to use PowerShell Core (for the -raw parameter support)

koshkarov commented 5 years ago

For those who uses idempotent script to apply the migrations. This approach splits the script on separate queries by "GO" statements an checks if this query is a part of the migration that was already applied. _Note: the other way of fixing this issue is to generate 'per migration' idempotent scripts (which EF Core CLI allows to do), but this method is very slow. I use PowerShell to apply them.

  1. Read migrations idempotent script file to a list of strings - $queries:
$sqlData = Get-Content $SQL_FILE_PATH -Raw
$queries = $sqlData -split "GO\r\n"
  1. Then select migrations that were already applied to the database.
$existingMigrations = Invoke-Some-Magic-And-Return-List-Of-Migrations-As-Strings

NOTE: When you make this query, make sure to check if migration table exists first.

  1. Then iterate trough $queries and extract migration name from it using RegEx:
    foreach($query in $queries)
    {
    $migrationFromQuery = $null
    if ($query -match "(?<=\')(\d{14}.+)(?=\')") {
        $migrationFromQuery =  $matches[0]
    }
    ....
  2. And check if it is in the list of applied migrations and apply if it is not there:
    ...
    if ($existingMigrations -notcontains $migrationFromQuery) {
        Invoke-Some-Magic-And-Apply-Query-As-Transaction -QUERY $query
    }
    }

Good things about this solution:

Note: it will not filter the very first SQL query:

IF OBJECT_ID(N'[__EFMigrationsHistory]') IS NULL
BEGIN
    CREATE TABLE [__EFMigrationsHistory] (
        [MigrationId] nvarchar(150) NOT NULL,
        [ProductVersion] nvarchar(32) NOT NULL,
        CONSTRAINT [PK___EFMigrationsHistory] PRIMARY KEY ([MigrationId])
    );
END;

But this query is completely safe and will not cause any problems like discussed in this issue.

kovyfive commented 5 years ago

Also including :on error ignore in the beginning of the script will ignore all errors. This should help to run it, but will not show any error during deployment, potentially not showing possible deployment issues.

patrickklaeren commented 5 years ago

Is there an update as to when this is likely to be fixed? Run into this today, for the first time and halted deployment to our prod environment.

patrickklaeren commented 5 years ago

Follow up to @CezaryKMakingWaves great workaround, I've updated the code to work with EF Core 3.0

public class DynamicSqlRelationalCommandBuilder : RelationalCommandBuilder
    {
        public DynamicSqlRelationalCommandBuilder(RelationalCommandBuilderDependencies dependencies) : base(dependencies)
        {
        }

        public override IRelationalCommand Build()
        {
            var newCommandText = "EXECUTE ('" + base.ToString().Replace("'", "''") + "')";

            return new RelationalCommand(base.Dependencies, newCommandText, base.Parameters);
        }
    }

    public class DynamicSqlRelationalCommandBuilderFactory : RelationalCommandBuilderFactory
    {
        public DynamicSqlRelationalCommandBuilderFactory(RelationalCommandBuilderDependencies dependencies) : base(dependencies)
        {
        }

        public override IRelationalCommandBuilder Create()
        {
            return new DynamicSqlRelationalCommandBuilder(base.Dependencies);
        }
    }

Along with the design time options replacement

builder.ReplaceService<IRelationalCommandBuilderFactory, DynamicSqlRelationalCommandBuilderFactory>();
stephensowerby-ncl commented 4 years ago

Has there been any progress on this? This is causing the idempotent script to fail on simple column rename operations.

Adrian10988 commented 4 years ago

Hi,

Another workaround is to globally wrap SQL scripts with EXECUTE() statement. In case of someone looking for such an approach here is very dumb (roughly tested) implementation:

    public class DynamicSqlRelationalCommandBuilder : RelationalCommandBuilder
    {
        public DynamicSqlRelationalCommandBuilder(IDiagnosticsLogger<DbLoggerCategory.Database.Command> logger, IRelationalTypeMappingSource typeMappingSource) : base(logger, typeMappingSource)
        {
        }

        protected override IRelationalCommand BuildCore(IDiagnosticsLogger<DbLoggerCategory.Database.Command> logger, string commandText, IReadOnlyList<IRelationalParameter> parameters)
        {
            commandText = "EXECUTE ('" + commandText.Replace("'", "''") + "')";
            return base.BuildCore(logger, commandText, parameters);
        }
    }
    public class DynamicSqlRelationalCommandBuilderFactory : RelationalCommandBuilderFactory
    {
        public DynamicSqlRelationalCommandBuilderFactory(IDiagnosticsLogger<DbLoggerCategory.Database.Command> logger, IRelationalTypeMappingSource typeMappingSource) : base(logger, typeMappingSource)
        {
        }

        protected override IRelationalCommandBuilder CreateCore(IDiagnosticsLogger<DbLoggerCategory.Database.Command> logger,
            IRelationalTypeMappingSource relationalTypeMappingSource)
        {
            return new DynamicSqlRelationalCommandBuilder(logger, relationalTypeMappingSource);
        }
    }

Then somewhere in your IDesignTimeDbContextFactory implementation: options.ReplaceService<IRelationalCommandBuilderFactory, DynamicSqlRelationalCommandBuilderFactory>();

This blew up all of my entity framework calls. I am now getting syntactical errors in the ef generated sql scripts that get built from my ef queries. Here is an example error Must declare the scalar @__id_Value_0

It fixed my pipeline issues but it causes other problems on my apps utilizing this context. I was able to verify the issue was the custom code by commenting it out locally and verifying the issue was gone

EDIT

It's an ugly hack but I ended up throwing a simple conditional in the code that will only wrap the commands in "EXEC" if the command contains "CREATE PROCEDURE" or "ALTER PROCEDURE". These commands were the ones giving me problems

  protected override IRelationalCommand BuildCore(IDiagnosticsLogger<DbLoggerCategory.Database.Command> logger, string commandText, IReadOnlyList<IRelationalParameter> parameters)
        {
            if (commandText.Contains("CREATE PROCEDURE") || commandText.Contains("ALTER PROCEDURE"))
                commandText = "EXECUTE ('" + commandText.Replace("'", "''") + "')";

            var d = base.BuildCore(logger, commandText, parameters);
            return d;
        }
patrickklaeren commented 4 years ago

I am now getting syntactical errors in the ef generated sql scripts that get built from my ef queries.

I'm not sure I follow. You're only supposed to use this to generate scripts during design time migration generation? @Adrian10988

Adrian10988 commented 4 years ago

I am now getting syntactical errors in the ef generated sql scripts that get built from my ef queries.

I'm not sure I follow. You're only supposed to use this to generate scripts during design time migration generation? @Adrian10988

Yes exactly. Unless I am misunderstanding how to use your solution. This is what I did.

patrickklaeren commented 4 years ago

This leaves me more confused. The context you use at runtime in your app should have no idea that these subsititute services exist. They're only to be used in a design time entity context factory. @Adrian10988

joelmeaders commented 4 years ago

I am also running into issues with column renames and drops using scripts generated by EFC in Azure DevOps pipelines. Any updates?

natcoxall commented 4 years ago

We've just encountered this issue as well in EF Core 3.1. Is there any indication from the team when we might expect to see a fix for this?

Bluhmanman commented 4 years ago

Hi I'm unable to work around this when my migrations involve multiple SPROC's being added:

IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20200116214944_SegmentSProcs')
BEGIN
    EXEC('CREATE PROCEDURE [dbo].[GetMetricAverage]
        @type int,
        @intervals int,
        @isLast bit
    AS
    BEGIN

        SET NOCOUNT ON;

        SELECT 
            m.InteractionId,
            AVG(m.MetricAverage) MetricAverage
        FROM 
            (
                SELECT
                    seg.InteractionId,
                    met.Value as MetricAverage,
                    ROW_NUMBER() OVER (
                        PARTITION BY seg.InteractionId 
                        ORDER BY 
                            CASE WHEN @isLast = 1 THEN -seg.SegmentNumber ELSE seg.SegmentNumber END 
                        ) AS RowNumber
                FROM 
                    Segments AS seg
                    INNER JOIN (SELECT * FROM MetricValues WHERE MetricTypeId = @type) as met
                    ON met.InteractionId = seg.InteractionId AND met.SegmentNumber = seg.SegmentNumber
            ) AS m
        WHERE 
            m.RowNumber < (@intervals + 1)
        GROUP BY 
            m.InteractionId 

        END')
END;

GO

IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20200116214944_SegmentSProcs')
BEGIN
    EXEC('CREATE PROCEDURE [dbo].[GetMetricAverageSubSet]
        @interactionIds nvarchar(max),
        @type int,
        @intervals int,
        @isLast bit
    AS
    BEGIN

        SET NOCOUNT ON;

        SELECT value 
        INTO #Interactions 
        FROM STRING_SPLIT(@interactionIds, ',')

        SELECT 
            m.InteractionId,
            AVG(m.MetricAverage) MetricAverage
        FROM 
            (
                SELECT
                    seg.InteractionId,
                    met.Value as MetricAverage,
                    ROW_NUMBER() OVER (
                        PARTITION BY seg.InteractionId 
                        ORDER BY 
                            CASE WHEN @isLast = 1 THEN -seg.SegmentNumber ELSE seg.SegmentNumber END 
                        ) AS RowNumber
                FROM 
                    Segments AS seg
                    INNER JOIN (SELECT * FROM MetricValues WHERE MetricTypeId = @type) as met
                    ON met.InteractionId = seg.InteractionId AND met.SegmentNumber = seg.SegmentNumber
                    INNER JOIN #Interactions i
                    ON i.value = seg.InteractionId
            ) AS m
        WHERE 
            m.RowNumber < (@intervals + 1)
        GROUP BY 
            m.InteractionId 

    DROP TABLE #Interactions
        END')
END;

GO

When running a migration script with this section in it, it generates this error: Msg 102, Level 15, State 3, Line 1982 Incorrect syntax near ')'. and then the first SPROC makes it in, the second one does not.

bricelam commented 4 years ago

Is there any indication from the team when we might expect to see a fix for this?

This issue is currently in the 5.0.0 milestone. 5.0 is the next non-patch release of EF Core scheduled for November 2020. I'll do my best to get a fix in by then. Until then, keep post-processing the SQL file or use a non-idempotent script (e.g. dotnet ef migrations script %CURRENT_PRODUCTION_MIGRATION%)

markmonster commented 4 years ago

The workaround proposed by @Inzanit no longer works with EF Core 3.x because the interface has been changed. I rewrote the workaround to work with EF Core 3.x.

public class DynamicSqlRelationalCommandBuilderFactory : RelationalCommandBuilderFactory
{
    public DynamicSqlRelationalCommandBuilderFactory(RelationalCommandBuilderDependencies dependencies) : base(dependencies)
    {
    }

    public override IRelationalCommandBuilder Create()
    {
        return new DynamicSqlRelationalCommandBuilder(Dependencies);
    }
}

public class DynamicSqlRelationalCommandBuilder : RelationalCommandBuilder
{
    public DynamicSqlRelationalCommandBuilder(RelationalCommandBuilderDependencies dependencies) : base(dependencies)
    {
    }

    public override IRelationalCommand Build()
    {
        string commandText = ToString();
        commandText = "EXECUTE ('" + commandText.Replace("'", "''") + "')";

        return new RelationalCommand(Dependencies, commandText, Parameters);
    }
}

Along with the design time options replacement

builder.ReplaceService<IRelationalCommandBuilderFactory, DynamicSqlRelationalCommandBuilderFactory>();

Please be aware that this might not work with Stored Procedures as mentioned above.

onur-ozguzel commented 4 years ago

I' m trying to solve this problem for hours. My queries runs very well in SSMS but gave error in Azure Devops SQL Server database deploy task.

After i found out i cannot run queries in only sqlcmd, i changed my search and found this topic. It will be very healty to fix this issue on .netCore side.

@mkamonster it seems ok with View & Procedures.

Edit: Because i don' t want to use a design time fix approach, i changed my migration history on insert & update data' s.

By the way, on my local computer' s SSMS i receive error, on server' s SMSS i don' t receive any error. But on both computer' s sqlcmd raises "Invalid column name" error.

patrickklaeren commented 4 years ago

@bricelam @ajcvickers is this issue up for grabs by any chance? I'd love to contribute something here.

ajcvickers commented 4 years ago

@Inzanit We're happy to take community PRs for most issues. Make a proposal here for the approach you want to take to fix it, and if it makes sense then it can be followed up with a PR.

matthias-schuchardt commented 4 years ago

Besides renaming tables and columns, there is another issue that would be solved if the script's statements were wrapped into an EXEC('...').

When creating a schema (e.g. as needed for temporal tables), this statement needs to be the only one in its batch. Thus an error is thrown when executed within the migration-if-statements of the script. The current remedy seems to be to wrap the CREATE SCHEMA MySchema into dynamic SQL.

ChristopherHaws commented 4 years ago

Is there any indication from the team when we might expect to see a fix for this?

This issue is currently in the 5.0.0 milestone. 5.0 is the next non-patch release of EF Core scheduled for November 2020. I'll do my best to get a fix in by then. Until then, keep post-processing the SQL file or use a non-idempotent script (e.g. dotnet ef migrations script %CURRENT_PRODUCTION_MIGRATION%)

@bricelam Is there a reason this issue does not qualify for a patch release? The feature does not work in its current state.

ajcvickers commented 4 years ago

@ChristopherHaws The release planning section in the docs describe how we determine what to patch. In this case, this is a limitation that has existed in idempotent scripts for some time and that needs significant changes to the product to support it. This is not something we would patch.

onur-ozguzel commented 4 years ago

The thing is, if EF Core wraps EXEC('...') query for only Insert, Update & Delete queries (which is generated by hasData property) on idempotent scripts, the problem is solves for me.

Because i already wrapping my view, stored procedures etc. custom queries within an EXEC('') query.

For Example:

        protected override void Up(MigrationBuilder migrationBuilder)
        {
                    string sql = @"
                    IF (EXISTS (SELECT 1 FROM sys.views WHERE name = 'ViewXXX'))
                    BEGIN
                        EXECUTE ('DROP view ViewXXX')
                    END

                    IF (NOT EXISTS (SELECT 1 FROM sys.views WHERE name = 'ViewXXX'))
                    BEGIN
                        EXECUTE ('
CREATE VIEW [dbo].[ViewXXX] as
...
                        ')
                    END";

            migrationBuilder.Sql(sql);
        }
coldacid commented 4 years ago

If it helps anyone, this seems to cover most/all the cases described above for anyone who is using the command builder hack from earlier in this issue:

class DynamicSqlRelationalCommandBuilder : RelationalCommandBuilder
{
    private readonly string _execRequiringStatements = @"(SET IDENTITY_INSERT|^(UPDATE|(CREATE|ALTER) PROCEDURE|ALTER TABLE|CREATE (UNIQUE )?INDEX|(CREATE|DROP) VIEW))";
    public DynamicSqlRelationalCommandBuilder(RelationalCommandBuilderDependencies dependencies)
        : base(dependencies)
    {
    }

    public override IRelationalCommand Build()
    {
        var commandText = ToString();

        if (Regex.IsMatch(commandText, _execRequiringStatements, RegexOptions.IgnoreCase))
            commandText = "EXECUTE ('" + commandText.Replace("'", "''") + "')";

        return new RelationalCommand(Dependencies, commandText, Parameters);
    }
}
gabrielionita commented 4 years ago

@coldacid, Your solution works great for applying migration, but on application run, for custom Update entries I get an exec that doesn't work well with parameters in ExecuteSqlRaw or ExecuteSqlInterpolated, and throws the following message: Must declare scalar variable @Id. Is there any way to not execute ReplaceServices in startup when not applying the migrations idempotent, or can RelationalCommandBuilderFactory be aware if there are migrations being applied?

ChristopherHaws commented 4 years ago

@gabrielionita If you implement IDesignTimeDbContextFactory<ApplicationContext> then the tooling will use that configuration when generating migrations instead of the configuration in Startup.cs.

https://docs.microsoft.com/en-us/ef/core/miscellaneous/cli/dbcontext-creation#from-a-design-time-factory

bricelam commented 4 years ago

Here are the cases I found that require EXEC for the SQL EF generates:

matthias-schuchardt commented 4 years ago

Here are the cases I found that require EXEC for the SQL EF generates:

  • AddColumn with ComputedColumnSql
  • AddCheckConstraint
  • CreateIndex with Filter
  • DeleteData
  • InsertData
  • UpdateData

And CREATE SCHEMA for the temporal tables.

Thanks for the PR!

ChristopherHaws commented 4 years ago

We also wrap all of our migrationBuilder.Sql("...") calls with EXEC to prevent future failures if schema's change. :)

bricelam commented 4 years ago

@ChristopherHaws Good strategy. We should mention this in the docs. Filed https://github.com/dotnet/EntityFramework.Docs/issues/2561

derigel23 commented 4 years ago

@bricelam Any plans to backport fix to 3.1.x branch?

bricelam commented 4 years ago

No, sorry. This fix required too significant of changes for a patch release—especially on an LTS release.

binaryio commented 4 years ago

I've found another workaround to this issue by using a powershell script to edit the Idempotent script prior to executing it. It fetches the list of MigrationIds and comments out the relevant script blocks.

param ($server, $database, $username, $password, $script)

#
# Create the temporary file to contain the modified script
#
$newscript = Join-Path -Path (Split-Path -Path $script) -ChildPath ("fixed_" + (Split-Path -Path $script -Leaf));
Set-Content -Path $newscript -Value "" -Encoding 'utf8'

#
# Fetch the currently applied migrations
#
$migrationIds = ""
$qry = Invoke-Sqlcmd -ServerInstance $server -Database $database -Username $username -Password $password -Query "SELECT DISTINCT [MigrationId] FROM [__EFMigrationsHistory]" -ErrorAction SilentlyContinue
if ($qry -ne $null)
{
    $migrationIds = ($qry | Select-Object -ExpandProperty MigrationId) -Join "|"
}

#
# Match the chunks in the script with the list of applied migrations, and comment them out
#
if ($migrationIds -ne "")
{
    $regex = "(?ms)^IF NOT EXISTS\(SELECT \* FROM \[__EFMigrationsHistory\] WHERE \[MigrationId\] = N'(" + $migrationIds + ")'\).*?END;\s+GO"
    $c = (Get-Content $script -Raw) -replace $regex,"/*`r`n`$0`r`n*/";
    Set-Content -Path $newscript -Value $c -Encoding 'utf8'
} else {
    Copy-Item $script $newscript
}

#
# Execute the fixed Migrations script
#
Invoke-Sqlcmd -ServerInstance $server -Database $database -Username $username -Password $password -InputFile $newscript -Verbose
DmytroHeorhin commented 3 years ago

Here are the cases I found that require EXEC for the SQL EF generates:

  • AddColumn with ComputedColumnSql
  • AddCheckConstraint
  • CreateIndex with Filter
  • DeleteData
  • InsertData
  • UpdateData

It'd also be nice to have DROP INDEX and probably DROP any object in this list. I face an error like "Cannot drop the index 'my index name', because it does not exist or you do not have permission."

Thanks for the fix already made! It'll avoid the error in case of CREATE INDEX for me.

VidSankar commented 3 years ago

Another project where this problem occurs (without involving indexes) is the current version of the Contoso University sample app (available here https://github.com/aspnet/Docs/tree/0ee6b101d9d4b4022add3583ed25f0d89674b87b/aspnetcore/data/ef-mvc/intro/samples/cu-final). The Inheritance migration creates a temporary column (OldID) that is created and immediately removed in the same migration ; and after the first run, the database rejects the itempotent migration script because the column does not exist.

Unfortunately in this case, wrapping the instructions in an EXEC call with a simple regex replace won't be enough, as we would need to escape the quotes inside the EXEC argument, like in that case:

IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20170816195930_Inheritance')
BEGIN
    UPDATE dbo.Enrollment SET StudentId = (SELECT ID FROM dbo.Person WHERE OldId = Enrollment.StudentId AND Discriminator = 'Student')
END;

I am having the same issue. Was there any fix or work around for this?

Though Idempotent script, sql seems to evaluate the query, though it will not get executed. And I am getting an error "Invalid column name". Any idea?

derigel23 commented 3 years ago

@bricelam what about net5.0? Still doesn't work.