Builder method mismatch across providers in model snapshot

[roji]

Back in 3.0, we removed provider prefixes from builder method names, so ForSqlServerUseIdentityColumns became UseIdentityColumns (#16686).

In 5.0, we also started using provider-specific fluent APIs in model snapshots, instead of raw migrations, so our model snapshots now contain UseIdentityColumns instead of SetAnnotation(...) (#16922).

The combination of the above, means that model snapshots now contain ambiguous fluent method calls when multiple providers are referenced. In the best case, this doesn't compile because of an ambiguous invocation; in the worst case, the wrong method is chosen since Npgsql's UseIdentityColumns has just one parameter, whereas SqlServer's UseIdentityColumns has the same parameter but two additional optional ones... This leads to npgsql/efcore.pg#1587, reported by @mortenab.

A (not amazing) workaround is to edit model snapshots and replace the extension invocation with an explicit invocation specifying the provider's extension type (i.e. NpgsqlPropertyBuilderExtensions or SqlServerPropertyBuilderExtensions).

[roji]

Note that the right way for users to deal with this is most probably to have fully separate migration projects, with each one referencing one provider. But our guidance currently describes other options (e.g. multiple context types) which would suffer from the above.

[ajcvickers]

Note from triage: we will investigate changing the model snapshot to call these methods using normal method call syntax, rather than calling them as extension methods.

[ajcvickers]

@roji Just wanted to check you didn't miss this servicing issue.

[roji]

Sorry, I somehow remember us considering this as non-urgent for 5.0.2. I'll look into this tonight.

[ajcvickers]

@roji You are correct, we don't need to rush to get this into 5.0.2--I will remove it from that milestone. However, we should target 5.0.3 to either make the change or decide that it isn't something we will patch.

[roji]

I took a look at this. The fundamental problem is that MethodCallCodeFragment has a method name - which is assumed to just work as an extension over the builder - rather than a full MethodInfo; the method's declaring type simply isn't known in the snapshot generator.

Some options going forward:

1. Just guide multi-provider users to have separate per-provider migrations projects.
2. Stop using annotation code provider in the model snapshot, going back to raw annotations. I'm not sure there's enough user feedback to warrant this.
3. Use fancy reflection in the snapshot generator to find the declaring type for the method's name. This seems quite hacky and brittle.
4. We could modify MethodCallCodeFragment to accept a MethodInfo. This can be done in a non-breaking way - add an additional constructor, and modify our providers to use it. Existing code could continue using the method name only, and would still be rendered as extension methods. This seems like too much for patching though, so we'd probably do one of the above options until then.

(if MethodCallCodeFragment accepts a MethodInfo, it becomes very similar to MethodCallExpression 😁)

[codingyourlife]

+1 on this. I add further details and keywords that help Googler's (or BINGers - forgot where I am ^^) to not waste much time on this (like I did).

In my project my Snapshot file contains ValueGeneratedOnAdd() and SqlServerValueGenerationStrategy.IdentityColumn. When I do NO changes at all and run a empty migration my snapshot is updated and .UseIdentityColumn() is added everyhwere and .HasAnnotation("SqlServer:ValueGenerationStrategy", SqlServerValueGenerationStrategy.IdentityColumn) is removed. When I run A SECOND TIME an empty migration it does something unexpected. It alters my Id columns and adds changes about SqlServer:Identity (that's why I think this issue and the merge could have something to do with it #20971)

Here the message it fails with on database update:

To change the IDENTITY property of a column, the column needs to be dropped and recreated.

Recreation is not what I want to do because I don't have any changes. I think sneaked in with my .NET 5 upgrade.

For completeness I also played around with annotation changes and ran into this error message

The property 'Id' cannot be configured as 'ValueGeneratedOnUpdate' or 'ValueGeneratedOnAddOrUpdate' because the key value cannot be changed after the entity has been added to the store

As I found out here pointed out by @ajcvickers the workaround seems to be to manually comment in the line with UseIdentityColumns() in your snapshot.

So the workaround step by step is:

1. Create an empty migration -> Snapshot gets modified and UseIdentityColumn() is being used instead of SqlServerValueGenerationStrategy.IdentityColumn (kind of a .NET 5 upgrade to your Snapshot file)
2. Comment in the single line UseIdentityColumns() in your snapshot and add the line SqlServerModelBuilderExtensions.UseIdentityColumns(modelBuilder); like here as a temporary fix for the snapshot file
3. Delete your empty migration (it was just for your snapshot upgrade) and maybe check the snapshot upgrade changes in to your repo
4. Do the migration you actually wanted to do or another empty one for testing
5. Repeat step2 because Snapshot got overwritten again - so the fix is temporary
   
6. -> Now you can run database update without a problem

In general I have no idea if I run into another problem later when you guys release the fix to this but we all have to work with this somehow and have a workaround until a fix is released... For the fix it would be great to have an integration test at migration level (not snapshot level) that does an empty migration twice and there should be no change in any of those in the Up() and Down() functions.

[roji]

Design decisions:

• For 5.0, introduce a patch which specifically excludes the problematic (ambiguous) Fluent API methods (e.g. UseIdentityColumns) from being generated in the model snapshot. Investigate which other Fluent APIs are identical across providers to make sure we don't miss anything.
• For 6.0, make sure the method's type is passed via MethodCallCodeFragment, and generate a a static method call in the model snapshot instead of an extension method call.

[roji]

Some due diligence:

Model

SqlServer	Npgsql	Pomelo MySQL	Sqlite
UseIdentityColumns	UseIdentityColumns	(ValueGenerationStrategy)
HasIdentityColumnIncrement
HasIdentityColumnSeed
	UseIdentityAlwaysColumns
	UseIdentityByDefaultColumns
	UseSerialColumns
UseHiLo	UseHiLo
UseHiLoSequenceName	UseHiLoSequenceName
UseHiLoSequenceSchema	UseHiLoSequenceSchema
HasDatabaseMaxSize
HasPerformanceLevel
HasPerformanceLevelSql
HasServiceTier
HasServiceTierSql
	HasPostgresExtension
	HasPostgresEnum
	UseDatabaseTemplate
	HasPostgresRange
	UseTablespace

Entity

SqlServer	Npgsql	Pomelo MySQL	Sqlite
IsMemoryOptimized
	UseXminAsConcurrencyToken
	SetStorageParameter
	IsUnlogged
	UseCockroachDbInterleaveInParent

Property

SqlServer	Npgsql	Pomelo MySQL	Sqlite
UseIdentityColumn	UseIdentityColumn	UseMySqlIdentityColumn
HasIdentityColumnSeed
HasIdentityColumnIncrement
	UseSerialColumn
	UseIdentityAlwaysColumn
	UseIdentityByDefaultColumn
	HasIdentityOptions
		UseMySqlComputedColumn
UseHiLo	UseHiLo
HasHiLoSequence	HasHiLoSequence
		HasCharSet
		HasCollation
		HasSpatialReferenceSystem
			HasSrid

Index

SqlServer	Npgsql	Pomelo MySQL	Sqlite
IsClustered
IncludeProperties	IncludeProperties
IsCreatedOnline	IsCreatedConcurrently
HasFillFactor
	HasMethod
	HasOperators
	HasCollation
	HasSortOrder
	HasNullSortOrder
		IsFullText
		IsSpatial
		HasPrefixLength

Key

SqlServer	Npgsql	Pomelo MySQL	Sqlite
IsClustered
		HasPrefixLength