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

Using System.Text.Json, IAsyncEnumerable, and source generation together doesn't work #58221

Closed
richlander opened this issue Aug 26, 2021 · 9 comments
Assignees
Labels
area-System.Text.Json Priority:1 Work that is critical for the release, but we could probably ship without
Milestone

Comments

@richlander
Copy link
Member

richlander commented Aug 26, 2021

I'm using .NET 6 RC2.

using System.Text.Json;
using System.Text.Json.Serialization;

using Stream stream = Console.OpenStandardOutput();
Measurements data = new(DateTime.Now, GetValues(10));
await JsonSerializer.SerializeAsync<Measurements>(stream, data, JsonContext.Default.Measurements);
Console.WriteLine();

static async IAsyncEnumerable<int> GetValues(int n)
{
    for (int i = 0; i < n; i++) yield return Random.Shared.Next(i);
}
    
internal record Measurements(DateTime time, IAsyncEnumerable<int> Values);

// Source generator definition
[JsonSerializable(typeof(Measurements))]
internal partial class JsonContext : JsonSerializerContext
{
}

This code produces the following result:

{"time":"2021-08-26T15:08:56.93747-07:00","Values":{}}

If I switch out the JsonSerializer call to the following, I get the following desired result.

await JsonSerializer.SerializeAsync<Measurements>(stream, data);

and

{"time":"2021-08-26T15:07:58.698976-07:00","Values":[0,0,0,1,0,3,1,3,0,1]}

Is this scenario unsupported or I'm doing something wrong?

@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Aug 26, 2021
@ghost
Copy link

ghost commented Aug 26, 2021

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

I'm using .NET 6 RC2.

using System.Text.Json;
using System.Text.Json.Serialization;

using Stream stream = Console.OpenStandardOutput();
Measurements data = new(DateTime.Now, GetValues(10));
await JsonSerializer.SerializeAsync<Measurements>(stream, data, JsonContext.Default.Measurements);
Console.WriteLine();

static async IAsyncEnumerable<int> GetValues(int n)
{
    for (int i = 0; i < n; i++) yield return Random.Shared.Next(i);
}
    
internal record Measurements(DateTime time, IAsyncEnumerable<int> Values);

// Source generator definition
[JsonSerializable(typeof(Measurements))]
internal partial class JsonContext : JsonSerializerContext
{
}

This code produces the following result:

{"time":"2021-08-26T15:08:56.93747-07:00","Values":{}}

If I switch out the JsonSerializer call to the following, I get the following result.

await JsonSerializer.SerializeAsync<Measurements>(stream, data);

and

{"time":"2021-08-26T15:07:58.698976-07:00","Values":[0,0,0,1,0,3,1,3,0,1]}

Is this scenario unsupported or I'm doing something wrong?

Author: richlander
Assignees: -
Labels:

area-System.Text.Json, untriaged

Milestone: -

@eiriktsarpalis
Copy link
Member

Related to #53393. We discussed implementing this but ultimately didn't get around to doing it. @layomia @steveharter would it make sense to try and get this in for RC2? Note that it would require new APIs.

@richlander
Copy link
Member Author

I am not asking for a new .NET 6 feature at this point. This seemed like a natural scenario to try. I suggest we document it as non-functional for .NET 6 and re-consider for 7.

@eiriktsarpalis eiriktsarpalis added this to the 6.0.0 milestone Aug 27, 2021
@eiriktsarpalis eiriktsarpalis added blocking-release and removed untriaged New issue has not been triaged by the area owner labels Aug 27, 2021
@eiriktsarpalis
Copy link
Member

Triage: let's keep in 6.0.0 and investigate potential fallbacks. We probably should not add new JsonMetadataServices APIs in 6.0.0.

@steveharter steveharter self-assigned this Aug 27, 2021
@richlander
Copy link
Member Author

As an aside, this pattern wasn't immediately intuitive to me. I started out with this:

internal record Measurements(DateTime time, int[] Values);

Naturally, that didn't work with this:

Measurements data = new(DateTime.Now, GetValues(10));
static async IAsyncEnumerable<int> GetValues(int n)
{
    for (int i = 0; i < n; i++) yield return Random.Shared.Next(i);
}

Normally, we don't model POCOs with either async or IEnumerable. Once the data is realized, there is no need or desire for these mechanisms. I suspect that my problem is that I conflated the types for T and data into one. I suspect that I won't be the first person to run into this modeling problem. It probably deserves some guidance. This challenge would be more obvious if I was using the same POCO for both serialization and deserialization.

@steveharter
Copy link
Member

Related to #53393. We discussed implementing this but ultimately didn't get around to doing it. @layomia @steveharter would it make sense to try and get this in for RC2? Note that it would require new APIs.

Supporting IAsyncEnumerable requires additional public API(s) since the implementation of IAsyncEnumerable is done via a custom converter, like other collection types, and helper method(s) for source-gen would need to be added. However these helpers are just used by the generated code and not generally visible\usable by the consumer.

Something like this (following the pattern used to support other collection types):

namespace System.Text.Json.Serialization.Metadata
{
    public static partial class JsonMetadataServices
    {
+                public static JsonTypeInfo<IAsyncEnumerable> CreateIAsyncEnumerable(System.Text.Json.JsonSerializerOptions options, System.Func<IAsyncEnumerable>? createObjectFunc, System.Text.Json.Serialization.Metadata.JsonTypeInfo elementInfo, System.Text.Json.Serialization.JsonNumberHandling numberHandling, System.Action<Utf8JsonWriter, IAsyncEnumerable>? serializeFunc) { throw null; }
    }
}

Note that although the standard SerializeAsync<T>(Stream) methods work with T being IAsyncEnumerable, when deserializing the explicit DeserializeAsyncEnumerable() must be used, and currently there is not an overload for JsonTypeInfo. Thus if we fix the serialize side, there isn't a way to deserialize without adding additional public DeserializeAsyncEnumerable overloads that takeJsonTypeInfo.

Options for V6:

  • Add the support for serialize, but not deserialize.
  • Add support for both serialize and deserialize
  • Throw NotSupportedException when serializing\deserializing IAsyncEnumerable for source-gen cases.
  • Do nothing (silent failure not a good thing)
  • ...?

@steveharter steveharter removed their assignment Aug 27, 2021
@eiriktsarpalis
Copy link
Member

Good point about IAE deserialization requiring special treatment. Throwing NotSupportedException (or perhaps emitting a compile-time error) seems like a reasonable measure for 6.0.0.

@richlander
Copy link
Member Author

Throwing seems like the best approach for 6. It is worth taking that fix IMO.

@steveharter steveharter self-assigned this Aug 31, 2021
@eiriktsarpalis eiriktsarpalis modified the milestones: 6.0.0, 7.0.0 Sep 11, 2021
@eiriktsarpalis eiriktsarpalis added Priority:1 Work that is critical for the release, but we could probably ship without and removed blocking-release labels Sep 11, 2021
@steveharter steveharter changed the title Using System.Text.Json, IAsynEnumerable, and source generation together doesn't work Using System.Text.Json, IAsyncEnumerable, and source generation together doesn't work Sep 17, 2021
@eiriktsarpalis
Copy link
Member

Closing in favor of #59268.

@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json Priority:1 Work that is critical for the release, but we could probably ship without
Projects
None yet
Development

No branches or pull requests

3 participants