Add JsonContent.Create overloads that accept JsonTypeInfo.

[layomia]

EDIT New proposal can be found here: #51544 (comment)

Details

Background

The existing JsonContent type in System.Net.Http.Json uses existing JsonSerializerOptions-based JsonSerializer.Serialize(Async) overloads which root reflection code and all built-in STJ converters (even when not needed in an app). We should add a new type that uses new JsonSerializerContext-based overloads, which are AOT-friendly and allow for a smaller app. Also the existing type has been marked as "requires unreferenced code", so it is beneficial to provide an alternative which is trim-friendly.

API Proposal

public sealed partial class JsonContent<TValue> : HttpContent
{
  internal JsonContent() { }
  public TValue? Value { get { throw null; } }
}

We should also expose static Create methods for these types to the existing JsonContent :

public sealed partial class JsonContent : HttpContent
{
  // Existing
  // internal JsonContent() { }
  // public Type ObjectType { get { throw null; } }
  // public object? Value { get { throw null; } }
  // public static JsonContent Create(object? inputValue, Type inputType, MediaTypeHeaderValue? mediaType = null, JsonSerializerOptions? options = null) { throw null; }
  // public static JsonContent Create<T>(T inputValue, MediaTypeHeaderValue? mediaType = null, JsonSerializerOptions? options = null) { throw null; }

  // New create methods for JsonContent<TValue>
  public static JsonContent<object?> Create(object? inputValue, Type inputType, JsonSerializerContext context, MediaTypeHeaderValue? mediaType = null) { throw null; }
  public static JsonContent<TValue> Create(TValue? inputValue, JsonTypeInfo<TValue> jsonTypeInfo, MediaTypeHeaderValue? mediaType = null) { throw null; }
}

The new create methods should go on JsonContent - if we put them on JsonContent<TValue>, then it will be possible for users to write awkward code like this:

JsonContent<object> content = JsonContent<int>.Create(myVal, type, context);

Usage

Just like the existing JsonContent type, JsonContent<TValue> can be used when building a HttpResponseMessage based on JSON. The difference is that now the JSON will be serialized in a trim-safe way which is also optimized for app size:

public static Task<HttpResponseMessage> PutAsJsonAsync<TValue>(this HttpClient client, string? requestUri, TValue value, JsonTypeInfo<TValue> jsonTypeInfo, CancellationToken cancellationToken = default)
{
  if (client == null)
  {
      throw new ArgumentNullException(nameof(client));
  }

  JsonContent<TValue> content = JsonContent.Create(value, jsonTypeInfo);
  return client.PutAsync(requestUri, content, cancellationToken);
}

Q/A

Can JsonContent<TValue> derive from JsonContent<T>?

It's better not to do this, because the ILLinker won't be able to trim away reflection-based code and extra JsonConverters used by the existing JsonContent type. The underlying infrastructure for serializing the JSON content is based on overriding virtual methods. Making the new JsonContent<TValue> type derive from the existing type is bad because the linker will preserve the virtual and override methods of called methods in the hierarchy, because it doesn't know which one will be called at run-time. This is discussed in more detail here.

[ghost]

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

Issue Details

---

Following up from #51528, implement the following APIs approved in #45448. The corresponding HttpClientJsonExtensions.GetFromJsonAsync methods have already been implemented.

namespace System.Net.Http.Json
{
  public static partial class HttpClientJsonExtensions
  {
    public static Task<HttpResponseMessage> PostAsJsonAsync<TValue>(this HttpClient client, string? requestUri, TValue value, JsonTypeInfo<TValue> jsonTypeInfo, CancellationToken cancellationToken = default(CancellationToken)) { throw null; }
    public static Task<HttpResponseMessage> PostAsJsonAsync<TValue>(this HttpClient client, System.Uri? requestUri, TValue value, JsonTypeInfo<TValue> jsonTypeInfo, CancellationToken cancellationToken = default(CancellationToken)) { throw null; }
    public static Task<HttpResponseMessage> PutAsJsonAsync<TValue>(this HttpClient client, string? requestUri, TValue value, JsonTypeInfo<TValue> jsonTypeInfo, CancellationToken cancellationToken = default(CancellationToken)) { throw null; }
    public static Task<HttpResponseMessage> PutAsJsonAsync<TValue>(this HttpClient client, System.Uri? requestUri, TValue value, JsonTypeInfo<TValue> jsonTypeInfo, CancellationToken cancellationToken = default(CancellationToken)) { throw null; }
  }

