Allow shared columns with nullable value converters

AndriySvyryd · AndriySvyryd · commit d9b202119a2c · 2022-12-02T21:44:34.000-08:00
Make IColumn.ProviderClrType always non-nullable for value types Fixes #29531
diff --git a/src/EFCore.Relational/Infrastructure/RelationalModelValidator.cs b/src/EFCore.Relational/Infrastructure/RelationalModelValidator.cs
@@ -1388,8 +1388,6 @@ protected virtual void ValidateCompatible(
                     storeObject.DisplayName()));
         }
 
-        var typeMapping = property.GetRelationalTypeMapping();
-        var duplicateTypeMapping = duplicateProperty.GetRelationalTypeMapping();
         var currentTypeString = property.GetColumnType(storeObject);
         var previousTypeString = duplicateProperty.GetColumnType(storeObject);
         if (!string.Equals(currentTypeString, previousTypeString, StringComparison.OrdinalIgnoreCase))
@@ -1406,8 +1404,12 @@ protected virtual void ValidateCompatible(
                     currentTypeString));
         }
 
-        var currentProviderType = typeMapping.Converter?.ProviderClrType ?? typeMapping.ClrType;
-        var previousProviderType = duplicateTypeMapping.Converter?.ProviderClrType ?? duplicateTypeMapping.ClrType;
+        var typeMapping = property.GetRelationalTypeMapping();
+        var duplicateTypeMapping = duplicateProperty.GetRelationalTypeMapping();
+        var currentProviderType = typeMapping.Converter?.ProviderClrType.UnwrapNullableType()
+            ?? typeMapping.ClrType;
+        var previousProviderType = duplicateTypeMapping.Converter?.ProviderClrType.UnwrapNullableType()
+            ?? duplicateTypeMapping.ClrType;
         if (currentProviderType != previousProviderType)
         {
             throw new InvalidOperationException(
diff --git a/src/EFCore.Relational/Metadata/Internal/ColumnBase.cs b/src/EFCore.Relational/Metadata/Internal/ColumnBase.cs
@@ -84,7 +84,7 @@ public virtual Type ProviderClrType
             }
 
             var typeMapping = StoreTypeMapping;
-            var providerType = typeMapping.Converter?.ProviderClrType ?? typeMapping.ClrType;
+            var providerType = typeMapping.Converter?.ProviderClrType.UnwrapNullableType() ?? typeMapping.ClrType;
 
             return _providerClrType = providerType;
         }
diff --git a/src/EFCore.Relational/Update/Internal/ColumnAccessorsFactory.cs b/src/EFCore.Relational/Update/Internal/ColumnAccessorsFactory.cs
@@ -50,8 +50,7 @@ private static ColumnAccessors CreateGeneric<TColumn>(IColumn column)
                     }
 
                     var providerValue = entry.GetCurrentProviderValue(property);
-                    if (providerValue == null
-                        && !typeof(TColumn).IsNullableType())
+                    if (providerValue == null)
                     {
                         return (value!, valueFound);
                     }
@@ -93,8 +92,7 @@ private static ColumnAccessors CreateGeneric<TColumn>(IColumn column)
                     }
 
                     var providerValue = entry.GetOriginalProviderValue(property);
-                    if (providerValue == null
-                        && !typeof(TColumn).IsNullableType())
+                    if (providerValue == null)
                     {
                         return (value!, valueFound);
                     }
