Add extension methods for HttpClient that allow serialization from/to JSON

[terrajobst]

The spec is here:

https://github.com/dotnet/designs/blob/main/accepted/2020/json-http-extensions/json-http-extentions.md

Related issues:

• Add System.Net.Http.JsonContent for System.Net.Http.HttpClient #1754
• HttpClientJsonExtensions should be able to notify callers about responses from the server that can't be serialized to type T #31550
• Blazor - Add JsonSerializerOptions as parameter to HttpClient.GetJsonAsync<T>  #31551

[karelz]

@terrajobst you linked 2x the same issue. Does it have to go through API review first?

[davidsh]

From: https://github.com/dotnet/designs/blob/master/accepted/2020/json-http-extensions/json-http-extentions.md

Serializing and deserializing JSON payloads from the network is a very common operation for clients, especially in the upcoming Blazor environment. Right now, sending a JSON payload to the server requires multiple lines of code, which will be a major speed bump for those customers. We'd like to add extension methods on top of HttpClient that allows doing those operations with a single method call.

Why add these as extension methods to HttpClient? Why not just add them as regular methods to HttpClient?

[terrajobst]

@karelz

you linked 2x the same issue.

Clipboard is hard man! Fixed.

Does it have to go through API review first?

We did a brief run in a smaller group; at this point we should go ahead with the implementation first to see what issues we encounter and do the API review after with the complete API set and all modifications we made since then.

[terrajobst]

@davidsh

Why add these as extension methods to HttpClient? Why not just add them as regular methods to HttpClient?

Good question, this was discussed. Basically it goes like this: do we want HttpClient to depend on the JSON serializer? I think the answer is no; serializers come and go, much more than networking APIs. We concluded these should sit on top. This also allows customers to use either this one or the existing ASP.NET formatters which will use JSON.NET.

[terrajobst]

@karelz

I see you set the milestone to 5.0. Please note that we need to ship a preview by Build; it looks like we don't have a milestone for that. Should we?

[karelz]

@terrajobst unless we have more than 5-10 issues tracking as must have for Build, I don't think it makes sense to create milestone/label.
And once we hit the threshold, let's discuss what makes most sense ... (I feel that a label will be more flexible, but I may be wrong).

[scalablecory]

JsonContent.Create(...) usage is unlike other HttpContent we have. We should see if these could reasonably be changed to new JsonContent(...).

The verbs are also inconsistent with current naming. Something to consider:

• HttpClient.GetFromJsonAsync vs HttpClient.GetStringAsync
• HttpContent.ReadFromJsonAsync vs HttpContent.ReadAsStringAsync

Otherwise I think these APIs look good. I used the Formatting assembly extensively in a past life and would have had no problems migrating to this.

[mariusGundersen]

I want to propose having these methods throw an exception when the response status code is not 2xx, and to include the HttpResponse in that exception, so that it can be used to pattern match the error and potentially deserialize the validation mesages, for example:

            try
            {
                return await client.GetJsonAsync<IReadOnlyCollection<Person>>("/people");
            }
            catch (HttpStatusCodeException e) when (e.StatusCode > 500)
            {
                //something bad happened at the server
                Log(e);
                throw;
            }
            catch (HttpStatusCodeException e) when (e.StatusCode > 400)
            {
                //something was wrong in the request
                var validationMessage = await e.HttpResponse.Content.ReadAsJsonAsync<ValidationResult>();
                BindValidationMesage(validationMessage);
                return null;
            }
            catch (Exception e)
            {
                Log(e);
                throw;
            }

[scalablecory]

I want to propose having these methods throw an exception when the response status code is not 2xx

I would expect the convenience methods (the ones not returning HttpResponseMessage) to do exactly that.

and to include the HttpResponse in that exception

HttpRequestException will include a StatusCode property in .NET 5. We will not be including the full response, as having an IDisposable inside an exception is just asking for trouble.

[terrajobst]

Because I'm an idiot, I filed a new issue for today's API review (#33566 (comment)) instead of using this. I've marked this as API approved. I'll close the review one in favor of this.

[IanKemp]

JsonContent.Create(...) usage is unlike other HttpContent we have. We should see if these could reasonably be changed to new JsonContent(...).

The verbs are also inconsistent with current naming. Something to consider:

* `HttpClient.GetFromJsonAsync` vs `HttpClient.GetStringAsync`

* `HttpContent.ReadFromJsonAsync` vs `HttpContent.ReadAsStringAsync`

Seems like these comments weren't addressed even though this PR was merged?

[jozkee]

@IanKemp for JsonContent.Create, it was discussed during API review.
See #33566 (comment).

We should make the constructors internal and only have the factory methods. This avoids confusion where people never know that there are generic versions.