-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Comments
CreatePersonResponseModel model = await response.Content.ReadFromJsonAsync<CreatePersonResponseModel>(); ? |
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. |
So the ask here is to make 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 ( |
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. |
For me this API appear half-backed. Previous
Current
All of that can be just out of confusion how to properly use API and for what scenarios new API intended. |
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. |
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. |
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);
} |
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? |
A few thoughts from my POV... Ultimately the proposal that was implemented was a combination of two things:
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 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 There's a few classes of methods that could exist - classified by what they serialize, what they deserialize, and what they do with errors:
Here's a table and some notes:
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 // 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 One way that I've seen this resolved in some other libraries ( |
@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. |
If we want to talk about this more, I'm game. I also want to amend something I said (based on re-reading).
I think I was wrong about this. Consider the case of a |
As mentioned in #32937 (comment), we might not need |
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>();
}
}
All of this assuming that we agree on adding this set of APIs. |
For what is worth, having this set of APIs could also provide an alternative for users from having to dispose the |
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. |
@velocitytoo You can send and receive different types: var response = await Http.PostAsJsonAsync("some/url", myOutgoingValueOfAnyType);
var deserializedResult = response.Content.ReadFromJsonAsync<SomeOtherType>(); |
I agree that this should be added; I was surprised to find out that no such overload exists already. |
+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 |
Previously I use
To get information from the server, and
CreatePersonResponseModel
was type of result. andthis.model
wasobject
or so.Now
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
this allow have
or even better.
/cc @SteveSandersonMS or who else might be interested in this.
The text was updated successfully, but these errors were encountered: