Support polymorphic value-type converters and add validation on values returned by custom converters #40914

devsko · 2020-08-17T02:45:49Z

Support polymorphic converters that return value types (TypeToConvert == typeof(object) or an interface).
- Works for both Nullable and non-Nullable value types.
Validate the return value from converters so they don't return an incompatible object on deserialization.

When an object or interface typed custom converter gets or sets a property or field, an additional box/unbox needs to be emitted.
cc @layomia

Dotnet-GitSync-Bot · 2020-08-17T02:45:53Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

devsko · 2020-08-18T07:14:30Z

@layomia Is there a chance for this to be in .net5?

layomia · 2020-08-19T15:47:32Z

@devsko yes, we definitely want a fix for this issue in 5.0. Taking a look at this PR now.

...ibraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs

...ies/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.Object.cs

...stem.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.StructMember.cs

src/libraries/System.Text.Json/tests/Serialization/TestClasses/TestClasses.StructMember.cs

...ies/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.Object.cs

layomia · 2020-08-19T17:34:24Z

Re #40878 (comment): I think the extra box & unbox is okay wrt perf since this will only be done when a interface/typeof(object) returning converter is used to deserialize a value-type property. I think this converter usage pattern is pretty edge case. The change is also good to prevent a regression for converter usages that worked in 3.1 e.g. https://dotnetfiddle.net/3GmeIQ.

layomia · 2020-08-19T17:35:21Z

FYI @YohDeadfall

...ibraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs

devsko · 2020-08-20T00:45:23Z

@layomia PTAL Hope I addressed all your feedback. ~~3 of the new tests now have an ActiveIssue attribute because of #36329. Shall I have a look?~~
I fixed all issues with JsonConverter<object> and nullable value-types I am aware of and added 2 more tests where the nullable properties/fields are actually null.
Please let me know when there is more I should do.

layomia

Thanks for adding the fix for #36329. Just a few more misc comments.

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs

layomia · 2020-08-20T14:49:15Z

...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs

+        private static bool IsNullableValueType(Type type)
+        {
+            return type.IsGenericType && type.GetGenericTypeDefinition() == s_nullableOfTType;
+        }
+
+        private static bool IsNullableType(Type type)
+        {
+            return !type.IsValueType || IsNullableValueType(type);
+        }
+
+        internal static bool IsAssignableFrom(Type type, Type from)
+        {
+            // Special case for which Type.IsAssignableFrom returns false
+            //    type: Nullable<T> where T : IInterface
+            //    from: IInterface
+
+            if (IsNullableValueType(from) && type.IsInterface)
+            {
+                return type.IsAssignableFrom(from.GetGenericArguments()[0]);
+            }
+
+            return type.IsAssignableFrom(from);
+        }


These are enough type helpers for us to create a new TypeExtensions class similar to

runtime/src/libraries/System.Text.Json/src/System/Collections/Generic/StackExtensions.netstandard.cs

Line 11 in 906b41c

internal static class StackExtensions

but this will be used for all TFMs i.e can include here:

runtime/src/libraries/System.Text.Json/src/System.Text.Json.csproj

Line 21 in 906b41c

<ItemGroup>

This will help internal static bool IsAssignableFrom(Type type, Type from) above read more naturally .

I had to find a unique name for TypeExtensions.IsAssignableFrom(this Type, Type) to distinguish it from Type.IsAssignableFrom(Type). Now I call it IsConvertibleFrom - even when nothing is really converted. Better idea?

Maybe IsAssignableFromInternal

...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs

src/libraries/System.Text.Json/tests/Serialization/ExtensionDataTests.cs

...ies/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.Object.cs

...stem.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.StructMember.cs

devsko · 2020-08-20T17:54:40Z

Done.

I just recognized that there is no TargetFramework for testing the .NET Standard version, which means that there are no tests for ReflectionMemberAccessor. I created a UWP app locally for testing but I think there is no UWP test runner in CI anymore :)

layomia · 2020-08-21T03:07:30Z

@devsko - Yes, it would be good to have CI coverage for this. I verify locally by commenting out the #ifdef in

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs

Lines 498 to 502 in c35942c

    
           #if NETFRAMEWORK || NETCOREAPP 
        
                               _memberAccessorStrategy = new ReflectionEmitMemberAccessor(); 
        
           #else 
        
                               _memberAccessorStrategy = new ReflectionMemberAccessor(); 
        
           #endif

and just assigning ReflectionMemberAccessor, and then I run the tests as usual.

layomia

@devsko, actually we do have some failures in the ReflectionMemberAccessor code paths. Can you pls use the approach I describe in #40914 (comment) to get a repro and address these?

Test failures (click to view)

 ----- start Thu 08/20/2020 20:19:10.77 ===============  To repro directly: =====================================================
  pushd D:\repos\dotnet_runtime_4\artifacts\bin\System.Text.Json.Tests\net5.0-Debug\
  "D:\repos\dotnet_runtime_4\artifacts\bin\testhost\net5.0-Windows_NT-Debug-x64\dotnet.exe" exec --runtimeconfig System.Text.Json.Tests.runtimeconfig.json --depsfile System.Text.Json.Tests.deps.json xunit.console.dll System.Text.Json.Tests.dll -xml testResults.xml -nologo -notrait category=OuterLoop -notrait category=failing
  popd
  ===========================================================================================================
    Discovering: System.Text.Json.Tests (method display = ClassAndMethod, method display options = None)
    Discovered:  System.Text.Json.Tests (found 2060 of 2089 test cases)
    Starting:    System.Text.Json.Tests (parallel test collections = on, max threads = 8)
      System.Text.Json.Serialization.Tests.CustomConverterTests.NullableValueTypedMemberToObjectConverter [FAIL]
        System.ArgumentException : Object of type 'System.Text.Json.Serialization.Tests.ValueTypedMember' cannot be converted to type 'System.Text.Json.Serialization.Tests.RefTypedMember'.
        Stack Trace:
          D:\repos\dotnet_runtime_4\src\coreclr\src\System.Private.CoreLib\src\System\RuntimeType.CoreCLR.cs(3460,0): at System.RuntimeType.TryChangeType(Object value, Binder binder, CultureInfo culture, Boolean needsSpecialCast)
          D:\repos\dotnet_runtime_4\src\coreclr\src\System.Private.CoreLib\src\System\RuntimeType.CoreCLR.cs(3421,0): at System.RuntimeType.CheckValue(Object value, Binder binder, CultureInfo culture, BindingFlags invokeAttr)
          D:\repos\dotnet_runtime_4\src\coreclr\src\System.Private.CoreLib\src\System\Reflection\MethodBase.CoreCLR.cs(84,0): at System.Reflection.MethodBase.CheckArguments(Object[] parameters, Binder binder, BindingFlags invokeAttr, CultureInfo culture, Signature sig)
          D:\repos\dotnet_runtime_4\src\coreclr\src\System.Private.CoreLib\src\System\Reflection\RuntimeMethodInfo.cs(428,0): at System.Reflection.RuntimeMethodInfo.InvokeArgumentsCheck(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
          D:\repos\dotnet_runtime_4\src\coreclr\src\System.Private.CoreLib\src\System\Reflection\RuntimeMethodInfo.cs(386,0): at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
          D:\repos\dotnet_runtime_4\src\libraries\System.Private.CoreLib\src\System\Reflection\MethodBase.cs(49,0): at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\ReflectionMemberAccessor.cs(156,0): at System.Text.Json.Serialization.ReflectionMemberAccessor.<>c__DisplayClass7_0`1.<CreatePropertySetter>b__0(Object obj, TProperty value)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonPropertyInfoOfT.cs(233,0): at System.Text.Json.JsonPropertyInfo`1.ReadJsonAndSetMember(Object obj, ReadStack& state, Utf8JsonReader& reader)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\Converters\Object\ObjectDefaultConverter.cs(58,0): at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonConverterOfT.cs(267,0): at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonConverterOfT.ReadCore.cs(62,0): at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.Helpers.cs(29,0): at System.Text.Json.JsonSerializer.ReadCore[TValue](JsonConverter jsonConverter, Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.Helpers.cs(21,0): at System.Text.Json.JsonSerializer.ReadCore[TValue](Utf8JsonReader& reader, Type returnType, JsonSerializerOptions options)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.String.cs(113,0): at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, Type returnType, JsonSerializerOptions options)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.String.cs(44,0): at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, JsonSerializerOptions options)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\tests\Serialization\CustomConverterTests\CustomConverterTests.ValueTypedMember.cs(191,0): at System.Text.Json.Serialization.Tests.CustomConverterTests.NullableValueTypedMemberToObjectConverter()
      System.Text.Json.Serialization.Tests.CustomConverterTests.NullableValueTypedMemberToInterfaceConverter [FAIL]
        System.ArgumentException : Object of type 'System.Text.Json.Serialization.Tests.ValueTypedMember' cannot be converted to type 'System.Text.Json.Serialization.Tests.RefTypedMember'.
        Stack Trace:
          D:\repos\dotnet_runtime_4\src\coreclr\src\System.Private.CoreLib\src\System\RuntimeType.CoreCLR.cs(3460,0): at System.RuntimeType.TryChangeType(Object value, Binder binder, CultureInfo culture, Boolean needsSpecialCast)
          D:\repos\dotnet_runtime_4\src\coreclr\src\System.Private.CoreLib\src\System\RuntimeType.CoreCLR.cs(3421,0): at System.RuntimeType.CheckValue(Object value, Binder binder, CultureInfo culture, BindingFlags invokeAttr)
          D:\repos\dotnet_runtime_4\src\coreclr\src\System.Private.CoreLib\src\System\Reflection\MethodBase.CoreCLR.cs(84,0): at System.Reflection.MethodBase.CheckArguments(Object[] parameters, Binder binder, BindingFlags invokeAttr, CultureInfo culture, Signature sig)
          D:\repos\dotnet_runtime_4\src\coreclr\src\System.Private.CoreLib\src\System\Reflection\RuntimeMethodInfo.cs(428,0): at System.Reflection.RuntimeMethodInfo.InvokeArgumentsCheck(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
          D:\repos\dotnet_runtime_4\src\coreclr\src\System.Private.CoreLib\src\System\Reflection\RuntimeMethodInfo.cs(386,0): at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
          D:\repos\dotnet_runtime_4\src\libraries\System.Private.CoreLib\src\System\Reflection\MethodBase.cs(49,0): at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\ReflectionMemberAccessor.cs(156,0): at System.Text.Json.Serialization.ReflectionMemberAccessor.<>c__DisplayClass7_0`1.<CreatePropertySetter>b__0(Object obj, TProperty value)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonPropertyInfoOfT.cs(233,0): at System.Text.Json.JsonPropertyInfo`1.ReadJsonAndSetMember(Object obj, ReadStack& state, Utf8JsonReader& reader)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\Converters\Object\ObjectDefaultConverter.cs(58,0): at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonConverterOfT.cs(267,0): at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonConverterOfT.ReadCore.cs(62,0): at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.Helpers.cs(29,0): at System.Text.Json.JsonSerializer.ReadCore[TValue](JsonConverter jsonConverter, Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.Helpers.cs(21,0): at System.Text.Json.JsonSerializer.ReadCore[TValue](Utf8JsonReader& reader, Type returnType, JsonSerializerOptions options)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.String.cs(113,0): at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, Type returnType, JsonSerializerOptions options)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.String.cs(44,0): at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, JsonSerializerOptions options)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\tests\Serialization\CustomConverterTests\CustomConverterTests.ValueTypedMember.cs(159,0): at System.Text.Json.Serialization.Tests.CustomConverterTests.NullableValueTypedMemberToInterfaceConverter()
      System.Text.Json.Serialization.Tests.CustomConverterTests.ValueTypedMemberToObjectConverter [FAIL]
        System.ArgumentException : Object of type 'System.Text.Json.Serialization.Tests.ValueTypedMember' cannot be converted to type 'System.Text.Json.Serialization.Tests.RefTypedMember'.
        Stack Trace:
          D:\repos\dotnet_runtime_4\src\coreclr\src\System.Private.CoreLib\src\System\RuntimeType.CoreCLR.cs(3460,0): at System.RuntimeType.TryChangeType(Object value, Binder binder, CultureInfo culture, Boolean needsSpecialCast)
          D:\repos\dotnet_runtime_4\src\coreclr\src\System.Private.CoreLib\src\System\RuntimeType.CoreCLR.cs(3421,0): at System.RuntimeType.CheckValue(Object value, Binder binder, CultureInfo culture, BindingFlags invokeAttr)
          D:\repos\dotnet_runtime_4\src\coreclr\src\System.Private.CoreLib\src\System\Reflection\MethodBase.CoreCLR.cs(84,0): at System.Reflection.MethodBase.CheckArguments(Object[] parameters, Binder binder, BindingFlags invokeAttr, CultureInfo culture, Signature sig)
          D:\repos\dotnet_runtime_4\src\coreclr\src\System.Private.CoreLib\src\System\Reflection\RuntimeMethodInfo.cs(428,0): at System.Reflection.RuntimeMethodInfo.InvokeArgumentsCheck(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
          D:\repos\dotnet_runtime_4\src\coreclr\src\System.Private.CoreLib\src\System\Reflection\RuntimeMethodInfo.cs(386,0): at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
          D:\repos\dotnet_runtime_4\src\libraries\System.Private.CoreLib\src\System\Reflection\MethodBase.cs(49,0): at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\ReflectionMemberAccessor.cs(156,0): at System.Text.Json.Serialization.ReflectionMemberAccessor.<>c__DisplayClass7_0`1.<CreatePropertySetter>b__0(Object obj, TProperty value)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonPropertyInfoOfT.cs(233,0): at System.Text.Json.JsonPropertyInfo`1.ReadJsonAndSetMember(Object obj, ReadStack& state, Utf8JsonReader& reader)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\Converters\Object\ObjectDefaultConverter.cs(58,0): at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonConverterOfT.cs(267,0): at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonConverterOfT.ReadCore.cs(62,0): at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.Helpers.cs(29,0): at System.Text.Json.JsonSerializer.ReadCore[TValue](JsonConverter jsonConverter, Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.Helpers.cs(21,0): at System.Text.Json.JsonSerializer.ReadCore[TValue](Utf8JsonReader& reader, Type returnType, JsonSerializerOptions options)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.String.cs(113,0): at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, Type returnType, JsonSerializerOptions options)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.String.cs(44,0): at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, JsonSerializerOptions options)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\tests\Serialization\CustomConverterTests\CustomConverterTests.ValueTypedMember.cs(127,0): at System.Text.Json.Serialization.Tests.CustomConverterTests.ValueTypedMemberToObjectConverter()
      System.Text.Json.Serialization.Tests.CustomConverterTests.ValueTypedMemberToInterfaceConverter [FAIL]
        System.ArgumentException : Object of type 'System.Text.Json.Serialization.Tests.ValueTypedMember' cannot be converted to type 'System.Text.Json.Serialization.Tests.RefTypedMember'.
        Stack Trace:
          D:\repos\dotnet_runtime_4\src\coreclr\src\System.Private.CoreLib\src\System\RuntimeType.CoreCLR.cs(3460,0): at System.RuntimeType.TryChangeType(Object value, Binder binder, CultureInfo culture, Boolean needsSpecialCast)
          D:\repos\dotnet_runtime_4\src\coreclr\src\System.Private.CoreLib\src\System\RuntimeType.CoreCLR.cs(3421,0): at System.RuntimeType.CheckValue(Object value, Binder binder, CultureInfo culture, BindingFlags invokeAttr)
          D:\repos\dotnet_runtime_4\src\coreclr\src\System.Private.CoreLib\src\System\Reflection\MethodBase.CoreCLR.cs(84,0): at System.Reflection.MethodBase.CheckArguments(Object[] parameters, Binder binder, BindingFlags invokeAttr, CultureInfo culture, Signature sig)
          D:\repos\dotnet_runtime_4\src\coreclr\src\System.Private.CoreLib\src\System\Reflection\RuntimeMethodInfo.cs(428,0): at System.Reflection.RuntimeMethodInfo.InvokeArgumentsCheck(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
          D:\repos\dotnet_runtime_4\src\coreclr\src\System.Private.CoreLib\src\System\Reflection\RuntimeMethodInfo.cs(386,0): at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
          D:\repos\dotnet_runtime_4\src\libraries\System.Private.CoreLib\src\System\Reflection\MethodBase.cs(49,0): at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\ReflectionMemberAccessor.cs(156,0): at System.Text.Json.Serialization.ReflectionMemberAccessor.<>c__DisplayClass7_0`1.<CreatePropertySetter>b__0(Object obj, TProperty value)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonPropertyInfoOfT.cs(233,0): at System.Text.Json.JsonPropertyInfo`1.ReadJsonAndSetMember(Object obj, ReadStack& state, Utf8JsonReader& reader)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\Converters\Object\ObjectDefaultConverter.cs(58,0): at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonConverterOfT.cs(267,0): at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonConverterOfT.ReadCore.cs(62,0): at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.Helpers.cs(29,0): at System.Text.Json.JsonSerializer.ReadCore[TValue](JsonConverter jsonConverter, Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.Helpers.cs(21,0): at System.Text.Json.JsonSerializer.ReadCore[TValue](Utf8JsonReader& reader, Type returnType, JsonSerializerOptions options)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.String.cs(113,0): at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, Type returnType, JsonSerializerOptions options)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.String.cs(44,0): at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, JsonSerializerOptions options)
          D:\repos\dotnet_runtime_4\src\libraries\System.Text.Json\tests\Serialization\CustomConverterTests\CustomConverterTests.ValueTypedMember.cs(95,0): at System.Text.Json.Serialization.Tests.CustomConverterTests.ValueTypedMemberToInterfaceConverter()
    Finished:    System.Text.Json.Tests
  === TEST EXECUTION SUMMARY ===
     System.Text.Json.Tests  Total: 8683, Errors: 0, Failed: 4, Skipped: 0, Time: 27.864s

