Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a hook for intercepting decoding errors #99

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jpsim
Copy link

@jpsim jpsim commented Feb 4, 2024

Motivation

It's useful for clients to be able to have a centralized way to handle decoding errors, which can be useful for logging or telemetry.

See apple/swift-openapi-generator#522 for more discussion on this.

Modifications

Add a new DecodingErrorHandler protocol which has a single willThrow(_ error: any Error) requirement.

A decoding handler can be passed to the client configuration on initialization.

This is just a proof-of-concept, if this direction is reasonable, it should be fleshed out to handle all decoding paths.

Result

With this change, errors produced by body decoders can be intercepted in a centralized way for all methods going through the Client.

Test Plan

I added a simple unit test. More tests would need to be added to sufficiently cover the functionality being introduced here.

This is just a proof-of-concept, if this direction is reasonable, it
should be fleshed out to handle all decoding paths.

See apple/swift-openapi-generator#522 for
motivation.
Copy link
Collaborator

@czechboy0 czechboy0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great overall, apologies for the delay on this PR, it somehow slipped through unnoticed.

@@ -96,6 +96,10 @@ extension JSONDecoder.DateDecodingStrategy {
}
}

public protocol DecodingErrorHandler: Sendable {
func willThrow(_ error: any Error)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you be interested in the context this was thrown in? See ClientError and ServerError, where we add more context.

@@ -113,9 +119,11 @@ public struct Configuration: Sendable {
/// - multipartBoundaryGenerator: The generator to use when creating mutlipart bodies.
public init(
dateTranscoder: any DateTranscoder = .iso8601,
multipartBoundaryGenerator: any MultipartBoundaryGenerator = .random
multipartBoundaryGenerator: any MultipartBoundaryGenerator = .random,
decodingErrorHandler: (any DecodingErrorHandler)? = nil
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, please just add a deprecated 2-parameter version of this initializer in Deprecated.swift, as adding a param is technically a source break. Similar to #102 (comment)

@czechboy0
Copy link
Collaborator

Hi @jpsim - are you still interested in a solution here? I'm happy with either you picking this up again, or letting us take over. It'd just be good to finalize the details with a concrete use case in mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants