Builder method mismatch across providers in model snapshot

roji commented 3 years ago

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 https://github.com/npgsql/efcore.pg/issues/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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

roji commented 3 years ago

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

ajcvickers commented 3 years ago

@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 commented 3 years ago

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:

Just guide multi-provider users to have separate per-provider migrations projects.
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.~
Use fancy reflection in the snapshot generator to find the declaring type for the method's name. This seems quite hacky and brittle.
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 :grin:)

codingyourlife commented 3 years ago

+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 https://github.com/dotnet/efcore/issues/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:

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)
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
Delete your empty migration (it was just for your snapshot upgrade) and maybe check the snapshot upgrade changes in to your repo
Do the migration you actually wanted to do or another empty one for testing
Repeat step2 because Snapshot got overwritten again - so the fix is temporary
-> 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 commented 3 years ago

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 commented 3 years ago

Some due diligence:

Model

SqlServer	Npgsql	Pomelo MySQL
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
IsClustered
IncludeProperties	IncludeProperties
IsCreatedOnline	IsCreatedConcurrently
HasFillFactor
	HasMethod
	HasOperators
	HasCollation
	HasSortOrder
	HasNullSortOrder
		IsFullText
		IsSpatial
		HasPrefixLength

Key

SqlServer	Npgsql	Pomelo MySQL	Sqlite
IsClustered
		HasPrefixLength

roji commented 3 years ago

Note: #23848 tracks fixing this properly for 6.0 (via type-qualified calls instead of extension calls).

xrkolovos commented 3 years ago

Why Npgsql doesn't change its methods names?

roji commented 3 years ago

@xrkolovos previously, Fluent API methods used to be called ForSqlServerUseIdentityColumn, to avoid ambiguity; however, for single-provider use (which is the majority of cases), this makes the method names long and unwieldy. So the decision was made to drop the prefix and deal with the ambiguity differently.

dotnet / efcore