Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide a recipe for creating JsonDocument via the serializer #29501

Closed
rynowak opened this issue May 9, 2019 · 17 comments
Closed

Provide a recipe for creating JsonDocument via the serializer #29501

rynowak opened this issue May 9, 2019 · 17 comments
Assignees
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json tenet-performance Performance related issue
Milestone

Comments

@rynowak
Copy link
Member

rynowak commented May 9, 2019

See here: aspnet-contrib/AspNet.Security.OAuth.Providers@9a80d10#diff-f9b7fb28139b9e23c519bca3be7c2930R154

This is a community project that @Tratcher was helping with, so this could be considered external customer feedback.

They are trying to use JsonDocument as part of their object model to represent some unstructured JSON data, but there's no easy recipe to create a JsonDocument unless you are starting from a string or bytes.


Here's the cool workaround that they used:

        private async Task CopyPayloadAsync(Dictionary<string, StringValues> content, IBufferWriter<byte> bufferWriter)
        {
            await using (var writer = new Utf8JsonWriter(bufferWriter))
            {
                writer.WriteStartObject();
                foreach (var item in content)
                {
                    writer.WriteString(item.Key, item.Value);
                }
                writer.WriteEndObject();
                await writer.FlushAsync();
            }

            return JsonDocument.Parse(bufferWriter.WrittenMemory);
        }

This example uses a dictionary, but there are cases where they have the data already is structured like a POCO object.


A proposal to make this much better would be the serializer could be used to create a JsonDocument directly. This would work really well for cases where you need to transform strongly-typed data into a "JSON Container". We'll also end up needing this anyway once it becomes possible to mutate JsonDocument.

In JSON.NET this is exposed as: https://www.newtonsoft.com/json/help/html/M_Newtonsoft_Json_Linq_JObject_FromObject.htm

return JObject.FromObject(content);
@rynowak
Copy link
Member Author

rynowak commented May 9, 2019

I'm tagging this as blocking to have a discussion about it. If this is really cheap to do, I think we should consider it - this will help fill gaps due to JsonDocument being immutable in this release.

@ahsonkhan
Copy link
Member

If this is really cheap to do, I think we should consider it

After discussing with @bartonjs, adding a convenience API on the serializer to return a JsonDocument which simplifies the user code is doable (effectively do what the user's doing today), particularly for the scenario mentioned in the original post. And we can continue to improve its performance by leveraging internal details of the document, and build up the document as we serialize (to avoid re-parsing), in the future.

Especially after we have serializer interop with the writer (see https://github.com/dotnet/corefx/issues/36714), adding an api that returns a document should be quite straightforward.

@ahsonkhan ahsonkhan self-assigned this May 10, 2019
@ahsonkhan
Copy link
Member

ahsonkhan commented May 17, 2019

Here's the cool workaround that they used

This workaround might have been required previously because of lack of dictionary support within the serializer.

I believe the caller can now write something like this:

private JsonDocument CopyPayload(Dictionary<string, StringValues> content)
{
    byte[] utf8JsonData = JsonSerializer.ToBytes<Dictionary<string, StringValues>>(content);
    return JsonDocument.Parse(utf8JsonData);
}

Given that, I am don't see this issue as blocking. The convenience API is nice to have, but deferring for now. Please let me know if the above solution doesn't work and we can re-evaluate.

@martincostello
Copy link
Member

I tried out the above with preview 5, but right now it's throwing an exception:

   System.Exception : An error was encountered while handling the remote login.
  ---- System.InvalidCastException : Unable to cast object of type 'System.Collections.Generic.KeyValuePair`2[System.Str
ing,Microsoft.Extensions.Primitives.StringValues]' to type 'System.Collections.IEnumerable'.
  Stack Trace:
     at Microsoft.AspNetCore.Authentication.RemoteAuthenticationHandler`1.HandleRequestAsync()
     at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
     at Microsoft.AspNetCore.TestHost.HttpContextBuilder.<>c__DisplayClass14_0.<<SendAsync>b__0>d.MoveNext()
  --- End of stack trace from previous location where exception was thrown ---
     at Microsoft.AspNetCore.TestHost.ClientHandler.SendAsync(HttpRequestMessage request, CancellationToken cancellation
Token)
     at AspNet.Security.OAuth.Infrastructure.LoopbackRedirectHandler.SendAsync(HttpRequestMessage request, CancellationT
oken cancellationToken) in C:\Coding\AspNet.Security.OAuth.Providers\test\AspNet.Security.OAuth.Providers.Tests\Infrastr
ucture\LoopbackRedirectHandler.cs:line 58
     at System.Net.Http.HttpClient.FinishSendAsyncBuffered(Task`1 sendTask, HttpRequestMessage request, CancellationToke
nSource cts, Boolean disposeCts)
     at AspNet.Security.OAuth.OAuthTests`1.AuthenticateUserAsync(WebApplicationFactory`1 server) in C:\Coding\AspNet.Sec
urity.OAuth.Providers\test\AspNet.Security.OAuth.Providers.Tests\OAuthTests`1.cs:line 116
     at AspNet.Security.OAuth.QQ.QQTests.Can_Sign_In_Using_QQ(String claimType, String claimValue) in C:\Coding\AspNet.S
ecurity.OAuth.Providers\test\AspNet.Security.OAuth.Providers.Tests\QQ\QQTests.cs:line 44
  --- End of stack trace from previous location where exception was thrown ---
  ----- Inner Stack Trace -----
     at System.Text.Json.Serialization.JsonSerializer.HandleEnumerable(JsonClassInfo elementClassInfo, JsonSerializerOpt
ions options, Utf8JsonWriter writer, WriteStack& state)
     at System.Text.Json.Serialization.JsonSerializer.Write(Utf8JsonWriter writer, Int32 flushThreshold, JsonSerializerO
ptions options, WriteStack& state)
     at System.Text.Json.Serialization.JsonSerializer.WriteCore(PooledBufferWriter`1 output, Object value, Type type, Js
onSerializerOptions options)
     at System.Text.Json.Serialization.JsonSerializer.WriteCoreBytes(Object value, Type type, JsonSerializerOptions opti
ons)
     at System.Text.Json.Serialization.JsonSerializer.ToBytes[TValue](TValue value, JsonSerializerOptions options)
     at AspNet.Security.OAuth.QQ.QQAuthenticationHandler.CopyPayload(Dictionary`2 content) in C:\Coding\AspNet.Security.
OAuth.Providers\src\AspNet.Security.OAuth.QQ\QQAuthenticationHandler.cs:line 157
     at AspNet.Security.OAuth.QQ.QQAuthenticationHandler.ExchangeCodeAsync(String code, String redirectUri) in C:\Coding
\AspNet.Security.OAuth.Providers\src\AspNet.Security.OAuth.QQ\QQAuthenticationHandler.cs:line 118
     at Microsoft.AspNetCore.Authentication.OAuth.OAuthHandler`1.HandleRemoteAuthenticateAsync()
     at Microsoft.AspNetCore.Authentication.RemoteAuthenticationHandler`1.HandleRequestAsync()

@Tratcher
Copy link
Member

Tratcher commented May 18, 2019

Hmm, corefx issue tracking doesn't tell you what preview work items were completed in...

Here's the PR: dotnet/corefx#37186, merged 22 days ago so that might be preview6.

@bartonjs
Copy link
Member

@Tratcher The merge commit from that PR (dotnet/corefx@c5bb6ab) only says it's part of master. Backing up to dotnet/corefx@431f655 we see one that is tagged as part of preview5. Ergo, I agree with your conclusion 😄.

@ahsonkhan
Copy link
Member

@martincostello, are you still seeing an issue on latest preview?

@martincostello
Copy link
Member

@ahsonkhan I just tried it out with the following tweaked update to the sample above for preview 6 and got this exception:

private JsonDocument CopyPayload(Dictionary<string, StringValues> content)
{
    byte[] utf8JsonData = JsonSerializer.ToUtf8Bytes(content);
    return JsonDocument.Parse(utf8JsonData);
}
System.NotSupportedException : The collection type 'Microsoft.Extensions.Primitives.StringValues' is not supported.
   at System.Text.Json.Serialization.JsonClassInfo.GetElementType(Type propertyType, Type parentType, MemberInfo memberInfo)
   at System.Text.Json.Serialization.JsonClassInfo.CreateProperty(Type declaredPropertyType, Type runtimePropertyType, PropertyInfo propertyInfo, Type parentClassType, JsonSerializerOptions options)
   at System.Text.Json.Serialization.JsonClassInfo.AddProperty(Type propertyType, PropertyInfopropertyInfo, Type classType, JsonSerializerOptions options)
   at System.Text.Json.Serialization.JsonClassInfo..ctor(Type type, JsonSerializerOptions options)
   at System.Text.Json.Serialization.JsonSerializerOptions.GetOrAddClass(Type classType)
   at System.Text.Json.Serialization.JsonClassInfo..ctor(Type type, JsonSerializerOptions options)
   at System.Text.Json.Serialization.JsonSerializerOptions.GetOrAddClass(Type classType)
   at System.Text.Json.Serialization.WriteStackFrame.Initialize(Type type, JsonSerializerOptions options)
   at System.Text.Json.Serialization.JsonSerializer.WriteCore(PooledByteBufferWriter output, Object value, Type type, JsonSerializerOptions options)
   at System.Text.Json.Serialization.JsonSerializer.WriteCoreBytes(Object value, Type type, JsonSerializerOptions options)
   at System.Text.Json.Serialization.JsonSerializer.ToUtf8Bytes[TValue](TValue value, JsonSerializerOptions options)
   at AspNet.Security.OAuth.QQ.QQAuthenticationHandler.CopyPayload(Dictionary`2 content)

