Add support for global error handling

[baywet]

Currently the http core implementations do no try to parse error responses. Those could contain useful information to convey to the consumer.
You can already define responses in open API for HTTP status codes that correspond to errors (4XX, 5XX), however defining those for each endpoint in Microsoft Graph would be fastidious.
Instead we should make use of another feature in openAPI called overlays. This feature is not yet implemented in the openAPI.net lib but it'd allow defining global responses that apply to all the endpoints if not already defined locally.

• get the schema that's defined in overlays
• use that information for the API client (to register the error type)
• modify http core to call deserialization on error response bodies with that type and throw
  AB#10401

[darrelmiller]

Kiota wouldn't need to process the Overlay directly. That overlay code would be implemented somewhere else.

[baywet]

how would the HTTP library know which type to deserialize the error to then?

[darrelmiller]

The Overlay would be processed before Kiota started processing the OpenAPI.

[baywet]

So kiota would see responses with schemas on different response codes. But how would it map the successful response codes with a specific bad response code schéma? Especially if the schema for 5xx differs from 4xx for some reason?

[darrelmiller]

defining those for each endpoint in Microsoft Graph would be fastidious.
Microsoft Graph is fine because we autogen the OpenAPI

For people writing OpenAPI by hand, this feels like an OpenAPI problem, not a Kiota problem. I feel like we should close this issue as this is not where this problem should be solved.

[baywet]

So if we say that we need an error description for each endpoint, we still have work to do in Kiota/design decisions to make.
Let's assume I have one type described for 4XX, one type for 403, and one type for 5XX for the same operation, how should we pass that to the http request adapter?

Having global error handling (things that apply to all endpoints) would have allowed us to pass those type registrations once for the client/request adapter initialization.
Allowing them to vary per operation means that kiota must pass a map of error code/error type factory to the send method for each operation.

It's possible but means two things:

• Breaking change on most of the interfaces (I think we're ok with that at this point)
• Significant generated code result increase (wanted to double check we were ok with this)

Thoughts @darrelmiller?

[baywet]

Also, should we assume any type described in the error responses can derive from Exception/Error and throw?

[jchannon]

This is a tricky one from my point of view working with strongly typed languages however if a path returns 200 and a model and a possiblte 400 with another model the api client has to return one of those types. The only thing I can think of, off the top of my head is using the Go approach and have a model from the client that is something like:

public class ResponseModel
{
  public SuccessModel Model{get;set;}
  public ErrorModel ErrorModel{get;set;}
}

The types of the Model/ErrorModel would have to match the openapi schema defined models and the developer would have to check if the error model is populated before continuing using the response but it could work

[baywet]

Thanks for jumping in here, this is an interesting suggestion. Couple of follow up questions:

• Why would you return instead of throwing? (or returning an error as last return value in Go)
• What would you do with multiple error types being documented for the same operation in your suggestion? (one for 403 let's say, one for 404, one for 500, etc...)

[jchannon]

1. Because this is not an exception, I know we want to wrap the HTTP calls but the developer/client needs to handle the different responses whether that's a 200/400/500, if the developer can continue with the information from the server/openapi model then cool, if they decide they want to throw they can but this lib should not automatically force that.

2)You can only every get one response whether thats 400/403/404/500 so assuming the openapi doc has those in it and a defined schema for each possible response status code then you return that model in the generated classes and the developer has to make a decision on what to do if the error model is populated

[baywet]

extending on your example with this information we'd have

public class ResponseModel
{
  public SuccessModel Model{get;set;}
  public ErrorModel400 ErrorModel400{get;set;}
  public ErrorModel403 ErrorModel403{get;set;}
  public ErrorModel404 ErrorModel404{get;set;}
  public ErrorModel500 ErrorModel500{get;set;}
}

Which means the SDK user would have to do something like

var myResult = await client.Something.SomethingElse.GetAsync();
if (myresul.SuccessModel != null) {
 // continue normally
} else if (myresult.ErrorModel400 != null) {
 // error handling for 400
} else if (myresult.ErrorModel403 != null) {
 // error handling for 403
} else if (myresult.ErrorModel404 != null) {
 // error handling for 404
} else if (myresult.ErrorModel500 != null) {
 // error handling for 500
} else {
 // I'm not sure what to do but my static analysis tool told me that I'm not handling all cases
}

Which I don't believe is a great experience over throwing exceptions

try {
   var myResult = await client.Something.SomethingElse.GetAsync();
   // continue normally
} catch (ErrorModel400 ex) {
 // error handling for 400
} catch (ErrorModel403 ex) {
 // error handling for 403
} catch (ErrorModel404 ex) {
 // error handling for 404
} catch (ErrorModel500 ex) {
 // error handling for 500
}
// I can add a finally block if I really need to, but if I don't, the static analysis tool won't complain

Which would also allow me to do the following

try {
   var myResult = await client.Something.SomethingElse.GetAsync();
   // continue normally
} catch (Exception ex) {
 // global error handling, like logging
}

Or even just

var myResult = await client.Something.SomethingElse.GetAsync();
// continue normally

Assuming I have some cross-cutting concern reusable implementation at a higher-level with things like APM (application insights), global error display (ASP.NET error handlers), logging, or retry-handling policies (Polly etc). Which would work across the different path items for statically typed languages.

Thoughts?

[jchannon]

Ah good point, I was having in my head run time deserializing to one model but that's not going to work is it for C# 😂😂

And so you would end up with what you propose which is pretty grim however after more thought I think it's explicit. Even if you used exceptions to control flow you're still going to have to do the same if blocks in the exception to check innerexception of have multiple exception types for each model so you have a try and catch with numerous exception types. Whilst perhaps not very aesthetic I think it conveys what the open api spec states and is explicit and readable just not very pretty :)