layomia · 2020-08-24T17:47:59Z

@devsko @YohDeadfall thank you for your work on these changes.

Due to the complexity and implementation considerations of the fix for #41146, and also because of the exceptional nature of the scenario, I agree with the initial thought to address it in a separate PR. This will help reduce the risk with back-porting this PR (#40914) to .NET 5.

This PR should be simplified to only contain the fixes and tests for #40878 and #36329. We'd appreciate a follow up PR bringing in the changes to fix #41146, which can also be considered for back-porting to .NET 5.0. Any tests in this PR written as part of the fix for #41146 can be retained and annotated with ActiveIssue.

devsko · 2020-08-25T09:40:36Z

Okay. Will do

devsko · 2020-08-25T11:34:58Z

@layomia I've undone all changes for #41146 and for consolidating the exception type thrown when an assignment fails. As suggested the tests for #41146 remained in this PR but are annotated with ActiveIssue.
All tests succeed locally for both member accessors.

steveharter · 2020-08-25T15:05:48Z

...ystem.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.InvalidCast.cs

+
+            var json = JsonSerializer.Serialize(obj);
+
+            var ex = Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<ObjectWrapperWithProperty>(json));


In 3.1 we throw InvalidCastException; that seems more appropriate than JsonException which typically means the JSON is invalid for the given object model, not that a converter did the wrong thing.

Same applies to other tests here.

These tests are annotated with ActiveIssue in this PR. The unique exception type (JsonException or another new exception type) was introduced because
@layomia:

I think we should create and throw a new exception type explaining that the value returned by the converter is invalid for the property type.

steveharter · 2020-08-25T15:30:18Z

...ies/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.Object.cs

+        [Fact]
+        public static void ClassWithPrimitivesObjectConverter()
+        {
+            const string expected =


Since reflection order is non-deterministic, we can't assume ordering of properties. Each property must be searched for independently or perhaps just a length check on the JSON.

steveharter · 2020-08-25T15:38:04Z

src/libraries/System.Text.Json/src/System/Text/Json/TypeExtensions.cs

+        /// </summary>
+        public static bool IsNullableValueType(this Type type)
+        {
+            return type.IsGenericType && type.GetGenericTypeDefinition() == s_nullableOfTType;


I believe this can just be:

return Nullable.GetUnderlyingType(type) != null;

This is what we do in

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/NullableConverterFactory.cs

Lines 11 to 14 in 6072e4d

public override bool CanConvert(Type typeToConvert)

{

return Nullable.GetUnderlyingType(typeToConvert) != null;

}

steveharter · 2020-08-25T16:01:29Z

...ibraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs

-                generator.Emit(OpCodes.Castclass, declaringType);
+                LocalBuilder value = generator.DeclareLocal(declaredPropertyType);
+
+                // try


I don't think adding a try\catch is the right thing to do. Letting the raw exception (InvalidCast; perhaps NullReference) propagate due to an invalid converter is acceptable IMO.

A try\catch is also not completely free from a perf standpoint (even when no exception is thrown) and doing that on every setter\getter would likely impact perf.

@steveharter I think you are reviewing an outdated version. All that stuff is in a separate PR #41323 now

steveharter

Thanks for tackling this issue.

I think this is very close. Two requested changes:

Remove the ActiveIssue tests and assume InvalidCastException (or NullReference)
Add the code below to ReadJsonAndSetMember(). This will verify the runtime type returned from the converter is compatible with the declared property type.

Change this

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs

Lines 228 to 235 in dc6ad2a

    
           if (!isNullToken || !IgnoreDefaultValuesOnRead || !Converter.CanBeNull) 
        
           { 
        
               success = Converter.TryRead(ref reader, RuntimePropertyType!, Options, ref state, out T value); 
        
               if (success) 
        
               { 
        
                   Set!(obj, value!); 
        
               } 
        
           }

to this (note we'll need a resource string and ThrowHelper not shown here)

                if (!isNullToken || !IgnoreDefaultValuesOnRead || !Converter.CanBeNull)
                {
                    success = Converter.TryRead(ref reader, RuntimePropertyType!, Options, ref state, out T value);
                    if (success)
                    {
                        if (!Converter.IsInternalConverter && value != null && !DeclaredPropertyType.IsAssignableFrom(value.GetType()))
                        {
                            // todo: add exception resource string and use ThrowHelper
                            throw new InvalidCastException($"Unable to cast object of type '{value.GetType()}' to type '{DeclaredPropertyType}'.");
                        }

                        Set!(obj, value!);
                    }
                }

We could throw InvalidOperation instead here, but the cast exception is consistent with 3.1.

devsko · 2020-08-25T17:30:51Z

@steveharter Made the changes. I added an additional test for null assignment to a non-nullable value type. I think this is important because ReflectionEmitAccessor will throw a NRE and ReflectionAccessor silently assigns default.

steveharter

The prior commit helped a lot.

Some additional changes requested:

Change NullReferenceException to InvalidOperationException (I previously thought these were coming from IL Emit, but now I see that we are directly throwing NRE which we should never do).
A converter should not have to have a nullable check; two of the tests in their CanConvert() have a line like this:

typeToConvert = Nullable.GetUnderlyingType(typeToConvert) ?? typeToConvert;

that should not be needed. In order to make it possible to remove that code, add an additional if check.

Change
https://github.com/devsko/runtime/blob/box-member-access/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/NullableConverterFactory.cs#L16-L26

to:

        public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializerOptions options)
        {
            Debug.Assert(typeToConvert.GetGenericArguments().Length > 0);

            Type valueTypeToConvert = typeToConvert.GetGenericArguments()[0];

            JsonConverter valueConverter = options.GetConverter(valueTypeToConvert);
            Debug.Assert(valueConverter != null);

            // If the value type has an interface or object converter, just return that converter directly.
            if (!valueConverter.TypeToConvert.IsValueType && valueTypeToConvert.IsValueType)
            {
                return valueConverter;
            }

            return CreateValueConverter(valueTypeToConvert, valueConverter);
        }

Also I manually verified the tests pass when using ReflectionMemberAccessor instead of ReflectionEmitMemberAccessor as would be the case for non-inbox scenarios.

steveharter · 2020-08-25T19:41:27Z

src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs

+        [DoesNotReturn]
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        public static void ThrowNullReferenceException_DeserializeUnableToAssignNull(Type declaredType)
+        {
+            throw new NullReferenceException(SR.Format(SR.DeserializeUnableToAssignNull, declaredType));
+        }


Suggested change

[DoesNotReturn]

[MethodImpl(MethodImplOptions.NoInlining)]

public static void ThrowNullReferenceException_DeserializeUnableToAssignNull(Type declaredType)

{

throw new NullReferenceException(SR.Format(SR.DeserializeUnableToAssignNull, declaredType));

}

[DoesNotReturn]

[MethodImpl(MethodImplOptions.NoInlining)]

public static void ThrowInvalidOperationException_DeserializeUnableToAssignNull(Type declaredType)

{

throw new InvalidOperationException(SR.Format(SR.DeserializeUnableToAssignNull, declaredType));

}

steveharter · 2020-08-25T19:43:59Z

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs

+                            }
+                            else if (DeclaredPropertyType.IsValueType && !DeclaredPropertyType.IsNullableValueType())
+                            {
+                                ThrowHelper.ThrowNullReferenceException_DeserializeUnableToAssignNull(DeclaredPropertyType);


Suggested change

ThrowHelper.ThrowNullReferenceException_DeserializeUnableToAssignNull(DeclaredPropertyType);

ThrowHelper.ThrowInvalidOperationException_DeserializeUnableToAssignNull(DeclaredPropertyType);

steveharter · 2020-08-25T19:46:05Z

....Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.ValueTypedMember.cs

+            ex = Assert.Throws<NullReferenceException>(() => JsonSerializer.Deserialize<TestClassWithValueTypedMember>(@"{""MyValueTypedProperty"":null}", options));
+            ex = Assert.Throws<NullReferenceException>(() => JsonSerializer.Deserialize<TestClassWithValueTypedMember>(@"{""MyValueTypedField"":null}", options));


Suggested change

ex = Assert.Throws<NullReferenceException>(() => JsonSerializer.Deserialize<TestClassWithValueTypedMember>(@"{""MyValueTypedProperty"":null}", options));

ex = Assert.Throws<NullReferenceException>(() => JsonSerializer.Deserialize<TestClassWithValueTypedMember>(@"{""MyValueTypedField"":null}", options));

ex = Assert.Throws<InvalidOperationException>(() => JsonSerializer.Deserialize<TestClassWithValueTypedMember>(@"{""MyValueTypedProperty"":null}", options));

ex = Assert.Throws<InvalidOperationException>(() => JsonSerializer.Deserialize<TestClassWithValueTypedMember>(@"{""MyValueTypedField"":null}", options));

steveharter · 2020-08-25T19:46:39Z

....Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.ValueTypedMember.cs

+            ex = Assert.Throws<NullReferenceException>(() => JsonSerializer.Deserialize<TestClassWithValueTypedMember>(@"{""MyValueTypedProperty"":null}", options));
+            ex = Assert.Throws<NullReferenceException>(() => JsonSerializer.Deserialize<TestClassWithValueTypedMember>(@"{""MyValueTypedField"":null}", options));


Suggested change

ex = Assert.Throws<NullReferenceException>(() => JsonSerializer.Deserialize<TestClassWithValueTypedMember>(@"{""MyValueTypedProperty"":null}", options));

ex = Assert.Throws<NullReferenceException>(() => JsonSerializer.Deserialize<TestClassWithValueTypedMember>(@"{""MyValueTypedField"":null}", options));

ex = Assert.Throws<InvalidOperationException>(() => JsonSerializer.Deserialize<TestClassWithValueTypedMember>(@"{""MyValueTypedProperty"":null}", options));

ex = Assert.Throws<InvalidOperationException>(() => JsonSerializer.Deserialize<TestClassWithValueTypedMember>(@"{""MyValueTypedField"":null}", options));

steveharter · 2020-08-25T19:48:41Z

....Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.ValueTypedMember.cs

+
+            public override bool CanConvert(Type typeToConvert)
+            {
+                typeToConvert = Nullable.GetUnderlyingType(typeToConvert) ?? typeToConvert;


Suggested change

typeToConvert = Nullable.GetUnderlyingType(typeToConvert) ?? typeToConvert;

This should not be needed with the other change mentioned in the review.

steveharter · 2020-08-25T19:49:04Z

....Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.ValueTypedMember.cs

+
+            public override bool CanConvert(Type typeToConvert)
+            {
+                typeToConvert = Nullable.GetUnderlyingType(typeToConvert) ?? typeToConvert;


Suggested change

typeToConvert = Nullable.GetUnderlyingType(typeToConvert) ?? typeToConvert;

This should not be needed with the other change mentioned in the review.

steveharter · 2020-08-25T19:49:31Z

src/libraries/System.Text.Json/src/System/Text/Json/TypeExtensions.cs

+
+            return type.IsAssignableFrom(from);
+        }
+


nit: extra line

steveharter · 2020-08-25T23:02:15Z

Test failures appear unrelated:

System.Net.Http.Functional.Tests.WorkItemExecution
System.Net.Http.Functional.Tests.SocketsHttpHandlerTest_Http2.Http2_MultipleConnectionsEnabled_IdleConnectionTimeoutExpired_ConnectionRemovedAndNewCreated
System.Net.Http.Functional.Tests.SocketsHttpHandlerTest_Http2.Http2_MultipleConnectionsEnabled_ConnectionLimitNotReached_ConcurrentRequestsSuccessfullyHandled

steveharter

Thanks @devsko for addressing these important scenarios.

Thanks @YohDeadfall for your reviews.

layomia · 2020-08-25T23:40:34Z

/backport to release/5.0

github-actions · 2020-08-25T23:41:33Z

Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/224488487

github-actions · 2020-08-25T23:43:44Z

@layomia backporting to release/5.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: IL emit Box/Unbox in member access
Applying: Test case for object converter
Applying: Test case for class with primitive members and object converter
Applying: Address feedback
Applying: Fixed issues with JsonConverter<object>and nullable value-types
Applying: Address feedback
Applying: Fix test for .NETStandard version
Applying: Fix #41146
Applying: Accidentally commited the local test hack
Applying: Throw JsonException when a deserialized value cannot be assigned to the property/field
error: sha1 information is lacking or useless (src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0010 Throw JsonException when a deserialized value cannot be assigned to the property/field
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

…s returned by custom converters (dotnet#40914) * IL emit Box/Unbox in member access * Test case for object converter * Test case for class with primitive members and object converter * Address feedback * Fixed issues with JsonConverter<object>and nullable value-types * Address feedback * Fix test for .NETStandard version * Fix dotnet#41146 * Accidentally commited the local test hack * Throw JsonException when a deserialized value cannot be assigned to the property/field * Fix merge * Fix merge again * Undo Fix dotnet#41146 * Consolidate nullable annotations * Addressed feedback * Addressed feedback

* Fixup JSON equality checks in polymorphic converter tests * Limit deserialization value validation to custom _polymorphic_ converters

…ation on values returned by custom converters (#40914) (#41366) * Support polymorphic value-type converters and add validation on values returned by custom converters (#40914) * IL emit Box/Unbox in member access * Test case for object converter * Test case for class with primitive members and object converter * Address feedback * Fixed issues with JsonConverter<object>and nullable value-types * Address feedback * Fix test for .NETStandard version * Fix #41146 * Accidentally commited the local test hack * Throw JsonException when a deserialized value cannot be assigned to the property/field * Fix merge * Fix merge again * Undo Fix #41146 * Consolidate nullable annotations * Addressed feedback * Addressed feedback * Fixup JSON equality checks in polymorphic converter tests * Limit deserialization value validation to custom _polymorphic_ converters Co-authored-by: devsko <[email protected]>

devsko requested review from jozkee, layomia and steveharter as code owners August 17, 2020 02:45

IL emit Box/Unbox in member access

6d70fff

devsko force-pushed the box-member-access branch from a5bfd3b to 6d70fff Compare August 18, 2020 06:58

devsko added 2 commits August 18, 2020 11:58

Test case for object converter

e77e7cc

Test case for class with primitive members and object converter

c35942c

jkotas added the area-System.Text.Json label Aug 18, 2020

devsko mentioned this pull request Aug 19, 2020

System.Text.Json Access Violation Exception (Attempted to read or write protected memory) #40878

Closed

Turnerj mentioned this pull request Aug 19, 2020

Switch to System.Text.Json RehanSaeed/Schema.NET#100

Merged

layomia reviewed Aug 19, 2020

View reviewed changes

layomia added this to the 5.0.0 milestone Aug 19, 2020

layomia assigned devsko Aug 19, 2020

YohDeadfall reviewed Aug 19, 2020

View reviewed changes

Address feedback

9cd5f5e

Fixed issues with JsonConverter<object>and nullable value-types

906b41c

devsko force-pushed the box-member-access branch from c532aac to 906b41c Compare August 20, 2020 10:16

devsko mentioned this pull request Aug 20, 2020

A converter that inherits JsonConverter<object> doesn't work on properties of nullable value type #36329

Closed

layomia reviewed Aug 20, 2020

View reviewed changes

Address feedback

1b795cf

layomia suggested changes Aug 21, 2020

View reviewed changes

Fix test for .NETStandard version

f635c21

devsko added 2 commits August 25, 2020 12:56

Undo Fix dotnet#41146

36f7ec6

Consolidate nullable annotations

baa58c7

devsko mentioned this pull request Aug 25, 2020

Type check the actual values during deserialization in polymorphic scenarios #41323

Closed

steveharter reviewed Aug 25, 2020

View reviewed changes

steveharter suggested changes Aug 25, 2020

View reviewed changes

Addressed feedback

9151c96

steveharter suggested changes Aug 25, 2020

View reviewed changes

steveharter changed the title ~~Emit box/unbox IL in generated member accessor when needed~~ Support polymorphic value-type converters and add validation on values returned by custom converters Aug 25, 2020

Addressed feedback

a8b2046

steveharter approved these changes Aug 25, 2020

View reviewed changes

layomia approved these changes Aug 25, 2020

View reviewed changes

layomia merged commit c08be25 into dotnet:master Aug 25, 2020

layomia mentioned this pull request Aug 26, 2020

[release/5.0] Support polymorphic value-type converters and add validation on values returned by custom converters (#40914) #41366

Merged

devsko deleted the box-member-access branch August 26, 2020 06:09

layomia mentioned this pull request Aug 27, 2020

Clean up changes in #40914 #41427

Merged

layomia added a commit that referenced this pull request Aug 27, 2020

Clean up changes in #40914 (#41427)

fb753ae

* Fixup JSON equality checks in polymorphic converter tests * Limit deserialization value validation to custom _polymorphic_ converters

ghost locked as resolved and limited conversation to collaborators Dec 7, 2020


		var json = JsonSerializer.Serialize(obj);

		var ex = Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<ObjectWrapperWithProperty>(json));

	public override bool CanConvert(Type typeToConvert)
	{
	return Nullable.GetUnderlyingType(typeToConvert) != null;
	}

	if (!isNullToken \|\| !IgnoreDefaultValuesOnRead \|\| !Converter.CanBeNull)
	{
	success = Converter.TryRead(ref reader, RuntimePropertyType!, Options, ref state, out T value);
	if (success)
	{
	Set!(obj, value!);
	}
	}

	ThrowHelper.ThrowNullReferenceException_DeserializeUnableToAssignNull(DeclaredPropertyType);
	ThrowHelper.ThrowInvalidOperationException_DeserializeUnableToAssignNull(DeclaredPropertyType);

		ex = Assert.Throws<NullReferenceException>(() => JsonSerializer.Deserialize<TestClassWithValueTypedMember>(@"{""MyValueTypedProperty"":null}", options));
		ex = Assert.Throws<NullReferenceException>(() => JsonSerializer.Deserialize<TestClassWithValueTypedMember>(@"{""MyValueTypedField"":null}", options));

Support polymorphic value-type converters and add validation on values returned by custom converters #40914

Support polymorphic value-type converters and add validation on values returned by custom converters #40914

Uh oh!

Conversation

devsko commented Aug 17, 2020 • edited by steveharter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dotnet-GitSync-Bot commented Aug 17, 2020

Uh oh!

devsko commented Aug 18, 2020

Uh oh!

layomia commented Aug 19, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

layomia commented Aug 19, 2020

Uh oh!

layomia commented Aug 19, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

devsko commented Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

layomia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

devsko commented Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

layomia commented Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

layomia left a comment

Choose a reason for hiding this comment

Uh oh!

layomia commented Aug 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devsko commented Aug 25, 2020

Uh oh!

devsko commented Aug 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveharter Aug 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devsko commented Aug 17, 2020 •

edited by steveharter

Loading

devsko commented Aug 20, 2020 •

edited

Loading

devsko commented Aug 20, 2020 •

edited

Loading

layomia commented Aug 21, 2020 •

edited

Loading

layomia commented Aug 24, 2020 •

edited

Loading

steveharter Aug 25, 2020 •

edited

Loading

steveharter left a comment •

edited

Loading