Failure on returnResultAsyncIfNeeded without optional completion handler causes assertion failure

[richardtop]

Bug report

Link to the source:

apollo-ios/Sources/Apollo/DispatchQueue+Optional.swift

Line 30 in 4e2a495

assertionFailure("Encountered failure result, but no completion handler was defined to handle it: \(error)")

A completion handler is defined as optional: action: ((Result<T, Error>) -> Void)?,, yet if not included, causes the program to crash with assertionFailure in case of a failure of the Result.

Based on the API, the lack of the completionHandler should not affect program execution in any way (hence, it's optional), and that's why I believe, this behavior is incompatible with the API declaration.

I suggest completely removing the assertionFailure and replacing it with debug logging message.

Versions

Please fill in the versions you're currently using:

• apollo-ios SDK version: 0.49.1
• Xcode version: 13.1
• Swift version: 5.5
• Package manager: Any

Steps to reproduce

Try calling static func returnResultAsyncIfNeeded<T>(on callbackQueue: DispatchQueue?, action: ((Result<T, Error>) -> Void)?, result: Result<T, Error>) { with the result that returns failure, on a debug build.
File: https://github.com/apollographql/apollo-ios/blob/main/Sources/Apollo/DispatchQueue%2BOptional.swift

[designatednerd]

Ooh, good catch. Looking at the history I'm not quite clear why that was added, I've got a question in to the person who added it to double check the intention.

[richardtop]

Sounds good. It's ok to have it in internal methods, but there is also some other public API method which calls this eventually. Both are with an optional callback, triggering assertion failure if it has completed unsuccessfully.

[designatednerd]

Yeah, after talking with the original author it seems like the better option is to turn this into a debugPrint.