HTTP responses don't have information from body on non-success status

[adamjones1]

When the response of a call doesn't have a success status code, the call throws an HttpRequestException with no context for what went wrong beyond the actual status code. EnsureSuccessStatusCode is called and the content of the response is lost.

In some (most?) OpenAPI schemas, the shape of the response for non-success status codes is also documented and a schema is provided for each media type, just as it is for success codes. SwaggerProvider should see if any are present, and if so generate custom exceptions which can be thrown containing the deserialised body when such statuses are returned.

This will greatly help with diagnosing errors that come from a service, when eg. they provide further info than just "bad request" (what about my request was bad?).

[sergey-tihon]

Can you please share the sample?

I saw description for not-successful status codes, so yes it should be possible to provide meaningful error messages bases on status code.

But why would you want to have strongly typed (deserialized) body of failed http request?

[sergey-tihon]

Released 0.10.0-beta11 throw custom OpenApiException instead of general HttpRequestException (in case of error) and also contains error description from schema

[adamjones1]

Was this applied to the SwaggerClientProvider as well as the OpenApiClientProvider? I still seem to be getting an HttpRequestException thrown. I tried migrating to OpenApiClientProvider instead but it wouldn't compile with the spec I was using - it complained that a key of the "definitions" node didn't adhere to some alphanumeric/dot/underscore regex but I believe this is only a requirement of the v3 OpenAPI spec, not of the v2 (at least I can't see it referenced).

Using the SwaggerClientProvider though, I did note that there was an OpenApiException class generated. Its shape didn't appear to be what I was after though: it had just an integer Status Code and a string Description (which I'm presuming is just the textual description of the status code, like "BadRequest", "Unauthorised" etc?).

I was hoping for something where if eg. I have the following spec:

{
  "swagger": "2.0",
  "info": {
    "version": "v1",
    "title": " ",
    "description": " "
  },
  "host": "myserver.com",
  "basePath": "/",
  "schemes": ["https"],
  "paths": {
    "/SomeEndpoint": {
      "get": {
        "tags": [],
        "summary": "an operation",
        "operationId": "MyOperation",
        "consumes": ["application/json"],
        "produces": [],
        "parameters": [],
        "responses": {
          "200": {
            "description": "Success",
            "schema": {
              "description": "a success object",
              "type": "object",
              "properties": {
                "Value1": { "type": "integer" },
                "Value2": { "type": "string" }
              }
            }
          },
          "400": {
            "description": "Bad Request",
            "schema": {
              "description": "invalid request response",
              "type": "object",
              "properties": {
                "ValidationMessages": { 
                  "type": "array", 
                  "items": { "type": "string" } 
                }
              }
            }
          },
          "403": {
            "description": "Forbidden",
            "schema": {
              "description": "forbidden request response",
              "type": "object",
              "properties": {
                "ErrorMessage": { "type": "string" }
              }
            }
          }
        }
      }
    }
  }
}

then I would have the following exceptions generated:

exception MyOperation400 of ValidationMessages: string list
exception MyOperation403 of ErrorMessage: string

which would each throw when I get their respective status back, and I'd then be able to read the structured error data from the response. In my use case, it's much easier for me to diagnose a 400 if I can log what the validation errors were that went wrong, and also my program can respond differently to the 400 if it can consume the extra metadata about the failure that's returned.

[sergey-tihon]

Was this applied to the SwaggerClientProvider as well as the OpenApiClientProvider? No, I did it only for OpenApiClientProvider and do not really plan to port back new features to SwaggerClientProvider.

OpenApiClientProvider should throw instance of OpenApiException (not generated, just class) https://github.com/fsprojects/SwaggerProvider/blob/master/src/SwaggerProvider.Runtime/ProvidedApiClientBase.fs#L9-L12

it complained that a key of the "definitions" node didn't adhere to some alphanumeric/dot/underscore regex but I believe this is only a requirement of the v3 OpenAPI spec

