System.Text.Json support #1685

oskardudycz · 2020-12-27T20:19:16Z

Added

System.Text.Json support,
test matrix configuration for different serializer,
updated ISerializer public API to allow passing Stream and DBDataReader

jeremydmiller · 2021-01-23T22:38:01Z

src/Marten.Testing/Documents/Target.cs

        public string StringField;

        public double Double { get; set; }
        public decimal Decimal { get; set; }
        public DateTime Date { get; set; }
        public DateTimeOffset DateOffset { get; set; }

-        public float Float;
+        public float Float { get; set; }


There's purposely a mix of properties and public fields to ensure that Marten works with both. If we do this, we just need to make sure there are tests specifically for Newtonsoft against public fields. So, ability to use a field as an identity and Linq querying.

Annoying, I know.

System.Text.Json requires explicit attribute on fields. I reverted the change and added attribute with a comment informing that's only needed for it.

jeremydmiller · 2021-01-23T22:38:50Z

src/Marten.Storyteller/Fixtures/EventStore/InlineAggregationFixture.cs

@@ -74,7 +75,7 @@ public IGrammar QuestPartyShouldBe()
        }

        [FormatAs("Members should be {Members}")]
-        public string[] Members()
+        public List<string> Members()


Why did you have to do this? This wouldn't be serialized anywhere.

This was done to align other changes, but it's reverted.

jeremydmiller · 2021-01-23T22:42:35Z

src/Marten.Testing/Events/Projections/QuestMonsters.cs

@@ -13,150 +13,88 @@ public class Root
    public interface IMonstersView
    {
        Guid Id { get; }
-        string[] Monsters { get; }
+        List<string> Monsters { get; }


STJ doesn't support arrays?

It wasn't supporting it properly when behind the Array was another type. It's fixed in the System.Text.Json 5, so I reverted the change here with the dependency added.

jeremydmiller · 2021-01-23T22:44:09Z

src/Marten.Testing/performance_tuning.cs

        }

-        public object FromJson(Type type, TextReader reader)
+        public Task<T> FromJsonAsync<T>(Stream stream, CancellationToken cancellationToken = default)


We should change the signature of this method to return a ValueTask

Used ValueTask in 1ef5e80.

jeremydmiller · 2021-01-23T22:44:40Z

src/Marten.Testing/performance_tuning.cs

+
+        public Task<object> FromJsonAsync(Type type, Stream stream, CancellationToken cancellationToken = default)
+        {
+            return Task.FromResult(FromJson(type, stream));


We should probably be using a ValueTask

Used ValueTask in 1ef5e80.

jeremydmiller · 2021-01-23T22:49:13Z

src/Marten/Linq/Selectors/SerializationSelector.cs

            return _serializer.FromJson<T>(json);
        }

-        public Task<T> ResolveAsync(DbDataReader reader, CancellationToken token)
+        public async Task<T> ResolveAsync(DbDataReader reader, CancellationToken token)


And let's look to see about using ValueTask. Might also want the actual reading from the Stream to be async. We should look at how the ASP.net JsonFormatter uses STJ and see if it buffers data from the Stream or anything like that. Not saying this is necessarily wrong, but we should do our homework.

If it's the right one - then it looks like is doing that the same way as we do: https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonOutputFormatter.cs#L77 (plus they're doing additional serialisation from object that we don't want to do).

I think that we should add method ToStream and ToStreamAsync to IQueryable etc. Plus some custom output formatter that'll just return stream with proper encoding and response content type.

jeremydmiller · 2021-01-23T22:50:54Z

src/Marten/Linq/SqlGeneration/JsonSelectClause.cs

                builder.Write(',');
-                builder.Write(text.ReadToEnd());
+                builder.Write(text.GetStreamReader().ReadToEnd());


Let's reevaluate this one to see if there's a way to not have to read and create the string. See if there's a way to pipe the Stream bytes directly to the CommandBuilder

jeremydmiller · 2021-01-23T22:52:13Z

src/Marten/Marten.csproj

@@ -31,6 +31,7 @@
        <PackageReference Include="System.Threading.Tasks.Dataflow" Version="4.11.1" />
        <PackageReference Include="System.Reflection.Emit.Lightweight" Version="4.7.0" />
        <PackageReference Include="Microsoft.Extensions.DependencyInjection.Abstractions" Version="[2.1.0, 6.0.0]" />
+        <PackageReference Include="System.Text.Json" Version="5.0.0" />


We'll make this an add on lib, right? Not be in the box?

This we need to discuss - in .NET 5 it's already built-in. I added NuGet, as built-in version in .NET Core 3.1 is too old and missing features. We can distribute that as separate package, but then maintainance might be harder as we'll have growing start of packages (NodaTime, Console, etc.).

jeremydmiller · 2021-01-23T22:53:23Z

src/Marten/Services/CommandRunnerExtensions.cs

