[API Proposal]: Add new overloads to ReadFromJsonAsync method in HttpContentJsonExtensions

[N0D4N]

Background and motivation

Currently ReadFromJsonAsync method parameters follow pattern (with parameter names skipped): this HttpContent, Type/*if it isn't generic overload*/, JsonSerializerOptions? = null /*or JsonSerializerContext or JsonTypeInfo<T>*/, CancellationToken = default https://github.com/dotnet/runtime/blob/a5f3676cc71e176084f0f7f1f6beeecd86fbeafc/src/libraries/System.Net.Http.Json/ref/System.Net.Http.Json.cs#L85-L89 Which makes impossible calling ReadFromJsonAsync method without specifying JsonSerializerOptions/JsonSerializerContext/JsonTypeInfo<T> parameters, but with passing CancellationToken parameter. Prior to S.T.J source-gen it was possible via calling for example content.ReadFromJsonAsync<MyClass>(null, cancellationToken). However with introducing json source-gen now this is impossible since compiler can't deduce type behind null passed as param, since JsonSerializerOptions overload was the only one, and now it isn't. So now user needs to specify parameter of null like this content.ReadFromJsonAsync<MyClass>((JsonSerializerOptions?)null, cancellationToken) which is additional typing, and doesn't look pleasant.

API Proposal

namespace System.Net.Http.Json;

public static partial class HttpContentJsonExtensions
{
        public static Task<object?> ReadFromJsonAsync(this HttpContent content, Type type, CancellationToken cancellationToken = default) { throw null; }
        public static Task<T?> ReadFromJsonAsync<T>(this HttpContent content, CancellationToken cancellationToken = default) { throw null; }
}

It should also be noted that HttpClientJsonExtensions methods overloads already follow the same pattern as proposed of skipping nullable JsonSerializerOptions parameter: https://github.com/dotnet/runtime/blob/a5f3676cc71e176084f0f7f1f6beeecd86fbeafc/src/libraries/System.Net.Http.Json/ref/System.Net.Http.Json.cs#L25

API Usage

public async Task<MyClass> MyMethodAsync(HttpRequestMessage message, CancellationToken cancellationToken)
{
	// do some work
	return await message.Content.ReadAsJsonAsync(cancellationToken);
	// compared to
	// return await message.Content.ReadFromJsonAsync<MyClass>((JsonSerializerOptions?)null, cancellationToken)
}

Alternative Designs

Can't think of any.

Risks

Can't think of any, since it follows the same pattern present as in other class.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background and motivation

Currently ReadFromJsonAsync method parameters follow pattern (with parameter names skipped): this HttpContent, Type/*if it isn't generic overload*/, JsonSerializerOptions? = null /*or JsonSerializerContext or JsonTypeInfo<T>*/, CancellationToken = default https://github.com/dotnet/runtime/blob/a5f3676cc71e176084f0f7f1f6beeecd86fbeafc/src/libraries/System.Net.Http.Json/ref/System.Net.Http.Json.cs#L85-L89 Which makes impossible calling ReadFromJsonAsync method without specifying JsonSerializerOptions/JsonSerializerContext/JsonTypeInfo<T> parameters, but with passing CancellationToken parameter. Prior to S.T.J source-gen it was possible via calling for example content.ReadFromJsonAsync<MyClass>(null, cancellationToken). However with introducing json source-gen now this is impossible since compiler can't deduce type behind null passed as param, since JsonSerializerOptions overload was the only one, and now it isn't. So now user needs to specify parameter of null like this content.ReadFromJsonAsync<MyClass>((JsonSerializerOptions?)null, cancellationToken) which is additional typing, and doesn't look pleasant.

API Proposal

namespace System.Net.Http.Json;

public static partial class HttpContentJsonExtensions
{
        public static Task<object?> ReadFromJsonAsync(this HttpContent content, Type type, CancellationToken cancellationToken = default) { throw null; }
        public static Task<T?> ReadFromJsonAsync<T>(this HttpContent content, CancellationToken cancellationToken = default) { throw null; }
}

