Added support for IReadOnlyList deserialization (#3520)

* Added Unit tests for current state

* Fixed Tests

* Fixed comment

* Fixed other issues

* removed a couple types that weren't needed

Core/Core/Serialisation/SerializationUtilities/ValueConverter.cs

@@ -1,6 +1,7 @@
 using System;
 using System.Collections;
 using System.Collections.Generic;
+using System.Diagnostics.Contracts;
 using System.DoubleNumerics;
 using System.Drawing;
 using System.Globalization;
@@ -157,16 +158,17 @@ public static bool ConvertValue(Type type, object? value, out object? convertedV
       #endregion
     }
 
-    // Handle List<?>
-    if (type.IsGenericType && type.GetGenericTypeDefinition() == typeof(List<>))
+    // Handle List<>, IList<>, and IReadOnlyList<>
+    if (type.IsGenericType && IsGenericList(type))
     {
       if (value is not List<object> valueList)
       {
         return false;
       }
 
+      var targetType = typeof(List<>).MakeGenericType(type.GenericTypeArguments);
       Type listElementType = type.GenericTypeArguments[0];
-      IList ret = Activator.CreateInstance(type, valueList.Count) as IList;
+      IList ret = Activator.CreateInstance(targetType, valueList.Count) as IList;
       foreach (object inputListElement in valueList)
       {
         if (!ConvertValue(listElementType, inputListElement, out object? convertedListElement))
@@ -311,4 +313,21 @@ public static bool ConvertValue(Type type, object? value, out object? convertedV
 
     return false;
   }
+
+  /// <summary>
+  /// Tests that the given <paramref name="type"/> is assignable from a generic type def <see cref="List{T}"/>
+  /// </summary>
+  /// <param name="type"></param>
+  /// <returns></returns>
+  [Pure]
+  private static bool IsGenericList(Type type)
+  {
+    if (!type.IsGenericType)
+    {
+      return false;
+    }
+
+    Type typeDef = type.GetGenericTypeDefinition();
+    return typeDef == typeof(List<>) || typeDef == typeof(IList<>) || typeDef == typeof(IReadOnlyList<>);
+  }
 }

Core/Tests/Speckle.Core.Tests.Unit/Serialisation/SerializerBreakingChanges.cs

@@ -1,5 +1,4 @@
 using NUnit.Framework;
-using Speckle.Core.Logging;
 using Speckle.Core.Serialisation;
 
 namespace Speckle.Core.Tests.Unit.Serialisation;

Core/Tests/Speckle.Core.Tests.Unit/Serialisation/SerializerNonBreakingChanges.cs

@@ -87,6 +87,42 @@ public void ListToArray(double[] testCase)
     Assert.That(res.value, Is.EquivalentTo(testCase));
   }
 
+  [Test, TestCaseSource(nameof(s_arrayTestCases))]
+  public void ListToIList(double[] testCase)
+  {
+    var from = new ListDoubleValueMock { value = testCase.ToList() };
+
+    var res = from.SerializeAsTAndDeserialize<IReadOnlyListDoubleValueMock>();
+    Assert.That(res.value, Is.EquivalentTo(testCase));
+  }
+
+  [Test, TestCaseSource(nameof(s_arrayTestCases))]
+  public void ListToIReadOnlyList(double[] testCase)
+  {
+    var from = new ListDoubleValueMock { value = testCase.ToList() };
+
+    var res = from.SerializeAsTAndDeserialize<IListDoubleValueMock>();
+    Assert.That(res.value, Is.EquivalentTo(testCase));
+  }
+
+  [Test, TestCaseSource(nameof(s_arrayTestCases))]
+  public void IListToList(double[] testCase)
+  {
+    var from = new IListDoubleValueMock { value = testCase.ToList() };
+
+    var res = from.SerializeAsTAndDeserialize<ListDoubleValueMock>();
+    Assert.That(res.value, Is.EquivalentTo(testCase));
+  }
+
+  [Test, TestCaseSource(nameof(s_arrayTestCases))]
+  public void IReadOnlyListToList(double[] testCase)
+  {
+    var from = new IReadOnlyListDoubleValueMock { value = testCase.ToList() };
+
+    var res = from.SerializeAsTAndDeserialize<ListDoubleValueMock>();
+    Assert.That(res.value, Is.EquivalentTo(testCase));
+  }
+
   [Test, TestCaseSource(nameof(MyEnums))]
   public void EnumToInt(MyEnum testCase)
   {
@@ -171,6 +207,16 @@ public class ListDoubleValueMock : SerializerMock
   public List<double> value { get; set; }
 }
 
+public class IListDoubleValueMock : SerializerMock
+{
+  public IList<double> value { get; set; }
+}
+
+public class IReadOnlyListDoubleValueMock : SerializerMock
+{
+  public IReadOnlyList<double> value { get; set; }
+}
+
 public class ArrayDoubleValueMock : SerializerMock
 {
   public double[] value { get; set; }

Objects/Tests/Objects.Tests.Unit/ModelPropertySupportedTypes.cs

@@ -0,0 +1,81 @@
+using System;
+using System.Collections.Generic;
+using System.DoubleNumerics;
+using System.Drawing;
+using System.Linq;
+using NUnit.Framework;
+using Speckle.Core.Models;
+using Speckle.Core.Serialisation;
+using Speckle.Newtonsoft.Json;
+
+namespace Objects.Tests.Unit;
+
+/// <summary>
+/// Tests that all Base object models in the kit have properties that are an allowed type
+/// This test is not exhaustive, there are plenty of generic arg combinations that will pass this test,
+/// but still not work / are not defined behaviour. This test will just catch many types that definitely won't work
+/// </summary>
+public class ModelPropertySupportedTypes
+{
+  /// <summary>
+  /// Set of types that we support in Base objects
+  /// If it's not in the list, or is commented out, it's not supported by our serializer!
+  /// </summary>
+  /// <remarks>
+  /// If you're tempted to add to this list, please ensure both our serializer and deserializer support properties of this type
+  /// Check the <see cref="Speckle.Core.Serialisation.SerializationUtilities.ValueConverter"/>
+  /// Check the <see cref="BaseObjectSerializerV2"/>
+  /// (or is an interface where all concrete types are supported)
+  /// You should also consider adding a test in SerializerNonBreakingChanges
+  /// </remarks>
+  private readonly HashSet<Type> _allowedTypes =
+    new()
+    {
+      typeof(Boolean),
+      typeof(Byte),
+      typeof(UInt32),
+      typeof(UInt64),
+      typeof(Int16),
+      typeof(Int32),
+      typeof(Int64),
+      //typeof(Half),
+      typeof(Single),
+      typeof(Double),
+      typeof(Char),
+      typeof(string),
+      typeof(DateTime),
+      typeof(Guid),
+      typeof(Color),
+      typeof(List<>),
+      typeof(Nullable<>),
+      typeof(IList<>),
+      typeof(IReadOnlyList<>),
+      typeof(Dictionary<,>),
+      //typeof(IDictionary<,>),
+      //typeof(IReadOnlyDictionary<,>),
+      typeof(ICurve),
+      typeof(Object),
+      typeof(Matrix4x4),
+    };
+
+  [Test]
+  [TestCaseSource(typeof(GenericTests), nameof(GenericTests.AvailableTypesInKit))]
+  public void TestObjects(Type t)
+  {
+    var members = DynamicBase.GetInstanceMembers(t).Where(p => !p.IsDefined(typeof(JsonIgnoreAttribute), true));
+
+    foreach (var prop in members)
+    {
+      if (prop.PropertyType.IsAssignableTo(typeof(Base)))
+        continue;
+      if (prop.PropertyType.IsEnum)
+        continue;
+      if (prop.PropertyType.IsSZArray)
+        continue;
+
+      Type propType = prop.PropertyType;
+      Type typeDef = propType.IsGenericType ? propType.GetGenericTypeDefinition() : propType;
+      Assert.That(_allowedTypes, Does.Contain(typeDef), $"{typeDef} was not in allowedTypes");
+    }
+  }
+}