  public sealed partial class JsonContent : HttpContent
  {
    public static JsonContent Create(object? inputValue, Type inputType, JsonSerializerContext context, MediaTypeHeaderValue mediaType = null) { throw null; }
    public static JsonContent Create<T>(T inputValue, JsonTypeInfo<T> jsonValueInfo, MediaTypeHeaderValue mediaType = null) { throw null; }
  }
}

The challenge here is making these methods linker/size friendly, since the underlying HttpContent.SerializeToStreamAsync abstraction on JsonContent is already reserved for the JsonSerializerOptions-based implementation, which roots all built-in converters (size), and reflection-based logic to create type metadata (linker unfriendly).

The solution could be to add new (internal) PostAsync methods directly on JsonContent which can accept and use type metadata to forward to JsonSerializer.

Currently targeting preview 5 to get this work in. cc @eerhardt, @CoffeeFlux, @jozkee.

Author:	layomia
Assignees:	layomia
Labels:	area-System.Text.Json
Milestone:	6.0.0

[bartonjs]

Video

• Because the JsonContent type is currently sealed with a non-public constructor there's more flexibility and we can add the JsonContent<T> : JsonContent relationship.
• The JsonContent.Create that takes a TValue parameter should be declared generic (typo).

namespace System.Net.Http.Json
{
    public sealed partial class JsonContent<TValue> : JsonContent
    {
        internal JsonContent() { }
        public TValue? Value { get { throw null; } }
    }
    partial class JsonContent
    {
        public static JsonContent<object?> Create(object? inputValue, Type inputType, JsonSerializerContext context, MediaTypeHeaderValue? mediaType = null) { throw null; }
        public static JsonContent<TValue> Create<TValue>(TValue? inputValue, JsonTypeInfo<TValue> jsonTypeInfo, MediaTypeHeaderValue? mediaType = null) { throw null; }
    }
}

namespace System.Net.Http.Json
{
-    public sealed partial class JsonContent
+    public abstract partial class JsonContent
     {
        internal JsonContent();
     }
+   internal sealed partial class ReflectionBasedJsonContent  : JsonContent { ... }
}

[layomia]

Triage: we can do this in .NET 7.

[eiriktsarpalis]

Question: what necessitates needing to make JsonContent<T> public? It's my impression that the T parameter can be encapsulated in the implementation and hidden away from the user. E.g.

    partial class JsonContent
    {
        public static JsonContent Create(object? inputValue, Type inputType, JsonSerializerContext context, MediaTypeHeaderValue? mediaType = null) { throw null; }
        public static JsonContent Create<TValue>(TValue? inputValue, JsonTypeInfo<TValue> jsonTypeInfo, MediaTypeHeaderValue? mediaType = null) { throw null; }
    }

[layomia]

@eiriktsarpalis IL trimming: we need a separate content type which does not derive from JsonContent so that we can provide a fresh implementation of HttpContent.SerializeToStreamAsync that does not root the JsonSerializer methods that use reflection. Some discussion here: #51609 (comment).

[martincostello]

I was just following this blog post which still lists the new JsonContent.Create() overloads as part of .NET 6, but couldn't find them which lead me to this issue.

Might be worth updating the blog post to remove them. I'd have left a comment on the blog, but comments are closed.

[layomia]

I've updated the blog post - we should add these APIs in .NET 7.

[tipa]

This issue seems to have been postponed a few times. Is this planned for .NET8 or should I be looking for workarounds (e.g. using StringContent) instead?

[eiriktsarpalis]

It's currently in the Future milestone so we're not planning on delivering this for .NET 8.

[tipa]

Ok, too bad. Is there a recommended workaround? Like using JsonSerializer.Serialize and hand over the string to a StringContent object? Or is there something else that I might have overlooked? Thanks!

[eiriktsarpalis]