diff --git a/src/EFCore.Relational/Update/Internal/RowForeignKeyValueFactoryFactory.cs b/src/EFCore.Relational/Update/Internal/RowForeignKeyValueFactoryFactory.cs
@@ -53,29 +53,29 @@ private static IRowForeignKeyValueFactory CreateSimple<TKey, TForeignKey>(
         IValueConverterSelector valueConverterSelector)
         where TKey : notnull
     {
-        var dependentColumn = foreignKey.Columns.Single();
-        var dependentType = dependentColumn.ProviderClrType;
-        var principalType = foreignKey.PrincipalColumns.Single().ProviderClrType;
+        var dependentColumn = foreignKey.Columns.First();
+        var principalColumn = foreignKey.PrincipalColumns.First();
         var columnAccessors = ((Column)dependentColumn).Accessors;
 
-        if (dependentType.IsNullableType()
-            && principalType.IsNullableType())
+        if (principalColumn.ProviderClrType.IsNullableType()
+            || (dependentColumn.IsNullable
+                && principalColumn.IsNullable))
         {
             return new SimpleFullyNullableRowForeignKeyValueFactory<TKey, TForeignKey>(
                 foreignKey, dependentColumn, columnAccessors, valueConverterSelector);
         }
 
-        if (dependentType.IsNullableType())
+        if (dependentColumn.IsNullable)
         {
             return (IRowForeignKeyValueFactory<TKey>)Activator.CreateInstance(
                 typeof(SimpleNullableRowForeignKeyValueFactory<,>).MakeGenericType(
                     typeof(TKey), typeof(TForeignKey)), foreignKey, dependentColumn, columnAccessors, valueConverterSelector)!;
         }
 
-        return principalType.IsNullableType()
+        return principalColumn.IsNullable
             ? (IRowForeignKeyValueFactory<TKey>)Activator.CreateInstance(
-                typeof(SimpleNullablePrincipalRowForeignKeyValueFactory<,,>).MakeGenericType(
-                    typeof(TKey), typeof(TKey).UnwrapNullableType(), typeof(TForeignKey)), foreignKey, dependentColumn, columnAccessors)!
+                typeof(SimpleNullablePrincipalRowForeignKeyValueFactory<,>).MakeGenericType(
+                    typeof(TKey), typeof(TKey), typeof(TForeignKey)), foreignKey, dependentColumn, columnAccessors)!
             : new SimpleNonNullableRowForeignKeyValueFactory<TKey, TForeignKey>(
                 foreignKey, dependentColumn, columnAccessors, valueConverterSelector);
     }
diff --git a/src/EFCore.Relational/Update/Internal/SimpleNullablePrincipalRowForeignKeyValueFactory.cs b/src/EFCore.Relational/Update/Internal/SimpleNullablePrincipalRowForeignKeyValueFactory.cs
@@ -11,9 +11,9 @@ namespace Microsoft.EntityFrameworkCore.Update.Internal;
 ///     any release. You should only use it directly in your code with extreme caution and knowing that
 ///     doing so can result in application failures when updating to a new Entity Framework Core release.
 /// </summary>
-public class SimpleNullablePrincipalRowForeignKeyValueFactory<TKey, TNonNullableKey, TForeignKey>
+public class SimpleNullablePrincipalRowForeignKeyValueFactory<TKey, TForeignKey>
     : RowForeignKeyValueFactory<TKey, TForeignKey>
