URLError: localized error message and precise error code are lost

[groue]

OpenAPIRuntime makes it impossible to access precise error localized message and codes.

To reproduce:

1. Generate some code from an OpenAPI specification, as documented.
2. Setup a Client with a URLSessionTransport
3. Run the client and make sure an error is reported by turning on the Airplane mode on a device.

Expected: It is possible to grab the original cause of the error, and the original localized message of the underlying URLError. Very precisely, I expect to be able to detect the notConnectedToInternet code of URLError, and confidently display the excellent French message "La connexion Internet semble interrompue." that is embedded in the NSLocalizedDescription key of the URLError. This is very important for a quality user interface. Experience shows that URLError messages are very suitable for display (at least for some specific codes).

// Expected
let clientError: ClientError
if let urlError = clientError.underlyingError as? URLError {
    // 😊 Deal with the raw URLError, without any information loss
}

What happens instead: I get a ClientError whose underlyingError is a RuntimeError (internal to OpenAPIRuntime) that hides the original error in its transportFailed case, and corrupts the localized message in its prettyDescription by prepending English text.

// 😭 I can't possible present any suitable error message to the end user?
// I have no error code, and no localized message.
//
// Prints:
//   Optional("Client error - operationID: ..., underlying error: Transport failed with error: La connexion Internet semble interrompue.")
//   RuntimeError
//   nil
print(clientError.errorDescription)
print(type(of: clientError.underlyingError))
print((clientError.underlyingError as? LocalizedError)?.errorDescription)

---

In summary, please don't hide the raw errors. I opted in for URLSession by providing URLSessionTransport: I expect to be able to get some genuine URLError at some point, along with their high quality information and localized messages, without any information loss.

[simonjbeaumont]

Looking at this, it appears this is coming from the transport-agnostic UniversalClient. As you note all the errors are wrapped.

I'm wondering if we removed one layer of wrapping here¹ then the underlying error would be the URLError, rather than a OpenAPIRuntime.RuntimeError.

@groue is the principal complaint that we taint the localised description or that you cannot get your hands on the underlying error, or both?

Footnotes

1. https://github.com/apple/swift-openapi-runtime/blob/main/Sources/OpenAPIRuntime/Interface/UniversalClient.swift#L139 ↩

[simonjbeaumont]

So, if we remove that layer, like so...

--- a/Sources/OpenAPIRuntime/Interface/UniversalClient.swift
+++ b/Sources/OpenAPIRuntime/Interface/UniversalClient.swift
@@ -130,7 +130,7 @@ public struct UniversalClient: Sendable {
                         operationID: operationID
                     )
                 } mapError: { error in
-                    RuntimeError.transportFailed(error)
+                    error
                 }
             }
             for middleware in middlewares.reversed() {

...then we are able to do this...

struct MockClientTransport: ClientTransport {
    func send(_ request: Request, baseURL: URL, operationID: String) async throws -> Response {
        throw URLError(.notConnectedToInternet)
    }
}

let client = Client(serverURL: URL(string: "not used")!, transport: MockClientTransport())

do {
    _ = try await client.getGreeting(.init())
} catch let error as ClientError {
    if let urlError = error.underlyingError as? URLError {
        print(urlError.code)
        print(urlError.localizedDescription)
    }
}

We'd need to think through the implications of this change, but presumably this is the concrete ask @groue?

[czechboy0]

I wonder if ClientError should have both underlyingError and some kind of causeDescription where we could put e.g. "transportFailed". It's useful information but I agree it probably shouldn't make it impossible to get the original thrown error.

[groue]

Hello @simonjbeaumont, @czechboy0

The main issue is the impossibility to access the raw URLError, yes.

Please excuse me for talking about the "message corruption" in the initial issue. This only happens in internal apis, and I could only observe it indirectly, by printing the error. I should not have presented this as a problem.

It's difficult to me to suggest a refactoring, because I'm not familiar with OpenAPI error types and cases, and their respective goals. In particular, I'm unsure which cases are intended for debugging feedback (in which case the api contract should be weak), and which errors are expected to be used for control flow in the client app (in which case the api contract must be very strong).

I do intend to use the URLError for control flow, and that's why I need a strong api contract that allows me to get it (when it exists).

What I can note, though, is that the ClientError I got had a underlyingError of type RuntimeError. Since RuntimeError is internal, it is only useful via the public protocol it conforms to: LocalizedError and CustomStringConvertible. So basically it only provides some strings, and is thus unsuitable for control flow.

So... I can finally share an idea with you 😇

Please update the documentation of ClientError.underlyingError so that it specifies that errors thrown by the Client's transport, if any, are directly returned by this property, without exception.

That would become the strong contract on which client apps could rely on, in order to write control flow based on ClientError. Some tests would enforce this contract in the long run.

// One can't be more straightforward
let clientError: ClientError

// Thanks to the new documentation of ClientError.underlyingError,
// I'm 100% guaranteed that this is the way to get the URLError
// thrown by URLSession, and perform reliable control flow.
if let urlError = clientError.underlyingError as? URLError {
    // Perform control flow with the raw URLError
} else {
    // Other errors thrown by URLSessionTransport,
    // or by OpenAPI.
}

Disclaimer: I'm not familiar with the error system of the library, so it's likely I forget other important use cases when I'm focusing on my own 😅 It's a frequent bias of new users 😬

Note: It is in the pitch for typed throws in the Swift forums that I learned about errors used for "debugging" (weak api contract) vs. errors used for "control flow" (strong api contract, with respect for source stability in the client applications, and totally in the realm of semantic versioning). I found these concepts useful, that's how I could suggest a documentation update, above. And of course the main course about the philosophy of errors is the enlightening Precise error typing in Swift.

---

EDIT: If my suggestion for updating the documentation of ClientError.underlyingError sounds OK to you, we'd also need to update the documentation (and maybe the runtime) of URLSessionTransport, so that it guarantees that all URLError are rethrown, unmodified, from send(_:baseURL:operationID:).

A possible alternative would be that URLSessionTransport is documented to throw a specific error (let's say URLSessionTransportError) that gives access to the raw URLError (but we'd need to identify (current or future) use cases that URLSessionTransport would like to allow with this extra error type):

let clientError: ClientError

// Fine as well, as long as there exists a documented contract
// enforced by tests in the OpenAPI libraries.
if let transportError = clientError.underlyingError as? URLSessionTransportError,
   let urlError = transportError.underlyingError as? URLError
{
    // Perform control flow with the raw URLError
} else {
    // Other errors thrown by URLSessionTransport,
    // or by OpenAPI.
}

[simonjbeaumont]

Thanks for taking the time to crystallise this @groue.

I think it's a reasonable ask that the ClientError.underlyingError be usable for control flow. I think removing the additional layer of internal OpenAPIRuntime.RuntimeError in OpenAPIRuntime.UniversalClient can help move this forward.

I think it's also reasonable for the documentation for ClientTransport encourage transport implementations to provide the underlying error in a way that can be used for control flow (cf. there being any such internal wrapping within the transport).

Of course, this project cannot make any guarantee that the transport implementation follows this recommendation so adopters would then be in the hands of those packages. The only API contract that the OpenAPIRuntime package could make is to faithfully propagate the underlying error from the transport, which may or may not be useful.

In the case of the URLSessionTransport, we can make that happen, of course.

I haven't formed a strong opinion on whether URLSessionTransport should wrap the error in a public URLSessionTransportError or just push through the URLError, although I'm leaning toward the latter.

What do you think to all this @czechboy0?

[groue]

Thanks for your feedback, @simonjbeaumont. Let's wait for @czechboy0.

Meanwhile:

Of course, this project cannot make any guarantee that the transport implementation follows this recommendation so adopters would then be in the hands of those packages.

I agree. Yet we have a way to strongly encourage transport implementers to be virtuous, with yet another documentation change, in ClientTransport.send(_:body:baseURL:operationID:):

 /// Sends the underlying HTTP request and returns the received HTTP response.
+///
+/// Errors thrown by this method are rethrown, unmodified, by the
+/// `Client` types generated by the Swift OpenAPI Generator. It is
+/// strongly recommended that your implementation throws public
+/// and well-documented errors. This will help developers who use
+/// generated clients perform reliable error handling.

The Implement a custom client transport section could contain a similar note as well.

We can never force people to implement protocol semantics. But we can document them, explain the intent behind them, and reveal what can turn wrong if they are not honored.

For example, we can not force people to implement matching implementations for Hashable and Equatable. Yet the Hashable documentation uses the word must:

Two instances that are equal must feed the same values to Hasher in hash(into:), in the same order.

[czechboy0]

I agree we should keep the original thrown error in underlyingError, but also add a causeDescription, because the wrapping today does add information that we don't want to lose, which explains what was happening when the underlying error was thrown.

Implementation-wise, I suspect we could move RuntimeError's description to be returned by ClientError's hypothetical new causeDescription, and keep underlyingError reserved for errors thrown by subsystems. That means it might be nil in many cases, for example when there's a missing header, the ClientError would only have causeDescription with that information, and a nil underlyingError.

Since this is practically a breaking change, we should align it with a minor bump pre-1.0, possibly even with 0.3.0 going out next week, if the change can be done by early next week.

[simonjbeaumont]

Since this is practically a breaking change, we should align it with a minor bump pre-1.0, possibly even with 0.3.0 going out next week, if the change can be done by early next week.

How is it a breaking change?

The new causeDescription would be an API-additive change in OpenAPIRuntime, along with some documentation changes. The URLSessionTransport change would go from providing an error that cannot be matched at all to one that can—isn't that ostensibly API-additive too?

[czechboy0]

Yes, but for many other cases, underlyingError would become nil, where it was not nil before (since people should move to causeDescription to understand the context of what threw the underlying error).