[baywet]

One assumption I forgot to communicate in my previous response, each of the types derive from Exception (ErrorModel400, ErrorModel403, ErrorModel403, ErrorModel500). That's an assumption we're making for types referenced in responses >399 < 600.
The tricky part is: what if the the same model is referenced in both an error response and a successful one?
Also another question this brings: what do we do about default responses? assume they are success or errors?

[jchannon]

Default, as in they've not defined a schema response? I'd assume success

Doesn't matter if they reference the same model does it? The SuccessModel will be set to an instance of T and if it's a 400 response the ErrorModel will be an instance of T

[baywet]

default as default in the response section of the OpenAPI description instead of 200, 400, or 404.

sure, but they'll derive from Error/Exception which will include additional behavior depending on the target language/platform.

[darrelmiller]

@baywet default is just evil. Let's worry about how we deal with that later.

Your example of having different models for different status codes is a good scenario.

This example is a bit extreme, but all of the different responses are viable.

openapi: 3.0.3
info:
  title: Responses with different models
  version: 1.0.0
paths:
  /: 
    get:
      responses:
        '2XX':
           description: Success
           content:
             application/json:
               schema:
                  $ref: '#/components/schemas/MySpecialEntity'
        '202':
           description: Status monitor
           content:
             application/json:
               schema:
                  $ref: '#/components/schemas/StatusMonitor'
        '300':
           description: Download links
           content:
             application/json:
               schema:
                  $ref: '#/components/schemas/DownloadLinks'
        '4XX': 
           description: Client Error
           content:
             application/json:
               schema:
                  $ref: '#/components/schemas/ClientFailureInfo'
        '5XX': 
           description: Server Error
           content:
             application/json:
               schema:
                  $ref: '#/components/schemas/ServerFailureInfo'

I guess we either pass a map to core, or core returns a stream and we emit code in the service library to do the deserialization. Whatever happened to the plan of passing a codegen'd response handler to core?

[baywet]

If your question is "how do we pass the response type and constructor /factory to the request adapter?" the code generator passes those to the send method. And yes we could pass a map of error codes and factories there too.
What about the error type deriving from exception?

[darrelmiller]

I don't have a problem with the error type deriving from exception. We can throw one of multiple exceptions depending on the status code. That still leaves us with the issue of different types for 200, 202 & 300.

What I meant was, what if instead of using the default implementation of IResponseHandler that is defined in core, we actually generated a response handler that mapped the status codes to code that deserialized the correct type.

I suppose the problem that still remains is the request executor method still can only return one type unless we create a wrapper type. If we have to create a wrapper type anyway, we might as well pass the map to core and avoid doing the codegen.

[baywet]

Thanks for all the additional information and the open discussion, the next step will be to update the conversion library so we have an accurate description for Microsoft Graph.

[baywet]

as an additional precision to the "spec", if the request adapter received an error code [400, 599], and if it doesn't find an exact match in the mapping (e.g. 403), or a class match (eg 4XX), it should just throw an exception with an error code and ignore the body.

[baywet]

@jchannon FYI I've put together a PR #1100