It should also be noted that HttpClientJsonExtensions methods overloads already follow the same pattern as proposed of skipping nullable JsonSerializerOptions parameter: https://github.com/dotnet/runtime/blob/a5f3676cc71e176084f0f7f1f6beeecd86fbeafc/src/libraries/System.Net.Http.Json/ref/System.Net.Http.Json.cs#L15

API Usage

public async Task<MyClass> MyMethodAsync(HttpRequestMessage message, CancellationToken cancellationToken)
{
	// do some work
	return await message.Content.ReadAsJsonAsync(cancellationToken);
	// compared to
	// return await message.Content.ReadFromJsonAsync<MyClass>((JsonSerializerOptions?)null, cancellationToken)
}

Alternative Designs

Can't think of any.

Risks

Can't think of any, since it follows the same pattern present as in other class.

Author:	N0D4N
Assignees:	-
Labels:	api-suggestion, area-System.Text.Json, untriaged
Milestone:	-

[eiriktsarpalis]

Sounds reasonable from a consistency standpoint, since HttpClientJsonExtensions is already doing this.

[terrajobst]

Video

The intended usage

return await message.Content.ReadAsJsonAsync(cancellationToken);

almost works today, by using named arguments:

return await message.Content.ReadAsJsonAsync(cancellationToken: cancellationToken);

Which would bind to this method:

namespace System.Net.Http.Json;

public static class HttpContentJsonExtensions
{
    [RequiresDynamicCode("...")]
    [RequiresUnreferencedCode("...")]
    public static Task<T> ReadFromJsonAsync<T>(this HttpContent content,
                                               JsonSerializerOptions? options = null,
                                               CancellationToken cancellationToken = default);
}

We're not opposed to adding this overload but then we should probably add corresponding ones for the methods on HttpClientJsonExtensions.

[N0D4N]

return await message.Content.ReadAsJsonAsync(cancellationToken: cancellationToken);

Is good workaround, i didn't think of at the time of writing a proposal.

However, HttpClientJsonExtensions is already following the approach i proposed for HttpContentJsonExtensions:

https://github.com/dotnet/runtime/blob/a5f3676cc71e176084f0f7f1f6beeecd86fbeafc/src/libraries/System.Net.Http.Json/ref/System.Net.Http.Json.cs#L25

https://github.com/dotnet/runtime/blob/a5f3676cc71e176084f0f7f1f6beeecd86fbeafc/src/libraries/System.Net.Http.Json/ref/System.Net.Http.Json.cs#L35

https://github.com/dotnet/runtime/blob/a5f3676cc71e176084f0f7f1f6beeecd86fbeafc/src/libraries/System.Net.Http.Json/ref/System.Net.Http.Json.cs#L65

to name a few, there is more at the source file.
That's why i think its good to have HttpContentJsonExtensions follow this approach from consistency point of view.

So i am not sure what should be done here, i assume proposal should be marked as api-ready-for-review, and go through review process once again, this time considering HttpClientJsonExtensions already follow proposed approach?

[N0D4N]

@eiriktsarpalis sorry for ping, but i figured you would be the best person to ask, since you are an area owner and you commented here already. A month have passed but no action were taken, can you, please, point what should be done here? In my opinion it should be remarked as ready-for-review since people on review missed notes about proposed api bringing api parity, and they are generally fine (as far as i understand) with proposed API. Or should it be postponed to a later date, since 8.0 release is more than a year away?

[eiriktsarpalis]

I missed that API review meeting, but it looks like the fact that the same overloads already exist in HttpClientJsonExtensions is something that wasn't pointed out during review. I think we should take another look cc @terrajobst

[bartonjs]

Video

• We should add the overloads as presented, with a defaulted CancellationToken; but we need to remove the defaults from JsonSerializerOptions overloads to avoid the defaults making an ambiguous compile error.

namespace System.Net.Http.Json;

public static partial class HttpContentJsonExtensions
{
        public static Task<object?> ReadFromJsonAsync(this HttpContent content, Type type, CancellationToken cancellationToken = default) { throw null; }
        public static Task<T?> ReadFromJsonAsync<T>(this HttpContent content, CancellationToken cancellationToken = default) { throw null; }

        // Remove the defaults for `JsonSerializerOptions? options`
        public static System.Threading.Tasks.Task<object?> ReadFromJsonAsync(this System.Net.Http.HttpContent content, System.Type type, System.Text.Json.JsonSerializerOptions? options, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } 
        public static System.Threading.Tasks.Task<T?> ReadFromJsonAsync<T>(this System.Net.Http.HttpContent content, System.Text.Json.JsonSerializerOptions? options, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } 
 
}

[eiriktsarpalis]

@N0D4N would you be interested in contributing an implementation?

[N0D4N]

@eiriktsarpalis sure! Just need to look up docs on adding new API.

[N0D4N]

Wait, maybe i miss something, But adding overloads with removed defaults JsonSerializerOptions is impossible since overloads with defaults are already shipped in .NET 5 & 6 and AFAIK C# doesn't allow overloading on parameters being default or not. And these overloads with JsonSerializerOptions without default value weren't proposed to add in original post. Or the idea is to make a breaking change by removing default values from overloads? Judging by video it isn't the case but i just want to be clear here.

[eiriktsarpalis]

Or the idea is to make a breaking change by removing default values from overloads?

Removing the default value from a public method is technically a source breaking change, however in this case the compiler will simply resolve to the newly added overloads which we expect to have the same behavior.

[N0D4N]

I think i get it, but wouldn't it be binary breaking change? And i don't see a reason to do it. In video it's mentioned that calls with just HttpContent would become ambiguous if we add proposed overloads, but don't change existing by removing defaulting parameter to JsonSerializerOptions, but i don't see it being true

using System.Net.Http.Json;

HttpContent c = new StringContent("/*json here*/");
// Pretend we get CancellationToken from somewhere else
using var cts = new CancellationTokenSource();

// Use case we want to achieve by adding new overloads
//c.ReadFromJsonAsync(typeof(string), cts.Token);
//c.ReadFromJsonAsync<string>(cts.Token);

c.ReadFromJsonAsync(typeof(string));
c.ReadFromJsonAsync<string>();
c.ReadFromJsonAsync(typeof(string), cancellationToken: cts.Token);
c.ReadFromJsonAsync<string>(cancellationToken: cts.Token);

c.ReadFromJsonAsync(typeof(string), options: null, cts.Token);
c.ReadFromJsonAsync(typeof(string), options: null, cancellationToken: cts.Token);
c.ReadFromJsonAsync<string>(options: null, cts.Token);
c.ReadFromJsonAsync<string>(options: null, cancellationToken: cts.Token);

// public static class Extensions
// {
// 	public static Task<object?> ReadFromJsonAsync(this HttpContent content, Type type, CancellationToken cancellationToken = default) => throw null;
//
// 	public static Task<T?> ReadFromJsonAsync<T>(this HttpContent content, CancellationToken cancellationToken = default) => throw null;
// }

Try uncommenting Extensions, and you'll see that methods are resolved to correct/expected overloads. Or are there some cases i missed that would cause errors, if we leave overloads JsonSerializerOptions with default value unchanged, but add new overloads?

[eiriktsarpalis]

wouldn't it be binary breaking change?

It isn't, the default value is metadata that is hardcoded by the callsite, see https://sharplab.io/#v2:C4LglgNgPgAgTARgLACgYGYAE9MGFMDeqmJ2WYAdsJgLIIAUl1AHpgLyYAscAlOwHyZmAbmKkMmJrTj0+bQXVmiUAXyA

In any case, this would make it fully consistent with the signatures used in HttpClientJsonExtensions.