Improved enriched error propagation from the transport and middlewares

[czechboy0]

Motivation

Fixes apple/swift-openapi-generator#302 and apple/swift-openapi-generator#17.

The issue was that we hid away errors thrown in transports and middlewares, and the adopter would get ClientError where the underlyingError wasn't the error thrown by the underlying transport/middleware, but instead a private wrapper of ours.

Modifications

Make sure {Client,Server}Error.underlyingError contains the error thrown from the underlying transport/middleware when that was the cause of the error, otherwise keep RuntimeError there as was the behavior until now.

Also added a causeDescription property on both public error types to allow communicating the context for the underlying error.

Also made sure middleware errors are now wrapped in Client/ServerError, they weren't before so didn't contain the context necessary to debug issues well.

Result

Adopters can now extract the errors thrown e.g. by URLSession from our public error types using the underlyingError property and understand the context of where it was thrown by checking the user-facing causeDescription.

Also, adopters now get enriched errors thrown from middlewares.

Test Plan

Wrote unit tests for both UniversalClient and UniversalServer, inevitably found some minor bugs there as well, fixed them all, plus the unit tests now verify the behavior new in this PR.

[czechboy0]

cc @groue @MahdiBM

[MahdiBM]

It appears to get the full description of the error we'll need to do something like String(describing: error) instead of just a string interpolation.
That seems to be a trend as PostgresNIO already does that to make sure not to leak database info, but I was hoping for the latter in this library since i'm not sure if any secret info will be leaked by just showing a connection error to users mistakenly when you when string-interpolating the error.

Having it this way makes debugging decently harder, specially for less-experienced folks.

Overall I'm fine with that as we've already moved to using String(reflecting:) for errors in most places, but I'm still not sure if it's the correct way for this library.

[czechboy0]

Hmm I'm not sure I understand your concern. Can you elaborate on what you're trying to do, how the library makes that difficult, and what you'd like to see instead?

This PR is about allowing you to programmatically get the unmodified underlyingError. Is that not what you needed?

[MahdiBM]

Sort of, but not exactly.

Now that i'm testing it, it actually doesn't solve my problem for the most part at all.

Given this code:

let error: any Error = RuntimeError.transportFailed(
    DecodingError.dataCorrupted(.init(codingPath: [], debugDescription: "description!"))
)

print(error) // Transport threw an error.
print(String(describing: error)) // Transport threw an error.
logger.log("Got error", metadata: ["error": "\(error)"]) // Transport threw an error.
logger.log("Got error", metadata: ["error": "\(String(reflecting: error))"]) // Transport threw an error.

You see? It's not very debuggable. This is my exact problem.
One could catch the error and print it, but IMO it's not right to expect an explicit catch at all times just to be able to debug.

Solution 1:
Add CustomDebugStringConvertible to the error types, and let it include everything.
String(reflecting: error) will use that, so result could look like:

print(error) // Transport threw an error.
print(String(describing: error)) // Transport threw an error: THE UNDERLYING ERR
logger.log("Got error", metadata: ["error": "\(error)"]) // Transport threw an error.
logger.log("Got error", metadata: ["error": "\(String(reflecting: error))"]) // Transport threw an error: THE UNDERLYING ERR

Solution 2:
Make the CustomStringConvertible contain the full error, in which case all the above print/logs will contain the full error.

As I explained above, I think solution 2 is the better way, but i'm also fine with solution 1.

[MahdiBM]

Another note. I notice some (edited) Error types still conform to LocalizedError but don't provide the whole point of the conformance which is the errorDescription variable, unlike other Error types.
Something should probably change. Either removal of the conformance, or providing the errorDescription. I think the latter would be what you want considering other Error types do that.

[czechboy0]

RuntimeError is an internal type, so what you showed above is never what you'll get. You'll get a ClientError, where the causeDescription is "Transport threw an error." and the underlyingError is the actual error threw by the transport. When you print the ClientError, it should contain a summary of everything it includes, but you can always grab the underlyingError yourself and print or cast it to a concrete type and inspect if further.

Can you check out this branch and show, by using a generated Client and a mock ClientTransport that throws a custom error, how any part of the error is missing when you print it?

[czechboy0]

Also note that because ClientError and ServerError are public types, you can create a completely custom way to print them, all their properties that store what's printed by description is public.

[MahdiBM]

RuntimeError is an internal type ... You'll get a ClientError

That's actually great, sorry that i failed to remember/notice that. If what users always get is a ClientError, then this PR absolutely solves the problem I was having, considering you've added the underlyingError to ClientError's description.

[czechboy0]

RuntimeError is an internal type ... You'll get a ClientError

That's actually great, sorry that i failed to remember/notice that. If what users always get is a ClientError, then this PR absolutely solves the problem I was having, considering you've added the underlyingError to ClientError's description.

Cool - yeah and this PR also fixes apple/swift-openapi-generator#17 as previously, you wouldn't get a ClientError when a middleware threw, but the raw underlying error. This PR makes it all consistent, the generated client should always throw a ClientError, and the underlyingError either has the error thrown by the middleware/transport, or it has the internal runtime error which is only useful for logging. But in general, for logging, I recommend logging the full ClientError as its description contains both the context of where the error was thrown, plus the error itself. We only expect users who need to programmatically inspect the underlyingError to actually access that property.

[czechboy0]

The API breakage CI pipeline is red but it's a bug in diagnose-api-breaking-changes, I believe, and filed as rdar://117520415. So we can consider the CI green.