Bug: ToView + Navigation property + HasPrincipalKey breaks migrations (with root cause identification)

[ntziolis]

When having the following scenario configured in the model:

• EntityA is marked as readonly by using ToView()
• EntityA has a navigation property to EntityB, using Alternate Key on EntityB

It results in:

• Every subsequent migration created will now always contain the creation of alternate key on EntityB.
• The alternate key is not dropped inside the Up method but only created
• Which makes every execution of ef migrations update fail moving forward

To be clear this happens not just the first time after adding, but every time we create a new migration from now on.

Please see the Identifying the root cause section at the end as I think I have pinpointed the problem.

Business Scenario:

• New application with normal entities
• Legacy System (all tables are treated as readonly via toView as recommened in the docs)
• Both are contained in the same database
• To connect both worlds we use navigation properties

Steps to reproduce

• Create new project with a DbContext
• Add Entities and model builder code to DbContext as seen below
• Create initial migration
• Create additional migration right afterwards
• In second migration the alternate key on Entity B is created again without dropping (same is true for all subsequently created migrations)

// the table comes from a legacy system, we cannot change it in any way
public class EntityA{
	public int Id { get; set; }
		
	public int MyNavigationPropertyId { get; set; }
	public EntityB MyNavigationProperty { get; set; }
}

public class EntityB{
	public int Id { get; set; }
	public int AlternateKeyOnEntityB { get; set; }
}

modelBuilder.Entity<EntityA>(entity =>
{
	entity.ToView("EntityA");
		
	entity.HasOne(d => d.MyNavigationProperty)
	     .WithMany()                    
	     .HasPrincipalKey(d => d.AlternateKeyOnEntityB);
});

Identifying the root cause:

• When looking at the snapshot file the alternate key is not defined here
• When manually adding it and then creating a migration
  • the Up/Down methods are empty as expected
  • but the newly generated snapshot will again NOT contain the alternate key definition
• Based on the above behaviour + Looking at the code I think I could pinpoint the issue
  • When generating a new snapshot the IsIgnoredByMigrations extension method is called
  • This method returns true when the entity is a view (besides other things)
  • Since the relation + alternate key are defined as part of the entity that is marked ToView
  • The entity (and all operations attached to it are filtered out
• Also: While the sample is one to many, the same error appears in one to one scenario

Further technical details

EF Core version: 3.1.3
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET Core 3.1
Operating system: Windows
IDE: Visual Studio 2019 16.3, but doesn't matter as migrations were crated via dotnet ef cli

[ntziolis]

After further investigation:

• We have tried explicitly defining the alternate key AlternateKeyOnEntityB on EntityB via model builder
• But with the same result of the model snapshot not containing the alternate key definition after creating a new migration
  • At this point the assumption is that explicit calls to HasAlternateKey are removed when there is a relationship that would redefine them (as in our case)
• However while generating the snapshot IsIgnoredByMigrations filters out any view (EntityA), hence the relationship that would define the alternate key implicitly is not generated either

As far as I can asses this could either be fixed by:

• Creating special handling for the above case
• Include read-only entities in snapshot
  • As far as I can see there would be no harm in that
  • Since the snapshot is a serialized version of the model, hence including everything that is relevant for comparison would make sense
  • This would allow for correct comparison when the next migration is generated which is used to generate Up/Down method contents.
  • Only when creating those we wanne be sure to filter out any view related operations. Which already is the case today.

The second solution would likely also cover other edge cases outside of this issue that stem from the snapshot not containing readonly entities.

Note:

• Without use of HasPrincipalKey call everything works just fine (migration wise)
• Sadly our data model does not allow us to use the tables primary key as the Alternate key used on EntityB comes from a legacy system

[smitpatel]

Notes for implementation:
Issue here is that ModelSnapshot ignores generating anything for EntityTypes which are ignored by migrations hence there is no entity type call generate for EntityA.
But our key generation code ignores generating HasAlternateKey when there is referencing FK (though it was ignored in previous step). Hence every model diff will say there is a new alternate key since previous alternate key was never stored in snapshot.

efcore/src/EFCore.Design/Migrations/Design/CSharpSnapshotGenerator.cs

Lines 651 to 657 in 9f316e5

	foreach (var key in keys.Where(
	key => key != primaryKey
	&& (!key.GetReferencingForeignKeys().Any()
	|| key.GetAnnotations().Any(a => a.Name != RelationalAnnotationNames.UniqueConstraintMappings))))
	{
	GenerateKey(builderName, key, stringBuilder);
	}

[ntziolis]

Is there any chance to see a bugfix for this in 3.x? Totally understand if not just trying to understand if this is rather short term ro timeline is release of 5.0

If it wont be available before 5.0 release do you have any suggestions for a workaround?

Here are the ones we came up with:

• manually delete alternate key creation after every new migration (we have quite a few devs working on this project and probability of this being screwed up is near 1 :))
• create an actual view to replace the read-only entity in question, the view would handle the join, hence no relation and expose the necessary joined columns directly hence no need to create a relationship in our model
  • we really really would like to avoid this as we have quite a few of these cases as our plan was to bridge legacy + new app via this approach
  • additionally this would mean changing our business logic from hierarchical to flat wherever the new views would be used
  • we would def. roll this back once the issue is fixed
  • which means having to change or business logic again

Can you think of any other options? We are not afraid to meddle with internal infrastructure if necessary. When a patch comes along we would then be able to remove whatever intermediate solution there might be and not have to change the business logic of our app.

[smitpatel]

In the model snapshot file, just add

modelBuilder.Entity("EntityB").HasAlternateKey("AlternateKeyOnEntityB");

That will register the alternate key and future migration will stop generating operation in Up method.
For already generated migrations, just remove the operation which generates AK from Up method.

[ntziolis]

@smitpatel You are correct that when editing the snapshot the next migration will be generated correctly. However the snapshot after the migration does not contain the alternate key anymore. Meaning developers would need to remember todo it (similar to option 1 above).

Still manipulating the snapshot file is the most promising avenue. I have looked into whether these unrecognized keys could be defined in a partial class, but there is no way to hook into BuildModel method that is already defined in the generated snapshot file.

We are now looking at one of two options:

• post migration script that string concats the alternate keys to the snapshot (again devs would have to use a special command but at least its not manual)
• using postsharp targeting the BuildModel method and adding the keys after the original method has run (this happens on every build + has build time validation)

The last will be what we are focusing on. Will report back here if this worked out for others to find.

[ntziolis]

We found another workaround using compiler symbols, the general idea is

• have both relationship and alternate key defined in model
• exclude relationship when generating new migrations

We have defined a custom compiler symbol SKIPVIEWRELATIONS, to be able to quickly identify it later in our code. The only downside is that devs need to define the alternate key as well as the relation, but that's manageable.

The benefits are that once setup correctly:

• migrations can be generated via standard command, no need for post migration creation scripts or Postsharp post compilation.
• no breaking changes when this issue gets fixed

I wanne thank you for the time you invested to pinpoint this issue which allowed to to come up with this workaround.