-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
[API Proposal]: Add new overloads to ReadFromJsonAsync method in HttpContentJsonExtensions #72103
Comments
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue DetailsBackground and motivationCurrently
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 Proposalnamespace 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
API Usagepublic 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 DesignsCan't think of any. RisksCan't think of any, since it follows the same pattern present as in other class.
|
Sounds reasonable from a consistency standpoint, since |
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 |
return await message.Content.ReadAsJsonAsync(cancellationToken: cancellationToken); Is good workaround, i didn't think of at the time of writing a proposal. However,
to name a few, there is more at the source file. So i am not sure what should be done here, i assume proposal should be marked as |
@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. |
I missed that API review meeting, but it looks like the fact that the same overloads already exist in |
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; }
} |
@N0D4N would you be interested in contributing an implementation? |
@eiriktsarpalis sure! |
Wait, maybe i miss something, |
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. |
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 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 |
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 |
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
runtime/src/libraries/System.Net.Http.Json/ref/System.Net.Http.Json.cs
Lines 85 to 89 in a5f3676
Which makes impossible calling
ReadFromJsonAsync
method without specifyingJsonSerializerOptions
/JsonSerializerContext
/JsonTypeInfo<T>
parameters, but with passingCancellationToken
parameter. Prior to S.T.J source-gen it was possible via calling for examplecontent.ReadFromJsonAsync<MyClass>(null, cancellationToken)
. However with introducing json source-gen now this is impossible since compiler can't deduce type behindnull
passed as param, sinceJsonSerializerOptions
overload was the only one, and now it isn't. So now user needs to specify parameter of null like thiscontent.ReadFromJsonAsync<MyClass>((JsonSerializerOptions?)null, cancellationToken)
which is additional typing, and doesn't look pleasant.API Proposal
It should also be noted that
HttpClientJsonExtensions
methods overloads already follow the same pattern as proposed of skipping nullableJsonSerializerOptions
parameter:runtime/src/libraries/System.Net.Http.Json/ref/System.Net.Http.Json.cs
Line 25 in a5f3676
API Usage
Alternative Designs
Can't think of any.
Risks
Can't think of any, since it follows the same pattern present as in other class.
The text was updated successfully, but these errors were encountered: