Fix for some minor bugs around JSON mappings

#29214 - Json column: constructor with optional enum-parameter throws ArgumentException When producing shaper code for json nullable properties with converters we check if the converter handles null, if not we need to peek into the json value and only pass it thru converter if it's not null, and return null otherwise #29217 - Json: nullable enums on entities mapped to json don't use enum-to-int converter, rather than string, like for non-nullable enums In convention code unwrap nullable type when testing for enum properties that we then apply converter to string #29225 - Json: projecting nullable scalar produces debug.assert in SqlExpression ctor Unwrap nullable type from property before passing it as type to SqlExpression ctor Fixes #29214 Fixes #29217 Fixes #29225
dotnet · Sep 28, 2022 · ddf33c5 · ddf33c5
1 parent 19d5f60
commit ddf33c5
Show file tree

Hide file tree

Showing 9 changed files with 199 additions and 21 deletions.
diff --git a/src/EFCore.Relational/Metadata/Conventions/RelationalMapToJsonConvention.cs b/src/EFCore.Relational/Metadata/Conventions/RelationalMapToJsonConvention.cs
@@ -70,7 +70,7 @@ public virtual void ProcessModelFinalizing(
     {
         foreach (var jsonEntityType in modelBuilder.Metadata.GetEntityTypes().Where(e => e.IsMappedToJson()))
         {
-            foreach (var enumProperty in jsonEntityType.GetDeclaredProperties().Where(p => p.ClrType.IsEnum))
+            foreach (var enumProperty in jsonEntityType.GetDeclaredProperties().Where(p => p.ClrType.UnwrapNullableType().IsEnum))
             {
                 // by default store enums as strings - values should be human-readable
                 enumProperty.Builder.HasConversion(typeof(string));

diff --git a/...uery/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs b/...uery/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs
@@ -1504,21 +1504,67 @@ private Expression CreateExtractJsonPropertyExpression(
             Expression resultExpression;
             if (property.GetTypeMapping().Converter is ValueConverter converter)
             {
-                resultExpression = Expression.Call(
-                    ExtractJsonPropertyMethodInfo,
-                    jsonElementParameter,
-                    Expression.Constant(property.GetJsonPropertyName()),
-                    Expression.Constant(converter.ProviderClrType));
-
-                if (resultExpression.Type != converter.ProviderClrType)
+                var providerClrType = converter.ProviderClrType.MakeNullable(property.IsNullable);
+                if (!property.IsNullable || converter.ConvertsNulls)
                 {
-                    resultExpression = Expression.Convert(resultExpression, converter.ProviderClrType);
+                    resultExpression = Expression.Call(
+                        ExtractJsonPropertyMethodInfo,
+                        jsonElementParameter,
+                        Expression.Constant(property.GetJsonPropertyName()),
+                        Expression.Constant(providerClrType));
+
+                    if (resultExpression.Type != providerClrType)
+                    {
+                        resultExpression = Expression.Convert(resultExpression, providerClrType);
+                    }
+
+                    resultExpression = ReplacingExpressionVisitor.Replace(
+                        converter.ConvertFromProviderExpression.Parameters.Single(),
+                        resultExpression,
+                        converter.ConvertFromProviderExpression.Body);
                 }
+                else
+                {
+                    // property is nullable and the converter can't handle nulls
+                    // we need to peek into the JSON value and only pass it thru converter if it's not null
+                    var testExpression = Expression.NotEqual(
+                        Expression.Convert(
+                            Expression.Call(
+                                ExtractJsonPropertyMethodInfo,
+                                jsonElementParameter,
+                                Expression.Constant(property.GetJsonPropertyName()),
+                                Expression.Constant(providerClrType)),
+                            providerClrType),
+                        Expression.Default(providerClrType));
 
-                resultExpression = ReplacingExpressionVisitor.Replace(
-                    converter.ConvertFromProviderExpression.Parameters.Single(),
-                    resultExpression,
-                    converter.ConvertFromProviderExpression.Body);
+                    resultExpression = Expression.Call(
+                        ExtractJsonPropertyMethodInfo,
+                        jsonElementParameter,
+                        Expression.Constant(property.GetJsonPropertyName()),
+                        Expression.Constant(converter.ProviderClrType));
+
+                    if (resultExpression.Type != converter.ProviderClrType)
+                    {
+                        resultExpression = Expression.Convert(resultExpression, converter.ProviderClrType);
+                    }
+
+                    var replacedExpression = ReplacingExpressionVisitor.Replace(
+                        converter.ConvertFromProviderExpression.Parameters.Single(),
+                        resultExpression,
+                        converter.ConvertFromProviderExpression.Body);
+
+                    if (replacedExpression.Type != property.ClrType)
+                    {
+                        replacedExpression = Expression.Convert(replacedExpression, property.ClrType);
+                    }
+
+                    var condition = Expression.Condition(
+                        testExpression,
+                        replacedExpression,
+                        Expression.Default(property.ClrType));
+
+                    resultExpression = condition;
+                }
             }
             else
             {

diff --git a/src/EFCore.Relational/Query/SqlExpressions/JsonScalarExpression.cs b/src/EFCore.Relational/Query/SqlExpressions/JsonScalarExpression.cs
@@ -26,7 +26,7 @@ public JsonScalarExpression(
         IProperty property,
         IReadOnlyList<PathSegment> path,
         bool nullable)
-        : this(jsonColumn, path, property.ClrType, property.FindRelationalTypeMapping()!, nullable)
+        : this(jsonColumn, path, property.ClrType.UnwrapNullableType(), property.FindRelationalTypeMapping()!, nullable)
     {
     }
 

diff --git a/test/EFCore.Relational.Specification.Tests/Query/JsonQueryFixtureBase.cs b/test/EFCore.Relational.Specification.Tests/Query/JsonQueryFixtureBase.cs
@@ -297,6 +297,7 @@ private static void AssertOwnedBranch(JsonOwnedBranch expected, JsonOwnedBranch
         Assert.Equal(expected.Date, actual.Date);
         Assert.Equal(expected.Fraction, actual.Fraction);
         Assert.Equal(expected.Enum, actual.Enum);
+        Assert.Equal(expected.NullableEnum, actual.NullableEnum);
 
         AssertOwnedLeaf(expected.OwnedReferenceLeaf, actual.OwnedReferenceLeaf);
         Assert.Equal(expected.OwnedCollectionLeaf.Count, actual.OwnedCollectionLeaf.Count);
@@ -345,6 +346,12 @@ public static void AssertAllTypes(JsonOwnedAllTypes expected, JsonOwnedAllTypes
         Assert.Equal(expected.TestUnsignedInt16, actual.TestUnsignedInt16);
         Assert.Equal(expected.TestUnsignedInt32, actual.TestUnsignedInt32);
         Assert.Equal(expected.TestUnsignedInt64, actual.TestUnsignedInt64);
+        Assert.Equal(expected.TestNullableInt32, actual.TestNullableInt32);
+        Assert.Equal(expected.TestEnum, actual.TestEnum);
+        Assert.Equal(expected.TestEnumWithIntConverter, actual.TestEnumWithIntConverter);
+        Assert.Equal(expected.TestNullableEnum, actual.TestNullableEnum);
+        Assert.Equal(expected.TestNullableEnumWithIntConverter, actual.TestNullableEnumWithIntConverter);
+        Assert.Equal(expected.TestNullableEnumWithConverterThatHandlesNulls, actual.TestNullableEnumWithConverterThatHandlesNulls);
     }
 
     protected override string StoreName { get; } = "JsonQueryTest";
@@ -496,6 +503,27 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
             {
                 b.ToJson();
                 b.Property(x => x.TestDecimal).HasPrecision(18, 3);
+                b.Property(x => x.TestEnumWithIntConverter).HasConversion<int>();
+                b.Property(x => x.TestNullableEnumWithIntConverter).HasConversion<int>();
+                b.Property(x => x.TestNullableEnumWithConverterThatHandlesNulls).HasConversion(
+                    new ValueConverter<JsonEnum?, string>(
+                        x => x == null
+                            ? "Null"
+                            : x == JsonEnum.One
+                                ? "One"
+                                : x == JsonEnum.Two
+                                    ? "Two"
+                                    : x == JsonEnum.Three
+                                        ? "Three"
+                                        : "INVALID",
+                        x => x == "One"
+                            ? JsonEnum.One
+                            : x == "Two"
+                                ? JsonEnum.Two
+                                : x == "Three"
+                                    ? JsonEnum.Three
+                                    : null,
+                        convertsNulls: true));
             });
         modelBuilder.Entity<JsonEntityAllTypes>().OwnsMany(
             x => x.Collection, b =>

diff --git a/test/EFCore.Relational.Specification.Tests/Query/JsonQueryTestBase.cs b/test/EFCore.Relational.Specification.Tests/Query/JsonQueryTestBase.cs
@@ -726,7 +726,7 @@ public virtual Task Json_all_types_entity_projection(bool async)
         => AssertQuery(
             async,
             ss => ss.Set<JsonEntityAllTypes>(),
-            entryCount: 3);
+            entryCount: 6);
 
     [ConditionalTheory]
     [MemberData(nameof(IsAsyncData))]
@@ -752,6 +752,12 @@ public virtual Task Json_all_types_projection_individual_properties(bool async)
                     x.Reference.TestTimeSpan,
                     x.Reference.TestUnsignedInt16,
                     x.Reference.TestUnsignedInt32,
-                    x.Reference.TestUnsignedInt64
+                    x.Reference.TestUnsignedInt64,
+                    x.Reference.TestNullableInt32,
+                    x.Reference.TestEnum,
+                    x.Reference.TestEnumWithIntConverter,
+                    x.Reference.TestNullableEnum,
+                    x.Reference.TestNullableEnumWithIntConverter,
+                    x.Reference.TestNullableEnumWithConverterThatHandlesNulls,
                 }));
 }
diff --git a/test/EFCore.Relational.Specification.Tests/TestModels/JsonQuery/JsonOwnedAllTypes.cs b/test/EFCore.Relational.Specification.Tests/TestModels/JsonQuery/JsonOwnedAllTypes.cs
@@ -22,4 +22,10 @@ public class JsonOwnedAllTypes
     public ulong TestUnsignedInt64 { get; set; }
     public char TestCharacter { get; set; }
     public sbyte TestSignedByte { get; set; }
+    public int? TestNullableInt32 { get; set; }
+    public JsonEnum TestEnum { get; set; }
+    public JsonEnum TestEnumWithIntConverter { get; set; }
+    public JsonEnum? TestNullableEnum { get; set; }
+    public JsonEnum? TestNullableEnumWithIntConverter { get; set; }
+    public JsonEnum? TestNullableEnumWithConverterThatHandlesNulls { get; set; }
 }
diff --git a/test/EFCore.Relational.Specification.Tests/TestModels/JsonQuery/JsonOwnedBranch.cs b/test/EFCore.Relational.Specification.Tests/TestModels/JsonQuery/JsonOwnedBranch.cs
@@ -10,6 +10,8 @@ public class JsonOwnedBranch
 
     public JsonEnum Enum { get; set; }
 
+    public JsonEnum? NullableEnum { get; set; }
+
     public JsonOwnedLeaf OwnedReferenceLeaf { get; set; }
     public List<JsonOwnedLeaf> OwnedCollectionLeaf { get; set; }
 }