Custom error mapper

[czechboy0]

Motivation

In some cases, the caller doesn't want to explicitly catch the ClientError and extract the underlyingError from it, and just want the underlying error (thrown by the generated code, middleware, or handler). Before this change, that required wrapping all calls to the client with a special closure that'd unwrap the error, but this should be moved into the client itself.

Similarly, on the server, some adopters might not be interested in the extra context in the final ServerError error thrown down the middleware path, and just want the underlying error.

Also drafted more comprehensive error handling docs, which would include this new feature: apple/swift-openapi-generator#733

Modifications

Add an error mapper customization point, one for client errors, one for server.

This allows the following now:

let client = Client(
    serverURL: try Servers.server1.url(), 
    configuration: .init(clientErrorMapper: { clientError in
        // Always throw the underlying error, discard the extra context
        clientError.underlyingError
    }),
    transport: URLSessionTransport()
)

do {
    let response = try await client.greet()
} catch {
    print(error) // this error is now the underlyingError, rather than ClientError
}

Tip: since this PR changes some indentation, it's easier to review with whitespace ignored: https://github.com/apple/swift-openapi-runtime/pull/141/files?w=1

Result

More flexibility when it comes to error handling.

Test Plan

Added a unit test.

[simonjbeaumont]

IIUC, before this PR we map all errors in a way that's internal, and this PR doesn't provide a way of overriding that. Instead, it offers a way of adding another layer of mapping, with an example of how to undo the default mapping.

Assuming I've understood this correctly, how would you feel about inverting the configuration here?

My hunch is that the API is misleading. It takes a parameter clientMapper, which is optional and defaults to nil, which, to me at least, would imply that no mapping would take place.

I wonder if the parameter should still be an optional closure, but be defaulted to the one we provide, which we can then attach API documentation to.

It also means if I override that as a user with nil I get the plain error, untouched at all.

WDYT?

[czechboy0]

That's an interesting idea. It would mean that the closure wouldn't be (ClientError) -> Error anymore, but instead (Error) -> Error, and if the adopter wanted to customize the mapping they wouldn't be able to compose it with our built-in wrapping, only replace it wholesale. Maybe that's okay, but I just wanted to call it out.

It'd also mean that there wouldn't be a single hook for e.g. telemetry and recording e.g. the ClientError.causeDescription - if the adopter wanted to do it, they'd have to go back to wrapping every call to the Client, and do it there.

[simonjbeaumont]

the closure wouldn't be (ClientError) -> Error anymore, but instead (Error) -> Error, and if the adopter wanted to customize the mapping they wouldn't be able to compose it with our built-in wrapping, only replace it wholesale. Maybe that's okay, but I just wanted to call it out.

Right, but if the default error mapper we install was API and clearly documented, why wouldn't they be able to compose on top of it? The one we expose would be (Error) -> Error and users can either (a) leave it alone; (b) disable it by setting it to nil; (c) install their own; or (c.1) install their own that first calls our one and then does some more things?

For folks that are interested in transforming errors to responses we already have support for that in a middleware. This is purely for folks that care about how thrown errors propagate through the stack. We had a way baked in, and IMO the ask is to be able to override it (including opting out), not just to compose on top of our wrapping.

[czechboy0]

if the default error mapper we install was API

This is the part I missed in your first message - yes, if the adopter can compose our default mapper with a custom one, I'm happy for the shape you're proposing.

Can you suggest some starting spelling for this? What would be the default mapper closure/function value attached to? I can then update this PR.

[simonjbeaumont]

Can you suggest some starting spelling for this? What would be the default mapper closure/function value attached to? I can then update this PR.

From first glance, the most logical place to attach this would be Configuration. This type already holds converter-like things that we install defaults for, but support overriding: Configuration.dateTranscoder, .multipartBoundaryGenerator, etc.

We've typically then provided the protocol extension with a self constraint to provide some preconfigured instances for people to use, (e.g. DateTranscoder.iso8601, and MultipartBoundaryGenerator.random).

So something like:

protocol ErrorMapper { /* ... */ }

extension ErrorMapper where Self == DefaultErrorMapper {
    public static var default: Self { DefaultErrorMapper() }
}

struct DefaultErrorMapper { /* ... */ }

struct Configuration {
  // ...

  var errorMapper: (any ErrorMapper)?

  public init(
        dateTranscoder: any DateTranscoder = .iso8601,
        jsonEncodingOptions: JSONEncodingOptions = [.sortedKeys, .prettyPrinted],
        multipartBoundaryGenerator: any MultipartBoundaryGenerator = .random,
        xmlCoder: (any CustomCoder)? = nil,
        errorMapper: (any ErrorMapper)? = .default  // <-- this is new param (with old init moved to deprecated)
    ) { /* ... */ }
}

Following this pattern seems the most discoverable to me since folks will likely head to the Configuration docs to find the things they can configure.

I think the name "default" might benefit from something better, if we can come up with something.

