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

SQL Server Migrations - SqlOperation.Sql is stripped of all empty lines #32730

Closed
sergeitemkin opened this issue Jan 5, 2024 · 0 comments · Fixed by #32788
Closed

SQL Server Migrations - SqlOperation.Sql is stripped of all empty lines #32730

sergeitemkin opened this issue Jan 5, 2024 · 0 comments · Fixed by #32788
Assignees
Labels
area-migrations closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported Servicing-approved type-bug
Milestone

Comments

@sergeitemkin
Copy link

We've upgraded to EF Core 8.0.0 (from 7.0.8) and have started experiencing an issue with MigrationBuilder.Sql where all empty lines are dropped from the SQL executed by the resulting migration. This seems to be a result of the change in SqlServerMigrationsSqlGenerator.Generate(SqlOperation operation...) that was done to fix a regex timeout.

In the current code, it seems that new string.IsNullOrWhiteSpace(line) check is causing this issue. Here's the related snippet from SqlServerMigrationsSqlGenerator:

protected override void Generate(SqlOperation operation, IModel? model, MigrationCommandListBuilder builder)
{
    var preBatched = operation.Sql
        .Replace("\\\n", "")
        .Replace("\\\r\n", "")
        .Split(new[] { "\r\n", "\n" }, StringSplitOptions.None);

    var batchBuilder = new StringBuilder();
    foreach (var line in preBatched)
    {
        if (string.IsNullOrWhiteSpace(line)) <--- breaking change
        {
            continue;
        }

While some empty lines could be considered inconsequential, some are not, and currently all of them seem to be removed. For example, a migration like this:

public partial class TestMigrationWithLineBreaks : Migration
{
    protected override void Up(MigrationBuilder migrationBuilder)
    {
        migrationBuilder.Sql("INSERT INTO SomeTable SELECT 'Some\n\nValue'");
    }
    protected override void Down(MigrationBuilder migrationBuilder) { }
}

Results in the below SQL execution (note the missing empty line):

INSERT INTO SomeTable SELECT 'Some
Value'

Sample Code w/ Issue

Another of our current use cases involves using MigrationBuilder.Sql to manage changes to stored procedures and other db objects, and the dropped empty lines reduce readability of the definitions for these objects in the db.

My current workaround is to create a separate generator that inherits from SqlServerMigrationsSqlGenerator, override the Generate(SqlOperation operation...) method with the addition of batchBuilder.AppendLine(line) inside of the aforementioned if block, and replacing the IMigrationsSqlGenerator service with this new implementation via UseSqlServer().ReplaceService<>. However, it would be awesome to not have to do that as we're losing potential future optimizations of the generator and introducing potential for incompatibility and maintenance overhead for our future EF Core updates.

EF Core version: 8.0.0
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET 8.0
Operating system: Windows 11 Pro 22H2
IDE: Visual Studio 2022 17.8.3

@ajcvickers ajcvickers self-assigned this Jan 10, 2024
@ajcvickers ajcvickers added this to the 8.0.x milestone Jan 11, 2024
ajcvickers added a commit that referenced this issue Jan 11, 2024
Fixes #32730

Will port to 8.0 for patch after merge.
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jan 11, 2024
ajcvickers added a commit that referenced this issue Jan 11, 2024
Fixes #32730

Will port to 8.0 for patch after merge.
ajcvickers added a commit that referenced this issue Jan 15, 2024
Fixes #32730

Will port to 8.0 for patch after merge.
ajcvickers added a commit that referenced this issue Jan 17, 2024
* Stop stripping new lines from migration script literals

Fixes #32730

Will port to 8.0 for patch after merge.

* More tests for cases with GO
@ajcvickers ajcvickers reopened this Jan 17, 2024
ajcvickers added a commit that referenced this issue Jan 17, 2024
* Stop stripping new lines from migration script literals

Fixes #32730

Will port to 8.0 for patch after merge.

* More tests for cases with GO
@ajcvickers ajcvickers modified the milestones: 8.0.x, 8.0.3 Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-migrations closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported Servicing-approved type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants