fix: correctly assign null if target is nullable instead of throwing (#…

…1191)
riok · Mar 25, 2024 · f08b7b4 · f08b7b4
1 parent 1dcb9b6
commit f08b7b4
Show file tree

Hide file tree

Showing 5 changed files with 120 additions and 39 deletions.
diff --git a/...Mapperly/Descriptors/MappingBodyBuilders/BuilderContext/MembersContainerBuilderContext.cs b/...Mapperly/Descriptors/MappingBodyBuilders/BuilderContext/MembersContainerBuilderContext.cs
@@ -30,6 +30,10 @@ public void AddNullDelegateMemberAssignmentMapping(IMemberAssignmentMapping memb
         {
             container.AddNullMemberAssignment(SetterMemberPath.Build(BuilderContext, memberMapping.TargetPath));
         }
+        else if (BuilderContext.Configuration.Mapper.ThrowOnPropertyMappingNullMismatch)
+        {
+            container.ThrowOnSourcePathNull();
+        }
     }
 
     private void AddMemberAssignmentMapping(IMemberAssignmentMappingContainer container, IMemberAssignmentMapping mapping)
@@ -95,7 +99,6 @@ private MemberNullDelegateAssignmentMapping GetOrCreateNullDelegateMappingForPat
         mapping = new MemberNullDelegateAssignmentMapping(
             GetterMemberPath.Build(BuilderContext, nullConditionSourcePath),
             parentMapping,
-            BuilderContext.Configuration.Mapper.ThrowOnPropertyMappingNullMismatch,
             needsNullSafeAccess
         );
         _nullDelegateMappings[nullConditionSourcePath] = mapping;

diff --git a/src/Riok.Mapperly/Descriptors/Mappings/MemberMappings/MemberNullDelegateAssignmentMapping.cs b/src/Riok.Mapperly/Descriptors/Mappings/MemberMappings/MemberNullDelegateAssignmentMapping.cs
@@ -12,13 +12,17 @@ namespace Riok.Mapperly.Descriptors.Mappings.MemberMappings;
 public class MemberNullDelegateAssignmentMapping(
     GetterMemberPath nullConditionalSourcePath,
     IMemberAssignmentMappingContainer parent,
-    bool throwInsteadOfConditionalNullMapping,
     bool needsNullSafeAccess
 ) : MemberAssignmentMappingContainer(parent)
 {
     private readonly GetterMemberPath _nullConditionalSourcePath = nullConditionalSourcePath;
-    private readonly bool _throwInsteadOfConditionalNullMapping = throwInsteadOfConditionalNullMapping;
     private readonly List<SetterMemberPath> _targetsToSetNull = new();
+    private bool _throwOnSourcePathNull;
+
+    public void ThrowOnSourcePathNull()
+    {
+        _throwOnSourcePathNull = true;
+    }
 
     public override IEnumerable<StatementSyntax> Build(TypeMappingBuildContext ctx, ExpressionSyntax targetAccess)
     {
@@ -48,32 +52,21 @@ public override bool Equals(object? obj)
         if (obj.GetType() != GetType())
             return false;
 
-        return Equals((MemberNullDelegateAssignmentMapping)obj);
+        var other = (MemberNullDelegateAssignmentMapping)obj;
+        return _nullConditionalSourcePath.Equals(other._nullConditionalSourcePath);
     }
 
-    public override int GetHashCode()
-    {
-        unchecked
-        {
-            return (_nullConditionalSourcePath.GetHashCode() * 397) ^ _throwInsteadOfConditionalNullMapping.GetHashCode();
-        }
-    }
+    public override int GetHashCode() => _nullConditionalSourcePath.GetHashCode();
 
     public static bool operator ==(MemberNullDelegateAssignmentMapping? left, MemberNullDelegateAssignmentMapping? right) =>
         Equals(left, right);
 
     public static bool operator !=(MemberNullDelegateAssignmentMapping? left, MemberNullDelegateAssignmentMapping? right) =>
         !Equals(left, right);
 
-    protected bool Equals(MemberNullDelegateAssignmentMapping other)
-    {
-        return _nullConditionalSourcePath.Equals(other._nullConditionalSourcePath)
-            && _throwInsteadOfConditionalNullMapping == other._throwInsteadOfConditionalNullMapping;
-    }
-
     private IEnumerable<StatementSyntax>? BuildElseClause(TypeMappingBuildContext ctx, ExpressionSyntax targetAccess)
     {
-        if (_throwInsteadOfConditionalNullMapping)
+        if (_throwOnSourcePathNull)
         {
             // throw new ArgumentNullException
             var nameofSourceAccess = _nullConditionalSourcePath.BuildAccess(ctx.Source, false, false, true);

diff --git a/test/Riok.Mapperly.Tests/Mapping/EnumerableTest.cs b/test/Riok.Mapperly.Tests/Mapping/EnumerableTest.cs
@@ -63,14 +63,14 @@ public void ArrayOfPrimitiveTypesToNullablePrimitiveTypesArray()
     [Fact]
     public void ArrayCustomClassToArrayCustomClass()
     {
-        var source = TestSourceBuilder.Mapping("B[]", "B[]", "class B { public int Value {get; set; }}");
+        var source = TestSourceBuilder.Mapping("B[]", "B[]", "class B { public int Value { get; set; } }");
         TestHelper.GenerateMapper(source).Should().HaveSingleMethodBody("return source;");
     }
 
     [Fact]
     public void ArrayCustomClassNullableToArrayCustomClassNonNullable()
     {
-        var source = TestSourceBuilder.Mapping("B?[]", "B[]", "class B { public int Value {get; set; }}");
+        var source = TestSourceBuilder.Mapping("B?[]", "B[]", "class B { public int Value { get; set; } }");
         TestHelper
             .GenerateMapper(source)
             .Should()
@@ -89,7 +89,7 @@ public void ArrayCustomClassNullableToArrayCustomClassNonNullable()
     [Fact]
     public void ArrayCustomClassNonNullableToArrayCustomClassNullable()
     {
-        var source = TestSourceBuilder.Mapping("B[]", "B?[]", "class B { public int Value {get; set; }}");
+        var source = TestSourceBuilder.Mapping("B[]", "B?[]", "class B { public int Value { get; set; } }");
         TestHelper.GenerateMapper(source).Should().HaveSingleMethodBody("return (global::B?[])source;");
     }
 

diff --git a/test/Riok.Mapperly.Tests/Mapping/ObjectPropertyNullableTest.cs b/test/Riok.Mapperly.Tests/Mapping/ObjectPropertyNullableTest.cs
@@ -1,3 +1,5 @@
+using Riok.Mapperly.Diagnostics;
+
 namespace Riok.Mapperly.Tests.Mapping;
 
 public class ObjectPropertyNullableTest
@@ -145,8 +147,8 @@ public void NullableClassToNonNullableClassProperty()
             "B",
             "class A { public C? Value { get; set; } }",
             "class B { public D Value { get; set; } }",
-            "class C { public string V {get; set; } }",
-            "class D { public string V {get; set; } }"
+            "class C { public string V { get; set; } }",
+            "class D { public string V { get; set; } }"
         );
 
         TestHelper
@@ -280,8 +282,8 @@ public void NonNullableClassToNullableClassProperty()
             "B",
             "class A { public C Value { get; set; } }",
             "class B { public D? Value { get; set; } }",
-            "class C { public string V {get; set; } }",
-            "class D { public string V {get; set; } }"
+            "class C { public string V { get; set; } }",
+            "class D { public string V { get; set; } }"
         );
 
         TestHelper
@@ -304,8 +306,8 @@ public void NullableClassToNullableClassProperty()
             "B",
             "class A { public C? Value { get; set; } }",
             "class B { public D? Value { get; set; } }",
-            "class C { public string V {get; set; } }",
-            "class D { public string V {get; set; } }"
+            "class C { public string V { get; set; } }",
+            "class D { public string V { get; set; } }"
         );
 
         TestHelper
@@ -339,8 +341,8 @@ TestSourceBuilderOptions.Default with
             },
             "class A { public C? Value { get; set; } }",
             "class B { public D? Value { get; set; } }",
-            "class C { public string V {get; set; } }",
-            "class D { public string V {get; set; } }"
+            "class C { public string V { get; set; } }",
+            "class D { public string V { get; set; } }"
         );
 
         TestHelper
@@ -371,8 +373,8 @@ TestSourceBuilderOptions.Default with
             },
             "class A { public C? Value { get; set; } }",
             "class B { public D? Value { get; set; } }",
-            "class C { public string V {get; set; } }",
-            "class D { public string V {get; set; } }"
+            "class C { public string V { get; set; } }",
+            "class D { public string V { get; set; } }"
         );
 
         TestHelper
@@ -402,8 +404,8 @@ public void DisabledNullableClassPropertyToNonNullableProperty()
             "B",
             "#nullable disable\n class A { public C Value { get; set; } }\n#nullable enable",
             "class B { public D Value { get; set; } }",
-            "class C { public string V {get; set; } }",
-            "class D { public string V {get; set; } }"
+            "class C { public string V { get; set; } }",
+            "class D { public string V { get; set; } }"
         );
 
         TestHelper
