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

[System.Net.Http.Json] Post method miss overloads for reading content #34157

Open
kant2002 opened this issue Mar 26, 2020 · 21 comments
Open

[System.Net.Http.Json] Post method miss overloads for reading content #34157

kant2002 opened this issue Mar 26, 2020 · 21 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Http
Milestone

Comments

@kant2002
Copy link
Contributor

kant2002 commented Mar 26, 2020

Previously I use

CreatePersonResponseModel response = await Http.PostJsonAsync<CreatePersonResponseModel>("/persons", this.model);

To get information from the server, and CreatePersonResponseModel was type of result. and this.model was object or so.

Now

HttpResponseMessage response = await Http.PostAsJsonAsync<CreatePersonResponseModel>("/persons", this.model);

And there no way to read JSON content. This seems to be just moving bits around, and make utility classes slightly useful

Is it possible make public

private static async Task<T> GetFromJsonAsyncCore<T>(Task<HttpResponseMessage> taskResponse, JsonSerializerOptions options, CancellationToken cancellationToken);

this allow have

HttpResponseMessage response = await Http.PostAsJsonAsync("/persons", this.model).GetFromJsonAsyncCore<CreatePersonResponseModel>();

or even better.

public static Task<TResponse> PostAsJsonAsync<TValue, TResponse>(this HttpClient client, Uri? requestUri, TValue value, JsonSerializerOptions? options = null, CancellationToken cancellationToken = default(CancellationToken))

/cc @SteveSandersonMS or who else might be interested in this.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Net.Http untriaged New issue has not been triaged by the area owner labels Mar 26, 2020
@davidsh
Copy link
Contributor

davidsh commented Mar 26, 2020

@karelz @jozkee

@stephentoub
Copy link
Member

And there no way to read JSON content.

CreatePersonResponseModel model = await response.Content.ReadFromJsonAsync<CreatePersonResponseModel>();

?

@kant2002
Copy link
Contributor Author

technically yes. But overall idea if I understand correctly is to GET/POST/PUT date in simple fashion. It was quite confusing migrate from Preview 2. Maybe worth mentioning in the upgrade guide if this is intended.

@jozkee
Copy link
Member

jozkee commented Mar 26, 2020

So the ask here is to make PostAsJsonAsync and PutAsJsonAsync methods to directly deserialize the HttpResponseMessage's content?

I think that when you send a Post request, the server does not necessarily will respond with a JSON so I don't know how common is that scenario.

Using the previous APIs (PostJsonAsync), what happens when the response's content-type is not application/json?

@rynowak @terrajobst

@davidsh
Copy link
Contributor

davidsh commented Mar 26, 2020

I think that when you send a Post request, the server does not necessarily will respond with a JSON so I don't know how common is that scenario.

Servers rely on HTTP request headers to decide what format to send back as the response. The 'Accept' request header will specify something like 'application/json' when it sends the request to the server. Then the server knows what is suitable to send back to the client.

@kant2002
Copy link
Contributor Author

For me this API appear half-backed.

Previous

  • Previous API was to communicate with HTTP API which use JSON as in/out format. Symmetric, easy to remember.
  • Assume no fancy content negotiation happens.
  • For mixed scenarios, was suggested go directly with HttpClient. This make sense. At least this was explicitly stated in public announcements.

Current

  • Assume that GET out data in JSON, but for POST/PUT only in data assumed to be JSON. Not symmetric in my opinion.
  • I can upload files, and most likely in data require customization in the in part, and not out.

All of that can be just out of confusion how to properly use API and for what scenarios new API intended.

@ericstj ericstj removed the untriaged New issue has not been triaged by the area owner label Mar 31, 2020
@ericstj ericstj added this to the 5.0 milestone Mar 31, 2020
@ericstj
Copy link
Member

ericstj commented Mar 31, 2020

I think that when you send a Post request, the server does not necessarily will respond with a JSON so I don't know how common is that scenario.

Using the previous APIs (PostJsonAsync), what happens when the response's content-type is not application/json?

I believe most of these convenience APIs assume the server will behave in a particular way and then throw when it doesn't. Same case could be true with a Get. If we can make these APIs be overloads (and not ambiguous) then it seems a reasonable request.

Why were they omitted? I remember discussion around this but can't recall the conclusion.

@scalablecory
Copy link
Contributor

Many (most?) JSON APIs support some sort of error feedback that is not directly encoded in a successful response's object model -- either using e.g. "404" response with no content, or still JSON but with a different object model.

We discussed this briefly in API review and decided to leave it for future design. I think the challenge is to identify strong use cases for this.

@ericstj
Copy link
Member

ericstj commented Apr 1, 2020

Many (most?) JSON APIs support some sort of error feedback that is not directly encoded in a successful response's object model -- either using e.g. "404" response with no content, or still JSON but with a different object model.

Doesn't that same concern apply to all Get methods we already expose?

@scalablecory
Copy link
Contributor

Doesn't that same concern apply to all Get methods we already expose?

Yes. I would not recommend using them outside of very trivial APIs or in toy code.

99% of the time I'd use something that captures the data those methods throw away in the non-success paths:

