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<TResult>(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:

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:

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:

[HttpHead]
[Route("api/foos/{id}")]
[SwaggerResponse(HttpStatusCode.OK, typeof(void))]
[SwaggerResponse(HttpStatusCode.NotFound, typeof(void))]
public async Task<IHttpActionResult> 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:

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.

[emanuel-v-r]

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

[0xorial]

@RicoSuter

How to detect successful vs exception responses?

like this:

Backend:

public class CreatedUser { public string Id {get;set;} }
public class CreateUserError {public string Message {get;set;} }

[ProducesResponseType(typeof(CreatedUser), (int)HttpStatusCode.OK)]
[ProducesResponseType(typeof(CreateUserError), (int)HttpStatusCode.BadRequest)]
[ProducesResponseType(typeof(void), (int)HttpStatusCode.NotFound)]
[HttpPost]
public async Task CreateUser(){}

Frontend (generated):

type CreatedUser = {id: string}
type CreateUserError = {message: string}
type CreateMemberResponse = 
  | {statusCode: 200; result: CreatedUser} 
  | {statusCode: 400; result: CreateUserError}
  | {statusCode: 404}

 createUserSafe(): Promise<CreateMemberResponse> {}
// possibly add a version where we don't mind throwing an exception
 createUser(): Promise<CreatedUser>

This will force clients to check status codes. For example:

const result = (await api.createUser()).result; // compile error: result does not exist on {statusCode: 404}
const response = api.createUser();
if (response.statusCode !== 404){
  console.log(response.result.id); //  compile error: id does not exist on CreateUserError
} 
if (response.statusCode === 404) {
} else if (response.statusCode === 200){
   console.log(response.result.id); // OK
   console.error(response.result.message); // compile error: message does not exist on CreatedUser 
} else {
  console.error(response.result.message); // OK
}

[RicoSuter]

Returning object and relaying on casting is really bad in my opinion.

So if this is mainly about 404 we could add a config specifying that 404 should not throw but return null for example?

Everything on top of that is probably huge pile of work and/or leads to a leaky HTTP abstraction which i wanted to avoid...

We could also do some object/cast style based extensions together with the WrapResponses config..

Currently I just dont have the time to do all this in my free time and I also need to ensure to not break any existing users with these changes.

[mdarefull]

I don't understand why so much overthinking around it. We should wrap api calls around try/catch either way and act accordingly (or are you assuming an API call will never fail due to other issues) It is no effort for me to provide different strategies in the catch portion based on what happened... actually that's a good practice... would you do the same if the network failed than if the server (500) failed?

Personally, 90% of the scenario I've run myself into a 404 is not part of a user-flow... I mean, you are querying an object, usually by id... how did the user get a hand on an unexistent id in the first place?? My POST/PUT APIs perform validations, so 400 is expected. My API could fail to access the DB, so a 500 is expected... My APIs are protected so 401 and 403 are also expected... Does it mean we will expect a normal return for every possible scenario and then an exception would only be if there was a network error? I don't think so...

4** are error states. I'm OK with treating them as exceptions... I would be OK if NSwag didn't throw an exception but instead returned a "Result" object, with an isSuccess bool property (like other APIs do) because I don't like exceptions to handle API responses... but it's not a big deal... If an 'object is not found' is not an error for your use-case, then do not return a client-error status code (404) :)

[0xorial]

My take on the workaround

export async function wrapApiException<TReturn>(
    cb: () => Promise<void>,
    statuses: { [statusCode: number]: (result: string, response: any) => void }
): Promise<void> {
    try {
        await cb();
    } catch (e) {
        if (ApiException.isApiException(e)) {
            const errorHandler = statuses[e.status];
            if (errorHandler) {
                errorHandler(e.response, e.result);
            } else {
                throw e;
            }
        }
    }
}

usage:

await wrapApiException(
    async () => {
        const response = await api.member_Login({userName, password} as LoginUserDto);
        recordLogin(response);
    },
    {
        400: (result: string, response: LoginError) => {
            if (response === LoginError.IncorrectUserPassword) {
                toast.warn(t.login.error.passwordIncorrect());
            }
            setLoginError(response);
        },
    }
);

leaky HTTP abstraction

Tell me about leaky abstractions in web-world... :( I wish there was a way around it, but by now it seems to have become an integral part of REST: https://restfulapi.net/http-status-codes/ (I wish I wasn't forced to use this buzzword in 80% of projects, but I have to)

[skironDotNet]

@graemechristie you raising a problem that has been raised many times, but for some reason @RicoSuter makes big deal out of it and I saw this recent "fix" for a case of 404 + empty body (null) and I don't understand why to throw

To me this is as simple as if not found, then why would have any body, thus null is natural. If we work in C# LINQ var user = db.users.Where(id == 0); //user is null here no exception then in client code var user = nswagClient.GetUser(0); //I would expect null here, why exception? having null we can just do null check and drive front end code

And with whole respect to the author (to whom we should send big BTC tips) and the all contributors, when we talk about code smell, here is the art I can't understand

Are we exception or success? or are we happy we successfully thrown an exception. Can some one explain the intent?

I think the road we are going to take is we are going to fork the code and 404 case to our architecture.

I think ideally NSwag config could accept some dictionary for HttpCode + OutputResult so developers could "configure" what the output should look like, for those who want null, be able to return null, for those who want exception, return exception. While Swagger 2.0 specification can't provide all the details, we could have more configurable NSwag generator. I know it's lots of work + easier said than done. If I only had time to participate I would...

Again thanks to all who participate

[skironDotNet]

@mdarefull

90% of the scenario I've run myself into a 404 is not part of a user-flow Yes for smaller apps that's true, but I can give you an example where it is part of BL flow. We have a form of polyglot architecture. Multiple systems connected together, where some are proprietary with own APIs and we build own tools around them. So 404 valid scenario

• Per REST API we return 404 not found on not found data
• User portal A makes an entry to system B, but does not have all needed data (BL rule) for user portal C (system B is neural)
• A user goes to system C (connected to sys B) and loads an account with missing contact info caused by A, here we experience 404, and we need to do null check and show UI with empty fields to force user of C to fill up missing data or user won't be able to save by BL of portal C.
• Throwing an error by nswag client means we would have to try catch but there are other calls made in async way, hard to explain, but having null object can be detected after all calls return, while try cach would have to be done for every call, or we need to decouple that one call that we know could return 404.

So while 404 is not part of a user-flow it actually is. If you would hard delete this issue from github and next day try to load this URL you would see 404, isn't that part of user flow to let you know it no longer exists. Would you crash the app, or should user friendly page? Throwing an exception means crashing the app, mostly not wanted flow.

[mdarefull]

@skironDotNet: I honestly cannot understand why there's so much fuss about this 404 issue... We are discussing as if trying/catching an API call for errors were the end of the world. Do you never check for issues on your API calls? What if the API didn't respond in time? what if there was a network issue? what if the server faulted for whatever reason? I use catchException on all of my API calls, then inside the body, I determine how to handle each different type of exception based on response code. I don't understand why it is so big of a deal to have the check for 404 => handle appropriately in the catchException Vs. in the map() or subscribe() body.

Per HTTP spec 4** are error responses, so there's nothing wrong with treating them as errors. Would there be scenarios where a 400 is expected? Of course! I might not be able to validate everything client-side... that doesn't make it less of an error.

[skironDotNet]

@graemechristie you can override the result with Templates. I recall this being mentioned by someone long time ago but I didn't know where the Templates are and what they are, so here is info https://github.com/RicoSuter/NJsonSchema/wiki/Templates

Copy this template to your custom location \NSwag\src\NSwag.CodeGeneration.CSharp\Templates\Client.Class.ProcessResponse.liquid

replace line 27 with return null;

Configure UI to use your template dir with one modified file (in my example I point directly to NSwag source but that's just to play)

And we have now 404 with return null

I don't know if this answer your question, but for myself I wish I found that solution earlier.

@RicoSuter thank you. I didn't know it's so easy

[skironDotNet]

@colbroth The templates are written in liquid (at least for c#), you can figure it out, and adjust to for condition (using abstract code here not actual values)

if request.verb == "PUT" and response.status == 204 return ""; else //here existing code from template to process other cases as is endif

[kornakar]

@skironDotNet: I honestly cannot understand why there's so much fuss about this 404 issue... We are discussing as if trying/catching an API call for errors were the end of the world. Do you never check for issues on your API calls? What if the API didn't respond in time? what if there was a network issue? what if the server faulted for whatever reason? I use catchException on all of my API calls, then inside the body, I determine how to handle each different type of exception based on response code. I don't understand why it is so big of a deal to have the check for 404 => handle appropriately in the catchException Vs. in the map() or subscribe() body.

Per HTTP spec 4** are error responses, so there's nothing wrong with treating them as errors. Would there be scenarios where a 400 is expected? Of course! I might not be able to validate everything client-side... that doesn't make it less of an error.

One reason for me is that even if I catch all the exceptions, when I run the C# client in Azure, it still logs the exceptions and shows them as errors. Then all I see is dozens of errors even when it's just an API returning "not found" as it should. And it makes it really hard to differentiate them from actual errors.

[kornakar]

Client.Class.ProcessResponse.liquid

I can't find a file named Client.Class.ProcessResponse.liquid in the repo. Is it an older commit version you're using?

[skironDotNet]

@kornakar here it is https://github.com/RicoSuter/NSwag/blob/master/src/NSwag.CodeGeneration.CSharp/Templates/Client.Class.ProcessResponse.liquid

[matohoza]

Hi guys, I just stumbled over this issue and I just must add my 2 cents to it. For me this is a matter of bad API design. We are talking about web APIs, not web page serving endpoints, right? If the API want's to say that the resource is not there, but (server side) this is not considered to be a bad thing, it can return an empty response 204. But if the API considers it being an error-like situation, that the resource is not there, it may send back a 404 which is an error then.

Another example why (or when) 404 should throw errors: URL: host/parentResourceId/childResourceId

• in this case I am requesting the child, but if parentResourceId is not found, then I consider 404 to be appropriate
• if parentResourceId is found, but childResourceId is not, then it is still worth a consideration, if it is a 204 rather than 404. This might differ from one endpoint to another.

Wrapped up: the current behavior of status code handling perfectly fits my needs.

[jonnybi]

Hi,

I totally agree with @matohoza here. I have a minor issue with this whole thing:

if (status_ == 404)
{
    var objectResponse_ = await ReadObjectResponseAsync<ProblemDetails>(response_, headers_, cancellationToken).ConfigureAwait(false);
    if (objectResponse_.Object == null)
    {
        throw new ApiException("Response was null which was not expected.", status_, objectResponse_.Text, headers_, null);
    }
    throw new ApiException<ProblemDetails>("A server side error occurred.", status_, objectResponse_.Text, headers_, objectResponse_.Object, null);
}

All 4xx responses should be seen as client side errors with means: come on client, send me a proper request!

I'm fine with the exception but i'd love to see better messages

throw new ApiException<ProblemDetails>("Client sent an invalid request.".... (or any better text)

and "A server side error occurred" on 5xx.

I know custom templates can help, but there is a tradeoff: it's easier to keep your nugets updated, then your custom templates. I also know you can add xml comments to get custom reponse descriptions but this is a maintainability nightmare too. This should be default in client generations.

[CarvellWakeman]

In my opinion, the main reason to support inject-able behavior to the response handling pipeline of an auto generated nswag client is because this is clearly a grey area. Despite what some have said above, there is no distinctly correct way to handle different responses from the wide variety of APIs out there, and even if there was, there is no guarantee you're calling an API implemented "properly".

In a lot of cases you are beholden to a vendor's API response structure. Maybe your system needs to behave differently when an entity does not exist in the consumed API... but the consumed API returns a 500 and text/HTML when something doesn't exist (🤮), because it's garbage. Sure, this is an argument not to build garbage APIs, but those of you who think that have never worked with a mixture of legacy and modern systems where you can't just rewrite 20 year old production systems because you don't like them.

Extensibility is important, and so are best practices. You can distill best practices (like avoiding leaky http abstractions) in your default implementation, and guide users towards it with documentation, but it's nice not to cut them off from what you consider a hacky implementation when it is required.

My personal need for this extensibility is consuming 3rd party vendor APIs where "error" responses are a valid part of the workflow of the resource being operated on. Yeah, I can litter try/catches everywhere in our application layer, but that's dumb and messy. Much more convenient to provide a general implementation of error handling for all responses and keep your application layer free of vendor or client package specific implementation details, like whether an error appears as an exception, a null, or some other type by shoving that response handling into a structure between the API call in the nswag client and the application layer that just needs to know if the operation succeeded or not. Ideally, that response handler is a structure I implement, injected into the nswag client and called from it. It could just as well be a layer on top of the nswag client interfaces, but that defeats a lot of the convenience of nswag.

[mattiaskagstrom]

https://github.com/RicoSuter/NSwag/blob/master/src/NSwag.CodeGeneration.CSharp/CSharpClientGeneratorSettings.cs

The settings file for C# code generation is missing the TemplateDirectory. It's also not used when setting up the defaulttemplatefactory.

[biqas]

Hello, I'm late to the party ;)

@RicoSuter I have read all comments and I was surprised that no one is really calling out what the fundamental problem here is and then proposing based on the fundamental problem a solution.

So, what is the fundamental problem/issue here. You are requesting something, and you get a result out of different options and options is an important term here. What are the result options?! We must divide into two parts, because we are talking about an external service/code (no ownership) and internal service/code (partially ownership in this case generated).

External: Here we can expect results which can be grouped into

• Successful with and without data,
• Unsuccessful with and without data.

Internal:

• External result with or without data
• Internal exception based on processing or other things

For the consuming part it is the combination of those result options (internal and external). Considering this, would lead to one possible solution to return result options and not returning a single value and the rest through exceptions.

Example:

public Taks<OneOf<bool, NotFound, BadRequest, ProblemHttpResult>> SomeApiCall()
{
...
  if (status == 200)
  {
    if (jsonConvertError)
    {
      throw new ApiException();
    }
    return true;
  }
  else if (status == 400)
  {
    return new BadRequest();
  }
  if (status == 404)
  {
    return new NotFound();
  }
...
}

For consumer there is not broken data flow and the exception path is really an exception!

Possible library: OneOf: https://github.com/mcintyre321/OneOf

[lindeberg]

I'm having an endpoint with ETag support, where a successful response code is both 200 and 304. Really weird to try catch that.

[skironDotNet]

I'm having an endpoint with ETag support, where a successful response code is both 200 and 304. Really weird to try catch that.

Just use NSwag templates, search for 200 and add 200 or 304 as success check and you'll be good.

That's how we fixed 404 to return null instead of throwing an exception

[scooter12]

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.

Same here, only for 401s. Took hours of debugging to even conceptualize that the generated client wasn't throwing because it received something unexpected, rather it was throwing as per its design.

I think this is just a case of semantics. An exception is something unexpected. An "Exception" is data structure object that describes what was unexpected. The act of "throwing" an Exception, says, "not only did something unexpected happen, its so bad I don't know what to do from here, so YOU try to fix it".

So at least IMHO, the generated client should never throw an exception for documented response codes, unless the response itself is somehow malformed.