@@ -29,7 +29,7 @@ public static QueryPlan ExplainQuery(this IManagedConnection runner, NpgsqlComma
            cmd.CommandText = string.Concat($"explain ({config} format json) ", cmd.CommandText);
            using (var reader = runner.ExecuteReader(cmd))
            {
-                var queryPlans = reader.Read() ? serializer.FromJson<QueryPlanContainer[]>(reader.GetTextReader(0)) : null;
+                var queryPlans = reader.Read() ? serializer.FromJson<QueryPlanContainer[]>(reader.GetStream(0)) : null;


One thing TODO here might be to make the ISerializer interface take in the DbDataReader and index, then let each serializer to whatever they need to do. So you don't have to make the serializers conform to something that isn't idea.

Refactored in be80fc6.

jeremydmiller · 2021-01-23T22:58:43Z

src/Marten/Services/JsonNetSerializer.cs

-
-
-        private readonly JsonSerializer _clean = new JsonSerializer
+        private readonly JsonSerializer _clean = new()


I'm not sure we use the containment operator anymore, so this might no longer be needed. Not 100% sure about that though.

I tried to use ToJson instead of ToCleanJson but there were set of failing tests around querying interface types collection.

jeremydmiller · 2021-01-23T22:59:42Z

src/Marten/Services/JsonNetSerializer.cs

        }

        public string ToJson(object document)
        {
-            var writer = new StringWriter();
-            ToJson(document, writer);
+            using var stream = new MemoryStream();


Why are we having to write to a stream like this?

I reverted those changes and brought back old implementation after changing method signature with DBReader etc.

jeremydmiller · 2021-01-23T23:01:08Z

src/Marten/Services/JsonNetSerializer.cs

            };

            return _serializer.Deserialize<T>(jsonReader);
        }

-        public T FromJson<T>(TextReader reader)
+        public Task<T> FromJsonAsync<T>(Stream stream, CancellationToken cancellationToken = default)


We need to be using ValueTask.

Updated to ValueTask in 1ef5e80. We should probably review if there are no places in our API that should return ValueTask (however, that's out of scope of this PR).

jeremydmiller · 2021-01-23T23:04:24Z

That looks like a ton of work.

With the ISerializer interface, we might be better off having it take in DbDataReader and an index, then letting each serializer do what works best for it, rather than trying to make an ISerializer conforming interface that isn't ideal. And we've gotta switch to ValueTask instead of returning Task.FromResult() in so many places.

….Text.Json happy (it does not allow to deserialize the public array property supported by the list backing field).

…ility between parser Added JsonPropertyName to QueryPlan to support also System.Text.Json

…o allow System.Text.Json work properly

Added JsonInclude for the field in `Target` test document class. Changed `Float` from field to property, as tests do not assume that it should be field.

Fixed Casing in System.Text.Json

…th SerializerType Used it in StoreOptions Added TestSetup to allow matrix tests on serializers Added TestsSettings to keep the test settings together Added SerializerTypeTargetedFact to allow skipping tests when feature is not supported for the particular serializer Added pipelines for System.Text.Json

…taReader and index params to allow serializers use its own the most pefromant way

Skipped unsupported test scenarios Added SerializerTypeTargetedTheory to skip theories for System.Text.Json

Unified streaming approach

Added serializer matrix to AsynDaemon and Schema testing projects

oskardudycz · 2021-02-13T11:02:18Z

@jeremydmiller @mysticmind PR is ready for the final review.

jeremydmiller · 2021-02-14T16:10:13Z

src/Marten/Services/JsonNetSerializer.cs

        {
-            using var jsonReader = new JsonTextReader(reader)
+            using var textReader = reader.GetTextReader(index);


For later. If we took in the native Npgsql DbDataReader type instead, you get access to async versions of GetTextReader() and GetStream()

jeremydmiller

My vote is to pull this in now.

My only comment beyond "thank you for doing all of this and I'm shocked at how much work it really required" is that we need to think seriously about either passing around NpgsqlReader instead of DbDataReader, or downcast (ick) as appropriate to use the async GetStreamAsync() everywhere.

mysticmind

Looks good to me. Apart from all the changes required to make STJ work, I really liked that you have spruced up the code with C# 9 lang features 👍

BTW, my changes pertaining to changing the code samples markers for migrating docs to vitepress looks to have added conflicts in few files, will address it and then merge the PR.

oskardudycz force-pushed the features/system-text-json branch 2 times, most recently from 3b7839a to 9944183 Compare January 10, 2021 09:10

oskardudycz force-pushed the features/system-text-json branch 2 times, most recently from 6a8a4ad to 523331a Compare January 23, 2021 20:50

jeremydmiller reviewed Jan 23, 2021

View reviewed changes

oskardudycz force-pushed the features/system-text-json branch 4 times, most recently from 64965d4 to c598cdc Compare January 31, 2021 20:13

oskardudycz force-pushed the features/system-text-json branch 3 times, most recently from 0a13912 to a64371b Compare February 12, 2021 20:51

lahma and others added 6 commits February 13, 2021 09:16

trying out the STJ

dfe8932

Improved Newtonsoft.JsonNet deserialization

8990b7e

Added System.Text.Json deserialization

ce9c6d8

Added async versions of FromJson deserialization methods

d0ddbf1

Fixed Casing handling in SystemTextJsonSerializer

cbc4f6b

Replaced backing field in QueryMonsters test class to make the System…

5bc093b

….Text.Json happy (it does not allow to deserialize the public array property supported by the list backing field).

oskardudycz added 14 commits February 13, 2021 09:16

Added SystemObjectNewtonsoftCompatibleConverter for metadata compatib…

936d6f3

…ility between parser Added JsonPropertyName to QueryPlan to support also System.Text.Json

Updated test document constructor parameter type to match the field t…

ef13926

…o allow System.Text.Json work properly

Fixed skipping SOH byte in StreamReader

6b24245

Updated ability_to_persist_generic_types tests to use the real DTO

fbf091c

Added JsonInclude for the field in `Target` test document class. Changed `Float` from field to property, as tests do not assume that it should be field.

Updated test classes to use properties instead of strings

e1f8210

Fixed Skipping SOH byte in Stream reading

443ec02

Added handling for snake_case with System.Text.Json

4e1f99a

Fixed Enum System.Text.Json serialisation.

e52098e

Fixed Casing in System.Text.Json

Added CancellationToken to Serializers methods FromJsonAsync

c53ba86

Updated ISerializer asyn method to return ValueTask

e31bb1b

Refactored ISerializer deserialization methods (FromJson) to use DbDa…

858603b

…taReader and index params to allow serializers use its own the most pefromant way

Fixed Skipping SOH byte at the beginning of the Npgsql Stream

d70c297

Skipped unsupported test scenarios Added SerializerTypeTargetedTheory to skip theories for System.Text.Json

Brought back FromJson methods with Stream param to serializers

ceba441

Unified streaming approach

oskardudycz force-pushed the features/system-text-json branch from a64371b to ceba441 Compare February 13, 2021 09:45

oskardudycz self-assigned this Feb 13, 2021

oskardudycz changed the title ~~WIP: System.Text.Json support~~ System.Text.Json support Feb 13, 2021

oskardudycz requested review from jokokko and mysticmind February 13, 2021 10:07

Reverted redundant changes to Testing classes

224c06c

Added serializer matrix to AsynDaemon and Schema testing projects

oskardudycz force-pushed the features/system-text-json branch from a0ada75 to 224c06c Compare February 13, 2021 10:24

Enabled dynamic type serialisation

f317376

oskardudycz marked this pull request as ready for review February 13, 2021 15:42

jeremydmiller reviewed Feb 14, 2021

View reviewed changes

jeremydmiller approved these changes Feb 14, 2021

View reviewed changes

mysticmind approved these changes Feb 15, 2021

View reviewed changes

Merge branch 'master' into features/system-text-json

aff0416

mysticmind merged commit 5d114a1 into master Feb 15, 2021

mysticmind deleted the features/system-text-json branch February 15, 2021 07:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

System.Text.Json support #1685

System.Text.Json support #1685

oskardudycz commented Dec 27, 2020 •

edited

Loading

jeremydmiller Jan 23, 2021

oskardudycz Feb 13, 2021

jeremydmiller Jan 23, 2021

oskardudycz Feb 13, 2021

jeremydmiller Jan 23, 2021

oskardudycz Feb 13, 2021

jeremydmiller Jan 23, 2021

oskardudycz Jan 27, 2021

jeremydmiller Jan 23, 2021

oskardudycz Jan 27, 2021

jeremydmiller Jan 23, 2021

oskardudycz Feb 7, 2021

oskardudycz Feb 7, 2021 •

edited

Loading

jeremydmiller Jan 23, 2021

jeremydmiller Jan 23, 2021

oskardudycz Feb 6, 2021

jeremydmiller Jan 23, 2021

oskardudycz Feb 6, 2021

jeremydmiller Jan 23, 2021

oskardudycz Feb 6, 2021

jeremydmiller Jan 23, 2021

oskardudycz Feb 6, 2021

jeremydmiller Jan 23, 2021

oskardudycz Jan 27, 2021

jeremydmiller commented Jan 23, 2021

oskardudycz commented Feb 13, 2021

jeremydmiller Feb 14, 2021

jeremydmiller left a comment

mysticmind left a comment •

edited

Loading



		private readonly JsonSerializer _clean = new JsonSerializer
		private readonly JsonSerializer _clean = new()

System.Text.Json support #1685

System.Text.Json support #1685

Conversation

oskardudycz commented Dec 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oskardudycz Feb 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremydmiller commented Jan 23, 2021

oskardudycz commented Feb 13, 2021

Choose a reason for hiding this comment

jeremydmiller left a comment

Choose a reason for hiding this comment

mysticmind left a comment • edited Loading

Choose a reason for hiding this comment

oskardudycz commented Dec 27, 2020 •

edited

Loading

oskardudycz Feb 7, 2021 •

edited

Loading

mysticmind left a comment •

edited

Loading