async Task<SuccessResponseObject?> ProcessResponse(HttpResponseMessage response)
{
    if(response.IsSuccessStatusCode)
    {
        return await response.Content.ReadAsAsync<SuccessResponseObject>();
    }

    if(response.StatusCode == 404)
    {
        return null;
    }

    if(response.StatusCode == Some500StatusWellDefinedByAPI)
    {
        // might contain more specific error messages, a model dictionary of validation errors, etc.
        var errorInfo = await response.Content.ReadAsAsync<ErrorResponseObject>();
        throw CreateExceptionFromErrorInfo(errorInfo);
    }

    throw CreateUnknownError(response.StatusCode);
}

@ericstj
Copy link
Member

ericstj commented Apr 1, 2020

So if the same concern applies to Get, and we have Get methods, then we can't apply that concern as a reason not to have the Post methods, right? Or am I missing something?

@rynowak
Copy link
Member

rynowak commented Apr 1, 2020

A few thoughts from my POV...


Ultimately the proposal that was implemented was a combination of two things:

  • The functionality available as part of the WebAPI client (using Newtonsoft.Json)
  • The set of functionality that was in Blazor that we felt like could be easily agreed that it belongs in in a System. package

This feedback seems to be about something that was in Blazor that wasn't included in the proposal.


So why didn't we include this functionality in the proposal for dotnet/runtime? Well simply put we wanted to avoid asking for too many options/opinions, because all of this functionality is additive.

Example: right now you can write the thing being asked for as:

var response = await Http.PostAsJsonAsync<CreatePersonResponseModel>("/persons", this.model);
response.EnsureSuccessStatusCode();
var person = await response.Content.ReadAsJsonAsync<CreatePersonResponseModel>();

We could decide to add an accelerator for this in the future, but it would be additive:

var person = await Http.PostAsJsonAsync<CreatePersonResponseModel, CreatePersonResponseModel>("/persons", this.model); // TRequest, TResponse are the same in this case

I didn't propose this, because adding just the API listed in the example of above would feel incomplete. In short, we have added APIs that read, and APIs that write, but we didn't propose APIs that both read and write in a single method call.

You can add them, but there are more overloads/flavors/opinions that are necessary to do so.

Regardless of what accelerators we think need to exist, we should definitely document the basic recipes in detail. Users will always have use-cases that fall out of what we provide convenience APIs for.


The APIs that were proposed are really simple to describe and understand - let's look at fleshing this out more and see which ones seem like they are easy to use, describe and understand vs writing the 3 lines of code this takes without them. I'm going to describe all of these with POST. PUT is equally applicable.

There's a few classes of methods that could exist - classified by what they serialize, what they deserialize, and what they do with errors:

  • Is there a serialized request payload or not? (TRequest)
  • Is there a deserialized response payload or not? (TResponse) (in some cases its the same as TRequest)
  • What do you do with errors? Do you throw? Do you attempt to read some different data type? (in case you're wondering, ASP.NET Core provides a standard error type that will be returned from most 400 responses).

Here's a table and some notes:

TRequest TResponse TError Notes
Yes No (void) No This is satisfied by the existing accelerator.
Yes No (void) Yes It's somewhat rare that an API would return a value on failure but not on success
Yes Yes (same as request) No This is a common case for "Create" APIs with ASP.NET Core that will return 201/202 and a response object.
Yes Yes (same as request) Yes This is a common case for "Create" APIs with ASP.NET Core that will return 201/202 and a response object.
Yes Yes (different from request) No This is a common case for general use.
Yes Yes (different from request) Yes This is a common case for general use.
No No (void) No Since there is no serialization, there's no need for an accelerator.
No No (void) Yes It's somewhat bizzare that an API would return a value on failure but not on success
No Yes No It's pretty rare to have an "empty" post in a REST API.
No Yes Yes It's pretty rare to have an "empty" post in a REST API.

So if we looked at the things that I think are pretty common in that table the API additions would look like (ignoring permutations like string vs Uri, JsonSerializerOptions, and CancelationTokens:

// Methods that throw when an error is encountered.
public static Task<TResponse> PostAsJsonAsync<TRequest, TResponse>(this HttpClient client, string uri, TRequest request);
public static Task<TResponse> PostAsJsonAsync<TResponse>(this HttpClient client, string uri);

// Methods that attempt to deserialize an error.
// All of these really want discriminated unions.
public static Task<(TResponse, TError>) PostAsJsonAsync<TRequest, TResponse, TError>(this HttpClient client, string uri, TRequest request);
public static Task<(TResponse, TError)> PostAsJsonAsync<TResponse, TError>(this HttpClient client, string uri);

Does functionality like this get classified as too opinionated? Similar to the code that @scalablecory provided:

- Send an HTTP POST Request
- Serialize this `TRequest` object into the message body
- If the response is a 2XX and JSON then:
  - Read the response body as a `TResponse`
- If the response is another status code and JSON then:
  - Read the response body as a `TError`
- Else throw

This assuming that I'm right about what's common and what's not. I did not add a method for the TRequest == TResponse case because that can be handled by the method with two generic parameters - adding it causes a conflict with the currently defined (and most general) method.

One way that I've seen this resolved in some other libraries (Microsoft.Rest) is that they use different method names for the ones that return HttpResponseMessage-likes, so that there's no ambiguity about who's responsible for serialization.

@ericstj
Copy link
Member

ericstj commented Apr 8, 2020

@jozkee @terrajobst can you please have a look here and consider @rynowak's analysis? This is precisely the type of feedback we're looking to consider with our preview of this package.

@rynowak
Copy link
Member

rynowak commented Apr 8, 2020

If we want to talk about this more, I'm game. I also want to amend something I said (based on re-reading).

It's somewhat bizzare that an API would return a value on failure but not on success

I think I was wrong about this. Consider the case of a DELETE. I think there are probably plenty of cases where you return nothing on success, but have the potential to return an error object on failure.

@jozkee
Copy link
Member

jozkee commented Apr 8, 2020

As mentioned in #32937 (comment), we might not need TError on methods that does not directly return a HttpResponsemessage, instead we should throw on responses that are not 2xx; that is how GetFromJsonAsync works.

@jozkee
Copy link
Member

jozkee commented Apr 8, 2020

As @rynowak suggested, this could be shaped as the following (extended to the remaining overloads):

public static Task<TResponse> PostAsJsonAsync<TRequest, TResponse>(this HttpClient client, string? requestUri, TRequest value, JsonSerializerOptions? options = null, CancellationToken cancellationToken = default);
public static Task<TResponse> PostAsJsonAsync<TRequest, TResponse>(this HttpClient client, string? requestUri, TRequest value, CancellationToken cancellationToken);
public static Task<TResponse> PostAsJsonAsync<TRequest, TResponse>(this HttpClient client, Uri? requestUri, TRequest value, JsonSerializerOptions? options = null, CancellationToken cancellationToken = default);
public static Task<TResponse> PostAsJsonAsync<TRequest, TResponse>(this HttpClient client, Uri? requestUri, TRequest value, CancellationToken cancellationToken);

The implementation is probably going to look something like this:

public static Task<TResponse> PostAsJsonAsync<TRequest, TResponse>(this HttpClient client, string? requestUri, TRequest value, JsonSerializerOptions? options = null, CancellationToken cancellationToken = default)
{
    return PostAsJsonAsyncCore<TRequest, TResponse>(client, requestUri, value, options, cancellationToken);
}

private static async Task<TResponse> PostAsJsonAsyncCore<TRequest, TResponse>(HttpClient client, string? requestUri, TRequest value, JsonSerializerOptions? options, CancellationToken cancellationToken)
{
    using (HttpResponseMessage response = await client.PostAsJsonAsync(requestUri, value, options, cancellationToken))
    {
        response.EnsureSuccessStatusCode();
        return await response.Content.ReadFromJsonAsync<TResponse>();
    }
}
  • We will have to add the equivalet set of signatures for Put methods
  • We will need to rename TValue to TRequest in the existing Post and Put methods
  • and rename to TResponse in the Get methods.

All of this assuming that we agree on adding this set of APIs.

@jozkee
Copy link
Member

jozkee commented Apr 8, 2020

For what is worth, having this set of APIs could also provide an alternative for users from having to dispose the HttpResponseMessage themselves. #33459 (comment)

@jozkee jozkee added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Apr 9, 2020
@layomia layomia changed the title System.Net.Http.Json Post method miss overloads for reading content [Json] System.Net.Http.Json Post method miss overloads for reading content May 6, 2020
@jozkee jozkee modified the milestones: 5.0.0, Future Jun 25, 2020
@ghost
Copy link

ghost commented Sep 26, 2020

Yep, API is totally half baked. Look at .NET ServiceStack or any normal, popular Javascript HTTP library. They all will accept a request object that is completely different from the response object. For a GET request, the request object is serialized in a common format on the url string, again ServiceStack, Angular, etc. all work this way and all support it. For a POST, it is very common for the returned data schema to be different than the request schema, which means two completely separate .NET POCOs for the request and response. I would totally expect an extension method from System.Net.Http.Json for Post as:

HttpClient.PostAsJsonAsync<TResponse, TRequest>(...)

but no such luck. Please add this ASAP, as we are having to add extension methods for the half baked solution of System.Net.Http.Json.

@SteveSandersonMS
Copy link
Member

@velocitytoo You can send and receive different types:

var response = await Http.PostAsJsonAsync("some/url", myOutgoingValueOfAnyType);
var deserializedResult = response.Content.ReadFromJsonAsync<SomeOtherType>();

@karelz karelz changed the title [Json] System.Net.Http.Json Post method miss overloads for reading content [System.Net.Http.Json] Post method miss overloads for reading content Jan 5, 2021
@jozkee jozkee removed their assignment Mar 8, 2023
@aradalvand
Copy link

aradalvand commented Oct 7, 2023

I agree that this should be added; I was surprised to find out that no such overload exists already.

@WeihanLi
Copy link
Contributor

WeihanLi commented May 20, 2024

+1, think this would be greatly useful when using typed JSON request and response

and please also add support for PUT and PATCH when considering POST

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Http
Projects
None yet
Development

No branches or pull requests