Fix value conversion of array over nullable (#2197)

Fixes #2189

Author: roji

src/EFCore.PG/Storage/ValueConversion/NpgsqlArrayConverter.cs

@@ -19,12 +19,12 @@ public NpgsqlArrayConverter(ValueConverter elementConverter)
             throw new ArgumentException("Can only convert between arrays");
         }
 
-        if (modelElementType.UnwrapNullableType() != elementConverter.ModelClrType)
+        if (modelElementType.UnwrapNullableType() != elementConverter.ModelClrType.UnwrapNullableType())
         {
             throw new ArgumentException($"The element's value converter model type ({elementConverter.ModelClrType}), doesn't match the array's ({modelElementType})");
         }
 
-        if (providerElementType.UnwrapNullableType() != elementConverter.ProviderClrType)
+        if (providerElementType.UnwrapNullableType() != elementConverter.ProviderClrType.UnwrapNullableType())
         {
             throw new ArgumentException($"The element's value converter provider type ({elementConverter.ProviderClrType}), doesn't match the array's ({providerElementType})");
         }
@@ -54,7 +54,7 @@ private static Expression<Func<TInput, TOutput>> ArrayConversionExpression<TInpu
 
         // elementConversionExpression is always over non-nullable value types. If the array is over nullable types,
         // we need to sanitize via an external null check.
-        if (inputElementType.IsGenericType && inputElementType.GetGenericTypeDefinition() == typeof(Nullable<>))
+        if (inputElementType.IsNullableType() && outputElementType.IsNullableType())
         {
             // p => p is null ? null : elementConversionExpression(p)
             var p = Expression.Parameter(inputElementType, "foo");
@@ -65,7 +65,12 @@ private static Expression<Func<TInput, TOutput>> ArrayConversionExpression<TInpu
                     Expression.Convert(
                         Expression.Invoke(
                             elementConversionExpression,
-                            Expression.Convert(p, inputElementType.UnwrapNullableType())),
+                            // The user-provided conversion lambda typically accepts non-nullable (value) types, with EF Core doing the
+                            // null-sanitization and conversion to non-nullable; do this here unless the user-provided lambda happens to
+                            // accept a nullable value type parameter.
+                            elementConversionExpression.Parameters[0].Type.IsNullableType()
+                                ? p
+                                : Expression.Convert(p, inputElementType.UnwrapNullableType())),
                         outputElementType)),
                 p);
         }

test/EFCore.PG.FunctionalTests/Query/ArrayArrayQueryTest.cs

@@ -54,7 +54,7 @@ public IReadOnlyDictionary<Type, object> GetEntityAsserters()
                         Assert.Equal(ee.NullableStringList, ee.NullableStringList);
                         Assert.Equal(ee.NullableText, ee.NullableText);
                         Assert.Equal(ee.NonNullableText, ee.NonNullableText);
-                        Assert.Equal(ee.ValueConvertedScalar, ee.ValueConvertedScalar);
+                        Assert.Equal(ee.EnumConvertedToInt, ee.EnumConvertedToInt);
                         Assert.Equal(ee.ValueConvertedArray, ee.ValueConvertedArray);
                         Assert.Equal(ee.ValueConvertedList, ee.ValueConvertedList);
                         Assert.Equal(ee.Byte, ee.Byte);
@@ -76,4 +76,4 @@ public IReadOnlyDictionary<Type, object> GetEntityAsserters()
                 }
             }
         }.ToDictionary(e => e.Key, e => (object)e.Value);
-}
+}

test/EFCore.PG.FunctionalTests/Query/ArrayListQueryTest.cs

@@ -245,7 +245,19 @@ public virtual async Task Nullable_array_column_Contains_literal_item(bool async
 
     [ConditionalTheory]
     [MemberData(nameof(IsAsyncData))]
-    public abstract Task Array_param_Contains_value_converted_column(bool async);
+    public abstract Task Array_param_Contains_value_converted_column_enum_to_int(bool async);
+
+    [ConditionalTheory]
+    [MemberData(nameof(IsAsyncData))]
+    public abstract Task Array_param_Contains_value_converted_column_enum_to_string(bool async);
+
+    [ConditionalTheory]
+    [MemberData(nameof(IsAsyncData))]
+    public abstract Task Array_param_Contains_value_converted_column_nullable_enum_to_string(bool async);
+
+    [ConditionalTheory]
+    [MemberData(nameof(IsAsyncData))]
+    public abstract Task Array_param_Contains_value_converted_column_nullable_enum_to_string_with_non_nullable_lambda(bool async);
 
     [ConditionalTheory]
     [MemberData(nameof(IsAsyncData))]

test/EFCore.PG.FunctionalTests/Query/ArrayQueryFixture.cs

@@ -17,8 +17,11 @@ public class ArrayEntity
     public List<string?> NullableStringList { get; set; } = null!;
     public string? NullableText { get; set; }
     public string NonNullableText { get; set; } = null!;
-    public SomeEnum ValueConvertedScalar { get; set; }
+    public SomeEnum EnumConvertedToInt { get; set; }
+    public SomeEnum EnumConvertedToString { get; set; }
+    public SomeEnum? NullableEnumConvertedToString { get; set; }
+    public SomeEnum? NullableEnumConvertedToStringWithNonNullableLambda { get; set; }
     public SomeEnum[] ValueConvertedArray { get; set; } = null!;
     public List<SomeEnum> ValueConvertedList { get; set; } = null!;
     public byte Byte { get; set; }
-}
+}

test/EFCore.PG.FunctionalTests/Query/ArrayQueryTest.cs

@@ -12,9 +12,18 @@ protected override void OnModelCreating(ModelBuilder modelBuilder)
             e =>
             {
                 // We do negative to make sure our value converter is properly used, and not the built-in one
-                e.Property(ae => ae.ValueConvertedScalar)
+                e.Property(ae => ae.EnumConvertedToInt)
                     .HasConversion(w => -(int)w, v => (SomeEnum)(-v));
 
+                e.Property(ae => ae.EnumConvertedToString)
+                    .HasConversion(w => w.ToString(), v => Enum.Parse<SomeEnum>(v));
+
+                e.Property(ae => ae.NullableEnumConvertedToString)
+                    .HasConversion(w => w.ToString(), v => Enum.Parse<SomeEnum>(v));
+
+                e.Property(ae => ae.NullableEnumConvertedToStringWithNonNullableLambda)
+                    .HasConversion(new ValueConverter<SomeEnum, string>(w => w.ToString(), v => Enum.Parse<SomeEnum>(v)));
+
                 e.Property(ae => ae.ValueConvertedArray)
                     .HasPostgresArrayConversion(w => -(int)w, v => (SomeEnum)(-v));
 
@@ -36,4 +45,4 @@ public static void Seed(ArrayQueryContext context)
         );
         context.SaveChanges();
     }
-}
+}

test/EFCore.PG.FunctionalTests/TestModels/Array/ArrayEntity.cs

@@ -42,7 +42,10 @@ public static IReadOnlyList<ArrayEntity> CreateArrayEntities()
                 NullableStringList = new List<string> { "3", "4", null},
                 NullableText = "foo",
                 NonNullableText = "foo",
-                ValueConvertedScalar = SomeEnum.One,
+                EnumConvertedToInt = SomeEnum.One,
+                EnumConvertedToString = SomeEnum.One,
+                NullableEnumConvertedToString = SomeEnum.One,
+                NullableEnumConvertedToStringWithNonNullableLambda = SomeEnum.One,
                 ValueConvertedArray = new[] { SomeEnum.Eight, SomeEnum.Nine },
                 ValueConvertedList = new List<SomeEnum> { SomeEnum.Eight, SomeEnum.Nine },
                 Byte = 10
@@ -62,7 +65,10 @@ public static IReadOnlyList<ArrayEntity> CreateArrayEntities()
                 NullableStringList = new List<string> { "5", "6", "7", "8" },
                 NullableText = "bar",
                 NonNullableText = "bar",
-                ValueConvertedScalar = SomeEnum.Two,
+                EnumConvertedToInt = SomeEnum.Two,
+                EnumConvertedToString = SomeEnum.Two,
+                NullableEnumConvertedToString = SomeEnum.Two,
+                NullableEnumConvertedToStringWithNonNullableLambda = SomeEnum.Two,
                 ValueConvertedArray = new[] { SomeEnum.Nine, SomeEnum.Ten },
                 ValueConvertedList = new List<SomeEnum> { SomeEnum.Nine, SomeEnum.Ten },
                 Byte = 20
@@ -78,4 +84,4 @@ public static IReadOnlyList<ArrayContainerEntity> CreateContainerEntities()
                 ArrayEntities = CreateArrayEntities().ToList()
             }
         };
-}
+}