-    where TNonNullableKey : struct
+    where TKey : notnull
 {
     /// <summary>
     ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
diff --git a/src/EFCore.Relational/Update/Internal/SimpleRowKeyValueFactory.cs b/src/EFCore.Relational/Update/Internal/SimpleRowKeyValueFactory.cs
@@ -138,8 +138,29 @@ private sealed class NoNullsCustomEqualityComparer : IEqualityComparer<TKey>
 
         public NoNullsCustomEqualityComparer(ValueComparer comparer)
         {
-            _equals = (Func<TKey?, TKey?, bool>)comparer.EqualsExpression.Compile();
-            _hashCode = (Func<TKey, int>)comparer.HashCodeExpression.Compile();
+            var equals = comparer.EqualsExpression;
+            var getHashCode = comparer.HashCodeExpression;
+            var type = typeof(TKey);
+            if (type != comparer.Type)
+            {
+                var newEqualsParam1 = Expression.Parameter(type, "v1");
+                var newEqualsParam2 = Expression.Parameter(type, "v2");
+                equals = Expression.Lambda(
+                    comparer.ExtractEqualsBody(
+                        Expression.Convert(newEqualsParam1, comparer.Type),
+                        Expression.Convert(newEqualsParam2, comparer.Type)),
+                    newEqualsParam1, newEqualsParam2);
+
+
+                var newHashCodeParam = Expression.Parameter(type, "v");
+                getHashCode = Expression.Lambda(
+                    comparer.ExtractHashCodeBody(
+                        Expression.Convert(newHashCodeParam, comparer.Type)),
+                    newHashCodeParam);
+            }
+
+            _equals = (Func<TKey?, TKey?, bool>)equals.Compile();
+            _hashCode = (Func<TKey, int>)getHashCode.Compile();
         }
 
         public bool Equals(TKey? x, TKey? y)
diff --git a/src/EFCore.Relational/Update/ModificationCommand.cs b/src/EFCore.Relational/Update/ModificationCommand.cs
@@ -981,7 +981,14 @@ public bool TryPropagate(IColumnMappingBase mapping, IUpdateEntry entry)
                 if (property.GetAfterSaveBehavior() == PropertySaveBehavior.Save
                     || entry.EntityState == EntityState.Added)
                 {
-                    entry.SetStoreGeneratedValue(property, _currentValue);
+                    var value = _currentValue;
+                    var converter = property.GetTypeMapping().Converter;
+                    if (converter != null)
+                    {
+                        value = converter.ConvertFromProvider(value);
+                    }
+
+                    entry.SetStoreGeneratedValue(property, value);
                 }
 
                 return false;
diff --git a/src/EFCore/ChangeTracking/Internal/CurrentValueComparerFactory.cs b/src/EFCore/ChangeTracking/Internal/CurrentValueComparerFactory.cs
@@ -50,11 +50,11 @@ public virtual IComparer<IUpdateEntry> Create(IPropertyBase propertyBase)
                 if (IsGenericComparable(providerType, nonNullableProviderType))
                 {
                     var comparerType = modelType.IsClass
-                        ? typeof(NullableClassCurrentProviderValueComparer<,>).MakeGenericType(modelType, converter.ProviderClrType)
+                        ? typeof(NullableClassCurrentProviderValueComparer<,>).MakeGenericType(modelType, providerType)
                         : modelType == converter.ModelClrType
-                            ? typeof(CurrentProviderValueComparer<,>).MakeGenericType(modelType, converter.ProviderClrType)
+                            ? typeof(CurrentProviderValueComparer<,>).MakeGenericType(modelType, providerType)
                             : typeof(NullableStructCurrentProviderValueComparer<,>).MakeGenericType(
-                                nonNullableModelType, converter.ProviderClrType);
+                                nonNullableModelType, providerType);
 
                     return (IComparer<IUpdateEntry>)Activator.CreateInstance(comparerType, propertyBase, converter)!;
                 }
