Initial Json serialization functionality #35609

steveharter · 2019-02-27T00:22:53Z

This adds basic Json serialization functionality per https://github.com/dotnet/corefx/issues/34372

Notes:

Only the APIs which have been reviewed are present which includes the main entry points in JsonSerializer and the corresponding options class JsonSerializerOptions. This does not include the extensibility model that that existed in the prototype.
The extensibility model is currently under design and review.
Api documentation will be added soon to the reviewed APIs.

cc @joshfree @terrajobst @davidfowl

src/System.Text.Json/src/JsonReflectionEmitMaterializer.cs

stephentoub · 2019-02-27T14:04:52Z

src/System.Text.Json/src/System/Text/Json/Serialization/ArrayBufferWriter.cs

+
+namespace System.Text.Json.Serialization
+{
+    // Note: this is currently an internal class that will be replaced with a shared version.


My understanding from the API review is that the shared version will actually be something like a ReadOnlySequenceWriter. Will that work for the serializer's needs? If so, can it be adapted to that design now?

@ahsonkhan what is the status of having a public implementation of IBufferWriter?

src/System.Text.Json/src/System/Text/Json/Serialization/ArrayBufferWriter.cs

src/System.Text.Json/src/System/Text/Json/Serialization/Converters/DefaultArrayConverter.cs

src/System.Text.Json/src/System/Text/Json/Serialization/Converters/DefaultConverters.cs

src/System.Text.Json/src/System/Text/Json/Serialization/Converters/DefaultEnumConverter.cs

src/System.Text.Json/src/System/Text/Json/Serialization/Converters/JsonPropertyInfoChar.cs

...stem.Text.Json/src/System/Text/Json/Serialization/Converters/JsonPropertyInfoCharNullable.cs

src/System.Text.Json/src/System/Text/Json/Serialization/Converters/JsonPropertyInfoDateTime.cs

src/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.AddProperty.cs

src/System.Text.Json/src/System/Text/Json/Serialization/Converters/JsonValueConverterByte.cs

src/System.Text.Json/src/System/Text/Json/Serialization/Converters/JsonValueConverterChar.cs

...ext.Json/src/System/Text/Json/Serialization/Converters/JsonValueConverterDateTimeNullable.cs

src/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfo.cs

src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleArray.cs

src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Stream.cs

src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.cs

src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Helpers.cs

TylerBrinkley · 2019-03-01T21:19:14Z

src/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfTClassTProperty.cs

+                    Type propertyType = PropertyType;
+                    if (propertyType.IsGenericType && propertyType.GetGenericTypeDefinition() == typeof(Nullable<>))
+                    {
+                        propertyType = Nullable.GetUnderlyingType(propertyType);


Perhaps cache the non-nullable property type so it's not computed on each property being read?

steveharter · 2019-03-02T03:06:06Z

I've addressed the active concerns and will commit this soon pending no new actionable feedback. Thanks.

ahsonkhan

Otherwise, LGTM.

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

src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Span.cs

ahsonkhan · 2019-03-02T05:09:07Z

src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Stream.cs

+                        // We have less than half the buffer available, double the buffer size.
+                        bufferSize = (bufferSize < HalfMaxValue) ? bufferSize * 2 : int.MaxValue;
+
+                        byte[] dest = ArrayPool<byte>.Shared.Rent(bufferSize);


Renting int.MaxValue would throw OOM. Should we cap it to something smaller (either 2GB, or maybe int.MaxValue - 56)?

Do we have a policy on this elsewhere? It seems like a common scenario.

src/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.AddProperty.cs

src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Helpers.cs

ahsonkhan · 2019-03-02T05:31:51Z

src/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs

+            Assert.Null(objCopy.GetMyString());
+        }
+
+        // Todo: add tests with missing object property and missing collection property.


Can you file an issue to improve test coverage here. Also to track your earlier comment:

Branch coverage is 80%-100% depending on file, but somewhat hampered because some tests are disabled because some of the APIs are not yet public (they were made internal). I do need more negative tests.

src/System.Text.Json/tests/Serialization/SpanTests.cs

src/System.Text.Json/tests/Serialization/Stream.WriteTests.cs

src/System.Text.Json/tests/System.Text.Json.Tests.csproj

davidfowl · 2019-03-02T19:27:57Z

W00p!

davidfowl · 2019-03-03T00:41:11Z

src/System.Text.Json/ref/System.Text.Json.cs

@@ -281,3 +287,30 @@ public partial struct JsonWriterState
        public void WriteStringValue(string value, bool escape = true) { }
    }
 }
+namespace System.Text.Json.Serialization
+{
+    public static partial class JsonSerializer


Oh wait this PR removed the PipeReader and PipeWriter overloads 😢

TylerBrinkley · 2019-03-04T21:20:05Z

FYI, I've removed the need for Nullable converters and have added support for generic enum converters in #35754.

Commit migrated from dotnet/corefx@20657b5

steveharter added * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) area-System.Text.Json labels Feb 27, 2019

steveharter self-assigned this Feb 27, 2019

steveharter force-pushed the JsonSerializer branch from cd2031f to ebd9c75 Compare February 27, 2019 00:41

steveharter requested review from ahsonkhan and bartonjs February 27, 2019 00:45

steveharter force-pushed the JsonSerializer branch from 9d38598 to e06427e Compare February 27, 2019 00:56