@ahsonkhan
Copy link
Member

StringValues is a struct that implements IList<string>. I don't think that is supported. It' the same issue as https://github.com/dotnet/corefx/issues/38767

cc @steveharter, @layomia

@layomia
Copy link
Contributor

layomia commented Jul 11, 2019

StringValues is a struct that implements IList<string>. I don't think that is supported. It' the same issue as dotnet/corefx#38767

cc @steveharter, @layomia

@martincostello are you still seeing the StringValues issue now that dotnet/corefx#39001 is in?

@martincostello
Copy link
Member

@layomia I haven't tried it out yet - I was waiting for the "official" preview 7 to drop.

@martincostello
Copy link
Member

@layomia Still no joy with 3.0.100-preview7-012821.

Using this snippet:

private JsonDocument CopyPayload(Dictionary<string, StringValues> content)
{
    byte[] utf8JsonData = JsonSerializer.SerializeToUtf8Bytes(content);
    return JsonDocument.Parse(utf8JsonData);
}

I get this exception:

   System.Exception : An error was encountered while handling the remote login.
---- System.NotSupportedException : The collection type 'Microsoft.Extensions.Primitives.StringValues' is not supported.

  Stack Trace:
     at Microsoft.AspNetCore.Authentication.RemoteAuthenticationHandler`1.HandleRequestAsync()
   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.TestHost.HttpContextBuilder.<>c__DisplayClass20_0.<<SendAsync>g__RunRequestAsync|0>d.MoveNext
()
--- End of stack trace from previous location where exception was thrown ---
   at Microsoft.AspNetCore.TestHost.ClientHandler.SendAsync(HttpRequestMessage request, CancellationToken cancellationTo
ken)
   at AspNet.Security.OAuth.Infrastructure.LoopbackRedirectHandler.SendAsync(HttpRequestMessage request, CancellationTok
en cancellationToken) in C:\Coding\AspNet.Security.OAuth.Providers\test\AspNet.Security.OAuth.Providers.Tests\Infrastruc
ture\LoopbackRedirectHandler.cs:line 43
   at System.Net.Http.HttpClient.FinishSendAsyncBuffered(Task`1 sendTask, HttpRequestMessage request, CancellationTokenS
ource cts, Boolean disposeCts)
   at AspNet.Security.OAuth.OAuthTests`1.AuthenticateUserAsync(WebApplicationFactory`1 server) in C:\Coding\AspNet.Secur
