Automatically throw error if HTTP response represents a failure

[fumoboy007]

Motivation

Hi! I am the developer of the swift-http-error-handling package, which helps with interpreting HTTP responses and handling failures. I am looking to apply some of the concepts from that package to my usage of swift-openapi-generator, but I am running into some issues.

Overview of swift-http-error-handling

A core part of swift-http-error-handling is interpreting an HTTP response as a success or a failure.

Representing an HTTP Application Failure

The package introduces an HTTPApplicationError type to represent a failure. This allows the failure to be propagated and handled using Swift’s standard try-catch error handling mechanism. The error type contains the following information:

• The HTTPResponse value.
• A generically-typed value representing the response body, if desired. Some HTTP servers add additional details about a failure to the response body.
• Whether the failure is transient. This is important for the package’s retry feature.

Interpreting HTTP Responses

The package also extends HTTPResponse to add two methods: throwIfFailed(successStatuses:transientFailureStatuses:) and throwIfFailed(successStatuses:transientFailureStatuses:makeResponseBody:).

The methods have default values for the successStatuses and transientFailureStatuses parameters so that the methods can be called conveniently:

try response.throwIfFailed()

The successStatuses and transientFailureStatuses parameters allow for a customized interpretation if needed:

try response.throwIfFailed(
  successStatuses: [.created],
  transientFailureStatuses: HTTPResponse.Status.transientFailures.union([.conflict])
)

The second throwIfFailed method accepts an async closure to attach a response body to the error:

try await response.throwIfFailed {
  return try await deserializeFailureDetails(from: responseBody)
}

The response body can be accessed later like so:

do {
   try await performRequest()
} catch let error as HTTPApplicationError<MyFailureDetails> {
   let failureDetails = error.responseBody
   doSomething(with: failureDetails)
}

Shortcomings of the OpenAPI-Generated Client

If I wanted to interpret an OpenAPI-generated response as a success or failure, I might do the following:

let response = try await client.getGreeting()
doSomething(with: try response.ok.body.json)

If the HTTP response status was not .ok, then the response.ok property access would throw RuntimeError.unexpectedResponseStatus. This seems like it works for our use case; however, there are a number of issues with this approach.

Unsafe

A developer needs to remember to check the response status. Not checking the response status introduces a subtle bug.

This is obvious for experienced developers who have used Swift HTTP libraries like Foundation or AsyncHTTPClient but may be non-intuitive for other developers. Even experienced developers may forget to check the response status in cases where they do not intend to access the response body.

Inconvenient

Consider the case where a response can have multiple successful statuses (e.g. .ok, .notFound, and .gone for a delete request). The developer would have to do one of two things, both of which are inconvenient:

let response = try await client.deleteSomething()
do {
   _ = try response.ok
} catch {
  do {
     _ = try response.notFound
  } catch {
    _ = try response.gone
  }
}

