Nswag: Known Response Types throw Exception with wrapped responses

[graemechristie]

The current implementation of Client generator will throw exceptions when the response code is not 2XX, even when the wrapResponses option is set to true. It is perfectly valid and not exceptional for a swagger api to return a non 2xx response code.

The canonical example is returning a 404 code, when a resource is not found. This is the behaviour of most of our Api's when a user requests a specific resource that is not found.

e.g. the swagger document will contain:

    "responses": {
      "200": {
        "description": "Success",
        "schema": { "$ref": "#/definitions/Transaction" }
      },
      "404": {
        "description": "Not Found",
        "schema": { "$ref": "#/definitions/Transaction" }
      }

Having to wrap every request in a try {} catch block just to determine if the item is not found is extremely messy, and what I would consider code smell. Exceptions should occur when an error condition occurs on the current code path, not when an expected response (this response is defined in the known responses of the API, so it is expected) occurs.

I realise changing this behaviour may be considered a breaking change (although I cannot imagine a real world use case for the wrapResponses option, if only 200 status codes are handled) . If I was to submit a PR to address this, would you expect it to be behind a configuration variable of some kind ?

[RicoSuter]

The idea of this setting is to give access to some http level data of the response (ie http headers). The problem is that a method can only have one return type. In your case both responses have the same type so it would be possible to merge them... But with what rule? For me, your sample is fine, i would expect an exception on 404...

Question: What would be the method return type if 200 returns an instance of foo and 404 bar?

How to detect successful vs exception responses?

[graemechristie]

In the case where a 404 was a known response type, I would not expect an exception to be thrown. The response was an expected condition where a resource has not been found, the service advertised that the response should be expected. It's a normal part of the application flow.

Currently we would have to wrap most of our service calls in try/catch blocks. Using exceptions to manage normal control flow is pretty bad practice (not to mention very verbose and inelegant), and I wouldn't ask my team to go down that path.

I'd just like to note that I would only expect that multiple responses codes/types would be supported for Wrapped Responses (or behind another feature flag if you want to leave wrapped responses as they are). For simple interfaces where the return type from the client is the actual DTO object (i.e. wrappedResponses = false) your current logic would be used

Question: What would be the method return type if 200 returns an instance of foo and 404 bar?

The framework we currently use (Autorest) handles this by returning foo if all methods return foo, otherwise returning object and relying on the consumer to cast the result to the correct type. This is not particularly elegent, which is why we (currently) mark our responses that return null as returning foo.

How I suggest this should be handled, is to add a generic TryGetResult(responseCode, out TResult result) method. This would enable handling the response with canonical C# code. In the case where the developer is using the new c#7 out variable handling (if not, you would just need to declare the out variable up front) the client code would look something like:

```C#
var response = await myservice.Foo_GetFooAsync(FooNumber);
If (response.TryGetResult(200, out FooDto fooDto)) {
// Do something with fooDto
} else if (response.StatusCode == "404") {
ShowToast ("Not Found");
}

Obviously you could have any number of If-TryGetResult conditions to handle any other return types your swagger document defines for that method .. however I cannot think of a canonical example where we would do that (but the spec allows it, so we should handle it).

In the case where multiple status codes can return the same type, and you want to handle them in the same block of code, the first argument could be an array:

```C#
var response = await myservice.Foo_GetOrCreateFooAsync(FooNumber);
If (response.TryGetResult(new [200, 201], out FooDto fooDto)) {
      // Do something with fooDto
} else  if (response.StatusCode == "404") {
    ShowToast ("Not Found");
}

How to detect successful vs exception responses?

I would consider any response that is marked as a known response type in the swagger document to be non-exceptional. Where it is "successful" or not is up to the consumer. They can inspect the response objects response code to determine what action to take. Exceptions would be for the case where a result that was not expected was returned (e.g. Internal Server Errors, Not Authorised or Not Found in the case where it was not explicitly defined as a response in the swagger document).

If I was to submit a pull request along the lines of the above strategy would you consider accepting it ? If you did, would you prefer that we put it behind a new feature flag .. or modify the existing "wrap responses" behaviour (possibly with a separate "SuppressExceptionsForKnownRepsonseTypes") flag or similar.

[yvdh]

I have to agree that swagger defined responses should not result in a wrapped error, especially 20X responses.
Luckily the exception still contains the parsed result ...
As it is now I adjust the generated code manually (GASP)

[graemechristie]

@Rsuter do you have any more thoughts on the above ? If you agree in prinicipal that NSwag should support multiple successful response types I can submit a PR for this as per my comment above.

[stijnherreman]

I agree with OP. Consider the following method, with WrapResponses enabled:

```c#
[HttpHead]
[Route("api/foos/{id}")]
[SwaggerResponse(HttpStatusCode.OK, typeof(void))]
[SwaggerResponse(HttpStatusCode.NotFound, typeof(void))]
public async Task Head(string id)
{
var fooExists = await this.fooRepository.Exists(id);

if (fooExists)
{
    return this.Ok();
}
else
{
    return this.NotFound();
}

}

The generated code will contain this:

```c#
if (status_ == "200") 
{
    return new SwaggerResponse(status_, headers_); 
}
else
if (status_ == "404") 
{
    var responseData_ = await response_.Content.ReadAsStringAsync().ConfigureAwait(false); 
    throw new SwaggerException("A server side error occurred.", status_, responseData_, headers_, null);
}
else
if (status_ != "200" && status_ != "204")
{
    var responseData_ = await response_.Content.ReadAsStringAsync().ConfigureAwait(false); 
    throw new SwaggerException("The HTTP status code of the response was not expected (" + (int)response_.StatusCode + ").", status_, responseData_, headers_, null)
}

It should not throw on a 404, because I explicitly specified this as an expected response.

[nickniverson]

I have been replacing an implementation, with C# swagger clients, that returned "null" for 404's. So, this limitation has been a bit of a rabbit hole for me as well. I finally just gave up and returned OK status with a null response instead of a 404 (since I have access to the source code for the original endpoint).

Also, I agree that the response wrapper for success responses is fairly worthless unless you want to inspect the headers. Otherwise, in theory, the status code will always be 200 (or whatever you have configured for your default response status).

However, I would like to offer a suggestion for all features going forward. If you provided a "strategy" (extensibility point) via constructor injection for response handling... then developers could customize the behavior across _all_ clients with a single implementation. Likewise, if you just implement the same interface and provide the "default" implementation, then the overall code generation will be much more flexible/extensible in the long term. Obviously, I could fork the code and make the change myself... but it would be nice if extensibility was more of a first class citizen in regards to response handling. I suspect it would lower your overall work-load with regards to one off requests for behavior changes... i.e. the response for most complaints becomes... "if you don't like the default behavior... implement this interface... "... Anyhow, food for thought.

[imaramos]

Hey guys, is this topic dead?

[stijnherreman]

I think this being an edge case with a workaround available, caused the discussion to die down for now. I think there are lots of other more important things that @RicoSuter is working on :)

So probably the only way to move forward is to come with a full-fledged proposal.

[timothymcgrath]

This is tricky because I don't really want to have to check the StatusCode of every response I make to NSwag. Usually, an Exception is okay for responses like BadRequest.

But, I have a case where a 404 isn't really an exception, it is just null. What if we had the ability to return null for a 404 instead of throwing an ApiException if it is an expected response.

[daniel-white]

I'd appreciate this too. Autorest at least thought about this and added a custom property in the open api doc:

https://github.com/Azure/autorest/blob/master/.attic/proposals/generator-specific-settings/improved-exceptions.md

Looks like if the custom property route would be taken - changes would be here:
https://github.com/RicoSuter/NSwag/blob/master/src/NSwag.CodeGeneration/Models/ResponseModelBase.cs