Get icon indicating copy to clipboard operation
Get copied to clipboard

send(_:) method always tries to decode empty response since newest version

Open Pomanks opened this issue 3 years ago • 5 comments

Hi 👋🏻

When declaring a method with an optional response body (see example), the call only results in a decoding error because the method tries to decode an empty response.

Optional response body method:

func getWatching(slug: String, extended info: ExtendedInfo?) async throws -> Response<Users.GetWatching.Response?>

Error:

dataCorrupted(Swift.DecodingError.Context(
    codingPath: [], 
    debugDescription: "The given data was not valid JSON.", 
    underlyingError: Optional(Error Domain=NSCocoaErrorDomain Code=3840 "Unable to parse empty data." UserInfo={NSDebugDescription=Unable to parse empty data.})
    )
)

The same code worked in previous version though (it simply returned nil).

Pomanks avatar Sep 12 '22 17:09 Pomanks

The optional variant was removed in v1.0 to avoid "polluting" the API – this feature is rarely needed. I think it should be possible to implement it without introducing new public methods. It's something worth exploring. PRs are welcome.

kean avatar Sep 12 '22 21:09 kean

A potential solution would be to treat the empty response as an error, even if the status code is not… The implementation is not public and only located in the delegate:

    func client(_ client: APIClient, validateResponse response: HTTPURLResponse, data: Data, task: URLSessionTask) throws {
        guard (200..<300).contains(response.statusCode) else {
            throw APIError.unacceptableStatusCode(response.statusCode)
        }
        if response.statusCode == 204, data.isEmpty {
            throw APIError.emptyResponse(response.statusCode)
        }
    }

It would then be up to the developer to handle such a case.

I understand the rarity of this response but I feel it's something than needs to be available up front instead of something handled like an issue (which it isn't).

What are your thoughts?

Pomanks avatar Sep 13 '22 10:09 Pomanks

That's a good idea. It will allow the developers to cover this scenario. I think that's what Alamofire does by default as well.

But I'm not sure throwing an error is an ideal option. It'll be nice to allow developers to use optionals and consider this scenario a successful completion. I was thinking something like this:

protocol OptionalProtocol {}

extension Optional: OptionalProtocol {}

func decodeResponse<T: Decodable>(_ type: T.Type) {
    print(type is OptionalProtocol.Type)
}

kean avatar Sep 13 '22 11:09 kean

It's not indeed, I just didn't see any other option at the time 😅 I get it now.

I'll make a PR 👍🏻

Pomanks avatar Sep 13 '22 12:09 Pomanks

Here you go: Pull Request #58

Pomanks avatar Sep 14 '22 08:09 Pomanks

Released in 2.1.0. Thanks for your contribution, @Pomanks!

kean avatar Sep 17 '22 18:09 kean