You could use the existing overloads accepting JsonSerializerOptions, if you pass in the Options property associated with a source generated JsonSerializerContext.

[lateapexearlyspeed]

Hi, when think about implementation, I have question about the new class JsonContent<T> shape (I know it had been approved though)
I understand we should use new class rather than existing JsonContent for IL trimming, but just confirm, is using a generic version JsonContent to accept both compiler-timed type and run-timed type argument proper ? Note for run-time type, return type of the Create() method overload is JsonContent<object?>
For implementation, consider constructor of JsonContent<T> is internal, so during 2 public Create() methods, we can just unify both Create()'s implementations to generate info like:
(object inputValue and JsonTypeInfo jsonTypeInfo) and then pass them to new JsonContent class which just uses these info to use source generate.
The only thing I can find is because JsonSerializer.Serialize(Stream utf8Json, object? value, JsonTypeInfo jsonTypeInfo) not existing in old STJ package ?
Please correct me, thanks !

[brantburnett]

It's currently in the Future milestone so we're not planning on delivering this for .NET 8.

Is this postponed simply due to team bandwidth and you'd be interested in accepting a community contribution that meets the accepted API surface on this issue? Or are there other considerations? I ask because this would be very helpful for https://github.com/CenterEdge/Yardarm so I'd be happy to do the work to get it included.

[eiriktsarpalis]

It's also lack of bandwidth, but it's also the fact that the proposal was approved two years ago and is probably no longer fit for purpose given the recent additions in contract customisation:

1. We should not expose the generic JsonContent<T>. The reason originally cited does not apply since .NET 7 and the inclusion of the EnableReflectionByDefault feature switch in .NET 8 means that even with the current code unchanged we don't risk rooting reflection components unless done intentionally.
2. We should not expose a JsonSerializerContext overload. Most scenaria nowadays (including the aspnetcore defaults in .NET 8) involve combining multiple contexts so in most cases JsonSerializerOptions is the predominant unit of serialization configuration.

We should instead consider exposing overloads that do not force linker warnings at all:

public sealed partial class JsonContent : HttpContent
{
  public static JsonContent Create(TValue? inputValue, JsonTypeInfo<TValue> jsonTypeInfo, MediaTypeHeaderValue? mediaType = null);
  public static JsonContent Create(object? inputValue, JsonTypeInfo typeInfo, MediaTypeHeaderValue? mediaType = null);
}

[tipa]

You could use the existing overloads accepting JsonSerializerOptions, if you pass in the Options property associated with a source generated JsonSerializerContext.

I tried it like you recommended:

requestMessage.Content = JsonContent.Create(new MyPostObject()
{
    ....
}, options: new JsonSerializerOptions() { TypeInfoResolver = MyContext.Default });

However the same build warning persists, because the same method is still being called, just with an options object instead of null. I don't see another overload I could use instead.

warning IL2026: Using member 'System.Net.Http.Json.JsonContent.Create<T>(T, MediaTypeHeaderValue, JsonSerializerOptions)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. JSON serialization and deserialization might require types that cannot be statically analyzed. Use the overload that takes a JsonTypeInfo or JsonSerializerContext, or make sure all of the required types are preserved.

[eiriktsarpalis]

@tipa it's unfortunate, but all serialization overloads accepting a JsonSerializerOptions parameter will produce this linker warning because of their default behavior. Assuming you're passing it an options that belongs to a JsonSerializerContext then that warning should remain a false positive that does not translate to any run time linker-related problems.

[bartonjs]

Video

Looks good as proposed (modulo typos)

namespace System.Net.Http.Json
{
    public partial class JsonContent
    {
        public static JsonContent Create<TValue>(TValue? inputValue, JsonTypeInfo<TValue> jsonTypeInfo, MediaTypeHeaderValue? mediaType = null);
        public static JsonContent Create(object? inputValue, JsonTypeInfo jsonTypeInfo, MediaTypeHeaderValue? mediaType = null);
    }
}

[brantburnett]

Great to see this API approved! I'd still love to take this on if that helps, just let me know. I don't want to cross streams if someone is planning to take this internally.

[eiriktsarpalis]

Feel free to pick it up @brantburnett