if I got it correctly, it looks like by design in OpenAPI.NET =( https://github.com/microsoft/OpenAPI.NET/issues/198#issuecomment-390818839

Also I pushed version 0.10.0-beta12 with the null ref fix when schema does not contain Components schema https://github.com/fsprojects/SwaggerProvider/commit/4019b602853de8b2234e1712762b8bc7f7a71478

It is possible in your case to update names in schema? It is manually created or generated by tool like Swashbuckle?

[adamjones1]

if I got it correctly, it looks like by design in OpenAPI.NET =( microsoft/OpenAPI.NET#198 (comment)

Urgh, that's annoying.

It is possible in your case to update names in schema? It is manually created or generated by tool like Swashbuckle?

The spec in question is out of my control but I've requested its owners if they can make a workaround due to this issue. I'll let you know the progress on that.

OpenApiClientProvider should throw instance of OpenApiException (not generated, just class)

Yep, would it be possible to change this to be as described above, generated based off the spec? I'm not sure how much work this is but it would certainly be valuable. At the moment the OpenApiException only gives programmatic access to the info already present in the HttpRequestException anyway. My preference would strongly be for a generated typed response body on the exception but if that's too much, even just having the raw body as a string field on the exception would be a step up which allows me to present more useful diagnostics in logs/to users.

[sergey-tihon]

I asked about the schema because if it generated by Swashbuckle there is new version for aspnetcore https://github.com/domaindrivendev/Swashbuckle.AspNetCore where MS helped to migrate to OpenApi.NET. So they use the same object model and always generate compliant schemas (v2 and v3).

Yep, would it be possible to change this to be as described above, generated based off the spec?

Let's think together, I am not sure that it is best option.

If I throw new type of exception for every error of every method your code will crazy because you will have to catch all of them (and somehow find them, maybe even open schema and take a look how errors are documented). It may be hard to do and lead to ugly code.

One exception type does not give you strongly types access to error payload, but you need to handle only one case "call not succeeded". If you plan to just log error information, string even better, you will not need serialize payload back.

[adamjones1]

Yeah, probably a preferred and more functional way would be to have a union type returned like

type MyOperationResult = 
    | Success of MyOperationSuccessSchema
    | NotFound of MyOperationNotFoundSchema
    | BadRequest of MyOperationBadRequestSchema
    | ... etc

but alas, provided union types can't be done 😢 (https://github.com/fsharp/fslang-suggestions/issues/154).

You could get around the issues above with a tweak to the multiple exception route though - for the issue of trying to catch them all, they could all inherit from OpenApiException as it's implemented now. Then clients who don't care about the response body or distinguishing between different codes can just catch OpenApiException, and clients who only care for a subset of them can do eg.

try
    client.MyOperation(...)
with
| :? MyOperationNotFoundException as ex -> failwithf "Not found: %A" ex.Body
| :? OpenApiException as ex -> failwith "response code wasn't success" // catch all other non-success status codes (BadRequest etc)

To solve the discoverability issue, could these custom exception types all be nested in a hierarchy (since we can now open static classes)?

public static class Exceptions
{
    public static class ForMyOperation
    {
        public class MyOperationNotFoundException : OpenApiException { ... }
        public class MyOperationBadRequestException : OpenApiException { ... }
        ...
    }
    public static class ForSomeOtherOperation
    {
        public class SomeOtherOperationInternalServerErrorException : OpenApiException { ... }
        public class SomeOtherOperationForbiddenException : OpenApiException { ... }
        ...
    }
    ...
}

(writing in C# as nested types aren't in F#, but I assume it's still possible to generate them via type providers?)

[NickDarvey]

I understand this doesn't help with getting the error as defined in the schema, but if you want a workaround to at least get the body when it throws, you can write some middleware like this:

exception private UnreliableApiException of HttpStatusCode * string

type ErrorRaisingHandler (inner) =
    inherit DelegatingHandler (inner)

    // https://wizardsofsmart.wordpress.com/2014/05/13/how-to-use-base-sendasync-in-f-delegatinghandler/
    member private x.SendAsync' (request, cancellationToken) = base.SendAsync(request, cancellationToken)

    override this.SendAsync (request, cancellationToken) = Async.StartAsTask <| async {
        let! result = Async.AwaitTask <| this.SendAsync' (request, cancellationToken)
        if result.IsSuccessStatusCode then return result else
        let! body =  Async.AwaitTask <| result.Content.ReadAsStringAsync ()
        raise <| UnreliableApiException (result.StatusCode, body)
        return result
    }

and use it like:

let http = HttpClientHandler () :> HttpMessageHandler |> ErrorRaisingHandler |> HttpClient
let client = OpenApiClientProvider<"my-schema.json">.Client(http)

(Or you could implement an ErrorLoggingHandler which accepts a delegate on construction if you just want to log.)

[adamjones1]

@NickDarvey Yeah true...it does feel a bit hacky to me though and I don't feel great about it, sneaking an exception in the handler pipeline to divert control flow and circumvent the logic in the provided wrapper. (Although I'm not aware of much else besides status code handling that this dodges at the moment, in general over time what else might it inadvertently circumvent, if more post-processing options are added in SwaggerProvider?)

Still yep thanks, it's a good workaround in the meantime until support is added in SwaggerProvider itself.

[Thorium]

System.Net.Http.HttpRequestException seems to be still a problem for SwaggerProvider.

I went for mix of Nick's answer and LoggingHandler.

let unSuccessStatusCode = new Event<_>() // id, status, content
type CustomErrorHandler(messageHandler) =
    inherit DelegatingHandler(messageHandler)
    override __.SendAsync(request, cancellationToken) =
        let resp = base.SendAsync(request, cancellationToken)
        async {
            let! result = resp |> Async.AwaitTask
            if not result.IsSuccessStatusCode then
                let! cont = result.Content.ReadAsStringAsync() |> Async.AwaitTask
                let hasId, idvals = request.Headers.TryGetValues("RequestId") // Some unique id
                unSuccessStatusCode.Trigger(
                    (if not hasId then None else idvals |> Seq.tryHead),
                    result.StatusCode,
                    cont)
            return result
        } |> Async.StartAsTask

...and then just parse the content with FSharp.Data.JsonProvider

[abelbraaksma]

@sergey-tihon, Hawaii (which is not a TP, but instead a code generator) solves the issue with returning a Result instead of raising an error.

This seems to make more sense in general, as quite a few apis use 404 for stuff like “customer not found” after a query and being able to inspect result and have a concrete object in the Error type would make this much simpler than injecting an extra response handler.

Not sure how hard it would be to do, but adding tryXXX methods for each of the operations would be rather brilliant to have.

(I’m writing this after spending a day or so trying to find where the body of the error went with OpenApiProvider, and this thread is not the most discoverable ;). Love the TP though, but this would be awesome to have!)