apple / swift-openapi-generator

Generate Swift client and server code from an OpenAPI document.
https://swiftpackageindex.com/apple/swift-openapi-generator/documentation
Apache License 2.0
1.21k stars 87 forks source link

Handle encoding and decoding errors in a middleware-like extension point #522

Open jpsim opened 3 months ago

jpsim commented 3 months ago

Question

I'd like to add telemetry to my app to report issues where an API response is missing a required key or otherwise has decoding errors. Ideally I could write middleware to achieve this, but from what I've been able to tell, middleware can't intercept decoding errors.

In a perfect world, the server handling the API requests would always return 100% spec-compliant responses, never introducing breaking changes, but mistakes happen.

How would you suggest I add this telemetry to my client application using swift-openapi-*?

czechboy0 commented 3 months ago

Hi @jpsim,

you're right that a middleware won't be the solution here, as transports and middlewares still work with generic HTTP requests and responses, and only the type-safe layer closest to your code turns them into values of your generated types.

If you want to e.g. emit metrics for specifically decoding errors, you can catch ClientError, which, as its underlyingError will have the original DecodingError thrown by JSONDecoder. I'd suggest logging it together with the causeDescription.

So, in code, you could do something like:

@main struct HelloWorldURLSessionClient {
    static func main() async throws {
        let client = Client(serverURL: URL(string: "http://localhost:8080/api")!, transport: URLSessionTransport())
        do {
            let response = try await client.getGreeting()
            print(try response.ok.body.json.message)
        } catch let error as ClientError where error.underlyingError is DecodingError {
            print("The reason for this request failing was the following decoding error: \(error)")
            // Increment your metric here.
            // Rethrow the original error.
            throw error
        }
    }
}
jpsim commented 3 months ago

Yes, I can catch decoding errors at the call site like this, but that will lead to a lot of duplication.

This seems like a perfect fit for middleware conceptually, where other metrics and instrumentation can easily be added for all endpoints without needing the call sites to explicitly handle those cases.

Consider this a feature request to consider having centralized handling of decoding errors 🙏

And thanks for all your efforts in making this library ❤️

czechboy0 commented 3 months ago

Thanks for the kind words 🙂

If we were to add a middleware at the suggested layer, do you have ideas of what the signature could be?

Thinking out-loud here, but if we based it on the raw HTTP one:

protocol Middleware: Sendable {
    func intercept(_ input: Sendable, operationId: String, next: (Sendable) async throws -> Sendable) async throws -> Sendable
}

The input/output values would be of the Operations.MyOperation.{Input,Output} types.

Would that be useful? I wonder if the as? casting you'd need to do in the body of it would actually be useful/acceptable.

Opinions/ideas welcome here, also cc @simonjbeaumont.

jpsim commented 3 months ago

Could we keep the type information by using generic parameters?

func intercept<Input: Sendable, Output: Sendable>(
  input: Input,
  operationId: String,
  next: (Input) async throws -> Output
) async throws -> Output {
  // Middleware could transform or log input, output or errors here
  return try await next(input)
}
czechboy0 commented 3 months ago

The Client type needs to have an array of these middlewares that all run on every request to every operation. But these Input/Output types are per-operation.

Today, we have a property var middlewares: [any ClientMiddleware] on the underlying client. In my less-than-ideal scenario, that would be var middlewares: [any Middleware], for example.

What would that array signature be in your example? How would the Client code iterate through them?

jpsim commented 3 months ago

I think with my suggestion, you could keep var middlewares: [any ClientMiddleware] since the generic parameters are defined on the function, not the protocol, so it doesn't add any generic constraints to the protocol itself.

czechboy0 commented 3 months ago

Sorry, I'm still not sure I understand how it'd actually work. Could you try to open a draft PR to the runtime library with your experimental change, and maybe that'll help me get it? 😇

jpsim commented 3 months ago

Proof of concept here: https://github.com/apple/swift-openapi-runtime/pull/99

I basically just took the shortest path I could find that would allow me to hook into decoding errors in a centralized place. I suspect you may want to go a different direction that may be a better fit conceptually for the project, but I'll leave that to you to decide.

czechboy0 commented 3 months ago

Oh right, if you don't need the Input/Output when handling these errors, then yes we could totally do that.

A few thoughts:

Yeah let's discuss, this is definitely an interesting direction.

jpsim commented 3 months ago

These are no doubt important questions for you to answer as a library author. From my perspective, any answer to those questions would satisfy my need to add instrumentation and alerting to api response decoding errors.

simonjbeaumont commented 3 months ago

Oh right, if you don't need the Input/Output when handling these errors, then yes we could totally do that.

A few thoughts:

  • should this cover encoding errors as well, and errors such as a missing header when it was required?
  • should the method itself throw, to allow you to handle the error and rethrow a different one (or the same one)? Maybe the method should return an error to ensure we always have something to rethrow after the handler returns.
  • should the error be ClientError, which has more useful context, or just the underlying error?

Yeah let's discuss, this is definitely an interesting direction.

Users can already catch and unpack the error today; the complaint wasn't that it wasn't possible, but that it was tedious to wrap every call like this. For this reason, I think that allowing the user to convert to a different error doesn't add much value. Same argument goes for whether this error is wrapped or unwrapped.

IIUC, the request is to instrument this particular error flow to log or collect metrics? In that case, any hook we provide should probably be purely an optional side-effect, so a void function.

I have a couple of questions of my own:

czechboy0 commented 3 months ago

All great points. If we say that this is purely a read-only action (no transforming of errors), then maybe something like this? (Naming to be bikeshed separately)

protocol ClientErrorDelegate: Sendable {
  func requestSerializationDidThrow(_ error: ClientError) async throws
  func transportOrMiddlewareDidThrow(_ error: ClientError) async throws
  func responseDeserializationDidThrow(_ error: ClientError) async throws
}

And then you could optionally provide one of these on the Configuration struct. WDYT?

czechboy0 commented 3 months ago

We'd probably also provide a server variant:

protocol ServerErrorDelegate: Sendable {
  func requestDeserializationDidThrow(_ error: ClientError) async throws
  func transportOrMiddlewareDidThrow(_ error: ClientError) async throws // Not entirely sure how this one would work, TBD
  func responseSerializationDidThrow(_ error: ClientError) async throws
}

// Edit: Actually, if this is Client/Server, we'd probably just add it as another parameter to the Client initializer/registerHandlers call. Configuration only has things that are the same for both today.

jpsim commented 3 months ago

If it's just instrumentation that users are after, should we consider adding "actual" instrumentation to the runtime library, i.e. by depending on Swift Metrics and emitting metrics for these sorts of things with dimensions for the operationID and the kind of error?

  1. Comprehensive metrics would be great, but much broader scope compared to my near-term needs
  2. Ideally comprehensive metrics would be pluggable like ClientTransport currently is to: a. avoid the cost of the swift-metrics dependency for users who don't need it b. allow for alternative metrics implementations

Also I consider fine-grained metrics (number of requests, failure #s, timing data histograms, etc) to be fairly distinct from and very differently actionable compared to decoding errors where a server is returning non-conformant responses. So I can see some benefit in having a unified metrics solution but that's not necessary for this level of instrumentation.

czechboy0 commented 1 month ago

Discussion happening on the runtime PR: https://github.com/apple/swift-openapi-runtime/pull/99