[czechboy0]

And ErrorMapper have a method that's (Error) -> Error, and get the underlying error (not ClientError), right? And the ClientError wrapping would happen in DefaultErrorMapper only?

[simonjbeaumont]

And ErrorMapper have a method that's (Error) -> Error, and get the underlying error (not ClientError), right? And the ClientError wrapping would happen in DefaultErrorMapper only?

Yes; the only protocol requirement would be e.g. func mapError(Error) -> Error.

We then have two choices as to how to enable opting out:

1. Configuration.errorMapper: (any ErrorMapper)? = .default and users pass nil to opt-out.
2. Configuration.errorMapper: any ErrorMapper = .default and provide a .noop mapper too.

[czechboy0]

I'll start with option 2, and we can see how we like it once it's ready.

[czechboy0]

Actually, I don't think it'll work. In the current PR, the ClientError/ServerError is always created by the middleware/transport/handler stack and the additional context is always attached.

It's not easy to move the logic of attaching the extra context into an implementation of a (Error) -> Error function. If we wanted to do that, we'd need to pass all of the extra context into the closure explicitly, and then we could implement the creation of the ClientError as an error mapper. But I'm not sure if that's better.

In the current PR, we always provide the wrapper error for the error mapper to customize/strip down to just the underlying error. That's easy. But flipping that around and adding the context in the error mapper for the default implementation would be harder, and might require rewriting the code that creates the stack in both UniversalClient and UniversalServer.

Please double check that I'm not missing anything obvious here. If not, which path forward would you prefer here?

[simonjbeaumont]

I guess it's this one that needs customising: https://github.com/apple/swift-openapi-runtime/blob/main/Sources/OpenAPIRuntime/Interface/UniversalClient.swift#L101-L108?

So the mapper protocol might look like:

public protocol ErrorMapper {
    func makeError(
        operationID: String,
        input: any Sendable,
        request: HTTPRequest?,
        requestBody: HTTPBody?,
        baseURL: URL?,
        response: HTTPResponse?,
        responseBody: HTTPBody?,
        error: any Error
    ) -> any Error
}

The default would be the one currently defined inline in UniversalClient.send.

Here's an example: main...simonjbeaumont:swift-openapi-runtime:sb/error-mapper-poc

[czechboy0]

Right, we'd need to make the full list of fields we'll ever want to attach to ClientError part of the public API to achieve this, and never change it. If we wanted to attach more diagnostic information in the future (e.g. information about the depth of the middleware stack, capturing the service context at the time of throwing the error, capturing the call stack/file/line, etc), we wouldn't be able to. We'd essentially be freezing ClientError. Do we want that?

[simonjbeaumont]

We'd essentially be freezing ClientError.

Is that true? I don't see how. ClientError is already public API so anything we do to extend it in the future will need to be done in a backwards compatible way, which is fine and doable.

The same would also be true for protocol ErrorMapper, which can gain new requirements, so long as you provide a default implementation, right?

[czechboy0]

Yeah, if we're okay adding a new method to ErrorMapper whenever we want to add a new property to ClientError, it could work.

[hactar]

Wish I would have found this PR and the new documentation earlier.

I'm using the openapi generator via the hummingbird runtime. Hummingbird has HTTPError which conforms to HTTPResponseError - a dev can throw this and hummingbird will gracefully set the correct status code in the response, and output the message parameter in a nice json to a client to parse - if included.

I was hoping to be able to throw HTTPError in my APIProtocol handler, and use hummingbirds functionality to log this error and also send it to the client. But the openapi runtime wraps the error RuntimeError.handlerFailed, with the HTTPError being the underlying error - so Hummingbird doesn't recognize it and only does the default implementation of sending a 500 status code.

I then thought, ok, maybe I should catch my errors within the handler and then return a .serverError or .clientError myself. This is fine for informing my client of the error, but now Hummingbird doesn't know an error was thrown, so any error handling/logging middleware isn't triggered. I could log in my handler, but in the handler I don't have access to RequestContext, so i can't use the context.logger so that things like the request.id are logged automatically for me.

I've now written a custom middleware which catches any ServerError, extracts the underlying error, and rethrows that to restore default Hummingbird functionality. This works.

Based on the new documentation using serverErrorMapper would have been much simpler and would have solved my problem. Glad you are adding that.

Posting this comment as a "user journey", may it be useful in designing and documenting the custom error mapper 😊.

[czechboy0]

Have you seen https://swiftpackageindex.com/apple/swift-openapi-runtime/1.8.0/documentation/openapiruntime/errorhandlingmiddleware

[hactar]

Have you seen https://swiftpackageindex.com/apple/swift-openapi-runtime/1.8.0/documentation/openapiruntime/errorhandlingmiddleware

Nope, did not spot this, thanks.

[czechboy0]

Closing for now, as the root cause is being fixed in #143, and I'm not convinced we need this anymore.