-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Merge 2xx responses with the same type and handle them in one block #1259
Comments
If 204 (No content) is in the possible answers, just make the result nullable. The point is : 2xx status codes should not throw in the client, because they semantically indicate success. What would be the behavior if two different types are declared, e.g. for 200 and 201 ?
|
To avoid bigger breaking changes, I'd suggest to only merge the responses which do not require a changed return type otherwise generate the same code... for generating wrappers etc., we have additional settings (e.g. WrapResponses, etc.) |
My team was caught off guard to see a 2xx response, in our case a 204, throw an exception, especially with a message in the exception of "Success", which is kind of a bizarre thing by itself. I would consider this more of a bug than an enhancement (how it's currently tagged). We are totally fine with different blocks for each status code even with the same response type. Merging the code blocks is an optimization that I wouldn't hitch to a solution for this, especially with the complexity it would add. Is anyone working on this? |
Probably not |
I'll see if I can help out! |
I was surprised to see that a 201 throws an exception on a post operation since it is a positive response code. |
Just hit this by inspecting the generated code. Why is the default behaviour to throw if not 200 or 204. Seems a very odd decision. I would personally prefer if there was an option such as |
The fact that this is tagged enhancement is really bizarre to me. To me this is a bug with the library. I understand it'd cause backward compatibility issues, but a code generation flag, as was suggested earlier seems like a very reasonable solution. The fact that we need to handle this "normally" with a try/catch is very frustrating to me. |
The idea is to hide HTTP specific stuff (avoid leaky abstractions). But i agree that 2xx should not throw but either return a result (200) or null (2xx) - if they dont have another model.. ie merge if possible. Would this be ok? |
Hi @RicoSuter any ideas implementation of this feature? When will this be available? |
Hi, Anything new related to this issue? |
Can we expect the pr from @Trentm95 to get merged anywhere in the near future? |
@RicoSuter, what is the current state of this issue? Do you need help with merging, or is there some other issue I'm not seeing? |
I probably broke the PR by refactoring the status code handling. We need to rewrite it with the new status code handling |
Related to #2995 because both should be fixed at the same time : if a 204 is declared (or any empty response), the generated response type should be changed to |
Referencing item #1602, would be great to cater for the scenario mentioned there. |
@nealsu :please don't cross-post. issues are already linked and they will be fixed all at once |
I talked on gitter this evening with @RicoSuter and here is what we decided: We need to merge the success (2xx) responses properly, to be able to fix issues like #1602 . But we need to be careful, because it will likely break things. Here is what was defined for the result merging:
If several different types are declared as success, we need to resolve the conflict. It doesn't seem to me that the conflict will occur in the typescript generator, because of the language's flexibility. What follows only applies to the C# client generator.
I'm in favor of the first option. Any opinion welcome. |
My point wasn't about the usefulness of the 204 by itself, it's about the API being able to return both 200 and 204. |
Whether to return 204 or 404 depends on the use case. I don't get why it is so difficult to return a nullable in the client, when the server returns one. The api falls behind basic c# functionality here. I don't care about the 204 status itself, if nullable data types would work, like they do in c#. |
The OpenAPI spec contains two response types, one for 200 (with the return type), one with 204 (without body). Handling 200/204 would not be easily doable in the template, so it would need to be handled before, in the generator, and a new propery must be added to handle the 200/204 separately. By doing so, you're breaking the whole code base where it assumes there is a list of return types and probably other case I didn't see. The NSwag code base has been built incrementally, and now it's hard to make any change to it without refactoring everything, but feel free to try and make a PR. |
Hey all, I read this discussion and i understand that this is a difficult one. Is there maybe a work around to support a 200 with a response and a 204 with no content for now?
|
Also encountered an unexpected exception for an 204 status code. Would appreciate a solution or work around for this. |
Im having to wrap my object in a |
@vindberg your comment comes across as rude and ignorant. Microsoft should not cannibalize oss projects: https://aaronstannard.com/new-rules-dotnet-oss/ |
@Jogai Im sorry for writing with such direct language. I know that Rico is doing a great job on the project - and thanks for that. The outstanding issues with NSwag could be fixed with some help from MS (just funding). |
This is still a problem. Anybody fixing it soon? |
Yep, still a problem. Can't find a workaround and have to redesign an endpoint so it would return some wrapper object instead of null. Feels like a crutch |
@asevos I proxy the generated client then put a try catch around like below - then I can catch the status code and return null:
This also sucks - but being between a rock and a hard place id rather this over the wrapper obj response |
I like the idea of this suggestion given out-of-box defaults currently break with APIs that return null that expect clients to deserialize the response to a null value (we had one converted from the older Web API platform before .NET Core). Given ASP.NET Core has a Currently we're removing that formatter to return a 200 with a serialized |
The fact that this is still a problem in 2023, half a decade after it was first raised, blows my mind. This is incredibly basic functionality that should've been the team's absolute highest priority to be implemented. The fact that such fundamentals remain missing is why our team is strongly looking at writing our own HTTP client generator, and likely why Microsoft is doing the same. |
Great, @IanKemp , please don't forget to open source your implementation. |
If your response to someone pointing out that not addressing an open issue in half a decade is a problem, is to attack that person, you need to reconsider your life choices. |
But who should address an open issue such like this? The person needing the feature or someone from the OSS project that tries to balance their free time? I could imagine that if somebody considering how to use their free time, they might not share the same priorities like someone ordering features. Their highest priority might be health (mental or physical), their family or maybe some other hobby. I'm quite sure that like many other OSS projects there might be possibility to get a refund if you're not getting your money's worth.
|
I think that the one that is complaining in such a manner about the issue in OS project should definitely reconsider HIS attitude and life choices. |
Five years without solution, must be real rocket science to fix... How about introducing new optional .nswag configuration option, something like 'AllowedSuccessStatusCodes' containing comma-separated list. By default it's just '200'. If the option is not defined in .nswag file the client generation engine also treats it as '200'. If someone wants to extend it with other status codes (eg 204), he just adds it to the values list - '200,204' and it is reflected in |
When will your PR be ready to review? |
I could fix code generation issue for 204 response with the following operation filter:
The action looks like this:
The operation filter is used this way:
|
Uh, no, this doesn't work. NSwag still generates |
Hi @IanKemp, Could you provide your code snipet. This works for me. Thanks, |
Totally worked for me. Thanks alot @fuliii1 |
I also noticed just by adding this annotation to my Controller endpoint method, the issue is resolve in the generated ts & cs files. |
In .net core 8.0 I added |
This is all fine as long as the spec provides all necessary information and you are in control of the spec. If the foreign API is not correctly documented, the generator will rightfully throw an error because the response is unexpected. IMHO blaming the generator for acting wrong on incomplete specifications is nonsensical. I wish, though, the generator would allow for a "ignore-all-except-success" parameter. In any case, although a little bit tacky or hacky, this is the "band aid" I use now together with "Wrap success responses": public static bool IsHttpSuccessCode(this int statusCode) => statusCode is >= 200 and <= 299;
/// <summary>
/// Retrieves the typed result from the API response without throwing exceptions on success codes.
/// This method is necessary because the API client will throw an exception if the API responds with other success status codes than 200 because the API spec does not include all possible return status codes.
/// </summary>
/// <typeparam name="T">The type of the result.</typeparam>
/// <param name="responseTask">The API response.</param>
/// <returns>The typed result.</returns>
/// <exception cref="ApiException">Emits exceptions when the response status code indicates no success</exception>
public static async Task<T> GetTypedResult<T>(this Task<ApiResponse<T>> responseTask)
{
try
{
var response = await responseTask;
if (response.StatusCode.IsHttpSuccessCode())
{
return response.Result;
}
throw new ApiException("API call was not successful", response.StatusCode, null, response.Headers, null);
}
catch (ApiException e) when (e.StatusCode.IsHttpSuccessCode())
{
return default;
}
catch (ApiException e)
{
throw new ApiException("API call was not successful", e.StatusCode, e.Response, e.Headers,
e.InnerException);
}
} Still, sensical error handling is mandatory when performing calls. |
I'm wondering how people say, that adding the 204 as response code will make it magically work. When can we expect this bug to be fixed? |
merge 2xx responses with the same type and handle them in one if (status... || block
The text was updated successfully, but these errors were encountered: