Fix serialization of data-records with get-only properties (#6204)

* Fix serialization of data-records with get-only properties

* even less nesting

* asserts

Robust.Shared/Serialization/Manager/Definition/DataDefinition.cs

@@ -332,21 +332,15 @@ namespace Robust.Shared.Serialization.Manager.Definition
             inheritanceBehavior ??= InheritanceBehavior.Default;
 
             if (fieldInfo.HasAttribute<AlwaysPushInheritanceAttribute>(true))
-            {
                 inheritanceBehavior = InheritanceBehavior.Always;
-            }
             else if (fieldInfo.HasAttribute<NeverPushInheritanceAttribute>(true))
-            {
                 inheritanceBehavior = InheritanceBehavior.Never;
-            }
 
             if (fieldInfo is SpecificPropertyInfo propertyInfo)
             {
                 // We only want the most overriden instance of a property for the type we are working with
                 if (!propertyInfo.IsMostOverridden(typeof(T)))
-                {
                     return false;
-                }
 
                 if (propertyInfo.PropertyInfo.GetMethod == null)
                 {
@@ -355,36 +349,54 @@ namespace Robust.Shared.Serialization.Manager.Definition
                 }
             }
 
-            if (!fieldInfo.TryGetAttribute<DataFieldAttribute>(out var dataFieldAttribute, true))
+            // Most data fields have an explicit data field attribute
+            if (fieldInfo.TryGetAttribute<DataFieldAttribute>(out var dataFieldAttribute, true))
+                return GatherDataFieldData(fieldInfo, out dataFieldBaseAttribute, ref backingField, dataFieldAttribute);
+
+            if (fieldInfo.TryGetAttribute<IncludeDataFieldAttribute>(out var includeDataFieldAttribute, true))
             {
-                if (!fieldInfo.TryGetAttribute<IncludeDataFieldAttribute>(out var includeDataFieldAttribute, true))
-                {
-                    var potentialBackingField = fieldInfo.GetBackingField();
-                    if (potentialBackingField != null)
-                    {
-                        return GatherFieldData(potentialBackingField, out dataFieldBaseAttribute,
-                            out backingField, ref inheritanceBehavior);
-                    }
-                    return true;
-                }
                 dataFieldBaseAttribute = includeDataFieldAttribute;
+                return true;
             }
-            else
+
+            // This field/property has no explicit data field related annotations. However, things like
+            // DataRecordAttribute will cause all fields to be interpreted as data fields, so we still handle them
+
+            if (fieldInfo is not SpecificPropertyInfo)
+                return true;
+
+            var potentialBackingField = fieldInfo.GetBackingField();
+            if (potentialBackingField == null)
+                return false;
+
+            return GatherFieldData(potentialBackingField,
+                out dataFieldBaseAttribute,
+                out backingField,
+                ref inheritanceBehavior);
+        }
+
+        private static bool GatherDataFieldData(
+            AbstractFieldInfo fieldInfo,
+            out DataFieldBaseAttribute dataFieldBaseAttribute,
+            ref AbstractFieldInfo backingField,
+            DataFieldAttribute dataFieldAttribute)
+        {
+            dataFieldBaseAttribute = dataFieldAttribute;
+
+            if (fieldInfo is not SpecificPropertyInfo property
+                || dataFieldAttribute.ReadOnly
+                || property.PropertyInfo.SetMethod != null)
             {
-                dataFieldBaseAttribute = dataFieldAttribute;
-
-                if (fieldInfo is SpecificPropertyInfo property && !dataFieldAttribute.ReadOnly && property.PropertyInfo.SetMethod == null)
-                {
-                    if (!property.TryGetBackingField(out var backingFieldInfo))
-                    {
-                        Logger.ErrorS(LogCategory, $"Property {property} in type {property.DeclaringType} is annotated with DataFieldAttribute as non-readonly but has no auto-setter");
-                        return false;
-                    }
-
-                    backingField = backingFieldInfo;
-                }
+                return true;
             }
 
+            if (!property.TryGetBackingField(out var backingFieldInfo))
+            {
+                Logger.ErrorS(LogCategory, $"Property {property} in type {property.DeclaringType} is annotated with DataFieldAttribute as non-readonly but has no auto-setter");
+                return false;
+            }
+
+            backingField = backingFieldInfo;
             return true;
         }
 