diff --git a/src/EFCore/ChangeTracking/ValueComparer`.cs b/src/EFCore/ChangeTracking/ValueComparer`.cs
@@ -117,8 +117,7 @@ public ValueComparer(
             || unwrappedType == typeof(Guid)
             || unwrappedType == typeof(bool)
             || unwrappedType == typeof(decimal)
-            || unwrappedType == typeof(object)
-           )
+            || unwrappedType == typeof(object))
         {
             return Expression.Lambda<Func<T?, T?, bool>>(
                 Expression.Equal(param1, param2),
diff --git a/src/EFCore/Metadata/Internal/Property.cs b/src/EFCore/Metadata/Internal/Property.cs
@@ -787,8 +787,8 @@ public virtual PropertySaveBehavior GetAfterSaveBehavior()
         => FindAnnotation(CoreAnnotationNames.ProviderClrType)?.GetConfigurationSource();
 
     private Type GetEffectiveProviderClrType()
-        => TypeMapping?.Converter?.ProviderClrType
-            ?? ClrType.UnwrapNullableType();
+        => (TypeMapping?.Converter?.ProviderClrType
+            ?? ClrType).UnwrapNullableType();
 
     /// <summary>
     ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
@@ -997,7 +997,7 @@ public virtual CoreTypeMapping? TypeMapping
     /// </summary>
     public virtual ValueComparer? GetProviderValueComparer()
         => GetProviderValueComparer(null)
-            ?? (GetEffectiveProviderClrType() == ClrType
+            ?? (GetEffectiveProviderClrType() == ClrType.UnwrapNullableType()
                 ? GetKeyValueComparer()
                 : TypeMapping?.ProviderValueComparer);
 
diff --git a/test/EFCore.Relational.Specification.Tests/UpdatesRelationalTestBase.cs b/test/EFCore.Relational.Specification.Tests/UpdatesRelationalTestBase.cs
@@ -174,14 +174,19 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
 
             modelBuilder.Entity<Product>().HasIndex(p => new { p.Name, p.Price }).IsUnique().HasFilter("Name IS NOT NULL");
 
-            modelBuilder.Entity<Person>()
-                .Property(p => p.Country)
-                .HasColumnName("Country");
-
-            modelBuilder.Entity<Person>()
-                .OwnsOne(p => p.Address)
-                .Property(p => p.Country)
-                .HasColumnName("Country");
+            modelBuilder.Entity<Person>(pb =>
+            {
+                pb.Property(p => p.Country)
+                    .HasColumnName("Country");
+                pb.Property(p => p.ZipCode)
+                    .HasColumnName("ZipCode");
+                pb.OwnsOne(p => p.Address)
+                    .Property(p => p.Country)
+                    .HasColumnName("Country");
+                pb.OwnsOne(p => p.Address)
+                    .Property(p => p.ZipCode)
+                    .HasColumnName("ZipCode");
+            });
 
             modelBuilder
                 .Entity<
diff --git a/test/EFCore.Relational.Tests/Infrastructure/RelationalModelValidatorTest.cs b/test/EFCore.Relational.Tests/Infrastructure/RelationalModelValidatorTest.cs
@@ -421,22 +421,26 @@ public virtual void Detects_incompatible_primary_key_columns_with_shared_table()
     }
 
     [ConditionalFact]
-    public virtual void Passes_on_not_configured_shared_columns_with_shared_table()
+    public virtual void Passes_on_shared_columns_with_shared_table()
     {
         var modelBuilder = CreateConventionModelBuilder();
 
         modelBuilder.Entity<A>().HasOne<B>().WithOne(b => b.A).HasForeignKey<A>(a => a.Id).HasPrincipalKey<B>(b => b.Id).IsRequired();
         modelBuilder.Entity<A>().Property(a => a.P0).HasColumnName(nameof(A.P0));
+        modelBuilder.Entity<A>().Property(a => a.P3).HasColumnName(nameof(A.P3))
+            .HasConversion(e => (long?)e, e => (int?)e);
         modelBuilder.Entity<A>().Property(a => a.P1).IsRequired();
         modelBuilder.Entity<A>().ToTable("Table");
         modelBuilder.Entity<B>().Property(b => b.P0).HasColumnName(nameof(A.P0)).HasColumnType("someInt");
+        modelBuilder.Entity<B>().Property(b => b.P3).HasColumnName(nameof(A.P3))
+            .HasConversion(e => (long)e, e => (int?)e);
         modelBuilder.Entity<B>().ToTable("Table");
 
         Validate(modelBuilder);
     }
 
     [ConditionalFact]
-    public virtual void Throws_on_not_configured_shared_columns_with_shared_table_with_dependents()
+    public virtual void Throws_on_nullable_shared_columns_with_shared_table_with_dependents()
     {
         var modelBuilder = CreateConventionModelBuilder();
 
@@ -450,7 +454,7 @@ public virtual void Throws_on_not_configured_shared_columns_with_shared_table_wi
     }
 
     [ConditionalFact]
-    public virtual void Warns_on_not_configured_shared_columns_with_shared_table()
+    public virtual void Warns_on_no_required_columns_with_shared_table()
     {
         var modelBuilder = CreateConventionModelBuilder();
 
diff --git a/test/EFCore.Specification.Tests/TestModels/UpdatesModel/Address.cs b/test/EFCore.Specification.Tests/TestModels/UpdatesModel/Address.cs
@@ -9,4 +9,5 @@ public class Address
 {
     public string City { get; set; } = null!;
     public Country Country { get; set; }
+    public int? ZipCode { get; set; }
 }
diff --git a/test/EFCore.Specification.Tests/TestModels/UpdatesModel/Person.cs b/test/EFCore.Specification.Tests/TestModels/UpdatesModel/Person.cs
@@ -22,6 +22,7 @@ public Person(string name, Person? parent)
     public string Name { get; set; }
     public int? ParentId { get; set; }
     public string? Country { get; set; }
+    public string? ZipCode { get; set; }
     public Person? Parent { get; set; }
     public Address? Address { get; set; }
 }
diff --git a/test/EFCore.Specification.Tests/UpdatesTestBase.cs b/test/EFCore.Specification.Tests/UpdatesTestBase.cs
@@ -376,7 +376,11 @@ public virtual void Can_change_enums_with_conversion()
         => ExecuteWithStrategyInTransaction(
             context =>
             {
-                var person = new Person("1", null) { Address = new Address { Country = Country.Eswatini, City = "Bulembu" }, Country = "Eswatini" };
+                var person = new Person("1", null)
+                {
+                    Address = new Address { Country = Country.Eswatini, City = "Bulembu" },
+                    Country = "Eswatini"
+                };
 
                 context.Add(person);
 
@@ -385,7 +389,7 @@ public virtual void Can_change_enums_with_conversion()
             context =>
             {
                 var person = context.Set<Person>().Single();
-                person.Address = new Address { Country = Country.Türkiye, City = "Konya" };
+                person.Address = new Address { Country = Country.Türkiye, City = "Konya", ZipCode = 42100 };
                 person.Country = "Türkiye";
 
                 context.SaveChanges();
@@ -396,7 +400,9 @@ public virtual void Can_change_enums_with_conversion()
 
                 Assert.Equal(Country.Türkiye, person.Address!.Country);
                 Assert.Equal("Konya", person.Address.City);
+                Assert.Equal(42100, person.Address.ZipCode);
                 Assert.Equal("Türkiye", person.Country);
+                Assert.Equal("42100", person.ZipCode);
             });
 
     [ConditionalFact]
@@ -644,15 +650,17 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
                 .HasForeignKey(e => e.DependentId)
                 .HasPrincipalKey(e => e.PrincipalId);
 
-            modelBuilder.Entity<Person>()
-                .HasOne(p => p.Parent)
-                .WithMany()
-                .OnDelete(DeleteBehavior.Restrict);
-
-            modelBuilder.Entity<Person>()
-                .OwnsOne(p => p.Address)
-                .Property(p => p.Country)
-                .HasConversion<string>();
+            modelBuilder.Entity<Person>(pb =>
+                {
+                    pb.HasOne(p => p.Parent)
+                        .WithMany()
+                        .OnDelete(DeleteBehavior.Restrict);
+                    pb.OwnsOne(p => p.Address)
+                        .Property(p => p.Country)
+                        .HasConversion<string>();
+                    pb.Property(p => p.ZipCode)
+                        .HasConversion<int?>(v => v == null ? null : int.Parse(v), v => v == null ? null : v.ToString()!);
+                });
 
             modelBuilder.Entity<Category>().HasMany(e => e.ProductCategories).WithOne(e => e.Category)
                 .HasForeignKey(e => e.CategoryId);
diff --git a/test/EFCore.SqlServer.FunctionalTests/Update/MismatchedKeyTypesSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Update/MismatchedKeyTypesSqlServerTest.cs
@@ -361,7 +361,7 @@ public virtual void Queries_work_but_SaveChanges_fails_when_keys_incompatible_in
         Assert.Equal(
             RelationalStrings.StoredKeyTypesNotConvertable(
                 nameof(OptionalSingleBad.PrincipalId), "uniqueidentifier", "bigint", nameof(PrincipalBad.Id)),
-            Assert.Throws<TargetInvocationException>(() => context.SaveChanges()).InnerException!.Message);
+            Assert.Throws<TargetInvocationException>(() => context.SaveChanges()).InnerException!.InnerException!.Message);
     }
 
     protected class MismatchedKeyTypesContextNoFks : MismatchedKeyTypesContext
diff --git a/test/EFCore.SqlServer.FunctionalTests/UpdatesSqlServerTestBase.cs b/test/EFCore.SqlServer.FunctionalTests/UpdatesSqlServerTestBase.cs

Original file line number	Diff line number	Diff line change
`@@ -84,7 +84,7 @@ public virtual Type ProviderClrType`
`84`	`84`	`}`
`85`	`85`
`86`	`86`	`var typeMapping = StoreTypeMapping;`
`87`		`- var providerType = typeMapping.Converter?.ProviderClrType ?? typeMapping.ClrType;`
	`87`	`+ var providerType = typeMapping.Converter?.ProviderClrType.UnwrapNullableType() ?? typeMapping.ClrType;`
`88`	`88`
`89`	`89`	`return _providerClrType = providerType;`
`90`	`90`	`}`
Original file line number	Diff line number	Diff line change
`@@ -50,8 +50,7 @@ private static ColumnAccessors CreateGeneric<TColumn>(IColumn column)`
`50`	`50`	`}`
`51`	`51`
`52`	`52`	`var providerValue = entry.GetCurrentProviderValue(property);`
`53`		`- if (providerValue == null`
`54`		`- && !typeof(TColumn).IsNullableType())`
	`53`	`+ if (providerValue == null)`
`55`	`54`	`{`
`56`	`55`	`return (value!, valueFound);`
`57`	`56`	`}`
`@@ -93,8 +92,7 @@ private static ColumnAccessors CreateGeneric<TColumn>(IColumn column)`
`93`	`92`	`}`
`94`	`93`
`95`	`94`	`var providerValue = entry.GetOriginalProviderValue(property);`
`96`		`- if (providerValue == null`
`97`		`- && !typeof(TColumn).IsNullableType())`
	`95`	`+ if (providerValue == null)`
`98`	`96`	`{`
`99`	`97`	`return (value!, valueFound);`
`100`	`98`	`}`
Original file line number	Diff line number	Diff line change
`@@ -50,11 +50,11 @@ public virtual IComparer<IUpdateEntry> Create(IPropertyBase propertyBase)`
`50`	`50`	`if (IsGenericComparable(providerType, nonNullableProviderType))`
`51`	`51`	`{`
`52`	`52`	`var comparerType = modelType.IsClass`
`53`		`- ? typeof(NullableClassCurrentProviderValueComparer<,>).MakeGenericType(modelType, converter.ProviderClrType)`
	`53`	`+ ? typeof(NullableClassCurrentProviderValueComparer<,>).MakeGenericType(modelType, providerType)`
`54`	`54`	`: modelType == converter.ModelClrType`
`55`		`- ? typeof(CurrentProviderValueComparer<,>).MakeGenericType(modelType, converter.ProviderClrType)`
	`55`	`+ ? typeof(CurrentProviderValueComparer<,>).MakeGenericType(modelType, providerType)`
`56`	`56`	`: typeof(NullableStructCurrentProviderValueComparer<,>).MakeGenericType(`
`57`		`- nonNullableModelType, converter.ProviderClrType);`
	`57`	`+ nonNullableModelType, providerType);`
`58`	`58`
`59`	`59`	`return (IComparer<IUpdateEntry>)Activator.CreateInstance(comparerType, propertyBase, converter)!;`
`60`	`60`	`}`