ity.OAuth.Providers\test\AspNet.Security.OAuth.Providers.Tests\OAuthTests`1.cs:line 136
   at AspNet.Security.OAuth.QQ.QQTests.Can_Sign_In_Using_QQ(String claimType, String claimValue) in C:\Coding\AspNet.Sec
urity.OAuth.Providers\test\AspNet.Security.OAuth.Providers.Tests\QQ\QQTests.cs:line 44
--- End of stack trace from previous location where exception was thrown ---
----- Inner Stack Trace -----
   at System.Text.Json.JsonClassInfo.GetElementType(Type propertyType, Type parentType, MemberInfo memberInfo, JsonSeria
lizerOptions options)
   at System.Text.Json.JsonClassInfo.CreateProperty(Type declaredPropertyType, Type runtimePropertyType, PropertyInfo pr
opertyInfo, Type parentClassType, JsonSerializerOptions options)
   at System.Text.Json.JsonClassInfo.AddProperty(Type propertyType, PropertyInfo propertyInfo, Type classType, JsonSeria
lizerOptions options)
   at System.Text.Json.JsonClassInfo.AddPolicyProperty(Type propertyType, JsonSerializerOptions options)
   at System.Text.Json.JsonClassInfo..ctor(Type type, JsonSerializerOptions options)
   at System.Text.Json.JsonSerializerOptions.GetOrAddClass(Type classType)
   at System.Text.Json.JsonClassInfo..ctor(Type type, JsonSerializerOptions options)
   at System.Text.Json.JsonSerializerOptions.GetOrAddClass(Type classType)
   at System.Text.Json.WriteStackFrame.Initialize(Type type, JsonSerializerOptions options)
   at System.Text.Json.JsonSerializer.WriteCore(Utf8JsonWriter writer, PooledByteBufferWriter output, Object value, Type
 type, JsonSerializerOptions options)
   at System.Text.Json.JsonSerializer.WriteCore(PooledByteBufferWriter output, Object value, Type type, JsonSerializerOp
tions options)
   at System.Text.Json.JsonSerializer.WriteCoreBytes(Object value, Type type, JsonSerializerOptions options)
   at System.Text.Json.JsonSerializer.SerializeToUtf8Bytes[TValue](TValue value, JsonSerializerOptions options)
   at AspNet.Security.OAuth.QQ.QQAuthenticationHandler.CopyPayload(Dictionary`2 content) in C:\Coding\AspNet.Security.OA
uth.Providers\src\AspNet.Security.OAuth.QQ\QQAuthenticationHandler.cs:line 154
   at AspNet.Security.OAuth.QQ.QQAuthenticationHandler.ExchangeCodeAsync(OAuthCodeExchangeContext context) in C:\Coding\
AspNet.Security.OAuth.Providers\src\AspNet.Security.OAuth.QQ\QQAuthenticationHandler.cs:line 115
   at Microsoft.AspNetCore.Authentication.OAuth.OAuthHandler`1.HandleRemoteAuthenticateAsync()
   at Microsoft.AspNetCore.Authentication.RemoteAuthenticationHandler`1.HandleRequestAsync()

@layomia
Copy link
Contributor

layomia commented Aug 12, 2019

Still no joy with 3.0.100-preview7-012821.

@martincostello Enumerable support in JsonSerializer is scoped to types in System.Collections /System.Collections.Generic and types that derive from them (particularly user-defined types).

Support for derived types was added in dotnet/corefx#39001 (part of preview 8). I thought this PR would enable support for StringValues (implements ICollection<string>), but it doesn't only serialization works (see https://github.com/dotnet/corefx/issues/40237).

The solution here is to add a custom converter that implements JsonConverter<T> where T is StringValues. See https://github.com/dotnet/corefx/issues/36639#issue-429928740 for some examples.

@layomia
Copy link
Contributor

layomia commented Aug 12, 2019

@martincostello My comments above are for deserialization. Your serialization scenario above works now in preview 8/latest code in master.

@martincostello
Copy link
Member

@layomia Thanks - I'll try swapping over to it in the providers' PR to swap over to preview 8 once it lands officially.

@martincostello
Copy link
Member

@layomia The snippet I posted above now works with preview 8.

It has now exposed that the OAuth provider was assuming each key-value pair only had one value in the query string (which in practical terms isn't unreasonable, even if technically incorrect) and was treating the whole thing as a string, so the new recipe actually breaks things as it serializes the payload (correctly) as an array now.

I'll need to stick with the original code anyway to ensure the OAuth token is built in the right format, but otherwise the general case for StringValues serialization does indeed appear fixed 👍.

@layomia
Copy link
Contributor

layomia commented Nov 23, 2019

Closing as duplicate of https://github.com/dotnet/corefx/issues/42056. The semantics of this feature are covered in the API proposal:

namespace System.Text.Json
{
    public static partial class JsonSerializer
    {
        public static TValue Deserialize<TValue>(this JsonDocument document);
        public static TValue Deserialize<TValue>(this JsonElement element);

        public static JsonDocument SerializeToDocument<TValue>(TValue value);
    }
}

@layomia layomia closed this as completed Nov 23, 2019
@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 5.0 milestone Feb 1, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json tenet-performance Performance related issue
Projects
None yet
Development

No branches or pull requests

7 participants