@@ -429,8 +431,8 @@ public void NullableClassPropertyToDisabledNullableProperty()
             "B",
             "class A { public C? Value { get; set; } }",
             "#nullable disable\n class B { public D Value { get; set; } }\n#nullable enable",
-            "class C { public string V {get; set; } }",
-            "class D { public string V {get; set; } }"
+            "class C { public string V { get; set; } }",
+            "class D { public string V { get; set; } }"
         );
 
         TestHelper
@@ -493,8 +495,43 @@ TestSourceBuilderOptions.Default with
             },
             "class A { public C? Value { get; set; } }",
             "class B { public D Value { get; set; } }",
-            "class C { public string V {get; set; } }",
-            "class D { public string V {get; set; } }"
+            "class C { public string V { get; set; } }",
+            "class D { public string V { get; set; } }"
+        );
+
+        TestHelper
+            .GenerateMapper(source)
+            .Should()
+            .HaveMapMethodBody(
+                """
+                var target = new global::B();
+                if (source.Value != null)
+                {
+                    target.Value = MapToD(source.Value);
+                }
+                else
+                {
+                    throw new System.ArgumentNullException(nameof(source.Value));
+                }
+                return target;
+                """
+            );
+    }
+
+    [Fact]
+    public void NullableClassToNullableClassPropertyThrowShouldSetNull()
+    {
+        var source = TestSourceBuilder.Mapping(
+            "A",
+            "B",
+            TestSourceBuilderOptions.Default with
+            {
+                ThrowOnPropertyMappingNullMismatch = true
+            },
+            "class A { public C? Value { get; set; } }",
+            "class B { public D? Value { get; set; } }",
+            "class C { public string V { get; set; } }",
+            "class D { public string V { get; set; } }"
         );
 
         TestHelper
@@ -508,6 +545,54 @@ TestSourceBuilderOptions.Default with
                     target.Value = MapToD(source.Value);
                 }
                 else
+                {
+                    target.Value = null;
+                }
+                return target;
+                """
+            );
+    }
+
+    [Fact]
+    public void NullableClassToNullableClassFlattenedPropertyThrow()
+    {
+        // the flattened property is not nullable
+        // therefore if source.Value is null
+        // an exception should be thrown
+        // instead of assigning null to target.Value.
+        var source = TestSourceBuilder.MapperWithBodyAndTypes(
+            """
+            [MapProperty("Value", "Value")]
+            [MapProperty("Value.Flattened", "ValueFlattened")]
+            partial B Map(A source);
+            """,
+            TestSourceBuilderOptions.Default with
+            {
+                ThrowOnPropertyMappingNullMismatch = true
+            },
+            "class A { public C? Value { get; set; } }",
+            "class B { public D? Value { get; set; } public string ValueFlattened { get; set; } }",
+            "class C { public string V { get; set; } public string Flattened { get; set; } }",
+            "class D { public string V { get; set; } }"
+        );
+
+        TestHelper
+            .GenerateMapper(source, TestHelperOptions.AllowInfoDiagnostics)
+            .Should()
+            .HaveDiagnostic(
+                DiagnosticDescriptors.SourceMemberNotMapped,
+                "The member Flattened on the mapping source type C is not mapped to any member on the mapping target type D"
+            )
+            .HaveAssertedAllDiagnostics()
+            .HaveMapMethodBody(
+                """
+                var target = new global::B();
+                if (source.Value != null)
+                {
+                    target.Value = MapToD(source.Value);
+                    target.ValueFlattened = source.Value.Flattened;
+                }
+                else
                 {
                     throw new System.ArgumentNullException(nameof(source.Value));
                 }

diff --git a/test/Riok.Mapperly.Tests/Mapping/ValueTupleTest.cs b/test/Riok.Mapperly.Tests/Mapping/ValueTupleTest.cs
@@ -216,7 +216,7 @@ public void ClassToTuple()
         var source = TestSourceBuilder.Mapping(
             "A",
             "(int B, string C)",
-            "public class A { public int B { get;set;} public int C {get;set;} }"
+            "public class A { public int B { get; set; } public int C { get; set; } }"
         );
 
         TestHelper
@@ -280,7 +280,7 @@ public void ClassToTupleWithIgnoredSource()
             [MapperIgnoreSource("A")]
             partial (int, int) Map(B source);
             """,
-            "public class B { public int Item1 { get;set;} public int A {get;set;} public int Item2 {get;set;} }"
+            "public class B { public int Item1 { get;set;} public int A { get; set; } public int Item2 {get;set;} }"
         );
 
         TestHelper