@@ -424,6 +436,9 @@ namespace Robust.Shared.Serialization.Manager.Definition
                 fieldDefinitions.Add(fieldDefinition);
             }
 
+            // There should be no duplicates
+            // I.e., we haven't accidentally included a property's backing field twice?
+            DebugTools.Assert(fieldDefinitions.Select(x=> x.FieldInfo).Distinct().Count() == fieldDefinitions.Count);
             return fieldDefinitions;
         }
     }

Robust.UnitTesting/Shared/Serialization/DataRecordTest.cs

@@ -1,4 +1,5 @@
-using NUnit.Framework;
+using System.Numerics;
+using NUnit.Framework;
 using Robust.Shared.IoC;
 using Robust.Shared.Prototypes;
 using Robust.Shared.Serialization.Manager;
@@ -48,6 +49,23 @@ public sealed partial class DataRecordTest : SerializationTest
     [DataRecord]
     public record struct DataRecordStruct(IntStruct Struct, string String, int Integer);
 
+    [DataRecord]
+    public record struct DataRecordWithProperties
+    {
+        public Vector2 Position;
+        public int Foo { get; }
+        public int Bar { get; set; }
+        public float X => Position.X;
+    }
+
+    [DataRecord]
+    public readonly record struct ReadonlyDataRecord
+    {
+        public readonly Vector2 Position;
+        public int Foo { get; }
+        public float X => Position.X;
+    }
+
     [Test]
     public void TwoIntRecordTest()
     {
@@ -244,4 +262,67 @@ public sealed partial class DataRecordTest : SerializationTest
             Assert.That(integerNode!.Value, Is.EqualTo("2"));
         });
     }
+
+    [Test]
+    public void DataRecordWithPropertiesTest()
+    {
+        var mapping = new MappingDataNode
+        {
+            ["foo"] = new ValueDataNode("1"),
+            ["bar"] = new ValueDataNode("2"),
+            ["position"] = new ValueDataNode("3, .4"),
+        };
+        var val = Serialization.Read<DataRecordWithProperties>(mapping);
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(val.Foo, Is.EqualTo(1));
+            Assert.That(val.Bar, Is.EqualTo(2));
+            Assert.That(val.Position, Is.EqualTo(new Vector2(3, .4f)));
+        });
+
+        var newMapping = Serialization.WriteValueAs<MappingDataNode>(val);
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(newMapping, Has.Count.EqualTo(3));
+            Assert.That(newMapping.TryGet<ValueDataNode>("foo", out var node));
+            Assert.That(node!.Value, Is.EqualTo("1"));
+
+            Assert.That(newMapping.TryGet<ValueDataNode>("bar", out node));
+            Assert.That(node!.Value, Is.EqualTo("2"));
+
+            Assert.That(newMapping.TryGet<ValueDataNode>("position", out node));
+            Assert.That(node!.Value, Is.EqualTo("3,0.4"));
+        });
+    }
+
+    [Test]
+    public void ReadonlyDataRecordTest()
+    {
+        var mapping = new MappingDataNode
+        {
+            ["foo"] = new ValueDataNode("1"),
+            ["position"] = new ValueDataNode("3, .4"),
+        };
+        var val = Serialization.Read<ReadonlyDataRecord>(mapping);
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(val.Foo, Is.EqualTo(1));
+            Assert.That(val.Position, Is.EqualTo(new Vector2(3, .4f)));
+        });
+
+        var newMapping = Serialization.WriteValueAs<MappingDataNode>(val);
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(newMapping, Has.Count.EqualTo(2));
+            Assert.That(newMapping.TryGet<ValueDataNode>("foo", out var node));
+            Assert.That(node!.Value, Is.EqualTo("1"));
+
+            Assert.That(newMapping.TryGet<ValueDataNode>("position", out node));
+            Assert.That(node!.Value, Is.EqualTo("3,0.4"));
+        });
+    }
 }