let response = try await client.deleteSomething()
switch response {
case .ok, .notFound, .gone:
  break

default:
  throw MyCustomError()  // need to define our own error type

Limited Error Handling Ability

The ability to handle such an error is limited by the following:

• The error type is not public.
• The error does not contain the response body.

Duplicate Logic in Middleware

Some middleware also needs to interpret the response as a success or failure (e.g. to retry, to log failures, etc.). Now, the response interpretation logic is duplicated across the caller and the middleware, which is both inconvenient and could cause inconsistency across the two implementations, causing bugs.

Proposed Solution

I propose a breaking change for swift-openapi-generator version 2.0.0 to address all of the current shortcomings.

I propose that the client automatically interprets responses and throws HTTPApplicationError when the response represents a failure.

Since the response deserialization code is inside of the client, the client could deserialize the response body, if any, and attach it to the error automatically. Error handling code would be able to catch the error using its concrete type HTTPApplicationError<Operations.myOperation.Output> or as an instance of HTTPApplicationErrorProtocol if the code does not care about the response body.

The generated client could expose properties to customize the response interpretation. The generated client methods could also have additional parameters to customize the response interpretation per operation.

Finally, it would be important for the client to interpret the response before passing the response to the middleware so that the middleware do not have to do their own response interpretation.

Putting it all together, it would look something like the following:

let client = Client(serverURL: URL(string: "http://localhost:8080/api")!,
                    transport: URLSessionTransport(),
                    // The middleware will handle HTTP application failures using the standard
                    // try-catch mechanism.
                    middlewares: [RetryMiddleware(), LoggingMiddleware()])

do {
   // Safer than before.
   _ = try await client.doSomething()

   // More convenient than before.
   _ = try await client.deleteSomething(successStatuses: [.ok, .notFound, .gone])
} catch let error as any HTTPApplicationErrorProtocol {
  // The error type is public, allowing for more sophisticated error handling.
  doSomething(with: error)
}

Alternatives Considered

Alternative 1: Add middleware that interprets the response

Instead of building the response interpretation into the client directly, swift-openapi-runtime or a third-party library could expose a hypothetical ResponseInterpretationMiddleware that does the response interpretation. Developers would add ResponseInterpretationMiddleware to the list of middleware when creating the client.

The first issue with this alternative is that middleware do not have access to the response deserialization code, so ResponseInterpretationMiddleware would not be able to attach the deserialized response body to the error.

Second, since the ResponseInterpretationMiddleware is optional, other middleware would not be able to rely on its availability and would need to do their own response interpretation, duplicating logic.

Finally, this alternative is less safe compared to the proposed solution since the developer would need to remember to add the ResponseInterpretationMiddleware to the list of middleware.

Alternative 2: Add a property to the generated response that returns the HTTPResponse value

In this alternative, the generated response would have a property to access the HTTPResponse value. The caller could then interpret the response themselves.

This alternative does not solve the safety issue since the developer still needs to remember to interpret the response.

This alternative also does not solve the middleware code duplication issue since the middleware would still need to do their own response interpretation.

Additional Information

If the proposed solution is accepted, feel free to add a dependency to my swift-http-error-handling package to help with implementation. You would gain access to the HTTPApplicationError type, the throwIfFailed methods, the default set of success statuses, and the default set of transient failure statuses.

I promise I won’t do anything sketchy with my package. 😆 But if it makes you feel better, I am also considering creating a proposal to merge my package into the swift-http-types package. Let me know if that would be a prerequisite. If it is, I’ll get right on it!

[czechboy0]

Hi @fumoboy007,

thanks for looking into Swift OpenAPI Generator and providing such a thoughtful proposal. Let me build on some of the points below.

But first, I think it's important to remind ourselves the scope and goals of this project, which can be found here: https://swiftpackageindex.com/apple/swift-openapi-generator/1.2.0/documentation/swift-openapi-generator/project-scope-and-goals

The TL;DR is that this package's one and only goal is to help you call (client) or implement (server) HTTP APIs that are described by an OpenAPI document. This isn't a general purpose HTTP library, like URLSession, AsyncHTTPClient, and others are. We made a decision to only focus on solving for the OpenAPI-generated code, but not apply any assumptions on top.

The most important assumption that we chose to not bake into this tool is interpreting what "success" and "failure" codes are. You provide a great example - when deleting a resource, 404 is considered "success", but if you're fetching a resource, 404 would be considered "failure". APIs come in all shapes and sizes, and we want the adopter to be in control of what success and failure means to them.

That's why the response type is a rich enum, where the response headers and body are the associated type of each case. That means that you have to unwrap the enum (either with a switch statement, or using the convenience throwing getters) to get to the value you're interested in.

A developer needs to remember to check the response status. Not checking the response status introduces a subtle bug.

Yes, that's right - however that's the case in all the libraries that we discuss here.

OpenAPI:

let response = try await client.getGreeting()
try response.ok.body.json // <-- I might forget this

URLSession:

let (_, response) = try await URLSession.shared.data(for: URLRequest(url: URL(string: "https://example.com/info")!))
guard (response as? HTTPURLResponse)?.statusCode == 200 else { throw ... } // <-- I might forget this

Using your library:

let response = try await someLibrary.makeRequest(...)
try response.throwIfFailed() // <-- I might forget this

It seems that they all work the same way - you make a request in one step, you interpret the response in another step. If you forget the second step, all the solutions leave you in the same place, where you might be ignoring an unexpected HTTP response code.

Now, that said, as you pointed out, using ResponseInterpretationMiddleware would actually make this less likely to happen, as you'd only need to remember to add the middleware when creating the client, but then wouldn't have to remember to explicitly check the result for every HTTP call. So if you'd like to optimize for "adopters forgetting to interpret responses", the middleware route seems like the one to use.

Consider the case where a response can have multiple successful statuses (e.g. .ok, .notFound, and .gone for a delete request). The developer would have to do one of two things, both of which are inconvenient:

Yes the first option, the nested do/catch blocks, are very inconvenient, and the throwing getters are not meant to be used when you need to interpret more than one status code. In those cases, use the switch statement, which you have in the second example - can you elaborate on why that version is inconvenient? That's how we designed it, deliberately, so that adopters have to handle every single documented case. That's considered a feature, not a bug 🙂

The error type is not public.

That's intentional, as it allows us to evolve the error type without breaking the API. We also didn't expect adopters to programmatically inspect these errors, they're only meant to be logged - and the string representation of these errors should contain enough information to know what the issue is. If not, please file a separate issue and we'll improve how these errors log, but I don't think they should be made public.

The error does not contain the response body.

Right, because they might be thrown before a response body even arrived. We assumed that if you wanted to log every response body, you'd add a logging middleware. Can you describe the use case of where you'd need to get the response body from the error for something other than logging it (i.e. something that can be done today in the middleware)?

Some middleware also needs to interpret the response as a success or failure (e.g. to retry, to log failures, etc.). Now, the response interpretation logic is duplicated across the caller and the middleware, which is both inconvenient and could cause inconsistency across the two implementations, causing bugs.

I actually find that most middlewares that I've seen don't even look at the status code, or at most log it.

Now, the middlewares that do inspect the status code could take the desired list of success/failure codes as input, and when you're instantiating them, you provide the same input to both, keeping them in sync. But I'd also like to learn more about you use case where there's more than one middleware inspecting the status code, as I generally assumed that only one middleware, such as your proposed ResponseInterpretationMiddleware, would turn any failure codes into a thrown error, so the previous middlewares wouldn't see the response anymore, they'd be handling a throw error from next.

From your proposal, I quite like Alternative 1 - that's the flow we recommend adopters use, and so far I haven't seen realistic use cases that couldn't be handled by it. Again, this project is not meant to be a full-featured, generic HTTP library, it's really just a layer on top of an HTTP library that provides access to your OpenAPI-defined API. So if anything doesn't come from OpenAPI, it's considered out-of-scope by default, unless there's a compelling case made that many users of this library will need to solve the problem anyway - in those cases, we have opted to provide some conveniences.

Let's also keep in mind that this is a problem only if the adopter isn't interpreting the response's headers or body, which is also not nearly as common as actually looking at the response to extract header values or bodies. And for those cases, I think the middleware would be a good fallback, in case someone forgets to interpret the response.

To summarize, I think your library can work well when used with general-purpose HTTP clients, such as URLSession, AsyncHTTPClient, where you work with raw HTTP requests and responses, so your extensions on those types can be very useful.

However, in clients generated by this project, you don't work with raw HTTP requests and responses, you work with types generated just for you from your OpenAPI document. Raw HTTP requests and responses are only used in middleware and transport implementations, however once the ecosystem of popular transports and middlewares gets large enough, I don't expect most people to need to write new middlewares and transports, as most common use cases will already have a solution they can just pull in (logging, metrics, tracing, retrying, authentication, even success/failure interpretation). Meaning that most adopters won't ever encounter raw HTTP request and responses through this library.

Hope this addresses at least some of the concerns you brought up, but in a way, the generated code from OpenAPI is an alternative way to interpret HTTP responses to what your library provides, which is why I'm not yet convinced that combining the two provides enough value to justify the cost.

Also tagging @simonjbeaumont who might have more thoughts as well.

[simonjbeaumont]

thanks for looking into Swift OpenAPI Generator and providing such a thoughtful proposal.

First off, let me echo this. Thanks for such a considered and thorough writeup. And, more specifically, I'm very interested in whether we can make things more ergonomic and safer by default.

OpenAPI:

let response = try await client.getGreeting()
try response.ok.body.json // <-- I might forget this

URLSession:

let (_, response) = try await URLSession.shared.data(for: URLRequest(url: URL(string: "https://example.com/info")!))
guard (response as? HTTPURLResponse)?.statusCode == 200 else { throw ... } // <-- I might forget this

Using your library:

let response = try await someLibrary.makeRequest(...)
try response.throwIfFailed() // <-- I might forget this

It seems that they all work the same way - you make a request in one step, you interpret the response in another step. If you forget the second step, all the solutions leave you in the same place, where you might be ignoring an unexpected HTTP response code.

I think this is a good observation but IIUC the point is that we should add this to generated code by default precisely so adopters don't forget.

Do I understand correctly that the proposal would have a predefined set of acceptable status codes indicating success? If so, for a given operation defined in the OpenAPI document, how would we determine the appropriate set, given the discussion about the pervasive non-standard use of HTTP response codes in APIs.

Now, that said, as you pointed out, using ResponseInterpretationMiddleware would actually make this less likely to happen, as you'd only need to remember to add the middleware when creating the client, but then wouldn't have to remember to explicitly check the result for every HTTP call. So if you'd like to optimize for "adopters forgetting to interpret responses", the middleware route seems like the one to use.

I quite like this idea too, however it still relies on the end user to make a decision on what's an error and what isn't. And, given this might be different for different operations then the convenience of a middleware starts to diminish, and if you start trying to use different middlewares for different operations, you're not gaining much over using the switch-based APIs.

OpenAPI is very expressive and people can (and do) use all sorts of patterns in their OpenAPI document, which makes me skeptical that we can come up with a solution to this problem that generalises well, unlike in e.g. GRPC where success and error have stronger conventions.

[fumoboy007]

The TL;DR is that this package's one and only goal is to help you call (client) or implement (server) HTTP APIs that are described by an OpenAPI document. This isn't a general purpose HTTP library, like URLSession, AsyncHTTPClient, and others are. We made a decision to only focus on solving for the OpenAPI-generated code, but not apply any assumptions on top.

Indeed! I was hoping the problems I listed could be solved without changing the current design of the library; however, as I’ve detailed, I don’t think the alternatives are as good as the proposed solution.

---

The most important assumption that we chose to not bake into this tool is interpreting what "success" and "failure" codes are. You provide a great example - when deleting a resource, 404 is considered "success", but if you're fetching a resource, 404 would be considered "failure". APIs come in all shapes and sizes, and we want the adopter to be in control of what success and failure means to them.

Do I understand correctly that the proposal would have a predefined set of acceptable status codes indicating success? If so, for a given operation defined in the OpenAPI document, how would we determine the appropriate set, given the discussion about the pervasive non-standard use of HTTP response codes in APIs.

I proposed a default set of success statuses (2XX) and transient failure statuses (408, 429, 500, 502, 503, 504) for convenience in the common case. However, the proposal also mentions easy ways to provide a custom set of statuses both at the client level and at the operation level.

---

A developer needs to remember to check the response status. Not checking the response status introduces a subtle bug.

Yes, that's right - however that's the case in all the libraries that we discuss here.

[…]

It seems that they all work the same way - you make a request in one step, you interpret the response in another step. If you forget the second step, all the solutions leave you in the same place, where you might be ignoring an unexpected HTTP response code.

I think this is a good observation but IIUC the point is that we should add this to generated code by default precisely so adopters don't forget.

Right! The OpenAPI generator is well-positioned to solve this problem because it is at a higher layer than a general-purpose HTTP library.

---

Now, that said, as you pointed out, using ResponseInterpretationMiddleware would actually make this less likely to happen, as you'd only need to remember to add the middleware when creating the client, but then wouldn't have to remember to explicitly check the result for every HTTP call. So if you'd like to optimize for "adopters forgetting to interpret responses", the middleware route seems like the one to use.

The middleware solution would make it less likely to happen but the proposed solution avoids the issue completely.

I quite like this idea too, however it still relies on the end user to make a decision on what's an error and what isn't. And, given this might be different for different operations then the convenience of a middleware starts to diminish, and if you start trying to use different middlewares for different operations, you're not gaining much over using the switch-based APIs.

Ah yes, that is another disadvantage of the middleware solution: it would be inconvenient to customize the response interpretation per operation.

---

Consider the case where a response can have multiple successful statuses (e.g. .ok, .notFound, and .gone for a delete request). The developer would have to do one of two things, both of which are inconvenient:

Yes the first option, the nested do/catch blocks, are very inconvenient, and the throwing getters are not meant to be used when you need to interpret more than one status code. In those cases, use the switch statement, which you have in the second example - can you elaborate on why that version is inconvenient? That's how we designed it, deliberately, so that adopters have to handle every single documented case. That's considered a feature, not a bug 🙂

The second example is less convenient than the proposed solution because the developer needs to create their own error type.

---

The error type is not public.

That's intentional, as it allows us to evolve the error type without breaking the API. We also didn't expect adopters to programmatically inspect these errors, they're only meant to be logged - and the string representation of these errors should contain enough information to know what the issue is. If not, please file a separate issue and we'll improve how these errors log, but I don't think they should be made public.

The error does not contain the response body.

Right, because they might be thrown before a response body even arrived. We assumed that if you wanted to log every response body, you'd add a logging middleware. Can you describe the use case of where you'd need to get the response body from the error for something other than logging it (i.e. something that can be done today in the middleware)?

An app may want to do more than just log a failure. Here’s an example from RFC 7807:

For example, consider a response that indicates that the client's account doesn't have enough credit. The 403 Forbidden status code might be deemed most appropriate to use, as it will inform HTTP- generic software (such as client libraries, caches, and proxies) of the general semantics of the response.

However, that doesn't give the API client enough information about why the request was forbidden, the applicable account balance, or how to correct the problem. If these details are included in the response body in a machine-readable format, the client can treat it appropriately; for example, triggering a transfer of more credit into the account.

---

I actually find that most middlewares that I've seen don't even look at the status code, or at most log it.

I haven’t put much thought into it but at the very least, a retry middleware and a logging middleware might look at the status code.

But I'd also like to learn more about you use case where there's more than one middleware inspecting the status code, as I generally assumed that only one middleware, such as your proposed ResponseInterpretationMiddleware, would turn any failure codes into a thrown error, so the previous middlewares wouldn't see the response anymore, they'd be handling a throw error from next.

As I mentioned, the response interpretation middleware wouldn’t relieve other middleware from having to do their own response interpretation because they wouldn’t be able to rely on the response interpretation middleware being available. We could only avoid doing response interpretation in middleware if the client itself does the response interpretation.

---

Again, this project is not meant to be a full-featured, generic HTTP library, it's really just a layer on top of an HTTP library that provides access to your OpenAPI-defined API. So if anything doesn't come from OpenAPI, it's considered out-of-scope by default, unless there's a compelling case made that many users of this library will need to solve the problem anyway - in those cases, we have opted to provide some conveniences.

Indeed! I am trying to make the case that we should provide some conveniences for this problem, which all users need to solve.

[czechboy0]

Okay, while I'm not sure how it'd integrate today and about some of the concerns I brought up, I'm happy to keep this issue open and let others chime in over time.