Removing a migration adds a `(string)null` argument to all ToTable calls in the snapshot

[Tragetaschen]

File a bug

I have a main project referencing a shared project that contains the EF Core entities, context and migrations.

Without any changes to the entities, I add a new migration using the CLI dotnet ef migrations add Empty -p ../shared. Expectedly, this creates a migration script and a corresponding .Designer.cs, where both Up and Down methods are empty and the snapshot does not change.

I now remove that migration by running dotnet ef migrations remove -p ../shared and I expect to be in the same state where I started and both migration files are removed as expected. However, the snapshot now has open changes, because every b.ToTable("…") call has changed:

-    b.ToTable("…");
+    b.ToTable("…", (string)null);

When I add another new (empty) migration, the additional null parameters are removed again and the snapshot has no outstanding changes.

Include verbose output

dotnet ef with --verbose and more details

``` PS …\main> dotnet ef migrations add Empty -p ..\shared\ --verbose Using project '..\shared\shared.csproj'. Using startup project '…\main\main.csproj'. Writing '..\shared\obj\shared.csproj.EntityFrameworkCore.targets'... dotnet msbuild /target:GetEFProjectMetadata /property:EFProjectMetadataFile=…\AppData\Local\Temp\tmp3717.tmp /verbosity:quiet /nologo ..\shared\shared.csproj Writing '…\main\obj\main.csproj.EntityFrameworkCore.targets'... dotnet msbuild /target:GetEFProjectMetadata /property:EFProjectMetadataFile=…\AppData\Local\Temp\tmp392C.tmp /verbosity:quiet /nologo …\main\main.csproj Build started... dotnet build …\main\main.csproj /verbosity:quiet /nologo Build succeeded. 0 Warning(s) 0 Error(s) Time Elapsed 00:00:04.55 Build succeeded. dotnet exec --depsfile …\main\bin\Debug\net6.0\main.deps.json --additionalprobingpath …\.nuget\packages --additionalprobingpath "C:\Program Files (x86)\Microsoft Visual Studio\Shared\NuGetPackages" --additionalprobingpath "C:\Program Files (x86)\Microsoft\Xamarin\NuGet" --runtimeconfig …\main\bin\Debug\net6.0\main.runtimeconfig.json …\.dotnet\tools\.store\dotnet-ef\6.0.0-rc.1.21452.10\dotnet-ef\6.0.0-rc.1.21452.10\tools\netcoreapp3.1\any\tools\netcoreapp2.0\any\ef.dll migrations add Empty --assembly …\main\bin\Debug\net6.0\shared.dll --project ..\shared\shared.csproj --startup-assembly …\main\bin\Debug\net6.0\main.dll --startup-project …\main\main.csproj --project-dir …\shared\ --root-namespace shared --language C# --framework net6.0 --nullable --working-dir …\main --verbose Using assembly 'shared'. Using startup assembly 'main'. Using application base '…\main\bin\Debug\net6.0'. Using working directory '…\main'. Using root namespace 'shared'. Using project directory '…\shared\'. Remaining arguments: . Finding DbContext classes... Finding IDesignTimeDbContextFactory implementations... Finding application service provider in assembly 'main'... Finding Microsoft.Extensions.Hosting service provider... Using environment 'Development'. Using application service provider from Microsoft.Extensions.Hosting. Found DbContext 'StateDbContext'. Finding DbContext classes in the project... Using context 'StateDbContext'. info: Microsoft.EntityFrameworkCore.Infrastructure[10403] Entity Framework Core 6.0.0-rc.1.21452.10 initialized 'StateDbContext' using provider 'Microsoft.EntityFrameworkCore.SqlServer:6.0.0-rc.1.21452.10' with options: None Finding design-time services referenced by assembly 'main'... Finding design-time services referenced by assembly 'shared'... No referenced design-time services were found. Finding design-time services for provider 'Microsoft.EntityFrameworkCore.SqlServer'... Using design-time services from provider 'Microsoft.EntityFrameworkCore.SqlServer'. Finding IDesignTimeServices implementations in assembly 'main'... No design-time services were found. Writing migration to '…\shared\Migrations\20210916120127_Empty.cs'. Writing model snapshot to '…\shared\Migrations\StateDbContextModelSnapshot.cs'. Done. To undo this action, use 'ef migrations remove' PS …\main> git status On branch feature/fix-migrations Untracked files: (use "git add ..." to include in what will be committed) ../shared/Migrations/20210916120127_Empty.Designer.cs ../shared/Migrations/20210916120127_Empty.cs nothing added to commit but untracked files present (use "git add" to track) PS …\main> dotnet ef migrations remove -p ..\shared\ --verbose Using project '..\shared\shared.csproj'. Using startup project '…\main\main.csproj'. Writing '..\shared\obj\shared.csproj.EntityFrameworkCore.targets'... dotnet msbuild /target:GetEFProjectMetadata /property:EFProjectMetadataFile=…\AppData\Local\Temp\tmpBCB2.tmp /verbosity:quiet /nologo ..\shared\shared.csproj Writing '…\main\obj\main.csproj.EntityFrameworkCore.targets'... dotnet msbuild /target:GetEFProjectMetadata /property:EFProjectMetadataFile=…\AppData\Local\Temp\tmpBEC7.tmp /verbosity:quiet /nologo …\main\main.csproj Build started... dotnet build …\main\main.csproj /verbosity:quiet /nologo Build succeeded. 0 Warning(s) 0 Error(s) Time Elapsed 00:00:02.31 Build succeeded. dotnet exec --depsfile …\main\bin\Debug\net6.0\main.deps.json --additionalprobingpath …\.nuget\packages --additionalprobingpath "C:\Program Files (x86)\Microsoft Visual Studio\Shared\NuGetPackages" --additionalprobingpath "C:\Program Files (x86)\Microsoft\Xamarin\NuGet" --runtimeconfig …\main\bin\Debug\net6.0\main.runtimeconfig.json …\.dotnet\tools\.store\dotnet-ef\6.0.0-rc.1.21452.10\dotnet-ef\6.0.0-rc.1.21452.10\tools\netcoreapp3.1\any\tools\netcoreapp2.0\any\ef.dll migrations remove --assembly …\main\bin\Debug\net6.0\shared.dll --project ..\shared\shared.csproj --startup-assembly …\main\bin\Debug\net6.0\main.dll --startup-project …\main\main.csproj --project-dir …\shared\ --root-namespace shared --language C# --framework net6.0 --nullable --working-dir …\main --verbose Using assembly 'shared'. Using startup assembly 'main'. Using application base '…\main\bin\Debug\net6.0'. Using working directory '…\main'. Using root namespace 'shared'. Using project directory '…\shared\'. Remaining arguments: . Finding DbContext classes... Finding IDesignTimeDbContextFactory implementations... Finding application service provider in assembly 'main'... Finding Microsoft.Extensions.Hosting service provider... Using environment 'Development'. Using application service provider from Microsoft.Extensions.Hosting. Found DbContext 'StateDbContext'. Finding DbContext classes in the project... Using context 'StateDbContext'. info: Microsoft.EntityFrameworkCore.Infrastructure[10403] Entity Framework Core 6.0.0-rc.1.21452.10 initialized 'StateDbContext' using provider 'Microsoft.EntityFrameworkCore.SqlServer:6.0.0-rc.1.21452.10' with options: None Finding design-time services referenced by assembly 'main'... Finding design-time services referenced by assembly 'shared'... No referenced design-time services were found. Finding design-time services for provider 'Microsoft.EntityFrameworkCore.SqlServer'... Using design-time services from provider 'Microsoft.EntityFrameworkCore.SqlServer'. Finding IDesignTimeServices implementations in assembly 'main'... No design-time services were found. Removing migration '20210916120127_Empty'. Reverting the model snapshot. Done. PS …\main> git status On branch feature/fix-migrations Changes not staged for commit: (use "git add ..." to update what will be committed) (use "git restore ..." to discard changes in working directory) modified: ../shared/Migrations/StateDbContextModelSnapshot.cs no changes added to commit (use "git add" and/or "git commit -a") PS …\main> git diff diff --git a/shared/Migrations/StateDbContextModelSnapshot.cs b/shared/Migrations/StateDbContextModelSnapshot.cs index 8c1403c..368ca98 100644 --- a/shared/Migrations/StateDbContextModelSnapshot.cs +++ b/shared/Migrations/StateDbContextModelSnapshot.cs @@ -,7 +,7 @@ namespace shared.Migrations b.HasIndex("MyEntityGuid"); - b.ToTable("MyEntity"); + b.ToTable("MyEntity", (string)null); }); ```

Include provider and version information

EF Core version: 6.0.0-rc.1.21452.10 (both library and global tool) Database provider: Microsoft.EntityFrameworkCore.SqlServer Target framework: net6.0 RC1 Operating system: Windows 10 IDE: (not used) VS 2022 Preview 4

[ajcvickers]

@Tragetaschen Are you seeing any functional impact from this?

[Tragetaschen]

I do not see any functional difference besides the uncommited changes in Git

[ajcvickers]

@Tragetaschen We discussed this and while ideally there wouldn't be a change in this case, but we're going to close this since it's not true that in general going up then down will always result in the same snapshot, and so any process that does this must account for potential differences.

[jasekiw]

This is really frustrating considering that it is really easy to corrupt the model snapshot. I commonly review diffs to ensure that it is not corrupted but this is muddying the waters. I haven't ran into any other cases that changes my snapshot other than this one. What other cases are there @ajcvickers?

[jasekiw]

Another note:

It appears this stems from using the attribute [Table("MyTableName)]. If .ToTable is used below in OnModelCreating then the migrations always has the (string)null and yields no differences when removed.

  modelBuilder.Entity<MyEntity>(e =>
        {
            e.ToTable("MyEntity");

[derrexal]

Все еще актуально...

[jasekiw]

@ajcvickers Closing this issue is in direct conflict from the recommended practice of merging the snapshot file posted by the ef core team https://learn.microsoft.com/en-us/ef/core/managing-schemas/migrations/teams#merging

Yes a revert may not always match but I have never encountered that. This issue causes a change on every single table in the snapshot table leaving hundreds of changes making it impossible to deal with a merge due to the noise. I believe this needs to be reconsidered to be re-opened.

[pcoffer]

This needs to be reopened. It makes it very difficult to see real changes when diffing a merge request. One small real change is buried in a haystack of hundreds of fake changes in a real project.

[icottiero]

Please reopen this and fix it! My team had a lot of issues because of the noise generated by these unnecessary changes which caused us to miss some important changes that have broken the snapshot. This isn't a functional bug, because the snapshot does the same thing with or without the (string) null, but non-functional requirements like the ease of use can also be a good motivation for fixing something 🙏

[jasekiw]

@ajcvickers I understood last comment to mean: ideally this wouldn't happen but it's not worth our time. However, would you be open to PR's?

We discussed this and while ideally there wouldn't be a change in this case, but we're going to close this since it's not true that in general going up then down will always result in the same snapshot, and so any process that does this must account for potential differences.

I was able to fix this issue with a small adjustment to the migration code that you can see in this commit: https://github.com/dotnet/efcore/commit/353612c2cd7173eb3ae5fd3e2800b92475a11395

I figured out the schemaAnnotion variable is not null during revert but has a null schemaAnnotion.Value causing it to add (string)null. It seems that during "add migration" this doesn't happen. Since .ToTable has an overload for (tableName,schemaName,tableBuilder) and (tableName, tableBuilder) I decided that we can just not include the ,(string)null as it should not cause any issues with the overloads.

Would you be open to me creating a pr with this change? Are there any tests you think I would need to add to it?

[zdenek-jelinek]

Another practical issue that stems from this is when two concurrent development branches introduce a migration and it comes time to merge them to trunk.

In our context, we want the same merge and migration application order in trunk. To achieve this, the branch that goes second has to remove, merge and reintroduce the migration.

The migration removal process adds all these (string)null changes to the snapshot needlessly so there's always merge conflicts, unnecessarily. I do understand that the snapshot equality after migration removal is not guaranteed in all cases but this is always the same on every migration removal. Dev time is wasted on this.

[jasekiw]

@AndriySvyryd What did you think of my comment here and it's associated commit? https://github.com/dotnet/efcore/commit/353612c2cd7173eb3ae5fd3e2800b92475a11395

[AndriySvyryd]

@jasekiw We'd accept a PR, but there are other issues here around the default schema

Add this test to https://github.com/dotnet/efcore/blob/main/test/EFCore.Relational.Specification.Tests/Migrations/MigrationsTestBase.cs

[ConditionalFact]
public virtual async Task Convert_owned_entity_with_no_schema_to_regular_entity()
    => await Test(
        common =>
        {
            common.HasDefaultSchema(null);
            common.Entity(
                "Entity", e =>
                {
                    e.Property<int>("Id").ValueGeneratedOnAdd();
                    e.HasKey("Id");
                    e.Property<string>("Name");
                    e.ToTable("Entity", "MySchema");
                });
        },
        source =>
        {
            source.Entity(
                "Entity", e =>
                {
                    e.OwnsOne(
                        "Owned", "OwnedReference", o =>
                        {
                            o.Property<DateTime>("Date");
                            o.ToTable("Owned", (string)null);
                        });
                });
        },
        target =>
        {
            target.Entity(
                "Owned", e =>
                {
                    e.Property<int>("EntityId").ValueGeneratedNever();
                    e.HasKey("EntityId");
                    e.Property<DateTime>("Date");
                    e.ToTable("Owned", (string)null);
                });
        },
        model =>
        {
            Assert.Collection(model.Tables,
                t =>
                {
                    Assert.Equal("Owned", t.Name);
                    Assert.Null(t.Schema);
                },
                t =>
                {
                    Assert.Equal("Entity", t.Name);
                    Assert.Equal("MySchema", t.Schema);
                });
        });

Also, add a test for the scenario being fixed to https://github.com/dotnet/efcore/blob/main/test/EFCore.Design.Tests/Migrations/Design/CSharpMigrationsGeneratorTest.cs

[jasekiw]

@AndriySvyryd I am having some type incompatibilities with the test you posted. I tried adding it on main and on the 9.0-rc2. The errors are below. Also Is there documentation for onboarding someone to contribute to ef core? ex. the different parts of the architecture, how to approach tests, etc.

[AndriySvyryd]

Sorry, I switched the links. That test should go to https://github.com/dotnet/efcore/blob/main/test/EFCore.Relational.Specification.Tests/Migrations/MigrationsTestBase.cs

[jasekiw]

@AndriySvyryd I analyzed the test and found what I think you are getting at. I noticed the "Entity" table's schema is null when it should be "MySchema" in the assertion of the test. I assume this is the problem. Are you saying that this issue and this test are related and that fixing one might fix the other?

Note: I did have to switch the order of the Assert.Collection anonymous function params since the "Entity" table was first in the list which seems expected. I'm not sure if that matters.

[AndriySvyryd]

Yes, basically this is a case for when the snapshot should still contain a .ToTable("table", (string)null) call. Though I think it only matters if the default schema is null, as otherwise the entity type would already have a non-null schema.

I fixed the related issue I found while investigating in https://github.com/dotnet/efcore/pull/34974

[jasekiw]

@AndriySvyryd What is the difference between .ToTable("table", (string)null) and .ToTable("table")? They appear identical in the implementation but I might be missing something.

I am also trying to look into why .ToTable("table", (string)null) only happens when reverting migration and not adding migrations. Do you have any insights here?

Thank you for your help in solving this problem! 😊

[AndriySvyryd]

@AndriySvyryd What is the difference between .ToTable("table", (string)null) and .ToTable("table")? They appear identical in the implementation but I might be missing something.

Right now, they are the same, though we might change the former one to actually mean null schema instead of the default in the future. However, this is unlikely.

I am also trying to look into why .ToTable("table", (string)null) only happens when reverting migration and not adding migrations. Do you have any insights here?

This I don't know without debugging into it. There might be a difference in annotations that aren't round tripped correctly.