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

EF Core 8.0.3 produces incorrect idempotent migration script #33337

Closed
pekspro opened this issue Mar 16, 2024 · 7 comments
Closed

EF Core 8.0.3 produces incorrect idempotent migration script #33337

pekspro opened this issue Mar 16, 2024 · 7 comments
Labels
area-migrations closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. consider-for-next-release customer-reported priority-bug Issues which requires API breaks and have bigger impact hence should be fixed earlier in the release punted-for-9.0 Originally planned for the EF Core 9.0 (EF9) release, but moved out due to resource constraints. regression Servicing-consider type-bug
Milestone

Comments

@pekspro
Copy link

pekspro commented Mar 16, 2024

After I updated a library from this:

    <PackageReference Include="Microsoft.EntityFrameworkCore" Version="8.0.2" />
    <PackageReference Include="Microsoft.EntityFrameworkCore.Relational" Version="8.0.2" />
    <PackageReference Include="Microsoft.EntityFrameworkCore.SqlServer" Version="8.0.2" />
    <PackageReference Include="Microsoft.EntityFrameworkCore.Design" Version="8.0.2">

To this:

    <PackageReference Include="Microsoft.EntityFrameworkCore" Version="8.0.3" />
    <PackageReference Include="Microsoft.EntityFrameworkCore.Relational" Version="8.0.3" />
    <PackageReference Include="Microsoft.EntityFrameworkCore.SqlServer" Version="8.0.3" />
    <PackageReference Include="Microsoft.EntityFrameworkCore.Design" Version="8.0.3">

I get incorrect SQL when I'm build migration script. I run this command:

dotnet ef migrations script --startup-project ..\MyStartUpProject.csproj --context MyDatabaseContext --output migrations.sql --verbose --idempotent

Then I get a large SQL script. Almost everything is correct. But this correct snippet:

IF NOT EXISTS (
    SELECT * FROM [__EFMigrationsHistory]
    WHERE [MigrationId] = N'20200401082009_AddsCustomerGroupTables'
)
BEGIN
    INSERT INTO [__EFMigrationsHistory] ([MigrationId], [ProductVersion])
    VALUES (N'20200401082009_AddsCustomerGroupTables', N'8.0.2');
END;
GO

Is changed to:

IF NOT EXISTS (
    SELECT * FROM [__EFMigrationsHistory]
    WHERE [MigrationId] = N'20200401082009_AddsCustomerGroupTables'
)
BEGIN


END;
GO

IF NOT EXISTS (
    SELECT * FROM [__EFMigrationsHistory]
    WHERE [MigrationId] = N'20200401082009_AddsCustomerGroupTables'
)
BEGIN
    INSERT INTO [__EFMigrationsHistory] ([MigrationId], [ProductVersion])
    VALUES (N'20200401082009_AddsCustomerGroupTables', N'8.0.3');
END;
GO

Somehow, a BEGIN followed directly by and END is not valid SQL I guess.

I have checked this migration, and migrations before that, and I do not se anything special.

Unfortunate, I haven’t been able to reproduce this in a new library and I cannot share the code in public. But I do not mind sharing the code with someone in the team if needed. In my build pipeline I produced 6 migrations script, and only one of these has this problem.

Include provider and version information

EF Core version: 8.0.3 / 8.0.4
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET 8.0

@kemmis
Copy link

kemmis commented Apr 9, 2024

I'm having this exact issue. +1 please fix.

@pekspro
Copy link
Author

pekspro commented Apr 9, 2024

This issue remains in the fresh version 8.0.4.

@pekspro
Copy link
Author

pekspro commented Apr 9, 2024

I might have found was it causing this. If I have this migration class:

public partial class AddsCustomerGroupTables : Migration
{
  protected override void Up(MigrationBuilder migrationBuilder)
  {
      migrationBuilder.Sql(@"

SELECT GetDate()
GO
SELECT GetDate()
GO
SELECT GetDate()
GO

");
  }

  protected override void Down(MigrationBuilder migrationBuilder)
  {
      
  }
}

Then the incorrect SQL is generated. My current guess the that the last GO keyword triggers some bad behavior. If I remove the everything seems to be fine. @kemmis, can you see if you do the same?

@kemmis
Copy link

kemmis commented Apr 9, 2024

Yep, It's the last GO that causes this. I just fixed it in my project by updating about a dozen sql scripts that were triggering this.

@cincuranet cincuranet added the closed-no-further-action The issue is closed and no further action is planned. label Apr 10, 2024
@cincuranet cincuranet closed this as not planned Won't fix, can't repro, duplicate, stale Apr 10, 2024
@ajcvickers ajcvickers reopened this Apr 10, 2024
@kristofdaems
Copy link

The problem is not the GO statement, it are the empty lines after the GO statement.

This issue started by this commit: 9b8b38f#diff-c80a80f96194920fd36fd4daaa22d80408aaa7f083128172c4c05331f0b92de0
File: src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs

Before that commit, empty lines were ignored so no new batch was created after the GO statement. Now after the GO statement a new batch is started but only contains empty lines which is the error.

Proposed solution: check wether the batch is empty before adding it to the builder (MigrationCommandListBuilder)
If this is a valid solution, I can have a look at it to solve it.

@maumar maumar added regression and removed closed-no-further-action The issue is closed and no further action is planned. labels Apr 19, 2024
@ajcvickers ajcvickers self-assigned this Apr 23, 2024
@ajcvickers ajcvickers added this to the 8.0.x milestone Apr 23, 2024
@ajcvickers ajcvickers removed this from the 8.0.x milestone Aug 12, 2024
@ajcvickers ajcvickers added priority-bug Issues which requires API breaks and have bigger impact hence should be fixed earlier in the release consider-for-next-release labels Aug 12, 2024
@ajcvickers ajcvickers modified the milestones: 9.0.0, Backlog Aug 14, 2024
@ajcvickers ajcvickers added the punted-for-9.0 Originally planned for the EF Core 9.0 (EF9) release, but moved out due to resource constraints. label Aug 19, 2024
@ajcvickers ajcvickers removed their assignment Aug 31, 2024
@JakeYallop
Copy link

JakeYallop commented Sep 5, 2024

Just mentioning the actual PR that caused the break: #32788

Unfortunately, setting the app context switch (which is present in the backport PR, not present in the PR to main) does not resolve this specific issue.

@AndriySvyryd AndriySvyryd self-assigned this Sep 10, 2024
@lorenzoregazzoni
Copy link

The issue is still present in 8.0.8 version

AndriySvyryd added a commit that referenced this issue Oct 10, 2024
Don't remove quoted 'GO' lines

Fixes #32826
Fixes #33337
AndriySvyryd added a commit that referenced this issue Oct 10, 2024
@AndriySvyryd AndriySvyryd added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Oct 10, 2024
@AndriySvyryd AndriySvyryd modified the milestones: Backlog, 9.0.0 Oct 10, 2024
@AndriySvyryd AndriySvyryd removed their assignment Oct 10, 2024
AndriySvyryd added a commit that referenced this issue Oct 10, 2024
AndriySvyryd added a commit that referenced this issue Oct 10, 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. consider-for-next-release customer-reported priority-bug Issues which requires API breaks and have bigger impact hence should be fixed earlier in the release punted-for-9.0 Originally planned for the EF Core 9.0 (EF9) release, but moved out due to resource constraints. regression Servicing-consider type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants