Efcore: 5.0 RC1 generates invalid idempotent migration's script for SQL Server INSERT INTO statements

[cjaliaga]

Exporting the idempotent migration script on RC1, using the dotnet-ef global tool, when an EntityConfiguration contains a HasData() and is using SQL Server provider, generates invalid SQL like this:

IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20200915225618_InitialCreate')
BEGIN
    IF EXISTS (SELECT * FROM [sys].[identity_columns] WHERE [name] IN (N'Id', N'Title') AND [object_id] = OBJECT_ID(N'[Shows]'))
        SET IDENTITY_INSERT [Shows] ON;
    EXEC(CONCAT(N'INSERT INTO [Shows] ([Id], [Title])', CHAR(13), CHAR(10), N'VALUES (1Game of Thrones'')'));
    IF EXISTS (SELECT * FROM [sys].[identity_columns] WHERE [name] IN (N'Id', N'Title') AND [object_id] = OBJECT_ID(N'[Shows]'))
        SET IDENTITY_INSERT [Shows] OFF;
END;
GO
````

### Steps to reproduce
I created a sample project reproducing the issue: https://github.com/cjaliaga/MigrationScriptIssue

Data class:
```csharp
public class Show
{
    public int Id { get; set; }
    public string Title { get; set; }
}

EntityConfiguration:

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
    base.OnModelCreating(modelBuilder);
    modelBuilder.Entity<Show>().HasData(new Show { Id = 1, Title = "Game of Thrones" });
}

Generated migration: https://github.com/cjaliaga/MigrationScriptIssue/blob/master/migration.sql

Global tool output

dotnet-ef migrations script -i -p "MigrationScriptIssue" -o "migration.sql" --verbose
Using project 'MigrationScriptIssue\MigrationScriptIssue.csproj'.
Using startup project 'MigrationScriptIssue\MigrationScriptIssue.csproj'.
Writing 'MigrationScriptIssue\obj\MigrationScriptIssue.csproj.EntityFrameworkCore.targets'...
dotnet msbuild /target:GetEFProjectMetadata /property:EFProjectMetadataFile=D:\Users\carlo\AppData\Local\Temp\tmp2B32.tmp /verbosity:quiet /nologo MigrationScriptIssue\MigrationScriptIssue.csproj
Writing 'MigrationScriptIssue\obj\MigrationScriptIssue.csproj.EntityFrameworkCore.targets'...
dotnet msbuild /target:GetEFProjectMetadata /property:EFProjectMetadataFile=D:\Users\carlo\AppData\Local\Temp\tmp2DE2.tmp /verbosity:quiet /nologo MigrationScriptIssue\MigrationScriptIssue.csproj
Build started...
dotnet build MigrationScriptIssue\MigrationScriptIssue.csproj /verbosity:quiet /nologo

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:02.20
Build succeeded.
dotnet exec --depsfile D:\Users\carlo\source\repos\MigrationScriptIssue\MigrationScriptIssue\bin\Debug\netcoreapp3.1\MigrationScriptIssue.deps.json --additionalprobingpath D:\Users\carlo\.nuget\packages --additionalprobingpath D:\Microsoft\Xamarin\NuGet --additionalprobingpath "C:\Program Files\dotnet\sdk\NuGetFallbackFolder" --runtimeconfig D:\Users\carlo\source\repos\MigrationScriptIssue\MigrationScriptIssue\bin\Debug\netcoreapp3.1\MigrationScriptIssue.runtimeconfig.json D:\Users\carlo\.dotnet\tools\.store\dotnet-ef\5.0.0-rc.1.20451.13\dotnet-ef\5.0.0-rc.1.20451.13\tools\netcoreapp3.1\any\tools\netcoreapp2.0\any\ef.dll migrations script -i -o migration.sql --assembly D:\Users\carlo\source\repos\MigrationScriptIssue\MigrationScriptIssue\bin\Debug\netcoreapp3.1\MigrationScriptIssue.dll --startup-assembly D:\Users\carlo\source\repos\MigrationScriptIssue\MigrationScriptIssue\bin\Debug\netcoreapp3.1\MigrationScriptIssue.dll --project-dir D:\Users\carlo\source\repos\MigrationScriptIssue\MigrationScriptIssue\ --language C# --working-dir D:\Users\carlo\source\repos\MigrationScriptIssue --verbose --root-namespace MigrationScriptIssue
Using assembly 'MigrationScriptIssue'.
Using startup assembly 'MigrationScriptIssue'.
Using application base 'D:\Users\carlo\source\repos\MigrationScriptIssue\MigrationScriptIssue\bin\Debug\netcoreapp3.1'.
Using working directory 'D:\Users\carlo\source\repos\MigrationScriptIssue\MigrationScriptIssue'.
Using root namespace 'MigrationScriptIssue'.
Using project directory 'D:\Users\carlo\source\repos\MigrationScriptIssue\MigrationScriptIssue\'.
Remaining arguments: .
Finding DbContext classes...
Finding IDesignTimeDbContextFactory implementations...
Finding application service provider...
Finding Microsoft.Extensions.Hosting service provider...
Using environment 'Development'.
Using application service provider from Microsoft.Extensions.Hosting.
Found DbContext 'ShowsContext'.
Finding DbContext classes in the project...
Using context 'ShowsContext'.
Finding design-time services for provider 'Microsoft.EntityFrameworkCore.SqlServer'...
Using design-time services from provider 'Microsoft.EntityFrameworkCore.SqlServer'.
Finding design-time services referenced by assembly 'MigrationScriptIssue'.
No referenced design-time services were found.
Finding IDesignTimeServices implementations in assembly 'MigrationScriptIssue'...
No design-time services were found.
Writing 'migration.sql'...

Further technical details

EF Core version: 5.0.0-rc.1.20451.13
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET Core 3.1
Operating system: Windows
IDE: Visual Studio 2019 16.7.3

[ajcvickers]

/cc @bricelam

[bricelam]

I looked through the code after seeing this on Twitter and couldn't see any obvious reason for the weird N'VALUES (1Game of Thrones'')' literal. I'll need to debug into it using the repro to understand it.

[cjaliaga]

I've been debugging this a bit today, when we are escaping the line breaks for this SQLLiteral:

INSERT INTO [Table1] ([Id], [Title])\r\nVALUES (1, N''Game of Thrones'')

We are doing this Replace

.Replace($", {unicodePrefix}''", string.Empty);

That removes ", N''" from this one:

CONCAT(N'INSERT INTO [Table1] ([Id], [Title])', CHAR(13), N'', CHAR(10), N'VALUES (1, N''Game of Thrones'')')

So we end with the problematic one:

CONCAT(N'INSERT INTO [Table1] ([Id], [Title])', CHAR(13), CHAR(10), N'VALUES (1Game of Thrones'')')

https://github.com/dotnet/efcore/blob/3974d3f1c3cd3a91f9048b919ce8f77d17bb8af5/src/EFCore.SqlServer/Storage/Internal/SqlServerStringTypeMapping.cs#L183-L190

Callstack:

Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal.SqlServerStringTypeMapping.EscapeLineBreaks(string value)
Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal.SqlServerStringTypeMapping.GenerateNonNullSqlLiteral(object value)
Microsoft.EntityFrameworkCore.Storage.RelationalTypeMapping.GenerateProviderValueSqlLiteral(object value)
Microsoft.EntityFrameworkCore.Storage.RelationalTypeMapping.GenerateSqlLiteral(object value)
Microsoft.EntityFrameworkCore.Migrations.SqlServerMigrationsSqlGenerator.Generate(Microsoft.EntityFrameworkCore.Migrations.Operations.InsertDataOperation operation, Microsoft.EntityFrameworkCore.Metadata.IModel model, Microsoft.EntityFrameworkCore.Migrations.MigrationCommandListBuilder builder, bool terminate)

[bricelam]

Yeah, I just barely found the issue too. The code tries to trim out leading and trailing empy strings, but it's buggy (as you've seen). I'm rewriting it (and the one in SQLite) now so we can fix this in RC2.

[SOHODeveloper]

Just would like to note that the code created by an earlier preview (5 or 6) seemed very reasonable at least for the SQL provider.

    IF EXISTS (SELECT * FROM [sys].[identity_columns] WHERE [name] IN (N'Id', N'Name') AND [object_id] = OBJECT_ID(N'[CfgEmail].[BucketTypes]'))
        SET IDENTITY_INSERT [CfgEmail].[BucketTypes] ON;
    INSERT INTO [CfgEmail].[BucketTypes] ([Id], [Name])
    VALUES (1, N'Consumer'),
    (2, N'Comercial');
    IF EXISTS (SELECT * FROM [sys].[identity_columns] WHERE [name] IN (N'Id', N'Name') AND [object_id] = OBJECT_ID(N'[CfgEmail].[BucketTypes]'))
        SET IDENTITY_INSERT [CfgEmail].[BucketTypes] OFF;

versus the new code

    IF EXISTS (SELECT * FROM [sys].[identity_columns] WHERE [name] IN (N'Id', N'Name') AND [object_id] = OBJECT_ID(N'[CfgEmail].[BucketTypes]'))
        SET IDENTITY_INSERT [CfgEmail].[BucketTypes] ON;
    EXEC(CONCAT(N'INSERT INTO [CfgEmail].[BucketTypes] ([Id], [Name])', CHAR(13), CHAR(10), N'VALUES (1Consumer''),', CHAR(13), CHAR(10), N'(2Comercial'')'));
    IF EXISTS (SELECT * FROM [sys].[identity_columns] WHERE [name] IN (N'Id', N'Name') AND [object_id] = OBJECT_ID(N'[CfgEmail].[BucketTypes]'))
        SET IDENTITY_INSERT [CfgEmail].[BucketTypes] OFF;

Glad you are on it for RC2.

[bricelam]

I thought we had an issue similar to #20723 but for SQL. I can't seem to find it.

[bricelam]

Filed #22611 to make it prettier.

[kyletindle]

Hi @AndriySvyryd . I dont think this is fixed fully. I've installed .NET 5 RC2 AND VS2019 16.8 preview 4 and upgraded all of my NuGet packages to latest versions but its giving the exact same error messgae. This did used to work in .NET 5 preview 6 i think from memory. How to proceed?

The generated SQL in EXEC(CONCAT()) does have commas now and thus differences between RC1 and RC2 but RC2 still isnt fully valid SQL. In RC2 a sample (cleansed) section it is now giving:

IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'InitialVersion')
BEGIN
   IF EXISTS (SELECT * FROM [sys].[identity_columns] WHERE [name] IN (N'Id', N'Description', N'Name') AND [object_id] = OBJECT_ID(N'[ApplicationType]'))

       SET IDENTITY_INSERT [ApplicationType] ON;

        EXEC(CONCAT(N'INSERT INTO [ApplicationType] ([Id], [Description], [Name])', NCHAR(13), NCHAR(10), N'VALUES (1, N''Description 1'', N''Name 1''),', NCHAR(13), NCHAR(10), N'(2, N''Description 2'', N''Name 2'')'));

   IF EXISTS (SELECT * FROM [sys].[identity_columns] WHERE [name] IN (N'Id', N'Description', N'Name') AND [object_id] = OBJECT_ID(N'[ApplicationType]'))
        SET IDENTITY_INSERT [ApplicationType] OFF;
END;
GO

[bricelam]

Hmm, we might need to up the priority of #22611

[ChrisKlug]

I just ran into the same issue. It is definitely not fixed in RC2. Not sure why the CONCAT is needed at all. But I think that was already discussed in #22611.

Also slightly curious about how it can be considered fixed without actually being fixed. Did anyone actually try the code and run the generated migration against an actual database? Seems like an obvious thing to do before closing the issue...

[rathga]

Same issue here. Kinda wiped out my primordial CICD pipeline for the time being :(

It looks like the PR from @bricelam #23005 fixing this has gone into release rather than RC2 ?

[bricelam]

Correct, the fix for #22611 is coming in 5.0.0 GA. You can verify it using the daily builds.