Merge 2xx responses with the same type and handle them in one block

[RicoSuter]

merge 2xx responses with the same type and handle them in one if (status... || block

[jeremyVignelles]

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 ?

• Will code generation for C# crash, since typescript can handle that?
• Will that automatically create a wrapper?

[RicoSuter]

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.)

[Blackbaud-MitchellThomas]

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?

[RicoSuter]

Is anyone working on this?

Probably not

[Blackbaud-MitchellThomas]

I'll see if I can help out!

[dutchmartin]

I was surprised to see that a 201 throws an exception on a post operation since it is a positive response code.
https://httpstatuses.com/

[jchannon]

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 ThrowExceptions that hides this default feature. As a client I don't think I should be expecting an exception, it should be my responsibility to check the status code and process that how I want.

[kensykora]

The fact that this is tagged enhancement is really bizarre to me. 202 Accepted is super common in rest patterns for long running requests.

See https://github.com/microsoft/api-guidelines/blob/vNext/Guidelines.md#132-stepwise-long-running-operations

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.

[RicoSuter]

As a client I don't think I should be expecting an exception, it should be my responsibility to check the status code and process that how I want.

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?

[xaviergxf]

Hi @RicoSuter any ideas implementation of this feature? When will this be available?

[ken2k]

Hi,

Anything new related to this issue?

[Jogai]

Can we expect the pr from @Trentm95 to get merged anywhere in the near future?

[Reintjuu]

@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?

[jeremyVignelles]

I probably broke the PR by refactoring the status code handling. We need to rewrite it with the new status code handling

[jeremyVignelles]

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 T? (adds nullability to the response type), unless it is already void.

[nealsu]

Referencing item #1602, would be great to cater for the scenario mentioned there.

[jeremyVignelles]

@nealsu :please don't cross-post. issues are already linked and they will be fixed all at once

[jeremyVignelles]

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:

• Only applicable if WrapResponse is not used (no need for a merge in that case)
• Can merge one or more success responses if they have the same response type
• If one of the declared success responses has no type, it can merge with the result, but that makes the result nullable
• If one of the declared success responses has a different type than the others, an exception is thrown in the C# generator (still discussing, see below)
• Consequently, all the success cases are handled under the same if, the only one that has a return
• It is worth a major version bump, to signal it's a breaking change.

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.

• Either we throw if such conflict is encountered, crashing the generation. The message would suggest to use WrapResponse to be able to return different types and that would allow to accomodate funny specs.
• Or we decide to generate a valid code anyway. Some success responses would be treated as returns, and others would throw a SwaggerException. Then question arises like "How do we decide which result is a return?" "Is that logical?" "Why does my 201 throws? You didn't fix anything!" etc...

I'm in favor of the first option.

Any opinion welcome.

[Jogai]

@vindberg your comment comes across as rude and ignorant. Microsoft should not cannibalize oss projects: https://aaronstannard.com/new-rules-dotnet-oss/

[vindberg]

@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).

[ardalis]

This is still a problem. Anybody fixing it soon?

[asevos]

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

[msharp-hk-dev-fdt]

@asevos I proxy the generated client then put a try catch around like below - then I can catch the status code and return null:

public class ProxyXyz : IXyz {
     public override  async Task<Abc> GetSomethingThatAlsoReturnsNoContentAsync(int assetMasterId,  CancellationToken cancellationToken = default(CancellationToken))
    {
        try
        {
            // Call base method (but in try catch)
            return await base.GetSomethingThatAlsoReturnsNoContentAsync(assetMasterId, cancellationToken);
        }
        catch (AbcApiException ex)
        {
            // Here we want null NOT an error
            if (ex.StatusCode == (int)HttpStatusCode.NoContent)
                return null;

            // Continue to propagate the initial error if not matching our status code
            else throw ex;
        }
    }

This also sucks - but being between a rock and a hard place id rather this over the wrapper obj response

[TheXenocide]

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 HttpNoContentOutputFormatter which is enabled by default, might it be appropriate to look for that on nullable return types when generating the Open API Specification document to determine if a 204 should be expected for null responses?

Currently we're removing that formatter to return a 200 with a serialized null in the response body, which is how things worked before, but in many cases it makes sense that it could be treated as a 204 (or a 404) that isn't considered an exception by the client. I think more control of this client-side functionality will help APIs and their associated clients be more pure in their RESTfulness so +1 from me.

[IanKemp]

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.

[jvmlet]

Great, @IanKemp , please don't forget to open source your implementation.
I suppose such contribution should be your highest priority given the fact that you are enjoying nswag for half a decade....

[IanKemp]

Great, @IanKemp , please don't forget to open source your implementation. I suppose such contribution should be your highest priority given the fact that you are enjoying nswag for half a decade....

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.

[lahma]

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.

RicoSuter added the help wanted label on Sep 7, 2018

[jvmlet]

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.

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.

[Eugene-Vector]

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 if (status_ == ...) condition.
In this way no breaking changes would be introduced.

[ardalis]

When will your PR be ready to review?

[fuliii1]

I could fix code generation issue for 204 response with the following operation filter:

public class NullableResponseOperationFilter : IOperationFilter
{
    /// <inheritdoc />
    public void Apply(OpenApiOperation operation, OperationFilterContext context)
    {
        foreach (var response in operation.Responses.Where(response => response.Key == "204"))
        {
            foreach (var content in response.Value.Content)
            {
                content.Value.Schema.Nullable = true;
            }
        }
    }
}

The action looks like this:

[HttpPost("{id:long}/action/{orderAction:orderAction}")]
[Produces(Constants.MimeTypes.Pdf, Type = typeof(FileStreamResult))]
[ProducesResponseType(typeof(FileStreamResult), StatusCodes.Status204NoContent)]
[ProducesResponseType(typeof(FileStreamResult), StatusCodes.Status200OK)]
public async Task<IActionResult> ExecuteActionAsync(int id, Models.Orders.Action orderAction)
{
    ...
}

The operation filter is used this way:

builder.Services.AddSwaggerGen(options =>
{
    options.OperationFilter<SwaggerDefaultValuesSessionIdParameter>();
});

[IanKemp]

I could fix code generation issue for 204 response with the following operation filter:

public class NullableResponseOperationFilter : IOperationFilter
{
    /// <inheritdoc />
    public void Apply(OpenApiOperation operation, OperationFilterContext context)
    {
        foreach (var response in operation.Responses.Where(response => response.Key == "204"))
        {
            foreach (var content in response.Value.Content)
            {
                content.Value.Schema.Nullable = true;
            }
        }
    }
}

The action looks like this:

[HttpPost("{id:long}/action/{orderAction:orderAction}")]
[Produces(Constants.MimeTypes.Pdf, Type = typeof(FileStreamResult))]
[ProducesResponseType(typeof(FileStreamResult), StatusCodes.Status204NoContent)]
[ProducesResponseType(typeof(FileStreamResult), StatusCodes.Status200OK)]
public async Task<IActionResult> ExecuteActionAsync(int id, Models.Orders.Action orderAction)
{
    ...
}

The operation filter is used this way:

builder.Services.AddSwaggerGen(options =>
{
    options.OperationFilter<SwaggerDefaultValuesSessionIdParameter>();
});

Uh, no, this doesn't work. NSwag still generates if response is 204 and null, then throw exception code.

[fuliii1]

Hi @IanKemp,

Could you provide your code snipet. This works for me.

Thanks,

[amithkumarg]

I could fix code generation issue for 204 response with the following operation filter:

public class NullableResponseOperationFilter : IOperationFilter
{
    /// <inheritdoc />
    public void Apply(OpenApiOperation operation, OperationFilterContext context)
    {
        foreach (var response in operation.Responses.Where(response => response.Key == "204"))
        {
            foreach (var content in response.Value.Content)
            {
                content.Value.Schema.Nullable = true;
            }
        }
    }
}

The action looks like this:

[HttpPost("{id:long}/action/{orderAction:orderAction}")]
[Produces(Constants.MimeTypes.Pdf, Type = typeof(FileStreamResult))]
[ProducesResponseType(typeof(FileStreamResult), StatusCodes.Status204NoContent)]
[ProducesResponseType(typeof(FileStreamResult), StatusCodes.Status200OK)]
public async Task<IActionResult> ExecuteActionAsync(int id, Models.Orders.Action orderAction)
{
    ...
}

The operation filter is used this way:

builder.Services.AddSwaggerGen(options =>
{
    options.OperationFilter<SwaggerDefaultValuesSessionIdParameter>();
});

Totally worked for me. Thanks alot @fuliii1

[amithkumarg]

I also noticed just by adding this annotation to my Controller endpoint method, the issue is resolve in the generated ts & cs files.
[SwaggerResponse(HttpStatusCode.NoContent, typeof(void))]

[achansonjr]

In .net core 8.0 I added [ProducesResponseType(statusCode: (int)HttpStatusCode.NoContent)] on my put controller, and now the generated code is handling 204 as if it was a valid response and no longer throws an exception.

[Neuroquila-n8fall]

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.
To work around this issue I opted for an extension method which wraps the possible exceptions, re-evaluates them and passes down the exception if desired.

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.

[markusahrweileramcon]

I'm wondering how people say, that adding the 204 as response code will make it magically work.
We create a js client from the generated swagger file
nswag openapi2tsclient /input:swagger.json /output:src/lib/client.ts /TypeScriptVersion:5 /Template:Angular /HttpClass:HttpClient /UseSingletonProvider:true /RxJsVersion:7.8 /NullValue:Null /InjectionTokenType:InjectionToken
and it does not work.
The swagger file contains a 201, 204 and 404. The 204 has no content, as expected.

When can we expect this bug to be fixed?

[AnReZa]

Any news on this topic? I'm affected as well. Is there any workaround? Can we somehow modify the way the 204 implementation looks like by modifying some template file?

[markusahrweileramcon]

Any news on this topic? I'm affected as well. Is there any workaround? Can we somehow modify the way the 204 implementation looks like by modifying some template file?

Afaik no. We made a fork and editet the file src/NSwag.CodeGeneration/Models/ResponseModelBase.cs and removed the condition in IsSuccess for type checking. So, we only delegate to the HttpUtilities.
This works for us

[AnReZa]

@markusahrweileramcon Is this fork available somewhere so that it can be used from within Visual Studio?

[markusahrweileramcon]

https://github.com/AMCON-GmbH/NSwag/tree/feature/treat-every-200-as-success

[AnReZa]

@markusahrweileramcon Hey, thank you very much. Any idea how to replace the NSwag that is used by the Visual Studio code generator?

[markusahrweileramcon]

@AnReZa sorry, no. We only use the npm package which is also the only package i can confirm works, but I would guess, that any other package would use the same code.

If you're lucky, VSC uses maybe an installed package for that, so you "only" have to host a nuget package somewhere

[AnReZa]

@markusahrweileramcon It seems like it. I can update the NuGet package and it updates the version numbers in the generated code.

[VusalDev]

6 years of waiting :(